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

[Manasi Navare]

On Fri, Feb 03, 2017 at 01:30:14AM +0200, Martin Peres wrote:
> On 01/02/17 22:05, Manasi Navare wrote:
> >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.
> >>
> >>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.
> >Yes I agree. The kernel sets the link status BAD as soona s link training fails
> >but does not prune the modes until a new modelist is requested by the userspace.
> >So this patch should re query the mode list as soon as it sees the link status
> >BAD in order for the kernel to validate the modes again based on new link
> >paarmeters and send a new mode list back.
> 
> Seems like a bad behaviour from the kernel, isn't it? It should
> return immediatly
> if the mode is gonna be pruned :s

The kernel only knows that the link was bad because link training failed
so it sets the link status as BAD and sends a uevent expecting userspace
to take action. It will not prune the modes unless drm_helper_probe_single_connector_modes
is called by the userspace which would happen only when drmModeGetConnector call
is initiated by userspace. 

So in your function too to read the link status
you should do a drmModeGetConnector() that will probe the connector modes and 
validate and prune necessary modes, then if the link status is BAD handle it by two ways:

1. Modeset on the existing mode which will fail if the current mode was pruned already
2. If step 1 fails, then fetch the modes and this will be an updated mode list and
modeset on the first mode in the list.

This is how SNA driver implements this.

Danvet/Jani, please correct me if I am wrong and suggest if pruning should
be done by kernel instead before sending a uevent on link status BAD.

Regards
Manasi




> 
> With the behaviour you are talking about, I should see 2 uevents
> when injecting one
> BAD link-state (first uevent generates a new modeset that will then
> generate a BAD
> state and another uevent, but this time the mode will have been
> pruned so when
> -modesetting tries to set the mode, it will fail immediatly). During
> my testing, I do
> not remember seeing such behaviour, so the kernel seemed to be doing
> the right thing
> from my PoV (failing a modeset to a mode that is known not to be
> achievable). I can
> verify tommorow, but it would be nice to make sure it is part of the ABI.
> 
> As for re-fetching the modes, this is something the DE will do
> anyway when asking
> for them via randr. So, really, that will generate double probing in
> the common
> case for what seems to be a workaround. Given that probing can be a
> super expensive
> operation (request EDID from all monitors, potentially first
> starting up powered-down
> GPUs such as NVIDIA or AMD), I would say that probing should not be
> taken lightly.
> 
> Isn't it possible to just return an error from the kernel if the
> mode should disapear?
> As far as my testing goes, this was already what seemed to be
> happening... but I may be
> wrong, especially since my DP monitor was fine with no link training
> whatsoever. What
> is the current ABI for the userspace requesting a mode from a
> previous monitor to a new
> one, because it did not reprobe?
> 
> In any case, this is a good discussion to have!
> >Remember that it could still not prune any mode if the same mode is valid
> >with lower bpp, it will still keep the mode list same and when the
> >userspace retries the same mode, it will do a modeset at lower bpp (say 18bpp)
> >and still succeed. (Same mode at lower bpp still better than dropping the resolution)
> 
> Yes, this is the reason why I am doing the re-set of the mode ;)
> Otherwise, we would not
> need to do anything in there ;)
> 
> Martin
>