[RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[przanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This ioctl is used to signal the drivers that the screen is rotated,
not to make the drivers rotate the screen.
 - add a driver-specific "rotation_set" function
 - implement Intel's rotation_set by setting the right values to the
   PIPECONF registers.

The idea is that when user-space does rotation, it can call this ioctl
to inform the Kernel that we have a rotation. This feature is needed
by the KVMr feature of VPro.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---

Hi

We need this feature for the KVMr feature of VPro. I'm not sure how useful this
will be for the non-Intel drivers, so maybe the current approach is not the
best. I'm open to suggestions.

I also have a patch to libdrm that just implements a wrapper for the ioctl
(drmModeCrtcSetRotation) and a patch for xf86-video-intel that callss the libdrm
function whenever needed.

I already have the libdrm and xf86-video-intel patches (they're simple) but I'll
wait until I get some comments on this one before I send the others.

 drivers/gpu/drm/drm_crtc.c           |   28 ++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_drv.c            |    3 ++-
 drivers/gpu/drm/i915/i915_reg.h      |    5 +++++
 drivers/gpu/drm/i915/intel_display.c |   34 ++++++++++++++++++++++++++++++++++
 include/drm/drm.h                    |    2 ++
 include/drm/drm_crtc.h               |    8 +++++++-
 include/drm/drm_mode.h               |   15 +++++++++++++++
 7 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f259a25..4a33ea1 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3125,6 +3125,34 @@ out:
 	return ret;
 }
 
+int drm_crtc_rotation_set_ioctl(struct drm_device *dev, void *data,
+				struct drm_file *file_priv)
+{
+	struct drm_mode_crtc_rotation *rotation = data;
+	struct drm_mode_object *obj;
+	struct drm_crtc *crtc;
+	int ret = 0;
+
+	DRM_DEBUG_KMS("changing rotation to %d\n", rotation->rotation);
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	mutex_lock(&dev->mode_config.mutex);
+	obj = drm_mode_object_find(dev, rotation->crtc_id, DRM_MODE_OBJECT_CRTC);
+	if (!obj) {
+		ret = -EINVAL;
+		goto out;
+	}
+	crtc = obj_to_crtc(obj);
+
+	if (crtc->funcs->rotation_set)
+		crtc->funcs->rotation_set(crtc, rotation->rotation);
+out:
+	mutex_unlock(&dev->mode_config.mutex);
+	return ret;
+}
+
 int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index bc5febe..ba0dac1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -159,7 +159,8 @@ static struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
 	DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
-	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED)
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+	DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETROTATION, drm_crtc_rotation_set_ioctl, DRM_MASTER|DRM_UNLOCKED)
 };
 
 #define DRM_CORE_IOCTL_COUNT	ARRAY_SIZE( drm_ioctls )
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 194d987..3d9d46e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2323,6 +2323,11 @@
 #define   PIPECONF_INTERLACE_FIELD_0_ONLY		(7 << 21)
 #define   PIPECONF_INTERLACE_MASK	(7 << 21)
 #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 55a5b4c..08780f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6317,6 +6317,39 @@ static void intel_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
 	intel_crtc_load_lut(crtc);
 }
 
+static void intel_crtc_rotation_set(struct drm_crtc *crtc,
+				    enum drm_crtc_rotation 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 & ~(DRM_CRTC_ROTATION_INV_X |
+			     DRM_CRTC_ROTATION_INV_Y)) {
+	case DRM_CRTC_ROTATION_0:
+		val |= PIPECONF_ROTATION_0;
+		break;
+	case DRM_CRTC_ROTATION_90:
+		val |= PIPECONF_ROTATION_90;
+		break;
+	case DRM_CRTC_ROTATION_180:
+		val |= PIPECONF_ROTATION_180;
+		break;
+	case DRM_CRTC_ROTATION_270:
+		val |= PIPECONF_ROTATION_270;
+		break;
+	default:
+		DRM_ERROR("Invalid rotation: 0x%x\n", rotation);
+		val |= PIPECONF_ROTATION_0;
+	}
+
+	I915_WRITE(reg, val);
+}
+
 /**
  * Get a pipe with a simple mode set on it for doing load-based monitor
  * detection.
@@ -7383,6 +7416,7 @@ 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,
+	.rotation_set = intel_crtc_rotation_set,
 };
 
 static void intel_crtc_init(struct drm_device *dev, int pipe)
diff --git a/include/drm/drm.h b/include/drm/drm.h
index 49d94ed..f33b09a 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -719,6 +719,8 @@ struct drm_get_cap {
 #define DRM_IOCTL_MODE_SETPLANE	DRM_IOWR(0xB7, struct drm_mode_set_plane)
 #define DRM_IOCTL_MODE_ADDFB2		DRM_IOWR(0xB8, struct drm_mode_fb_cmd2)
 
+#define DRM_IOCTL_MODE_SETROTATION	DRM_IOWR(0xB9, struct drm_mode_crtc_rotation)
+
 /**
  * Device specific ioctls should only be in their respective headers
  * The device specific ioctl range is from 0x40 to 0x99.
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 2deb6f9..3515ee7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -297,7 +297,8 @@ struct drm_plane;
  * @mode_fixup: fixup proposed mode
  * @mode_set: set the desired mode on the CRTC
  * @gamma_set: specify color ramp for CRTC
- * @destroy: deinit and free object.
+ * @destroy: deinit and free object
+ * @rotation_set: specify the CRTC rotation
  *
  * The drm_crtc_funcs structure is the central CRTC management structure
  * in the DRM.  Each CRTC controls one or more connectors (note that the name
@@ -341,6 +342,9 @@ struct drm_crtc_funcs {
 	int (*page_flip)(struct drm_crtc *crtc,
 			 struct drm_framebuffer *fb,
 			 struct drm_pending_vblank_event *event);
+
+	void (*rotation_set)(struct drm_crtc *crtc,
+			     enum drm_crtc_rotation rotation);
 };
 
 /**
@@ -975,6 +979,8 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
 extern int drm_mode_gamma_set_ioctl(struct drm_device *dev,
 				    void *data, struct drm_file *file_priv);
+extern int drm_crtc_rotation_set_ioctl(struct drm_device *dev,
+				    void *data, struct drm_file *file_priv);
 extern u8 *drm_find_cea_extension(struct edid *edid);
 extern bool drm_detect_hdmi_monitor(struct edid *edid);
 extern bool drm_detect_monitor_audio(struct edid *edid);
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
index 2a2acda..58200e8 100644
--- a/include/drm/drm_mode.h
+++ b/include/drm/drm_mode.h
@@ -379,6 +379,21 @@ struct drm_mode_crtc_lut {
 	__u64 blue;
 };
 
+/* Matches randr.h for convenience */
+enum drm_crtc_rotation {
+	DRM_CRTC_ROTATION_0     = 1,
+	DRM_CRTC_ROTATION_90    = 2,
+	DRM_CRTC_ROTATION_180   = 4,
+	DRM_CRTC_ROTATION_270   = 8,
+	DRM_CRTC_ROTATION_INV_X = 16,
+	DRM_CRTC_ROTATION_INV_Y = 32
+};
+
+struct drm_mode_crtc_rotation {
+	__u32 crtc_id;
+	enum drm_crtc_rotation rotation;
+};
+
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_FLAGS DRM_MODE_PAGE_FLIP_EVENT
 
-- 
1.7.7.3

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Daniel Vetter]

On Thu, Jan 05, 2012 at 02:16:23PM -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This ioctl is used to signal the drivers that the screen is rotated,
> not to make the drivers rotate the screen.
>  - add a driver-specific "rotation_set" function
>  - implement Intel's rotation_set by setting the right values to the
>    PIPECONF registers.
> 
> The idea is that when user-space does rotation, it can call this ioctl
> to inform the Kernel that we have a rotation. This feature is needed
> by the KVMr feature of VPro.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Quick comments before I dig into this:
- Please post at least a link to the userspace patches, so that it's
  possible to check how this all fits together
- Please separate the generic infrastructre in drm from the changes to
  support this in i915. I know, this is a rather small feature, but still.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Paulo Zanoni]

Hi

2012/1/5 Daniel Vetter <daniel@ffwll.ch>:
> - Please post at least a link to the userspace patches, so that it's
>  possible to check how this all fits together

libdrm: http://people.freedesktop.org/~pzanoni/0001-Implement-drmModeCrtcSetRotation.patch
xf86-video-intel:
http://people.freedesktop.org/~pzanoni/0001-Call-drmModeCrtcSetRotation-after-we-apply-the-crtc-.patch

> - Please separate the generic infrastructre in drm from the changes to
>  support this in i915. I know, this is a rather small feature, but still.

Will do, but I'll wait for more reviews/comments first.

Cheers,
Paulo

-- 
Paulo Zanoni

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Jakob Bornecrantz]

----- Original Message -----
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This ioctl is used to signal the drivers that the screen is rotated,
> not to make the drivers rotate the screen.
>  - add a driver-specific "rotation_set" function
>  - implement Intel's rotation_set by setting the right values to the
>    PIPECONF registers.
> 
> The idea is that when user-space does rotation, it can call this
> ioctl to inform the Kernel that we have a rotation. This feature is
> needed by the KVMr feature of VPro.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Couldn't this be done by just adding a property instead of a ioctl?

Also the name make it sound like you are setting the rotation and not
just setting a hint, so the word hit should probably be in the name.


Cheers Jakob.

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Chris Wilson]

On Thu,  5 Jan 2012 14:16:23 -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This ioctl is used to signal the drivers that the screen is rotated,
> not to make the drivers rotate the screen.
>  - add a driver-specific "rotation_set" function
>  - implement Intel's rotation_set by setting the right values to the
>    PIPECONF registers.
> 
> The idea is that when user-space does rotation, it can call this ioctl
> to inform the Kernel that we have a rotation. This feature is needed
> by the KVMr feature of VPro.

I'm interested to see how the fb is laid out in memory and how does this
integrate with mode configuration?

Comment inline.

> +int drm_crtc_rotation_set_ioctl(struct drm_device *dev, void *data,
> +				struct drm_file *file_priv)
> +{
> +	struct drm_mode_crtc_rotation *rotation = data;
> +	struct drm_mode_object *obj;
> +	struct drm_crtc *crtc;
> +	int ret = 0;
> +
> +	DRM_DEBUG_KMS("changing rotation to %d\n", rotation->rotation);
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	obj = drm_mode_object_find(dev, rotation->crtc_id, DRM_MODE_OBJECT_CRTC);
> +	if (!obj) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	crtc = obj_to_crtc(obj);
> +
> +	if (crtc->funcs->rotation_set)
> +		crtc->funcs->rotation_set(crtc, rotation->rotation);

In the absence of an implementation, return ENOTTY, otherwise return an
error code from crtc->rotation_set(). Then userspace can simply try to
set a rotation or take the same recovery paths for unsupported
configuations/kernels.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 743 bytes --]

On Thu,  5 Jan 2012 14:16:23 -0200, przanoni@gmail.com wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This ioctl is used to signal the drivers that the screen is rotated,
> not to make the drivers rotate the screen.
>  - add a driver-specific "rotation_set" function
>  - implement Intel's rotation_set by setting the right values to the
>    PIPECONF registers.
> 
> The idea is that when user-space does rotation, it can call this ioctl
> to inform the Kernel that we have a rotation. This feature is needed
> by the KVMr feature of VPro.

So am I following this right, that these register bits are used to
communicate from one piece of software to another piece of software,
across the virtualization boundary?

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Jesse Barnes]

On Thu, 05 Jan 2012 19:10:43 -0800
Eric Anholt <eric@anholt.net> wrote:

> On Thu,  5 Jan 2012 14:16:23 -0200, przanoni@gmail.com wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This ioctl is used to signal the drivers that the screen is rotated,
> > not to make the drivers rotate the screen.
> >  - add a driver-specific "rotation_set" function
> >  - implement Intel's rotation_set by setting the right values to the
> >    PIPECONF registers.
> > 
> > The idea is that when user-space does rotation, it can call this ioctl
> > to inform the Kernel that we have a rotation. This feature is needed
> > by the KVMr feature of VPro.
> 
> So am I following this right, that these register bits are used to
> communicate from one piece of software to another piece of software,
> across the virtualization boundary?

Right, but not for virtualization; these bits are read by the AMT
engine for its built-in KVM functionality.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Paulo Zanoni]

Hi

2012/1/5 Jakob Bornecrantz <jakob@vmware.com>:
> Couldn't this be done by just adding a property instead of a ioctl?
>

So I've discussed this with Jesse and it seems the best way to turn
this into a property is to add support for CRTC properties, then add a
"rotation" property for the i915 driver. This will actually introduce
2 new ioctls: one to list the properties and one to change them.

I'll send three Kernel patches to this list in the next minutes. One
just moves code around (so we can reuse it), another adds CRTC
properties in drm/ and the other adds the rotation property for i915.

I also wrote patches for libdrm (implement the ioctls, change
tests/modetest to call these ioctls, and some janitor work on modetest
and libdrm) and xf86-video-intel (use the rotation property). The drm
patches are applied to this tree:
http://cgit.freedesktop.org/~pzanoni/drm/ . I was only planning to
send them to the list after I get some review on the kernel patches.
The full list of patches I wrote so far is here:
http://people.freedesktop.org/~pzanoni/rotation/ .

For those asking more information about the problem: these bits are
somehow needed by the embedded VNC: it needs to know how the screen is
rotated. More information:
http://en.wikipedia.org/wiki/Intel_Active_Management_Technology#VNC-based_KVM_remote_control

Cheers,
Paulo

-- 
Paulo Zanoni

Re: [Intel-gfx] [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Dave Airlie]

On Mon, Jan 16, 2012 at 7:59 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2012/1/5 Jakob Bornecrantz <jakob@vmware.com>:
>> Couldn't this be done by just adding a property instead of a ioctl?
>>
>
> So I've discussed this with Jesse and it seems the best way to turn
> this into a property is to add support for CRTC properties, then add a
> "rotation" property for the i915 driver. This will actually introduce
> 2 new ioctls: one to list the properties and one to change them.

Okay I must have missed the bit where you explain why a connector
property isn't used?

Dave.

Re: [Intel-gfx] [RFC] drm: implement DRM_IOCTL_MODE_SETROTATION

[Paulo Zanoni]

2012/1/16 Dave Airlie <airlied@gmail.com>:
>
> Okay I must have missed the bit where you explain why a connector
> property isn't used?

The registers that contain the rotation information are the pipe
registers and, as far as I understood, each pipe is associated with
only one crtc. We can have more than one connector associated with one
crtc. So, in a case where we have two different connectors associated
with the same crtc, we would have to keep synchronizing the property
value between the two connectors: this doesn't sound like the best
solution and it could also create confusion (imagine the case where
connectors A and B are associated with the same crtc, and you set
connector_a.rotation to 0 and then connector_b.rotation to 180, then
you'll read connector_a.rotation and it will be 180, not what you just
set).

(and by the way, the three patches were sent just to dri-devel@)


-- 
Paulo Zanoni