[PATCH v3 0/3] Send a hotplug when edid changes
Daniel Vetter
daniel at ffwll.ch
Wed Sep 4 09:23:01 UTC 2019
On Wed, Sep 04, 2019 at 08:36:46AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-09-03 at 17:49 +0200, Daniel Vetter wrote:
> > On Tue, Sep 03, 2019 at 11:49:23AM +0000, Lisovskiy, Stanislav wrote:
> > > On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> > > >
> > > > > > In fact I was wrong - when it worked, it was using exactly
> > > > > > those
> > > > > > patches :). With clean drm-tip - it seems to work
> > > > > > ocassionally
> > > > > > and it
> > > > > > doesn't update the actual display edid and other stuff, so
> > > > > > even
> > > > > > when
> > > > > > displays are changed we still see the old info/edid from
> > > > > > userspace.
> > > > > >
> > > > > > We always get a hpd irq when suspend/resume however it
> > > > > > doesn't
> > > > > > always
> > > > > > result in uevent being sent. So there is a real need in those
> > > > > > patches.
> > > > > >
> > > > >
> > > > > Just decided to "ping" this discussion again. The issue is
> > > > > already
> > > > > some
> > > > > years old and still nothing is fixed. I do agree that may be
> > > > > something
> > > > > needs to be fixed/changed here in those patches, but something
> > > > > must
> > > > > be
> > > > > agreed at least I guess, as discussions themself do not fix
> > > > > bugs.
> > > > > Currently those patches address a particular issue which
> > > > > occurs, if
> > > > > display is changed during suspend.
> > > > > On ocassional basis, userspace might not get a hotplug event at
> > > > > all,
> > > > > causing different kind of problems(like wrong mode set on
> > > > > display
> > > > > or
> > > > > diaply not working at all). Also some kms_chamelium hotplug
> > > > > tests
> > > > > fail
> > > > > because of that.
> > > >
> > > > I still think we'll long-term regret this if we just duct-tape
> > > > more
> > > > stuff
> > > > on top, instead of giving userspace a more informative uevent.
> > > > This
> > > > will
> > > > send more uevents to userspace, so maybe then userspace tries to
> > > > filter
> > > > more and be clever, which never works, and we're back to tears.
> > >
> > > But here we actually do need a uevent as currently we don't get any
> > > at
> > > all, if edid changes during suspend. If userspace will try to
> > > filter
> > > this out - it's just stupid, however we still need to do things
> > > correctly.
> > >
> > > >
> > > > Anyway, on the approach itself: It's extremely i915 specific, and
> > > > it
> > > > requires that all drivers roll out drm_edid_equal checks and not
> > > > forget to
> > > > increment the epoch counter.
> > > >
> > > > What I had in mind is that when we set the edid for a connector
> > > > with
> > > > drm_connector_update_edid_property() or whatever, then the epoch
> > > > counter
> > > > would auto-increment if anything has changed. Similarly (long-
> > > > term
> > > > idea at
> > > > least) if anything important with DP registers has changed.
> > > >
> > > > Can't we do that, instead of this sub-optimal solution of
> > > > requiring
> > > > all
> > > > drivers to roll out lots of code?
> > >
> > > 1) We update edid in intel_dp_set_edid, which is called from
> > > intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which
> > > is
> > > called from drm_helper_probe_detect. That one is called either from
> > > specific intel_encoder->hotplug hook in i915_hotplug_work_func or
> > > by
> > > userspace request during reprobe.
> > >
> > > 2) Previously we were simply updating edid in intel_dp_set_edid
> > > without
> > > caring if it is the same or not and hotplug event was sent only
> > > once
> > > connection_status had changed.
> > >
> > > 3) drm_connector_update_edid_property is called from connector-
> > > > get_modes hook(lets say intel_dp_get_modes fo dp) however it
> > > > simply
> > >
> > > uses results of
> > > drm_helper_probe_detect so without actual comparison it would not
> > > be
> > > able to detect if we really need to update epoch_counter or not.
> > >
> > > Because as I said currently intel_dp_set_edid simply assigns it
> > > without
> > > checking, so that way you will get epoch_counter updated every
> > > time,
> > > i.e exactly what you wanted to avoid here.
> > >
> > > So we really need someway to determine if edid had changed, instead
> > > of
> > > simply assigning it all the time - that is why I had to make this
> > > function.
> >
> > update_edid is the thing which changes the userspace visible edid. We
> > can
> > check there with the previous userspace visible edid, and if it's
> > different, increase the epoch counter. If we never call update_edid
> > then
> > userspace won't see the changed edid (it might see the changed mode
> > list
> > or whatever), so doing that is pretty much a requirement for
> > correctness.
> >
> > The higher levels should notice the epoch counter change (you might
> > not
> > have captured all of them, there's a bunch between ioctl, poll worker
> > and
> > sysfs iirc), and send out the uevent.
> >
> > Why does that not work?
>
> Sure this will work, but still we need somehow to be able to determine
> this "if it's different" state. In your solution we just move that
> comparison to drm_connector_update_edid_property, which is quite fine
> for me.
Yes we need to compare edid somewhere, that much is clear. I'm not
disputing that. I just want something where we don't have to roll this out
over all the drivers, because that's a hopeless endeavour.
> I would say that yes, this idea may be is even better because
> drivers won't need to implement this comparison in encoder->hotplug in
> each driver.
> However:
> we still need a comparison in
> drm_connector_update_edid_property(drm_edid_equal) and also I'm not
> sure we can send a hotplug event based on that as
> drm_connector_update_edid_property seems
> to get called only during connector init or during reprobe from
> userspace from connector->get_modes hook.
> Also it is called from drm_kms_helper_hotplug_event from, but this one
> is called from i915 only if connection status had changed.
So ditch the optimization to only call ->get_modes when called from
userspace? We've been talking about this one too in the past ...
I'd really like a solution where it will work for most drivers out of the
box.
-Daniel
>
> - Stanislav
>
>
> > -Daniel
> >
> > >
> > > Cheers,
> > >
> > > Stanislav
> > >
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > - Stanislav
> > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > -Stanislav
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Cheers, Daniel
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > -Stanislav
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > -Daniel
> > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > > > > drm: Add helper to compare edids.
> > > > > > > > > > > > > > drm: Introduce change counter to
> > > > > > > > > > > > > > drm_connector
> > > > > > > > > > > > > > drm/i915: Send hotplug event if edid had
> > > > > > > > > > > > > > changed.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > drivers/gpu/drm/drm_connector.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 1 +
> > > > > > > > > > > > > > drivers/gpu/drm/drm_edid.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 33
> > > > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > > > > drivers/gpu/drm/drm_probe_helper.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 29
> > > > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > > > -
> > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > +++++++++-
> > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_hdmi.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > ++++++++--
> > > > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 21
> > > > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > include/drm/drm_connector.h
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 3 ++
> > > > > > > > > > > > > > include/drm/drm_edid.h
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 9
> > > > > > > > > > > > > > ++++++
> > > > > > > > > > > > > > 8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel at lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> >
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list