From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754282AbdEQObG (ORCPT ); Wed, 17 May 2017 10:31:06 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:38269 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751936AbdEQObE (ORCPT ); Wed, 17 May 2017 10:31:04 -0400 Subject: Re: [PATCH v2] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= References: <20170516155500.25616-1-robert.foss@collabora.com> <20170516162011.GT12629@intel.com> Cc: dri-devel@lists.freedesktop.org, Tomeu Vizoso , Emil Velikov , Daniel Vetter , =?UTF-8?Q?Kristian_H=c3=b8gsberg?= , linux-kernel@vger.kernel.org From: Robert Foss Message-ID: Date: Wed, 17 May 2017 10:30:59 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170516162011.GT12629@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >> #include >> +#include >> >> struct drm_device; >> struct drm_atomic_state; >> >> -/* >> - * Rotation property bits. DRM_ROTATE_ 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 >