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

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

On 2017-05-16 06:58 AM, Emil Velikov wrote:
> On 15 May 2017 at 18:13, Robert Foss <robert.foss@collabora.com> wrote:
>>
>>
>> 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?
>>
> Feel free to give them a DRM_MODE_ prefix or something else - say
> DRM_MODE_PROP_.
>
> AFAICT drm_mode.h deals with KMS specifics and the prop_id mentioned
> in the documentation is already there, so it would make sense to add
> it there.

Done.


Rob.
>
> -Emil
>