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@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@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@xxxxxxxxxxxxxxx> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
