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

[Mario Limonciello <mario.limonciello@amd.com>]

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@intel.com>
> Co-developed-by: Rodrigo Siqueira <siqueira@igalia.com>
> Signed-off-by: Rodrigo Siqueira <siqueira@igalia.com>
> Signed-off-by: Melissa Wen <mwen@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)?

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 (drm_edid_1->size != drm_edid_2->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,