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

2019-10-04 Thread Laszlo Ersek 
On 09/26/19 14:43, Laszlo Ersek wrote:
> On 09/23/19 17:59, Philippe Mathieu-Daudé wrote:
>> 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: , 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.
> 
> Right, there's been some movement in Mantis#2018.
> 
> It looks like the spec is wrong, but all [*] consumers and producers of
> GetEdid(), investigated thus far, have simply ignored the mistake in the
> spec, and done the right thing in practice.
> 
> So there seems to be a chance for the spec to be fixed. That would be
> followed by fixing the GetEdid() prototype in edk2. And then this patch
> would be dropped.
> 
> [*] the only exception found thus far is the call site in
> edk2-platform's "DisplayLinkPkg", mentioned above in the commit message.
> However, that is a very recent code addition (commit 9df63499ea01,
> 2019-09-09), and it might not reflect "historical" usage, but an attempt
> to write brand new code, conforming to the *letter* of the spec. So in
> case the spec gets fixed, the DisplayLinkPkg code could be fixed in
> tandem, perhaps.

See new thread started here:

985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com">http://mid.mail-archive.com/985de369-7880-b6cc-46e7-5a2edca6582b@redhat.com
https://edk2.groups.io/g/devel/message/48487

Thanks!
Laszlo

>>> Cc: Ard Biesheuvel 
>>> Cc: David Woodhouse 
>>> Cc: Jordan Justen 
>>> Signed-off-by: Laszlo Ersek 
>>> ---
>>>
>>> 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,
>>>   ,
>>>   ,
>>>   (UINT8 **) 
>>>
>>
>>
>>
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48488): https://edk2.groups.io/g/devel/message/48488
Mute This Topic: https://groups.io/mt/34180226/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-09-26 Thread Laszlo Ersek 
On 09/23/19 17:59, Philippe Mathieu-Daudé wrote:
> 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: , 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.

Right, there's been some movement in Mantis#2018.

It looks like the spec is wrong, but all [*] consumers and producers of
GetEdid(), investigated thus far, have simply ignored the mistake in the
spec, and done the right thing in practice.

So there seems to be a chance for the spec to be fixed. That would be
followed by fixing the GetEdid() prototype in edk2. And then this patch
would be dropped.

[*] the only exception found thus far is the call site in
edk2-platform's "DisplayLinkPkg", mentioned above in the commit message.
However, that is a very recent code addition (commit 9df63499ea01,
2019-09-09), and it might not reflect "historical" usage, but an attempt
to write brand new code, conforming to the *letter* of the spec. So in
case the spec gets fixed, the DisplayLinkPkg code could be fixed in
tandem, perhaps.

Thanks
Laszlo

> 
>> Cc: Ard Biesheuvel 
>> Cc: David Woodhouse 
>> Cc: Jordan Justen 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>
>> 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,
>>   ,
>>   ,
>>   (UINT8 **) 
>>
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48103): https://edk2.groups.io/g/devel/message/48103
Mute This Topic: https://groups.io/mt/34180226/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-09-23 Thread Philippe Mathieu-Daudé 
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: , 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 
> Cc: David Woodhouse 
> Cc: Jordan Justen 
> Signed-off-by: Laszlo Ersek 
> ---
> 
> 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,
>   ,
>   ,
>   (UINT8 **) 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47857): https://edk2.groups.io/g/devel/message/47857
Mute This Topic: https://groups.io/mt/34180226/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

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

2019-09-17 Thread Laszlo Ersek 
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: , in the
direction that matches this patch.)

Cc: Ard Biesheuvel 
Cc: David Woodhouse 
Cc: Jordan Justen 
Signed-off-by: Laszlo Ersek 
---

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,
  ,
  ,
  (UINT8 **) 
-- 
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47412): https://edk2.groups.io/g/devel/message/47412
Mute This Topic: https://groups.io/mt/34180226/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-