All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: don't disable disconnected outputs
@ 2012-10-11 17:46 Daniel Vetter
  2012-10-12  9:02 ` Chris Wilson
  0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-10-11 17:46 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

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(-)

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. */
 
-- 
1.7.11.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: don't disable disconnected outputs
  2012-10-11 17:46 [PATCH] drm/i915: don't disable disconnected outputs Daniel Vetter
@ 2012-10-12  9:02 ` Chris Wilson
  2012-10-12  9:13   ` Daniel Vetter
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2012-10-12  9:02 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

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

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: don't disable disconnected outputs
  2012-10-12  9:02 ` Chris Wilson
@ 2012-10-12  9:13   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2012-10-12  9:13 UTC (permalink / raw)
  To: Chris Wilson, Dave Airlie; +Cc: Intel Graphics Development

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] drm/i915: don't disable disconnected outputs
@ 2012-12-18  8:37 Daniel Vetter
  2012-12-18 11:58 ` Rodrigo Vivi
  2012-12-18 16:56 ` [Intel-gfx] " Jesse Barnes
  0 siblings, 2 replies; 6+ messages in thread
From: Daniel Vetter @ 2012-12-18  8:37 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter, stable

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. */
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: don't disable disconnected outputs
  2012-12-18  8:37 Daniel Vetter
@ 2012-12-18 11:58 ` Rodrigo Vivi
  2012-12-18 16:56 ` [Intel-gfx] " Jesse Barnes
  1 sibling, 0 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2012-12-18 11:58 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development, stable

makes sense
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

couldn't we close the bug already?

On Tue, Dec 18, 2012 at 6:37 AM, 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. */
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: don't disable disconnected outputs
  2012-12-18 16:56 ` [Intel-gfx] " Jesse Barnes
@ 2012-12-18 20:33   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2012-12-18 20:33 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: Daniel Vetter, Intel Graphics Development, stable

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-18 20:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-11 17:46 [PATCH] drm/i915: don't disable disconnected outputs Daniel Vetter
2012-10-12  9:02 ` Chris Wilson
2012-10-12  9:13   ` Daniel Vetter
  -- strict thread matches above, loose matches on Subject: below --
2012-12-18  8:37 Daniel Vetter
2012-12-18 11:58 ` Rodrigo Vivi
2012-12-18 16:56 ` [Intel-gfx] " Jesse Barnes
2012-12-18 20:33   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.