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

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

On Tue, Aug 19, 2014 at 11:56:43AM +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. Removing reset
> of rotation property from lastclose. rotation_property is moved to
> drm_mode_config, so drm layer will take care of resetting. Adding updation of
> fbc when rotation is set to 0. Allowing rotation only if value is
> different than old one.
> 
> v10: Calling intel_primary_plane_setplane instead of update_primary_plane in
> set_property(Daniel).
> 
> Testcase: igt/kms_rotation_crc
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Sagar Kamble <sagar.a.kamble@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 |  116 ++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c      |    6 ++
>  3 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7a6cc69..f8aaf0b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4209,6 +4209,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 1b0e403..cc2999b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2332,6 +2332,9 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg = DSPCNTR(plane);
> +	int pixel_size;
> +
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(reg, 0);
> @@ -2359,6 +2362,7 @@ static void i9xx_update_primary_plane(struct drm_crtc *crtc,
>  			   (intel_crtc->config.pipe_src_w - 1));
>  		I915_WRITE(DSPPOS(plane), 0);
>  	}
> +	dspcntr &= ~DISPPLANE_ROTATE_180;
>  
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
> @@ -2398,8 +2402,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) {
> @@ -2412,6 +2414,21 @@ 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);
> +
> +		/* Finding the last pixel of the last line of the display
> +		data and adding to linear_offset*/
> +		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]);
> @@ -2438,6 +2455,9 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	unsigned long linear_offset;
>  	u32 dspcntr;
>  	u32 reg = DSPCNTR(plane);
> +	int pixel_size;
> +
> +	pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>  
>  	if (!intel_crtc->primary_enabled) {
>  		I915_WRITE(reg, 0);
> @@ -2453,6 +2473,8 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
>  
> +	dspcntr &= ~DISPPLANE_ROTATE_180;
> +
>  	switch (fb->pixel_format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -2486,14 +2508,26 @@ static void ironlake_update_primary_plane(struct drm_crtc *crtc,
>  	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
>  		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,
>  					       fb->bits_per_pixel / 8,
>  					       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,
> @@ -11647,10 +11681,70 @@ 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 drm_crtc *crtc = plane->crtc;
> +	struct intel_crtc *intel_crtc;
> +	uint64_t old_val;
> +
> +	if (prop == dev->mode_config.rotation_property) {
> +		/* exactly one rotation angle please */
> +		if (hweight32(val & 0xf) != 1)
> +			return -EINVAL;
> +
> +		old_val = intel_plane->rotation;
> +		intel_plane->rotation = val;
> +
> +		if (old_val == intel_plane->rotation)
> +			return 0;
> +
> +		if (crtc == NULL)
> +			return -EINVAL;

For sprite planes it looks like we allow this property to be updated
while the plane is off (no FB assigned and thus no crtc pointer) without
error.  Is it intentional that the primary plane behaves differently
here?  Note that you're returning -EINVAL here, but you still leave the
intel_plane->rotation value you set above, which seems a bit odd.

> +
> +		intel_crtc = to_intel_crtc(plane->crtc);
> +
> +		if (intel_crtc && intel_crtc->active &&
> +				intel_crtc->primary_enabled) {
> +			intel_crtc_wait_for_pending_flips(crtc);
> +
> +		    /* FBC does not work on some platforms for rotated
> +			planes, so disable it when rotation is not 0 and update
> +			it when rotation is set back to 0 */
> +			if (INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev)) {
> +				if (dev_priv->fbc.plane == intel_crtc->plane &&
> +				intel_plane->rotation != BIT(DRM_ROTATE_0))
> +					intel_disable_fbc(dev);
> +				else if (intel_plane->rotation == BIT(DRM_ROTATE_0)) {
> +					mutex_lock(&dev->struct_mutex);
> +					intel_update_fbc(dev);
> +					mutex_unlock(&dev->struct_mutex);
> +				}
> +			}
> +
> +			intel_primary_plane_setplane(plane, crtc, crtc->primary->fb,
> +				intel_plane->crtc_x, intel_plane->crtc_y,
> +				intel_plane->crtc_w, intel_plane->crtc_h,
> +				intel_plane->src_x, intel_plane->src_y,
> +				intel_plane->src_w, intel_plane->src_h);
> +
> +		} else {
> +			DRM_DEBUG_KMS("[CRTC:%d] inactive. Only rotation"
> +				"property is updated\n", crtc->base.id);
> +		}
> +	}
> +	return 0;
> +}

I haven't followed this thread very carefully, so sorry if this has
already been discussed, but could you just use the same set_property()
handler for both primary and sprite planes to avoid duplicate effort?
It seems like you'd only need to make a few changes:

 * Move the wait for asynchronous flips and FBC stuff here into an "if
   (plane type is primary)" block
 * Call intel_plane_restore() here rather than
   intel_primary_plane_setplane()
 * Update intel_plane_restore() such that it calls
   drm_plane->funcs->update_plane() rather than intel_update_plane()
   directly.

I'm sure in the future we'll add other properties that we'll want to
handle on both primary and sprite (and cursor) planes, so having a
single set_property handler might allow us to cut down on needing to
duplicate future code in multiple places.

On the cosmetic side, I also kind of feel like it might make sense to
rename intel_plane_restore() to intel_plane_reprogram() since that seems
like a more accurate description of what we're using the function for
now.


Matt

> +
>  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,
> @@ -11668,6 +11762,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;
>  
> @@ -11683,6 +11778,19 @@ 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->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(&primary->base.base,
> +				dev->mode_config.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 12f4e14..12f5b99 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -581,6 +581,12 @@ void intel_update_fbc(struct drm_device *dev)
>  			DRM_DEBUG_KMS("framebuffer not tiled or fenced, disabling compression\n");
>  		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("Rotation unsupported, disabling\n");
> +		goto out_disable;
> +	}
>  
>  	/* If the kernel debugger is active, always disable compression */
>  	if (in_dbg_master())
> -- 
> 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