Re: [PATCH] drm/i915: Add rotation support for cursor plane

[Matt Roper <matthew.d.roper@intel.com>]

On Tue, Oct 07, 2014 at 08:43:46AM +0000, Jindal, Sonika wrote:
> Hi,
> 
> Did anybody get a chance to look at this patch?
> 
> Thanks,
> Sonika

Looks like we waited a bit too long and the codebase has evolved, so I
needed to make some tweaks to your patches to get them to apply cleanly
on the latest di-nightly.  However the overall changes here look good to
me, and we don't seem to be missing any details from the bspec, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

After tweaking your patches to apply against the latest di-nightly, your
i-g-t test runs properly for me on IVB, as well as another simple test I
wrote myself, so you can also put

Tested-by: Matt Roper <matthew.d.roper@intel.com>

Let me know if you want the rebased copies of the patches I used for
testing.


Matt

> 
> -----Original Message-----
> From: Jindal, Sonika 
> Sent: Monday, September 15, 2014 1:14 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä; Kamble, Sagar A; Jindal, Sonika
> Subject: [PATCH] drm/i915: Add rotation support for cursor plane
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The cursor plane also supports 180 degree rotation. Add a new "cursor-rotation" property on the crtc which controls this.
> 
> Unlike sprites, the cursor has a fixed size, so if you have a small cursor image with the rest of the bo filled by transparent pixels, simply flipping the rotation property will cause the visible part of the cursor to shift. This is something to keep in mind when using cursor rotation.
> 
> v2: Fix gen4/vlv by offsetting the base address appropriately
> 
> v3: Removing cursor-rotation property and using rotation property on cursor plane.
> v4: Changing the author name back to Ville.
> 
> Testcase: kms_rotation_crc
> 
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   26 +++++++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 15c0eaa..5a9fab9 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4162,6 +4162,7 @@ enum punit_power_well {
>  #define   MCURSOR_PIPE_A	0x00
>  #define   MCURSOR_PIPE_B	(1 << 28)
>  #define   MCURSOR_GAMMA_ENABLE  (1 << 26)
> +#define   CURSOR_ROTATE_180	(1<<15)
>  #define   CURSOR_TRICKLE_FEED_DISABLE	(1 << 14)
>  #define _CURABASE		0x70084
>  #define _CURAPOS		0x70088
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 122ac6e..8c83bcc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8247,6 +8247,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		cntl |= CURSOR_PIPE_CSC_ENABLE;
>  
> +	if (to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180))
> +		cntl |= CURSOR_ROTATE_180;
> +
>  	if (intel_crtc->cursor_cntl != cntl) {
>  		I915_WRITE(CURCNTR(pipe), cntl);
>  		POSTING_READ(CURCNTR(pipe));
> @@ -8302,6 +8305,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  
>  	I915_WRITE(CURPOS(pipe), pos);
>  
> +	/* ILK+ do this automagically */

Minor note, but it might be nice to clarify this with a little more
detail.  Something like "Gen4 and Valley View expect the address to
point to the lower right corner for 180-rotated cursors.  Other
platforms expect the address to point to the top-left corner regardless
of rotation."


> +	if (HAS_GMCH_DISPLAY(dev) &&
> +		to_intel_plane(crtc->cursor)->rotation == BIT(DRM_ROTATE_180)) {
> +		base += (intel_crtc->cursor_height *
> +			intel_crtc->cursor_width - 1) * 4;
> +	}
> +
>  	if (IS_845G(dev) || IS_I865G(dev))
>  		i845_update_cursor(crtc, base);
>  	else
> @@ -8453,7 +8463,6 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc *crtc,
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	old_width = intel_crtc->cursor_width;
> -

Unintentional whitespace change?

>  	intel_crtc->cursor_addr = addr;
>  	intel_crtc->cursor_bo = obj;
>  	intel_crtc->cursor_width = width;
> @@ -12074,6 +12083,7 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>  	.update_plane = intel_cursor_plane_update,
>  	.disable_plane = intel_cursor_plane_disable,
>  	.destroy = intel_plane_destroy,
> +	.set_property = intel_plane_set_property,
>  };
>  
>  static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev, @@ -12089,12 +12099,26 @@ static struct drm_plane *intel_cursor_plane_create(struct drm_device *dev,
>  	cursor->max_downscale = 1;
>  	cursor->pipe = pipe;
>  	cursor->plane = pipe;
> +	cursor->rotation = BIT(DRM_ROTATE_0);
>  
>  	drm_universal_plane_init(dev, &cursor->base, 0,
>  				 &intel_cursor_plane_funcs,
>  				 intel_cursor_formats,
>  				 ARRAY_SIZE(intel_cursor_formats),
>  				 DRM_PLANE_TYPE_CURSOR);
> +
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (!dev->mode_config.rotation_property)
> +			dev->mode_config.rotation_property =
> +				drm_mode_create_rotation_property(dev,
> +							BIT(DRM_ROTATE_0) |
> +							BIT(DRM_ROTATE_180));
> +		if (dev->mode_config.rotation_property)
> +			drm_object_attach_property(&cursor->base.base,
> +				dev->mode_config.rotation_property,
> +				cursor->rotation);
> +	}
> +
>  	return &cursor->base;
>  }
>  
> --
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795