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
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
