Re: [PATCH] drm/edid: Refine HDMI VSDB detect

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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