diff mbox

drm/i915: Fix cursor updates on some platforms

Message ID 20170714155227.6089-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä July 14, 2017, 3:52 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Turns out that just writing CURPOS isn't sufficient to move the cursor
on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
On those platforms we need to arm even the CURPOS update with a
CURBASE write.

Even worse, a write to any of the cursor register apart from CURBASE
will cancel an already pending cursor update. So if we have armed a
CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
would cancel that armed update. Thus we're left with a cursor that
doesn't appear to move, or even change shape.

Fix the problem by always performing the CURBASE write after a
CURPOS write. Bspec is somewhat unclear which platforms actually
require this CURBASE write and which don't. So to keep it simple
and to make sure we really fix the problem across all supported
devices, let's just perform the CURBASE write unconditionally.

Cc: Paul Menzel <pmenzel@molgen.mpg.de>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101790
Fixes: 75343a44c901 ("drm/i915: Drop useless posting reads from cursor commit")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Chris Wilson July 14, 2017, 4:24 p.m. UTC | #1 
Quoting ville.syrjala@linux.intel.com (2017-07-14 16:52:27)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Turns out that just writing CURPOS isn't sufficient to move the cursor
> on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
> On those platforms we need to arm even the CURPOS update with a
> CURBASE write.
> 
> Even worse, a write to any of the cursor register apart from CURBASE
> will cancel an already pending cursor update. So if we have armed a
> CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
> would cancel that armed update. Thus we're left with a cursor that
> doesn't appear to move, or even change shape.
> 
> Fix the problem by always performing the CURBASE write after a
> CURPOS write. Bspec is somewhat unclear which platforms actually
> require this CURBASE write and which don't. So to keep it simple
> and to make sure we really fix the problem across all supported
> devices, let's just perform the CURBASE write unconditionally.

Hmm, it seems that kms_cursor_crc should catch this? I guess we are
missing a move N times quickly test? We have CRC support on pnv right?
-Chris
Ville Syrjälä July 14, 2017, 4:34 p.m. UTC | #2 
On Fri, Jul 14, 2017 at 05:24:03PM +0100, Chris Wilson wrote:
> Quoting ville.syrjala@linux.intel.com (2017-07-14 16:52:27)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Turns out that just writing CURPOS isn't sufficient to move the cursor
> > on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
> > On those platforms we need to arm even the CURPOS update with a
> > CURBASE write.
> > 
> > Even worse, a write to any of the cursor register apart from CURBASE
> > will cancel an already pending cursor update. So if we have armed a
> > CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
> > would cancel that armed update. Thus we're left with a cursor that
> > doesn't appear to move, or even change shape.
> > 
> > Fix the problem by always performing the CURBASE write after a
> > CURPOS write. Bspec is somewhat unclear which platforms actually
> > require this CURBASE write and which don't. So to keep it simple
> > and to make sure we really fix the problem across all supported
> > devices, let's just perform the CURBASE write unconditionally.
> 
> Hmm, it seems that kms_cursor_crc should catch this? I guess we are
> missing a move N times quickly test?

Yeah. I guess what we have is basically this:
1. enable cursor at some location
2. wait for vblank and grab the crc
3. disable cursor and render the cursor image to the primary plane fb
4. wait for vblank and grab the crc

I guess what we could do is make step 1 enable the cursor at an
incorrect location, and then perform a few extra cursor movements,
ending in the correct location, before we grab the crc.

> We have CRC support on pnv right?

We should have it across the board.
Ville Syrjälä July 14, 2017, 8:39 p.m. UTC | #3 
On Fri, Jul 14, 2017 at 07:34:15PM +0300, Ville Syrjälä wrote:
> On Fri, Jul 14, 2017 at 05:24:03PM +0100, Chris Wilson wrote:
> > Quoting ville.syrjala@linux.intel.com (2017-07-14 16:52:27)
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Turns out that just writing CURPOS isn't sufficient to move the cursor
> > > on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
> > > On those platforms we need to arm even the CURPOS update with a
> > > CURBASE write.
> > > 
> > > Even worse, a write to any of the cursor register apart from CURBASE
> > > will cancel an already pending cursor update. So if we have armed a
> > > CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
> > > would cancel that armed update. Thus we're left with a cursor that
> > > doesn't appear to move, or even change shape.
> > > 
> > > Fix the problem by always performing the CURBASE write after a
> > > CURPOS write. Bspec is somewhat unclear which platforms actually
> > > require this CURBASE write and which don't. So to keep it simple
> > > and to make sure we really fix the problem across all supported
> > > devices, let's just perform the CURBASE write unconditionally.
> > 
> > Hmm, it seems that kms_cursor_crc should catch this? I guess we are
> > missing a move N times quickly test?
> 
> Yeah. I guess what we have is basically this:
> 1. enable cursor at some location
> 2. wait for vblank and grab the crc
> 3. disable cursor and render the cursor image to the primary plane fb
> 4. wait for vblank and grab the crc

Actually kms_cursor_crc does catch this. At least the "sliding" test
seems to fail on pnv without the patch, and pass with the patch.
The problem is we don't run any of that in BAT.
Paul Menzel July 17, 2017, 10:42 a.m. UTC | #4 
Dear Ville,


On 07/14/17 17:52, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Turns out that just writing CURPOS isn't sufficient to move the cursor
> on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
> On those platforms we need to arm even the CURPOS update with a
> CURBASE write.
> 
> Even worse, a write to any of the cursor register apart from CURBASE
> will cancel an already pending cursor update. So if we have armed a
> CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
> would cancel that armed update. Thus we're left with a cursor that
> doesn't appear to move, or even change shape.
> 
> Fix the problem by always performing the CURBASE write after a
> CURPOS write. Bspec is somewhat unclear which platforms actually
> require this CURBASE write and which don't. So to keep it simple
> and to make sure we really fix the problem across all supported
> devices, let's just perform the CURBASE write unconditionally.
> 
> Cc: Paul Menzel <pmenzel@molgen.mpg.de>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101790
> Fixes: 75343a44c901 ("drm/i915: Drop useless posting reads from cursor commit")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2144adc5b1d5..460bd942fcb7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9555,7 +9555,16 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>   	 * On some platforms writing CURCNTR first will also
>   	 * cause CURPOS to be armed by the CURBASE write.
>   	 * Without the CURCNTR write the CURPOS write would
> -	 * arm itself.
> +	 * arm itself. Thus we always start the full update
> +	 * with a CURCNTR write.
> +	 *
> +	 * On other platforms CURPOS always requires the
> +	 * CURBASE write to arm the update. Additonally
> +	 * a write to any of the cursor register will cancel
> +	 * an already armed cursor update. Thus leaving out
> +	 * the CURBASE write after CURPOS could lead to a
> +	 * cursor that doesn't appear to move, or even change
> +	 * shape. Thus we always write CURBASE.
>   	 *
>   	 * CURCNTR and CUR_FBC_CTL are always
>   	 * armed by the CURBASE write only.
> @@ -9574,6 +9583,7 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>   		plane->cursor.cntl = cntl;
>   	} else {
>   		I915_WRITE_FW(CURPOS(pipe), pos);
> +		I915_WRITE_FW(CURBASE(pipe), base);
>   	}
>   
>   	POSTING_READ_FW(CURBASE(pipe));

Tested-by: Paul Menzel <paulepanter@users.sourceforge.net>

I created two more bugs for an issue with pasting text with the middle 
mouse button [1], and failing *kms_cursor_crc* tests [2].


Kind regards,

Paul Menzel


[1] https://bugs.freedesktop.org/show_bug.cgi?id=101819
[2] https://bugs.freedesktop.org/show_bug.cgi?id=101817
Daniel Vetter July 18, 2017, 10:08 a.m. UTC | #5 
On Mon, Jul 17, 2017 at 12:42:18PM +0200, Paul Menzel wrote:
> Dear Ville,
> 
> 
> On 07/14/17 17:52, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Turns out that just writing CURPOS isn't sufficient to move the cursor
> > on some platforms. My 830 works just fine, but eg. 945 and PNV don't.
> > On those platforms we need to arm even the CURPOS update with a
> > CURBASE write.
> > 
> > Even worse, a write to any of the cursor register apart from CURBASE
> > will cancel an already pending cursor update. So if we have armed a
> > CURCNTR/CURBASE update, a subsequent CURPOS write prior to vblank
> > would cancel that armed update. Thus we're left with a cursor that
> > doesn't appear to move, or even change shape.
> > 
> > Fix the problem by always performing the CURBASE write after a
> > CURPOS write. Bspec is somewhat unclear which platforms actually
> > require this CURBASE write and which don't. So to keep it simple
> > and to make sure we really fix the problem across all supported
> > devices, let's just perform the CURBASE write unconditionally.
> > 
> > Cc: Paul Menzel <pmenzel@molgen.mpg.de>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101790
> > Fixes: 75343a44c901 ("drm/i915: Drop useless posting reads from cursor commit")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 2144adc5b1d5..460bd942fcb7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9555,7 +9555,16 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> >   	 * On some platforms writing CURCNTR first will also
> >   	 * cause CURPOS to be armed by the CURBASE write.
> >   	 * Without the CURCNTR write the CURPOS write would
> > -	 * arm itself.
> > +	 * arm itself. Thus we always start the full update
> > +	 * with a CURCNTR write.
> > +	 *
> > +	 * On other platforms CURPOS always requires the
> > +	 * CURBASE write to arm the update. Additonally
> > +	 * a write to any of the cursor register will cancel
> > +	 * an already armed cursor update. Thus leaving out
> > +	 * the CURBASE write after CURPOS could lead to a
> > +	 * cursor that doesn't appear to move, or even change
> > +	 * shape. Thus we always write CURBASE.
> >   	 *
> >   	 * CURCNTR and CUR_FBC_CTL are always
> >   	 * armed by the CURBASE write only.
> > @@ -9574,6 +9583,7 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> >   		plane->cursor.cntl = cntl;
> >   	} else {
> >   		I915_WRITE_FW(CURPOS(pipe), pos);
> > +		I915_WRITE_FW(CURBASE(pipe), base);
> >   	}
> >   	POSTING_READ_FW(CURBASE(pipe));
> 
> Tested-by: Paul Menzel <paulepanter@users.sourceforge.net>

Since Ville is on vacation I applied this.
-Daniel

> 
> I created two more bugs for an issue with pasting text with the middle mouse
> button [1], and failing *kms_cursor_crc* tests [2].
> 
> 
> Kind regards,
> 
> Paul Menzel
> 
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=101819
> [2] https://bugs.freedesktop.org/show_bug.cgi?id=101817
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2144adc5b1d5..460bd942fcb7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9555,7 +9555,16 @@  static void i9xx_update_cursor(struct intel_plane *plane,
 	 * On some platforms writing CURCNTR first will also
 	 * cause CURPOS to be armed by the CURBASE write.
 	 * Without the CURCNTR write the CURPOS write would
-	 * arm itself.
+	 * arm itself. Thus we always start the full update
+	 * with a CURCNTR write.
+	 *
+	 * On other platforms CURPOS always requires the
+	 * CURBASE write to arm the update. Additonally
+	 * a write to any of the cursor register will cancel
+	 * an already armed cursor update. Thus leaving out
+	 * the CURBASE write after CURPOS could lead to a
+	 * cursor that doesn't appear to move, or even change
+	 * shape. Thus we always write CURBASE.
 	 *
 	 * CURCNTR and CUR_FBC_CTL are always
 	 * armed by the CURBASE write only.
@@ -9574,6 +9583,7 @@  static void i9xx_update_cursor(struct intel_plane *plane,
 		plane->cursor.cntl = cntl;
 	} else {
 		I915_WRITE_FW(CURPOS(pipe), pos);
+		I915_WRITE_FW(CURBASE(pipe), base);
 	}
 
 	POSTING_READ_FW(CURBASE(pipe));