Re: [PATCH 11/13] drm/edid: introduce a helper that compares edid data from two drm_edid

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 11 Apr 2025, Melissa Wen <mwen@igalia.com> 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().
>
> 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 | 18 ++++++++++++++++++
>  include/drm/drm_edid.h     |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 855beafb76ff..328a25d198e5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -7502,3 +7502,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_is_edid_eq - Check if it the EDID is equal

I think drm_edid_eq() is the better name. Please rename the static one
to make room. Maybe make it drm_edid_eq_buf() or something, because
that's what it really does.

"Check if the EDIDs are equal"

> + *
> + * @drm_edid_old: old drm_edid to compare edid
> + * @drm_edid_new: new drm_edid to compare edid

Old and new are meaningless here. It's supposed to be a generic
function. Just a/b or edid1/edid2 or something.

> + *
> + * Return true if the EDID is equal

"Return true if the EDIDs are equal."

> + */
> +bool drm_edid_is_edid_eq(const struct drm_edid *drm_edid_old,
> +			 const struct drm_edid *drm_edid_new)
> +{
> +	const void *old_edid = drm_edid_old->edid;
> +	size_t old_edid_size = drm_edid_old->size;

The existing drm_edid_eq() function supports the use case of either or
both EDIDs being NULL, and returning true for two NULL EDIDs. This one
oopses if the "old" EDID is NULL.

I'm not sure you can trivially replicate that behaviour by reusing the
existing function, though.

> +
> +	return drm_edid_eq(drm_edid_new, old_edid, old_edid_size);
> +}
> +EXPORT_SYMBOL(drm_edid_is_edid_eq);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index eaac5e665892..0e062761296c 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_is_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,

-- 
Jani Nikula, Intel