On Tue, 24 May 2022, Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
> 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@xxxxxxxxx>
>> ---
>> 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?
I am removing it...?
>
>
>> 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@xxxxxxxxx>
There's already the struct drm_edid_iter stuff that this could be
converted to, but just haven't gotten around to it yet. I'll follow up
with that later. Thanks for the review.
BR,
Jani.
>
> Regards
> Andrzej
>
--
Jani Nikula, Intel Open Source Graphics Center
