Re: [PATCH 28/38] drm/i915: Create/destroy VM (ppGTT) for use with contexts

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-01-23 11:30:39)
> > +int i915_gem_vm_create_ioctl(struct drm_device *dev, void *data,
> > +                          struct drm_file *file)
> > +{
> > +     struct drm_i915_private *i915 = to_i915(dev);
> > +     struct drm_i915_gem_vm_create *args = data;
> > +     struct drm_i915_file_private *file_priv = file->driver_priv;
> > +     struct i915_hw_ppgtt *ppgtt;
> > +     int err;
> > +
> > +     if (!HAS_FULL_PPGTT(i915))
> > +             return -ENODEV;
> > +
> > +     if (args->flags)
> > +             return -EINVAL;
> > +
> > +     if (args->extensions)
> > +             return -EINVAL;
> > +
> > +     ppgtt = i915_ppgtt_create(i915, file_priv);
> > +     if (IS_ERR(ppgtt))
> > +             return PTR_ERR(ppgtt);
> > +
> > +     err = mutex_lock_interruptible(&file_priv->vm_lock);
> > +     if (err)
> > +             goto err_put;
> > +
> > +     err = idr_alloc(&file_priv->vm_idr, ppgtt, 0, 0, GFP_KERNEL);
> > +     mutex_unlock(&file_priv->vm_lock);
> > +     if (err < 0)
> > +             goto err_put;
> 
>         else if (GEM_WARN_ON(err == 0) {
>                 err = -EINVAL;
>                 goto err_put;
>         }
> 
> ?
> 
> Or a GEM_BUG_ON(err == 0) if you must. :)

Not our bug :) We told it never to return 0.

> > +
> > +     ppgtt->user_handle = err;
> > +     args->id = err;
> > +     return 0;
> > +
> > +err_put:
> > +     i915_ppgtt_put(ppgtt);
> > +     return err;
> > +}
> > +
> > +int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void *data,
> 
> Can we cheat and declare data as u32 * here and so avoid need for the 
> local, since there are only two dereferences?

Tempting, we need to cook up the macros to hide the function pointer
differences for the ioctl tables.

> > +                           struct drm_file *file)
> > +{
> > +     struct drm_i915_file_private *file_priv = file->driver_priv;
> > +     struct i915_hw_ppgtt *ppgtt;
> > +     int err;
> > +     u32 id;
> > +
> > +     id = *(u32 *)data;

Oh crikey, did I write this?

Remember, do it right the first time as I won't remember when I was
cutting corners.

> > +     if (!id)
> > +             return -ENOENT;
> > +
> > +     err = mutex_lock_interruptible(&file_priv->vm_lock);
> > +     if (err)
> > +             return err;
> > +
> > +     ppgtt = idr_remove(&file_priv->vm_idr, id);
> > +     if (ppgtt)
> 
> GEM_WARN_ON(id != ppgtt->user_handle) too much paranoia?

BUG_ON then!

> > +             ppgtt->user_handle = 0;
> > +
> > +     mutex_unlock(&file_priv->vm_lock);
> > +     if (!ppgtt)
> > +             return -ENOENT;
> > +
> > +     i915_ppgtt_put(ppgtt);
> > +     return 0;
> 
> Or end with simply:
> 
> i915_ppgtt_put(ppgtt);
> 
> return ppgtt ? 0 : -ENOENT;
> 
> ?

I feel a slight disappointment at the anticlimatic nature of this
function, leaving a gap, nay, a yearning, for more.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx