[PATCH] drm/i915: Don't write DSPSURF for old chips

[PATCH] drm/i915: Don't write DSPSURF for old chips

[Takashi Iwai]

It seems that writing DSPSURF in intel_flush_display_plane() causes
the blank screen on some old laptops like Dell D630 with 965GM.
Since this operation is needed only for ILK+, make it conditional.

Cc: <stable@kernel.org> [v3.1+]
Signed-off-by: Takashi Iwai <tiwai@suse.de>

---
 drivers/gpu/drm/i915/intel_display.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1379,7 +1379,8 @@ static void intel_flush_display_plane(st
 				      enum plane plane)
 {
 	I915_WRITE(DSPADDR(plane), I915_READ(DSPADDR(plane)));
-	I915_WRITE(DSPSURF(plane), I915_READ(DSPSURF(plane)));
+	if (dev_priv->info->gen >= 5)
+		I915_WRITE(DSPSURF(plane), I915_READ(DSPSURF(plane)));
 }
 
 /**

Re: [PATCH] drm/i915: Don't write DSPSURF for old chips

[Chris Wilson]

On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> It seems that writing DSPSURF in intel_flush_display_plane() causes
> the blank screen on some old laptops like Dell D630 with 965GM.
> Since this operation is needed only for ILK+, make it conditional.

The specs say that DSPASURF is the latch register for updates of the DSPA
registers on gen4 (including 965gm) as well. Presumably the bug is that
we only partially update the DSPA registers prior to the first call to
intel_flush_display_plane() which this papers over by disabling the
update until a valid address is written to DSPASURF. And there is such a
spurious call to intel_enable_plane() prior to us setting a valid
scanout surface:

diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_d
index 66b19d3..ea8e4a1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc
*crtc,
 
        I915_WRITE(DSPCNTR(plane), dspcntr);
        POSTING_READ(DSPCNTR(plane));
-       intel_enable_plane(dev_priv, plane, pipe);
 
        ret = intel_pipe_set_base(crtc, x, y, old_fb);

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Don't write DSPSURF for old chips

[Takashi Iwai]

At Mon, 27 Feb 2012 12:17:57 +0000,
Chris Wilson wrote:
> 
> On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> > It seems that writing DSPSURF in intel_flush_display_plane() causes
> > the blank screen on some old laptops like Dell D630 with 965GM.
> > Since this operation is needed only for ILK+, make it conditional.
> 
> The specs say that DSPASURF is the latch register for updates of the DSPA
> registers on gen4 (including 965gm) as well. Presumably the bug is that
> we only partially update the DSPA registers prior to the first call to
> intel_flush_display_plane() which this papers over by disabling the
> update until a valid address is written to DSPASURF. And there is such a
> spurious call to intel_enable_plane() prior to us setting a valid
> scanout surface:

Sounds reasonable.  FWIW, the change was first introduced in commit
[b24e7179: drm/i915: add pipe/plane enable/disable functions],
then in commit [efc2924e: drm/i915: Call intel_enable_plane from
i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set().

This explains the fact that it was discovered only on old machines
as i9xx_crtc_mode_set() is the only crtc_mode_set op calling
intel_enable_plane().

BTW, the bisection leaded to a merge commit, so the bug is really
depending on the activation path or timing.

I'll ask a tester to try your patch.


thanks,

Takashi

> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_d
> index 66b19d3..ea8e4a1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc
> *crtc,
>  
>         I915_WRITE(DSPCNTR(plane), dspcntr);
>         POSTING_READ(DSPCNTR(plane));
> -       intel_enable_plane(dev_priv, plane, pipe);
>  
>         ret = intel_pipe_set_base(crtc, x, y, old_fb);
> 
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> 

Re: [PATCH] drm/i915: Don't write DSPSURF for old chips

[Takashi Iwai]

At Mon, 27 Feb 2012 14:08:28 +0100,
Takashi Iwai wrote:
> 
> At Mon, 27 Feb 2012 12:17:57 +0000,
> Chris Wilson wrote:
> > 
> > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> > > It seems that writing DSPSURF in intel_flush_display_plane() causes
> > > the blank screen on some old laptops like Dell D630 with 965GM.
> > > Since this operation is needed only for ILK+, make it conditional.
> > 
> > The specs say that DSPASURF is the latch register for updates of the DSPA
> > registers on gen4 (including 965gm) as well. Presumably the bug is that
> > we only partially update the DSPA registers prior to the first call to
> > intel_flush_display_plane() which this papers over by disabling the
> > update until a valid address is written to DSPASURF. And there is such a
> > spurious call to intel_enable_plane() prior to us setting a valid
> > scanout surface:
> 
> Sounds reasonable.  FWIW, the change was first introduced in commit
> [b24e7179: drm/i915: add pipe/plane enable/disable functions],
> then in commit [efc2924e: drm/i915: Call intel_enable_plane from
> i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set().
> 
> This explains the fact that it was discovered only on old machines
> as i9xx_crtc_mode_set() is the only crtc_mode_set op calling
> intel_enable_plane().
> 
> BTW, the bisection leaded to a merge commit, so the bug is really
> depending on the activation path or timing.
> 
> I'll ask a tester to try your patch.

He reported back that it reduces the failure rate but doesn't fix
completely.  Still get a blank screen in 20% rate.

Any other clue?


thanks,

Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_d
> > index 66b19d3..ea8e4a1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5453,7 +5453,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc
> > *crtc,
> >  
> >         I915_WRITE(DSPCNTR(plane), dspcntr);
> >         POSTING_READ(DSPCNTR(plane));
> > -       intel_enable_plane(dev_priv, plane, pipe);
> >  
> >         ret = intel_pipe_set_base(crtc, x, y, old_fb);
> > 
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> > 

Re: [PATCH] drm/i915: Don't write DSPSURF for old chips

[Chris Wilson]

On Tue, 28 Feb 2012 07:43:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> At Mon, 27 Feb 2012 14:08:28 +0100,
> Takashi Iwai wrote:
> > 
> > At Mon, 27 Feb 2012 12:17:57 +0000,
> > Chris Wilson wrote:
> > > 
> > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> > > > It seems that writing DSPSURF in intel_flush_display_plane() causes
> > > > the blank screen on some old laptops like Dell D630 with 965GM.
> > > > Since this operation is needed only for ILK+, make it conditional.
> > > 
> > > The specs say that DSPASURF is the latch register for updates of the DSPA
> > > registers on gen4 (including 965gm) as well. Presumably the bug is that
> > > we only partially update the DSPA registers prior to the first call to
> > > intel_flush_display_plane() which this papers over by disabling the
> > > update until a valid address is written to DSPASURF. And there is such a
> > > spurious call to intel_enable_plane() prior to us setting a valid
> > > scanout surface:
> > 
> > Sounds reasonable.  FWIW, the change was first introduced in commit
> > [b24e7179: drm/i915: add pipe/plane enable/disable functions],
> > then in commit [efc2924e: drm/i915: Call intel_enable_plane from
> > i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set().
> > 
> > This explains the fact that it was discovered only on old machines
> > as i9xx_crtc_mode_set() is the only crtc_mode_set op calling
> > intel_enable_plane().
> > 
> > BTW, the bisection leaded to a merge commit, so the bug is really
> > depending on the activation path or timing.
> > 
> > I'll ask a tester to try your patch.
> 
> He reported back that it reduces the failure rate but doesn't fix
> completely.  Still get a blank screen in 20% rate.
> 
> Any other clue?

I haven't yet found anything else in the same vein as this, but the
965gm does ring a warning bell for this recent regression:

commit 5ca0c34ae28344b6b4ca3036bc82f89c8db16a59
Author: Dave Airlie <airlied@redhat.com>
Date:   Thu Feb 23 15:33:40 2012 +0000

    drm/i915: fix mode set on load pipe. (v2)
    
    Booted my i965 machine and it started printing the unsupported pixel
    format of 0 message (once I added content to it).
    
    Oh looksie here, we pass 0. fix.
    
    v2: compile it.
    
    Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45966
    
    Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Dave Airlie <airlied@redhat.com>
    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

which is currently sitting in airlied/drm-fixes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Don't write DSPSURF for old chips

[Takashi Iwai]

At Wed, 29 Feb 2012 23:54:46 +0000,
Chris Wilson wrote:
> 
> On Tue, 28 Feb 2012 07:43:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> > At Mon, 27 Feb 2012 14:08:28 +0100,
> > Takashi Iwai wrote:
> > > 
> > > At Mon, 27 Feb 2012 12:17:57 +0000,
> > > Chris Wilson wrote:
> > > > 
> > > > On Mon, 27 Feb 2012 12:39:24 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> > > > > It seems that writing DSPSURF in intel_flush_display_plane() causes
> > > > > the blank screen on some old laptops like Dell D630 with 965GM.
> > > > > Since this operation is needed only for ILK+, make it conditional.
> > > > 
> > > > The specs say that DSPASURF is the latch register for updates of the DSPA
> > > > registers on gen4 (including 965gm) as well. Presumably the bug is that
> > > > we only partially update the DSPA registers prior to the first call to
> > > > intel_flush_display_plane() which this papers over by disabling the
> > > > update until a valid address is written to DSPASURF. And there is such a
> > > > spurious call to intel_enable_plane() prior to us setting a valid
> > > > scanout surface:
> > > 
> > > Sounds reasonable.  FWIW, the change was first introduced in commit
> > > [b24e7179: drm/i915: add pipe/plane enable/disable functions],
> > > then in commit [efc2924e: drm/i915: Call intel_enable_plane from
> > > i9xx_crtc_mode_set (again)], it's placed into i9xx_crtc_mode_set().
> > > 
> > > This explains the fact that it was discovered only on old machines
> > > as i9xx_crtc_mode_set() is the only crtc_mode_set op calling
> > > intel_enable_plane().
> > > 
> > > BTW, the bisection leaded to a merge commit, so the bug is really
> > > depending on the activation path or timing.
> > > 
> > > I'll ask a tester to try your patch.
> > 
> > He reported back that it reduces the failure rate but doesn't fix
> > completely.  Still get a blank screen in 20% rate.
> > 
> > Any other clue?
> 
> I haven't yet found anything else in the same vein as this, but the
> 965gm does ring a warning bell for this recent regression:
> 
> commit 5ca0c34ae28344b6b4ca3036bc82f89c8db16a59
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Thu Feb 23 15:33:40 2012 +0000
> 
>     drm/i915: fix mode set on load pipe. (v2)
>     
>     Booted my i965 machine and it started printing the unsupported pixel
>     format of 0 message (once I added content to it).
>     
>     Oh looksie here, we pass 0. fix.
>     
>     v2: compile it.
>     
>     Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45966
>     
>     Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Signed-off-by: Dave Airlie <airlied@redhat.com>
>     Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> which is currently sitting in airlied/drm-fixes.

Hm, this must be irrelevant (inapplicable) because the tests were done
with 3.1, 3.2 (and SLE11-SP2 kernels which has backports from 3.3-rc2
but without these fixes).  The bug seems to have been introduced
between 3.0 and 3.1.


thanks,

Takashi