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

[Eric Anholt]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170202/d33fea61/attachment.sig>