Patchwork [Quantal] (pre-stable)drm/i915/lvds: ditch ->prepare special case

login
register
mail settings
Submitter Haitao Zhang
Date Jan. 24, 2013, 8:46 a.m.
Message ID <1359017177.3566.14.camel@x201panda>
Download mbox | patch
Permalink /patch/215271/
State New
Headers show

Comments

Haitao Zhang - Jan. 24, 2013, 8:46 a.m.
From 3273adbb6123abdda68695b663b1c4cc97e00fe9 Mon Sep 17 00:00:00 2001
From: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Wed, 11 Jul 2012 16:27:52 +0200
Subject: [PATCH] [Quantal](pre-stable)drm/i915/lvds: ditch ->prepare special
 case

LVDS is the first output where dpms on/off and prepare/commit don't
perfectly match. Now the idea behind this special case seems to be
that for simple resolution changes on the LVDS we don't need to stop
the pipe, because (at least on newer chips) we can adjust the panel
fitter on the fly.

There are a few problems with the current code though:
- We still stop and restart the pipe unconditionally, because the crtc
  helper code isn't flexible enough.
- We show some ugly flickering, especially when changing crtcs (this
  the crtc helper would actually take into account, but we don't
  implement the encoder->get_crtc callback required to make this work
  properly).

So it doesn't even work as advertised. I agree that it would be nice
to do resolution changes on LVDS (and also eDP) whithout blacking the
screen where the panel fitter allows to do that. But imo we should
implement this as a special case a few layers up in the mode set code,
akin to how we already detect simple framebuffer changes (and only
update the required registers with ->mode_set_base).

Until this is all in place, make our lives easier and just rip it out.

Also note that this seems to fix actual bugs with enabling the lvds
output, see:

http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Giacomo Comes <comes@naic.edu>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Takashi Iwai <tiwai@suse.de>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

(Cherry-picked from commit 520c41cf2fa029d1e8b923ac2026f96664f17c4b)
BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1103856
Signed-Off-by: Haitao Zhang <haitao.zhang@canonical.com>
---
 drivers/gpu/drm/i915/intel_lvds.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
Colin King - Jan. 24, 2013, 1:23 p.m.
On 24/01/13 08:46, Haitao Zhang wrote:
>  From 3273adbb6123abdda68695b663b1c4cc97e00fe9 Mon Sep 17 00:00:00 2001
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date: Wed, 11 Jul 2012 16:27:52 +0200
> Subject: [PATCH] [Quantal](pre-stable)drm/i915/lvds: ditch ->prepare special
>   case
>
> LVDS is the first output where dpms on/off and prepare/commit don't
> perfectly match. Now the idea behind this special case seems to be
> that for simple resolution changes on the LVDS we don't need to stop
> the pipe, because (at least on newer chips) we can adjust the panel
> fitter on the fly.
>
> There are a few problems with the current code though:
> - We still stop and restart the pipe unconditionally, because the crtc
>    helper code isn't flexible enough.
> - We show some ugly flickering, especially when changing crtcs (this
>    the crtc helper would actually take into account, but we don't
>    implement the encoder->get_crtc callback required to make this work
>    properly).
>
> So it doesn't even work as advertised. I agree that it would be nice
> to do resolution changes on LVDS (and also eDP) whithout blacking the
> screen where the panel fitter allows to do that. But imo we should
> implement this as a special case a few layers up in the mode set code,
> akin to how we already detect simple framebuffer changes (and only
> update the required registers with ->mode_set_base).
>
> Until this is all in place, make our lives easier and just rip it out.
>
> Also note that this seems to fix actual bugs with enabling the lvds
> output, see:
>
> http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html
>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Giacomo Comes <comes@naic.edu>
> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Takashi Iwai <tiwai@suse.de>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> (Cherry-picked from commit 520c41cf2fa029d1e8b923ac2026f96664f17c4b)
> BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1103856
> Signed-Off-by: Haitao Zhang <haitao.zhang@canonical.com>
> ---
>   drivers/gpu/drm/i915/intel_lvds.c |    8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index f85e2b1..3aa53e9 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -408,13 +408,7 @@ static void intel_lvds_prepare(struct drm_encoder *encoder)
>   {
>   	struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
>
> -	/*
> -	 * Prior to Ironlake, we must disable the pipe if we want to adjust
> -	 * the panel fitter. However at all other times we can just reset
> -	 * the registers regardless.
> -	 */
> -	if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty)
> -		intel_lvds_disable(intel_lvds);
> +	intel_lvds_disable(intel_lvds);
>   }
>
>   static void intel_lvds_commit(struct drm_encoder *encoder)
>

Well, this seems to be accepted upstream.  It also seems to fix other 
bugs when enabling LVS as referenced in the freedesktop link.  My only 
real concern is possible regressions on chipsets other than the one 
tested.  What kind of testing has been run on this fix?

Colin
Herton Ronaldo Krzesinski - Jan. 24, 2013, 1:49 p.m.
On Thu, Jan 24, 2013 at 04:46:17PM +0800, Haitao Zhang wrote:
> From 3273adbb6123abdda68695b663b1c4cc97e00fe9 Mon Sep 17 00:00:00 2001
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date: Wed, 11 Jul 2012 16:27:52 +0200
> Subject: [PATCH] [Quantal](pre-stable)drm/i915/lvds: ditch ->prepare special
>  case
> 
> LVDS is the first output where dpms on/off and prepare/commit don't
> perfectly match. Now the idea behind this special case seems to be
> that for simple resolution changes on the LVDS we don't need to stop
> the pipe, because (at least on newer chips) we can adjust the panel
> fitter on the fly.
> 
> There are a few problems with the current code though:
> - We still stop and restart the pipe unconditionally, because the crtc
>   helper code isn't flexible enough.
> - We show some ugly flickering, especially when changing crtcs (this
>   the crtc helper would actually take into account, but we don't
>   implement the encoder->get_crtc callback required to make this work
>   properly).
> 
> So it doesn't even work as advertised. I agree that it would be nice
> to do resolution changes on LVDS (and also eDP) whithout blacking the
> screen where the panel fitter allows to do that. But imo we should
> implement this as a special case a few layers up in the mode set code,
> akin to how we already detect simple framebuffer changes (and only
> update the required registers with ->mode_set_base).
> 
> Until this is all in place, make our lives easier and just rip it out.
> 
> Also note that this seems to fix actual bugs with enabling the lvds
> output, see:
> 
> http://lists.freedesktop.org/archives/intel-gfx/2012-July/018614.html

This isn't a pre-stable patch, since Cc: stable was omitted when the
patch was added upstream (first submission with the "more conservative"
diff had Cc to stable though), and as a result it did not and will not
enter in any stable release. So you'll need to send the patch to
stable@vger.kernel.org to ask it to be included on stables.
Andy Whitcroft - Jan. 24, 2013, 1:56 p.m.
On Thu, Jan 24, 2013 at 01:23:11PM +0000, Colin Ian King wrote:
> On 24/01/13 08:46, Haitao Zhang wrote:
> > From 3273adbb6123abdda68695b663b1c4cc97e00fe9 Mon Sep 17 00:00:00 2001
> >From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Date: Wed, 11 Jul 2012 16:27:52 +0200
> >Subject: [PATCH] [Quantal](pre-stable)drm/i915/lvds: ditch ->prepare special
> >  case

It should be noted that this is not (pre-stable).  It is upstream and in
Raring already.

> >-	/*
> >-	 * Prior to Ironlake, we must disable the pipe if we want to adjust
> >-	 * the panel fitter. However at all other times we can just reset
> >-	 * the registers regardless.
> >-	 */
> >-	if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty)
> >-		intel_lvds_disable(intel_lvds);
> >+	intel_lvds_disable(intel_lvds);

It think this says that behaviour prior to ironlake is unchanged with this
removal.  But I do think we need to confirm an older system with this, and
several affected ones to confirm there is no regression potential on this.

From the description in the bug, this is ultimatly needed for
linux-lts-quantal in precise.  It fixes a flash of white during resume
from suspend, so any testing needs to test that part as well as have
visual confirmation during boot.  I suspect we may be introducing a
flash to black in some cases.

-apw

Patch

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index f85e2b1..3aa53e9 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -408,13 +408,7 @@  static void intel_lvds_prepare(struct drm_encoder *encoder)
 {
 	struct intel_lvds *intel_lvds = to_intel_lvds(encoder);
 
-	/*
-	 * Prior to Ironlake, we must disable the pipe if we want to adjust
-	 * the panel fitter. However at all other times we can just reset
-	 * the registers regardless.
-	 */
-	if (!HAS_PCH_SPLIT(encoder->dev) && intel_lvds->pfit_dirty)
-		intel_lvds_disable(intel_lvds);
+	intel_lvds_disable(intel_lvds);
 }
 
 static void intel_lvds_commit(struct drm_encoder *encoder)