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

(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