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

[Tvrtko Ursulin]

On 23/01/2019 11:51, Chris Wilson wrote:
> 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.

Probably not then, I was hoping that we could get away with function 
declaration and definition not being exactly the same and that would be 
all that's needed. If not never mind.

Regards,

Tvrtko

>>> +                           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
>