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

[Lisovskiy, Stanislav]

On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> On Wed, Aug 07, 2019 at 07:43:18AM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote:
> > > On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
> > > <stanislav.lisovskiy at intel.com> wrote:
> > > > 
> > > > On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> > > > > On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy
> > > > > wrote:
> > > > > > This series introduce to drm a way to determine if
> > > > > > something
> > > > > > else
> > > > > > except connection_status had changed during probing, which
> > > > > > can be used by other drivers as well. Another i915 specific
> > > > > > part
> > > > > > uses this approach to determine if edid had changed without
> > > > > > changing the connection status and send a hotplug event.
> > > > > 
> > > > > Did you read through the entire huge thread on all this (with
> > > > > I
> > > > > think
> > > > > Paul, Pekka, Ram and some others)? I feel like this is mostly
> > > > > taking
> > > > > that
> > > > > idea, but not taking a lot of the details we've discussed
> > > > > there.
> > > > > Specifically I'm not sure how userspace should handle this
> > > > > without
> > > > > also
> > > > > exposing the epoch counter, or at least generating a hotplug
> > > > > event
> > > > > for the
> > > > > specific connector. All that and more we discussed there.
> > > > > 
> > > > > And then there's the follow-up question: What's the userspace
> > > > > for
> > > > > this?
> > > > > Existing expectations are a minefield here, so even if you
> > > > > don't
> > > > > plan
> > > > > to
> > > > > change userspace we need to know what this is aimed for, and
> > > > > see
> > > > > above I
> > > > > don't think this is possible to use without userspace changes
> > > > > ...
> > > > 
> > > > Yes, sure I agree about userspace. However I guess we must
> > > > start
> > > > from
> > > > something at least.
> > > > I think I have seen some discussion regarding exposing this
> > > > epoch
> > > > counter as a property. Was even implementing this at some point
> > > > but
> > > > didn't dare to send to mailing list :)
> > > > 
> > > > My idea was just to expose this epoch counter as a drm
> > > > property, to
> > > > let
> > > > userspace then to figure out, what has happened, as imho adding
> > > > different events for this and that is a bit of an overkill and
> > > > brings
> > > > additional complexity..
> > > 
> > > Adding Ram and link to the (warning, huge!) thread:
> > > 
> > > 
https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9
> > > 
> > > > However, please let me know, what do you think we should do for
> > > > userspace.
> > > 
> > > That seems backwards, since this is an uapi change I'd expect
> > > you're
> > > solving some specific issue in some specific userspace? If we're
> > > doing
> > > this just because I'm not really following.
> > 
> > Specifically, I'm solving an issue of changed edid during suspend,
> > like
> > we for example have in kms_chamelium hotplug tests(some of which
> > now
> > fail because of that). 
> > So even if connection status hasn't changed, we still need to send
> > hotplug event and userspace needs to be able to understand that
> > something had changed and whether we need to do a full reprobe or
> > not.
> > Epoch counter property would be responsible for this.
> > As I understand in general there might be other connector updates,
> > except edid which we need propagate in a similar way.
> 
> 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.

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