| Message ID | 1349977587-7534-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 11 Oct 2012 19:46:27 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This piece of neat lore has been ported painstakingly and bug-for-bug > compatible from the old crtc helper code. > > Imo it's utter nonsense. > > If you have disconnect a cable and before you reconnect it, userspace > (or the kernel) does an set_crtc call, this will result in that > connector getting disable. Which will result in a nice black screen > when plugging in the cable again. > > There's absolutely no reason the kernel does such policy changes - if > userspace tries to set up a mode on something disconnected we might > fail loudly (since the dp link training fails), but silently adjusting > the output configuration behind userspace's back is a recipe for > disaster. Specifically I think that this could explain some of our > MI_WAIT hangs around suspend, where userspace issues a scanline wait > on a disable pipe. This mechanisims here could explain how that pipe > got disabled without userspace noticing. Indeed, it might just explain all those funky disables on other crtc after a setcrtc. Won't a side effect of this be that the system continues to power a disconnected output if userspace is asleep and fails to notice the disconnect? -Chris
On Fri, Oct 12, 2012 at 11:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, 11 Oct 2012 19:46:27 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> This piece of neat lore has been ported painstakingly and bug-for-bug >> compatible from the old crtc helper code. >> >> Imo it's utter nonsense. >> >> If you have disconnect a cable and before you reconnect it, userspace >> (or the kernel) does an set_crtc call, this will result in that >> connector getting disable. Which will result in a nice black screen >> when plugging in the cable again. >> >> There's absolutely no reason the kernel does such policy changes - if >> userspace tries to set up a mode on something disconnected we might >> fail loudly (since the dp link training fails), but silently adjusting >> the output configuration behind userspace's back is a recipe for >> disaster. Specifically I think that this could explain some of our >> MI_WAIT hangs around suspend, where userspace issues a scanline wait >> on a disable pipe. This mechanisims here could explain how that pipe >> got disabled without userspace noticing. > > Indeed, it might just explain all those funky disables on other crtc > after a setcrtc. Yeah, those are pretty much the reason why I want to see this dead. > Won't a side effect of this be that the system continues to power a > disconnected output if userspace is asleep and fails to notice the > disconnect? Well, we _only_ disable disconnected outputs at setcrtc time. Which means as long as userspace is sound asleep, nothing will happen at all. Note that the crtc helper only calls disable_unused_functions when doing a full modeset (and not just a call to ->mode_set_base). So vt-switching or panning won't be good enough. Hence I think we can safely ignore power consumption concerns, this wont' make it worse. One thing I've noticed though is that some graphical xrandr tools do not allow you to disable disconnected outputs. Which is imo again totally brain-dead - you have to either disable the output before unplugging (but the gui only pops up at the unplug event, so a neat chicken/egg userinterface fail). Or you change some unrelated crtc's mode ... I've just looked at the drm helper code, and this logic has 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. For shared tv-out and VGA encoders, we really need to know if the encoder is just being switched off temporarily in blanking or if we are really disabling it hard. Also we need to try harder to disconnect encoders from unused connectors so we can share more efficently. (shared encoders stuff is coming in radeon tv-out support) Signed-off-by: Dave Airlie <airlied@redhat.com> tbh I have no idea why we want this behaviour (besides that some GUI xrandr interfaces seem to be broken like described). Adding Dave. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f84fb2e..d469b42 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7319,10 +7319,6 @@ intel_modeset_stage_output_state(struct drm_device *dev, DRM_DEBUG_KMS("encoder changed, full mode switch\n"); config->mode_changed = true; } - - /* Disable all disconnected encoders. */ - if (connector->base.status == connector_status_disconnected) - connector->new_encoder = NULL; } /* connector->new_encoder is now updated for all connectors. */
This piece of neat lore has been ported painstakingly and bug-for-bug compatible from the old crtc helper code. Imo it's utter nonsense. If you have disconnect a cable and before you reconnect it, userspace (or the kernel) does an set_crtc call, this will result in that connector getting disable. Which will result in a nice black screen when plugging in the cable again. There's absolutely no reason the kernel does such policy changes - if userspace tries to set up a mode on something disconnected we might fail loudly (since the dp link training fails), but silently adjusting the output configuration behind userspace's back is a recipe for disaster. Specifically I think that this could explain some of our MI_WAIT hangs around suspend, where userspace issues a scanline wait on a disable pipe. This mechanisims here could explain how that pipe got disabled without userspace noticing. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 4 ---- 1 file changed, 4 deletions(-)