[v1] drm: Add DRM_ROTATE_ and DRM_REFLECT_ defines to UAPI
diff mbox

Message ID 20170514172637.28937-1-robert.foss@collabora.com
State New
Headers show

Commit Message

Robert Foss May 14, 2017, 5:26 p.m. UTC
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(-)

Comments

Emil Velikov May 15, 2017, 1:23 p.m. UTC | #1
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?

> -/*
> - * 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
Robert Foss May 15, 2017, 5:13 p.m. UTC | #2
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.
Emil Velikov May 16, 2017, 10:58 a.m. UTC | #3
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.

-Emil
Robert Foss May 16, 2017, 3:51 p.m. UTC | #4
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
>

Patch
diff mbox

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;
 
-/*
- * 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
@@ -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.
+ */
+#define DRM_ROTATE_0           BIT(0)
+
+/** DRM_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_ROTATE_90          BIT(1)
+
+/** DRM_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_ROTATE_180         BIT(2)
+
+/** DRM_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_ROTATE_270         BIT(3)
+
+
+/** DRM_ROTATE_MASK
+ *
+ * Bitmask used to look for drm plane rotations.
+ */
+#define DRM_ROTATE_MASK (DRM_ROTATE_0   | DRM_ROTATE_90 | \
+                        DRM_ROTATE_180 | DRM_ROTATE_270)
+
+/** DRM_REFLECT_X
+ *
+ * Signals that a drm plane has been reflected in the X 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_REFLECT_X          BIT(4)
+
+/** DRM_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_REFLECT_Y          BIT(5)
+
+
+/** DRM_REFLECT_MASK
+ *
+ * Bitmask used to look for drm plane reflections.
+ */
+#define DRM_REFLECT_MASK (DRM_REFLECT_X | DRM_REFLECT_Y)
+
+
 #if defined(__cplusplus)
 }
 #endif