Re: [Intel-gfx] [PATCH 2/3] drm/edid: Iterate through all DispID ext blocks

["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 there are EDIDs in the wild with multiple DispID extension
> blocks. Iterate through them all.
> 
> In one particular case the tile information is specicied in the
> second DispID ext block, and since the current parser only looks
> at the first DispID ext block we don't notice that we're dealing
> with a tiled display.
> 
> While at it change a few functions to return void since we have
> no use for the errno.

With the change in the previous patch:
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> 
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/27
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 84 +++++++++++++++++---------------------
>  1 file changed, 38 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index f2531d51dfa2..dcb23563d29d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3248,6 +3248,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  	int ext_index;
>  
>  	/* Look for a top level CEA extension block */
> +	/* FIXME: make callers iterate through multiple CEA ext blocks? */
>  	ext_index = 0;
>  	cea = drm_find_edid_extension(edid, CEA_EXT, &ext_index);
>  	if (cea)
> @@ -3255,20 +3256,20 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
>  
>  	/* CEA blocks can also be found embedded in a DisplayID block */
>  	ext_index = 0;
> -	displayid = drm_find_displayid_extension(edid, &length, &idx,
> -						 &ext_index);
> -	if (!displayid)
> -		return NULL;
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
> +			return NULL;
>  
> -	idx += sizeof(struct displayid_hdr);
> -	for_each_displayid_db(displayid, block, idx, length) {
> -		if (block->tag == DATA_BLOCK_CTA) {
> -			cea = (u8 *)block;
> -			break;
> +		idx += sizeof(struct displayid_hdr);
> +		for_each_displayid_db(displayid, block, idx, length) {
> +			if (block->tag == DATA_BLOCK_CTA)
> +				return (u8 *)block;
>  		}
>  	}
>  
> -	return cea;
> +	return NULL;
>  }
>  
>  static __always_inline const struct drm_display_mode *cea_mode_for_vic(u8 vic)
> @@ -5205,19 +5206,22 @@ static int add_displayid_detailed_modes(struct drm_connector *connector,
>  	int num_modes = 0;
>  	int ext_index = 0;
>  
> -	displayid = drm_find_displayid_extension(edid, &length, &idx,
> -						 &ext_index);
> -	if (!displayid)
> -		return 0;
> -
> -	idx += sizeof(struct displayid_hdr);
> -	for_each_displayid_db(displayid, block, idx, length) {
> -		switch (block->tag) {
> -		case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> -			num_modes += add_displayid_detailed_1_modes(connector, block);
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
>  			break;
> +
> +		idx += sizeof(struct displayid_hdr);
> +		for_each_displayid_db(displayid, block, idx, length) {
> +			switch (block->tag) {
> +			case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
> +				num_modes += add_displayid_detailed_1_modes(connector, block);
> +				break;
> +			}
>  		}
>  	}
> +
>  	return num_modes;
>  }
>  
> @@ -5797,8 +5801,8 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe *frame,
>  }
>  EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode);
>  
> -static int drm_parse_tiled_block(struct drm_connector *connector,
> -				 const struct displayid_block *block)
> +static void drm_parse_tiled_block(struct drm_connector *connector,
> +				  const struct displayid_block *block)
>  {
>  	const struct displayid_tiled_block *tile = (struct displayid_tiled_block *)block;
>  	u16 w, h;
> @@ -5836,7 +5840,7 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
>  		tg = drm_mode_create_tile_group(connector->dev, tile->topology_id);
>  	}
>  	if (!tg)
> -		return -ENOMEM;
> +		return;
>  
>  	if (connector->tile_group != tg) {
>  		/* if we haven't got a pointer,
> @@ -5848,14 +5852,12 @@ static int drm_parse_tiled_block(struct drm_connector *connector,
>  	} else
>  		/* if same tile group, then release the ref we just took. */
>  		drm_mode_put_tile_group(connector->dev, tg);
> -	return 0;
>  }
>  
> -static int drm_displayid_parse_tiled(struct drm_connector *connector,
> -				     const u8 *displayid, int length, int idx)
> +static void drm_displayid_parse_tiled(struct drm_connector *connector,
> +				      const u8 *displayid, int length, int idx)
>  {
>  	const struct displayid_block *block;
> -	int ret;
>  
>  	idx += sizeof(struct displayid_hdr);
>  	for_each_displayid_db(displayid, block, idx, length) {
> @@ -5864,16 +5866,13 @@ static int drm_displayid_parse_tiled(struct drm_connector *connector,
>  
>  		switch (block->tag) {
>  		case DATA_BLOCK_TILED_DISPLAY:
> -			ret = drm_parse_tiled_block(connector, block);
> -			if (ret)
> -				return ret;
> +			drm_parse_tiled_block(connector, block);
>  			break;
>  		default:
>  			DRM_DEBUG_KMS("found DisplayID tag 0x%x, unhandled\n", block->tag);
>  			break;
>  		}
>  	}
> -	return 0;
>  }
>  
>  void drm_update_tile_info(struct drm_connector *connector,
> @@ -5882,26 +5881,19 @@ void drm_update_tile_info(struct drm_connector *connector,
>  	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,
> -						 &ext_index);
> -	if (!displayid) {
> -		/* drop reference to any tile group we had */
> -		goto out_drop_ref;
> +	for (;;) {
> +		displayid = drm_find_displayid_extension(edid, &length, &idx,
> +							 &ext_index);
> +		if (!displayid)
> +			break;
> +
> +		drm_displayid_parse_tiled(connector, displayid, length, idx);
>  	}
>  
> -	ret = drm_displayid_parse_tiled(connector, displayid, length, idx);
> -	if (ret < 0)
> -		goto out_drop_ref;
> -	if (!connector->has_tile)
> -		goto out_drop_ref;
> -	return;
> -out_drop_ref:
> -	if (connector->tile_group) {
> +	if (!connector->has_tile && connector->tile_group) {
>  		drm_mode_put_tile_group(connector->dev, connector->tile_group);
>  		connector->tile_group = NULL;
>  	}
> -	return;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel