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 at 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 at vger.kernel.org > > Tested-by: Peter Ujfalusi <peter.ujfalusi at gmail.com> > > Signed-off-by: Daniel Vetter <daniel.vetter at 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 at 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
