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

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

[ville.syrjala]

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

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

✓ Fi.CI.BAT: success for drm/i915: Fix cursor updates on some platforms

[Patchwork]

== Series Details ==

Series: drm/i915: Fix cursor updates on some platforms
URL   : https://patchwork.freedesktop.org/series/27314/
State : success

== Summary ==

Series 27314v1 drm/i915: Fix cursor updates on some platforms
https://patchwork.freedesktop.org/api/1.0/series/27314/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-pnv-d510) fdo#101597 +1
        Subgroup suspend-read-crc-pipe-a:
                pass       -> FAIL       (fi-skl-6700k) fdo#100367
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900) fdo#101705

fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#100367 https://bugs.freedesktop.org/show_bug.cgi?id=100367
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:448s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:429s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:359s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:533s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:505s
fi-byt-j1900     total:279  pass:255  dwarn:0   dfail:0   fail:0   skip:24  time:491s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:487s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:597s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:432s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:414s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:412s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:494s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:466s
fi-kbl-7560u     total:279  pass:268  dwarn:1   dfail:0   fail:0   skip:10  time:571s
fi-kbl-r         total:279  pass:260  dwarn:1   dfail:0   fail:0   skip:18  time:583s
fi-pnv-d510      total:279  pass:222  dwarn:2   dfail:0   fail:0   skip:55  time:562s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:456s
fi-skl-6700hq    total:279  pass:262  dwarn:0   dfail:0   fail:0   skip:17  time:594s
fi-skl-6700k     total:279  pass:256  dwarn:4   dfail:0   fail:1   skip:18  time:465s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:477s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:434s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:479s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:419s

e1306e0d65a3a850838534019a9a61e73bf1efcc drm-tip: 2017y-07m-14d-13h-56m-32s UTC integration manifest
3c5f5b7 drm/i915: Fix cursor updates on some platforms

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5192/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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.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ä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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.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.

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

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

[Paul Menzel]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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.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

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