[RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD

[Daniel Vetter]

On Thu, Feb 02, 2017 at 09:57:20AM -0800, Eric Anholt wrote:
> Daniel Vetter <daniel at ffwll.ch> writes:
> 
> > On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote:
> >> Jani Nikula <jani.nikula at linux.intel.com> writes:
> >> 
> >> > On Tue, 31 Jan 2017, Eric Anholt <eric at anholt.net> wrote:
> >> >> Martin Peres <martin.peres at linux.intel.com> writes:
> >> >>
> >> >>> Despite all the careful planing of the kernel, a link may become
> >> >>> insufficient to handle the currently-set mode. At this point, the
> >> >>> kernel should mark this particular configuration as being broken
> >> >>> and potentially prune the mode before setting the offending connector's
> >> >>> link-status to BAD and send the userspace a hotplug event. This may
> >> >>> happen right after a modeset or later on.
> >> >>>
> >> >>> When available, we should use the link-status information to reset
> >> >>> the wanted mode.
> >> >>>
> >> >>> Signed-off-by: Martin Peres <martin.peres at linux.intel.com>
> >> >>
> >> >> If I understand this right, there are two failure modes being handled:
> >> >>
> >> >> 1) A mode that won't actually work because the link isn't good enough.
> >> >>
> >> >> 2) A mode that should work, but link parameters were too optimistic and
> >> >> if we just ask the kernel to set the mode again it'll use more
> >> >> conservative parameters that work.
> >> >>
> >> >> This patch seems good for 2).  For 1), the drmmode_set_mode_major is
> >> >> going to set our old mode back.  Won't the modeset then fail to link
> >> >> train again, and bring us back into this loop?  The only escape that I
> >> >> see would be some other userspace responding to the advertised mode list
> >> >> changing, and then asking X to modeset to something new.
> >> >>
> >> >> To avoid that failure busy loop, should we re-fetching modes at this
> >> >> point, and only re-setting if our mode still exists?
> >> >
> >> > Disclaimer: I don't know anything about the internals of the modesetting
> >> > driver.
> >> >
> >> > Perhaps we can identify the two cases now, but I'd put this more
> >> > generally: if the link status has gone bad, it's an indicator to
> >> > userspace that the circumstances may have changed, and userspace should
> >> > query the kernel for the list of available modes again. It should no
> >> > longer trust information obtained prior to getting the bad link status,
> >> > including the current mode.
> >> >
> >> > But specifically, I think you're right, and AFAICT asking for the list
> >> > of modes again is the only way for the userspace to distinguish between
> >> > the two cases. I don't think there's a shortcut for deciding the current
> >> > mode is still valid.
> >> 
> >> To avoid the busy-loop problem, I think I'd like this patch to re-query
> >> the kernel to ask about the current mode list, and only try to re-set
> >> the mode if our mode is still there.
> >
> > Yeah, I guess that sounds like a reasonable heuristics. More integrated
> > compositors (aka the wayland ones) might be able to make more useful
> > decisions, but for -modesetting that's probably the best we can do.
> >  
> >> If the mode isn't there, then it's up to the DE to take action in
> >> response to the notification of new modes.  If you don't have a DE to
> >> take appropriate action, you're kind of out of luck.
> >> 
> >> As far as the ABI goes, this seems fine to me.  The only concern I had
> >> about ABI was having to walk all the connectors on every uevent to see
> >> if any had gone bad -- couldn't we have a flag of some sort about what
> >> the uevent indicates?  But uevents should be super rare, so I'd say the
> >> kernel could go ahead with the current plan.
> >
> > We've discussed finer-grained uevent singalling a few times, and I'd like
> > that to be an orthogonal abi extension. Right now we just don't have the
> > required tracking even within the kernel to know which connector changed
> > (and the tracking we do have missed a few things, too). My idea is to push
> > the tracking into the leaves of the callchains with a function that
> > increases some per-connector epoch counter. Then we'd just have to expose
> > that somehow cheaply to userspace (could probably just send it along in
> > the uevent). Plus expose it as a read-only property so that userspace can
> > re-synchronize.
> >
> > But again, that should be an orthogonal thing imo.
> 
> Yeah, that was roughly my thought process, too.

I merged the kernel side of this new uapi:

commit 40ee6fbef75fe6452dc9e69e6f9f1a2c7808ed67
Author: Manasi Navare <manasi.d.navare at intel.com>
Date:   Fri Dec 16 12:29:06 2016 +0200

    drm: Add a new connector atomic property for link status

Feel free to go ahead with the userspace side merging. (Yes the i915 side
isn't merged yet, but purely for "in which branch should I put it?"
technicality).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch