[4/5] drm/i915: Add rotation support for the cursor plane

Message ID	1392239704-21776-5-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> From: ville.syrjala@linux.intel.com To: intel-gfx@lists.freedesktop.org Subject: [PATCH 4/5] drm/i915: Add rotation support for the cursor plane Date: Wed, 12 Feb 2014 23:15:03 +0200 Message-Id: <1392239704-21776-5-git-send-email-ville.syrjala@linux.intel.com> In-Reply-To: <1392239704-21776-1-git-send-email-ville.syrjala@linux.intel.com> References: <1392239704-21776-1-git-send-email-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Cc: Sagar Kamble <sagar.a.kamble@intel.com>, dri-devel@lists.freedesktop.org Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org

Ville Syrjälä Feb. 12, 2014, 9:15 p.m. UTC

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.

Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  9 ++++++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 5 files changed, 51 insertions(+), 20 deletions(-)

sagar.a.kamble@intel.com Feb. 14, 2014, 11:01 a.m. UTC | #1

On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> 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.
By flipping you meant setting 180 degree rotation?
Don't we have to adjust the cursor base as well to the lower right
corner apart from setting the control bit?
> 
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  9 ++++++
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  5 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 4a3ef34..3dd9abb 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1916,6 +1916,15 @@ void i915_driver_lastclose(struct drm_device * dev)
>  		}
>  	}
>  
> +	if (dev_priv->cursor_rotation_property) {
> +		list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +			crtc->cursor_rotation = BIT(DRM_ROTATE_0);
> +			drm_object_property_set_value(&crtc->base.base,
> +						dev_priv->cursor_rotation_property,
> +						crtc->cursor_rotation);
> +		}
> +	}
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		intel_fbdev_restore_mode(dev);
>  		vga_switcheroo_process_delayed_switch();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6af78ee..8ecd3bb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1562,6 +1562,7 @@ typedef struct drm_i915_private {
>  	struct drm_property *force_audio_property;
>  	struct drm_property *rotation_property; /* "rotation" */
>  	struct drm_property *plane_rotation_property; /* "plane-rotation" */
> +	struct drm_property *cursor_rotation_property; /* "cursor-rotation" */
>  
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ab1aeb82..fee068a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3534,6 +3534,7 @@
>  #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		(dev_priv->info.display_mmio_offset + 0x70084)
>  #define _CURAPOS		(dev_priv->info.display_mmio_offset + 0x70088)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 37b23d1..e94167b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -42,7 +42,7 @@
>  #include <linux/dma_remapping.h>
>  
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
> -static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> +static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on, bool force);
>  
>  static void i9xx_crtc_clock_get(struct intel_crtc *crtc,
>  				struct intel_crtc_config *pipe_config);
> @@ -3640,7 +3640,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	intel_enable_pipe(intel_crtc);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, true, false);
>  
>  	if (intel_crtc->config.has_pch_encoder)
>  		ironlake_pch_enable(crtc);
> @@ -3682,7 +3682,7 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc)
>  
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, true, false);
>  
>  	hsw_enable_ips(intel_crtc);
>  
> @@ -3708,7 +3708,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
>  
>  	hsw_disable_ips(intel_crtc);
>  
> -	intel_crtc_update_cursor(crtc, false);
> +	intel_crtc_update_cursor(crtc, false, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_plane(dev_priv, plane, pipe);
>  }
> @@ -3836,7 +3836,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
>  
> -	intel_crtc_update_cursor(crtc, false);
> +	intel_crtc_update_cursor(crtc, false, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_plane(dev_priv, plane, pipe);
>  
> @@ -4211,7 +4211,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>  	intel_enable_primary_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, true, false);
>  
>  	intel_update_fbc(dev);
>  
> @@ -4253,7 +4253,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	/* The fixup needs to happen before cursor is enabled */
>  	if (IS_G4X(dev))
>  		g4x_fixup_plane(dev_priv, pipe);
> -	intel_crtc_update_cursor(crtc, true);
> +	intel_crtc_update_cursor(crtc, true, false);
>  
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
>  	intel_crtc_dpms_overlay(intel_crtc, true);
> @@ -4302,7 +4302,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  		intel_disable_fbc(dev);
>  
>  	intel_crtc_dpms_overlay(intel_crtc, false);
> -	intel_crtc_update_cursor(crtc, false);
> +	intel_crtc_update_cursor(crtc, false, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_plane(dev_priv, plane, pipe);
>  
> @@ -7429,7 +7429,7 @@ void intel_write_eld(struct drm_encoder *encoder,
>  		dev_priv->display.write_eld(connector, crtc, mode);
>  }
>  
> -static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
> +static void i845_update_cursor(struct drm_crtc *crtc, u32 base, bool force)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7437,7 +7437,7 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>  	bool visible = base != 0;
>  	u32 cntl;
>  
> -	if (intel_crtc->cursor_visible == visible)
> +	if (!force && intel_crtc->cursor_visible == visible)
>  		return;
>  
>  	cntl = I915_READ(_CURACNTR);
> @@ -7459,7 +7459,7 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>  	intel_crtc->cursor_visible = visible;
>  }
>  
> -static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> +static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, bool force)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7467,7 +7467,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	int pipe = intel_crtc->pipe;
>  	bool visible = base != 0;
>  
> -	if (intel_crtc->cursor_visible != visible) {
> +	if (force || intel_crtc->cursor_visible != visible) {
>  		uint32_t cntl = I915_READ(CURCNTR(pipe));
>  		if (base) {
>  			cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> @@ -7477,6 +7477,10 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
>  			cntl |= CURSOR_MODE_DISABLE;
>  		}
> +		if (intel_crtc->cursor_rotation == BIT(DRM_ROTATE_180))
> +			cntl |= CURSOR_ROTATE_180;
> +		else
> +			cntl &= ~CURSOR_ROTATE_180;
>  		I915_WRITE(CURCNTR(pipe), cntl);
>  
>  		intel_crtc->cursor_visible = visible;
> @@ -7487,7 +7491,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	POSTING_READ(CURBASE(pipe));
>  }
>  
> -static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
> +static void ivb_update_cursor(struct drm_crtc *crtc, u32 base, bool force)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7495,7 +7499,7 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  	int pipe = intel_crtc->pipe;
>  	bool visible = base != 0;
>  
> -	if (intel_crtc->cursor_visible != visible) {
> +	if (force || intel_crtc->cursor_visible != visible) {
>  		uint32_t cntl = I915_READ(CURCNTR_IVB(pipe));
>  		if (base) {
>  			cntl &= ~CURSOR_MODE;
> @@ -7508,6 +7512,10 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  			cntl |= CURSOR_PIPE_CSC_ENABLE;
>  			cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
>  		}
> +		if (intel_crtc->cursor_rotation == BIT(DRM_ROTATE_180))
> +			cntl |= CURSOR_ROTATE_180;
> +		else
> +			cntl &= ~CURSOR_ROTATE_180;
>  		I915_WRITE(CURCNTR_IVB(pipe), cntl);
>  
>  		intel_crtc->cursor_visible = visible;
> @@ -7520,7 +7528,7 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  
>  /* If no-part of the cursor is visible on the framebuffer, then the GPU may hang... */
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc,
> -				     bool on)
> +				     bool on, bool force)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7564,13 +7572,13 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		I915_WRITE(CURPOS_IVB(pipe), pos);
> -		ivb_update_cursor(crtc, base);
> +		ivb_update_cursor(crtc, base, force);
>  	} else {
>  		I915_WRITE(CURPOS(pipe), pos);
>  		if (IS_845G(dev) || IS_I865G(dev))
> -			i845_update_cursor(crtc, base);
> +			i845_update_cursor(crtc, base, force);
>  		else
> -			i9xx_update_cursor(crtc, base);
> +			i9xx_update_cursor(crtc, base, force);
>  	}
>  }
>  
> @@ -7677,7 +7685,7 @@ static int intel_crtc_cursor_set(struct drm_crtc *crtc,
>  	intel_crtc->cursor_height = height;
>  
>  	if (intel_crtc->active)
> -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> +		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL, false);
>  
>  	return 0;
>  fail_unpin:
> @@ -7697,7 +7705,7 @@ static int intel_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	intel_crtc->cursor_y = clamp_t(int, y, SHRT_MIN, SHRT_MAX);
>  
>  	if (intel_crtc->active)
> -		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL);
> +		intel_crtc_update_cursor(crtc, intel_crtc->cursor_bo != NULL, false);
>  
>  	return 0;
>  }
> @@ -10388,6 +10396,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->pipe = pipe;
>  	intel_crtc->plane = pipe;
>  	intel_crtc->primary_rotation = BIT(DRM_ROTATE_0);
> +	intel_crtc->cursor_rotation = BIT(DRM_ROTATE_0);
>  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4) {
>  		DRM_DEBUG_KMS("swapping pipes & planes for FBC\n");
>  		intel_crtc->plane = !pipe;
> @@ -10408,6 +10417,16 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  			drm_object_attach_property(&intel_crtc->base.base,
>  						dev_priv->plane_rotation_property,
>  						intel_crtc->primary_rotation);
> +
> +		if (!dev_priv->cursor_rotation_property)
> +			dev_priv->cursor_rotation_property =
> +				drm_mode_create_rotation_property(dev, "cursor-rotation",
> +								BIT(DRM_ROTATE_0) |
> +								BIT(DRM_ROTATE_180));
> +		if (dev_priv->cursor_rotation_property)
> +			drm_object_attach_property(&intel_crtc->base.base,
> +						dev_priv->cursor_rotation_property,
> +						intel_crtc->cursor_rotation);
>  	}
>  
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8c17a82..4a7f4f1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -332,6 +332,7 @@ struct intel_crtc {
>  	enum pipe pipe;
>  	enum plane plane;
>  	unsigned int primary_rotation; /* primary plane in relation to the pipe */
> +	unsigned int cursor_rotation; /* cursor plane in relation to the pipe */
>  
>  	u8 lut_r[256], lut_g[256], lut_b[256];
>  	/*

Ville Syrjälä Feb. 14, 2014, 11:39 a.m. UTC | #2

On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > 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.
> By flipping you meant setting 180 degree rotation?

Yes.

> Don't we have to adjust the cursor base as well to the lower right
> corner apart from setting the control bit?

No, the hardware does that automagically. Hmm. Except on gen4
apparently. Looks like I need to test on gen4, and fix it if it's
really the case.

sagar.a.kamble@intel.com Feb. 17, 2014, 5:23 p.m. UTC | #3

On Fri, 2014-02-14 at 13:39 +0200, Ville Syrjälä wrote:
> On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > 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.
> > By flipping you meant setting 180 degree rotation?
> 
> Yes.
> 
> > Don't we have to adjust the cursor base as well to the lower right
> > corner apart from setting the control bit?
> 
> No, the hardware does that automagically. Hmm. Except on gen4
> apparently. Looks like I need to test on gen4, and fix it if it's
> really the case.
I tried on BYT system and 180 rotation on cursor plane is showing
garbage data in cursor plane. We might need to adjust the cursor base.
Another thing, pipe rotation somehow did not work for me when I do this:
echo 0x4 > /sys/kernel/debug/dri/0/i915_pipe_rotation
Only cursor plane had impact. Need to debug this as well.
>

Ville Syrjälä Feb. 17, 2014, 5:51 p.m. UTC | #4

On Mon, Feb 17, 2014 at 10:53:50PM +0530, Sagar Arun Kamble wrote:
> On Fri, 2014-02-14 at 13:39 +0200, Ville Syrjälä wrote:
> > On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> > > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > > 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.
> > > By flipping you meant setting 180 degree rotation?
> > 
> > Yes.
> > 
> > > Don't we have to adjust the cursor base as well to the lower right
> > > corner apart from setting the control bit?
> > 
> > No, the hardware does that automagically. Hmm. Except on gen4
> > apparently. Looks like I need to test on gen4, and fix it if it's
> > really the case.
> I tried on BYT system and 180 rotation on cursor plane is showing
> garbage data in cursor plane. We might need to adjust the cursor base.

Yeah it's the same on gen4. I already have a fixed patch, but didn't
repost it yet.

> Another thing, pipe rotation somehow did not work for me when I do this:
> echo 0x4 > /sys/kernel/debug/dri/0/i915_pipe_rotation
> Only cursor plane had impact. Need to debug this as well.

That's expected. It doesn't actually call the set_property codepath,
instead it just sets the value directly and excpects a subsequent
modeset to do the actual work. It was anyway just a hack to try things
out a bit, so I didn't implement it properly. But it should be trivial
to make it work correctly, so I might as well do it...

sagar.a.kamble@intel.com Feb. 18, 2014, 7:49 a.m. UTC | #5

On Mon, 2014-02-17 at 19:51 +0200, Ville Syrjälä wrote:
> On Mon, Feb 17, 2014 at 10:53:50PM +0530, Sagar Arun Kamble wrote:
> > On Fri, 2014-02-14 at 13:39 +0200, Ville Syrjälä wrote:
> > > On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> > > > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > > > 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.
> > > > By flipping you meant setting 180 degree rotation?
> > > 
> > > Yes.
> > > 
> > > > Don't we have to adjust the cursor base as well to the lower right
> > > > corner apart from setting the control bit?
> > > 
> > > No, the hardware does that automagically. Hmm. Except on gen4
> > > apparently. Looks like I need to test on gen4, and fix it if it's
> > > really the case.
> > I tried on BYT system and 180 rotation on cursor plane is showing
> > garbage data in cursor plane. We might need to adjust the cursor base.
> 
> Yeah it's the same on gen4. I already have a fixed patch, but didn't
> repost it yet.
> 
> > Another thing, pipe rotation somehow did not work for me when I do this:
> > echo 0x4 > /sys/kernel/debug/dri/0/i915_pipe_rotation
> > Only cursor plane had impact. Need to debug this as well.
> 
> That's expected. It doesn't actually call the set_property codepath,
> instead it just sets the value directly and excpects a subsequent
> modeset to do the actual work. It was anyway just a hack to try things
> out a bit, so I didn't implement it properly. But it should be trivial
> to make it work correctly, so I might as well do it...
Yeah. Tried doing modeset and it works perfectly.
For Cursor rotation we might need to add check for 32bpp cursors as
well.
>

Ville Syrjälä Feb. 18, 2014, 9:23 a.m. UTC | #6

On Tue, Feb 18, 2014 at 01:19:05PM +0530, Sagar Arun Kamble wrote:
> On Mon, 2014-02-17 at 19:51 +0200, Ville Syrjälä wrote:
> > On Mon, Feb 17, 2014 at 10:53:50PM +0530, Sagar Arun Kamble wrote:
> > > On Fri, 2014-02-14 at 13:39 +0200, Ville Syrjälä wrote:
> > > > On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> > > > > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > > > > 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.
> > > > > By flipping you meant setting 180 degree rotation?
> > > > 
> > > > Yes.
> > > > 
> > > > > Don't we have to adjust the cursor base as well to the lower right
> > > > > corner apart from setting the control bit?
> > > > 
> > > > No, the hardware does that automagically. Hmm. Except on gen4
> > > > apparently. Looks like I need to test on gen4, and fix it if it's
> > > > really the case.
> > > I tried on BYT system and 180 rotation on cursor plane is showing
> > > garbage data in cursor plane. We might need to adjust the cursor base.
> > 
> > Yeah it's the same on gen4. I already have a fixed patch, but didn't
> > repost it yet.
> > 
> > > Another thing, pipe rotation somehow did not work for me when I do this:
> > > echo 0x4 > /sys/kernel/debug/dri/0/i915_pipe_rotation
> > > Only cursor plane had impact. Need to debug this as well.
> > 
> > That's expected. It doesn't actually call the set_property codepath,
> > instead it just sets the value directly and excpects a subsequent
> > modeset to do the actual work. It was anyway just a hack to try things
> > out a bit, so I didn't implement it properly. But it should be trivial
> > to make it work correctly, so I might as well do it...
> Yeah. Tried doing modeset and it works perfectly.
> For Cursor rotation we might need to add check for 32bpp cursors as
> well.

We don't support anything else at the moment. And I don't think there's
much point in adding support for any legacy cursor formats. The one
thing we want to do is add support for larger cursor sizes. But I think
that can wait until we expose the cursor as a drm_plane.

sagar.a.kamble@intel.com Feb. 18, 2014, 10:09 a.m. UTC | #7

On Tue, 2014-02-18 at 11:23 +0200, Ville Syrjälä wrote:
> On Tue, Feb 18, 2014 at 01:19:05PM +0530, Sagar Arun Kamble wrote:
> > On Mon, 2014-02-17 at 19:51 +0200, Ville Syrjälä wrote:
> > > On Mon, Feb 17, 2014 at 10:53:50PM +0530, Sagar Arun Kamble wrote:
> > > > On Fri, 2014-02-14 at 13:39 +0200, Ville Syrjälä wrote:
> > > > > On Fri, Feb 14, 2014 at 04:31:17PM +0530, Sagar Arun Kamble wrote:
> > > > > > On Wed, 2014-02-12 at 23:15 +0200, ville.syrjala@linux.intel.com wrote:
> > > > > > > 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.
> > > > > > By flipping you meant setting 180 degree rotation?
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > Don't we have to adjust the cursor base as well to the lower right
> > > > > > corner apart from setting the control bit?
> > > > > 
> > > > > No, the hardware does that automagically. Hmm. Except on gen4
> > > > > apparently. Looks like I need to test on gen4, and fix it if it's
> > > > > really the case.
> > > > I tried on BYT system and 180 rotation on cursor plane is showing
> > > > garbage data in cursor plane. We might need to adjust the cursor base.
> > > 
> > > Yeah it's the same on gen4. I already have a fixed patch, but didn't
> > > repost it yet.
> > > 
> > > > Another thing, pipe rotation somehow did not work for me when I do this:
> > > > echo 0x4 > /sys/kernel/debug/dri/0/i915_pipe_rotation
> > > > Only cursor plane had impact. Need to debug this as well.
> > > 
> > > That's expected. It doesn't actually call the set_property codepath,
> > > instead it just sets the value directly and excpects a subsequent
> > > modeset to do the actual work. It was anyway just a hack to try things
> > > out a bit, so I didn't implement it properly. But it should be trivial
> > > to make it work correctly, so I might as well do it...
> > Yeah. Tried doing modeset and it works perfectly.
> > For Cursor rotation we might need to add check for 32bpp cursors as
> > well.
> 
> We don't support anything else at the moment. And I don't think there's
> much point in adding support for any legacy cursor formats. The one
> thing we want to do is add support for larger cursor sizes. But I think
> that can wait until we expose the cursor as a drm_plane.
Actually I am working on enabling larger cursor sizes now. Will share
patch now.
>

[4/5] drm/i915: Add rotation support for the cursor plane

Commit Message

Comments

Patch