Re: [PATCH v2] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI

Robert Foss Wed, 17 May 2017 07:31:43 -0700

Hey Ville,

On 2017-05-16 12:20 PM, Ville Syrjälä wrote:

On Tue, May 16, 2017 at 11:55:00AM -0400, Robert Foss wrote:

Add DRM_ROTATE_ and DRM_REFLECT_ defines to the UAPI as a convenience.


I just noticed this line using the wrong define names.
Will fix in v3.


Ideally the DRM_ROTATE_ and DRM_REFLECT_ property ids are looked up
through the atomic API, but realizing that userspace is likely to take
shortcuts and assume that the enum values are what is sent over the
wire.

As a result these defines are provided purely as a convenience to
userspace applications.

Signed-off-by: Robert Foss <robert.f...@collabora.com>
---
Changes since v1:
 - Moved defines from drm.h to drm_mode.h
 - Changed define prefix from DRM_ to DRM_MODE_PROP_


DRM_MODE_PROP_ would potentially cause confusion with the prop types.
DRM_MODE_ROTATE_ etc. could be acceptable I suppose.

 - Updated uses of the defines to the new prefix
 - Removed include from drm_rect.c
 - Stopped using the BIT() macro

 drivers/gpu/drm/drm_atomic.c           |  2 +-
 drivers/gpu/drm/drm_atomic_helper.c    |  2 +-
 drivers/gpu/drm/drm_blend.c            | 43 +++++++++----------
 drivers/gpu/drm/drm_fb_helper.c        |  4 +-
 drivers/gpu/drm/drm_plane_helper.c     |  2 +-
 drivers/gpu/drm/drm_rect.c             | 36 ++++++++--------
 drivers/gpu/drm/nouveau/nv50_display.c |  2 +-
 include/drm/drm_blend.h                | 21 +---------
 include/uapi/drm/drm_mode.h            | 76 ++++++++++++++++++++++++++++++++++


I'm pretty sure this won't even compile properly since it's missing all
but one driver.


I did check it using an arbitrary Kconfig, but I also missed a ton of uses.
Will fix in v3.

 9 files changed, 124 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index f32506a7c1d6..ec1839b01d2a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -769,7 +769,7 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
        } else if (property == config->prop_src_h) {
                state->src_h = val;
        } else if (property == plane->rotation_property) {
-               if (!is_power_of_2(val & DRM_ROTATE_MASK))
+               if (!is_power_of_2(val & DRM_MODE_PROP_ROTATE_MASK))
                        return -EINVAL;
                state->rotation = val;
        } else if (property == plane->zpos_property) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
b/drivers/gpu/drm/drm_atomic_helper.c
index 8be9719284b0..37f461aa5e66 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3220,7 +3220,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
*plane)

        if (plane->state) {
                plane->state->plane = plane;
-               plane->state->rotation = DRM_ROTATE_0;
+               plane->state->rotation = DRM_MODE_PROP_ROTATE_0;
        }
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index a0d0d6843288..044640a04d51 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -119,15 +119,15 @@
  * drm_property_create_bitmask()) called "rotation" and has the following
  * bitmask enumaration values:
  *
- * DRM_ROTATE_0:
+ * DRM_MODE_PROP_ROTATE_0:
  *     "rotate-0"
- * DRM_ROTATE_90:
+ * DRM_MODE_PROP_ROTATE_90:
  *     "rotate-90"
- * DRM_ROTATE_180:
+ * DRM_MODE_PROP_ROTATE_180:
  *     "rotate-180"
- * DRM_ROTATE_270:
+ * DRM_MODE_PROP_ROTATE_270:
  *     "rotate-270"
- * DRM_REFLECT_X:
+ * DRM_MODE_PROP_REFLECT_X:
  *     "reflect-x"
  * DRM_REFELCT_Y:
  *     "reflect-y"
@@ -142,17 +142,17 @@ int drm_plane_create_rotation_property(struct drm_plane 
*plane,
                                       unsigned int supported_rotations)
 {
        static const struct drm_prop_enum_list props[] = {
-               { __builtin_ffs(DRM_ROTATE_0) - 1,   "rotate-0" },
-               { __builtin_ffs(DRM_ROTATE_90) - 1,  "rotate-90" },
-               { __builtin_ffs(DRM_ROTATE_180) - 1, "rotate-180" },
-               { __builtin_ffs(DRM_ROTATE_270) - 1, "rotate-270" },
-               { __builtin_ffs(DRM_REFLECT_X) - 1,  "reflect-x" },
-               { __builtin_ffs(DRM_REFLECT_Y) - 1,  "reflect-y" },
+               { __builtin_ffs(DRM_MODE_PROP_ROTATE_0) - 1,   "rotate-0" },
+               { __builtin_ffs(DRM_MODE_PROP_ROTATE_90) - 1,  "rotate-90" },
+               { __builtin_ffs(DRM_MODE_PROP_ROTATE_180) - 1, "rotate-180" },
+               { __builtin_ffs(DRM_MODE_PROP_ROTATE_270) - 1, "rotate-270" },
+               { __builtin_ffs(DRM_MODE_PROP_REFLECT_X) - 1,  "reflect-x" },
+               { __builtin_ffs(DRM_MODE_PROP_REFLECT_Y) - 1,  "reflect-y" },
        };
        struct drm_property *prop;

-       WARN_ON((supported_rotations & DRM_ROTATE_MASK) == 0);
-       WARN_ON(!is_power_of_2(rotation & DRM_ROTATE_MASK));
+       WARN_ON((supported_rotations & DRM_MODE_PROP_ROTATE_MASK) == 0);
+       WARN_ON(!is_power_of_2(rotation & DRM_MODE_PROP_ROTATE_MASK));
        WARN_ON(rotation & ~supported_rotations);

        prop = drm_property_create_bitmask(plane->dev, 0, "rotation",
@@ -178,14 +178,14 @@ EXPORT_SYMBOL(drm_plane_create_rotation_property);
  * @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
+ * Eg. if the hardware supports everything except DRM_MODE_PROP_REFLECT_X
  * one could call this function like this:
  *
- * drm_rotation_simplify(rotation, DRM_ROTATE_0 |
- *                       DRM_ROTATE_90 | DRM_ROTATE_180 |
- *                       DRM_ROTATE_270 | DRM_REFLECT_Y);
+ * drm_rotation_simplify(rotation, DRM_MODE_PROP_ROTATE_0 |
+ *                       DRM_MODE_PROP_ROTATE_90 | DRM_MODE_PROP_ROTATE_180 |
+ *                       DRM_MODE_PROP_ROTATE_270 | DRM_MODE_PROP_REFLECT_Y);
  *
- * to eliminate the DRM_ROTATE_X flag. Depending on what kind of
+ * to eliminate the DRM_MODE_PROP_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.
@@ -194,9 +194,10 @@ unsigned int drm_rotation_simplify(unsigned int rotation,
                                   unsigned int supported_rotations)
 {
        if (rotation & ~supported_rotations) {
-               rotation ^= DRM_REFLECT_X | DRM_REFLECT_Y;
-               rotation = (rotation & DRM_REFLECT_MASK) |
-                          BIT((ffs(rotation & DRM_ROTATE_MASK) + 1) % 4);
+               rotation ^= DRM_MODE_PROP_REFLECT_X | DRM_MODE_PROP_REFLECT_Y;
+               rotation = (rotation & DRM_MODE_PROP_REFLECT_MASK) |
+                          BIT((ffs(rotation & DRM_MODE_PROP_ROTATE_MASK) + 1)
+                          % 4);
        }

        return rotation;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1f178b878e42..0af024a9ff1d 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -378,7 +378,7 @@ static int restore_fbdev_mode_atomic(struct drm_fb_helper 
*fb_helper)
                        goto fail;
                }

-               plane_state->rotation = DRM_ROTATE_0;
+               plane_state->rotation = DRM_MODE_PROP_ROTATE_0;

                plane->old_fb = plane->fb;
                plane_mask |= 1 << drm_plane_index(plane);
@@ -431,7 +431,7 @@ static int restore_fbdev_mode_legacy(struct drm_fb_helper 
*fb_helper)
                if (plane->rotation_property)
                        drm_mode_plane_set_obj_prop(plane,
                                                    plane->rotation_property,
-                                                   DRM_ROTATE_0);
+                                                   DRM_MODE_PROP_ROTATE_0);
        }

        for (i = 0; i < fb_helper->crtc_count; i++) {
diff --git a/drivers/gpu/drm/drm_plane_helper.c 
b/drivers/gpu/drm/drm_plane_helper.c
index b84a295230fc..d46deea69baf 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -336,7 +336,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,

        ret = drm_plane_helper_check_update(plane, crtc, fb,
                                            &src, &dest, &clip,
-                                           DRM_ROTATE_0,
+                                           DRM_MODE_PROP_ROTATE_0,
                                            DRM_PLANE_HELPER_NO_SCALING,
                                            DRM_PLANE_HELPER_NO_SCALING,
                                            false, false, &visible);
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index bc5575960ebc..5adb528adb88 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -310,38 +310,38 @@ void drm_rect_rotate(struct drm_rect *r,
 {
        struct drm_rect tmp;

-       if (rotation & (DRM_REFLECT_X | DRM_REFLECT_Y)) {
+       if (rotation & (DRM_MODE_PROP_REFLECT_X | DRM_MODE_PROP_REFLECT_Y)) {
                tmp = *r;

-               if (rotation & DRM_REFLECT_X) {
+               if (rotation & DRM_MODE_PROP_REFLECT_X) {
                        r->x1 = width - tmp.x2;
                        r->x2 = width - tmp.x1;
                }

-               if (rotation & DRM_REFLECT_Y) {
+               if (rotation & DRM_MODE_PROP_REFLECT_Y) {
                        r->y1 = height - tmp.y2;
                        r->y2 = height - tmp.y1;
                }
        }

-       switch (rotation & DRM_ROTATE_MASK) {
-       case DRM_ROTATE_0:
+       switch (rotation & DRM_MODE_PROP_ROTATE_MASK) {
+       case DRM_MODE_PROP_ROTATE_0:
                break;
-       case DRM_ROTATE_90:
+       case DRM_MODE_PROP_ROTATE_90:
                tmp = *r;
                r->x1 = tmp.y1;
                r->x2 = tmp.y2;
                r->y1 = width - tmp.x2;
                r->y2 = width - tmp.x1;
                break;
-       case DRM_ROTATE_180:
+       case DRM_MODE_PROP_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 DRM_ROTATE_270:
+       case DRM_MODE_PROP_ROTATE_270:
                tmp = *r;
                r->x1 = height - tmp.y2;
                r->x2 = height - tmp.y1;
@@ -373,8 +373,8 @@ EXPORT_SYMBOL(drm_rect_rotate);
  * 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);
+ *     DRM_MODE_PROP_ROTATE(&r, width, height, rotation);
+ *     DRM_MODE_PROP_ROTATE_inv(&r, width, height, rotation);
  *
  * you will always get back the original rectangle.
  */
@@ -384,24 +384,24 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 {
        struct drm_rect tmp;

-       switch (rotation & DRM_ROTATE_MASK) {
-       case DRM_ROTATE_0:
+       switch (rotation & DRM_MODE_PROP_ROTATE_MASK) {
+       case DRM_MODE_PROP_ROTATE_0:
                break;
-       case DRM_ROTATE_90:
+       case DRM_MODE_PROP_ROTATE_90:
                tmp = *r;
                r->x1 = width - tmp.y2;
                r->x2 = width - tmp.y1;
                r->y1 = tmp.x1;
                r->y2 = tmp.x2;
                break;
-       case DRM_ROTATE_180:
+       case DRM_MODE_PROP_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 DRM_ROTATE_270:
+       case DRM_MODE_PROP_ROTATE_270:
                tmp = *r;
                r->x1 = tmp.y1;
                r->x2 = tmp.y2;
@@ -412,15 +412,15 @@ void drm_rect_rotate_inv(struct drm_rect *r,
                break;
        }

-       if (rotation & (DRM_REFLECT_X | DRM_REFLECT_Y)) {
+       if (rotation & (DRM_MODE_PROP_REFLECT_X | DRM_MODE_PROP_REFLECT_Y)) {
                tmp = *r;

-               if (rotation & DRM_REFLECT_X) {
+               if (rotation & DRM_MODE_PROP_REFLECT_X) {
                        r->x1 = width - tmp.x2;
                        r->x2 = width - tmp.x1;
                }

-               if (rotation & DRM_REFLECT_Y) {
+               if (rotation & DRM_MODE_PROP_REFLECT_Y) {
                        r->y1 = height - tmp.y2;
                        r->y2 = height - tmp.y1;
                }
diff --git a/drivers/gpu/drm/nouveau/nv50_display.c 
b/drivers/gpu/drm/nouveau/nv50_display.c
index a7663249b3ba..082c1012b138 100644
--- a/drivers/gpu/drm/nouveau/nv50_display.c
+++ b/drivers/gpu/drm/nouveau/nv50_display.c
@@ -1033,7 +1033,7 @@ nv50_wndw_reset(struct drm_plane *plane)
                plane->funcs->atomic_destroy_state(plane, plane->state);
        plane->state = &asyw->state;
        plane->state->plane = plane;
-       plane->state->rotation = DRM_ROTATE_0;
+       plane->state->rotation = DRM_MODE_PROP_ROTATE_0;
 }

 static void
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 13221cf9b3eb..b59708c1e7a6 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -25,31 +25,14 @@

 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <drm/drm_mode.h>

 struct drm_device;
 struct drm_atomic_state;

-/*
- * Rotation property bits. DRM_ROTATE_<degrees> rotates the image by the
- * specified amount in degrees in counter clockwise direction. DRM_REFLECT_X 
and
- * DRM_REFLECT_Y reflects the image along the specified axis prior to rotation
- *
- * WARNING: These defines are UABI since they're exposed in the rotation
- * property.
- */
-#define DRM_ROTATE_0   BIT(0)
-#define DRM_ROTATE_90  BIT(1)
-#define DRM_ROTATE_180 BIT(2)
-#define DRM_ROTATE_270 BIT(3)
-#define DRM_ROTATE_MASK (DRM_ROTATE_0   | DRM_ROTATE_90 | \
-                        DRM_ROTATE_180 | DRM_ROTATE_270)
-#define DRM_REFLECT_X  BIT(4)
-#define DRM_REFLECT_Y  BIT(5)
-#define DRM_REFLECT_MASK (DRM_REFLECT_X | DRM_REFLECT_Y)
-
 static inline bool drm_rotation_90_or_270(unsigned int rotation)
 {
-       return rotation & (DRM_ROTATE_90 | DRM_ROTATE_270);
+       return rotation & (DRM_MODE_PROP_ROTATE_90 | DRM_MODE_PROP_ROTATE_270);
 }

 int drm_plane_create_rotation_property(struct drm_plane *plane,
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 8c67fc03d53d..787a70ba974c 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -127,6 +127,82 @@ extern "C" {
 #define DRM_MODE_LINK_STATUS_GOOD      0
 #define DRM_MODE_LINK_STATUS_BAD       1

+/** DRM_MODE_PROP_ROTATE_0


Is this supposed to be kernel-doc or something like that?


No, not intentionally so. Do I can rework the formatting.
Will fix in v3.

+ *
+ * Signals that a drm plane has been rotated 0 degrees.


Past tense doesn't feel right to me. Maybe "is rotated"?
But I'm not a native speaker so maybe it's just me.

+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.


Repeating this for every define seems redundant.


Ack. Will fix in v3.

+ */
+#define DRM_MODE_PROP_ROTATE_0           (1<<0)
+
+/** DRM_MODE_PROP_ROTATE_90
+ *
+ * Signals that a drm plane has been rotated 90 degrees in counter clockwise
+ * direction.
+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.
+ */
+#define DRM_MODE_PROP_ROTATE_90          (1<<1)
+
+/** DRM_MODE_PROP_ROTATE_180
+ *
+ * Signals that a drm plane has been rotated 180 degrees in counter clockwise
+ * direction.
+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.
+ */
+#define DRM_MODE_PROP_ROTATE_180         (1<<2)
+
+/** DRM_MODE_PROP_ROTATE_270
+ *
+ * Signals that a drm plane has been rotated 270 degrees in counter clockwise
+ * direction.
+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.
+ */
+#define DRM_MODE_PROP_ROTATE_270         (1<<3)
+
+
+/** DRM_MODE_PROP_ROTATE_MASK
+ *
+ * Bitmask used to look for drm plane rotations.
+ */
+#define DRM_MODE_PROP_ROTATE_MASK (DRM_MODE_PROP_ROTATE_0  | \
+                                  DRM_MODE_PROP_ROTATE_90  | \
+                                  DRM_MODE_PROP_ROTATE_180 | \
+                                  DRM_MODE_PROP_ROTATE_270)
+
+/** DRM_MODE_PROP_REFLECT_X
+ *
+ * Signals that a drm plane has been reflected in the X axis.


Seems more vague that what we had before.


I'll add some clarifications.

+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.
+ */
+#define DRM_MODE_PROP_REFLECT_X          (1<<4)
+
+/** DRM_MODE_PROP_REFLECT_Y
+ *
+ * Signals that a drm plane has been reflected in the Y axis.
+ *
+ * This define is provided as a convenience, looking up the property id
+ * using the name->prop id lookup is the preferred method.
+ */
+#define DRM_MODE_PROP_REFLECT_Y          (1<<5)
+
+
+/** DRM_MODE_PROP_REFLECT_MASK
+ *
+ * Bitmask used to look for drm plane reflections.
+ */
+#define DRM_MODE_PROP_REFLECT_MASK (DRM_MODE_PROP_REFLECT_X \
+                                   | DRM_MODE_PROP_REFLECT_Y)
+
+
 struct drm_mode_modeinfo {
        __u32 clock;
        __u16 hdisplay;
--
2.11.0.453.g787f75f05

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

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

Re: [PATCH v2] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI

Reply via email to