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

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

On Thu, Feb 24, 2022 at 10:16:23PM +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 | 110 ++++++++++++++++++++-----------------
>  1 file changed, 60 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a504542238ed..19426f28b411 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3353,16 +3353,14 @@ const u8 *drm_find_edid_extension(const struct edid *edid,
>  	return edid_ext;
>  }
>  
> -static const u8 *drm_find_cea_extension(const struct edid *edid)
> +static const u8 *drm_find_cea_extension(const struct edid *edid, int *ext_index)
>  {
>  	const struct displayid_block *block;
>  	struct displayid_iter iter;
>  	const u8 *cea;
> -	int ext_index = 0;
>  
> -	/* Look for a top level CEA extension block */
> -	/* FIXME: make callers iterate through multiple CEA ext blocks? */
> -	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
> +	/* Look for a CEA extension block from ext_index */
> +	cea = drm_find_edid_extension(edid, CEA_EXT, ext_index);
>  	if (cea)
>  		return cea;
>  
> @@ -3643,10 +3641,10 @@ add_alternate_cea_modes(struct drm_connector *connector, struct edid *edid)
>  	struct drm_device *dev = connector->dev;
>  	struct drm_display_mode *mode, *tmp;
>  	LIST_HEAD(list);
> -	int modes = 0;
> +	int modes = 0, ext_index = 0;
>  
>  	/* Don't add CEA modes if the CEA extension block is missing */
> -	if (!drm_find_cea_extension(edid))
> +	if (!drm_find_cea_extension(edid, &ext_index))
>  		return 0;
>  
>  	/*
> @@ -4321,46 +4319,58 @@ static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
>  static int
>  add_cea_modes(struct drm_connector *connector, struct edid *edid)
>  {
> -	const u8 *cea = drm_find_cea_extension(edid);
> +	const u8 *cea = NULL;
>  	const u8 *db, *hdmi = NULL, *video = NULL;
>  	u8 dbl, hdmi_len, video_len = 0;
> -	int modes = 0;
> +	int modes = 0, ext_index = 0;
>  
> -	if (cea && cea_revision(cea) >= 3) {
> -		int i, start, end;
> +	for (;;) {
> +		cea = drm_find_cea_extension(edid, &ext_index);

Please split this into two patches:
1. do the *ext_index stuff
2. do the loop

>  
> -		if (cea_db_offsets(cea, &start, &end))
> -			return 0;
> +		if (!cea)
> +			break;
>  
> -		for_each_cea_db(cea, i, start, end) {
> -			db = &cea[i];
> -			dbl = cea_db_payload_len(db);
> +		if (cea && cea_revision(cea) >= 3) {
> +			int i, start, end;
> +
> +			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) {

Looks like you're potentially using stale information here from
previous loops. Please move all the possible variables to a tighter
scope so this can't happen.

> +			modes += do_hdmi_vsdb_modes(connector, hdmi, hdmi_len, video,
> +						    video_len);
> +			hdmi  = NULL;
> +			video = NULL;
> +			hdmi_len = 0;
> +			video_len = 0;
> +		}
> +	}
>  
>  	return modes;
>  }
> @@ -4562,7 +4572,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  	uint8_t *eld = connector->eld;
>  	const u8 *cea;
>  	const u8 *db;
> -	int total_sad_count = 0;
> +	int total_sad_count = 0, ext_index = 0;
>  	int mnl;
>  	int dbl;
>  
> @@ -4571,7 +4581,7 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>  	if (!edid)
>  		return;
>  
> -	cea = drm_find_cea_extension(edid);
> +	cea = drm_find_cea_extension(edid, &ext_index);
>  	if (!cea) {
>  		DRM_DEBUG_KMS("ELD: no CEA Extension found\n");
>  		return;
> @@ -4655,11 +4665,11 @@ static void drm_edid_to_eld(struct drm_connector *connector, struct edid *edid)
>   */
>  int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads)
>  {
> -	int count = 0;
> +	int count = 0, ext_index = 0;
>  	int i, start, end, dbl;
>  	const u8 *cea;
>  
> -	cea = drm_find_cea_extension(edid);
> +	cea = drm_find_cea_extension(edid, &ext_index);
>  	if (!cea) {
>  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
>  		return 0;
> @@ -4717,11 +4727,11 @@ EXPORT_SYMBOL(drm_edid_to_sad);
>   */
>  int drm_edid_to_speaker_allocation(struct edid *edid, u8 **sadb)
>  {
> -	int count = 0;
> +	int count = 0, ext_index = 0;
>  	int i, start, end, dbl;
>  	const u8 *cea;
>  
> -	cea = drm_find_cea_extension(edid);
> +	cea = drm_find_cea_extension(edid, &ext_index);
>  	if (!cea) {
>  		DRM_DEBUG_KMS("SAD: no CEA Extension found\n");
>  		return 0;
> @@ -4813,10 +4823,10 @@ EXPORT_SYMBOL(drm_av_sync_delay);
>  bool drm_detect_hdmi_monitor(struct edid *edid)
>  {
>  	const u8 *edid_ext;
> -	int i;
> +	int i, ext_index = 0;
>  	int start_offset, end_offset;
>  
> -	edid_ext = drm_find_cea_extension(edid);
> +	edid_ext = drm_find_cea_extension(edid, &ext_index);
>  	if (!edid_ext)
>  		return false;
>  
> @@ -4851,11 +4861,11 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  bool drm_detect_monitor_audio(struct edid *edid)
>  {
>  	const u8 *edid_ext;
> -	int i, j;
> +	int i, j, ext_index = 0;
>  	bool has_audio = false;
>  	int start_offset, end_offset;
>  
> -	edid_ext = drm_find_cea_extension(edid);
> +	edid_ext = drm_find_cea_extension(edid, &ext_index);
>  	if (!edid_ext)
>  		goto end;
>  
> @@ -5177,9 +5187,9 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  	const u8 *edid_ext;
> -	int i, start, end;
> +	int i, start, end, ext_index = 0;
>  
> -	edid_ext = drm_find_cea_extension(edid);
> +	edid_ext = drm_find_cea_extension(edid, &ext_index);
>  	if (!edid_ext)
>  		return;
>  
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel