Re: [PATCH 4/5] drm/i915: add 'rotation' CRTC property

[Marcus Lorentzon <marcus.xm.lorentzon@stericsson.com>]

Hi Paulo,
thanks for a quick response posting the patches.

In my use of CRTC rotation properties, I need the change to be 
synchronized with modeset. Your implementation activate the property 
change immediately instead of staging it for next modeset. Do you think 
it is ok for different drivers to behave differently when properties are 
set? I can't allow glitches in the output when changing the rotation in 
one frame and flipping the framebuffer to one of correct size and 
content in another. This would mean I will have to stage the setting and 
apply it at next modeset (different behavior). Or do I just have to wait 
until we have an atomic modeset that can also set properties (Jesse can 
you post your latest proposal ...)?

Do you think you could export the call registering the rotation 
property? That way other drivers can reuse it and we don't risk multiple 
properties with different ranges etc for the same logical property (like 
400 mil degrees or 0-3, 1-4, UR,UD,CCW,CW ...). So some sort of standard 
property create.

Then it would also be nice to define some rules in KMS of what should be 
modeset params and what should be properties. Modeset params seems hard 
to add (struct change) and properties seems very loosely defined and 
device specific. Having something in between would be nice (standard and 
easy to add), to make it easier to add things like plane z-order 
(instead of driver specific ioctls or properties).
Maybe a new "atomic" modeset with just a list of {object_id, 
property_id, value} making all params into properties and modeset more 
dynamic. It would also solve the atomic modset, even with device 
specific properties and future features.

Sorry for multiple topics, but they are sort of related.

/BR
/Marcus

On 03/20/2012 03:48 PM, Paulo Zanoni wrote:
> From: Paulo Zanoni<paulo.r.zanoni@intel.com>
>
> This property is needed so we can inform the KVMr feature about our
> current rotation: whenever we change the rotation, we should change
> that property so that the KVMr knows that the screen is rotated.
>
> How to reproduce the problem:
> - on an AMT machine, enable KVM
> - boot the machine, use xrandr to rotate the display
> - use VNC to connect to the KVM
> - try to use the mouse
>
> v2: only create the property once
>
> Signed-off-by: Paulo Zanoni<paulo.r.zanoni@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |    1 +
>   drivers/gpu/drm/i915/i915_reg.h      |    5 +++
>   drivers/gpu/drm/i915/intel_display.c |   66 ++++++++++++++++++++++++++++++++++
>   3 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e7a00b7..7994c4f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -780,6 +780,7 @@ typedef struct drm_i915_private {
>
>   	struct drm_property *broadcast_rgb_property;
>   	struct drm_property *force_audio_property;
> +	struct drm_property *rotation_property;
>   } drm_i915_private_t;
>
>   enum hdmi_force_audio {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 52a06be..79e8b12 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2407,6 +2407,11 @@
>   #define   PIPECONF_INTERLACED_DBL_ILK		(4<<  21) /* ilk/snb only */
>   #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5<<  21) /* ilk/snb only */
>   #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
> +#define   PIPECONF_ROTATION_MASK	(3<<  14)
> +#define   PIPECONF_ROTATION_0	(0<<  14)
> +#define   PIPECONF_ROTATION_90	(1<<  14)
> +#define   PIPECONF_ROTATION_180	(2<<  14)
> +#define   PIPECONF_ROTATION_270	(3<<  14)
>   #define   PIPECONF_BPP_MASK	(0x000000e0)
>   #define   PIPECONF_BPP_8	(0<<5)
>   #define   PIPECONF_BPP_10	(1<<5)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 683002fb..4842de8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7625,6 +7625,50 @@ static void intel_crtc_reset(struct drm_crtc *crtc)
>   	intel_sanitize_modesetting(dev, intel_crtc->pipe, intel_crtc->plane);
>   }
>
> +static void intel_crtc_set_rotation(struct drm_crtc *crtc,
> +				    uint64_t rotation)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int reg = PIPECONF(intel_crtc->pipe);
> +	u32 val = I915_READ(reg);
> +
> +	val&= ~PIPECONF_ROTATION_MASK;
> +
> +	switch (rotation) {
> +	case 0:
> +		val |= PIPECONF_ROTATION_0;
> +		break;
> +	case 90:
> +		val |= PIPECONF_ROTATION_90;
> +		break;
> +	case 180:
> +		val |= PIPECONF_ROTATION_180;
> +		break;
> +	case 270:
> +		val |= PIPECONF_ROTATION_270;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported rotation: %Lu\n", rotation);
> +		val |= PIPECONF_ROTATION_0;
> +	}
> +
> +	I915_WRITE(reg, val);
> +}
> +
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> +				   struct drm_property *property,
> +				   uint64_t val)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (property == dev_priv->rotation_property)
> +		intel_crtc_set_rotation(crtc, val);
> +	return 0;
> +}
> +
>   static struct drm_crtc_helper_funcs intel_helper_funcs = {
>   	.dpms = intel_crtc_dpms,
>   	.mode_fixup = intel_crtc_mode_fixup,
> @@ -7643,8 +7687,27 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>   	.set_config = drm_crtc_helper_set_config,
>   	.destroy = intel_crtc_destroy,
>   	.page_flip = intel_crtc_page_flip,
> +	.set_property = intel_crtc_set_property,
>   };
>
> +static void intel_attach_rotation_property(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_property *prop;
> +
> +	prop = dev_priv->rotation_property;
> +	if (prop == NULL) {
> +		prop = drm_property_create_range(dev, 0, "rotation", 0, 359);
> +		if (prop == NULL)
> +			return;
> +
> +		dev_priv->rotation_property = prop;
> +	}
> +
> +	drm_crtc_attach_property(crtc, prop, 0);
> +}
> +
>   static void intel_crtc_init(struct drm_device *dev, int pipe)
>   {
>   	drm_i915_private_t *dev_priv = dev->dev_private;
> @@ -7664,6 +7727,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>   		intel_crtc->lut_b[i] = i;
>   	}
>
> +	if (INTEL_INFO(dev)->gen>= 5)
> +		intel_attach_rotation_property(&intel_crtc->base);
> +
>   	/* Swap pipes&  planes for FBC on pre-965 */
>   	intel_crtc->pipe = pipe;
>   	intel_crtc->plane = pipe;