Recently some people from inside Intel have showed some interest in 180 degree plane rotation. To avoid a huge mess, I decided that I should implement the feature properly.
So I snatched the rotation property from omapdrm, and moved some of the code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect and implemented the relevant bits in i915. I didn't touch cursor or primary planes at all. I'm sort of hoping we might do the drm_plane conversion sometime soonish and then we'd avoid adding a bunch of plane properties to the crtc.
One thing I don't really like is the current way of stuffing the bit number into the enum_list resulting in DRM_ROTATE_FOO being just the bit number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm not sure if changing that would cause grief to userspace, so I left it alone for now.
From: Ville Syrjälä ville.syrjala@linux.intel.com
The rotation property stuff should be standardized among all drivers. Move the bits to drm_crtc.h from omap_drv.h.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/omapdrm/omap_drv.h | 7 ------- include/drm/drm_crtc.h | 8 ++++++++ 2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 30b95b7..dfe0beb 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -119,13 +119,6 @@ struct omap_drm_private { struct omap_drm_irq error_handler; };
-/* this should probably be in drm-core to standardize amongst drivers */ -#define DRM_ROTATE_0 0 -#define DRM_ROTATE_90 1 -#define DRM_ROTATE_180 2 -#define DRM_ROTATE_270 3 -#define DRM_REFLECT_X 4 -#define DRM_REFLECT_Y 5
#ifdef CONFIG_DEBUG_FS int omap_debugfs_init(struct drm_minor *minor); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 50cedad..7aec9f4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -64,6 +64,14 @@ struct drm_object_properties { uint64_t values[DRM_OBJECT_MAX_PROPERTY]; };
+/* rotation property bits */ +#define DRM_ROTATE_0 0 +#define DRM_ROTATE_90 1 +#define DRM_ROTATE_180 2 +#define DRM_ROTATE_270 3 +#define DRM_REFLECT_X 4 +#define DRM_REFLECT_Y 5 + /* * Note on terminology: here, for brevity and convenience, we refer to connector * control chips as 'CRTCs'. They can control any type of connector, VGA, LVDS,
From: Ville Syrjälä ville.syrjala@linux.intel.com
Make drm_property_create_bitmask() a bit more generic by allowing the caller to specify which bits are in fact supported. This allows multiple callers to use the same enum list, but still create different versions of the same property with different list of supported bits.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 6 +++++- include/drm/drm_crtc.h | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d7a8370..2087fe2 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2883,7 +2883,8 @@ EXPORT_SYMBOL(drm_property_create_enum); struct drm_property *drm_property_create_bitmask(struct drm_device *dev, int flags, const char *name, const struct drm_prop_enum_list *props, - int num_values) + int num_values, + unsigned int supported_bits) { struct drm_property *property; int i, ret; @@ -2895,6 +2896,9 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev, return NULL;
for (i = 0; i < num_values; i++) { + if (!(supported_bits & (1 << i))) + continue; + ret = drm_property_add_enum(property, i, props[i].type, props[i].name); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 7aec9f4..196b795 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1041,7 +1041,8 @@ extern struct drm_property *drm_property_create_enum(struct drm_device *dev, int struct drm_property *drm_property_create_bitmask(struct drm_device *dev, int flags, const char *name, const struct drm_prop_enum_list *props, - int num_values); + int num_values, + unsigned int supported_bits); struct drm_property *drm_property_create_range(struct drm_device *dev, int flags, const char *name, uint64_t min, uint64_t max);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a function to create a standards compliant rotation property.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2087fe2..0218681 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4088,3 +4088,21 @@ void drm_mode_config_cleanup(struct drm_device *dev) idr_destroy(&dev->mode_config.crtc_idr); } EXPORT_SYMBOL(drm_mode_config_cleanup); + +struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, + unsigned int supported_rotations) +{ + static const struct drm_prop_enum_list props[] = { + { DRM_ROTATE_0, "rotate-0" }, + { DRM_ROTATE_90, "rotate-90" }, + { DRM_ROTATE_180, "rotate-180" }, + { DRM_ROTATE_270, "rotate-270" }, + { DRM_REFLECT_X, "reflect-x" }, + { DRM_REFLECT_Y, "reflect-y" }, + }; + + return drm_property_create_bitmask(dev, 0, "rotation", + props, ARRAY_SIZE(props), + supported_rotations); +} +EXPORT_SYMBOL(drm_mode_create_rotation_property); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 196b795..d25ba84 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1152,5 +1152,7 @@ extern int drm_format_plane_cpp(uint32_t format, int plane); extern int drm_format_horz_chroma_subsampling(uint32_t format); extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); +extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, + unsigned int supported_rotations);
#endif /* __DRM_CRTC_H__ */
From: Ville Syrjälä ville.syrjala@linux.intel.com
Use the new drm_mode_create_rotation_property() in omapdrm.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/omapdrm/omap_plane.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 046d5e6..fee8f35 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -300,16 +300,13 @@ void omap_plane_install_properties(struct drm_plane *plane, if (priv->has_dmm) { prop = priv->rotation_prop; if (!prop) { - const struct drm_prop_enum_list props[] = { - { DRM_ROTATE_0, "rotate-0" }, - { DRM_ROTATE_90, "rotate-90" }, - { DRM_ROTATE_180, "rotate-180" }, - { DRM_ROTATE_270, "rotate-270" }, - { DRM_REFLECT_X, "reflect-x" }, - { DRM_REFLECT_Y, "reflect-y" }, - }; - prop = drm_property_create_bitmask(dev, 0, "rotation", - props, ARRAY_SIZE(props)); + prop = drm_mode_create_rotation_property(dev, + BIT(DRM_ROTATE_0) | + BIT(DRM_ROTATE_90) | + BIT(DRM_ROTATE_180) | + BIT(DRM_ROTATE_270) | + BIT(DRM_REFLECT_X) | + BIT(DRM_REFLECT_Y)); if (prop == NULL) return; priv->rotation_prop = prop;
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add some helper functions to move drm_rects between different rotated coordinate spaces. One function does the forward transform and another does the inverse.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_rect.c | 140 +++++++++++++++++++++++++++++++++++++++++++++ include/drm/drm_rect.h | 6 ++ 2 files changed, 146 insertions(+)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c index 7047ca0..631f5af 100644 --- a/drivers/gpu/drm/drm_rect.c +++ b/drivers/gpu/drm/drm_rect.c @@ -293,3 +293,143 @@ void drm_rect_debug_print(const struct drm_rect *r, bool fixed_point) DRM_DEBUG_KMS("%dx%d%+d%+d\n", w, h, r->x1, r->y1); } EXPORT_SYMBOL(drm_rect_debug_print); + +/** + * drm_rect_rotate - Rotate the rectangle + * @r: rectangle to be rotated + * @width: Width of the coordinate space + * @height: Height of the coordinate space + * @rotation: Transformation to be applied + * + * Apply @rotation to the coordinates of rectangle @r. + * + * @width and @height combined with @rotation define + * the location of the new origin. + * + * @width correcsponds to the horizontal and @height + * to the vertical axis of the untransformed coordinate + * space. + */ +void drm_rect_rotate(struct drm_rect *r, + int width, int height, + unsigned int rotation) +{ + struct drm_rect tmp; + + if (rotation & (BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y))) { + tmp = *r; + + if (rotation & BIT(DRM_REFLECT_X)) { + r->x1 = width - tmp.x2; + r->x2 = width - tmp.x1; + } + + if (rotation & BIT(DRM_REFLECT_Y)) { + r->y1 = height - tmp.y2; + r->y2 = height - tmp.y1; + } + } + + switch (rotation & 0xf) { + case BIT(DRM_ROTATE_0): + break; + case BIT(DRM_ROTATE_90): + tmp = *r; + r->x1 = tmp.y1; + r->x2 = tmp.y2; + r->y1 = width - tmp.x2; + r->y2 = width - tmp.x1; + break; + case BIT(DRM_ROTATE_180): + tmp = *r; + r->x1 = width - tmp.x2; + r->x2 = width - tmp.x1; + r->y1 = height - tmp.y2; + r->y2 = height - tmp.y1; + break; + case BIT(DRM_ROTATE_270): + tmp = *r; + r->x1 = height - tmp.y2; + r->x2 = height - tmp.y1; + r->y1 = tmp.x1; + r->y2 = tmp.x2; + break; + default: + break; + } +} +EXPORT_SYMBOL(drm_rect_rotate); + +/** + * drm_rect_rotate_inv - Inverse rotate the rectangle + * @r: rectangle to be rotated + * @width: Width of the coordinate space + * @height: Height of the coordinate space + * @rotation: Transformation whose inverse is to be applied + * + * Apply the inverse of @rotation to the coordinates + * of rectangle @r. + * + * @width and @height combined with @rotation define + * the location of the new origin. + * + * @width correcsponds to the horizontal and @height + * to the vertical axis of the original untransformed + * coordinate space, so that you never have to flip + * them when doing a rotatation and its inverse. + * That is, if you do: + * + * drm_rotate(&r, width, height, rotation); + * drm_rotate_inv(&r, width, height, rotation); + * + * you will always get back the original rectangle. + */ +void drm_rect_rotate_inv(struct drm_rect *r, + int width, int height, + unsigned int rotation) +{ + struct drm_rect tmp; + + switch (rotation & 0xf) { + case BIT(DRM_ROTATE_0): + break; + case BIT(DRM_ROTATE_90): + tmp = *r; + r->x1 = width - tmp.y2; + r->x2 = width - tmp.y1; + r->y1 = tmp.x1; + r->y2 = tmp.x2; + break; + case BIT(DRM_ROTATE_180): + tmp = *r; + r->x1 = width - tmp.x2; + r->x2 = width - tmp.x1; + r->y1 = height - tmp.y2; + r->y2 = height - tmp.y1; + break; + case BIT(DRM_ROTATE_270): + tmp = *r; + r->x1 = tmp.y1; + r->x2 = tmp.y2; + r->y1 = height - tmp.x2; + r->y2 = height - tmp.x1; + break; + default: + break; + } + + if (rotation & (BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y))) { + tmp = *r; + + if (rotation & BIT(DRM_REFLECT_X)) { + r->x1 = width - tmp.x2; + r->x2 = width - tmp.x1; + } + + if (rotation & BIT(DRM_REFLECT_Y)) { + r->y1 = height - tmp.y2; + r->y2 = height - tmp.y1; + } + } +} +EXPORT_SYMBOL(drm_rect_rotate_inv); diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h index d128629..26bb55e 100644 --- a/include/drm/drm_rect.h +++ b/include/drm/drm_rect.h @@ -163,5 +163,11 @@ int drm_rect_calc_vscale_relaxed(struct drm_rect *src, struct drm_rect *dst, int min_vscale, int max_vscale); void drm_rect_debug_print(const struct drm_rect *r, bool fixed_point); +void drm_rect_rotate(struct drm_rect *r, + int width, int height, + unsigned int rotation); +void drm_rect_rotate_inv(struct drm_rect *r, + int width, int height, + unsigned int rotation);
#endif
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_rotation_simplify() can be used to eliminate unsupported rotation flags. It will check if any unsupported flags are present, and if so it will modify the rotation to an alternate form by adding 180 degrees to rotation angle, and flipping the reflect x and y bits. The hope is that this identity transform will eliminate the unsupported flags.
Of course that might not result in any more supported rotation, so the caller is still responsible for checking the result afterwards.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 30 ++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 0218681..665b807 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3976,6 +3976,36 @@ int drm_format_vert_chroma_subsampling(uint32_t format) EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
/** + * drm_rotation_simplify() - Try to simplify the rotation + * @rotation: Rotation to be simplified + * @supported_rotations: Supported rotations + * + * Attempt to simplify the rotation to a form that is supported. + * Eg. if the hardware supports everything except DRM_REFLECT_X + * one could call this function like this: + * + * drm_rotation_simplify(rotation, BIT(DRM_ROTATE_0) | + * BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_180) | + * BIT(DRM_ROTATE_270) | BIT(DRM_REFLECT_Y)); + * + * to eliminate the DRM_ROTATE_X flag. Depending on what kind of + * transforms the hardware supports, this function may not + * be able to produce a supported transform, so the caller should + * check the result afterwards. + */ +unsigned int drm_rotation_simplify(unsigned int rotation, + unsigned int supported_rotations) +{ + if (rotation & ~supported_rotations) { + rotation ^= BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y); + rotation = (rotation & ~0xf) | BIT((ffs(rotation & 0xf) + 1) % 4); + } + + return rotation; +} +EXPORT_SYMBOL(drm_rotation_simplify); + +/** * drm_mode_config_init - initialize DRM mode_configuration structure * @dev: DRM device * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d25ba84..b97b367 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1154,5 +1154,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); +extern unsigned int drm_rotation_simplify(unsigned int rotation, + unsigned int supported_rotations);
#endif /* __DRM_CRTC_H__ */
On Mon, 2013-09-30 at 17:44 +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_rotation_simplify() can be used to eliminate unsupported rotation flags. It will check if any unsupported flags are present, and if so it will modify the rotation to an alternate form by adding 180 degrees to rotation angle, and flipping the reflect x and y bits. The hope is that this identity transform will eliminate the unsupported flags.
Of course that might not result in any more supported rotation, so the caller is still responsible for checking the result afterwards.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_crtc.c | 30 ++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 0218681..665b807 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3976,6 +3976,36 @@ int drm_format_vert_chroma_subsampling(uint32_t format) EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
/**
- drm_rotation_simplify() - Try to simplify the rotation
- @rotation: Rotation to be simplified
- @supported_rotations: Supported rotations
- Attempt to simplify the rotation to a form that is supported.
- Eg. if the hardware supports everything except DRM_REFLECT_X
- one could call this function like this:
- drm_rotation_simplify(rotation, BIT(DRM_ROTATE_0) |
BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_180) |
BIT(DRM_ROTATE_270) | BIT(DRM_REFLECT_Y));
- to eliminate the DRM_ROTATE_X flag. Depending on what kind of
^ DRM_REFLECT_X
You don't use this function in your patchset, though intel_plane_set_property() could use it by accepting REFLECT_X|REFLECT_Y too. But it does what it says, so I'm also ok if you leave it unused for now.
--Imre
- transforms the hardware supports, this function may not
- be able to produce a supported transform, so the caller should
- check the result afterwards.
- */
+unsigned int drm_rotation_simplify(unsigned int rotation,
unsigned int supported_rotations)+{
- if (rotation & ~supported_rotations) {
rotation ^= BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y);rotation = (rotation & ~0xf) | BIT((ffs(rotation & 0xf) + 1) % 4);- }
- return rotation;
+} +EXPORT_SYMBOL(drm_rotation_simplify);
+/**
- drm_mode_config_init - initialize DRM mode_configuration structure
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d25ba84..b97b367 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1154,5 +1154,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); +extern unsigned int drm_rotation_simplify(unsigned int rotation,
unsigned int supported_rotations);#endif /* __DRM_CRTC_H__ */
On Mon, Oct 14, 2013 at 04:46:50PM +0300, Imre Deak wrote:
On Mon, 2013-09-30 at 17:44 +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_rotation_simplify() can be used to eliminate unsupported rotation flags. It will check if any unsupported flags are present, and if so it will modify the rotation to an alternate form by adding 180 degrees to rotation angle, and flipping the reflect x and y bits. The hope is that this identity transform will eliminate the unsupported flags.
Of course that might not result in any more supported rotation, so the caller is still responsible for checking the result afterwards.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_crtc.c | 30 ++++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 0218681..665b807 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3976,6 +3976,36 @@ int drm_format_vert_chroma_subsampling(uint32_t format) EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
/**
- drm_rotation_simplify() - Try to simplify the rotation
- @rotation: Rotation to be simplified
- @supported_rotations: Supported rotations
- Attempt to simplify the rotation to a form that is supported.
- Eg. if the hardware supports everything except DRM_REFLECT_X
- one could call this function like this:
- drm_rotation_simplify(rotation, BIT(DRM_ROTATE_0) |
BIT(DRM_ROTATE_90) | BIT(DRM_ROTATE_180) |
BIT(DRM_ROTATE_270) | BIT(DRM_REFLECT_Y));
- to eliminate the DRM_ROTATE_X flag. Depending on what kind of
^ DRM_REFLECT_XYou don't use this function in your patchset, though intel_plane_set_property() could use it by accepting REFLECT_X|REFLECT_Y too. But it does what it says, so I'm also ok if you leave it unused for now.
Yeah I was going back and forth whether I should accept the reflect flags, but in the end decided against it since we can only support a limited combination of the flags. But the function could be useful for someone else.
--Imre
- transforms the hardware supports, this function may not
- be able to produce a supported transform, so the caller should
- check the result afterwards.
- */
+unsigned int drm_rotation_simplify(unsigned int rotation,
unsigned int supported_rotations)+{
- if (rotation & ~supported_rotations) {
rotation ^= BIT(DRM_REFLECT_X) | BIT(DRM_REFLECT_Y);rotation = (rotation & ~0xf) | BIT((ffs(rotation & 0xf) + 1) % 4);- }
- return rotation;
+} +EXPORT_SYMBOL(drm_rotation_simplify);
+/**
- drm_mode_config_init - initialize DRM mode_configuration structure
- @dev: DRM device
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index d25ba84..b97b367 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1154,5 +1154,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format); extern const char *drm_get_format_name(uint32_t format); extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev, unsigned int supported_rotations); +extern unsigned int drm_rotation_simplify(unsigned int rotation,
unsigned int supported_rotations);#endif /* __DRM_CRTC_H__ */
From: Ville Syrjälä ville.syrjala@linux.intel.com
The sprite planes (in fact all display planes starting from gen4) support 180 degree rotation. Add the relevant low level bits to the sprite code to make use of that feature.
The upper layers are not yet plugged in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 38 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 96fd2ce..ca5cc60 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3518,6 +3518,7 @@ #define DVS_YUV_ORDER_UYVY (1<<16) #define DVS_YUV_ORDER_YVYU (2<<16) #define DVS_YUV_ORDER_VYUY (3<<16) +#define DVS_ROTATE_180 (1<<15) #define DVS_DEST_KEY (1<<2) #define DVS_TRICKLE_FEED_DISABLE (1<<14) #define DVS_TILED (1<<10) @@ -3588,6 +3589,7 @@ #define SPRITE_YUV_ORDER_UYVY (1<<16) #define SPRITE_YUV_ORDER_YVYU (2<<16) #define SPRITE_YUV_ORDER_VYUY (3<<16) +#define SPRITE_ROTATE_180 (1<<15) #define SPRITE_TRICKLE_FEED_DISABLE (1<<14) #define SPRITE_INT_GAMMA_ENABLE (1<<13) #define SPRITE_TILED (1<<10) @@ -3661,6 +3663,7 @@ #define SP_YUV_ORDER_UYVY (1<<16) #define SP_YUV_ORDER_YVYU (2<<16) #define SP_YUV_ORDER_VYUY (3<<16) +#define SP_ROTATE_180 (1<<15) #define SP_TILED (1<<10) #define _SPALINOFF (VLV_DISPLAY_BASE + 0x72184) #define _SPASTRIDE (VLV_DISPLAY_BASE + 0x72188) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9364d98..89282d5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -370,6 +370,7 @@ struct intel_plane { unsigned int crtc_w, crtc_h; uint32_t src_x, src_y; uint32_t src_w, src_h; + unsigned int rotation;
/* Since we need to change the watermarks before/after * enabling/disabling the planes, we need to store the parameters here diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index f86caba..c0081f0 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -60,6 +60,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, sprctl &= ~SP_PIXFORMAT_MASK; sprctl &= ~SP_YUV_BYTE_ORDER_MASK; sprctl &= ~SP_TILED; + sprctl &= ~SP_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_YUYV: @@ -128,6 +129,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, fb->pitches[0]); linear_offset -= sprsurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + sprctl |= SP_ROTATE_180; + + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + if (obj->tiling_mode != I915_TILING_NONE) I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x); else @@ -233,6 +242,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, sprctl &= ~SPRITE_RGB_ORDER_RGBX; sprctl &= ~SPRITE_YUV_BYTE_ORDER_MASK; sprctl &= ~SPRITE_TILED; + sprctl &= ~SPRITE_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_XBGR8888: @@ -304,6 +314,14 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, pixel_size, fb->pitches[0]); linear_offset -= sprsurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + sprctl |= SPRITE_ROTATE_180; + + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + /* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET * register */ if (IS_HASWELL(dev)) @@ -429,6 +447,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, dvscntr &= ~DVS_RGB_ORDER_XBGR; dvscntr &= ~DVS_YUV_BYTE_ORDER_MASK; dvscntr &= ~DVS_TILED; + dvscntr &= ~DVS_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_XBGR8888: @@ -482,6 +501,14 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, pixel_size, fb->pitches[0]); linear_offset -= dvssurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + dvscntr |= DVS_ROTATE_180; + + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + if (obj->tiling_mode != I915_TILING_NONE) I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x); else @@ -729,6 +756,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, max_scale = intel_plane->max_downscale << 16; min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+ drm_rect_rotate(&src, fb->width << 16, fb->height << 16, + intel_plane->rotation); + hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale); BUG_ON(hscale < 0);
@@ -767,6 +797,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, drm_rect_width(&dst) * hscale - drm_rect_width(&src), drm_rect_height(&dst) * vscale - drm_rect_height(&src));
+ drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16, + intel_plane->rotation); + /* sanity check to make sure the src viewport wasn't enlarged */ WARN_ON(src.x1 < (int) src_x || src.y1 < (int) src_y || @@ -1129,6 +1162,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
intel_plane->pipe = pipe; intel_plane->plane = plane; + intel_plane->rotation = BIT(DRM_ROTATE_0); possible_crtcs = (1 << pipe); ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs, &intel_plane_funcs,
From: Ville Syrjälä ville.syrjala@linux.intel.com
The sprite planes (in fact all display planes starting from gen4) support 180 degree rotation. Add the relevant low level bits to the sprite code to make use of that feature.
The upper layers are not yet plugged in.
v2: HSW handles the rotated buffer offset automagically
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 96fd2ce..ca5cc60 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3518,6 +3518,7 @@ #define DVS_YUV_ORDER_UYVY (1<<16) #define DVS_YUV_ORDER_YVYU (2<<16) #define DVS_YUV_ORDER_VYUY (3<<16) +#define DVS_ROTATE_180 (1<<15) #define DVS_DEST_KEY (1<<2) #define DVS_TRICKLE_FEED_DISABLE (1<<14) #define DVS_TILED (1<<10) @@ -3588,6 +3589,7 @@ #define SPRITE_YUV_ORDER_UYVY (1<<16) #define SPRITE_YUV_ORDER_YVYU (2<<16) #define SPRITE_YUV_ORDER_VYUY (3<<16) +#define SPRITE_ROTATE_180 (1<<15) #define SPRITE_TRICKLE_FEED_DISABLE (1<<14) #define SPRITE_INT_GAMMA_ENABLE (1<<13) #define SPRITE_TILED (1<<10) @@ -3661,6 +3663,7 @@ #define SP_YUV_ORDER_UYVY (1<<16) #define SP_YUV_ORDER_YVYU (2<<16) #define SP_YUV_ORDER_VYUY (3<<16) +#define SP_ROTATE_180 (1<<15) #define SP_TILED (1<<10) #define _SPALINOFF (VLV_DISPLAY_BASE + 0x72184) #define _SPASTRIDE (VLV_DISPLAY_BASE + 0x72188) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9173baa..d9882e5 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -370,6 +370,7 @@ struct intel_plane { unsigned int crtc_w, crtc_h; uint32_t src_x, src_y; uint32_t src_w, src_h; + unsigned int rotation;
/* Since we need to change the watermarks before/after * enabling/disabling the planes, we need to store the parameters here diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index e001d2c..a23f8bd 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -60,6 +60,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, sprctl &= ~SP_PIXFORMAT_MASK; sprctl &= ~SP_YUV_BYTE_ORDER_MASK; sprctl &= ~SP_TILED; + sprctl &= ~SP_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_YUYV: @@ -128,6 +129,14 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc, fb->pitches[0]); linear_offset -= sprsurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + sprctl |= SP_ROTATE_180; + + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + if (obj->tiling_mode != I915_TILING_NONE) I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x); else @@ -233,6 +242,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, sprctl &= ~SPRITE_RGB_ORDER_RGBX; sprctl &= ~SPRITE_YUV_BYTE_ORDER_MASK; sprctl &= ~SPRITE_TILED; + sprctl &= ~SPRITE_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_XBGR8888: @@ -304,6 +314,17 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, pixel_size, fb->pitches[0]); linear_offset -= sprsurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + sprctl |= SPRITE_ROTATE_180; + + /* HSW does this automagically in hardware */ + if (!IS_HASWELL(dev)) { + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + } + /* HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET * register */ if (IS_HASWELL(dev)) @@ -429,6 +450,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, dvscntr &= ~DVS_RGB_ORDER_XBGR; dvscntr &= ~DVS_YUV_BYTE_ORDER_MASK; dvscntr &= ~DVS_TILED; + dvscntr &= ~DVS_ROTATE_180;
switch (fb->pixel_format) { case DRM_FORMAT_XBGR8888: @@ -482,6 +504,14 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, pixel_size, fb->pitches[0]); linear_offset -= dvssurf_offset;
+ if (intel_plane->rotation == BIT(DRM_ROTATE_180)) { + dvscntr |= DVS_ROTATE_180; + + x += src_w; + y += src_h; + linear_offset += src_h * fb->pitches[0] + src_w * pixel_size; + } + if (obj->tiling_mode != I915_TILING_NONE) I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x); else @@ -725,6 +755,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, max_scale = intel_plane->max_downscale << 16; min_scale = intel_plane->can_scale ? 1 : (1 << 16);
+ drm_rect_rotate(&src, fb->width << 16, fb->height << 16, + intel_plane->rotation); + hscale = drm_rect_calc_hscale_relaxed(&src, &dst, min_scale, max_scale); BUG_ON(hscale < 0);
@@ -763,6 +796,9 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc, drm_rect_width(&dst) * hscale - drm_rect_width(&src), drm_rect_height(&dst) * vscale - drm_rect_height(&src));
+ drm_rect_rotate_inv(&src, fb->width << 16, fb->height << 16, + intel_plane->rotation); + /* sanity check to make sure the src viewport wasn't enlarged */ WARN_ON(src.x1 < (int) src_x || src.y1 < (int) src_y || @@ -1126,6 +1162,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
intel_plane->pipe = pipe; intel_plane->plane = plane; + intel_plane->rotation = BIT(DRM_ROTATE_0); possible_crtcs = (1 << pipe); ret = drm_plane_init(dev, &intel_plane->base, possible_crtcs, &intel_plane_funcs,
From: Ville Syrjälä ville.syrjala@linux.intel.com
Propagate the error from intel_update_plane() up through intel_plane_restore() to the caller. This will be used for rollback purposes when setting properties fails.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_sprite.c | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 89282d5..bfa558a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -818,7 +818,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob); int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane); void intel_flush_display_plane(struct drm_i915_private *dev_priv, enum plane plane); -void intel_plane_restore(struct drm_plane *plane); +int intel_plane_restore(struct drm_plane *plane); void intel_plane_disable(struct drm_plane *plane); int intel_sprite_set_colorkey(struct drm_device *dev, void *data, struct drm_file *file_priv); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index c0081f0..028a979 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1033,18 +1033,18 @@ out_unlock: return ret; }
-void intel_plane_restore(struct drm_plane *plane) +int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane);
if (!plane->crtc || !plane->fb) - return; + return 0;
- intel_update_plane(plane, plane->crtc, plane->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); + return intel_update_plane(plane, plane->crtc, plane->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); }
void intel_plane_disable(struct drm_plane *plane)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Sprite planes support 180 degree rotation. The lower layers are now in place, so hook in the standard rotation property to expose the feature to the users.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 08e96a8..48d4d5f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1356,6 +1356,7 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property; + struct drm_property *rotation_property;
bool hw_contexts_disabled; uint32_t hw_context_size; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 028a979..49f8238 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1033,6 +1033,30 @@ out_unlock: return ret; }
+static int intel_plane_set_property(struct drm_plane *plane, + struct drm_property *prop, + uint64_t val) +{ + struct drm_i915_private *dev_priv = plane->dev->dev_private; + struct intel_plane *intel_plane = to_intel_plane(plane); + 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; + intel_plane->rotation = val; + ret = intel_plane_restore(plane); + if (ret) + intel_plane->rotation = old_val; + } + + return ret; +} + int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1059,6 +1083,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane, + .set_property = intel_plane_set_property, };
static uint32_t ilk_plane_formats[] = { @@ -1095,6 +1120,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) { + struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats; @@ -1168,8 +1194,22 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false); - if (ret) + if (ret) { kfree(intel_plane); + goto out; + } + + 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(&intel_plane->base.base, + dev_priv->rotation_property, + intel_plane->rotation);
+ out: return ret; }
On Mon, 2013-09-30 at 17:44 +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Sprite planes support 180 degree rotation. The lower layers are now in place, so hook in the standard rotation property to expose the feature to the users.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 08e96a8..48d4d5f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1356,6 +1356,7 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property;
struct drm_property *rotation_property;
bool hw_contexts_disabled; uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 028a979..49f8238 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1033,6 +1033,30 @@ out_unlock: return ret; }
+static int intel_plane_set_property(struct drm_plane *plane,
struct drm_property *prop,uint64_t val)+{
- struct drm_i915_private *dev_priv = plane->dev->dev_private;
- struct intel_plane *intel_plane = to_intel_plane(plane);
- 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;intel_plane->rotation = val;ret = intel_plane_restore(plane);if (ret)intel_plane->rotation = old_val;- }
- return ret;
+}
int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1059,6 +1083,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane,
- .set_property = intel_plane_set_property,
};
static uint32_t ilk_plane_formats[] = { @@ -1095,6 +1120,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) {
- struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats;
@@ -1168,8 +1194,22 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false);
- if (ret)
- if (ret) { kfree(intel_plane);
goto out;- }
- 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(&intel_plane->base.base,dev_priv->rotation_property,intel_plane->rotation);
drm_mode_create_rotation_property() can fail silently, I think we should handle it.
--Imre
On Mon, Oct 14, 2013 at 04:59:13PM +0300, Imre Deak wrote:
On Mon, 2013-09-30 at 17:44 +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Sprite planes support 180 degree rotation. The lower layers are now in place, so hook in the standard rotation property to expose the feature to the users.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 08e96a8..48d4d5f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1356,6 +1356,7 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property;
struct drm_property *rotation_property;
bool hw_contexts_disabled; uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 028a979..49f8238 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1033,6 +1033,30 @@ out_unlock: return ret; }
+static int intel_plane_set_property(struct drm_plane *plane,
struct drm_property *prop,uint64_t val)+{
- struct drm_i915_private *dev_priv = plane->dev->dev_private;
- struct intel_plane *intel_plane = to_intel_plane(plane);
- 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;intel_plane->rotation = val;ret = intel_plane_restore(plane);if (ret)intel_plane->rotation = old_val;- }
- return ret;
+}
int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1059,6 +1083,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane,
- .set_property = intel_plane_set_property,
};
static uint32_t ilk_plane_formats[] = { @@ -1095,6 +1120,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) {
- struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats;
@@ -1168,8 +1194,22 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false);
- if (ret)
- if (ret) { kfree(intel_plane);
goto out;- }
- 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(&intel_plane->base.base,dev_priv->rotation_property,intel_plane->rotation);drm_mode_create_rotation_property() can fail silently, I think we should handle it.
I figured I'd just move it to intel_modeset_init() but turns out we don't really handle errors there either. Frankly this looks like another rathole. Seeing as we already ignore property creation errors in other places (in a way that could oops even), I'm just going to run away before the urge to fix it all takes over.
On Mon, 2013-10-14 at 17:39 +0300, Ville Syrjälä wrote:
On Mon, Oct 14, 2013 at 04:59:13PM +0300, Imre Deak wrote:
On Mon, 2013-09-30 at 17:44 +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Sprite planes support 180 degree rotation. The lower layers are now in place, so hook in the standard rotation property to expose the feature to the users.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_sprite.c | 42 ++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 08e96a8..48d4d5f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1356,6 +1356,7 @@ typedef struct drm_i915_private {
struct drm_property *broadcast_rgb_property; struct drm_property *force_audio_property;
struct drm_property *rotation_property;
bool hw_contexts_disabled; uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 028a979..49f8238 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -1033,6 +1033,30 @@ out_unlock: return ret; }
+static int intel_plane_set_property(struct drm_plane *plane,
struct drm_property *prop,uint64_t val)+{
- struct drm_i915_private *dev_priv = plane->dev->dev_private;
- struct intel_plane *intel_plane = to_intel_plane(plane);
- 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;intel_plane->rotation = val;ret = intel_plane_restore(plane);if (ret)intel_plane->rotation = old_val;- }
- return ret;
+}
int intel_plane_restore(struct drm_plane *plane) { struct intel_plane *intel_plane = to_intel_plane(plane); @@ -1059,6 +1083,7 @@ static const struct drm_plane_funcs intel_plane_funcs = { .update_plane = intel_update_plane, .disable_plane = intel_disable_plane, .destroy = intel_destroy_plane,
- .set_property = intel_plane_set_property,
};
static uint32_t ilk_plane_formats[] = { @@ -1095,6 +1120,7 @@ static uint32_t vlv_plane_formats[] = { int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) {
- struct drm_i915_private *dev_priv = dev->dev_private; struct intel_plane *intel_plane; unsigned long possible_crtcs; const uint32_t *plane_formats;
@@ -1168,8 +1194,22 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane) &intel_plane_funcs, plane_formats, num_plane_formats, false);
- if (ret)
- if (ret) { kfree(intel_plane);
goto out;- }
- 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(&intel_plane->base.base,dev_priv->rotation_property,intel_plane->rotation);drm_mode_create_rotation_property() can fail silently, I think we should handle it.
I figured I'd just move it to intel_modeset_init() but turns out we don't really handle errors there either. Frankly this looks like another rathole. Seeing as we already ignore property creation errors in other places (in a way that could oops even), I'm just going to run away before the urge to fix it all takes over.
Ok, since properties are already unreliable in this sense, I'm fine with leaving this as-is for now. The series looks ok, so:
Reviewed-by: Imre Deak imre.deak@intel.com
On Mon, Sep 30, 2013 at 10:44 AM, ville.syrjala@linux.intel.com wrote:
Recently some people from inside Intel have showed some interest in 180 degree plane rotation. To avoid a huge mess, I decided that I should implement the feature properly.
So I snatched the rotation property from omapdrm, and moved some of the code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect and implemented the relevant bits in i915. I didn't touch cursor or primary planes at all. I'm sort of hoping we might do the drm_plane conversion sometime soonish and then we'd avoid adding a bunch of plane properties to the crtc.
fwiw, I was leaning towards introducing primary-plane's visible to userspace after we have atomic modeset (or really, the propertyification associated with atomic modeset).
But that should be independent of drm_plane conversion. You probably still want to do something like what I did in omapdrm where you attach plane properties on the crtc as well for benefit of old userspace.
One thing I don't really like is the current way of stuffing the bit number into the enum_list resulting in DRM_ROTATE_FOO being just the bit number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm not sure if changing that would cause grief to userspace, so I left it alone for now.
I think this shouldn't be visible to userspace. If I remember correctly, I just did it this way to make it easier to prevent users of bitmask property from doing the wrong thing (setting multiple bits, overlapping bitmask values, etc).
Anyways, from a really quick look, the core and omapdrm parts look good.
The drm_rotation_simplify() might be overkill.. or at least userspace should see what are the supported bitmask flags and not try to ask for something that is not supported. Or am I missing something?
BR, -R
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Sep 30, 2013 at 12:15:06PM -0400, Rob Clark wrote:
On Mon, Sep 30, 2013 at 10:44 AM, ville.syrjala@linux.intel.com wrote:
Recently some people from inside Intel have showed some interest in 180 degree plane rotation. To avoid a huge mess, I decided that I should implement the feature properly.
So I snatched the rotation property from omapdrm, and moved some of the code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect and implemented the relevant bits in i915. I didn't touch cursor or primary planes at all. I'm sort of hoping we might do the drm_plane conversion sometime soonish and then we'd avoid adding a bunch of plane properties to the crtc.
fwiw, I was leaning towards introducing primary-plane's visible to userspace after we have atomic modeset (or really, the propertyification associated with atomic modeset).
Less burden of maintaining all the crtc properties if we convert to drm_plane first.
But that should be independent of drm_plane conversion. You probably still want to do something like what I did in omapdrm where you attach plane properties on the crtc as well for benefit of old userspace.
Dunno. There's no old userspace that would use this. There are some folks inside Intel who apparently want rotation for some thing, but I was thinking I'd let them add the properties on the crtc and not upstream any of that.
One thing I don't really like is the current way of stuffing the bit number into the enum_list resulting in DRM_ROTATE_FOO being just the bit number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm not sure if changing that would cause grief to userspace, so I left it alone for now.
I think this shouldn't be visible to userspace. If I remember correctly, I just did it this way to make it easier to prevent users of bitmask property from doing the wrong thing (setting multiple bits, overlapping bitmask values, etc).
Well setting multiple bits should be allowed. If it's not we need to fix it since rotation+reflection sure needs it. Or did you mean users as in driver code in the kernel which registers the bitmask prop?
I also just had an idea to expose color keys, bg colors, etc. as bitmask props where each channel is represented by a multi-bit mask, and the whole thing is just one bitmask prop. That would expose some structure to userspace w/o need a new prop type. But maybe we want a specialized type for color props for extra clarty.
Anyways, from a really quick look, the core and omapdrm parts look good.
The drm_rotation_simplify() might be overkill.. or at least userspace should see what are the supported bitmask flags and not try to ask for something that is not supported. Or am I missing something?
My main idea was that, for example if the hardware support X and Y reflection, we can expose both 0 and 180 angles and X and Y reflection, and the driver code can then do the simplification to elimnate the 180 degree case. So it's just a convenience tool for the driver authors to eliminate the redundant information. I didn't actually end up using it in i915 since we really don't support anything but 0 and 180 cases, and advertising anything else to userspace would be a bad idea.
On Mon, Sep 30, 2013 at 1:09 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Sep 30, 2013 at 12:15:06PM -0400, Rob Clark wrote:
On Mon, Sep 30, 2013 at 10:44 AM, ville.syrjala@linux.intel.com wrote:
Recently some people from inside Intel have showed some interest in 180 degree plane rotation. To avoid a huge mess, I decided that I should implement the feature properly.
So I snatched the rotation property from omapdrm, and moved some of the code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect and implemented the relevant bits in i915. I didn't touch cursor or primary planes at all. I'm sort of hoping we might do the drm_plane conversion sometime soonish and then we'd avoid adding a bunch of plane properties to the crtc.
fwiw, I was leaning towards introducing primary-plane's visible to userspace after we have atomic modeset (or really, the propertyification associated with atomic modeset).
Less burden of maintaining all the crtc properties if we convert to drm_plane first.
But that should be independent of drm_plane conversion. You probably still want to do something like what I did in omapdrm where you attach plane properties on the crtc as well for benefit of old userspace.
Dunno. There's no old userspace that would use this. There are some folks inside Intel who apparently want rotation for some thing, but I was thinking I'd let them add the properties on the crtc and not upstream any of that.
for properties that aren't actually handled by the crtc, just make your crtc_set_prop() call:
intel_plane_set_property(intel_crtc->plane, property, val)
(or something roughly like that)
One thing I don't really like is the current way of stuffing the bit number into the enum_list resulting in DRM_ROTATE_FOO being just the bit number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm not sure if changing that would cause grief to userspace, so I left it alone for now.
I think this shouldn't be visible to userspace. If I remember correctly, I just did it this way to make it easier to prevent users of bitmask property from doing the wrong thing (setting multiple bits, overlapping bitmask values, etc).
Well setting multiple bits should be allowed. If it's not we need to fix it since rotation+reflection sure needs it. Or did you mean users as in driver code in the kernel which registers the bitmask prop?
yeah, setting multiple bits is allowed.. this is why it isn't an enum property ;-)
I meant users as in driver code rather than userspace. I guess that was worded a bit confusingly. I blame the cough syrup.
I also just had an idea to expose color keys, bg colors, etc. as bitmask props where each channel is represented by a multi-bit mask, and the whole thing is just one bitmask prop. That would expose some structure to userspace w/o need a new prop type. But maybe we want a specialized type for color props for extra clarty.
hmm.. should the values be interpreted in terms of current attached fb format?
Anyways, from a really quick look, the core and omapdrm parts look good.
The drm_rotation_simplify() might be overkill.. or at least userspace should see what are the supported bitmask flags and not try to ask for something that is not supported. Or am I missing something?
My main idea was that, for example if the hardware support X and Y reflection, we can expose both 0 and 180 angles and X and Y reflection, and the driver code can then do the simplification to elimnate the 180 degree case. So it's just a convenience tool for the driver authors to eliminate the redundant information. I didn't actually end up using it in i915 since we really don't support anything but 0 and 180 cases, and advertising anything else to userspace would be a bad idea.
oh, right.. we do have some redundancy in rotation values. Maybe we should have stuck with xyflip/yinvert/xinvert (which was how the omap hw worked.. but seemed a bit too hw specific).
I guess the main thing I care about is that we don't advertise things to userspace that we can't actually do. I'm not sure what other hw out there supports rotation in hw in some form or another, but it might be a good time to hear from 'em about whether these property values work for them or not.
BR, -R
-- Ville Syrjälä Intel OTC
On Mon, Sep 30, 2013 at 7:46 PM, Rob Clark robdclark@gmail.com wrote:
I guess the main thing I care about is that we don't advertise things to userspace that we can't actually do. I'm not sure what other hw out there supports rotation in hw in some form or another, but it might be a good time to hear from 'em about whether these property values work for them or not.
Hm, I've thought the plan was to let userspace figure that out with a dry-run flag, and if a certain configuration doesn't work it needs to fall back to rendering-based compositioning for the given surface. I don't think there's really much more we can do for fully generic compositors.
Tha might leave strange hw in the dust where planes aren't symmetric in capabilities and hence a simple linear walk over surfaces/planes, ordered by bw-savings or so, yields extermely bad surface->plane assignements. But my impression is that hw is moving to unified stacks of planes so I hope we can punt on solving this in a generic way (and resort to quick platform hacks in userspace where it's really needed to hit e.g. video playback power targets). -Daniel
On Mon, Sep 30, 2013 at 08:21:33PM +0200, Daniel Vetter wrote:
On Mon, Sep 30, 2013 at 7:46 PM, Rob Clark robdclark@gmail.com wrote:
I guess the main thing I care about is that we don't advertise things to userspace that we can't actually do. I'm not sure what other hw out there supports rotation in hw in some form or another, but it might be a good time to hear from 'em about whether these property values work for them or not.
Hm, I've thought the plan was to let userspace figure that out with a dry-run flag, and if a certain configuration doesn't work it needs to fall back to rendering-based compositioning for the given surface. I don't think there's really much more we can do for fully generic compositors.
Yeah, if no one comes up with anything better, the trial and error approach is the plan in my mind.
But in case the hardware never ever supports certain property/value I think we shouldn't expose it. Like 90/270 degree rotation in case of intel hardware.
Tha might leave strange hw in the dust where planes aren't symmetric in capabilities and hence a simple linear walk over surfaces/planes, ordered by bw-savings or so, yields extermely bad surface->plane assignements. But my impression is that hw is moving to unified stacks of planes so I hope we can punt on solving this in a generic way (and resort to quick platform hacks in userspace where it's really needed to hit e.g. video playback power targets).
Right. If someone has really specific needs I think they can just go and write their own compositor (or plugin if the compositor in question has such things).
On Mon, Sep 30, 2013 at 01:46:11PM -0400, Rob Clark wrote:
On Mon, Sep 30, 2013 at 1:09 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Mon, Sep 30, 2013 at 12:15:06PM -0400, Rob Clark wrote:
On Mon, Sep 30, 2013 at 10:44 AM, ville.syrjala@linux.intel.com wrote:
Recently some people from inside Intel have showed some interest in 180 degree plane rotation. To avoid a huge mess, I decided that I should implement the feature properly.
So I snatched the rotation property from omapdrm, and moved some of the code into drm_crtc.c, added a bunch of helper stuff to drm_crtc/rect and implemented the relevant bits in i915. I didn't touch cursor or primary planes at all. I'm sort of hoping we might do the drm_plane conversion sometime soonish and then we'd avoid adding a bunch of plane properties to the crtc.
fwiw, I was leaning towards introducing primary-plane's visible to userspace after we have atomic modeset (or really, the propertyification associated with atomic modeset).
Less burden of maintaining all the crtc properties if we convert to drm_plane first.
But that should be independent of drm_plane conversion. You probably still want to do something like what I did in omapdrm where you attach plane properties on the crtc as well for benefit of old userspace.
Dunno. There's no old userspace that would use this. There are some folks inside Intel who apparently want rotation for some thing, but I was thinking I'd let them add the properties on the crtc and not upstream any of that.
for properties that aren't actually handled by the crtc, just make your crtc_set_prop() call:
intel_plane_set_property(intel_crtc->plane, property, val)
(or something roughly like that)
Sure it's doable, but I tend to think it's rather pointless since we don't have any legacy userspace to worry about yet.
One thing I don't really like is the current way of stuffing the bit number into the enum_list resulting in DRM_ROTATE_FOO being just the bit number. I'd prefer to make DRM_ROTATE_FOO the shifted bitmask. But I'm not sure if changing that would cause grief to userspace, so I left it alone for now.
I think this shouldn't be visible to userspace. If I remember correctly, I just did it this way to make it easier to prevent users of bitmask property from doing the wrong thing (setting multiple bits, overlapping bitmask values, etc).
Well setting multiple bits should be allowed. If it's not we need to fix it since rotation+reflection sure needs it. Or did you mean users as in driver code in the kernel which registers the bitmask prop?
yeah, setting multiple bits is allowed.. this is why it isn't an enum property ;-)
I meant users as in driver code rather than userspace. I guess that was worded a bit confusingly. I blame the cough syrup.
Right.
I also just had an idea to expose color keys, bg colors, etc. as bitmask props where each channel is represented by a multi-bit mask, and the whole thing is just one bitmask prop. That would expose some structure to userspace w/o need a new prop type. But maybe we want a specialized type for color props for extra clarty.
hmm.. should the values be interpreted in terms of current attached fb format?
My current thinking is to go with a fixed 16 bits per channel, and just throw away any low bits you don't need.
Anyways, from a really quick look, the core and omapdrm parts look good.
The drm_rotation_simplify() might be overkill.. or at least userspace should see what are the supported bitmask flags and not try to ask for something that is not supported. Or am I missing something?
My main idea was that, for example if the hardware support X and Y reflection, we can expose both 0 and 180 angles and X and Y reflection, and the driver code can then do the simplification to elimnate the 180 degree case. So it's just a convenience tool for the driver authors to eliminate the redundant information. I didn't actually end up using it in i915 since we really don't support anything but 0 and 180 cases, and advertising anything else to userspace would be a bad idea.
oh, right.. we do have some redundancy in rotation values. Maybe we should have stuck with xyflip/yinvert/xinvert (which was how the omap hw worked.. but seemed a bit too hw specific).
That's omap4 or omap5 i take it. Omap3 had 0,90,180,270 + x mirror for DMA rotation, and 0+90+180+270 + y mirror for VRFB. The mismatch was interesting since omap angle was clockwise, randr counter-clockwise, and omap mirroring was post-rotation, randr pre-rotation.
Also the xyflip/etc. is just confusing to me, so I'm happy you went with randr compatible specification rather than whatever your hw had.
I guess the main thing I care about is that we don't advertise things to userspace that we can't actually do. I'm not sure what other hw out there supports rotation in hw in some form or another, but it might be a good time to hear from 'em about whether these property values work for them or not.
I think the current stuff is good. In my years I've seen hardware that supports everything (omap), just 0+180 (intel gen4+), just 0+180+x+y (intel gen2/3 video overlay), just 0+x (matrox g200+), just 0 (ati mach64 and r128 era stuff).
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Imre Deak -
Rob Clark -
Ville Syrjälä -
ville.syrjala＠linux.intel.com