| Message ID | 20190415194825.3786-1-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: turn syncobj timeline support off by default | expand |
On Tue, 16 Apr 2019 at 05:48, Lionel Landwerlin <lionel.g.landwerlin@intel.com> wrote: > > Unfortunately userspace users of this API cannot be publicly disclosed > yet so disable this stuff by default until all is revealed. This begs the question how userspace is meant to know we support these? Is there a CAP for it? if not why not? Dave.
On 15/04/2019 20:52, Dave Airlie wrote: > On Tue, 16 Apr 2019 at 05:48, Lionel Landwerlin > <lionel.g.landwerlin@intel.com> wrote: >> Unfortunately userspace users of this API cannot be publicly disclosed >> yet so disable this stuff by default until all is revealed. > This begs the question how userspace is meant to know we support these? > > Is there a CAP for it? if not why not? > > Dave. > This comes with a submission path, so in i915 I've added a CAP. AMD seems to have a versioned interface. -Lionel
On Tue, 16 Apr 2019 at 06:05, Lionel Landwerlin <lionel.g.landwerlin@intel.com> wrote: > > On 15/04/2019 20:52, Dave Airlie wrote: > > On Tue, 16 Apr 2019 at 05:48, Lionel Landwerlin > > <lionel.g.landwerlin@intel.com> wrote: > >> Unfortunately userspace users of this API cannot be publicly disclosed > >> yet so disable this stuff by default until all is revealed. > > This begs the question how userspace is meant to know we support these? > > > > Is there a CAP for it? if not why not? > > > > Dave. > > > This comes with a submission path, so in i915 I've added a CAP. > > AMD seems to have a versioned interface. I think we would do like we did for syncobjs, you have a generic cap that the driver controls. The versioned interface won't be useful if we decide to backport a fix for this. Dave. > > > -Lionel >
On Mon, Apr 15, 2019 at 08:48:25PM +0100, Lionel Landwerlin wrote: > Unfortunately userspace users of this API cannot be publicly disclosed > yet so disable this stuff by default until all is revealed. > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/Kconfig | 10 ++++++++++ > drivers/gpu/drm/drm_syncobj.c | 12 ++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 5e1bc630b885..5b002793f0e4 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -423,3 +423,13 @@ config DRM_PANEL_ORIENTATION_QUIRKS > config DRM_LIB_RANDOM > bool > default n > + > +config DRM_SYNCOBJ_TIMELINE > + bool "Enable syncobj timeline support" > + depends on DRM > + default n > + help > + Choose this option if you want to enable preliminary support > + for timeline syncobjs. > + > + If in doubt, say "N". > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index ea22b79a7170..dd1bdd287225 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -754,6 +754,9 @@ drm_syncobj_transfer_ioctl(struct drm_device *dev, void *data, > struct drm_syncobj_transfer *args = data; > int ret; > > + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) > + return -ENODEV; I think the latest rage for this stuff is a module_option_unsafe. Much easier for igts to set by default (we want to keep CI'ing this ofc). And _unsafe taints the kernel when you set it, so about equally good. -Daniel > + > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > return -ENODEV; > > @@ -1105,6 +1108,9 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data, > struct drm_syncobj **syncobjs; > int ret = 0; > > + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) > + return -ENODEV; > + > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > return -ENODEV; > > @@ -1209,6 +1215,9 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, > uint32_t i, j; > int ret; > > + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) > + return -ENODEV; > + > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > return -EOPNOTSUPP; > > @@ -1280,6 +1289,9 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, > uint32_t i; > int ret; > > + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) > + return -ENODEV; > + > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > return -ENODEV; > > -- > 2.21.0.392.gf8f6787159e >
On 15/04/2019 21:23, Dave Airlie wrote: > On Tue, 16 Apr 2019 at 06:05, Lionel Landwerlin > <lionel.g.landwerlin@intel.com> wrote: >> On 15/04/2019 20:52, Dave Airlie wrote: >>> On Tue, 16 Apr 2019 at 05:48, Lionel Landwerlin >>> <lionel.g.landwerlin@intel.com> wrote: >>>> Unfortunately userspace users of this API cannot be publicly disclosed >>>> yet so disable this stuff by default until all is revealed. >>> This begs the question how userspace is meant to know we support these? >>> >>> Is there a CAP for it? if not why not? >>> >>> Dave. >>> >> This comes with a submission path, so in i915 I've added a CAP. >> >> AMD seems to have a versioned interface. > I think we would do like we did for syncobjs, you have a generic cap > that the driver controls. > > The versioned interface won't be useful if we decide to backport a fix for this. > > Dave. Okay, but then it's not a global setting anymore :) Which is what was suggested on IRC. I'm fine with it regardless :) -Lionel > >> >> -Lionel >>
On Tue, Apr 16, 2019 at 11:51 AM Lionel Landwerlin <lionel.g.landwerlin@intel.com> wrote: > > On 15/04/2019 21:23, Dave Airlie wrote: > > On Tue, 16 Apr 2019 at 06:05, Lionel Landwerlin > > <lionel.g.landwerlin@intel.com> wrote: > >> On 15/04/2019 20:52, Dave Airlie wrote: > >>> On Tue, 16 Apr 2019 at 05:48, Lionel Landwerlin > >>> <lionel.g.landwerlin@intel.com> wrote: > >>>> Unfortunately userspace users of this API cannot be publicly disclosed > >>>> yet so disable this stuff by default until all is revealed. > >>> This begs the question how userspace is meant to know we support these? > >>> > >>> Is there a CAP for it? if not why not? > >>> > >>> Dave. > >>> > >> This comes with a submission path, so in i915 I've added a CAP. > >> > >> AMD seems to have a versioned interface. > > I think we would do like we did for syncobjs, you have a generic cap > > that the driver controls. > > > > The versioned interface won't be useful if we decide to backport a fix for this. I guess amdgpu could then backport the version bump. Also, this is why I don't like versioned uapi really. Either way we also need to hide the amdgpu version bump until we've finalized the uapi and khr pushed out the spec. > Okay, but then it's not a global setting anymore :) > > Which is what was suggested on IRC. > > > I'm fine with it regardless :) I think global is good enough. If either i915 or amdgpu screwed up the uapi enough that we can't backport the enabling patch, then maybe not a good idea to do for either. Ofc this assumes the uapi as merged is forward compatible already, i.e. new userspace has a way to figure out whether timeline sync are support or not for a given driver. -Daniel
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 5e1bc630b885..5b002793f0e4 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -423,3 +423,13 @@ config DRM_PANEL_ORIENTATION_QUIRKS config DRM_LIB_RANDOM bool default n + +config DRM_SYNCOBJ_TIMELINE + bool "Enable syncobj timeline support" + depends on DRM + default n + help + Choose this option if you want to enable preliminary support + for timeline syncobjs. + + If in doubt, say "N". diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index ea22b79a7170..dd1bdd287225 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -754,6 +754,9 @@ drm_syncobj_transfer_ioctl(struct drm_device *dev, void *data, struct drm_syncobj_transfer *args = data; int ret; + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) + return -ENODEV; + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -ENODEV; @@ -1105,6 +1108,9 @@ drm_syncobj_timeline_wait_ioctl(struct drm_device *dev, void *data, struct drm_syncobj **syncobjs; int ret = 0; + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) + return -ENODEV; + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -ENODEV; @@ -1209,6 +1215,9 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, uint32_t i, j; int ret; + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) + return -ENODEV; + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -EOPNOTSUPP; @@ -1280,6 +1289,9 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, uint32_t i; int ret; + if (!IS_ENABLED(CONFIG_DRM_SYNCOBJ_TIMELINE)) + return -ENODEV; + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) return -ENODEV;
Unfortunately userspace users of this API cannot be publicly disclosed yet so disable this stuff by default until all is revealed. Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/Kconfig | 10 ++++++++++ drivers/gpu/drm/drm_syncobj.c | 12 ++++++++++++ 2 files changed, 22 insertions(+)