Ben Skeggs
2015-May-21 04:48 UTC
[Nouveau] [PATCH 1/2] drm/nouveau: add staging module option
On 20 May 2015 at 15:56, Alexandre Courbot <acourbot at nvidia.com> wrote:> Add a module option allowing to enable staging/unstable APIs. This will > allow us to experiment freely with experimental APIs for a while before > setting them in stone. > > Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> > --- > drm/nouveau/nouveau_drm.c | 18 ++++++++++++++++++ > drm/nouveau/uapi/drm/nouveau_drm.h | 3 +++ > 2 files changed, 21 insertions(+) > > diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c > index 89049335b738..e4bd6ed51e73 100644 > --- a/drm/nouveau/nouveau_drm.c > +++ b/drm/nouveau/nouveau_drm.c > @@ -75,6 +75,10 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1 > int nouveau_runtime_pm = -1; > module_param_named(runpm, nouveau_runtime_pm, int, 0400); > > +MODULE_PARM_DESC(staging, "enable staging APIs"); > +int nouveau_staging = 0; > +module_param_named(staging, nouveau_staging, int, 0400); > + > static struct drm_driver driver_stub; > static struct drm_driver driver_pci; > static struct drm_driver driver_platform; > @@ -895,6 +899,7 @@ nouveau_ioctls[] = { > DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_PREP, nouveau_gem_ioctl_cpu_prep, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), > DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), > + /* Staging ioctls */ > }; > > long > @@ -1027,6 +1032,7 @@ static void nouveau_display_options(void) > DRM_DEBUG_DRIVER("... runpm : %d\n", nouveau_runtime_pm); > DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf); > DRM_DEBUG_DRIVER("... pstate : %d\n", nouveau_pstate); > + DRM_DEBUG_DRIVER("... staging : %d\n", nouveau_staging); > } > > static const struct dev_pm_ops nouveau_pm_ops = { > @@ -1088,6 +1094,18 @@ err_free: > static int __init > nouveau_drm_init(void) > { > + /* Do not register staging ioctsl if option not specified */ > + if (!nouveau_staging) { > + unsigned i; > + > + /* This keeps us safe is no staging ioctls are defined */ > + i = min(driver_stub.num_ioctls, DRM_NOUVEAU_STAGING_IOCTL); > + while (!nouveau_ioctls[i - 1].func) > + i--; > + > + driver_stub.num_ioctls = i; > + }Hey Alex, I've got no specific objection. But I'm curious as to why you took this approach as opposed to just adding "if (!nouveau_staging) return -EINVAL;" directly in the experimental ioctls? I think, in line with what's been done in other places, having module options per-api is perhaps a better choice too. Ben.> + > driver_pci = driver_stub; > driver_pci.set_busid = drm_pci_set_busid; > driver_platform = driver_stub; > diff --git a/drm/nouveau/uapi/drm/nouveau_drm.h b/drm/nouveau/uapi/drm/nouveau_drm.h > index 5507eead5863..4e7e21f41b5c 100644 > --- a/drm/nouveau/uapi/drm/nouveau_drm.h > +++ b/drm/nouveau/uapi/drm/nouveau_drm.h > @@ -140,11 +140,14 @@ struct drm_nouveau_gem_cpu_fini { > #define DRM_NOUVEAU_GEM_CPU_PREP 0x42 > #define DRM_NOUVEAU_GEM_CPU_FINI 0x43 > #define DRM_NOUVEAU_GEM_INFO 0x44 > +/* range 0x98..DRM_COMMAND_END (8 entries) is reserved for staging, unstable ioctls */ > +#define DRM_NOUVEAU_STAGING_IOCTL 0x58 > > #define DRM_IOCTL_NOUVEAU_GEM_NEW DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_NEW, struct drm_nouveau_gem_new) > #define DRM_IOCTL_NOUVEAU_GEM_PUSHBUF DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_PUSHBUF, struct drm_nouveau_gem_pushbuf) > #define DRM_IOCTL_NOUVEAU_GEM_CPU_PREP DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_PREP, struct drm_nouveau_gem_cpu_prep) > #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini) > #define DRM_IOCTL_NOUVEAU_GEM_INFO DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info) > +/* staging ioctls */ > > #endif /* __NOUVEAU_DRM_H__ */ > -- > 2.4.0 > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Alexandre Courbot
2015-May-21 05:49 UTC
[Nouveau] [PATCH 1/2] drm/nouveau: add staging module option
On Thu, May 21, 2015 at 1:48 PM, Ben Skeggs <skeggsb at gmail.com> wrote:> On 20 May 2015 at 15:56, Alexandre Courbot <acourbot at nvidia.com> wrote: >> Add a module option allowing to enable staging/unstable APIs. This will >> allow us to experiment freely with experimental APIs for a while before >> setting them in stone. >> >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >> --- >> drm/nouveau/nouveau_drm.c | 18 ++++++++++++++++++ >> drm/nouveau/uapi/drm/nouveau_drm.h | 3 +++ >> 2 files changed, 21 insertions(+) >> >> diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c >> index 89049335b738..e4bd6ed51e73 100644 >> --- a/drm/nouveau/nouveau_drm.c >> +++ b/drm/nouveau/nouveau_drm.c >> @@ -75,6 +75,10 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1 >> int nouveau_runtime_pm = -1; >> module_param_named(runpm, nouveau_runtime_pm, int, 0400); >> >> +MODULE_PARM_DESC(staging, "enable staging APIs"); >> +int nouveau_staging = 0; >> +module_param_named(staging, nouveau_staging, int, 0400); >> + >> static struct drm_driver driver_stub; >> static struct drm_driver driver_pci; >> static struct drm_driver driver_platform; >> @@ -895,6 +899,7 @@ nouveau_ioctls[] = { >> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_PREP, nouveau_gem_ioctl_cpu_prep, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >> + /* Staging ioctls */ >> }; >> >> long >> @@ -1027,6 +1032,7 @@ static void nouveau_display_options(void) >> DRM_DEBUG_DRIVER("... runpm : %d\n", nouveau_runtime_pm); >> DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf); >> DRM_DEBUG_DRIVER("... pstate : %d\n", nouveau_pstate); >> + DRM_DEBUG_DRIVER("... staging : %d\n", nouveau_staging); >> } >> >> static const struct dev_pm_ops nouveau_pm_ops = { >> @@ -1088,6 +1094,18 @@ err_free: >> static int __init >> nouveau_drm_init(void) >> { >> + /* Do not register staging ioctsl if option not specified */ >> + if (!nouveau_staging) { >> + unsigned i; >> + >> + /* This keeps us safe is no staging ioctls are defined */ >> + i = min(driver_stub.num_ioctls, DRM_NOUVEAU_STAGING_IOCTL); >> + while (!nouveau_ioctls[i - 1].func) >> + i--; >> + >> + driver_stub.num_ioctls = i; >> + } > Hey Alex, > > I've got no specific objection. But I'm curious as to why you took > this approach as opposed to just adding "if (!nouveau_staging) return > -EINVAL;" directly in the experimental ioctls?Mainly because we will likely forget to add this check (or to remove it) in some of the staging ioctls. The current solution doesn't require us to think about that - and the less things to think about, the better.> I think, in line with > what's been done in other places, having module options per-api is > perhaps a better choice too.Do you mean that each experimental ioctl should have its own enable option? I don't mind going that way if you think it is preferable. And in that case my comment above is void. But actually I wonder if having these experimental ioctls enabled as compile options (either individually or as a whole) would not be better. Some experimental ioctls may require code in staging (like the PUSHBUF_2 ioctl that I would like to submit next), and I don't think it is desirable to force extra code or kernel options (in this case, CONFIG_STAGING) to Nouveau users who will not make use of them. I remember that we concluded in favor or module options on IRC, but in the light of this, wouldn't a config option be a less intrusive choice?
Ben Skeggs
2015-May-21 05:55 UTC
[Nouveau] [PATCH 1/2] drm/nouveau: add staging module option
On 21 May 2015 at 15:49, Alexandre Courbot <gnurou at gmail.com> wrote:> On Thu, May 21, 2015 at 1:48 PM, Ben Skeggs <skeggsb at gmail.com> wrote: >> On 20 May 2015 at 15:56, Alexandre Courbot <acourbot at nvidia.com> wrote: >>> Add a module option allowing to enable staging/unstable APIs. This will >>> allow us to experiment freely with experimental APIs for a while before >>> setting them in stone. >>> >>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >>> --- >>> drm/nouveau/nouveau_drm.c | 18 ++++++++++++++++++ >>> drm/nouveau/uapi/drm/nouveau_drm.h | 3 +++ >>> 2 files changed, 21 insertions(+) >>> >>> diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c >>> index 89049335b738..e4bd6ed51e73 100644 >>> --- a/drm/nouveau/nouveau_drm.c >>> +++ b/drm/nouveau/nouveau_drm.c >>> @@ -75,6 +75,10 @@ MODULE_PARM_DESC(runpm, "disable (0), force enable (1), optimus only default (-1 >>> int nouveau_runtime_pm = -1; >>> module_param_named(runpm, nouveau_runtime_pm, int, 0400); >>> >>> +MODULE_PARM_DESC(staging, "enable staging APIs"); >>> +int nouveau_staging = 0; >>> +module_param_named(staging, nouveau_staging, int, 0400); >>> + >>> static struct drm_driver driver_stub; >>> static struct drm_driver driver_pci; >>> static struct drm_driver driver_platform; >>> @@ -895,6 +899,7 @@ nouveau_ioctls[] = { >>> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_PREP, nouveau_gem_ioctl_cpu_prep, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >>> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_CPU_FINI, nouveau_gem_ioctl_cpu_fini, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >>> DRM_IOCTL_DEF_DRV(NOUVEAU_GEM_INFO, nouveau_gem_ioctl_info, DRM_UNLOCKED|DRM_AUTH|DRM_RENDER_ALLOW), >>> + /* Staging ioctls */ >>> }; >>> >>> long >>> @@ -1027,6 +1032,7 @@ static void nouveau_display_options(void) >>> DRM_DEBUG_DRIVER("... runpm : %d\n", nouveau_runtime_pm); >>> DRM_DEBUG_DRIVER("... vram_pushbuf : %d\n", nouveau_vram_pushbuf); >>> DRM_DEBUG_DRIVER("... pstate : %d\n", nouveau_pstate); >>> + DRM_DEBUG_DRIVER("... staging : %d\n", nouveau_staging); >>> } >>> >>> static const struct dev_pm_ops nouveau_pm_ops = { >>> @@ -1088,6 +1094,18 @@ err_free: >>> static int __init >>> nouveau_drm_init(void) >>> { >>> + /* Do not register staging ioctsl if option not specified */ >>> + if (!nouveau_staging) { >>> + unsigned i; >>> + >>> + /* This keeps us safe is no staging ioctls are defined */ >>> + i = min(driver_stub.num_ioctls, DRM_NOUVEAU_STAGING_IOCTL); >>> + while (!nouveau_ioctls[i - 1].func) >>> + i--; >>> + >>> + driver_stub.num_ioctls = i; >>> + } >> Hey Alex, >> >> I've got no specific objection. But I'm curious as to why you took >> this approach as opposed to just adding "if (!nouveau_staging) return >> -EINVAL;" directly in the experimental ioctls? > > Mainly because we will likely forget to add this check (or to remove > it) in some of the staging ioctls. The current solution doesn't > require us to think about that - and the less things to think about, > the better. > >> I think, in line with >> what's been done in other places, having module options per-api is >> perhaps a better choice too. > > Do you mean that each experimental ioctl should have its own enable > option? I don't mind going that way if you think it is preferable. And > in that case my comment above is void.That would be more preferable I think, and obvious as to what exactly you're enabling.> > But actually I wonder if having these experimental ioctls enabled as > compile options (either individually or as a whole) would not be > better. Some experimental ioctls may require code in staging (like the > PUSHBUF_2 ioctl that I would like to submit next), and I don't think > it is desirable to force extra code or kernel options (in this case, > CONFIG_STAGING) to Nouveau users who will not make use of them. I > remember that we concluded in favor or module options on IRC, but in > the light of this, wouldn't a config option be a less intrusive > choice?Right, but the whole point of this is to encourage the ioctls to not live there for too long, and progress to fully supported interfaces. Ben.