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

[Robert Foss <robert.foss@collabora.com>]

On 2017-05-15 09:23 AM, Emil Velikov wrote:
> Hi Rob,
>
> On 14 May 2017 at 18:26, Robert Foss <robert.foss@collabora.com> 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@collabora.com>
>> ---
>>  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?

Done.

>
>> -/*
>> - * 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.

About this, I don't disagree, but other defines in drm_mode.h seem to be 
prefixed with DRM_MODE_ which this isn't which is why I didn't put it there.

Knowing this, do you still prefer these defines living in drm_mode.h?

>
>> @@ -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
>

Done.


Rob.