Re: [Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[Daniel Vetter]

On Mon, Jul 17, 2017 at 12:42:18PM +0200, Paul Menzel wrote:
> Dear Ville,
> 
> 
> On 07/14/17 17:52, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > 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 
> > 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ä 
> > ---
> >   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 

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[Paul Menzel]

Dear Ville,


On 07/14/17 17:52, ville.syrj...@linux.intel.com wrote:

From: Ville Syrjälä 

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 
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ä 
---
  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 

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

Re: [Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[Ville Syrjälä]

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.syrj...@linux.intel.com (2017-07-14 16:52:27)
> > > From: Ville Syrjälä 
> > > 
> > > 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.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[Ville Syrjälä]

On Fri, Jul 14, 2017 at 05:24:03PM +0100, Chris Wilson wrote:
> Quoting ville.syrj...@linux.intel.com (2017-07-14 16:52:27)
> > From: Ville Syrjälä 
> > 
> > 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ä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[Chris Wilson]

Quoting ville.syrj...@linux.intel.com (2017-07-14 16:52:27)
> From: Ville Syrjälä 
> 
> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Fix cursor updates on some platforms

[ville . syrjala]

From: Ville Syrjälä 

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 
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ä 
---
 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));
-- 
2.13.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx