(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@xxxxxxxxx> 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@xxxxxxxxxx>
Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
