Re: [PATCH 08/11] drm: Enforce unlocked ioctl operation for kms driver ioctls

Hi

On Tue, Sep 8, 2015 at 1:56 PM, Daniel Vetter daniel.vetter@ffwll.ch 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@intel.com

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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel