Re: [PATCH 10/11] drm/i915: Add 180 degree primary plane rotation support

[Damien Lespiau <damien.lespiau@intel.com>]

On Wed, Jun 18, 2014 at 02:27:26PM +0530, sonika.jindal@intel.com wrote:
> From: Sonika Jindal <sonika.jindal@intel.com>
> 
> Primary planes support 180 degree rotation. Expose the feature
> through rotation drm property.
> 
> v2: Calculating linear/tiled offsets based on pipe source width and
> height. Added 180 degree rotation support in ironlake_update_plane.
> 
> v3: Checking if CRTC is active before issueing update_plane. Added
> wait for vblank to make sure we dont overtake page flips. Disabling
> FBC since it does not work with rotated planes.
> 
> v4: Updated rotation checks for pending flips, fbc disable. Creating
> rotation property only for Gen4 onwards. Property resetting as part
> of lastclose.
> 
> v5: Resetting property in i915_driver_lastclose properly for planes
> and crtcs. Fixed linear offset calculation that was off by 1 w.r.t
> width in i9xx_update_plane and ironlake_update_plane. Removed tab
> based indentation and unnecessary braces in intel_crtc_set_property
> and intel_update_fbc. FBC and flip related checks should be done only
> for valid crtcs.
> 
> v6: Minor nits in FBC disable checks for comments in intel_crtc_set_property
> and positioning the disable code in intel_update_fbc.
> 
> v7: In case rotation property on inactive crtc is updated, we return
> successfully printing debug log as crtc is inactive and only property change
> is preserved.
> 
> v8: update_plane is changed to update_primary_plane, crtc->fb is changed to
> crtc->primary->fb  and return value of update_primary_plane is ignored.
> 
> v9: added rotation property to primary plane instead of crtc.
> 
> Testcase: igt/kms_rotation_crc
> 
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: David Airlie <airlied at linux.ie>
> Cc: dri-devel at lists.freedesktop.org
> Cc: linux-kernel at vger.kernel.org
> Cc: vijay.a.purushothaman at intel.com
> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble at intel.com>

Some checkpatch.pl warnings:

ERROR: code indent should use tabs where possible
#145: FILE: drivers/gpu/drm/i915/intel_display.c:2488:
+                                   (intel_crtc->config.pipe_src_w - 1) * pixel_size;$

WARNING: please, no spaces at the start of a line
#145: FILE: drivers/gpu/drm/i915/intel_display.c:2488:
+                                   (intel_crtc->config.pipe_src_w - 1) * pixel_size;$


> ---
>  drivers/gpu/drm/i915/i915_dma.c      |   17 +++++++
>  drivers/gpu/drm/i915/i915_reg.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   91 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c      |    8 +++
>  4 files changed, 113 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5e583a1..4c91fbc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1939,6 +1939,8 @@ int i915_driver_open(struct drm_device *dev, struct drm_file *file)
>  void i915_driver_lastclose(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *crtc;
> +	struct intel_plane *plane;
>  
>  	/* On gen6+ we refuse to init without kms enabled, but then the drm core
>  	 * goes right around and calls lastclose. Check for this and don't clean
> @@ -1946,6 +1948,21 @@ void i915_driver_lastclose(struct drm_device *dev)
>  	if (!dev_priv)
>  		return;
>  
> +	if (dev_priv->rotation_property) {
> +		list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +			to_intel_plane(crtc->base.primary)->rotation = BIT(DRM_ROTATE_0);
> +			drm_object_property_set_value(&crtc->base.base,
> +						dev_priv->rotation_property,
> +						to_intel_plane(crtc->base.primary)->rotation);
> +		}
> +		list_for_each_entry(plane, &dev->mode_config.plane_list, base.head) {
> +			plane->rotation = BIT(DRM_ROTATE_0);
> +			drm_object_property_set_value(&plane->base.base,
> +						dev_priv->rotation_property,
> +						plane->rotation);
> +		}
> +	}
> +

The purpose of this seems to be restoring rotation to 0 and rely on the next
modeset to re-program the register. This code, is orthogonal to the pure
primary plane rotation enabling, spans through both sprite and primary planes
and may actually be a bit tricky to get right.

-> This shouldn't be part of the same commit as the primary plane rotation.
Please span a new commit for it.

Now, we also need something like this when switching VT, and probably for
kernel panic and debug handling as well, so lastclose() is not enough (and we
can probably do better).

One idea would be for the core DRM code to know about this property, and make
sure we put the rotation back to 0 in restore_fbdev_mode(), we do something
similar for the for the sprite planes in there already. Another idea would be
to add a vfunc to execute driver specific code there in restore_fbdev_mode().

There is probably a better way to do it, I have to say I'm not super familiar
with this part of the driver.


>  	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_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c70c804..c600d3b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4087,6 +4087,7 @@ enum punit_power_well {
>  #define   DISPPLANE_NO_LINE_DOUBLE		0
>  #define   DISPPLANE_STEREO_POLARITY_FIRST	0
>  #define   DISPPLANE_STEREO_POLARITY_SECOND	(1<<18)
> +#define   DISPPLANE_ROTATE_180         (1<<15)
>  #define   DISPPLANE_TRICKLE_FEED_DISABLE	(1<<14) /* Ironlake */
>  #define   DISPPLANE_TILED			(1<<10)
>  #define _DSPAADDR				0x70184
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e8e711..1dc8b68 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2414,7 +2414,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg;
> +	int pixel_size;
>  
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> @@ -2422,6 +2424,8 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	dspcntr = I915_READ(reg);
>  	/* Mask out pixel format bits in case we change it */
>  	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	dspcntr &= ~DISPPLANE_ROTATE_180;
> +
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -2463,8 +2467,6 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	if (IS_G4X(dev))
>  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>  
> -	I915_WRITE(reg, dspcntr);
> -
>  	linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
>  
>  	if (INTEL_INFO(dev)->gen >= 4) {
> @@ -2477,6 +2479,17 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  		intel_crtc->dspaddr_offset = linear_offset;
>  	}
>  
> +	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +		dspcntr |= DISPPLANE_ROTATE_180;
> +
> +		x += (intel_crtc->config.pipe_src_w - 1);
> +		y += (intel_crtc->config.pipe_src_h - 1);
> +		linear_offset += (intel_crtc->config.pipe_src_h - 1) * fb->pitches[0] +
> +                                   (intel_crtc->config.pipe_src_w - 1) * pixel_size;
> +	}
> +
> +	I915_WRITE(reg, dspcntr);
> +
>  	DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
>  		      i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
>  		      fb->pitches[0]);
> @@ -2504,7 +2517,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg;
> +	int pixel_size;
>  
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  	intel_fb = to_intel_framebuffer(fb);
>  	obj = intel_fb->obj;
>  
> @@ -2512,6 +2527,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	dspcntr = I915_READ(reg);
>  	/* Mask out pixel format bits in case we change it */
>  	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	dspcntr &= ~DISPPLANE_ROTATE_180;
> +
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -2549,8 +2566,6 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	else
>  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>  
> -	I915_WRITE(reg, dspcntr);
> -
>  	linear_offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
>  	intel_crtc->dspaddr_offset =
>  		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> @@ -2558,6 +2573,19 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  					       fb->pitches[0]);
>  	linear_offset -= intel_crtc->dspaddr_offset;
>  
> +	if (to_intel_plane(crtc->primary)->rotation == BIT(DRM_ROTATE_180)) {
> +		dspcntr |= DISPPLANE_ROTATE_180;
> +
> +		if (!IS_HASWELL(dev) && !IS_BROADWELL(dev)) {
> +			x += (intel_crtc->config.pipe_src_w - 1);
> +			y += (intel_crtc->config.pipe_src_h - 1);
> +			linear_offset += (intel_crtc->config.pipe_src_h - 1) * fb->pitches[0] +
> +					(intel_crtc->config.pipe_src_w - 1) * pixel_size;
> +		}
> +	}
> +
> +	I915_WRITE(reg, dspcntr);
> +
>  	DRM_DEBUG_KMS("Writing base %08lX %08lX %d %d %d\n",
>  		      i915_gem_obj_ggtt_offset(obj), linear_offset, x, y,
>  		      fb->pitches[0]);
> @@ -11324,10 +11352,51 @@ static void intel_plane_destroy(struct drm_plane *plane)
>  	kfree(intel_plane);
>  }
>  
> +static int intel_primary_plane_set_property(struct drm_plane *plane,
> +				    struct drm_property *prop,
> +				    uint64_t val)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(plane->crtc);
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	uint64_t old_val;
> +	int ret = -ENOENT;
> +
> +	if (prop == dev_priv->rotation_property) {
> +		/* exactly one rotation angle please */
> +		if (hweight32(val & 0xf) != 1)
> +			return -EINVAL;
> +
> +		old_val = intel_plane->rotation;

This value is set but never used again?

> +		intel_plane->rotation = val;
> +
> +		if (intel_crtc->active && intel_crtc->primary_enabled) {
> +			intel_crtc_wait_for_pending_flips(crtc);
> +
> +			/* FBC does not work on some platforms for rotated planes */
> +			if (dev_priv->fbc.plane == intel_crtc->plane &&
> +			    INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +			    intel_plane->rotation != BIT(DRM_ROTATE_0))
> +				intel_disable_fbc(dev);
> +
> +			dev_priv->display.update_primary_plane(crtc, crtc->primary->fb, 0, 0);
> +		} else {
> +			DRM_DEBUG_KMS("[CRTC:%d] is not active. Only rotation property is updated\n",
> +					crtc->base.id);
> +			ret = 0;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct drm_plane_funcs intel_primary_plane_funcs = {
>  	.update_plane = intel_primary_plane_setplane,
>  	.disable_plane = intel_primary_plane_disable,
>  	.destroy = intel_plane_destroy,
> +	.set_property = intel_primary_plane_set_property
>  };
>  
>  static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> @@ -11335,6 +11404,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  {
>  	struct intel_plane *primary;
>  	const uint32_t *intel_primary_formats;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int num_formats;
>  
>  	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> @@ -11345,6 +11415,7 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  	primary->max_downscale = 1;
>  	primary->pipe = pipe;
>  	primary->plane = pipe;
> +	primary->rotation = BIT(DRM_ROTATE_0);
>  	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen < 4)
>  		primary->plane = !pipe;
>  
> @@ -11360,6 +11431,18 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  				 &intel_primary_plane_funcs,
>  				 intel_primary_formats, num_formats,
>  				 DRM_PLANE_TYPE_PRIMARY);
> +	if (INTEL_INFO(dev)->gen >= 4) {
> +		if (!dev_priv->rotation_property)
> +			dev_priv->rotation_property =
> +				drm_mode_create_rotation_property(dev,
> +								BIT(DRM_ROTATE_0) |
> +								BIT(DRM_ROTATE_180));
> +		if (dev_priv->rotation_property)
> +			drm_object_attach_property(&primary->base.base,
> +						dev_priv->rotation_property,
> +						primary->rotation);
> +	}
> +
>  	return &primary->base;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2043c4b..cabccfb 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -562,6 +562,14 @@ void intel_update_fbc(struct drm_device *dev)
>  		goto out_disable;
>  	}
>  
> +	if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) &&
> +	    to_intel_plane(crtc->primary)->rotation != BIT(DRM_ROTATE_0)) {
> +		if (set_no_fbc_reason(dev_priv, FBC_UNSUPPORTED_MODE))
> +			DRM_DEBUG_KMS("mode incompatible with compression, "
> +				      "disabling\n");

This debug message isn't helpful. You need to add that's because of the
rotation.

> +		goto out_disable;
> +	}
> +
>  	/* If the kernel debugger is active, always disable compression */
>  	if (in_dbg_master())
>  		goto out_disable;
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx