[PATCH 0/4] drm/i915: Cursor fixes and cleanups

[PATCH 0/4] drm/i915: Cursor fixes and cleanups

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I had to look at the cursor code recently, and as usual I ended up
doing bit of fixing and some cleanups. There's still more that needs
doing, but I'll leave that for later and not continue down this path
for now.

While trying to figure out what this CURSIZE register is I also ended
up implementing variable cursor size support for 845/865. I don't have
the hardware so I can't say whether it really works. But here's the
code anyway.

Ville Syrjälä (4):
  drm/i915: Don't try to enable cursor from setplane when crtc is
    disabled
  drm/i915: Move CURSIZE setup to i845_update_cursor()
  drm/i915: Unify ivb_update_cursor() and i9xx_update_cursor()
  drm/i915: Add support for variable cursor size on 845/865

 drivers/gpu/drm/i915/i915_reg.h      |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 134 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 3 files changed, 67 insertions(+), 71 deletions(-)

-- 
1.8.5.5

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

[PATCH 1/4] drm/i915: Don't try to enable cursor from setplane when crtc is disabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure the cursor gets fully clipped when enabling it on a disabled
crtc via setplane. This will prevent the lower level code from
attempting to enable the cursor in hardware.

Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 511c8f4..123cbf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11701,8 +11701,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
 	};
 	const struct drm_rect clip = {
 		/* integer pixels */
-		.x2 = intel_crtc->config.pipe_src_w,
-		.y2 = intel_crtc->config.pipe_src_h,
+		.x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+		.y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
 	};
 	bool visible;
 	int ret;
-- 
1.8.5.5

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

Re: [PATCH 1/4] drm/i915: Don't try to enable cursor from setplane when crtc is disabled

[Paulo Zanoni]

2014-08-12 13:39 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make sure the cursor gets fully clipped when enabling it on a disabled
> crtc via setplane. This will prevent the lower level code from
> attempting to enable the cursor in hardware.

If this is going to replace part of the fix I recently submitted, it
needs Cc: stable@vger.kernel.org.

I briefly smoke-tested it and it appears to properly replace the
"intel_crtc->active" early return which you pointed.

>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 511c8f4..123cbf1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11701,8 +11701,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>         };
>         const struct drm_rect clip = {
>                 /* integer pixels */
> -               .x2 = intel_crtc->config.pipe_src_w,
> -               .y2 = intel_crtc->config.pipe_src_h,
> +               .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
> +               .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
>         };
>         bool visible;
>         int ret;
> --
> 1.8.5.5
>



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

Re: [PATCH 1/4] drm/i915: Don't try to enable cursor from setplane when crtc is disabled

[Jani Nikula]

On Tue, 12 Aug 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2014-08-12 13:39 GMT-03:00  <ville.syrjala@linux.intel.com>:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Make sure the cursor gets fully clipped when enabling it on a disabled
>> crtc via setplane. This will prevent the lower level code from
>> attempting to enable the cursor in hardware.
>
> If this is going to replace part of the fix I recently submitted, it
> needs Cc: stable@vger.kernel.org.

Picked this up for -fixes.

BR,
Jani.

>
> I briefly smoke-tested it and it appears to properly replace the
> "intel_crtc->active" early return which you pointed.
>
>>
>> Cc: Paulo Zanoni <przanoni@gmail.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 511c8f4..123cbf1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11701,8 +11701,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
>>         };
>>         const struct drm_rect clip = {
>>                 /* integer pixels */
>> -               .x2 = intel_crtc->config.pipe_src_w,
>> -               .y2 = intel_crtc->config.pipe_src_h,
>> +               .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
>> +               .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
>>         };
>>         bool visible;
>>         int ret;
>> --
>> 1.8.5.5
>>
>
>
>
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Move CURSIZE setup to i845_update_cursor()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CURSIZE register exists on 845/865 only, so move it to
i845_update_cursor(). Changes to cursor size must be done only when the
cursor is disabled, so do the write just before enabling the cursor.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 123cbf1..744c161 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8028,6 +8028,7 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
 			CURSOR_GAMMA_ENABLE |
 			CURSOR_FORMAT_ARGB);
 	if (intel_crtc->cursor_cntl != cntl) {
+		I915_WRITE(CURSIZE, (64 << 12) | 64);
 		I915_WRITE(_CURACNTR, cntl);
 		POSTING_READ(_CURACNTR);
 		intel_crtc->cursor_cntl = cntl;
@@ -8177,7 +8178,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 				     uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	unsigned old_width;
@@ -8250,9 +8250,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 		addr = obj->phys_handle->busaddr;
 	}
 
-	if (IS_GEN2(dev))
-		I915_WRITE(CURSIZE, (height << 12) | width);
-
  finish:
 	if (intel_crtc->cursor_bo) {
 		if (!INTEL_INFO(dev)->cursor_needs_physical)
-- 
1.8.5.5

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

[PATCH 3/4] drm/i915: Unify ivb_update_cursor() and i9xx_update_cursor()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Ever since
 commit 5efb3e2838536832c9b6872512e6b6daf592cee9
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Wed Apr 9 13:28:53 2014 +0300

    drm/i915/chv: Add cursor pipe offsets

the only difference between i9xx_update_cursor() and ivb_update_cursor()
was the hsw+ pipe csc handling. Let's unify them and we can rid
outselves of some duplicated code.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 41 +-----------------------------------
 1 file changed, 1 insertion(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 744c161..a1cf052 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8062,43 +8062,6 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 		}
 		cntl |= pipe << 28; /* Connect to correct pipe */
 	}
-	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 */
-	I915_WRITE(CURBASE(pipe), base);
-	POSTING_READ(CURBASE(pipe));
-}
-
-static void ivb_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);
-	int pipe = intel_crtc->pipe;
-	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;
-			case 128:
-				cntl |= CURSOR_MODE_128_ARGB_AX;
-				break;
-			case 256:
-				cntl |= CURSOR_MODE_256_ARGB_AX;
-				break;
-			default:
-				WARN_ON(1);
-				return;
-		}
-	}
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		cntl |= CURSOR_PIPE_CSC_ENABLE;
 
@@ -8157,9 +8120,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 
 	I915_WRITE(CURPOS(pipe), pos);
 
-	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
-		ivb_update_cursor(crtc, base);
-	else if (IS_845G(dev) || IS_I865G(dev))
+	if (IS_845G(dev) || IS_I865G(dev))
 		i845_update_cursor(crtc, base);
 	else
 		i9xx_update_cursor(crtc, base);
-- 
1.8.5.5

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

[PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

845/865 support different cursor sizes as well, albeit a bit differently
than later platforms. Add the necessary code to make them work.

Untested due to lack of hardware.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  3 +-
 drivers/gpu/drm/i915/intel_display.c | 86 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 64 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f79c20d..203062e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4128,7 +4128,8 @@ enum punit_power_well {
 /* Old style CUR*CNTR flags (desktop 8xx) */
 #define   CURSOR_ENABLE		0x80000000
 #define   CURSOR_GAMMA_ENABLE	0x40000000
-#define   CURSOR_STRIDE_MASK	0x30000000
+#define   CURSOR_STRIDE_SHIFT	28
+#define   CURSOR_STRIDE(x)	((ffs(x)-9) << CURSOR_STRIDE_SHIFT) /* 256,512,1k,2k */
 #define   CURSOR_PIPE_CSC_ENABLE (1<<24)
 #define   CURSOR_FORMAT_SHIFT	24
 #define   CURSOR_FORMAT_MASK	(0x07 << CURSOR_FORMAT_SHIFT)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..db10870 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8005,30 +8005,53 @@ 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);
-	uint32_t cntl;
+	uint32_t cntl = 0, size = 0;
 
-	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;
+	if (base) {
+		unsigned int width = intel_crtc->cursor_width;
+		unsigned int height = intel_crtc->cursor_height;
+		unsigned int stride = roundup_pow_of_two(width) * 4;
+
+		switch (stride) {
+		case 256:
+		case 512:
+		case 1024:
+		case 2048:
+			cntl |= CURSOR_STRIDE(stride);
+			break;
+		default:
+			WARN_ON(1);
+			return;
 		}
 
+		cntl |= CURSOR_ENABLE |
+			CURSOR_GAMMA_ENABLE |
+			CURSOR_FORMAT_ARGB;
+
+		size = (height << 12) | width;
+	}
+
+	if (intel_crtc->cursor_cntl != 0 &&
+	    (intel_crtc->cursor_base != base ||
+	     intel_crtc->cursor_size != size ||
+	     intel_crtc->cursor_cntl != cntl)) {
+		/* On these chipsets we can only modify the base/size/stride
+		 * whilst the cursor is disabled.
+		 */
+		I915_WRITE(_CURACNTR, 0);
+		POSTING_READ(_CURACNTR);
+		intel_crtc->cursor_cntl = 0;
+	}
+
+	if (intel_crtc->cursor_base != base)
 		I915_WRITE(_CURABASE, base);
-		POSTING_READ(_CURABASE);
+
+	if (intel_crtc->cursor_size != size) {
+		I915_WRITE(CURSIZE, size);
+		intel_crtc->cursor_size = size;
 	}
 
-	/* XXX width must be 64, stride 256 => 0x00 << 28 */
-	cntl = 0;
-	if (base)
-		cntl = (CURSOR_ENABLE |
-			CURSOR_GAMMA_ENABLE |
-			CURSOR_FORMAT_ARGB);
 	if (intel_crtc->cursor_cntl != cntl) {
-		I915_WRITE(CURSIZE, (64 << 12) | 64);
 		I915_WRITE(_CURACNTR, cntl);
 		POSTING_READ(_CURACNTR);
 		intel_crtc->cursor_cntl = cntl;
@@ -8141,7 +8164,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
+	unsigned old_width, stride;
 	uint32_t addr;
 	int ret;
 
@@ -8155,14 +8178,23 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	}
 
 	/* Check for which cursor types we support */
-	if (!((width == 64 && height == 64) ||
-			(width == 128 && height == 128 && !IS_GEN2(dev)) ||
-			(width == 256 && height == 256 && !IS_GEN2(dev)))) {
-		DRM_DEBUG("Cursor dimension not supported\n");
-		return -EINVAL;
+	if (IS_845G(dev) || IS_I865G(dev)) {
+		if (width == 0 || height == 0 || (width & 63) != 0 ||
+		    width > (IS_845G(dev) ? 64 : 512) || height > 1023) {
+			DRM_DEBUG("Cursor dimension not supported\n");
+			return -EINVAL;
+		}
+	} else {
+		if (!((width == 64 && height == 64) ||
+		      (width == 128 && height == 128 && !IS_GEN2(dev)) ||
+		      (width == 256 && height == 256 && !IS_GEN2(dev)))) {
+			DRM_DEBUG("Cursor dimension not supported\n");
+			return -EINVAL;
+		}
 	}
 
-	if (obj->base.size < width * height * 4) {
+	stride = roundup_pow_of_two(width) * 4;
+	if (obj->base.size < stride * height) {
 		DRM_DEBUG_KMS("buffer is too small\n");
 		ret = -ENOMEM;
 		goto fail;
@@ -11755,6 +11787,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	intel_crtc->cursor_base = ~0;
 	intel_crtc->cursor_cntl = ~0;
+	intel_crtc->cursor_size = ~0;
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
@@ -12543,7 +12576,10 @@ void intel_modeset_init(struct drm_device *dev)
 		dev->mode_config.max_height = 8192;
 	}
 
-	if (IS_GEN2(dev)) {
+	if (IS_845G(dev) || IS_I865G(dev)) {
+		dev->mode_config.cursor_width = IS_845G(dev) ? 64 : 512;
+		dev->mode_config.cursor_height = 1023;
+	} else if (IS_GEN2(dev)) {
 		dev->mode_config.cursor_width = GEN2_CURSOR_WIDTH;
 		dev->mode_config.cursor_height = GEN2_CURSOR_HEIGHT;
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7..3cde050 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -405,6 +405,7 @@ struct intel_crtc {
 	uint32_t cursor_addr;
 	int16_t cursor_width, cursor_height;
 	uint32_t cursor_cntl;
+	uint32_t cursor_size;
 	uint32_t cursor_base;
 
 	struct intel_plane_config plane_config;
-- 
1.8.5.5

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

Re: [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865

[Chris Wilson]

On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 845/865 support different cursor sizes as well, albeit a bit differently
> than later platforms. Add the necessary code to make them work.
> 
> Untested due to lack of hardware.

Oh that looks fun. First 3 patches are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

This (4/4) requires a bit more checking and testing...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865

[Chris Wilson]

On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrjala@linux.intel.com wrote:
>  	/* Check for which cursor types we support */
> -	if (!((width == 64 && height == 64) ||
> -			(width == 128 && height == 128 && !IS_GEN2(dev)) ||
> -			(width == 256 && height == 256 && !IS_GEN2(dev)))) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> -		return -EINVAL;
> +	if (IS_845G(dev) || IS_I865G(dev)) {
> +		if (width == 0 || height == 0 || (width & 63) != 0 ||
> +		    width > (IS_845G(dev) ? 64 : 512) || height > 1023) {
> +			DRM_DEBUG("Cursor dimension not supported\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		if (!((width == 64 && height == 64) ||
> +		      (width == 128 && height == 128 && !IS_GEN2(dev)) ||
> +		      (width == 256 && height == 256 && !IS_GEN2(dev)))) {
> +			DRM_DEBUG("Cursor dimension not supported\n");
> +			return -EINVAL;

Whilst changing this code, could we rewrite this sanely?

	switch (width | height) { // fails width==0 xor height==0
	case 128:
	case 256: if (!IS_GEN2(dev))
	case 64: break;
	default:
		 DRM_DEBUG("Cursor dimension not supported\n");
		 return -EINVAL;
	}

Ok, maybe I was having too much fun, but there are simpler ways of
writing that predicate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865

[Chris Wilson]

On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrjala@linux.intel.com wrote:
>  	/* Check for which cursor types we support */
> -	if (!((width == 64 && height == 64) ||
> -			(width == 128 && height == 128 && !IS_GEN2(dev)) ||
> -			(width == 256 && height == 256 && !IS_GEN2(dev)))) {
> -		DRM_DEBUG("Cursor dimension not supported\n");
> -		return -EINVAL;
> +	if (IS_845G(dev) || IS_I865G(dev)) {
> +		if (width == 0 || height == 0 || (width & 63) != 0 ||
> +		    width > (IS_845G(dev) ? 64 : 512) || height > 1023) {
> +			DRM_DEBUG("Cursor dimension not supported\n");
> +			return -EINVAL;
> +		}


This is the cursor size in pixels and cannot be greater than the stride value being used.
[DevBDG]: In all modes the size is fixed to be 64. 
[DevSDG]: In all modes the size must be an integer mutliple of 64.

That does the right thing, though I had to read it through a couple of times.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865

[Chris Wilson]

On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 845/865 support different cursor sizes as well, albeit a bit differently
> than later platforms. Add the necessary code to make them work.
> 
> Untested due to lack of hardware.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  3 +-
>  drivers/gpu/drm/i915/intel_display.c | 86 +++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  3 files changed, 64 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f79c20d..203062e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4128,7 +4128,8 @@ enum punit_power_well {
>  /* Old style CUR*CNTR flags (desktop 8xx) */
>  #define   CURSOR_ENABLE		0x80000000
>  #define   CURSOR_GAMMA_ENABLE	0x40000000
> -#define   CURSOR_STRIDE_MASK	0x30000000
> +#define   CURSOR_STRIDE_SHIFT	28
> +#define   CURSOR_STRIDE(x)	((ffs(x)-9) << CURSOR_STRIDE_SHIFT) /* 256,512,1k,2k */
>  #define   CURSOR_PIPE_CSC_ENABLE (1<<24)
>  #define   CURSOR_FORMAT_SHIFT	24
>  #define   CURSOR_FORMAT_MASK	(0x07 << CURSOR_FORMAT_SHIFT)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a1cf052..db10870 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8005,30 +8005,53 @@ 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);
> -	uint32_t cntl;
> +	uint32_t cntl = 0, size = 0;
>  
> -	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;
> +	if (base) {
> +		unsigned int width = intel_crtc->cursor_width;
> +		unsigned int height = intel_crtc->cursor_height;
> +		unsigned int stride = roundup_pow_of_two(width) * 4;
> +
> +		switch (stride) {
> +		case 256:
> +		case 512:
> +		case 1024:
> +		case 2048:
> +			cntl |= CURSOR_STRIDE(stride);
> +			break;
> +		default:
> +			WARN_ON(1);
> +			return;

Hmm, this is just a programming error. I would rather

switch (stride) {
default:
	WARN_ONCE(1,
		  "Invalid cursor width/stride, width=%d, stride=%d\n",
		  width, stride));
	stride = 256;
	/* fallthrough */
case 256:
case 512:
case 1024:
case 2048:
	break;
}

You get the warning about the programming bug, the user keeps his cursor
(even a corrupt cursor is better than none, and is much easier to
spot!).

Sadly, I only have 845g and so since I only use square cursors in the
ddx I don't actually have the test case I thought I did.

Still,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
(preferrably with the minor suggestions taken into account, if not acted
upon :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v4 4/4] drm/i915: Add support for variable cursor size on 845/865

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

845/865 support different cursor sizes as well, albeit a bit differently
than later platforms. Add the necessary code to make them work.

Untested due to lack of hardware.

v2: Warn but accept invalid stride (Chris)
    Rewrite the cursor size checks for other platforms (Chris)
v3: More polish and magic to the cursor size checks (Chris)
v4: Moar polish and a comment (Chris)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   3 +-
 drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 3 files changed, 91 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f79c20d..203062e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4128,7 +4128,8 @@ enum punit_power_well {
 /* Old style CUR*CNTR flags (desktop 8xx) */
 #define   CURSOR_ENABLE		0x80000000
 #define   CURSOR_GAMMA_ENABLE	0x40000000
-#define   CURSOR_STRIDE_MASK	0x30000000
+#define   CURSOR_STRIDE_SHIFT	28
+#define   CURSOR_STRIDE(x)	((ffs(x)-9) << CURSOR_STRIDE_SHIFT) /* 256,512,1k,2k */
 #define   CURSOR_PIPE_CSC_ENABLE (1<<24)
 #define   CURSOR_FORMAT_SHIFT	24
 #define   CURSOR_FORMAT_MASK	(0x07 << CURSOR_FORMAT_SHIFT)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..0cefd15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8005,30 +8005,55 @@ 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);
-	uint32_t cntl;
+	uint32_t cntl = 0, size = 0;
 
-	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;
+	if (base) {
+		unsigned int width = intel_crtc->cursor_width;
+		unsigned int height = intel_crtc->cursor_height;
+		unsigned int stride = roundup_pow_of_two(width) * 4;
+
+		switch (stride) {
+		default:
+			WARN_ONCE(1, "Invalid cursor width/stride, width=%u, stride=%u\n",
+				  width, stride);
+			stride = 256;
+			/* fallthrough */
+		case 256:
+		case 512:
+		case 1024:
+		case 2048:
+			break;
 		}
 
+		cntl |= CURSOR_ENABLE |
+			CURSOR_GAMMA_ENABLE |
+			CURSOR_FORMAT_ARGB |
+			CURSOR_STRIDE(stride);
+
+		size = (height << 12) | width;
+	}
+
+	if (intel_crtc->cursor_cntl != 0 &&
+	    (intel_crtc->cursor_base != base ||
+	     intel_crtc->cursor_size != size ||
+	     intel_crtc->cursor_cntl != cntl)) {
+		/* On these chipsets we can only modify the base/size/stride
+		 * whilst the cursor is disabled.
+		 */
+		I915_WRITE(_CURACNTR, 0);
+		POSTING_READ(_CURACNTR);
+		intel_crtc->cursor_cntl = 0;
+	}
+
+	if (intel_crtc->cursor_base != base)
 		I915_WRITE(_CURABASE, base);
-		POSTING_READ(_CURABASE);
+
+	if (intel_crtc->cursor_size != size) {
+		I915_WRITE(CURSIZE, size);
+		intel_crtc->cursor_size = size;
 	}
 
-	/* XXX width must be 64, stride 256 => 0x00 << 28 */
-	cntl = 0;
-	if (base)
-		cntl = (CURSOR_ENABLE |
-			CURSOR_GAMMA_ENABLE |
-			CURSOR_FORMAT_ARGB);
 	if (intel_crtc->cursor_cntl != cntl) {
-		I915_WRITE(CURSIZE, (64 << 12) | 64);
 		I915_WRITE(_CURACNTR, cntl);
 		POSTING_READ(_CURACNTR);
 		intel_crtc->cursor_cntl = cntl;
@@ -8127,6 +8152,43 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	intel_crtc->cursor_base = base;
 }
 
+static bool cursor_size_ok(struct drm_device *dev,
+			   uint32_t width, uint32_t height)
+{
+	if (width == 0 || height == 0)
+		return false;
+
+	/*
+	 * 845g/865g are special in that they are only limited by
+	 * the width of their cursors, the height is arbitrary up to
+	 * the precision of the register. Everything else requires
+	 * square cursors, limited to a few power-of-two sizes.
+	 */
+	if (IS_845G(dev) || IS_I865G(dev)) {
+		if ((width & 63) != 0)
+			return false;
+
+		if (width > (IS_845G(dev) ? 64 : 512))
+			return false;
+
+		if (height > 1023)
+			return false;
+	} else {
+		switch (width | height) {
+		case 256:
+		case 128:
+			if (IS_GEN2(dev))
+				return false;
+		case 64:
+			break;
+		default:
+			return false;
+		}
+	}
+
+	return true;
+}
+
 /*
  * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
  *
@@ -8141,7 +8203,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	unsigned old_width;
+	unsigned old_width, stride;
 	uint32_t addr;
 	int ret;
 
@@ -8155,14 +8217,13 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
 	}
 
 	/* Check for which cursor types we support */
-	if (!((width == 64 && height == 64) ||
-			(width == 128 && height == 128 && !IS_GEN2(dev)) ||
-			(width == 256 && height == 256 && !IS_GEN2(dev)))) {
+	if (!cursor_size_ok(dev, width, height)) {
 		DRM_DEBUG("Cursor dimension not supported\n");
 		return -EINVAL;
 	}
 
-	if (obj->base.size < width * height * 4) {
+	stride = roundup_pow_of_two(width) * 4;
+	if (obj->base.size < stride * height) {
 		DRM_DEBUG_KMS("buffer is too small\n");
 		ret = -ENOMEM;
 		goto fail;
@@ -11755,6 +11816,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 
 	intel_crtc->cursor_base = ~0;
 	intel_crtc->cursor_cntl = ~0;
+	intel_crtc->cursor_size = ~0;
 
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
@@ -12543,7 +12605,10 @@ void intel_modeset_init(struct drm_device *dev)
 		dev->mode_config.max_height = 8192;
 	}
 
-	if (IS_GEN2(dev)) {
+	if (IS_845G(dev) || IS_I865G(dev)) {
+		dev->mode_config.cursor_width = IS_845G(dev) ? 64 : 512;
+		dev->mode_config.cursor_height = 1023;
+	} else if (IS_GEN2(dev)) {
 		dev->mode_config.cursor_width = GEN2_CURSOR_WIDTH;
 		dev->mode_config.cursor_height = GEN2_CURSOR_HEIGHT;
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7..3cde050 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -405,6 +405,7 @@ struct intel_crtc {
 	uint32_t cursor_addr;
 	int16_t cursor_width, cursor_height;
 	uint32_t cursor_cntl;
+	uint32_t cursor_size;
 	uint32_t cursor_base;
 
 	struct intel_plane_config plane_config;
-- 
1.8.5.5

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

Re: [PATCH v4 4/4] drm/i915: Add support for variable cursor size on 845/865

[Daniel Vetter]

On Wed, Aug 13, 2014 at 11:57:05AM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 845/865 support different cursor sizes as well, albeit a bit differently
> than later platforms. Add the necessary code to make them work.
> 
> Untested due to lack of hardware.
> 
> v2: Warn but accept invalid stride (Chris)
>     Rewrite the cursor size checks for other platforms (Chris)
> v3: More polish and magic to the cursor size checks (Chris)
> v4: Moar polish and a comment (Chris)
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

All merged to dinq, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   3 +-
>  drivers/gpu/drm/i915/intel_display.c | 111 +++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  3 files changed, 91 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f79c20d..203062e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4128,7 +4128,8 @@ enum punit_power_well {
>  /* Old style CUR*CNTR flags (desktop 8xx) */
>  #define   CURSOR_ENABLE		0x80000000
>  #define   CURSOR_GAMMA_ENABLE	0x40000000
> -#define   CURSOR_STRIDE_MASK	0x30000000
> +#define   CURSOR_STRIDE_SHIFT	28
> +#define   CURSOR_STRIDE(x)	((ffs(x)-9) << CURSOR_STRIDE_SHIFT) /* 256,512,1k,2k */
>  #define   CURSOR_PIPE_CSC_ENABLE (1<<24)
>  #define   CURSOR_FORMAT_SHIFT	24
>  #define   CURSOR_FORMAT_MASK	(0x07 << CURSOR_FORMAT_SHIFT)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a1cf052..0cefd15 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8005,30 +8005,55 @@ 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);
> -	uint32_t cntl;
> +	uint32_t cntl = 0, size = 0;
>  
> -	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;
> +	if (base) {
> +		unsigned int width = intel_crtc->cursor_width;
> +		unsigned int height = intel_crtc->cursor_height;
> +		unsigned int stride = roundup_pow_of_two(width) * 4;
> +
> +		switch (stride) {
> +		default:
> +			WARN_ONCE(1, "Invalid cursor width/stride, width=%u, stride=%u\n",
> +				  width, stride);
> +			stride = 256;
> +			/* fallthrough */
> +		case 256:
> +		case 512:
> +		case 1024:
> +		case 2048:
> +			break;
>  		}
>  
> +		cntl |= CURSOR_ENABLE |
> +			CURSOR_GAMMA_ENABLE |
> +			CURSOR_FORMAT_ARGB |
> +			CURSOR_STRIDE(stride);
> +
> +		size = (height << 12) | width;
> +	}
> +
> +	if (intel_crtc->cursor_cntl != 0 &&
> +	    (intel_crtc->cursor_base != base ||
> +	     intel_crtc->cursor_size != size ||
> +	     intel_crtc->cursor_cntl != cntl)) {
> +		/* On these chipsets we can only modify the base/size/stride
> +		 * whilst the cursor is disabled.
> +		 */
> +		I915_WRITE(_CURACNTR, 0);
> +		POSTING_READ(_CURACNTR);
> +		intel_crtc->cursor_cntl = 0;
> +	}
> +
> +	if (intel_crtc->cursor_base != base)
>  		I915_WRITE(_CURABASE, base);
> -		POSTING_READ(_CURABASE);
> +
> +	if (intel_crtc->cursor_size != size) {
> +		I915_WRITE(CURSIZE, size);
> +		intel_crtc->cursor_size = size;
>  	}
>  
> -	/* XXX width must be 64, stride 256 => 0x00 << 28 */
> -	cntl = 0;
> -	if (base)
> -		cntl = (CURSOR_ENABLE |
> -			CURSOR_GAMMA_ENABLE |
> -			CURSOR_FORMAT_ARGB);
>  	if (intel_crtc->cursor_cntl != cntl) {
> -		I915_WRITE(CURSIZE, (64 << 12) | 64);
>  		I915_WRITE(_CURACNTR, cntl);
>  		POSTING_READ(_CURACNTR);
>  		intel_crtc->cursor_cntl = cntl;
> @@ -8127,6 +8152,43 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	intel_crtc->cursor_base = base;
>  }
>  
> +static bool cursor_size_ok(struct drm_device *dev,
> +			   uint32_t width, uint32_t height)
> +{
> +	if (width == 0 || height == 0)
> +		return false;
> +
> +	/*
> +	 * 845g/865g are special in that they are only limited by
> +	 * the width of their cursors, the height is arbitrary up to
> +	 * the precision of the register. Everything else requires
> +	 * square cursors, limited to a few power-of-two sizes.
> +	 */
> +	if (IS_845G(dev) || IS_I865G(dev)) {
> +		if ((width & 63) != 0)
> +			return false;
> +
> +		if (width > (IS_845G(dev) ? 64 : 512))
> +			return false;
> +
> +		if (height > 1023)
> +			return false;
> +	} else {
> +		switch (width | height) {
> +		case 256:
> +		case 128:
> +			if (IS_GEN2(dev))
> +				return false;
> +		case 64:
> +			break;
> +		default:
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * intel_crtc_cursor_set_obj - Set cursor to specified GEM object
>   *
> @@ -8141,7 +8203,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	enum pipe pipe = intel_crtc->pipe;
> -	unsigned old_width;
> +	unsigned old_width, stride;
>  	uint32_t addr;
>  	int ret;
>  
> @@ -8155,14 +8217,13 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	}
>  
>  	/* Check for which cursor types we support */
> -	if (!((width == 64 && height == 64) ||
> -			(width == 128 && height == 128 && !IS_GEN2(dev)) ||
> -			(width == 256 && height == 256 && !IS_GEN2(dev)))) {
> +	if (!cursor_size_ok(dev, width, height)) {
>  		DRM_DEBUG("Cursor dimension not supported\n");
>  		return -EINVAL;
>  	}
>  
> -	if (obj->base.size < width * height * 4) {
> +	stride = roundup_pow_of_two(width) * 4;
> +	if (obj->base.size < stride * height) {
>  		DRM_DEBUG_KMS("buffer is too small\n");
>  		ret = -ENOMEM;
>  		goto fail;
> @@ -11755,6 +11816,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  
>  	intel_crtc->cursor_base = ~0;
>  	intel_crtc->cursor_cntl = ~0;
> +	intel_crtc->cursor_size = ~0;
>  
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
> @@ -12543,7 +12605,10 @@ void intel_modeset_init(struct drm_device *dev)
>  		dev->mode_config.max_height = 8192;
>  	}
>  
> -	if (IS_GEN2(dev)) {
> +	if (IS_845G(dev) || IS_I865G(dev)) {
> +		dev->mode_config.cursor_width = IS_845G(dev) ? 64 : 512;
> +		dev->mode_config.cursor_height = 1023;
> +	} else if (IS_GEN2(dev)) {
>  		dev->mode_config.cursor_width = GEN2_CURSOR_WIDTH;
>  		dev->mode_config.cursor_height = GEN2_CURSOR_HEIGHT;
>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1b3d1d7..3cde050 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -405,6 +405,7 @@ struct intel_crtc {
>  	uint32_t cursor_addr;
>  	int16_t cursor_width, cursor_height;
>  	uint32_t cursor_cntl;
> +	uint32_t cursor_size;
>  	uint32_t cursor_base;
>  
>  	struct intel_plane_config plane_config;
> -- 
> 1.8.5.5
> 
> _______________________________________________
> 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