[PATCH 0/9] drm/i915: Plane rotation support

[PATCH 0/9] drm/i915: Plane rotation support

[ville.syrjala]

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.

[PATCH 1/9] drm: Move DRM_ROTATE bits out of omapdrm into drm_crtc.h

[ville.syrjala]

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,
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/9] drm: Add support_bits parameter to drm_property_create_bitmask()

[ville.syrjala]

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);
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/9] drm: Add drm_mode_create_rotation_property()

[ville.syrjala]

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__ */
-- 
1.8.1.5

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

[PATCH 4/9] drm/omap: Switch omapdrm over to drm_mode_create_rotation_property()

[ville.syrjala]

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;
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm: Add drm_rect rotation functions

[ville.syrjala]

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
-- 
1.8.1.5

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

[PATCH 6/9] drm: Add drm_rotation_simplify()

[ville.syrjala]

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__ */
-- 
1.8.1.5

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

Re: [Intel-gfx] [PATCH 6/9] drm: Add drm_rotation_simplify()

[Imre Deak]

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

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__ */


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 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: [PATCH 6/9] drm: Add drm_rotation_simplify()

[Ville Syrjälä]

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_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.

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__ */
> 



-- 
Ville Syrjälä
Intel OTC

[PATCH 7/9] drm/i915: Add 180 degree sprite rotation support

[ville.syrjala]

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,
-- 
1.8.1.5

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

[PATCH v2 7/9] drm/i915: Add 180 degree sprite rotation support

[ville.syrjala]

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,
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm/i915: Make intel_plane_restore() return an error

[ville.syrjala]

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)
-- 
1.8.1.5

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

[PATCH 9/9] drm/i915: Add rotation property for sprites

[ville.syrjala]

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;
 }
-- 
1.8.1.5

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

Re: [PATCH 9/9] drm/i915: Add rotation property for sprites

[Imre Deak]

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

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


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 9/9] drm/i915: Add rotation property for sprites

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 9/9] drm/i915: Add rotation property for sprites

[Imre Deak]

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

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>


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Rob Clark]

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

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Rob Clark]

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

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Ville Syrjälä]

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).

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 0/9] drm/i915: Plane rotation support

[Ville Syrjälä]

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).

-- 
Ville Syrjälä
Intel OTC