| Message ID | 1371328919-16967-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jun 16, 2013 at 6:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This has originally been added in > > commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 > Author: Dave Airlie <airlied@redhat.com> > Date: Mon Aug 31 15:16:30 2009 +1000 > > drm/kms: add explicit encoder disable function and detach harder. > > I think it's the wrong thing to do for a few reasons: > - It's policy in the kernel. Userspace gets hotplug events when an > output disconnects, and it can inquire about all the routing that is > already set up. If userspace wants to disable a disconnected output > it can simply do so itself. > - The reason for adding it given in the commit message was to improve > use of shared encoders. But the disable_unused_functions call only > happens after the modeset has been completed, so it won't help too > much in making the modest succeed. > - We (at least in drm/i915, but iirc all other drivers work the same) > ensure that if the user accidentally yanks the cable and then > replugs, the same configuration will keep on working without any > userspace actions required. For most outputs that's the case by > default, and DP can have the same semantics with a simple re-train > handler fired off from the hpd replug event. This breaks it since if > the user is unlucky and hits the mouse, resulting in a panning of > e.g. the integrated panel that modeset might kill the accidentally > yanked output. Which isn't too great. > > i915 has applied this behaviour change as part of the bit modeset > review, thus far without resulting in an angry crowd with pitchforks: > The reason is that i915 hardware is nearly impossible to ever set something up that could trigger this on. You don't have shared encoders in places that could affect this from what I can see. I know this will break the radeon card I have that I wrote this for originally, where if you alternate hotplugging a tv-out and vga that share the same encoder it won't work. I can probably drag that card out of whatever hole if fell into, but the given justifications have give me no confidence that this patch just isn't reverting the original commit without fixing anything else. I suspect I was testing hotplugging at fbcon not using X also. Dave.
On Tue, Jun 25, 2013 at 3:09 AM, Dave Airlie <airlied@gmail.com> wrote: > On Sun, Jun 16, 2013 at 6:41 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> This has originally been added in >> >> commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 >> Author: Dave Airlie <airlied@redhat.com> >> Date: Mon Aug 31 15:16:30 2009 +1000 >> >> drm/kms: add explicit encoder disable function and detach harder. >> >> I think it's the wrong thing to do for a few reasons: >> - It's policy in the kernel. Userspace gets hotplug events when an >> output disconnects, and it can inquire about all the routing that is >> already set up. If userspace wants to disable a disconnected output >> it can simply do so itself. >> - The reason for adding it given in the commit message was to improve >> use of shared encoders. But the disable_unused_functions call only >> happens after the modeset has been completed, so it won't help too >> much in making the modest succeed. >> - We (at least in drm/i915, but iirc all other drivers work the same) >> ensure that if the user accidentally yanks the cable and then >> replugs, the same configuration will keep on working without any >> userspace actions required. For most outputs that's the case by >> default, and DP can have the same semantics with a simple re-train >> handler fired off from the hpd replug event. This breaks it since if >> the user is unlucky and hits the mouse, resulting in a panning of >> e.g. the integrated panel that modeset might kill the accidentally >> yanked output. Which isn't too great. >> >> i915 has applied this behaviour change as part of the bit modeset >> review, thus far without resulting in an angry crowd with pitchforks: >> > > The reason is that i915 hardware is nearly impossible to ever set > something up that could trigger this on. You don't have > shared encoders in places that could affect this from what I can see. Actually we do have shared encoders, and on second consideration I guess we could hit this. We get away with it though since we don't really implement the connector modeset correctly for shared encoders: We simply pick what has been last detected as connected ... Of course that's a bit broken, and we have the bug reports to prove it (and since the modeset rework they include noisy dmesg spam from the checker about the inconsistencies even). I guess I should fix that up first ;-) > I know this will break the radeon card I have that I wrote this for > originally, where if you alternate hotplugging a tv-out > and vga that share the same encoder it won't work. I can probably drag > that card out of whatever hole if fell into, > but the given justifications have give me no confidence that this > patch just isn't reverting the original commit without > fixing anything else. > > I suspect I was testing hotplugging at fbcon not using X also. Oh, that makes a lot of sense. fbcon kinda wants an atomic modeset interface, but since it doesn't have one it expects the backend to paper over any intermediate inconsistencies. I guess we should fix that first. For userspace I still think that it makes much more sense to obey userspace's request and not silently kill outputs. We do that already if userspace steals the crtc. But that's something userspace can predict, wheras the exact connector->encoder relationship is more elusive. So I think the right thing would be to fail such a modeset. Anyway I agree that this patch needs a bit more prep work. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1ace9c1..72282af 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -276,16 +276,8 @@ drm_encoder_disable(struct drm_encoder *encoder) void drm_helper_disable_unused_functions(struct drm_device *dev) { struct drm_encoder *encoder; - struct drm_connector *connector; struct drm_crtc *crtc; - list_for_each_entry(connector, &dev->mode_config.connector_list, head) { - if (!connector->encoder) - continue; - if (connector->status == connector_status_disconnected) - connector->encoder = NULL; - } - list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { if (!drm_helper_encoder_in_use(encoder)) { drm_encoder_disable(encoder);
This has originally been added in commit a3a0544b2c84e1d7a2022b558ecf66d8c6a8dd93 Author: Dave Airlie <airlied@redhat.com> Date: Mon Aug 31 15:16:30 2009 +1000 drm/kms: add explicit encoder disable function and detach harder. I think it's the wrong thing to do for a few reasons: - It's policy in the kernel. Userspace gets hotplug events when an output disconnects, and it can inquire about all the routing that is already set up. If userspace wants to disable a disconnected output it can simply do so itself. - The reason for adding it given in the commit message was to improve use of shared encoders. But the disable_unused_functions call only happens after the modeset has been completed, so it won't help too much in making the modest succeed. - We (at least in drm/i915, but iirc all other drivers work the same) ensure that if the user accidentally yanks the cable and then replugs, the same configuration will keep on working without any userspace actions required. For most outputs that's the case by default, and DP can have the same semantics with a simple re-train handler fired off from the hpd replug event. This breaks it since if the user is unlucky and hits the mouse, resulting in a panning of e.g. the integrated panel that modeset might kill the accidentally yanked output. Which isn't too great. i915 has applied this behaviour change as part of the bit modeset review, thus far without resulting in an angry crowd with pitchforks: commit b0a2658acb5bf9ca86b4aab011b7106de3af0add Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Tue Dec 18 09:37:54 2012 +0100 drm/i915: don't disable disconnected outputs v2: Remove unused variable that I've missed. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_crtc_helper.c | 8 -------- 1 file changed, 8 deletions(-)