Re: [PATCH 1/1] drm/i915: Enabling 128x128 and 256x256 ARGB Cursor Support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 18, 2014 at 03:39:46PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> With this patch we allow larger cursor planes of sizes 128x128
> and 256x256. Planning to extend kms_cursor_crc test for verifying
> these larger planes.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org

Let's not spam everyone. Just intel-gfx is enough for such patches.
Well, maybe keep dri-devel too since cursor size seems to be a hot
topic across other drivers currently.

> Signed-off-by: G, Pallavi <pallavi.g@intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++++
>  drivers/gpu/drm/i915/intel_display.c | 25 ++++++++++++++++++++-----
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2f564ce..2fee3a2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3522,7 +3522,11 @@
>  /* New style CUR*CNTR flags */
>  #define   CURSOR_MODE		0x27
>  #define   CURSOR_MODE_DISABLE   0x00
> +#define   CURSOR_MODE_128_32B_AX 0x02
> +#define   CURSOR_MODE_256_32B_AX 0x03
>  #define   CURSOR_MODE_64_32B_AX 0x07
> +#define   CURSOR_MODE_128_ARGB_AX ((1 << 5) | CURSOR_MODE_128_32B_AX)
> +#define   CURSOR_MODE_256_ARGB_AX ((1 << 5) | CURSOR_MODE_256_32B_AX)
>  #define   CURSOR_MODE_64_ARGB_AX ((1 << 5) | CURSOR_MODE_64_32B_AX)
>  #define   MCURSOR_PIPE_SELECT	(1 << 28)
>  #define   MCURSOR_PIPE_A	0x00
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..00b51f3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7411,10 +7411,18 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	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 |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +
> +			if (width == 64)
> +				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +			else if (width == 128)
> +				cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +			else if (width == 256)
> +				cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +
>  			cntl |= pipe << 28; /* Connect to correct pipe */
>  		} else {
>  			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> @@ -7439,10 +7447,17 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  	bool visible = base != 0;
>  
>  	if (intel_crtc->cursor_visible != visible) {
> +		int16_t width = intel_crtc->cursor_width;
>  		uint32_t cntl = I915_READ(CURCNTR_IVB(pipe));
>  		if (base) {
>  			cntl &= ~CURSOR_MODE;
> -			cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +
> +			if (width == 64)
> +				cntl |= CURSOR_MODE_64_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +			else if (width == 128)
> +				cntl |= CURSOR_MODE_128_ARGB_AX | MCURSOR_GAMMA_ENABLE;
> +			else if (width == 256)
> +				cntl |= CURSOR_MODE_256_ARGB_AX | MCURSOR_GAMMA_ENABLE;
>  		} else {
>  			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
>  			cntl |= CURSOR_MODE_DISABLE;
> @@ -7538,9 +7553,9 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  		goto finish;
>  	}
>  
> -	/* Currently we only support 64x64 cursors */
> -	if (width != 64 || height != 64) {
> -		DRM_ERROR("we currently only support 64x64 cursors\n");
> +	/* Check for which cursor types we support */
> +	if (width > 256 || height > 256) {

This has to check explicitly for 64x64, 128x128, or 256x256. Any other
combination of width and height is illegal.

Additionally gen2 supports only 64x64, so that has to be checked also.

Another issue is how to tell userspace about the cursor size. There was
a patch on dri-devel recently adding cursor width/height capability
queries to drm. I guess that should be enough for xorg since AFAICS it
only uses a single fixed cursor size. The downside is that if the actual
cursor image would fit one of the smaller sizes, we end up doing needless
memory fetches for the extra pixels.

Once we have drm_planes for cursors, I was thinking we might add some kind
of enum property that lists all the supported sizes for the plane.

> +		DRM_ERROR("We currently only support 64x64, 128x128, 256x256 cursors\n");
>  		return -EINVAL;
>  	}
>  
> -- 
> 1.8.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC