Re: [edk2-devel] [PATCH 25/35] OvmfPkg/VideoDxe: document EFI_EDID_OVERRIDE_PROTOCOL.GetEdid() call

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Laszlo,

On 9/17/19 9:49 PM, Laszlo Ersek wrote:
> According to the UEFI spec -- and to the edk2 header
> "MdePkg/Include/Protocol/EdidOverride.h" too --,
> EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID takes an (EFI_HANDLE*), and not an
> EFI_HANDLE, as second parameter ("ChildHandle").
> 
> This is probably [*] a bug in the UEFI spec. Given that this CSM module
> (VideoDxe) had been used for a long time on physical platforms before it
> was moved to OvmfPkg, keep the current "ChildHandle" argument, just cast
> it explicitly.
> 
> [*] The edk2 tree contains no other GetEdid() call, and also no GetEdid()
>     implementation.
> 
>     The edk2-platforms tree contains two GetEdid() calls, at commit
>     022c212167e0, in files
>     - "Drivers/DisplayLink/DisplayLinkPkg/DisplayLinkGop/Edid.c",
>     - "Drivers/OptionRomPkg/CirrusLogic5430Dxe/Edid.c".
> 
>     From these, the first passes an (EFI_HANDLE*) as "ChildHandle", the
>     second passes an EFI_HANDLE. It's difficult to draw a conclusion. :/
> 
> No functional changes.
> 
> (I've also requested a non-normative (informative) clarification for the
> UEFI spec: <https://mantis.uefi.org/mantis/view.php?id=2018>, in the
> direction that matches this patch.)

(EFI_HANDLE*) makes sense to me, but I'd rather wait for the spec
clarification before Acking this patch, I don't want we silent a bug
with this cast.

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     build-tested only
> 
>  OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c b/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
> index 0640656dba14..995136adee27 100644
> --- a/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
> +++ b/OvmfPkg/Csm/BiosThunk/VideoDxe/BiosVideo.c
> @@ -1402,9 +1402,13 @@ BiosVideoCheckForVbe (
>        goto Done;
>      }
>  
> +    //
> +    // Cast "ChildHandle" to (EFI_HANDLE*) in order to work around the spec bug
> +    // in UEFI v2.8, reported as Mantis#2018.
> +    //
>      Status = EdidOverride->GetEdid (
>                               EdidOverride,
> -                             BiosVideoPrivate->Handle,
> +                             (EFI_HANDLE *) BiosVideoPrivate->Handle,
>                               &EdidAttributes,
>                               &EdidOverrideDataSize,
>                               (UINT8 **) &EdidOverrideDataBlock
>