| Message ID | 20170514172637.28937-1-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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 >
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
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(-)