* [PATCH v2] drm/i915: Always apply cursor width changes
@ 2014-05-30 13:35 Chris Wilson
2014-06-02 8:49 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2014-05-30 13:35 UTC (permalink / raw)
To: intel-gfx
It is possible for userspace to create a big object large enough for a
256x256, and then switch over to using it as a 64x64 cursor. This
requires the cursor update routines to check for a change in width on
every update, rather than just when the cursor is originally enabled.
This also fixes an issue with 845g/865g which cannot change the base
address of the cursor whilst it is active.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
[Antti:rebased, adjusted macro names and moved some lines, no functional
changes]
Reviewed-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
Tested-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
drivers/gpu/drm/i915/intel_drv.h | 3 +-
3 files changed, 53 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2e5f76a..04d0b7c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2353,7 +2353,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
active = cursor_position(dev, crtc->pipe, &x, &y);
seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
- yesno(crtc->cursor_visible),
+ yesno(crtc->cursor_base),
x, y, crtc->cursor_addr,
yesno(active));
}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 731cd01..955f92d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7872,29 +7872,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- bool visible = base != 0;
- u32 cntl;
+ uint32_t cntl;
- if (intel_crtc->cursor_visible == visible)
- return;
-
- cntl = I915_READ(_CURACNTR);
- if (visible) {
+ if (base != intel_crtc->cursor_base) {
/* On these chipsets we can only modify the base whilst
* the cursor is disabled.
*/
+ if (intel_crtc->cursor_cntl) {
+ I915_WRITE(_CURACNTR, 0);
+ POSTING_READ(_CURACNTR);
+ intel_crtc->cursor_cntl = 0;
+ }
+
I915_WRITE(_CURABASE, base);
+ POSTING_READ(_CURABASE);
+ }
- cntl &= ~(CURSOR_FORMAT_MASK);
- /* XXX width must be 64, stride 256 => 0x00 << 28 */
- cntl |= CURSOR_ENABLE |
+ /* XXX width must be 64, stride 256 => 0x00 << 28 */
+ cntl = 0;
+ if (base)
+ cntl = (CURSOR_ENABLE |
CURSOR_GAMMA_ENABLE |
- CURSOR_FORMAT_ARGB;
- } else
- cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
- I915_WRITE(_CURACNTR, cntl);
-
- intel_crtc->cursor_visible = visible;
+ CURSOR_FORMAT_ARGB);
+ if (intel_crtc->cursor_cntl != cntl) {
+ I915_WRITE(_CURACNTR, cntl);
+ POSTING_READ(_CURACNTR);
+ intel_crtc->cursor_cntl = cntl;
+ }
}
static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
@@ -7903,16 +7907,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- bool visible = base != 0;
-
- if (intel_crtc->cursor_visible != visible) {
- int16_t width = intel_crtc->cursor_width;
- uint32_t cntl = I915_READ(CURCNTR(pipe));
- if (base) {
- cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
- cntl |= MCURSOR_GAMMA_ENABLE;
+ uint32_t cntl;
- switch (width) {
+ cntl = 0;
+ if (base) {
+ cntl = MCURSOR_GAMMA_ENABLE;
+ switch (intel_crtc->cursor_width) {
case 64:
cntl |= CURSOR_MODE_64_ARGB_AX;
break;
@@ -7925,18 +7925,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
default:
WARN_ON(1);
return;
- }
- cntl |= pipe << 28; /* Connect to correct pipe */
- } else {
- cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
- cntl |= CURSOR_MODE_DISABLE;
}
+ cntl |= pipe << 28; /* Connect to correct pipe */
+ }
+ if (intel_crtc->cursor_cntl != cntl) {
I915_WRITE(CURCNTR(pipe), cntl);
-
- intel_crtc->cursor_visible = visible;
+ POSTING_READ(CURCNTR(pipe));
+ intel_crtc->cursor_cntl = cntl;
}
+
/* and commit changes on next vblank */
- POSTING_READ(CURCNTR(pipe));
I915_WRITE(CURBASE(pipe), base);
POSTING_READ(CURBASE(pipe));
}
@@ -7947,15 +7945,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
int pipe = intel_crtc->pipe;
- bool visible = base != 0;
-
- if (intel_crtc->cursor_visible != visible) {
- int16_t width = intel_crtc->cursor_width;
- uint32_t cntl = I915_READ(CURCNTR(pipe));
- if (base) {
- cntl &= ~CURSOR_MODE;
- cntl |= MCURSOR_GAMMA_ENABLE;
- switch (width) {
+ uint32_t cntl;
+
+ cntl = 0;
+ if (base) {
+ cntl = MCURSOR_GAMMA_ENABLE;
+ switch (intel_crtc->cursor_width) {
case 64:
cntl |= CURSOR_MODE_64_ARGB_AX;
break;
@@ -7968,21 +7963,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
default:
WARN_ON(1);
return;
- }
- } else {
- cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
- cntl |= CURSOR_MODE_DISABLE;
- }
- if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- cntl |= CURSOR_PIPE_CSC_ENABLE;
- cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
}
- I915_WRITE(CURCNTR(pipe), cntl);
+ }
+ if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+ cntl |= CURSOR_PIPE_CSC_ENABLE;
- intel_crtc->cursor_visible = visible;
+ if (intel_crtc->cursor_cntl != cntl) {
+ I915_WRITE(CURCNTR(pipe), cntl);
+ POSTING_READ(CURCNTR(pipe));
+ intel_crtc->cursor_cntl = cntl;
}
+
/* and commit changes on next vblank */
- POSTING_READ(CURCNTR(pipe));
I915_WRITE(CURBASE(pipe), base);
POSTING_READ(CURBASE(pipe));
}
@@ -7998,7 +7990,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
int x = intel_crtc->cursor_x;
int y = intel_crtc->cursor_y;
u32 base = 0, pos = 0;
- bool visible;
if (on)
base = intel_crtc->cursor_addr;
@@ -8027,8 +8018,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
}
pos |= y << CURSOR_Y_SHIFT;
- visible = base != 0;
- if (!visible && !intel_crtc->cursor_visible)
+ if (base == 0 && intel_crtc->cursor_base == 0)
return;
I915_WRITE(CURPOS(pipe), pos);
@@ -8039,6 +8029,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
i845_update_cursor(crtc, base);
else
i9xx_update_cursor(crtc, base);
+ intel_crtc->cursor_base = base;
}
static int intel_crtc_cursor_set(struct drm_crtc *crtc,
@@ -10970,6 +10961,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
intel_crtc->plane = !pipe;
}
+ intel_crtc->cursor_base = ~0;
+ intel_crtc->cursor_cntl = ~0;
+
init_waitqueue_head(&intel_crtc->vbl_wait);
BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9bb70dc..9d1ff7b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -386,7 +386,8 @@ struct intel_crtc {
uint32_t cursor_addr;
int16_t cursor_x, cursor_y;
int16_t cursor_width, cursor_height;
- bool cursor_visible;
+ uint32_t cursor_cntl;
+ uint32_t cursor_base;
struct intel_plane_config plane_config;
struct intel_crtc_config config;
--
1.8.3.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/i915: Always apply cursor width changes
2014-05-30 13:35 [PATCH v2] drm/i915: Always apply cursor width changes Chris Wilson
@ 2014-06-02 8:49 ` Daniel Vetter
2014-06-02 10:46 ` Antti Koskipää
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2014-06-02 8:49 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Fri, May 30, 2014 at 04:35:26PM +0300, Chris Wilson wrote:
> It is possible for userspace to create a big object large enough for a
> 256x256, and then switch over to using it as a 64x64 cursor. This
> requires the cursor update routines to check for a change in width on
> every update, rather than just when the cursor is originally enabled.
>
> This also fixes an issue with 845g/865g which cannot change the base
> address of the cursor whilst it is active.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> [Antti:rebased, adjusted macro names and moved some lines, no functional
> changes]
> Reviewed-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> Tested-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
Still missing the igt testcase. Is Antti still working on that one? And I
guess now we also need an Cc: stable@vger.kernel.org on this one here.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> 3 files changed, 53 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2e5f76a..04d0b7c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2353,7 +2353,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
>
> active = cursor_position(dev, crtc->pipe, &x, &y);
> seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
> - yesno(crtc->cursor_visible),
> + yesno(crtc->cursor_base),
> x, y, crtc->cursor_addr,
> yesno(active));
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 731cd01..955f92d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7872,29 +7872,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - bool visible = base != 0;
> - u32 cntl;
> + uint32_t cntl;
>
> - if (intel_crtc->cursor_visible == visible)
> - return;
> -
> - cntl = I915_READ(_CURACNTR);
> - if (visible) {
> + if (base != intel_crtc->cursor_base) {
> /* On these chipsets we can only modify the base whilst
> * the cursor is disabled.
> */
> + if (intel_crtc->cursor_cntl) {
> + I915_WRITE(_CURACNTR, 0);
> + POSTING_READ(_CURACNTR);
> + intel_crtc->cursor_cntl = 0;
> + }
> +
> I915_WRITE(_CURABASE, base);
> + POSTING_READ(_CURABASE);
> + }
>
> - cntl &= ~(CURSOR_FORMAT_MASK);
> - /* XXX width must be 64, stride 256 => 0x00 << 28 */
> - cntl |= CURSOR_ENABLE |
> + /* XXX width must be 64, stride 256 => 0x00 << 28 */
> + cntl = 0;
> + if (base)
> + cntl = (CURSOR_ENABLE |
> CURSOR_GAMMA_ENABLE |
> - CURSOR_FORMAT_ARGB;
> - } else
> - cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> - I915_WRITE(_CURACNTR, cntl);
> -
> - intel_crtc->cursor_visible = visible;
> + CURSOR_FORMAT_ARGB);
> + if (intel_crtc->cursor_cntl != cntl) {
> + I915_WRITE(_CURACNTR, cntl);
> + POSTING_READ(_CURACNTR);
> + intel_crtc->cursor_cntl = cntl;
> + }
> }
>
> static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> @@ -7903,16 +7907,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> - bool visible = base != 0;
> -
> - if (intel_crtc->cursor_visible != visible) {
> - int16_t width = intel_crtc->cursor_width;
> - uint32_t cntl = I915_READ(CURCNTR(pipe));
> - if (base) {
> - cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> - cntl |= MCURSOR_GAMMA_ENABLE;
> + uint32_t cntl;
>
> - switch (width) {
> + cntl = 0;
> + if (base) {
> + cntl = MCURSOR_GAMMA_ENABLE;
> + switch (intel_crtc->cursor_width) {
> case 64:
> cntl |= CURSOR_MODE_64_ARGB_AX;
> break;
> @@ -7925,18 +7925,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> default:
> WARN_ON(1);
> return;
> - }
> - cntl |= pipe << 28; /* Connect to correct pipe */
> - } else {
> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> - cntl |= CURSOR_MODE_DISABLE;
> }
> + cntl |= pipe << 28; /* Connect to correct pipe */
> + }
> + if (intel_crtc->cursor_cntl != cntl) {
> I915_WRITE(CURCNTR(pipe), cntl);
> -
> - intel_crtc->cursor_visible = visible;
> + POSTING_READ(CURCNTR(pipe));
> + intel_crtc->cursor_cntl = cntl;
> }
> +
> /* and commit changes on next vblank */
> - POSTING_READ(CURCNTR(pipe));
> I915_WRITE(CURBASE(pipe), base);
> POSTING_READ(CURBASE(pipe));
> }
> @@ -7947,15 +7945,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> int pipe = intel_crtc->pipe;
> - bool visible = base != 0;
> -
> - if (intel_crtc->cursor_visible != visible) {
> - int16_t width = intel_crtc->cursor_width;
> - uint32_t cntl = I915_READ(CURCNTR(pipe));
> - if (base) {
> - cntl &= ~CURSOR_MODE;
> - cntl |= MCURSOR_GAMMA_ENABLE;
> - switch (width) {
> + uint32_t cntl;
> +
> + cntl = 0;
> + if (base) {
> + cntl = MCURSOR_GAMMA_ENABLE;
> + switch (intel_crtc->cursor_width) {
> case 64:
> cntl |= CURSOR_MODE_64_ARGB_AX;
> break;
> @@ -7968,21 +7963,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> default:
> WARN_ON(1);
> return;
> - }
> - } else {
> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> - cntl |= CURSOR_MODE_DISABLE;
> - }
> - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> - cntl |= CURSOR_PIPE_CSC_ENABLE;
> - cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
> }
> - I915_WRITE(CURCNTR(pipe), cntl);
> + }
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> + cntl |= CURSOR_PIPE_CSC_ENABLE;
>
> - intel_crtc->cursor_visible = visible;
> + if (intel_crtc->cursor_cntl != cntl) {
> + I915_WRITE(CURCNTR(pipe), cntl);
> + POSTING_READ(CURCNTR(pipe));
> + intel_crtc->cursor_cntl = cntl;
> }
> +
> /* and commit changes on next vblank */
> - POSTING_READ(CURCNTR(pipe));
> I915_WRITE(CURBASE(pipe), base);
> POSTING_READ(CURBASE(pipe));
> }
> @@ -7998,7 +7990,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> int x = intel_crtc->cursor_x;
> int y = intel_crtc->cursor_y;
> u32 base = 0, pos = 0;
> - bool visible;
>
> if (on)
> base = intel_crtc->cursor_addr;
> @@ -8027,8 +8018,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> }
> pos |= y << CURSOR_Y_SHIFT;
>
> - visible = base != 0;
> - if (!visible && !intel_crtc->cursor_visible)
> + if (base == 0 && intel_crtc->cursor_base == 0)
> return;
>
> I915_WRITE(CURPOS(pipe), pos);
> @@ -8039,6 +8029,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> i845_update_cursor(crtc, base);
> else
> i9xx_update_cursor(crtc, base);
> + intel_crtc->cursor_base = base;
> }
>
> static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -10970,6 +10961,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> intel_crtc->plane = !pipe;
> }
>
> + intel_crtc->cursor_base = ~0;
> + intel_crtc->cursor_cntl = ~0;
> +
> init_waitqueue_head(&intel_crtc->vbl_wait);
>
> BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9bb70dc..9d1ff7b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -386,7 +386,8 @@ struct intel_crtc {
> uint32_t cursor_addr;
> int16_t cursor_x, cursor_y;
> int16_t cursor_width, cursor_height;
> - bool cursor_visible;
> + uint32_t cursor_cntl;
> + uint32_t cursor_base;
>
> struct intel_plane_config plane_config;
> struct intel_crtc_config config;
> --
> 1.8.3.2
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/i915: Always apply cursor width changes
2014-06-02 8:49 ` Daniel Vetter
@ 2014-06-02 10:46 ` Antti Koskipää
2014-06-02 15:34 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Antti Koskipää @ 2014-06-02 10:46 UTC (permalink / raw)
To: daniel@ffwll.ch; +Cc: intel-gfx
On 06/02/2014 11:49 AM, Daniel Vetter wrote:
> On Fri, May 30, 2014 at 04:35:26PM +0300, Chris Wilson wrote:
>> It is possible for userspace to create a big object large enough for a
>> 256x256, and then switch over to using it as a 64x64 cursor. This
>> requires the cursor update routines to check for a change in width on
>> every update, rather than just when the cursor is originally enabled.
>>
>> This also fixes an issue with 845g/865g which cannot change the base
>> address of the cursor whilst it is active.
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> [Antti:rebased, adjusted macro names and moved some lines, no functional
>> changes]
>> Reviewed-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>> Tested-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
>
> Still missing the igt testcase. Is Antti still working on that one?
The testcase was just posted. It just needed some cleanup.
> And I
> guess now we also need an Cc: stable@vger.kernel.org on this one here.
> -Daniel
>
>> ---
>> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
>> drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
>> drivers/gpu/drm/i915/intel_drv.h | 3 +-
>> 3 files changed, 53 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 2e5f76a..04d0b7c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -2353,7 +2353,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
>>
>> active = cursor_position(dev, crtc->pipe, &x, &y);
>> seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
>> - yesno(crtc->cursor_visible),
>> + yesno(crtc->cursor_base),
>> x, y, crtc->cursor_addr,
>> yesno(active));
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 731cd01..955f92d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -7872,29 +7872,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>> struct drm_device *dev = crtc->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> - bool visible = base != 0;
>> - u32 cntl;
>> + uint32_t cntl;
>>
>> - if (intel_crtc->cursor_visible == visible)
>> - return;
>> -
>> - cntl = I915_READ(_CURACNTR);
>> - if (visible) {
>> + if (base != intel_crtc->cursor_base) {
>> /* On these chipsets we can only modify the base whilst
>> * the cursor is disabled.
>> */
>> + if (intel_crtc->cursor_cntl) {
>> + I915_WRITE(_CURACNTR, 0);
>> + POSTING_READ(_CURACNTR);
>> + intel_crtc->cursor_cntl = 0;
>> + }
>> +
>> I915_WRITE(_CURABASE, base);
>> + POSTING_READ(_CURABASE);
>> + }
>>
>> - cntl &= ~(CURSOR_FORMAT_MASK);
>> - /* XXX width must be 64, stride 256 => 0x00 << 28 */
>> - cntl |= CURSOR_ENABLE |
>> + /* XXX width must be 64, stride 256 => 0x00 << 28 */
>> + cntl = 0;
>> + if (base)
>> + cntl = (CURSOR_ENABLE |
>> CURSOR_GAMMA_ENABLE |
>> - CURSOR_FORMAT_ARGB;
>> - } else
>> - cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
>> - I915_WRITE(_CURACNTR, cntl);
>> -
>> - intel_crtc->cursor_visible = visible;
>> + CURSOR_FORMAT_ARGB);
>> + if (intel_crtc->cursor_cntl != cntl) {
>> + I915_WRITE(_CURACNTR, cntl);
>> + POSTING_READ(_CURACNTR);
>> + intel_crtc->cursor_cntl = cntl;
>> + }
>> }
>>
>> static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>> @@ -7903,16 +7907,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> int pipe = intel_crtc->pipe;
>> - bool visible = base != 0;
>> -
>> - if (intel_crtc->cursor_visible != visible) {
>> - int16_t width = intel_crtc->cursor_width;
>> - uint32_t cntl = I915_READ(CURCNTR(pipe));
>> - if (base) {
>> - cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
>> - cntl |= MCURSOR_GAMMA_ENABLE;
>> + uint32_t cntl;
>>
>> - switch (width) {
>> + cntl = 0;
>> + if (base) {
>> + cntl = MCURSOR_GAMMA_ENABLE;
>> + switch (intel_crtc->cursor_width) {
>> case 64:
>> cntl |= CURSOR_MODE_64_ARGB_AX;
>> break;
>> @@ -7925,18 +7925,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>> default:
>> WARN_ON(1);
>> return;
>> - }
>> - cntl |= pipe << 28; /* Connect to correct pipe */
>> - } else {
>> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
>> - cntl |= CURSOR_MODE_DISABLE;
>> }
>> + cntl |= pipe << 28; /* Connect to correct pipe */
>> + }
>> + if (intel_crtc->cursor_cntl != cntl) {
>> I915_WRITE(CURCNTR(pipe), cntl);
>> -
>> - intel_crtc->cursor_visible = visible;
>> + POSTING_READ(CURCNTR(pipe));
>> + intel_crtc->cursor_cntl = cntl;
>> }
>> +
>> /* and commit changes on next vblank */
>> - POSTING_READ(CURCNTR(pipe));
>> I915_WRITE(CURBASE(pipe), base);
>> POSTING_READ(CURBASE(pipe));
>> }
>> @@ -7947,15 +7945,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> int pipe = intel_crtc->pipe;
>> - bool visible = base != 0;
>> -
>> - if (intel_crtc->cursor_visible != visible) {
>> - int16_t width = intel_crtc->cursor_width;
>> - uint32_t cntl = I915_READ(CURCNTR(pipe));
>> - if (base) {
>> - cntl &= ~CURSOR_MODE;
>> - cntl |= MCURSOR_GAMMA_ENABLE;
>> - switch (width) {
>> + uint32_t cntl;
>> +
>> + cntl = 0;
>> + if (base) {
>> + cntl = MCURSOR_GAMMA_ENABLE;
>> + switch (intel_crtc->cursor_width) {
>> case 64:
>> cntl |= CURSOR_MODE_64_ARGB_AX;
>> break;
>> @@ -7968,21 +7963,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>> default:
>> WARN_ON(1);
>> return;
>> - }
>> - } else {
>> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
>> - cntl |= CURSOR_MODE_DISABLE;
>> - }
>> - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>> - cntl |= CURSOR_PIPE_CSC_ENABLE;
>> - cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
>> }
>> - I915_WRITE(CURCNTR(pipe), cntl);
>> + }
>> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>> + cntl |= CURSOR_PIPE_CSC_ENABLE;
>>
>> - intel_crtc->cursor_visible = visible;
>> + if (intel_crtc->cursor_cntl != cntl) {
>> + I915_WRITE(CURCNTR(pipe), cntl);
>> + POSTING_READ(CURCNTR(pipe));
>> + intel_crtc->cursor_cntl = cntl;
>> }
>> +
>> /* and commit changes on next vblank */
>> - POSTING_READ(CURCNTR(pipe));
>> I915_WRITE(CURBASE(pipe), base);
>> POSTING_READ(CURBASE(pipe));
>> }
>> @@ -7998,7 +7990,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>> int x = intel_crtc->cursor_x;
>> int y = intel_crtc->cursor_y;
>> u32 base = 0, pos = 0;
>> - bool visible;
>>
>> if (on)
>> base = intel_crtc->cursor_addr;
>> @@ -8027,8 +8018,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>> }
>> pos |= y << CURSOR_Y_SHIFT;
>>
>> - visible = base != 0;
>> - if (!visible && !intel_crtc->cursor_visible)
>> + if (base == 0 && intel_crtc->cursor_base == 0)
>> return;
>>
>> I915_WRITE(CURPOS(pipe), pos);
>> @@ -8039,6 +8029,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>> i845_update_cursor(crtc, base);
>> else
>> i9xx_update_cursor(crtc, base);
>> + intel_crtc->cursor_base = base;
>> }
>>
>> static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>> @@ -10970,6 +10961,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>> intel_crtc->plane = !pipe;
>> }
>>
>> + intel_crtc->cursor_base = ~0;
>> + intel_crtc->cursor_cntl = ~0;
>> +
>> init_waitqueue_head(&intel_crtc->vbl_wait);
>>
>> BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9bb70dc..9d1ff7b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -386,7 +386,8 @@ struct intel_crtc {
>> uint32_t cursor_addr;
>> int16_t cursor_x, cursor_y;
>> int16_t cursor_width, cursor_height;
>> - bool cursor_visible;
>> + uint32_t cursor_cntl;
>> + uint32_t cursor_base;
>>
>> struct intel_plane_config plane_config;
>> struct intel_crtc_config config;
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
- Antti
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] drm/i915: Always apply cursor width changes
2014-06-02 10:46 ` Antti Koskipää
@ 2014-06-02 15:34 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-06-02 15:34 UTC (permalink / raw)
To: Antti Koskipää; +Cc: intel-gfx
On Mon, Jun 02, 2014 at 01:46:11PM +0300, Antti Koskipää wrote:
> On 06/02/2014 11:49 AM, Daniel Vetter wrote:
> > On Fri, May 30, 2014 at 04:35:26PM +0300, Chris Wilson wrote:
> >> It is possible for userspace to create a big object large enough for a
> >> 256x256, and then switch over to using it as a 64x64 cursor. This
> >> requires the cursor update routines to check for a change in width on
> >> every update, rather than just when the cursor is originally enabled.
> >>
> >> This also fixes an issue with 845g/865g which cannot change the base
> >> address of the cursor whilst it is active.
> >>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> [Antti:rebased, adjusted macro names and moved some lines, no functional
> >> changes]
> >> Reviewed-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> >> Tested-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> >
> > Still missing the igt testcase. Is Antti still working on that one?
>
> The testcase was just posted. It just needed some cleanup.
Cool. Patch is queued for -next, thanks for the patch.
-Daniel
>
> > And I
> > guess now we also need an Cc: stable@vger.kernel.org on this one here.
> > -Daniel
> >
> >> ---
> >> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> >> drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
> >> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> >> 3 files changed, 53 insertions(+), 58 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >> index 2e5f76a..04d0b7c 100644
> >> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >> @@ -2353,7 +2353,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
> >>
> >> active = cursor_position(dev, crtc->pipe, &x, &y);
> >> seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
> >> - yesno(crtc->cursor_visible),
> >> + yesno(crtc->cursor_base),
> >> x, y, crtc->cursor_addr,
> >> yesno(active));
> >> }
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 731cd01..955f92d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -7872,29 +7872,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> >> struct drm_device *dev = crtc->dev;
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> - bool visible = base != 0;
> >> - u32 cntl;
> >> + uint32_t cntl;
> >>
> >> - if (intel_crtc->cursor_visible == visible)
> >> - return;
> >> -
> >> - cntl = I915_READ(_CURACNTR);
> >> - if (visible) {
> >> + if (base != intel_crtc->cursor_base) {
> >> /* On these chipsets we can only modify the base whilst
> >> * the cursor is disabled.
> >> */
> >> + if (intel_crtc->cursor_cntl) {
> >> + I915_WRITE(_CURACNTR, 0);
> >> + POSTING_READ(_CURACNTR);
> >> + intel_crtc->cursor_cntl = 0;
> >> + }
> >> +
> >> I915_WRITE(_CURABASE, base);
> >> + POSTING_READ(_CURABASE);
> >> + }
> >>
> >> - cntl &= ~(CURSOR_FORMAT_MASK);
> >> - /* XXX width must be 64, stride 256 => 0x00 << 28 */
> >> - cntl |= CURSOR_ENABLE |
> >> + /* XXX width must be 64, stride 256 => 0x00 << 28 */
> >> + cntl = 0;
> >> + if (base)
> >> + cntl = (CURSOR_ENABLE |
> >> CURSOR_GAMMA_ENABLE |
> >> - CURSOR_FORMAT_ARGB;
> >> - } else
> >> - cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> >> - I915_WRITE(_CURACNTR, cntl);
> >> -
> >> - intel_crtc->cursor_visible = visible;
> >> + CURSOR_FORMAT_ARGB);
> >> + if (intel_crtc->cursor_cntl != cntl) {
> >> + I915_WRITE(_CURACNTR, cntl);
> >> + POSTING_READ(_CURACNTR);
> >> + intel_crtc->cursor_cntl = cntl;
> >> + }
> >> }
> >>
> >> static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >> @@ -7903,16 +7907,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> int pipe = intel_crtc->pipe;
> >> - bool visible = base != 0;
> >> -
> >> - if (intel_crtc->cursor_visible != visible) {
> >> - int16_t width = intel_crtc->cursor_width;
> >> - uint32_t cntl = I915_READ(CURCNTR(pipe));
> >> - if (base) {
> >> - cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> >> - cntl |= MCURSOR_GAMMA_ENABLE;
> >> + uint32_t cntl;
> >>
> >> - switch (width) {
> >> + cntl = 0;
> >> + if (base) {
> >> + cntl = MCURSOR_GAMMA_ENABLE;
> >> + switch (intel_crtc->cursor_width) {
> >> case 64:
> >> cntl |= CURSOR_MODE_64_ARGB_AX;
> >> break;
> >> @@ -7925,18 +7925,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> >> default:
> >> WARN_ON(1);
> >> return;
> >> - }
> >> - cntl |= pipe << 28; /* Connect to correct pipe */
> >> - } else {
> >> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> >> - cntl |= CURSOR_MODE_DISABLE;
> >> }
> >> + cntl |= pipe << 28; /* Connect to correct pipe */
> >> + }
> >> + if (intel_crtc->cursor_cntl != cntl) {
> >> I915_WRITE(CURCNTR(pipe), cntl);
> >> -
> >> - intel_crtc->cursor_visible = visible;
> >> + POSTING_READ(CURCNTR(pipe));
> >> + intel_crtc->cursor_cntl = cntl;
> >> }
> >> +
> >> /* and commit changes on next vblank */
> >> - POSTING_READ(CURCNTR(pipe));
> >> I915_WRITE(CURBASE(pipe), base);
> >> POSTING_READ(CURBASE(pipe));
> >> }
> >> @@ -7947,15 +7945,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> >> struct drm_i915_private *dev_priv = dev->dev_private;
> >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >> int pipe = intel_crtc->pipe;
> >> - bool visible = base != 0;
> >> -
> >> - if (intel_crtc->cursor_visible != visible) {
> >> - int16_t width = intel_crtc->cursor_width;
> >> - uint32_t cntl = I915_READ(CURCNTR(pipe));
> >> - if (base) {
> >> - cntl &= ~CURSOR_MODE;
> >> - cntl |= MCURSOR_GAMMA_ENABLE;
> >> - switch (width) {
> >> + uint32_t cntl;
> >> +
> >> + cntl = 0;
> >> + if (base) {
> >> + cntl = MCURSOR_GAMMA_ENABLE;
> >> + switch (intel_crtc->cursor_width) {
> >> case 64:
> >> cntl |= CURSOR_MODE_64_ARGB_AX;
> >> break;
> >> @@ -7968,21 +7963,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> >> default:
> >> WARN_ON(1);
> >> return;
> >> - }
> >> - } else {
> >> - cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> >> - cntl |= CURSOR_MODE_DISABLE;
> >> - }
> >> - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> >> - cntl |= CURSOR_PIPE_CSC_ENABLE;
> >> - cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
> >> }
> >> - I915_WRITE(CURCNTR(pipe), cntl);
> >> + }
> >> + if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >> + cntl |= CURSOR_PIPE_CSC_ENABLE;
> >>
> >> - intel_crtc->cursor_visible = visible;
> >> + if (intel_crtc->cursor_cntl != cntl) {
> >> + I915_WRITE(CURCNTR(pipe), cntl);
> >> + POSTING_READ(CURCNTR(pipe));
> >> + intel_crtc->cursor_cntl = cntl;
> >> }
> >> +
> >> /* and commit changes on next vblank */
> >> - POSTING_READ(CURCNTR(pipe));
> >> I915_WRITE(CURBASE(pipe), base);
> >> POSTING_READ(CURBASE(pipe));
> >> }
> >> @@ -7998,7 +7990,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >> int x = intel_crtc->cursor_x;
> >> int y = intel_crtc->cursor_y;
> >> u32 base = 0, pos = 0;
> >> - bool visible;
> >>
> >> if (on)
> >> base = intel_crtc->cursor_addr;
> >> @@ -8027,8 +8018,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >> }
> >> pos |= y << CURSOR_Y_SHIFT;
> >>
> >> - visible = base != 0;
> >> - if (!visible && !intel_crtc->cursor_visible)
> >> + if (base == 0 && intel_crtc->cursor_base == 0)
> >> return;
> >>
> >> I915_WRITE(CURPOS(pipe), pos);
> >> @@ -8039,6 +8029,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> >> i845_update_cursor(crtc, base);
> >> else
> >> i9xx_update_cursor(crtc, base);
> >> + intel_crtc->cursor_base = base;
> >> }
> >>
> >> static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> >> @@ -10970,6 +10961,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> >> intel_crtc->plane = !pipe;
> >> }
> >>
> >> + intel_crtc->cursor_base = ~0;
> >> + intel_crtc->cursor_cntl = ~0;
> >> +
> >> init_waitqueue_head(&intel_crtc->vbl_wait);
> >>
> >> BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 9bb70dc..9d1ff7b 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -386,7 +386,8 @@ struct intel_crtc {
> >> uint32_t cursor_addr;
> >> int16_t cursor_x, cursor_y;
> >> int16_t cursor_width, cursor_height;
> >> - bool cursor_visible;
> >> + uint32_t cursor_cntl;
> >> + uint32_t cursor_base;
> >>
> >> struct intel_plane_config plane_config;
> >> struct intel_crtc_config config;
> >> --
> >> 1.8.3.2
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
>
> --
> - Antti
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-06-02 15:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-30 13:35 [PATCH v2] drm/i915: Always apply cursor width changes Chris Wilson
2014-06-02 8:49 ` Daniel Vetter
2014-06-02 10:46 ` Antti Koskipää
2014-06-02 15:34 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox