[PATCH v4 12/14] drm/edid: introduce a helper that compares edid data from two drm_edid

[Melissa Wen]

On 13/06/2025 15:35, Mario Limonciello wrote:
> On 6/13/2025 9:58 AM, Melissa Wen wrote:
>> AMD driver has a function used to compare if two edid are the same; this
>> is useful to some of the link detection algorithms implemented by
>> amdgpu. Since the amdgpu function can be helpful for other drivers, this
>> commit abstracts the AMD function to make it available at the DRM level
>> by wrapping existent drm_edid_eq().
>>
>> v2:
>> - rename drm_edid_eq to drm_edid_eq_buf (jani)
>> - add NULL checks (jani)
>>
>> v3:
>> - fix kernel-doc (jani)
>> - fix parameter names
>>
>> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
>> Co-developed-by: Rodrigo Siqueira <siqueira at igalia.com>
>> Signed-off-by: Rodrigo Siqueira <siqueira at igalia.com>
>> Signed-off-by: Melissa Wen <mwen at igalia.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 24 +++++++++++++++++++++---
>>   include/drm/drm_edid.h     |  2 ++
>>   2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index d5772a3d27f1..056e070b2f55 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1820,8 +1820,8 @@ static bool edid_block_is_zero(const void *edid)
>>       return mem_is_zero(edid, EDID_LENGTH);
>>   }
>>   -static bool drm_edid_eq(const struct drm_edid *drm_edid,
>> -            const void *raw_edid, size_t raw_edid_size)
>> +static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
>> +                const void *raw_edid, size_t raw_edid_size)
>>   {
>>       bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
>>       bool edid2_present = raw_edid && raw_edid_size;
>> @@ -6915,7 +6915,7 @@ static int 
>> _drm_edid_connector_property_update(struct drm_connector *connector,
>>           const void *old_edid = connector->edid_blob_ptr->data;
>>           size_t old_edid_size = connector->edid_blob_ptr->length;
>>   -        if (old_edid && !drm_edid_eq(drm_edid, old_edid, 
>> old_edid_size)) {
>> +        if (old_edid && !drm_edid_eq_buf(drm_edid, old_edid, 
>> old_edid_size)) {
>>               connector->epoch_counter++;
>>               drm_dbg_kms(dev, "[CONNECTOR:%d:%s] EDID changed, epoch 
>> counter %llu\n",
>>                       connector->base.id, connector->name,
>> @@ -7520,3 +7520,21 @@ bool drm_edid_is_digital(const struct drm_edid 
>> *drm_edid)
>>           drm_edid->edid->input & DRM_EDID_INPUT_DIGITAL;
>>   }
>>   EXPORT_SYMBOL(drm_edid_is_digital);
>> +
>> +/**
>> + * drm_edid_eq - Check if EDIDs are equal
>> + *
>> + * @drm_edid_1: first drm_edid to compare edid
>> + * @drm_edid_2: second drm_edid to compare edid
>> + *
>> + * Return true if EDIDs are equal.
>> + */
>> +bool drm_edid_eq(const struct drm_edid *drm_edid_1,
>> +         const struct drm_edid *drm_edid_2)
>> +{
>> +    const void *edid_1 = drm_edid_1 ? drm_edid_1->edid : NULL;
>> +    size_t edid_1_size = drm_edid_1 ? drm_edid_1->size : 0;
>> +
>> +    return drm_edid_eq_buf(drm_edid_2, edid_1, edid_1_size);
>
> What happens when the size for edid 2 is different than edid 1? Does 
> drm_edid_eq_buf() already handle this well (I didn't immediately look)?

drm_edid_eq_buf handles this case.
This is the code:

static bool drm_edid_eq_buf(const struct drm_edid *drm_edid,
                                              const void *raw_edid, 
size_t raw_edid_size)
{
     bool edid1_present = drm_edid && drm_edid->edid && drm_edid->size;
     bool edid2_present = raw_edid && raw_edid_size;

     if (edid1_present != edid2_present)
         return false;

     if (edid1_present) {
         if (drm_edid->size != raw_edid_size)
             return false;

         if (memcmp(drm_edid->edid, raw_edid, drm_edid->size))
             return false;
     }

     return true;
}

>
>
> If not; how about including an extra check directly in this function?
>
> I was thinking this will handle it effectively:
>
> if (!drm_edid_1 || !drm_edid_2)
>     return false;
If both EDIDs are NULL, they are equal (and we don't need to update 
anything).
So drm_edid_eq() should return true.


> if (drm_edid_1->size != drm_edid_2->size)
>     return false;
>
And the size comparison is handled by drm_edid_eq_buf() in this part:

     if (edid1_present) {
         if (drm_edid->size != raw_edid_size)
             return false;

> return drm_edid_eq_buf();
>
>
>
>> +}
>> +EXPORT_SYMBOL(drm_edid_eq);
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 960592167486..e7a9a4928b97 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -469,6 +469,8 @@ int drm_edid_connector_update(struct 
>> drm_connector *connector,
>>                     const struct drm_edid *edid);
>>   int drm_edid_connector_add_modes(struct drm_connector *connector);
>>   bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>> +bool drm_edid_eq(const struct drm_edid *drm_edid_first,
>> +             const struct drm_edid *drm_edid_second);
>>   void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>                    struct drm_edid_product_id *id);
>>   void drm_edid_print_product_id(struct drm_printer *p,
>