diff mbox

drm/i915: flush cursors harder

Message ID 1383549225-16841-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Vetter Nov. 4, 2013, 7:13 a.m. UTC 
Apparently they need the same treatment as primary planes. This fixes
modesetting failures because of stuck cursors (!) on Thomas' i830M
machine.

I've figured while at it I'll also roll it out for the ivb 3 pipe
version of this function. I didn't do this for i845/i865 since Bspec
says the update mechanism works differently, and there's some
additional rules about what can be updated in which order.

Tested-by: Thomas Richter <thor@math.tu-berlin.de>
Cc: stable@vger.kernel.org
Cc:  Thomas Richter <thor@math.tu-berlin.de>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Ville Syrjälä Nov. 4, 2013, 4:02 p.m. UTC | #1 
On Mon, Nov 04, 2013 at 08:13:45AM +0100, Daniel Vetter wrote:
> Apparently they need the same treatment as primary planes. This fixes
> modesetting failures because of stuck cursors (!) on Thomas' i830M
> machine.

What treatment? Primary planes don't need any extra posting reads AFAIK.

> 
> I've figured while at it I'll also roll it out for the ivb 3 pipe
> version of this function. I didn't do this for i845/i865 since Bspec
> says the update mechanism works differently, and there's some
> additional rules about what can be updated in which order.
> 
> Tested-by: Thomas Richter <thor@math.tu-berlin.de>

I didn't see an explicit note from Thomas saying that he tested it.

> Cc: stable@vger.kernel.org
> Cc:  Thomas Richter <thor@math.tu-berlin.de>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f34252d134b6..04d2699f51b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7123,7 +7123,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  		intel_crtc->cursor_visible = visible;
>  	}
>  	/* and commit changes on next vblank */
> +	POSTING_READ(CURCNTR(pipe));
>  	I915_WRITE(CURBASE(pipe), base);
> +	POSTING_READ(CURBASE(pipe));
>  }
>  
>  static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> @@ -7152,7 +7154,9 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  		intel_crtc->cursor_visible = visible;
>  	}
>  	/* and commit changes on next vblank */
> +	POSTING_READ(CURCNTR_IVB(pipe));
>  	I915_WRITE(CURBASE_IVB(pipe), base);
> +	POSTING_READ(CURBASE_IVB(pipe));
>  }
>  
>  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 4, 2013, 4:34 p.m. UTC | #2 
On Mon, Nov 04, 2013 at 06:02:24PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 04, 2013 at 08:13:45AM +0100, Daniel Vetter wrote:
> > Apparently they need the same treatment as primary planes. This fixes
> > modesetting failures because of stuck cursors (!) on Thomas' i830M
> > machine.
> 
> What treatment? Primary planes don't need any extra posting reads AFAIK.

If you look at flush_primary_plane it's definitely there. So I've copied
it over (it's just a real I915_READ, not a posting one).

> 
> > 
> > I've figured while at it I'll also roll it out for the ivb 3 pipe
> > version of this function. I didn't do this for i845/i865 since Bspec
> > says the update mechanism works differently, and there's some
> > additional rules about what can be updated in which order.
> > 
> > Tested-by: Thomas Richter <thor@math.tu-berlin.de>
> 
> I didn't see an explicit note from Thomas saying that he tested it.

It's burried somewhere in the thread, but he said that with the 2 earlier
dvo patches + this one here the lvds-only use-case now works well. Before
that he had issues with the display just showing a cursor and the kernel
complaining about the cursor being stuck in the enabled position when
trying to re-enable it.
-Daniel

> 
> > Cc: stable@vger.kernel.org
> > Cc:  Thomas Richter <thor@math.tu-berlin.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index f34252d134b6..04d2699f51b4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7123,7 +7123,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >  		intel_crtc->cursor_visible = visible;
> >  	}
> >  	/* and commit changes on next vblank */
> > +	POSTING_READ(CURCNTR(pipe));
> >  	I915_WRITE(CURBASE(pipe), base);
> > +	POSTING_READ(CURBASE(pipe));
> >  }
> >  
> >  static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > @@ -7152,7 +7154,9 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> >  		intel_crtc->cursor_visible = visible;
> >  	}
> >  	/* and commit changes on next vblank */
> > +	POSTING_READ(CURCNTR_IVB(pipe));
> >  	I915_WRITE(CURBASE_IVB(pipe), base);
> > +	POSTING_READ(CURBASE_IVB(pipe));
> >  }
> >  
> >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > -- 
> > 1.8.4.rc3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Nov. 4, 2013, 4:58 p.m. UTC | #3 
On Mon, Nov 04, 2013 at 05:34:41PM +0100, Daniel Vetter wrote:
> On Mon, Nov 04, 2013 at 06:02:24PM +0200, Ville Syrjälä wrote:
> > On Mon, Nov 04, 2013 at 08:13:45AM +0100, Daniel Vetter wrote:
> > > Apparently they need the same treatment as primary planes. This fixes
> > > modesetting failures because of stuck cursors (!) on Thomas' i830M
> > > machine.
> > 
> > What treatment? Primary planes don't need any extra posting reads AFAIK.
> 
> If you look at flush_primary_plane it's definitely there. So I've copied
> it over (it's just a real I915_READ, not a posting one).

The regular read is there just so that we don't modify the surface
base address. We should be able to replace the read w/ recomputing
the address.

> 
> > 
> > > 
> > > I've figured while at it I'll also roll it out for the ivb 3 pipe
> > > version of this function. I didn't do this for i845/i865 since Bspec
> > > says the update mechanism works differently, and there's some
> > > additional rules about what can be updated in which order.
> > > 
> > > Tested-by: Thomas Richter <thor@math.tu-berlin.de>
> > 
> > I didn't see an explicit note from Thomas saying that he tested it.
> 
> It's burried somewhere in the thread, but he said that with the 2 earlier
> dvo patches + this one here the lvds-only use-case now works well. Before
> that he had issues with the display just showing a cursor and the kernel
> complaining about the cursor being stuck in the enabled position when
> trying to re-enable it.

From the mails, I couldn't figure out if he tried them all or just the
two patches you posted earlier.

> -Daniel
> 
> > 
> > > Cc: stable@vger.kernel.org
> > > Cc:  Thomas Richter <thor@math.tu-berlin.de>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f34252d134b6..04d2699f51b4 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -7123,7 +7123,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  		intel_crtc->cursor_visible = visible;
> > >  	}
> > >  	/* and commit changes on next vblank */
> > > +	POSTING_READ(CURCNTR(pipe));
> > >  	I915_WRITE(CURBASE(pipe), base);
> > > +	POSTING_READ(CURBASE(pipe));
> > >  }
> > >  
> > >  static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > > @@ -7152,7 +7154,9 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > >  		intel_crtc->cursor_visible = visible;
> > >  	}
> > >  	/* and commit changes on next vblank */
> > > +	POSTING_READ(CURCNTR_IVB(pipe));
> > >  	I915_WRITE(CURBASE_IVB(pipe), base);
> > > +	POSTING_READ(CURBASE_IVB(pipe));
> > >  }
> > >  
> > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > > -- 
> > > 1.8.4.rc3
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter Nov. 15, 2013, 7:22 p.m. UTC | #4 
On Mon, Nov 04, 2013 at 06:58:13PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 04, 2013 at 05:34:41PM +0100, Daniel Vetter wrote:
> > On Mon, Nov 04, 2013 at 06:02:24PM +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 04, 2013 at 08:13:45AM +0100, Daniel Vetter wrote:
> > > > Apparently they need the same treatment as primary planes. This fixes
> > > > modesetting failures because of stuck cursors (!) on Thomas' i830M
> > > > machine.
> > > 
> > > What treatment? Primary planes don't need any extra posting reads AFAIK.
> > 
> > If you look at flush_primary_plane it's definitely there. So I've copied
> > it over (it's just a real I915_READ, not a posting one).
> 
> The regular read is there just so that we don't modify the surface
> base address. We should be able to replace the read w/ recomputing
> the address.
> 
> > 
> > > 
> > > > 
> > > > I've figured while at it I'll also roll it out for the ivb 3 pipe
> > > > version of this function. I didn't do this for i845/i865 since Bspec
> > > > says the update mechanism works differently, and there's some
> > > > additional rules about what can be updated in which order.
> > > > 
> > > > Tested-by: Thomas Richter <thor@math.tu-berlin.de>
> > > 
> > > I didn't see an explicit note from Thomas saying that he tested it.
> > 
> > It's burried somewhere in the thread, but he said that with the 2 earlier
> > dvo patches + this one here the lvds-only use-case now works well. Before
> > that he had issues with the display just showing a cursor and the kernel
> > complaining about the cursor being stuck in the enabled position when
> > trying to re-enable it.
> 
> From the mails, I couldn't figure out if he tried them all or just the
> two patches you posted earlier.

Well I've written that patch to combat a WARN backtrace where the cursor
somehow didn't turn of. Thomas said he hasn't seen it since, so might have
worked. In any case it shouldn't be harmful, so I've merged it to -fixes.
-Daniel

> 
> > -Daniel
> > 
> > > 
> > > > Cc: stable@vger.kernel.org
> > > > Cc:  Thomas Richter <thor@math.tu-berlin.de>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index f34252d134b6..04d2699f51b4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -7123,7 +7123,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> > > >  		intel_crtc->cursor_visible = visible;
> > > >  	}
> > > >  	/* and commit changes on next vblank */
> > > > +	POSTING_READ(CURCNTR(pipe));
> > > >  	I915_WRITE(CURBASE(pipe), base);
> > > > +	POSTING_READ(CURBASE(pipe));
> > > >  }
> > > >  
> > > >  static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > > > @@ -7152,7 +7154,9 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> > > >  		intel_crtc->cursor_visible = visible;
> > > >  	}
> > > >  	/* and commit changes on next vblank */
> > > > +	POSTING_READ(CURCNTR_IVB(pipe));
> > > >  	I915_WRITE(CURBASE_IVB(pipe), base);
> > > > +	POSTING_READ(CURBASE_IVB(pipe));
> > > >  }
> > > >  
> > > >  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
> > > > -- 
> > > > 1.8.4.rc3
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel OTC
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f34252d134b6..04d2699f51b4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7123,7 +7123,9 @@  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		intel_crtc->cursor_visible = visible;
 	}
 	/* and commit changes on next vblank */
+	POSTING_READ(CURCNTR(pipe));
 	I915_WRITE(CURBASE(pipe), base);
+	POSTING_READ(CURBASE(pipe));
 }
 
 static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
@@ -7152,7 +7154,9 @@  static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
 		intel_crtc->cursor_visible = visible;
 	}
 	/* and commit changes on next vblank */
+	POSTING_READ(CURCNTR_IVB(pipe));
 	I915_WRITE(CURBASE_IVB(pipe), base);
+	POSTING_READ(CURBASE_IVB(pipe));
 }
 
 /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */