modesetting: re-set the crtc's mode when link-status goes BAD

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 <[email protected]>

---

WARNING: The patches have not been merged in the kernel yet, so this patch is merely an RFC.

This patch is the result of discussions happening mostly in DRI-devel and Intel-GFX on how to handle link training failures. I would advise reading the thread [0] first and then this thread [1] which explain in great length why this is needed and why the selected approach seems to be the best.

The relevant kernel patches + this patch are enough to pass the relevant DisplayPort compliance tests, provided that the Desktop Environment or another program ([2]?) provides the initial modesetting on hotplug.

Would this patch be acceptable to you? Any comments or suggestions?

[0] https://lists.freedesktop.org/archives/dri-devel/2016-November/123366.html
[1] https://lists.freedesktop.org/archives/dri-devel/2016-November/124736.html
[2] https://cgit.freedesktop.org/~mperes/auto-randr/

 hw/xfree86/drivers/modesetting/drmmode_display.c | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

hw/xfree86/drivers/modesetting/drmmode_display.c +51 -0

index 6e755e9482..3144620c67 100644
@@ -2262,6 +2262,10 @@ drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn)
 }
 
 #ifdef CONFIG_UDEV_KMS
+
+#define DRM_MODE_LINK_STATUS_GOOD       0
+#define DRM_MODE_LINK_STATUS_BAD        1
+
 static void
 drmmode_handle_uevents(int fd, void *closure)
 {

@@ -2281,6 +2285,49 @@ drmmode_handle_uevents(int fd, void *closure)
     if (!found)
         return;
 
+    /* Try to re-set the mode on all the connectors with a BAD link-state:
+     * This may happen if a link degrades and a new modeset is necessary, using
+     * different link-training parameters. If the kernel found that the current
+     * mode is not achievable anymore, it should have pruned the mode before
+     * sending the hotplug event. Try to re-set the currently-set mode to keep
+     * the display alive, this will fail if the mode has been pruned.
+     * In any case, we will send randr events for the Desktop Environment to
+     * deal with it, if it wants to.
+     */
+    for (i = 0; i < config->num_output; i++) {
+        xf86OutputPtr output = config->output[i];
+        drmmode_output_private_ptr drmmode_output = output->driver_private;
+        uint32_t con_id = drmmode_output->mode_output->connector_id;
+        drmModeConnectorPtr koutput;
+
+        /* Get an updated view of the properties for the current connector and
+         * look for the link-status property
+         */
+        koutput = drmModeGetConnectorCurrent(drmmode->fd, con_id);
+        for (j = 0; koutput && j < koutput->count_props; j++) {
+            drmModePropertyPtr props;
+            props = drmModeGetProperty(drmmode->fd, koutput->props[j]);
+            if (props && props->flags & DRM_MODE_PROP_ENUM &&
+                !strcmp(props->name, "link-status") &&
+                koutput->prop_values[j] == DRM_MODE_LINK_STATUS_BAD) {
+                xf86CrtcPtr crtc = output->crtc;
+                if (!crtc)
+                    continue;
+
+                /* the connector got a link failure, re-set the current mode */
+                drmmode_set_mode_major(crtc, &crtc->mode, crtc->rotation,
+                                       crtc->x, crtc->y);
+
+                xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+                           "hotplug event: connector %u's link-state is BAD, "
+                           "tried resetting the current mode. You may be left"
+                           "with a black screen if this fails...\n", con_id);
+            }
+            drmModeFreeProperty(props);
+        }
+        drmModeFreeConnector(koutput);
+    }
+
     mode_res = drmModeGetResources(drmmode->fd);
     if (!mode_res)
         goto out;

@@ -2345,6 +2392,10 @@ out_free_res:
 out:
     RRGetInfo(xf86ScrnToScreen(scrn), TRUE);
 }
+
+#undef DRM_MODE_LINK_STATUS_BAD
+#undef DRM_MODE_LINK_STATUS_GOOD
+
 #endif
 
 void

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 13 lines · Martin Peres

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 <[email protected]>

---

Quoted 2 lines

WARNING: The patches have not been merged in the kernel yet, so this patch is merely an RFC.

That's how it's supposed to happen, before we can merge the kernel side, we need acceptance (=reviewed) by the userspace side. Which is why this patch here.
-Daniel

Quoted 95 lines

This patch is the result of discussions happening mostly in DRI-devel and Intel-GFX on how to handle link training failures. I would advise reading the thread [0] first and then this thread [1] which explain in great length why this is needed and why the selected approach seems to be the best.

The relevant kernel patches + this patch are enough to pass the relevant DisplayPort compliance tests, provided that the Desktop Environment or another program ([2]?) provides the initial modesetting on hotplug.

Would this patch be acceptable to you? Any comments or suggestions?

[0] https://lists.freedesktop.org/archives/dri-devel/2016-November/123366.html
[1] https://lists.freedesktop.org/archives/dri-devel/2016-November/124736.html
[2] https://cgit.freedesktop.org/~mperes/auto-randr/

 hw/xfree86/drivers/modesetting/drmmode_display.c | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

hw/xfree86/drivers/modesetting/drmmode_display.c +51 -0

index 6e755e9482..3144620c67 100644
@@ -2262,6 +2262,10 @@ drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn)
 }
 
 #ifdef CONFIG_UDEV_KMS
+
+#define DRM_MODE_LINK_STATUS_GOOD       0
+#define DRM_MODE_LINK_STATUS_BAD        1
+
 static void
 drmmode_handle_uevents(int fd, void *closure)
 {

@@ -2281,6 +2285,49 @@ drmmode_handle_uevents(int fd, void *closure)
     if (!found)
         return;
 
+    /* Try to re-set the mode on all the connectors with a BAD link-state:
+     * This may happen if a link degrades and a new modeset is necessary, using
+     * different link-training parameters. If the kernel found that the current
+     * mode is not achievable anymore, it should have pruned the mode before
+     * sending the hotplug event. Try to re-set the currently-set mode to keep
+     * the display alive, this will fail if the mode has been pruned.
+     * In any case, we will send randr events for the Desktop Environment to
+     * deal with it, if it wants to.
+     */
+    for (i = 0; i < config->num_output; i++) {
+        xf86OutputPtr output = config->output[i];
+        drmmode_output_private_ptr drmmode_output = output->driver_private;
+        uint32_t con_id = drmmode_output->mode_output->connector_id;
+        drmModeConnectorPtr koutput;
+
+        /* Get an updated view of the properties for the current connector and
+         * look for the link-status property
+         */
+        koutput = drmModeGetConnectorCurrent(drmmode->fd, con_id);
+        for (j = 0; koutput && j < koutput->count_props; j++) {
+            drmModePropertyPtr props;
+            props = drmModeGetProperty(drmmode->fd, koutput->props[j]);
+            if (props && props->flags & DRM_MODE_PROP_ENUM &&
+                !strcmp(props->name, "link-status") &&
+                koutput->prop_values[j] == DRM_MODE_LINK_STATUS_BAD) {
+                xf86CrtcPtr crtc = output->crtc;
+                if (!crtc)
+                    continue;
+
+                /* the connector got a link failure, re-set the current mode */
+                drmmode_set_mode_major(crtc, &crtc->mode, crtc->rotation,
+                                       crtc->x, crtc->y);
+
+                xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+                           "hotplug event: connector %u's link-state is BAD, "
+                           "tried resetting the current mode. You may be left"
+                           "with a black screen if this fails...\n", con_id);
+            }
+            drmModeFreeProperty(props);
+        }
+        drmModeFreeConnector(koutput);
+    }
+
     mode_res = drmModeGetResources(drmmode->fd);
     if (!mode_res)
         goto out;

@@ -2345,6 +2392,10 @@ out_free_res:
 out:
     RRGetInfo(xf86ScrnToScreen(scrn), TRUE);
 }
+
+#undef DRM_MODE_LINK_STATUS_BAD
+#undef DRM_MODE_LINK_STATUS_GOOD
+
 #endif
 

Quoted 8 lines

hw/xfree86/drivers/modesetting/drmmode_display.c +0 -0

 void

Signature · 6 lines

2.11.0

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 3 lines

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

Quoted 14 lines · Daniel Vetter

Quoted 13 lines · Martin Peres

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 <[email protected]>

---

Quoted 8 lines

Quoted 2 lines

WARNING: The patches have not been merged in the kernel yet, so this patch is merely an RFC.

That's how it's supposed to happen, before we can merge the kernel side, we need acceptance (=reviewed) by the userspace side. Which is why this patch here.
-Daniel

This has been validated with a compliance device inducing a forced link failure during modeset, after which the kernel sets the link-status to BAD and sends a hotplug to userspace. The -modesetting driver then resets the configuration by forcing another mdoeset and retraining the link at lower link rate/lane count. This has been proven to pass DP compliance. It has been also verified that this avoids the black screens in case of such mode failures due to link training failure.

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 <[email protected]>

Quoted 11 lines · Martin Peres

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 <[email protected]>

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?

Quoted 22 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 42 lines · Jani Nikula

Quoted 22 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 54 lines · Eric Anholt

Quoted 40 lines · Jani Nikula

Quoted 21 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

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)

Quoted 60 lines · Manasi Navare

Quoted 54 lines · Eric Anholt

Quoted 40 lines · Jani Nikula

Quoted 21 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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!

Quoted 4 lines

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

Quoted 60 lines · Manasi Navare

Quoted 54 lines · Eric Anholt

Quoted 40 lines · Jani Nikula

Quoted 21 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 5 lines

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

Quoted 66 lines · Martin Peres

Quoted 65 lines · Manasi Navare

Quoted 54 lines · Eric Anholt

Quoted 40 lines · Jani Nikula

Quoted 21 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · Martin Peres

Quoted 83 lines · Manasi Navare

Quoted 67 lines · Eric Anholt

Quoted 49 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Did I miss anything? Any opinions?

Quoted 131 lines · Martin Peres

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · Martin Peres

Quoted 83 lines · Manasi Navare

Quoted 67 lines · Eric Anholt

Quoted 49 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <martin.peres-VuQAYsv1563Yd54FQh9/[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

Quoted 132 lines · Martin Peres

Quoted 131 lines · Martin Peres

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · Martin Peres

Quoted 83 lines · Manasi Navare

Quoted 67 lines · Eric Anholt

Quoted 49 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 8 lines

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

Quoted 132 lines · Martin Peres

Quoted 131 lines · Martin Peres

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · Martin Peres

Quoted 83 lines · Manasi Navare

Quoted 67 lines · Eric Anholt

Quoted 49 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 8 lines

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

Hi Daniel,

We have ACKs on the userspace design from both Adams and Eric. Is this enough to merge the kernel patches?

I spoke to Eric briefly about this and he gave me a verbal r-b as well. He said the userspace patches cant get merged unless DRM patches are merged.

So what should be some of our next steps here?

Regards
Manasi

Quoted 148 lines · Eric Anholt

Quoted 132 lines · Martin Peres

Quoted 131 lines · Martin Peres

Quoted 85 lines · Daniel Vetter

Quoted 84 lines · Martin Peres

Quoted 83 lines · Manasi Navare

Quoted 67 lines · Eric Anholt

Quoted 49 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 8 lines

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

Hi Daniel,

We have ACKs on the userspace design from both Adams and Eric. Is this enough to merge the kernel patches?

I spoke to Eric briefly about this and he gave me a verbal r-b as well. He said the userspace patches cant get merged unless DRM patches are merged.

So what should be some of our next steps here?

Regards
Manasi

Quoted 146 lines · Eric Anholt

Quoted 130 lines · Martin Peres

Quoted 129 lines · Martin Peres

Quoted 83 lines · Daniel Vetter

Quoted 82 lines · Martin Peres

Quoted 81 lines · Manasi Navare

Quoted 65 lines · Eric Anholt

Quoted 48 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 8 lines

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

Quoted 8 lines · Manasi Navare

Hi Daniel,

We have ACKs on the userspace design from both Adams and Eric. Is this enough to merge the kernel patches?

I spoke to Eric briefly about this and he gave me a verbal r-b as well. He said the userspace patches cant get merged unless DRM patches are merged.

Quoted 1 line

So what should be some of our next steps here?

Still needs review on the kernel patches themselves, iirc Jani still had some opens on that. But I was out of the loop for 2 weeks :-) -Daniel

Thanks for merging the 1st patch in the kernel series. Are there any opens on the 2nd patch (the i915 patch)?
I wanted to actually respin the 2nd patch to add reset for intel_dp->lane_count on the link training failure so that it doesn't accidently retrain the link with the stale/failed values.

Regards
Manasi

Quoted 154 lines

Regards
Manasi

Quoted 146 lines · Eric Anholt

Quoted 130 lines · Martin Peres

Quoted 129 lines · Martin Peres

Quoted 83 lines · Daniel Vetter

Quoted 82 lines · Martin Peres

Quoted 81 lines · Manasi Navare

Quoted 65 lines · Eric Anholt

Quoted 48 lines · Jani Nikula

Quoted 33 lines · Eric Anholt

Quoted 8 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

Seems like a bad behaviour from the kernel, isn't it? It should return immediatly
if the mode is gonna be pruned :s

The mode list pruning isn't relevant for modeesets, the kernel doesn't validate requested modes against that. The mode list is purely for userspace's information. Which means updating it only when userspace asks for it is fine.

Hmm, ok. Fair enough!

Quoted 9 lines

I also thought some more about the loop when we're stuck on BAD, and I think it shouldn't happen: When the link goes BAD we update the link parameter values to the new limits, and the kernel will reject any mode that won't fit anymore. Of course you might be unlucky, and the new link limits are also not working, but eventually you're stuck at the "you're link is broken, sry" stage, where the kernel rejects everything :-)

So I think the busy-loop has strictly a limited amount of time until it runs out of steam.

OK, I have given it more thoughts and discussed with Ville and Chris IRL or on IRC.

My current proposal is based on the idea that the kernel should reject a mode
it knows it cannot set. This is already largely tested in real life: Try setting 4K@60Hz on an HDMI 1.x monitor, it should immediately fail on prepare(). For this proposal to work, we would need to put in the ABI that a
driver that sets the link-status to BAD should also make sure that whatever the userspace does, no infinite loop should be created (by changing the maximum link characteristics before setting the link-status property).

Re-probing the list of modes and checking if the mode is still in there is inherently racy, as a new screen may be plugged between the moment the list is sent to the userspace and the moment when we try setting the mode. Or the
DE may also have changed the mode in the mean time and the kernel would have reduced the limits even more. So, the userspace cannot be expected to always do the right thing here :s.

From this point of view, I really do not like the idea of re-probing, since it increases the delay before the DE can handle the change and it does not bring any guarantee of working properly.

Quoted 8 lines

Quoted 1 line

Did I miss anything? Any opinions?

Any comments on this, Eric? Does it sound logical to you or did I miss something?

The kernel patches are stuck until this patch gets in. So far, you look like the only person who would be willing to review this patch (Ajax acked the patch, but that's the extent he was willing to go).

I was just trying to provide review to get the kernel unstuck. The kernel should not be blocked until the patch gets lands (this obviously isn't the case with ioctls, which *don't* land in userspace until kernel does), you just need userspace published and generally agreed that the ABI works.

Signature · 3 lines

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

Quoted 42 lines · Jani Nikula

Quoted 22 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 44 lines · Eric Anholt

Quoted 42 lines · Jani Nikula

Quoted 22 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 78 lines · Daniel Vetter

Quoted 44 lines · Eric Anholt

Quoted 42 lines · Jani Nikula

Quoted 22 lines · Eric Anholt

Quoted 11 lines · Martin Peres

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 <[email protected]>

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.

Quoted 8 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 1 line

But again, that should be an orthogonal thing imo.

Yeah, that was roughly my thought process, too.

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 <[email protected]>

---

WARNING: The patches have not been merged in the kernel yet, so this patch is merely an RFC.

This patch is the result of discussions happening mostly in DRI-devel and Intel-GFX on how to handle link training failures. I would advise reading the thread [0] first and then this thread [1] which explain in great length why this is needed and why the selected approach seems to be the best.

The relevant kernel patches + this patch are enough to pass the relevant DisplayPort compliance tests, provided that the Desktop Environment or another program ([2]?) provides the initial modesetting on hotplug.

Would this patch be acceptable to you? Any comments or suggestions?

[0] https://lists.freedesktop.org/archives/dri-devel/2016-November/123366.html
[1] https://lists.freedesktop.org/archives/dri-devel/2016-November/124736.html
[2] https://cgit.freedesktop.org/~mperes/auto-randr/

 hw/xfree86/drivers/modesetting/drmmode_display.c | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

hw/xfree86/drivers/modesetting/drmmode_display.c +4 -0

index 6e755e9482..3144620c67 100644
@@ -2262,6 +2262,10 @@ drmmode_setup_colormap(ScreenPtr pScreen, ScrnInfoPtr pScrn)
 }
 
 #ifdef CONFIG_UDEV_KMS
+
+#define DRM_MODE_LINK_STATUS_GOOD       0
+#define DRM_MODE_LINK_STATUS_BAD        1
+
 static void
 drmmode_handle_uevents(int fd, void *closure)
 {

hw/xfree86/drivers/modesetting/drmmode_display.c +24 -0

+     * sending the hotplug event. Try to re-set the currently-set mode to keep
+     * the display alive, this will fail if the mode has been pruned.
+     * In any case, we will send randr events for the Desktop Environment to
+     * deal with it, if it wants to.
+     */
+    for (i = 0; i < config->num_output; i++) {
+        xf86OutputPtr output = config->output[i];
+        drmmode_output_private_ptr drmmode_output = output->driver_private;
+        uint32_t con_id = drmmode_output->mode_output->connector_id;
+        drmModeConnectorPtr koutput;
+
+        /* Get an updated view of the properties for the current connector and
+         * look for the link-status property
+         */
+        koutput = drmModeGetConnectorCurrent(drmmode->fd, con_id);
+        for (j = 0; koutput && j < koutput->count_props; j++) {
+            drmModePropertyPtr props;
+            props = drmModeGetProperty(drmmode->fd, koutput->props[j]);
+            if (props && props->flags & DRM_MODE_PROP_ENUM &&
+                !strcmp(props->name, "link-status") &&
+                koutput->prop_values[j] == DRM_MODE_LINK_STATUS_BAD) {
+                xf86CrtcPtr crtc = output->crtc;
+                if (!crtc)
+                    continue;

hw/xfree86/drivers/modesetting/drmmode_display.c +9 -0

+            drmModeFreeProperty(props);
+        }
+        drmModeFreeConnector(koutput);
+    }
+
     mode_res = drmModeGetResources(drmmode->fd);
     if (!mode_res)
         goto out;

@@ -2345,6 +2392,10 @@ out_free_res:
 out:
     RRGetInfo(xf86ScrnToScreen(scrn), TRUE);
 }
+
+#undef DRM_MODE_LINK_STATUS_BAD
+#undef DRM_MODE_LINK_STATUS_GOOD
+
 #endif
 
 void

Signature · 1 line

2.11.0

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 <[email protected]>

Quoted 7 lines · Martin Peres

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.

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

Quoted 25 lines · Manasi Navare

Quoted 22 lines · Eric Anholt

Quoted 8 lines · Martin Peres

Quoted 7 lines · Martin Peres

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.

Quoted 8 lines

Quoted 4 lines

When available, we should use the link-status information to reset the wanted mode.

Signed-off-by Martin Peres <[email protected]>

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 4 lines

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

Quoted 32 lines · Eric Anholt

Quoted 29 lines · Manasi Navare

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

Quoted 2 lines

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

So when userspace driver re-sets the same mode, the kernel will call the mode valid function where it will see it can allow the sam mode perhaps at a lower bpp now since the link parameters are lowered.
So the mode which failed at 30 bpp, might still work at 18bpp and is better going to a lower resolution.

Quoted 36 lines · Manasi Navare

Quoted 34 lines · Eric Anholt

Quoted 25 lines · Manasi Navare

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 4 lines

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

Quoted 5 lines

So when userspace driver re-sets the same mode, the kernel will call the mode valid function where it will see it can allow the sam mode perhaps at a lower bpp now since the link parameters are lowered.
So the mode which failed at 30 bpp, might still work at 18bpp and is better going to a lower resolution.

The question was whether the kernel will ever allow the same mode at the same bpp, since that's what this patch tries to do.

Quoted 38 lines · Eric Anholt

Quoted 36 lines · Manasi Navare

Quoted 34 lines · Eric Anholt

Quoted 25 lines · Manasi Navare

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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-VuQAYsv1563Yd54FQh9/[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 4 lines

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

Quoted 8 lines

Quoted 5 lines

So when userspace driver re-sets the same mode, the kernel will call the mode valid function where it will see it can allow the sam mode perhaps at a lower bpp now since the link parameters are lowered.
So the mode which failed at 30 bpp, might still work at 18bpp and is better going to a lower resolution.

The question was whether the kernel will ever allow the same mode at the same bpp, since that's what this patch tries to do.

Yes, this can happen. Doing a full modeset with recomputing clocks and everything behind userspace's back might not be something the kernel driver can pull of with a reasonable amount of effort, hence why we punt to userspace. The interface spec makes this a CAN, not WILL, to allow less unreasonable hw to handle these cases directly in the kernel driver. E.g. plain link-retraining is handled in i915.ko still.

Quoted 48 lines · Daniel Vetter

Quoted 38 lines · Eric Anholt

Quoted 36 lines · Manasi Navare

Quoted 34 lines · Eric Anholt

Quoted 25 lines · Manasi Navare

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 4 lines

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

Quoted 8 lines

Quoted 5 lines

So when userspace driver re-sets the same mode, the kernel will call the mode valid function where it will see it can allow the sam mode perhaps at a lower bpp now since the link parameters are lowered.
So the mode which failed at 30 bpp, might still work at 18bpp and is better going to a lower resolution.

The question was whether the kernel will ever allow the same mode at the same bpp, since that's what this patch tries to do.

Quoted 6 lines

Yes, this can happen. Doing a full modeset with recomputing clocks and everything behind userspace's back might not be something the kernel driver can pull of with a reasonable amount of effort, hence why we punt to userspace. The interface spec makes this a CAN, not WILL, to allow less unreasonable hw to handle these cases directly in the kernel driver. E.g. plain link-retraining is handled in i915.ko still.

So in that case you do need userspace to re-request the same mode at the same bpp?

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 2 lines

So in that case you do need userspace to re-request the same mode at the same bpp?

So yes because when userspace requests the same mode at same bpp, kernel will still call intel_dp->mod_valid which validates the mode against 18bpp so if the requested mode can be displayed at the lowest of 18bpp, then the kernel will try to do the modeset for that mode at lower bpp. What I am trying to say is irrespective of what bpp userspace requests, kernel will check if it can display that at the lowest of 18bpp.

Quoted 9 lines · Manasi Navare

Quoted 2 lines

So in that case you do need userspace to re-request the same mode at the same bpp?

So yes because when userspace requests the same mode at same bpp, kernel will still call intel_dp->mod_valid which validates the mode against 18bpp so if the requested mode can be displayed at the lowest of 18bpp, then the kernel will try to do the modeset for that mode at lower bpp. What I am trying to say is irrespective of what bpp userspace requests, kernel will check if it can display that at the lowest of 18bpp.

You're talking about two different bpp here I think. Eric talks about the pixel format of the framebuffer, Manasi here about the bpp we send over the wire. The kernel will auto-dither if the wire bpp is lower than the stuff we scan out. Same with 6bpc panels really.

Right now userspace can't request a specific bpp for the sink/pipe, that's fully under the kernel's control. It only gets to set the pixel format of fbs.
-Daniel

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Quoted 48 lines · Daniel Vetter

Quoted 38 lines · Eric Anholt

Quoted 36 lines · Manasi Navare

Quoted 34 lines · Eric Anholt

Quoted 25 lines · Manasi Navare

Quoted 15 lines · Eric Anholt

Quoted 13 lines · Martin Peres

Quoted 11 lines · Martin Peres

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 <[email protected]>

Quoted 7 lines

Quoted 3 lines

The relevant kernel patches have landed in drm-tip about a month ago.

Eric, would you mind providing feedback on or merging this patch?

The later discussion has sounded like the kernel will (always) prune the mode when we re-query, meaning that it doesn't make any sense to try to re-set to the old mode. Is this not the case?

Quoted 4 lines

No the kernel will simply send a hotplug with link status as bad and then after that point its userspace driver's responsibility to check if link status is BAD, retry the same mode and if it fails then re probe.

So the kernel will sometimes allow the same mode to be re-set with the same bpp?

Quoted 8 lines

Quoted 5 lines

So when userspace driver re-sets the same mode, the kernel will call the mode valid function where it will see it can allow the sam mode perhaps at a lower bpp now since the link parameters are lowered.
So the mode which failed at 30 bpp, might still work at 18bpp and is better going to a lower resolution.

The question was whether the kernel will ever allow the same mode at the same bpp, since that's what this patch tries to do.

Quoted 6 lines

Yes, this can happen. Doing a full modeset with recomputing clocks and everything behind userspace's back might not be something the kernel driver can pull of with a reasonable amount of effort, hence why we punt to userspace. The interface spec makes this a CAN, not WILL, to allow less unreasonable hw to handle these cases directly in the kernel driver. E.g. plain link-retraining is handled in i915.ko still.

So in that case you do need userspace to re-request the same mode at the same bpp?

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel