Re: [PATCH 1/3] drm/edid: parse multiple CEA extension block

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

On Tue, 22 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote:
>> Try to find and parse more CEA ext blocks if edid->extensions
>> is greater than one.
>> 
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++---------------
>>  1 file changed, 45 insertions(+), 30 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 12893e7be89b..3d5dbbeca7f9 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4313,43 +4313,58 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
>>  	const u8 *cea = drm_find_cea_extension(edid);
>>  	const u8 *db, *hdmi = NULL, *video = NULL;
>>  	u8 dbl, hdmi_len, video_len = 0;
>> -	int modes = 0;
>> +	int modes = 0, j;
>>  
>> -	if (cea && cea_revision(cea) >= 3) {
>> -		int i, start, end;
>> +	if (!cea)
>> +		return 0;
>>  
>> -		if (cea_db_offsets(cea, &start, &end))
>> -			return 0;
>> +	for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) {
>
> That looks rather illegible. I think we want a
> drm_find_cea_extension(const struct edid *edid, int *ext_index)
> and then just loop until it stops giving us stuff.

Neither approach takes multiple CEA blocks within DisplayID extension
into account. Or some blocks outside and some inside DisplayID
extension.

I think we're going to need abstracted EDID iteration similar to what
I've done for DisplayID iteration. We can't have all places
reimplementing the iteration like we have now.

BR,
Jani.

>
> There are also several other callers of drm_find_cea_extension().
> Why don't they require the same treatment?
>
>> +		if (cea && cea_revision(cea) >= 3) {
>> +			int i, start, end;
>>  
>> -		for_each_cea_db(cea, i, start, end) {
>> -			db = &cea[i];
>> -			dbl = cea_db_payload_len(db);
>> +			if (cea_db_offsets(cea, &start, &end))
>> +				continue;
>>  
>> -			if (cea_db_tag(db) == VIDEO_BLOCK) {
>> -				video = db + 1;
>> -				video_len = dbl;
>> -				modes += do_cea_modes(connector, video, dbl);
>> -			} else if (cea_db_is_hdmi_vsdb(db)) {
>> -				hdmi = db;
>> -				hdmi_len = dbl;
>> -			} else if (cea_db_is_y420vdb(db)) {
>> -				const u8 *vdb420 = &db[2];
>> -
>> -				/* Add 4:2:0(only) modes present in EDID */
>> -				modes += do_y420vdb_modes(connector,
>> -							  vdb420,
>> -							  dbl - 1);
>> +			for_each_cea_db(cea, i, start, end) {
>> +				db = &cea[i];
>> +				dbl = cea_db_payload_len(db);
>> +
>> +				if (cea_db_tag(db) == VIDEO_BLOCK) {
>> +					video = db + 1;
>> +					video_len = dbl;
>> +					modes += do_cea_modes(connector, video, dbl);
>> +				} else if (cea_db_is_hdmi_vsdb(db)) {
>> +					hdmi = db;
>> +					hdmi_len = dbl;
>> +				} else if (cea_db_is_y420vdb(db)) {
>> +					const u8 *vdb420 = &db[2];
>> +
>> +					/* Add 4:2:0(only) modes present in EDID */
>> +					modes += do_y420vdb_modes(connector,
>> +								  vdb420,
>> +								  dbl - 1);
>> +				}
>>  			}
>>  		}
>> -	}
>>  
>> -	/*
>> -	 * We parse the HDMI VSDB after having added the cea modes as we will
>> -	 * be patching their flags when the sink supports stereo 3D.
>> -	 */
>> -	if (hdmi)
>> -		modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>> -					    video_len);
>> +		/*
>> +		 * We parse the HDMI VSDB after having added the cea modes as we will
>> +		 * be patching their flags when the sink supports stereo 3D.
>> +		 */
>> +		if (hdmi) {
>> +			modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
>> +						    video_len);
>> +			hdmi  = NULL;
>> +			video = NULL;
>> +			hdmi_len = 0;
>> +			video_len = 0;
>> +		}
>> +
>> +		/* move to next CEA extension block */
>> +		cea = drm_find_edid_extension(edid, CEA_EXT, &j);
>> +		if (!cea)
>> +			break;
>> +	}
>>  
>>  	return modes;
>>  }
>> -- 
>> 2.17.1

-- 
Jani Nikula, Intel Open Source Graphics Center