Re: [PATCH 1/2] drm/edid: Add helper to detect whether EDID changed

[Daniel Vetter]

On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
<paul.kocialkowski@xxxxxxxxxxxxxxx> wrote:
> On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
>> On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
>> > This adds a common drm helper to detect whether the EDID changed
>> > from
>> > the last known cached one. This is useful help detect that a monitor
>> > was
>> > changed during a suspend/resume cycle.
>> >
>> > When that happens (a monitor is replaced by another one during
>> > suspend),
>> > no hotplug event will be triggered so the change will not be caught
>> > at
>> > resume time. Detecting that the EDID changed allows detecting it.
>> >
>> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx>
>>
>> I can't find the older mails I've typed about this, but the plan we've
>> discussed a while back was:
>> - Add a generational counter to each connector, maybe even expose it
>> to
>>   userspace.
>>
>> - Increment that counter every time something changed, e.g.
>>   connector->status in the propbe helpers, or when attaching a new
>> edid
>>   with the set_edid helper.
>>
>> - Tada, no changes needed to drivers, and easily extensible to other
>>   things than edid!
>
> I don't see how it solves the problem here though. After a
> suspend/resume cycle, there is simply no indication that anything has
> changed when a monitor was replaced by another one, so I don't see how
> adding a counter in the mix would help.
>
> Could you provide more details about the reasoning? I feel like I'm
> missing something here.

Your bug doesn't just exist over s/r, it's just much easier to observe
in s/r since users can take however long they want to with plugging in
a different monitor. But the same issue exists e.g. when we go from
hpd to polling because too much noise on the line.

Wrt the suspend/resume issue: What we need to do on resume is do a
full reprobe of all outputs, in an async worker. Telling userspace to
do this by sending an uevent was the cheapest way, but it'd be better
if the kernel could do that asynchronously and inform userspace about
the exact changes. And there's more to reprobe than just the edid, and
we don't want to re-invent a separate reprobe path just for resume
like you start in your patch series. So yeah my plan was missing:

- force a full async reprobe after resume (maybe we could reuse the
poll worker for that as a one-shot).

Cheers, Daniel
>
> Cheers,
>
> Paul
>
>> > ---
>> >  drivers/gpu/drm/drm_edid.c | 45
>> > +++++++++++++++++++++++++++++++++++++++++++++
>> >  include/drm/drm_edid.h     |  3 +++
>> >  2 files changed, 48 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index 6bb6337be920..f6ce8bc2907a 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct
>> > drm_connector *connector,
>> >     }
>> >     return;
>> >  }
>> > +
>> > +/**
>> > + * drm_check_edid_changed - Check whether the EDID changed since
>> > the last update
>> > + * @connector: connector we're probing
>> > + * @adapter: I2C adapter to use for DDC
>> > + *
>> > + * Check whether the EDID changed since the last time it was
>> > updated in the
>> > + * drm blob cache.
>> > + *
>> > + * Return: A boolean indicating whether a change happened or not.
>> > + */
>> > +bool drm_check_edid_changed(struct drm_connector *connector,
>> > +                       struct i2c_adapter *adapter)
>> > +{
>> > +   struct drm_property_blob *edid_blob;
>> > +   struct edid *edid_stored;
>> > +   struct edid *edid_read;
>> > +   int ret = 0;
>> > +
>> > +   if (!connector->edid_blob_ptr)
>> > +           return false;
>> > +
>> > +   edid_blob = drm_property_blob_get(connector-
>> > >edid_blob_ptr);
>> > +   if (!edid_blob)
>> > +           return false;
>> > +
>> > +   if (!edid_blob->data || edid_blob->length != sizeof(struct
>> > edid))
>> > +           goto out;
>> > +
>> > +   edid_stored = (struct edid *) edid_blob->data;
>> > +
>> > +   edid_read = drm_get_edid(connector, adapter);
>> > +   if (!edid_read)
>> > +           goto out;
>> > +
>> > +   ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
>> > +
>> > +   kfree(edid_read);
>> > +
>> > +out:
>> > +   drm_property_blob_put(edid_blob);
>> > +
>> > +   return ret != 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
>> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > index 1e1908a6b1d6..593a97b269c3 100644
>> > --- a/include/drm/drm_edid.h
>> > +++ b/include/drm/drm_edid.h
>> > @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid
>> > *edid, char *name,
>> >  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>> >                                        int hsize, int vsize,
>> > int fresh,
>> >                                        bool rb);
>> > +bool drm_check_edid_changed(struct drm_connector *connector,
>> > +                       struct i2c_adapter *adapter);
>> > +
>> >  #endif /* __DRM_EDID_H__ */
>> > --
>> > 2.13.2
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
> --
> Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxxxxxx>
> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch