Re: [Intel-gfx] [PATCH v1 01/13] drm/edid: add block count and data helper functions for drm_edid

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 24.05.2022 12:39, Jani Nikula wrote:
> Add drm_edid based block count and data access helper functions that
> take the EDID allocated size into account.
> 
> At the moment, the allocated size should always match the EDID size
> indicated by the extension count, but this will change in the future.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/drm_edid.c | 42 +++++++++++++++++++++++++++++++-------
>   1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 929fc0e46751..682d954a9e42 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1613,6 +1613,35 @@ static const void *edid_extension_block_data(const struct edid *edid, int index)
>   	return edid_block_data(edid, index + 1);
>   }
>   
> +static int drm_edid_block_count(const struct drm_edid *drm_edid)
> +{
> +	int num_blocks;
> +
> +	/* Starting point */
> +	num_blocks = edid_block_count(drm_edid->edid);
> +
> +	/* Limit by allocated size */
> +	num_blocks = min(num_blocks, (int)drm_edid->size / EDID_LENGTH);
> +
> +	return num_blocks;
> +}
> +
> +static int drm_edid_extension_block_count(const struct drm_edid *drm_edid)
> +{
> +	return drm_edid_block_count(drm_edid) - 1;
> +}
> +
> +static const void *drm_edid_block_data(const struct drm_edid *drm_edid, int index)
> +{
> +	return edid_block_data(drm_edid->edid, index);
> +}
> +
> +static const void *drm_edid_extension_block_data(const struct drm_edid *drm_edid,
> +						 int index)
> +{
> +	return edid_extension_block_data(drm_edid->edid, index);
> +}
> +
>   /*
>    * Initializer helper for legacy interfaces, where we have no choice but to
>    * trust edid size. Not for general purpose use.
> @@ -1665,8 +1694,8 @@ static const void *__drm_edid_iter_next(struct drm_edid_iter *iter)
>   	if (!iter->drm_edid)
>   		return NULL;
>   
> -	if (iter->index < edid_block_count(iter->drm_edid->edid))
> -		block = edid_block_data(iter->drm_edid->edid, iter->index++);
> +	if (iter->index < drm_edid_block_count(iter->drm_edid))
> +		block = drm_edid_block_data(iter->drm_edid, iter->index++);
>   
>   	return block;
>   }
> @@ -3574,22 +3603,21 @@ static int add_detailed_modes(struct drm_connector *connector,
>   const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>   				  int ext_id, int *ext_index)
>   {
> -	const struct edid *edid = drm_edid ? drm_edid->edid : NULL;

Do we still need this var?


>   	const u8 *edid_ext = NULL;
>   	int i;
>   
>   	/* No EDID or EDID extensions */
> -	if (!edid || !edid_extension_block_count(edid))
> +	if (!drm_edid || !drm_edid_extension_block_count(drm_edid))
>   		return NULL;
>   
>   	/* Find CEA extension */
> -	for (i = *ext_index; i < edid_extension_block_count(edid); i++) {
> -		edid_ext = edid_extension_block_data(edid, i);
> +	for (i = *ext_index; i < drm_edid_extension_block_count(drm_edid); i++) {
> +		edid_ext = drm_edid_extension_block_data(drm_edid, i);
>   		if (edid_block_tag(edid_ext) == ext_id)
>   			break;
>   	}
>   
> -	if (i >= edid_extension_block_count(edid))
> +	if (i >= drm_edid_extension_block_count(drm_edid))
>   		return NULL;
>   
>   	*ext_index = i + 1;

It looks OK. Some suggestions to consider:
1. While at it, refactor little bit the code to return ext from 'for' 
loop and NULL later (to kill after-loop checks, and better code IMO).
2. Implement kind of iterator, for example 
drm_edid_extension_block_next(drm_edid, edid_ext), then use loop:
for (edid_ext = NULL; edid_ext = drm_edid_extension_block_next(drm_edid, 
edid_ext;)
	...

Up to you.
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej