From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web09.11419.1639666341704764122 for ; Thu, 16 Dec 2021 06:52:22 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=pjfysuw4; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 00E76B82472 for ; Thu, 16 Dec 2021 14:52:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 433C2C36AE8 for ; Thu, 16 Dec 2021 14:52:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1639666337; bh=dJNznDPBZHEzM8EG7EjUzMiXfWs949QDkymORlqltkg=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=pjfysuw48jxK2EkDBOFnZMRw1+koNqEO1eITGrKLr/CA2cFZFj3JEqmyc91r23H50 Y6UAdQ6rGK4RAIx3/9LbXT46xYYwOikvMmA42hZrsr9LQL6nBSpGYhjNrbFGdDk2DO 6cCkZAN0AYn57347fJoTA6n9EkEIQ3p6AoMDIpnCFeB5tictS3z9e7qoA27/VDLYP5 taNNcFpE/tOkmH0wXMwz0BirYoJY7quFAiRYloDuUWjLa6gi7Qv9NMXyOVTEY6GmDV cOd3ZGEcuq+qalhAfC4XN40U/jJDDRE/SkP+f4ohs4q6FX/ECJMbrFSrrO4Z0ldPsQ 6zrTP1/UvVWHQ== Received: by mail-wm1-f48.google.com with SMTP id z206so4858197wmc.1 for ; Thu, 16 Dec 2021 06:52:17 -0800 (PST) X-Gm-Message-State: AOAM5326xygAuK5Pt4uNhgMMYlJ0QPi3cYRvfs4Z2AHzEOGDEAJGmvIY kupx70UViFN1R+2iqEukePWUiGYfUzc2rGr8TJ8= X-Google-Smtp-Source: ABdhPJw2+RoOwodgndq7Jq/5MBG5otbVomtdWvcXicxqEv7fIVO9MGIG9vhX67OKnB1VCr7hDCWGFlEQlPlq3pkJ61g= X-Received: by 2002:a05:600c:4ed2:: with SMTP id g18mr5344329wmq.25.1639666335491; Thu, 16 Dec 2021 06:52:15 -0800 (PST) MIME-Version: 1.0 References: <20211216142959.1998191-1-kraxel@redhat.com> In-Reply-To: <20211216142959.1998191-1-kraxel@redhat.com> From: "Ard Biesheuvel" Date: Thu, 16 Dec 2021 15:52:03 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 0/5] OvmfPkg/QemuVideoDxe: pick up display resolution settings from the host To: edk2-devel-groups-io , Gerd Hoffmann Cc: Erdem Aktas , Julien Grall , Brijesh Singh , Pawel Polawski , Min Xu , Jiewen Yao , Tom Lendacky , Ard Biesheuvel , Jordan Justen , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , James Bottomley , Anthony Perard Content-Type: text/plain; charset="UTF-8" On Thu, 16 Dec 2021 at 15:30, Gerd Hoffmann wrote: > > See last patch in the series for details. > > Gerd Hoffmann (5): > OvmfPkg: add PcdVideoResolutionSource > OvmfPkg/QemuVideoDxe: simplify InitializeBochsGraphicsMode > OvmfPkg/QemuVideoDxe: drop QEMU_VIDEO_BOCHS_MODES->ColorDepth > OvmfPkg/QemuVideoDxe: factor out QemuVideoBochsAddMode > OvmfPkg/QemuVideoDxe: parse edid blob, detect display resolution > This looks fine to me in principle, the only question I have about is the use of a PCD PcdVideoResolutionSource to keep track of what the height/width PCDs mean. I don't think this is very idiomatic for EDK2, and I wonder if it would be better to use [fixed] PCDs for build time configuration, and perhaps expose the other width/height data (and the associated policy of what takes precedence over what) via a protocol instead. In general, I am not too keen on overusing PCDs for any of this, as it does not have the implied dependency ordering that PPIs or protocols have. Jiewen, any thoughts? > OvmfPkg/OvmfPkg.dec | 7 + > OvmfPkg/AmdSev/AmdSevX64.dsc | 1 + > OvmfPkg/Microvm/MicrovmX64.dsc | 1 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/OvmfXen.dsc | 1 + > OvmfPkg/PlatformDxe/Platform.inf | 1 + > OvmfPkg/QemuVideoDxe/QemuVideoDxe.inf | 3 + > OvmfPkg/QemuVideoDxe/Qemu.h | 6 +- > OvmfPkg/PlatformDxe/Platform.c | 3 + > OvmfPkg/QemuVideoDxe/Driver.c | 14 +- > OvmfPkg/QemuVideoDxe/Gop.c | 2 +- > OvmfPkg/QemuVideoDxe/Initialize.c | 251 +++++++++++++++++++------- > 14 files changed, 213 insertions(+), 80 deletions(-) > > -- > 2.33.1 > > > > > >