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

[Emil Velikov]

Hi Rob,

On 14 May 2017 at 18:26, Robert Foss <robert.foss@xxxxxxxxxxxxx> 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>
> ---
>  drivers/gpu/drm/drm_rect.c |  1 +
>  include/drm/drm_blend.h    | 18 ------------
>  include/uapi/drm/drm.h     | 73 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
> index bc5575960ebc..bdb27434bb10 100644
> --- a/drivers/gpu/drm/drm_rect.c
> +++ b/drivers/gpu/drm/drm_rect.c
> @@ -24,6 +24,7 @@
>  #include <linux/errno.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
> +#include <drm/drm.h>
>  #include <drm/drmP.h>
>  #include <drm/drm_rect.h>
>
> diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> index 13221cf9b3eb..d149a63b893b 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -29,24 +29,6 @@
>  struct drm_device;
>  struct drm_atomic_state;
>
Since the defines are used here, move the above include to this file?

> -/*
> - * 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);
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 42d9f64ce416..d7140b0091bc 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h

drm_mode.h might be a better fit.

> @@ -697,6 +697,79 @@ struct drm_prime_handle {
>         __s32 fd;
>  };
>
> +/** DRM_ROTATE_0
> + *
> + * Signals that a drm plane has been rotated 0 degrees.
> + *
> + * This define is provided as a convenience, looking up the property id
> + * using the name->prop id lookup is the preferred method.
Comments look quite good, small question:
Is it plane only? Haven't looked at the code, but gut feeling says no.

> + */
> +#define DRM_ROTATE_0           BIT(0)
> +
The UAPI headers do not use BIT(). Please use (1<<x) instead.

-Emil