| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 17 May 2017 10:30:59 -0400
From: Robert Foss <robert.foss@...labora.com>
To: Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc: dri-devel@...ts.freedesktop.org,
Tomeu Vizoso <tomeu.vizoso@...labora.com>,
Emil Velikov <emil.l.velikov@...il.com>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Kristian Høgsberg <hoegsberg@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI
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.foss@...labora.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@...ts.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Powered by blists - more mailing lists