[PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Tue, Sep 03, 2019 at 09:17:49AM +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2019-08-19 at 09:05 +0000, Lisovskiy, Stanislav wrote:
> > On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote:
> > > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote:
> > > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > 
> > > > > So igt isn't valid userspace (it's just good testcases). Can we
> > > > > repro
> > > > > the
> > > > > same on real userspace? Does this work with real userspace?
> > > > > We've
> > > > > had
> > > > > userspace which tries to be clever and filters out what looks
> > > > > like
> > > > > redundant hotplug events. And then gets it wrong in cases like
> > > > > this.
> > > > > 
> > > > > Also, we've had forever an unconditional uevent on resume,
> > > > > exactly
> > > > > because
> > > > > anything could have changed. Did we loose this one on the way
> > > > > somewhere?
> > > > > Or maybe I misremember ...
> > > > > 
> > > > > If all we care about is resume re-adding that uncondtional
> > > > > uevent
> > > > > on
> > > > > resume is going to be a lot easier than this here.
> > > > > -Daniel
> > > > 
> > > > Sorry for long reply(was on vacation), that is a good question
> > > > regarding reproducing this in real life scenario. My obvious
> > > > guess
> > > > was to suspend the machine and meanwhile change connected display
> > > > to
> > > > another one. However this situation seems to be already handled
> > > > by
> > > > kernel nicely(tried few times and we always get a hotplug event).
> > > > So
> > > > that edid change during suspend chamelium test case seems to be
> > > > a bit different. I will talk to our guys who wrote this about
> > > > what
> > > > is
> > > > the real life scenario for this, because I'm now curious as well.
> > > 
> > > Thanks Daniel for the feedback.
> > > 
> > > I also now wonder why our IGT test (chamelium-based) does not pass
> > > if
> > > a
> > > uevent is sent on resume automatically and all the test is
> > > expecting
> > > is
> > > a uevent...
> > > 
> > > Martin
> > 
> > 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.

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?
-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