Re: [PATCH v2] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI
From: Ville Syrjälä
Date: Tue May 16 2017 - 12:20:21 EST
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.
>
> 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.foss@xxxxxxxxxxxxx>
> ---
> 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.
> 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?
> + *
> + * 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.
> + */
> +#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.
> + *
> + * 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC