[Nouveau] [PATCH 1/2] drm/nouveau: add staging module option

[Alexandre Courbot]

On Thu, May 21, 2015 at 5:35 PM, Ben Skeggs <skeggsb at gmail.com> wrote:
> On 21 May 2015 at 16:04, Alexandre Courbot <gnurou at gmail.com> wrote:
>> On Thu, May 21, 2015 at 2:55 PM, Ben Skeggs <skeggsb at gmail.com> wrote:
>>> 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.
>>
>> Definitely, but my concern is that doing this will make Nouveau depend
>> on STAGING for at least short periods of time. Do we really want this?
> I admit to having slightly misread your last paragraph.  For cases
> such as thas, a config option that depends on STAGING *and* the kernel
> parameter should be used.

Sounds good!

> What is pushbuf2 doing that requires staging btw?  You've linked me to
> patches previously, but I missed that.

It depends on the Android sync framework. Actually we also have a
patch that makes a change in the Android sync code that pushbuf2
depends on, and without pushbuf2 it does not make much sense by itself
(http://www.spinics.net/lists/linux-driver-devel/msg63832.html ), so I
will send the pushbuf2 patches along with it.