Hi
On Tue, Sep 8, 2015 at 1:56 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> With the prep patches for i915 all kms drivers either have
> DRM_UNLOCKED on all their ioctls. Or the ioctl always directly returns
> with an invariant return value when in modeset mode. But that's only
> the case for i915 and radeon. The drm core ioctls are unfortunately
> too much a mess still to dare this.
>
> Follow-up patches will remove DRM_UNLOCKED from all kms drivers to
> prove that this is indeed the case.
>
> Also update the documentation.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
drm_setclientcap() should probably lock _something_. It's not very
crucial, but I think we should guarantee consistency when setting
multiple values. struct_mutex of the corresponding DRM device sounds
sufficient, though not very promising. But drm_file doesn't have any
suitable lock..
drm_setversion(): This definitely needs _some_ lock. DRM_MASTER is not
reliable (we never merged the master-reliability patches).
...skipping review of the other ioctls...
...re-reading patch description...
So this patch is just meant to drop DRM_UNLOCKED from driver-ioctls,
right? See below.
> ---
> Documentation/DocBook/drm.tmpl | 4 +++-
> drivers/gpu/drm/drm_ioctl.c | 6 +++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index cfb43203a6a7..55dc106686df 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -3747,7 +3747,9 @@ int num_ioctls;</synopsis>
> </para></listitem>
> <listitem><para>
> DRM_UNLOCKED - The ioctl handler will be called without locking
> - the DRM global mutex
> + the DRM global mutex. This is the enforced default for kms drivers
> + (i.e. using the DRIVER_MODESET flag) and hence shouldn't be used
> + any more for new drivers.
> </para></listitem>
> </itemizedlist>
> </para>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 75df8ea87cd7..a5a4aa89b1b4 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -728,6 +728,7 @@ long drm_ioctl(struct file *filp,
> }
>
> retcode = drm_ioctl_permit(ioctl->flags, file_priv);
> +
Weird new-line. I actually prefer the previous style, anyway.
> if (unlikely(retcode))
> goto err_i1;
>
> @@ -755,7 +756,10 @@ long drm_ioctl(struct file *filp,
> memset(kdata, 0, usize);
> }
>
> - if (ioctl->flags & DRM_UNLOCKED)
> + /* Enforce sane locking for kms driver ioctls. Core ioctls are
> + * too messy still. */
> + if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> + (ioctl->flags & DRM_UNLOCKED))
> retcode = func(dev, kdata, file_priv);
This looks.. weird. Now we *never* lock *anything* for MODESET
drivers? This contradicts your commit-message, which rather tells me
you want _driver_ ioctls of modeset drivers to never rely on
drm_global_mutex. Several ways to make that work (and I'd review it
gladly), but this looks.. weird.
Care to elaborate?
Thanks
David
> else {
> mutex_lock(&drm_global_mutex);
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
