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