Re: [Intel-gfx] [PATCH 1/3] drm/edid: Allow looking for ext blocks starting from a specified index

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2020-05-27 at 16:03 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently EDIDs with multiple DispID ext blocks is a thing, so prepare
> for iterating through multiple ext blocks of the same type by
> passing the starting ext block index to drm_find_edid_extension(). Well
> also have drm_find_edid_extension() update the index to point to the
> next ext block on success. Thus we should be able to call
> drm_find_edid_extension() in loop.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d8372d63851b..f2531d51dfa2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3188,7 +3188,8 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  /*
>   * Search EDID for CEA extension block.
>   */
> -static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
> +static u8 *drm_find_edid_extension(const struct edid *edid,
> +				   int ext_id, int *ext_index)
>  {
>  	u8 *edid_ext = NULL;
>  	int i;
> @@ -3198,23 +3199,26 @@ static u8 *drm_find_edid_extension(const struct edid *edid, int ext_id)
>  		return NULL;
>  
>  	/* Find CEA extension */
> -	for (i = 0; i < edid->extensions; i++) {
> +	for (i = *ext_index; i < edid->extensions; i++) {
>  		edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1);
>  		if (edid_ext[0] == ext_id)
>  			break;
>  	}
>  
> -	if (i == edid->extensions)
> +	if (i >= edid->extensions)
>  		return NULL;
>  
> +	*ext_index = i + 1;
> +
>  	return edid_ext;
>  }
>  

I would add something like drm_find_edid_n_extension() with the implementation above and then implement drm_find_edid_extension() calling
drm_find_edid_n_extension() but it is just one caller that is not using ext_index so LGTM.

>  
>  static u8 *drm_find_displayid_extension(const struct edid *edid,
> -					int *length, int *idx)
> +					int *length, int *idx,
> +					int *ext_index)
>  {
> -	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
> +	u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT, ext_index);
>  	struct displayid_hdr *base;
>  	int ret;
>  
> @@ -3241,14 +3245,18 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  	struct displayid_block *block;
>  	u8 *cea;
>  	u8 *displayid;
> +	int ext_index;
>  
>  	/* Look for a top level CEA extension block */
> -	cea = drm_find_edid_extension(edid, CEA_EXT);
> +	ext_index = 0;

In 2 places ext_index is initialized in the variable declaration and in 2 other places is not, all of it could be done in the declaration or if you
really want to leave the context close to the users, initialize it in the "for (;;)" in the next patch.

With the change above:

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
>  	if (cea)
>  		return cea;
>  
>  	/* CEA blocks can also be found embedded in a DisplayID block */
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	ext_index = 0;
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid)
>  		return NULL;
>  
> @@ -5195,8 +5203,10 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	int length, idx;
>  	struct displayid_block *block;
>  	int num_modes = 0;
> +	int ext_index = 0;
>  
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid)
>  		return 0;
>  
> @@ -5870,11 +5880,13 @@ void drm_update_tile_info(struct drm_connector *connector,
>  			  const struct edid *edid)
>  {
>  	const void *displayid = NULL;
> +	int ext_index = 0;
>  	int length, idx;
>  	int ret;
>  
>  	connector->has_tile = false;
> -	displayid = drm_find_displayid_extension(edid, &length, &idx);
> +	displayid = drm_find_displayid_extension(edid, &length, &idx,
> +						 &ext_index);
>  	if (!displayid) {
>  		/* drop reference to any tile group we had */
>  		goto out_drop_ref;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel