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@linux.intel. 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.
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.
- 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).
Cheers, Daniel