(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 at 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 at vmware.com>
>> Signed-off-by: David Herrmann <dh.herrmann at 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
