Re: [PATCH] drm/edid: fix CEA extension byte #3 parsing

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 23 Mar 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Mar 23, 2022 at 12:04:38PM +0200, Jani Nikula wrote:
>> Only an EDID CEA extension has byte #3, while the CTA DisplayID Data
>> Block does not. Don't interpret bogus data for color formats.
>
> I think what we might want eventually is a cleaner split between
> the CTA data blocks vs. the rest of the EDID CTA ext block. Only
> the former is relevant for DisplayID.
>
> But for a bugfix we want to keep it simple.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, pushed to drm-misc-next-fixes.

BR,
Jani.

>
>> 
>> For most displays it's probably an unlikely scenario you'd have a CTA
>> DisplayID Data Block without a CEA extension, but they do exist.
>> 
>> Fixes: e28ad544f462 ("drm/edid: parse CEA blocks embedded in DisplayID")
>> Cc: <stable@vger.kernel.org> # v4.15
>> Cc: Shawn C Lee <shawn.c.lee@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> ---
>> 
>> commit e28ad544f462 was merged in v5.3, but it has Cc: stable for v4.15.
>> 
>> This is also fixed in my CEA data block iteration series [1], but we'll
>> want the simple fix for stable first.
>> 
>> Hum, CTA is formerly CEA, I and the code seem to use both, should we use
>> only one or the other?
>
> And before CEA it was called EIA (IIRC). Dunno if we also use that name
> somewhere.
>
> If someone cares enough I guess we could rename everything to "cta".
>
>> 
>> [1] https://patchwork.freedesktop.org/series/101659/
>> ---
>>  drivers/gpu/drm/drm_edid.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 561f53831e29..ccf7031a6797 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -5187,10 +5187,14 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>  
>>  	/* The existence of a CEA block should imply RGB support */
>>  	info->color_formats = DRM_COLOR_FORMAT_RGB444;
>> -	if (edid_ext[3] & EDID_CEA_YCRCB444)
>> -		info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
>> -	if (edid_ext[3] & EDID_CEA_YCRCB422)
>> -		info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
>> +
>> +	/* CTA DisplayID Data Block does not have byte #3 */
>> +	if (edid_ext[0] == CEA_EXT) {
>> +		if (edid_ext[3] & EDID_CEA_YCRCB444)
>> +			info->color_formats |= DRM_COLOR_FORMAT_YCBCR444;
>> +		if (edid_ext[3] & EDID_CEA_YCRCB422)
>> +			info->color_formats |= DRM_COLOR_FORMAT_YCBCR422;
>> +	}
>>  
>>  	if (cea_db_offsets(edid_ext, &start, &end))
>>  		return;
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center