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

[Daniel Vetter]

On Tue, Jul 25, 2017 at 03:18:04PM +0300, Paul Kocialkowski wrote:
> On Tue, 2017-07-25 at 10:16 +0200, Daniel Vetter wrote:
> > On Tue, Jul 25, 2017 at 10:58:55AM +0300, Paul Kocialkowski wrote:
> > > On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> > > > 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@xxxxxxxx
> > > > > > > tel.
> > > > > > > com>
> > > > > > 
> > > > > > 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).
> > > 
> > > First off, I definitely agree we need a way to tell userspace
> > > exactly
> > > what has happened. I wanted to start a discussion about that in i-g-
> > > t
> > > patch "Unrelated hotplug uevent masking out actual test result" but
> > > it
> > > didn't get much traction. For testing purposes, it is unacceptable
> > > that
> > > userspace only gets notified that "something happened".
> > > 
> > > Still, as far as I know, userspace is expected to ask for a full
> > > reprobe
> > > when something has changed, and that is apparently part of the DRM
> > > spec,
> > > so we can't expect that it could query for an update on "only the
> > > things
> > > that changed".
> > 
> > We can update that spec in a backwards compatible way. E.g. we can ask
> > for
> > the current properties without forcing a reprobe (won't even call down
> > into the driver), and userspace could use that to check which
> > connector
> > has an incremented epoche counter since the last time it sampled
> > things.
> > Then it can reprobe just that one.
> > 
> > Old userspace wouldn't know about this, and would keep working as-is.
> 
> So the level of detail you're aiming at providing userspace is
> "connector foo changed" then? I agree it is better than the current
> "some connector(s) changed", but what I'd like to see for proper testing
> is a way to find out "bar for connector foo changed".

If you want taht level of detail you need introspection in a in-kernel
selftest I think. We'd need to rather massively change/extend the uapi to
support that level of testing through the uapi, and thus far no one else
is asking for it with a real use-case.

> > > However, one way to mitigate this is to make sure that the driver
> > > knows
> > > what changed and only updates these things when a full reprobe is
> > > requested. Is this the approach that you have in mind?
> > > 
> > > The methodology behind my series follows what is currently done:
> > > detect
> > > change in whatever way necessary, inform userspace and let it
> > > trigger
> > > full reprobe. If I'm understanding correctly, what you're suggesting
> > > is
> > > instead to reprobe what is needed on the kernel side when an
> > > associated
> > > change occurs instead of having userspace trigger it, and then let
> > > userspace aware that something changed and return the "cached"
> > > updated
> > > status when userspace asks for the subsequent reprobe. Is that
> > > correct?
> > 
> > There's two things: the uapi discussion and the internal
> > implementation,
> > imo their separate (but somewhat connected) topics.
> > 
> > - For the internal implementation of detecting edid changes I don't
> > like
> >   your approach of rolling a completely new detect path just for
> > resume.
> >   I think we can very well integrate that into the existing probe code
> >   using the approach I've laid out.
> > 
> > - There's more than just edid (e.g. hdcp status, various stuff that's
> >   handled in dp aux for DP sinks), and I think a general mechanism for
> >   tracking that something changed will be useful for the internal
> >   implementation. The other plan would be that we have to wire a bool
> >   changed through the entire probe stack, and make sure it's handled
> >   correctly everywhere, which is a) a lot more work b) more fragile.
> > Doing
> >   a connector->status_epoch++ everywhere we detect a change is _much_
> >   simpler.
> 
> So to summarize, the following would happen: an async worker would
> detect whether something changed, then increase the counter for that
> connector and notify userspace, which would trigger full reprobe of that
> connector only. Legacy userspace would just trigger full reprobe for all
> connectors.
> 
> I am still under the impression that you'd like the full reprobe to be
> done on the kernel's async worker, to detect that e.g. EDID changed. But
> then userspace is going to fully reprobe again, so it will be
> duplicated. Unless the kernel also keeps a reference of the last time
> the counter was read from userspace, to determine when to skip full
> reprobe when it is asked from userspace? That feels pretty similar to
> having a bool indicating change.
> 
> My approach here was to look specifically for the thing that can change
> in the async worker (only EDID with this change, but it could be
> extended for the other things you mentioned) as to reduce the
> duplication as much as possible.
> 
> > - For the uapi change: We already support returning the cached stuff,
> > the
> >   only bit that's missing is the epoch counter to let userspace know
> > where
> >   it might need to do a full reprobe. Or maybe we'll just spec that a
> > full
> >   reprobe isn't necessary after a hpd event (but that's unlikely to
> > work
> >   out given how many bugs we'd need to fix first).
> 
> Okay, thanks for the additional explanation. I think I'm getting a
> better grasp on your idea.

Yeah, the full reprobe isn't needed if you we spec that as the new fancy
behaviour. We already support getting the cached values through
drmModeGetConnectorCurrent(), without forcing a full reprobe.

Note that really the only thing the hpd handling doesn't do is fill and
filter the mode list. I guess as part of this work we should fix that,
that should take care of most of the needs for full reprobing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch