Message-ID: <1501081778.13950.4.camel@linux.intel.com>
Subject: Re: [PATCH 1/2] drm/edid: Add helper to detect whether EDID changed
From: Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        intel-gfx <intel-gfx@lists.freedesktop.org>,
        Daniel Vetter <daniel.vetter@intel.com>
Date: Wed, 26 Jul 2017 18:09:38 +0300
In-Reply-To: <20170725155013.43mtpbpbpq26ywsh@phenom.ffwll.local>
References: <20170724145447.23449-1-paul.kocialkowski@linux.intel.com>
         <20170725065328.fhe75lpunqn7vcqv@phenom.ffwll.local>
         <1500967544.1277.1.camel@linux.intel.com>
         <CAKMK7uFB_y_WU36brL4Mor6mxNosu34GnLNbUWkwicez4Ff_=A@mail.gmail.com>
         <1500969535.1277.3.camel@linux.intel.com>
         <20170725081652.j43grc7oeyitr4tl@phenom.ffwll.local>
         <1500985084.1277.7.camel@linux.intel.com>
         <20170725155013.43mtpbpbpq26ywsh@phenom.ffwll.local>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160
 Espoo, Finland
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 10958
Lines: 289

On Tue, 2017-07-25 at 17:50 +0200, Daniel Vetter wrote:
> 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@linux.intel.com> 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@linu
> > > > > > > > x.in
> > > > > > > > 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.

I'm frankly surprised that no one has complained about this yet!

The current level of (lack of) detail about what is happening when a
hotplug event is triggered makes testing for various things really hard.
But I suppose it makes sense with the "reprobe everything all the time"
approach ;)

For instance, it wouldn't hurt to add a REASON=foo field to the uevent,
in addition to HOTPLUG=1, which would most certainly maintain
compatibility with current userspace. I'm not sure that would work out
too well if uevents are to be stacked together though.

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

So then userspace gets the cached values and considers them as "already
updated" if the counter has increased, and then stops there and doesn't
trigger a full reprobe (which was already done by the kernel's worker)?

So all in all, the win here is reprobing only 1 connector instead of
reprobing them all. The same could be achieved via a more detail hotplug
notification that also mentions which connector was concerned by the
event.

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

Either way, I do not have enough time left in my internship to start
working on something as big as this change, but I think it's a step in
the right direction.

-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland