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
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
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... xf86-video-intel: http://people.freedesktop.org/~pzanoni/0001-Call-drmModeCrtcSetRotation-afte...
- 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
----- 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.
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_KV...
Cheers, Paulo
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.
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@)
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
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?
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.
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Dave Airlie -
Eric Anholt -
Jakob Bornecrantz -
Jesse Barnes -
Paulo Zanoni -
przanoni＠gmail.com