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@xxxxxxxxxxxxxxx> writes: > >> > >>>On Tue, 31 Jan 2017, Eric Anholt <eric@xxxxxxxxxx> wrote: > >>>>Martin Peres <martin.peres@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > >>>>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 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
