[Intel-gfx] [PATCH v2 1/3] drm: Add helper to compare edids.

[Lyude Paul]

Sorry for the late response! I like the idea here and I've brought up edid
comparison a couple times. Hopefully this isn't overkill, but I had a little
more in mind then just a helper like this (and I've had this on my mind for a
while!

When it comes to suspend/resume reprobing, I think there's more work then just
comparing edids that are redundant. I think most drivers have connectors that
fall into one of the following classes:

 * Always "in-sync" with events, e.g. event handling does not stop just
   because the device is suspended. This is actually true in some cases for
   nouveau and amdgpu, where both drivers can rely on ACPI firmware to send
   events while the device is suspended.
 * Only in-sync with events while the device is awake. Of course, that's the
   whole reason for these patches!

>From what I understand based on previous discussions with some other intel
engineers back when they were trying to make the i915 suspend/resume process
faster, hotplug probing can be a pretty significant timesink in some cases.
Additionally, it's not always nessecarily everywhere if some connectors are
able to stay in sync, and that might be a benefit.

Additionally, I think there's a number of other parts of the process that I
would imagine every driver would end up needing to implement. A couple rather
simple examples: skipping edid comparisons for disconnected or newly-connected
connectors, assuming that failure to read an EDID on a connected connector
that could have it's EDID read before suspend means we have to consider said
connector to be changed, etc. So why not add helpers to handle all of this
boilerplate as well?

An idea I had at one point would be to add the ability to mark when a driver
believes a DRM connector has gone "out of sync" like I mentioned above. A
simple example: on laptops I've observed with nouveau that supported ACPI
hotplug events, ACPI hotplug events only ever seemed to come if a connector
was plugged in - not if a connector was removed. If we use this logic during
the runtime resume, we could ascertain that unless an ACPI hotplug event was
received before we resumed that we can actually skip reprobing any connectors
that were disconnected at runtime suspend and only probe the ones that were
connected.

So, maybe we could have helpers like this:

/* Inform DRM that we've disabled our primary means of receiving HPD
 * events.
 */
drm_connector_suspend_hpd()

/* A helper that goes through and performs basic connector reprobing and
 * EDID comparisons on connectors marked with
 * drm_connector_reprobe_on_hpd_resume(). Fires off an HPD event if any
 * connector changes are found/returns an int/etc. etc. whatever.
 *
 * To be called by the driver *after* it has re-enabled HPD detection
 * for all of it's connectors.
 */
drm_connector_resume_hpd()

/* Indicate to DRM that the given connector no longer has HPD, and will need
 * to be reprobed on resume
 */
drm_connector_reprobe_on_hpd_resume()

/* Convienence function to indicate to DRM that all connectors have lost
 * their primary means to receive HPD events, and will need to be
 * reprobed on resume. Useful for scenarios like S3 suspend.
 */
drm_connector_reprobe_all_on_hpd_resume()

I think this would also fit in nicely with the new hotplug uevent ideas
that have been floating around recently, since we could then use these
helpers to compress all of the connector changes that happened over a
s/r cycle into a single event (or, no event!)

On Fri, 2019-06-28 at 11:24 +0300, Stanislav Lisovskiy wrote:
> Many drivers would benefit from using
> drm helper to compare edid, rather
> than bothering with own implementation.
> 
> v2: Added documentation for this function.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  9 +++++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 9d8f2b952004..eaad5155fbdd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1361,6 +1361,39 @@ static bool drm_edid_is_zero(const u8 *in_edid, int
> length)
>  	return true;
>  }
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
> + * This helper can be used during probing to determine if
> + * edid had changed.
> + */
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2)
> +{
> +	int edid1_len, edid2_len;
> +	bool edid1_present = edid1 != NULL;
> +	bool edid2_present = edid2 != NULL;
> +
> +	if (edid1_present != edid2_present)
> +		return false;
> +
> +	if (edid1) {
> +
> +		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
> +		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
> +
> +		if (edid1_len != edid2_len)
> +			return false;
> +
> +		if (memcmp(edid1, edid2, edid1_len))
> +			return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_edid_are_equal);
> +
> +
>  /**
>   * drm_edid_block_valid - Sanity check the EDID block (base or extension)
>   * @raw_edid: pointer to raw EDID block
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index b9719418c3d2..716964f63312 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -354,6 +354,15 @@ drm_load_edid_firmware(struct drm_connector *connector)
>  }
>  #endif
>  
> +/**
> + * drm_edid_are_equal - compare two edid blobs.
> + * @edid1: pointer to first blob
> + * @edid2: pointer to second blob
> + * This helper can be used during probing to determine if
> + * edid had changed.
> + */
> +bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2);
> +
>  int
>  drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>  					 struct drm_connector *connector,
-- 
Cheers,
	Lyude Paul