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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
