Re: [PATCH] drm/i915: don't disable disconnected outputs

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Dec 18, 2012 at 08:56:33AM -0800, Jesse Barnes wrote:
> On Tue, 18 Dec 2012 09:37:54 +0100
> 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 disconnected a cable and before you reconnect it, userspace (or
> > the kernel) does an set_crtc call, this will result in that connector
> > getting disabled. Which will result in a nice black screen when
> > plugging in the cable again.
> > 
> > There's absolutely no reason the kernel does such policy enforcements
> > - 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.
> > 
> > Note that this fixes a NULL deref at BIOS takeover when the firmware
> > sets up a disconnected output in a clone configuration with a
> > connected output on the 2nd pipe: When doing the full modeset we don't
> > have a mode for the 2nd pipe and OOPS. On the first pipe this doesn't
> > matter, since at boot-up the fbdev helpers will set up the choosen
> > configuration on that on first. Since this is now the umptenth bug
> > around handling this imo brain-dead semantics correctly, I think it's
> > time to kill it and see whether there's any userspace out there which
> > relies on this.
> > 
> > It also nicely demonstrates that we have a tiny window where DP
> > hotplug can still kill the driver.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=58396
> > Cc: stable@vger.kernel.org
> > Tested-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 34832bc0..399f862 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7632,10 +7632,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. */
> >  
> 
> I think this is safe now; iirc this logic comes from X, when we weren't
> sure about the initial config so we just shut everything off pretty
> aggressively.  We clearly weren't thinking of atomic mode sets back
> then either...
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch