Re: [PATCH 08/16] drm/vmwgfx: implement mmap access managament

[Thomas Hellstrom <thellstrom@vmware.com>]

(CC'ing the proper people since I'm still on parental leave).

On 08/13/2013 11:44 PM, David Herrmann wrote:

Please see inline comments.

> Hi
>
> On Tue, Aug 13, 2013 at 9:38 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> Correctly allow and revoke buffer access on each open/close via the new
>> VMA offset manager.

I haven't yet had time to check the access policies of the new VMA 
offset manager, but anything that is identical or stricter than the 
current vmwgfx verify_access() would be fine. If it's stricter however, 
we need to double-check backwards user-space compatibility.

>>   We also need to make vmw_user_dmabuf_reference()
>> correctly increase the vma-allow counter, but it is unused so remove it
>> instead.
IIRC this function or a derivative thereof is used heavily in an 
upcoming version driver, so if possible, please add a corrected version 
rather than remove the (currently) unused code. This will trigger a 
merge error and the upcoming code can be more easily corrected.

>>
>> Cc: Thomas Hellstrom <thellstrom@vmware.com>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> Just as a hint, this patch would allow to remove the
> "->access_verify()" callback in vmwgfx. No other driver uses it,
> afaik. I will try to add this in v2.
>
> Regards
> David
>
>> ---
>>   drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 29 +++++++++++++++++------------
>>   1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>> index 0e67cf4..4d3f0ae 100644
>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
>> @@ -499,6 +499,12 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
>>          if (unlikely(ret != 0))
>>                  goto out_no_dmabuf;
>>
>> +       ret = drm_vma_node_allow(&dma_buf->base.vma_node, file_priv->filp);
>> +       if (ret) {
>> +               vmw_dmabuf_unreference(&dma_buf);
>> +               goto out_no_dmabuf;
>> +       }
>> +
>>          rep->handle = handle;
>>          rep->map_handle = drm_vma_node_offset_addr(&dma_buf->base.vma_node);
>>          rep->cur_gmr_id = handle;
>> @@ -517,7 +523,18 @@ int vmw_dmabuf_unref_ioctl(struct drm_device *dev, void *data,
>>   {
>>          struct drm_vmw_unref_dmabuf_arg *arg =
>>              (struct drm_vmw_unref_dmabuf_arg *)data;
>> +       struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
>> +       struct vmw_dma_buffer *dma_buf;
>> +       int ret;
>> +
>> +       ret = vmw_user_dmabuf_lookup(tfile, arg->handle, &dma_buf);
>> +       if (ret)
>> +               return -EINVAL;
>>
>> +       drm_vma_node_revoke(&dma_buf->base.vma_node, file_priv->filp);
>> +       vmw_dmabuf_unreference(&dma_buf);
>> +
>> +       /* FIXME: is this equivalent to vmw_dmabuf_unreference(dma_buf)? */

No. A ttm ref object is rather similar to a generic GEM object, only 
that it's generic in the sense that it is not restricted to buffers, and 
can make any desired object visible to user-space. So translated the 
below code removes a reference that the arg->handle holds on the "gem" 
object, potentially destroying the whole object in which the "gem" 
object is embedded.

>>          return ttm_ref_object_base_unref(vmw_fpriv(file_priv)->tfile,
>>                                           arg->handle,
>>                                           TTM_REF_USAGE);
>> @@ -551,18 +568,6 @@ int vmw_user_dmabuf_lookup(struct ttm_object_file *tfile,
>>          return 0;
>>   }
>>
>> -int vmw_user_dmabuf_reference(struct ttm_object_file *tfile,
>> -                             struct vmw_dma_buffer *dma_buf)
>> -{
>> -       struct vmw_user_dma_buffer *user_bo;
>> -
>> -       if (dma_buf->base.destroy != vmw_user_dmabuf_destroy)
>> -               return -EINVAL;
>> -
>> -       user_bo = container_of(dma_buf, struct vmw_user_dma_buffer, dma);
>> -       return ttm_ref_object_add(tfile, &user_bo->base, TTM_REF_USAGE, NULL);
>> -}
>> -
>>   /*
>>    * Stream management
>>    */
>> --
>> 1.8.3.4
>>

Otherwise looks OK to me.

Thanks,
Thomas