[PATCH libdrm v2] Header: Add rotation property fields

[PATCH libdrm v2] Header: Add rotation property fields

[Robert Foss]

From: Sean Paul <seanpaul@chromium.org>

From drm_crtc.h, for use with the plane "rotation" property.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
Signed-off-by: Robert Foss <robert.foss@collabora.com>
Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
---
Changes since v1:
 - Added r-b

 include/drm/drm.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/drm/drm.h b/include/drm/drm.h
index 1e7a4bc7a505..656c90045161 100644
--- a/include/drm/drm.h
+++ b/include/drm/drm.h
@@ -74,6 +74,14 @@ extern "C" {
 #define _DRM_LOCK_IS_CONT(lock)	   ((lock) & _DRM_LOCK_CONT)
 #define _DRM_LOCKING_CONTEXT(lock) ((lock) & ~(_DRM_LOCK_HELD|_DRM_LOCK_CONT))
 
+/* Rotation property bits */
+#define DRM_ROTATE_0		0
+#define DRM_ROTATE_90		1
+#define DRM_ROTATE_180		2
+#define DRM_ROTATE_270		3
+#define DRM_REFLECT_X		4
+#define DRM_REFLECT_Y		5
+
 typedef unsigned int drm_context_t;
 typedef unsigned int drm_drawable_t;
 typedef unsigned int drm_magic_t;
-- 
2.11.0.453.g787f75f05

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Emil Velikov]

On 17 April 2017 at 21:13, Robert Foss <robert.foss@collabora.com> wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> From drm_crtc.h, for use with the plane "rotation" property.
>
Please see the include/drm/README.

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Emil Velikov]

On 18 April 2017 at 11:32, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 17 April 2017 at 21:13, Robert Foss <robert.foss@collabora.com> wrote:
>> From: Sean Paul <seanpaul@chromium.org>
>>
>> From drm_crtc.h, for use with the plane "rotation" property.
>>
> Please see the include/drm/README.
>
To elaborate a bit:
The headers in include/drm should be synced via make headers_install +
copy, as seen in git log and described in the README file.

Although for the properties here we seems to be in a pickle. These are
defined in drm_blend.h which is not exported as part of the ABI.
Yet the defines _are_ part of the ABI - see the lovely warning, which
was added when someone broke and ABI and hence userspace.

My suggestion - fix this in the kernel first.
 - Move the DRM_ROTATE* and DRM_REFLECT_* to an ABI header.
Having a slight preference about drm_mode.h but drm.h should also be fine.
 - Optional: skim through for other properties that should be moved to
the ABI headers.
 - Update libdrm as described in the README (please send patches if
the documentation is not clear)

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Kristian Høgsberg]

On Mon, Apr 17, 2017 at 1:13 PM, Robert Foss <robert.foss@collabora.com> wrote:
> From: Sean Paul <seanpaul@chromium.org>
>
> From drm_crtc.h, for use with the plane "rotation" property.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
> ---
> Changes since v1:
>  - Added r-b
>
>  include/drm/drm.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/include/drm/drm.h b/include/drm/drm.h
> index 1e7a4bc7a505..656c90045161 100644
> --- a/include/drm/drm.h
> +++ b/include/drm/drm.h
> @@ -74,6 +74,14 @@ extern "C" {
>  #define _DRM_LOCK_IS_CONT(lock)           ((lock) & _DRM_LOCK_CONT)
>  #define _DRM_LOCKING_CONTEXT(lock) ((lock) & ~(_DRM_LOCK_HELD|_DRM_LOCK_CONT))
>
> +/* Rotation property bits */
> +#define DRM_ROTATE_0           0
> +#define DRM_ROTATE_90          1
> +#define DRM_ROTATE_180         2
> +#define DRM_ROTATE_270         3
> +#define DRM_REFLECT_X          4
> +#define DRM_REFLECT_Y          5

As far as I understand the property mechanism, the numeric values
aren't actually ABI. The string names of the enum values are the ABI
and you're supposed to parse the enum info and discover the numerical
values. For example:
https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570

Kristian

> +
>  typedef unsigned int drm_context_t;
>  typedef unsigned int drm_drawable_t;
>  typedef unsigned int drm_magic_t;
> --
> 2.11.0.453.g787f75f05
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Emil Velikov]

On 18 April 2017 at 18:38, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
> On Mon, Apr 17, 2017 at 1:13 PM, Robert Foss <robert.foss@collabora.com> wrote:
>> From: Sean Paul <seanpaul@chromium.org>
>>
>> From drm_crtc.h, for use with the plane "rotation" property.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
>> ---
>> Changes since v1:
>>  - Added r-b
>>
>>  include/drm/drm.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/include/drm/drm.h b/include/drm/drm.h
>> index 1e7a4bc7a505..656c90045161 100644
>> --- a/include/drm/drm.h
>> +++ b/include/drm/drm.h
>> @@ -74,6 +74,14 @@ extern "C" {
>>  #define _DRM_LOCK_IS_CONT(lock)           ((lock) & _DRM_LOCK_CONT)
>>  #define _DRM_LOCKING_CONTEXT(lock) ((lock) & ~(_DRM_LOCK_HELD|_DRM_LOCK_CONT))
>>
>> +/* Rotation property bits */
>> +#define DRM_ROTATE_0           0
>> +#define DRM_ROTATE_90          1
>> +#define DRM_ROTATE_180         2
>> +#define DRM_ROTATE_270         3
>> +#define DRM_REFLECT_X          4
>> +#define DRM_REFLECT_Y          5
>
> As far as I understand the property mechanism, the numeric values
> aren't actually ABI. The string names of the enum values are the ABI
> and you're supposed to parse the enum info and discover the numerical
> values. For example:
> https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570
>
Note sure I agree, yet then again my kernel experience is less than yours.
Do check the following commit and feel free to search the ML thread
that inspired it.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/drm/drm_blend.h?id=226714dc7c6af6d0acee449eb2afce08d128edad

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Kristian Høgsberg]

On Tue, Apr 18, 2017 at 11:03 AM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 18 April 2017 at 18:38, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
>> On Mon, Apr 17, 2017 at 1:13 PM, Robert Foss <robert.foss@collabora.com> wrote:
>>> From: Sean Paul <seanpaul@chromium.org>
>>>
>>> From drm_crtc.h, for use with the plane "rotation" property.
>>>
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>> Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
>>> ---
>>> Changes since v1:
>>>  - Added r-b
>>>
>>>  include/drm/drm.h | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/include/drm/drm.h b/include/drm/drm.h
>>> index 1e7a4bc7a505..656c90045161 100644
>>> --- a/include/drm/drm.h
>>> +++ b/include/drm/drm.h
>>> @@ -74,6 +74,14 @@ extern "C" {
>>>  #define _DRM_LOCK_IS_CONT(lock)           ((lock) & _DRM_LOCK_CONT)
>>>  #define _DRM_LOCKING_CONTEXT(lock) ((lock) & ~(_DRM_LOCK_HELD|_DRM_LOCK_CONT))
>>>
>>> +/* Rotation property bits */
>>> +#define DRM_ROTATE_0           0
>>> +#define DRM_ROTATE_90          1
>>> +#define DRM_ROTATE_180         2
>>> +#define DRM_ROTATE_270         3
>>> +#define DRM_REFLECT_X          4
>>> +#define DRM_REFLECT_Y          5
>>
>> As far as I understand the property mechanism, the numeric values
>> aren't actually ABI. The string names of the enum values are the ABI
>> and you're supposed to parse the enum info and discover the numerical
>> values. For example:
>> https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570
>>
> Note sure I agree, yet then again my kernel experience is less than yours.
> Do check the following commit and feel free to search the ML thread
> that inspired it.

I haven't worked much with the property mechanism tbh, but I know
Daniel (Stone) jumped through all those hoops to avoid hard-coding the
enum values.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/drm/drm_blend.h?id=226714dc7c6af6d0acee449eb2afce08d128edad
>
> Thanks
> Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Daniel Vetter]

On Tue, Apr 18, 2017 at 8:33 PM, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
>>> As far as I understand the property mechanism, the numeric values
>>> aren't actually ABI. The string names of the enum values are the ABI
>>> and you're supposed to parse the enum info and discover the numerical
>>> values. For example:
>>> https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570
>>>
>> Note sure I agree, yet then again my kernel experience is less than yours.
>> Do check the following commit and feel free to search the ML thread
>> that inspired it.
>
> I haven't worked much with the property mechanism tbh, but I know
> Daniel (Stone) jumped through all those hoops to avoid hard-coding the
> enum values.

In theory, this is correct and is how it's supposed to be done. In
practice, for a bunch of properties at least, we deal with the reality
of userspace being lazy and assuming that the enum values match with
the encode they send over the wire and tears result if we ever chance
that. I still think that going through the strings should be the
better way, since it makes it much easier to add/remove certain enum
values, depending upon what the hw/driverr combo can pull off.

The story is different for the properties themselves, there you
definitely need to make the name->prop id lookup, and you also need to
do that mapping for each object separately (because the value range is
attached to the prop, for uabi reasons, but might need to be
per-object).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Emil Velikov]

On 18 April 2017 at 23:16, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Tue, Apr 18, 2017 at 8:33 PM, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
>>>> As far as I understand the property mechanism, the numeric values
>>>> aren't actually ABI. The string names of the enum values are the ABI
>>>> and you're supposed to parse the enum info and discover the numerical
>>>> values. For example:
>>>> https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570
>>>>
>>> Note sure I agree, yet then again my kernel experience is less than yours.
>>> Do check the following commit and feel free to search the ML thread
>>> that inspired it.
>>
>> I haven't worked much with the property mechanism tbh, but I know
>> Daniel (Stone) jumped through all those hoops to avoid hard-coding the
>> enum values.
>
> In theory, this is correct and is how it's supposed to be done. In
> practice, for a bunch of properties at least, we deal with the reality
> of userspace being lazy and assuming that the enum values match with
> the encode they send over the wire and tears result if we ever chance
> that. I still think that going through the strings should be the
> better way, since it makes it much easier to add/remove certain enum
> values, depending upon what the hw/driverr combo can pull off.
>
> The story is different for the properties themselves, there you
> definitely need to make the name->prop id lookup, and you also need to
> do that mapping for each object separately (because the value range is
> attached to the prop, for uabi reasons, but might need to be
> per-object).

Daniel, this that mean that we're OK with moving DRM_ROTATE* [+others]
to a ABI header?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2] Header: Add rotation property fields

[Daniel Vetter]

On Mon, Apr 24, 2017 at 01:51:39PM +0100, Emil Velikov wrote:
> On 18 April 2017 at 23:16, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > On Tue, Apr 18, 2017 at 8:33 PM, Kristian Høgsberg <hoegsberg@gmail.com> wrote:
> >>>> As far as I understand the property mechanism, the numeric values
> >>>> aren't actually ABI. The string names of the enum values are the ABI
> >>>> and you're supposed to parse the enum info and discover the numerical
> >>>> values. For example:
> >>>> https://git.collabora.com/cgit/user/daniels/weston.git/tree/libweston/compositor-drm.c?h=wip/2017-03/atomic-v10-WIP#n570
> >>>>
> >>> Note sure I agree, yet then again my kernel experience is less than yours.
> >>> Do check the following commit and feel free to search the ML thread
> >>> that inspired it.
> >>
> >> I haven't worked much with the property mechanism tbh, but I know
> >> Daniel (Stone) jumped through all those hoops to avoid hard-coding the
> >> enum values.
> >
> > In theory, this is correct and is how it's supposed to be done. In
> > practice, for a bunch of properties at least, we deal with the reality
> > of userspace being lazy and assuming that the enum values match with
> > the encode they send over the wire and tears result if we ever chance
> > that. I still think that going through the strings should be the
> > better way, since it makes it much easier to add/remove certain enum
> > values, depending upon what the hw/driverr combo can pull off.
> >
> > The story is different for the properties themselves, there you
> > definitely need to make the name->prop id lookup, and you also need to
> > do that mapping for each object separately (because the value range is
> > attached to the prop, for uabi reasons, but might need to be
> > per-object).
> 
> Daniel, this that mean that we're OK with moving DRM_ROTATE* [+others]
> to a ABI header?

With a big comment, but yeah this is special.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel