On 21 May 2015 at 16:04, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
> On Thu, May 21, 2015 at 2:55 PM, Ben Skeggs <skeggsb@xxxxxxxxx> wrote:
>> On 21 May 2015 at 15:49, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>> On Thu, May 21, 2015 at 1:48 PM, Ben Skeggs <skeggsb@xxxxxxxxx> wrote:
>>>> On 20 May 2015 at 15:56, Alexandre Courbot <acourbot@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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.
What is pushbuf2 doing that requires staging btw? You've linked me to
patches previously, but I missed that.
Ben.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
