All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/edid: Refine HDMI VSDB detect
@ 2021-12-12 15:33 Lee Shawn C
  2021-12-13 12:30 ` Ville Syrjälä
  2021-12-23 13:02 ` Ville Syrjälä
  0 siblings, 2 replies; 9+ messages in thread
From: Lee Shawn C @ 2021-12-12 15:33 UTC (permalink / raw)
  To: dri-devel; +Cc: Lee Shawn C, Dave Airlie

According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain the
3 bytes of the IEEE OUI as well as any additional payload bytes needed."
Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
That may caused some HDMI monitors' audio feature can't be enabled.
Because of they only have three bytes payload (OUI only) in VSDB.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Adam Jackson <ajax@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 12893e7be89b..5aa4a6bf4a13 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
 	if (cea_db_tag(db) != VENDOR_BLOCK)
 		return false;
 
-	if (cea_db_payload_len(db) < 5)
+	if (cea_db_payload_len(db) < 3)
 		return false;
 
 	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-12 15:33 [PATCH] drm/edid: Refine HDMI VSDB detect Lee Shawn C
@ 2021-12-13 12:30 ` Ville Syrjälä
  2021-12-13 12:42   ` Lee, Shawn C
  2021-12-23 13:02 ` Ville Syrjälä
  1 sibling, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2021-12-13 12:30 UTC (permalink / raw)
  To: Lee Shawn C; +Cc: Dave Airlie, dri-devel

On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain the
> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
> That may caused some HDMI monitors' audio feature can't be enabled.
> Because of they only have three bytes payload (OUI only) in VSDB.

HDMI 1.4a says
"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source
 Physical Address field following the 24-bit identifier. ...
 The minimum value of N (length) is 5 and the maximum value of N is 31."

Do you actually have an EDID that violates that?

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..5aa4a6bf4a13 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>  		return false;
>  
> -	if (cea_db_payload_len(db) < 5)
> +	if (cea_db_payload_len(db) < 3)
>  		return false;
>  
>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
> -- 
> 2.31.1

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-13 12:30 ` Ville Syrjälä
@ 2021-12-13 12:42   ` Lee, Shawn C
  2021-12-13 12:45     ` Ville Syrjälä
  2021-12-13 14:09     ` Lee, Shawn C
  0 siblings, 2 replies; 9+ messages in thread
From: Lee, Shawn C @ 2021-12-13 12:42 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Dave Airlie, dri-devel


On Monday, December 13, 2021 8:31 PM, Ville Syrjälä wrote:
>On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
>> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
>> the
>> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
>> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
>> That may caused some HDMI monitors' audio feature can't be enabled.
>> Because of they only have three bytes payload (OUI only) in VSDB.
>
>HDMI 1.4a says
>"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source  Physical Address field following the 24-bit identifier. ...
> The minimum value of N (length) is 5 and the maximum value of N is 31."
>
>Do you actually have an EDID that violates that?
>

Yes! User report when connect to HDMI port on Acer V226HQL. Audio is not working.
But windows system did not have the same problem. We found its VSDB just have 3 bytes
payload (OUI). Then we share this patch to user then they report audio works properly
with this patch.

Best regards,
Shawn

>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Adam Jackson <ajax@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
>> index 12893e7be89b..5aa4a6bf4a13 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>>  		return false;
>>  
>> -	if (cea_db_payload_len(db) < 5)
>> +	if (cea_db_payload_len(db) < 3)
>>  		return false;
>>  
>>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
>> --
>> 2.31.1
>
>--
>Ville Syrjälä
>Intel
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-13 12:42   ` Lee, Shawn C
@ 2021-12-13 12:45     ` Ville Syrjälä
  2021-12-13 14:09     ` Lee, Shawn C
  1 sibling, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2021-12-13 12:45 UTC (permalink / raw)
  To: 20211212153331.15457-1-shawn.c.lee; +Cc: Dave Airlie, dri-devel

On Mon, Dec 13, 2021 at 12:42:22PM +0000, Lee, Shawn C wrote:
> 
> On Monday, December 13, 2021 8:31 PM, Ville Syrjälä wrote:
> >On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
> >> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
> >> the
> >> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
> >> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
> >> That may caused some HDMI monitors' audio feature can't be enabled.
> >> Because of they only have three bytes payload (OUI only) in VSDB.
> >
> >HDMI 1.4a says
> >"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source  Physical Address field following the 24-bit identifier. ...
> > The minimum value of N (length) is 5 and the maximum value of N is 31."
> >
> >Do you actually have an EDID that violates that?
> >
> 
> Yes! User report when connect to HDMI port on Acer V226HQL. Audio is not working.
> But windows system did not have the same problem. We found its VSDB just have 3 bytes
> payload (OUI). Then we share this patch to user then they report audio works properly
> with this patch.

Hrm. This deserves a comment then since it clearly violates the spec.
Also a link to the bug if you have one would be nice to include here.

> 
> Best regards,
> Shawn
> 
> >> 
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Adam Jackson <ajax@redhat.com>
> >> Cc: Dave Airlie <airlied@redhat.com>
> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
> >> index 12893e7be89b..5aa4a6bf4a13 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >>  	if (cea_db_tag(db) != VENDOR_BLOCK)
> >>  		return false;
> >>  
> >> -	if (cea_db_payload_len(db) < 5)
> >> +	if (cea_db_payload_len(db) < 3)
> >>  		return false;
> >>  
> >>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
> >> --
> >> 2.31.1
> >
> >--
> >Ville Syrjälä
> >Intel
> >

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-13 12:42   ` Lee, Shawn C
  2021-12-13 12:45     ` Ville Syrjälä
@ 2021-12-13 14:09     ` Lee, Shawn C
  2021-12-13 14:36       ` Ville Syrjälä
  2021-12-13 15:19       ` Lee, Shawn C
  1 sibling, 2 replies; 9+ messages in thread
From: Lee, Shawn C @ 2021-12-13 14:09 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Dave Airlie, dri-devel


On Monday, December 13, 2021 at 12:45 p.m, Ville Syrjälä wrote:
>On Mon, Dec 13, 2021 at 12:42:22PM +0000, Lee, Shawn C wrote:
>> 
>> On Monday, December 13, 2021 8:31 PM, Ville Syrjälä wrote:
>> >On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
>> >> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
>> >> the
>> >> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
>> >> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
>> >> That may caused some HDMI monitors' audio feature can't be enabled.
>> >> Because of they only have three bytes payload (OUI only) in VSDB.
>> >
>> >HDMI 1.4a says
>> >"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source  Physical Address field following the 24-bit identifier. ...
>> > The minimum value of N (length) is 5 and the maximum value of N is 31."
>> >
>> >Do you actually have an EDID that violates that?
>> >
>> 
>> Yes! User report when connect to HDMI port on Acer V226HQL. Audio is not working.
>> But windows system did not have the same problem. We found its VSDB just have 3 bytes
>> payload (OUI). Then we share this patch to user then they report audio works properly
>> with this patch.
>
>Hrm. This deserves a comment then since it clearly violates the spec.
>Also a link to the bug if you have one would be nice to include here.
>

Let me create an issue and update monitor's EDID for you reference.
But I'm not sure which community is suitable to report this problem.
It looks to me should belong to DRM driver https://gitlab.freedesktop.org/drm/misc/-/issues.
Do you have any suggestion? Thanks!

>> 
>> Best regards,
>> Shawn
>> 
>> >> 
>> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> Cc: Adam Jackson <ajax@redhat.com>
>> >> Cc: Dave Airlie <airlied@redhat.com>
>> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/drm_edid.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
>> >> index 12893e7be89b..5aa4a6bf4a13 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>> >>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>> >>  		return false;
>> >>  
>> >> -	if (cea_db_payload_len(db) < 5)
>> >> +	if (cea_db_payload_len(db) < 3)
>> >>  		return false;
>> >>  
>> >>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
>> >> --
>> >> 2.31.1
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-13 14:09     ` Lee, Shawn C
@ 2021-12-13 14:36       ` Ville Syrjälä
  2021-12-13 15:19       ` Lee, Shawn C
  1 sibling, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2021-12-13 14:36 UTC (permalink / raw)
  To: 20211212153331.15457-1-shawn.c.lee; +Cc: Dave Airlie, dri-devel

On Mon, Dec 13, 2021 at 02:09:33PM +0000, Lee, Shawn C wrote:
> 
> On Monday, December 13, 2021 at 12:45 p.m, Ville Syrjälä wrote:
> >On Mon, Dec 13, 2021 at 12:42:22PM +0000, Lee, Shawn C wrote:
> >> 
> >> On Monday, December 13, 2021 8:31 PM, Ville Syrjälä wrote:
> >> >On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
> >> >> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
> >> >> the
> >> >> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
> >> >> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
> >> >> That may caused some HDMI monitors' audio feature can't be enabled.
> >> >> Because of they only have three bytes payload (OUI only) in VSDB.
> >> >
> >> >HDMI 1.4a says
> >> >"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source  Physical Address field following the 24-bit identifier. ...
> >> > The minimum value of N (length) is 5 and the maximum value of N is 31."
> >> >
> >> >Do you actually have an EDID that violates that?
> >> >
> >> 
> >> Yes! User report when connect to HDMI port on Acer V226HQL. Audio is not working.
> >> But windows system did not have the same problem. We found its VSDB just have 3 bytes
> >> payload (OUI). Then we share this patch to user then they report audio works properly
> >> with this patch.
> >
> >Hrm. This deserves a comment then since it clearly violates the spec.
> >Also a link to the bug if you have one would be nice to include here.
> >
> 
> Let me create an issue and update monitor's EDID for you reference.
> But I'm not sure which community is suitable to report this problem.
> It looks to me should belong to DRM driver https://gitlab.freedesktop.org/drm/misc/-/issues.

That seems fine to me.

> Do you have any suggestion? Thanks!
> 
> >> 
> >> Best regards,
> >> Shawn
> >> 
> >> >> 
> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >> Cc: Adam Jackson <ajax@redhat.com>
> >> >> Cc: Dave Airlie <airlied@redhat.com>
> >> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/drm_edid.c | 2 +-
> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
> >> >> index 12893e7be89b..5aa4a6bf4a13 100644
> >> >> --- a/drivers/gpu/drm/drm_edid.c
> >> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> >> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> >> >>  	if (cea_db_tag(db) != VENDOR_BLOCK)
> >> >>  		return false;
> >> >>  
> >> >> -	if (cea_db_payload_len(db) < 5)
> >> >> +	if (cea_db_payload_len(db) < 3)
> >> >>  		return false;
> >> >>  
> >> >>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
> >> >> --
> >> >> 2.31.1
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >> >

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-13 14:09     ` Lee, Shawn C
  2021-12-13 14:36       ` Ville Syrjälä
@ 2021-12-13 15:19       ` Lee, Shawn C
  1 sibling, 0 replies; 9+ messages in thread
From: Lee, Shawn C @ 2021-12-13 15:19 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Dave Airlie, dri-devel

On Monday, December 13, 2021 at 02:36 p.m, Ville Syrjälä wrote:
>On Mon, Dec 13, 2021 at 02:09:33PM +0000, Lee, Shawn C wrote:
>> 
>> On Monday, December 13, 2021 at 12:45 p.m, Ville Syrjälä wrote:
>> >On Mon, Dec 13, 2021 at 12:42:22PM +0000, Lee, Shawn C wrote:
>> >> 
>> >> On Monday, December 13, 2021 8:31 PM, Ville Syrjälä wrote:
>> >> >On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
>> >> >> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
>> >> >> the
>> >> >> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
>> >> >> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
>> >> >> That may caused some HDMI monitors' audio feature can't be enabled.
>> >> >> Because of they only have three bytes payload (OUI only) in VSDB.
>> >> >
>> >> >HDMI 1.4a says
>> >> >"Sinks shall contain an HDMI VSDB minimally containing a 2-byte Source  Physical Address field following the 24-bit identifier. ...
>> >> > The minimum value of N (length) is 5 and the maximum value of N is 31."
>> >> >
>> >> >Do you actually have an EDID that violates that?
>> >> >
>> >> 
>> >> Yes! User report when connect to HDMI port on Acer V226HQL. Audio is not working.
>> >> But windows system did not have the same problem. We found its VSDB just have 3 bytes
>> >> payload (OUI). Then we share this patch to user then they report audio works properly
>> >> with this patch.
>> >
>> >Hrm. This deserves a comment then since it clearly violates the spec.
>> >Also a link to the bug if you have one would be nice to include here.
>> >
>> 
>> Let me create an issue and update monitor's EDID for you reference.
>> But I'm not sure which community is suitable to report this problem.
>> It looks to me should belong to DRM driver https://gitlab.freedesktop.org/drm/misc/-/issues.
>
>That seems fine to me.
>

Here is the link, thanks! https://gitlab.freedesktop.org/drm/misc/-/issues/28

>> Do you have any suggestion? Thanks!
>> 
>> >> 
>> >> Best regards,
>> >> Shawn
>> >> 
>> >> >> 
>> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> >> Cc: Adam Jackson <ajax@redhat.com>
>> >> >> Cc: Dave Airlie <airlied@redhat.com>
>> >> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/drm_edid.c | 2 +-
>> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >> 
>> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
>> >> >> index 12893e7be89b..5aa4a6bf4a13 100644
>> >> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> >> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>> >> >>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>> >> >>  		return false;
>> >> >>  
>> >> >> -	if (cea_db_payload_len(db) < 5)
>> >> >> +	if (cea_db_payload_len(db) < 3)
>> >> >>  		return false;
>> >> >>  
>> >> >>  	return oui(db[3], db[2], db[1]) == HDMI_IEEE_OUI;
>> >> >> --
>> >> >> 2.31.1
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel
>> >> >
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-12 15:33 [PATCH] drm/edid: Refine HDMI VSDB detect Lee Shawn C
  2021-12-13 12:30 ` Ville Syrjälä
@ 2021-12-23 13:02 ` Ville Syrjälä
  2021-12-23 14:40   ` Lee, Shawn C
  1 sibling, 1 reply; 9+ messages in thread
From: Ville Syrjälä @ 2021-12-23 13:02 UTC (permalink / raw)
  To: Lee Shawn C; +Cc: Dave Airlie, Hans Verkuil, dri-devel

On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain the
> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
> That may caused some HDMI monitors' audio feature can't be enabled.
> Because of they only have three bytes payload (OUI only) in VSDB.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Adam Jackson <ajax@redhat.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 12893e7be89b..5aa4a6bf4a13 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>  		return false;
>  
> -	if (cea_db_payload_len(db) < 5)
> +	if (cea_db_payload_len(db) < 3)
>  		return false;

I was a a bit worried that we are now assuming that we can parse some
stuff without further length checks, but looks like that's just the 
"source physical address" stuff which we do not parse in drm_edid.c,
and the other fields we do parse are actually optional and so already
have the require length checks. So this seems fine.

Closes: https://gitlab.freedesktop.org/drm/misc/-/issues/28
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Note that there is a second edid parser in cec_get_edid_spa_location()
that does parse the source physical address. You may want to double
check that it doesn't make any bad assumptions about the length 
of the vsdb either. I think we should probably get rid of that
second parser and just have drm_edid.c extract the source physical
address and pass that on directly to the cec code instead. But I
guess the cec code still couldn't remove the second parser
since some media drivers need it :( Though it could then perhaps
be moved into the media code instead of having it as a massive
inline function in the cec headers.

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] drm/edid: Refine HDMI VSDB detect
  2021-12-23 13:02 ` Ville Syrjälä
@ 2021-12-23 14:40   ` Lee, Shawn C
  0 siblings, 0 replies; 9+ messages in thread
From: Lee, Shawn C @ 2021-12-23 14:40 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Dave Airlie, Hans Verkuil, dri-devel

On Thursday, December 23, 2021 1:02 PM, Ville Syrjälä wrote:
>On Sun, Dec 12, 2021 at 11:33:31PM +0800, Lee Shawn C wrote:
>> According to CEA-861-F chapter 7.5.4. It says "The VSDB shall contain 
>> the
>> 3 bytes of the IEEE OUI as well as any additional payload bytes needed."
>> Now DRM driver check HDMI OUI but VSDB payload size at least five bytes.
>> That may caused some HDMI monitors' audio feature can't be enabled.
>> Because of they only have three bytes payload (OUI only) in VSDB.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Adam Jackson <ajax@redhat.com>
>> Cc: Dave Airlie <airlied@redhat.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
>> index 12893e7be89b..5aa4a6bf4a13 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4205,7 +4205,7 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>>  	if (cea_db_tag(db) != VENDOR_BLOCK)
>>  		return false;
>>  
>> -	if (cea_db_payload_len(db) < 5)
>> +	if (cea_db_payload_len(db) < 3)
>>  		return false;
>
>I was a a bit worried that we are now assuming that we can parse some
>stuff without further length checks, but looks like that's just the
>"source physical address" stuff which we do not parse in drm_edid.c,
>and the other fields we do parse are actually optional and so already
>have the require length checks. So this seems fine.
>

Thanks for the comments! We will share the information to customer.

Best regards,
Shawn

>Closes: https://gitlab.freedesktop.org/drm/misc/-/issues/28
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>Note that there is a second edid parser in cec_get_edid_spa_location()
>that does parse the source physical address. You may want to double
>check that it doesn't make any bad assumptions about the length
>of the vsdb either. I think we should probably get rid of that
>second parser and just have drm_edid.c extract the source physical
>address and pass that on directly to the cec code instead. But I
>guess the cec code still couldn't remove the second parser
>since some media drivers need it :( Though it could then perhaps
>be moved into the media code instead of having it as a massive
>inline function in the cec headers.
>
>--
>Ville Syrjälä
>Intel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-12-23 14:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-12 15:33 [PATCH] drm/edid: Refine HDMI VSDB detect Lee Shawn C
2021-12-13 12:30 ` Ville Syrjälä
2021-12-13 12:42   ` Lee, Shawn C
2021-12-13 12:45     ` Ville Syrjälä
2021-12-13 14:09     ` Lee, Shawn C
2021-12-13 14:36       ` Ville Syrjälä
2021-12-13 15:19       ` Lee, Shawn C
2021-12-23 13:02 ` Ville Syrjälä
2021-12-23 14:40   ` Lee, Shawn C

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.