[PATCH -next 18/18] drm/vmwgfx: Make user resource lookups reference-free during validation

From: Thomas Hellstrom <thellstrom@vmware.com>
To: dri-devel@lists.freedesktop.org
Cc: Thomas Hellstrom <thellstrom@vmware.com>,
	linux-graphics-maintainer@vmware.com
Subject: [PATCH -next 18/18] drm/vmwgfx: Make user resource lookups reference-free during validation
Date: Wed, 26 Sep 2018 18:18:39 +0200	[thread overview]
Message-ID: <20180926161839.4549-18-thellstrom@vmware.com> (raw)
In-Reply-To: <20180926161839.4549-1-thellstrom@vmware.com>

Make the process of looking up a user resource and adding it to the
validation list reference-free unless when it's actually added to the
validation list where a single reference is taken.
This saves two locked atomic operations per command stream buffer object
handle lookup, unless there is a lookup cache hit.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h      |  18 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c  | 267 ++++++++++++++++---------------
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c |  35 ++++
 3 files changed, 187 insertions(+), 133 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index ebb2ae86d900..2283bfb97f81 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -337,8 +337,6 @@ struct vmw_ctx_validation_info;
  * @last_query_ctx: Last context that submitted a query
  * @needs_post_query_barrier: Whether a query barrier is needed after
  * command submission
- * @error_resource: Pointer to hold a reference to the resource causing
- * an error
  * @staged_bindings: Cached per-context binding tracker
  * @staged_bindings_inuse: Whether the cached per-context binding tracker
  * is in use
@@ -365,7 +363,6 @@ struct vmw_sw_context{
 	struct vmw_res_cache_entry res_cache[vmw_res_max];
 	struct vmw_resource *last_query_ctx;
 	bool needs_post_query_barrier;
-	struct vmw_resource *error_resource;
 	struct vmw_ctx_binding_state *staged_bindings;
 	bool staged_bindings_inuse;
 	struct list_head staged_cmd_res;
@@ -698,6 +695,12 @@ extern int vmw_user_resource_lookup_handle(
 	uint32_t handle,
 	const struct vmw_user_resource_conv *converter,
 	struct vmw_resource **p_res);
+extern struct vmw_resource *
+vmw_user_resource_noref_lookup_handle(struct vmw_private *dev_priv,
+				      struct ttm_object_file *tfile,
+				      uint32_t handle,
+				      const struct vmw_user_resource_conv *
+				      converter);
 extern int vmw_stream_claim_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file_priv);
 extern int vmw_stream_unref_ioctl(struct drm_device *dev, void *data,
@@ -716,6 +719,15 @@ extern int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob);
 extern void vmw_resource_evict_all(struct vmw_private *dev_priv);
 extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo);
 
+/**
+ * vmw_user_resource_noref_release - release a user resource pointer looked up
+ * without reference
+ */
+static inline void vmw_user_resource_noref_release(void)
+{
+	ttm_base_object_noref_release();
+}
+
 /**
  * Buffer object helper functions - vmwgfx_bo.c
  */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
index dfa2d19274d5..5a6b70ba137a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
@@ -230,16 +230,55 @@ static int vmw_cmd_ctx_first_setup(struct vmw_private *dev_priv,
 }
 
 /**
- * vmw_resource_val_add - Add a resource to the software context's
- * resource list if it's not already on it.
+ * vmw_execbuf_res_size - calculate extra size fore the resource validation
+ * node
+ * @dev_priv: Pointer to the device private struct.
+ * @res_type: The resource type.
  *
- * @sw_context: Pointer to the software context.
+ * Guest-backed contexts and DX contexts require extra size to store
+ * execbuf private information in the validation node. Typically the
+ * binding manager associated data structures.
+ *
+ * Returns: The extra size requirement based on resource type.
+ */
+static unsigned int vmw_execbuf_res_size(struct vmw_private *dev_priv,
+					 enum vmw_res_type res_type)
+{
+	return (res_type == vmw_res_dx_context ||
+		(res_type == vmw_res_context && dev_priv->has_mob)) ?
+		sizeof(struct vmw_ctx_validation_info) : 0;
+}
+
+/**
+ * vmw_execbuf_rcache_update - Update a resource-node cache entry
+ *
+ * @rcache: Pointer to the entry to update.
  * @res: Pointer to the resource.
- * @p_node On successful return points to a valid pointer to a
- * struct vmw_resource_val_node, if non-NULL on entry.
+ * @private: Pointer to the execbuf-private space in the resource
+ * validation node.
  */
-static int vmw_resource_val_add(struct vmw_sw_context *sw_context,
-				struct vmw_resource *res)
+static void vmw_execbuf_rcache_update(struct vmw_res_cache_entry *rcache,
+				      struct vmw_resource *res,
+				      void *private)
+{
+	rcache->res = res;
+	rcache->private = private;
+	rcache->valid = 1;
+	rcache->valid_handle = 0;
+}
+
+/**
+ * vmw_execbuf_res_noref_val_add - Add a resource described by an
+ * unreferenced rcu-protected pointer to the validation list.
+ * @sw_context: Pointer to the software context.
+ * @res: Unreferenced rcu-protected pointer to the resource.
+ *
+ * Returns: 0 on success. Negative error code on failure. Typical error
+ * codes are %-EINVAL on inconsistency and %-ESRCH if the resource was
+ * doomed.
+ */
+static int vmw_execbuf_res_noref_val_add(struct vmw_sw_context *sw_context,
+					 struct vmw_resource *res)
 {
 	struct vmw_private *dev_priv = res->dev_priv;
 	int ret;
@@ -247,18 +286,18 @@ static int vmw_resource_val_add(struct vmw_sw_context *sw_context,
 	struct vmw_res_cache_entry *rcache;
 	struct vmw_ctx_validation_info *ctx_info;
 	bool first_usage;
-	size_t priv_size;
+	unsigned int priv_size;
 
-	/*
-	 * If the resource is a context, set up structures to track
-	 * context bindings.
-	 */
-	priv_size = (res_type == vmw_res_dx_context ||
-		     (res_type == vmw_res_context && dev_priv->has_mob)) ?
-		sizeof(*ctx_info) : 0;
+	rcache = &sw_context->res_cache[res_type];
+	if (likely(rcache->valid && rcache->res == res)) {
+		vmw_user_resource_noref_release();
+		return 0;
+	}
 
+	priv_size = vmw_execbuf_res_size(dev_priv, res_type);
 	ret = vmw_validation_add_resource(sw_context->ctx, res, priv_size,
 					  (void **)&ctx_info, &first_usage);
+	vmw_user_resource_noref_release();
 	if (ret)
 		return ret;
 
@@ -269,14 +308,37 @@ static int vmw_resource_val_add(struct vmw_sw_context *sw_context,
 			return ret;
 	}
 
-	/* Cache info about the last added resource */
+	vmw_execbuf_rcache_update(rcache, res, ctx_info);
+	return 0;
+}
+
+/**
+ * vmw_execbuf_res_noctx_val_add - Add a non-context resource to the resource
+ * validation list if it's not already on it
+ * @sw_context: Pointer to the software context.
+ * @res: Pointer to the resource.
+ *
+ * Returns: Zero on success. Negative error code on failure.
+ */
+static int vmw_execbuf_res_noctx_val_add(struct vmw_sw_context *sw_context,
+					 struct vmw_resource *res)
+{
+	struct vmw_res_cache_entry *rcache;
+	enum vmw_res_type res_type = vmw_res_type(res);
+	void *ptr;
+	int ret;
+
 	rcache = &sw_context->res_cache[res_type];
-	rcache->res = res;
-	rcache->private = ctx_info;
-	rcache->valid = 1;
-	rcache->valid_handle = 0;
+	if (likely(rcache->valid && rcache->res == res))
+		return 0;
 
-	return ret;
+	ret = vmw_validation_add_resource(sw_context->ctx, res, 0, &ptr, NULL);
+	if (ret)
+		return ret;
+
+	vmw_execbuf_rcache_update(rcache, res, ptr);
+
+	return 0;
 }
 
 /**
@@ -297,11 +359,11 @@ static int vmw_view_res_val_add(struct vmw_sw_context *sw_context,
 	 * First add the resource the view is pointing to, otherwise
 	 * it may be swapped out when the view is validated.
 	 */
-	ret = vmw_resource_val_add(sw_context, vmw_view_srf(view));
+	ret = vmw_execbuf_res_noctx_val_add(sw_context, vmw_view_srf(view));
 	if (ret)
 		return ret;
 
-	return vmw_resource_val_add(sw_context, view);
+	return vmw_execbuf_res_noctx_val_add(sw_context, view);
 }
 
 /**
@@ -371,7 +433,7 @@ static int vmw_resource_context_res_add(struct vmw_private *dev_priv,
 			if (IS_ERR(res))
 				continue;
 
-			ret = vmw_resource_val_add(sw_context, res);
+			ret = vmw_execbuf_res_noctx_val_add(sw_context, res);
 			if (unlikely(ret != 0))
 				return ret;
 		}
@@ -383,16 +445,11 @@ static int vmw_resource_context_res_add(struct vmw_private *dev_priv,
 	binding_list = vmw_context_binding_list(ctx);
 
 	list_for_each_entry(entry, binding_list, ctx_list) {
-		/* entry->res is not refcounted */
-		res = vmw_resource_reference_unless_doomed(entry->res);
-		if (unlikely(res == NULL))
-			continue;
-
 		if (vmw_res_type(entry->res) == vmw_res_view)
 			ret = vmw_view_res_val_add(sw_context, entry->res);
 		else
-			ret = vmw_resource_val_add(sw_context, entry->res);
-		vmw_resource_unreference(&res);
+			ret = vmw_execbuf_res_noctx_val_add(sw_context,
+							    entry->res);
 		if (unlikely(ret != 0))
 			break;
 	}
@@ -534,38 +591,6 @@ static int vmw_resources_reserve(struct vmw_sw_context *sw_context)
 	return ret;
 }
 
-/**
- * vmw_cmd_res_reloc_add - Add a resource to a software context's
- * relocation- and validation lists.
- * @dev_priv: Pointer to a struct vmw_private identifying the device.
- * @sw_context: Pointer to the software context.
- * @id_loc: Pointer to where the id that needs translation is located.
- * @res: Valid pointer to a struct vmw_resource.
- *
- *  Return: Zero on success, negative error code on error
- */
-static int vmw_cmd_res_reloc_add(struct vmw_private *dev_priv,
-				 struct vmw_sw_context *sw_context,
-				 uint32_t *id_loc,
-				 struct vmw_resource *res)
-{
-	int ret;
-
-	ret = vmw_resource_relocation_add(sw_context, res,
-					  vmw_ptr_diff(sw_context->buf_start,
-						       id_loc),
-					  vmw_res_rel_normal);
-	if (unlikely(ret != 0))
-		return ret;
-
-	ret = vmw_resource_val_add(sw_context, res);
-	if (unlikely(ret != 0))
-		return ret;
-
-	return 0;
-}
-
-
 /**
  * vmw_cmd_res_check - Check that a resource is present and if so, put it
  * on the resource validate list unless it's already there.
@@ -587,8 +612,7 @@ vmw_cmd_res_check(struct vmw_private *dev_priv,
 		  uint32_t *id_loc,
 		  struct vmw_resource **p_res)
 {
-	struct vmw_res_cache_entry *rcache =
-		&sw_context->res_cache[res_type];
+	struct vmw_res_cache_entry *rcache = &sw_context->res_cache[res_type];
 	struct vmw_resource *res;
 	int ret;
 
@@ -603,56 +627,41 @@ vmw_cmd_res_check(struct vmw_private *dev_priv,
 		return 0;
 	}
 
-	/*
-	 * Fastpath in case of repeated commands referencing the same
-	 * resource
-	 */
-
 	if (likely(rcache->valid_handle && *id_loc == rcache->handle)) {
-		struct vmw_resource *res = rcache->res;
+		res = rcache->res;
+	} else {
+		unsigned int size = vmw_execbuf_res_size(dev_priv, res_type);
 
-		if (p_res)
-			*p_res = res;
+		ret = vmw_validation_preload_res(sw_context->ctx, size);
+		if (ret)
+			return ret;
 
-		return vmw_resource_relocation_add
-			(sw_context, res,
-			 vmw_ptr_diff(sw_context->buf_start, id_loc),
-			 vmw_res_rel_normal);
-	}
+		res = vmw_user_resource_noref_lookup_handle
+			(dev_priv, sw_context->fp->tfile, *id_loc, converter);
+		if (unlikely(IS_ERR(res))) {
+			DRM_ERROR("Could not find or use resource 0x%08x.\n",
+				  (unsigned int) *id_loc);
+			return PTR_ERR(res);
+		}
 
-	ret = vmw_user_resource_lookup_handle(dev_priv,
-					      sw_context->fp->tfile,
-					      *id_loc,
-					      converter,
-					      &res);
-	if (unlikely(ret != 0)) {
-		DRM_ERROR("Could not find or use resource 0x%08x.\n",
-			  (unsigned) *id_loc);
-		dump_stack();
-		return ret;
-	}
+		ret = vmw_execbuf_res_noref_val_add(sw_context, res);
+		if (unlikely(ret != 0))
+			return ret;
 
-	ret = vmw_cmd_res_reloc_add(dev_priv, sw_context, id_loc,
-				    res);
-	if (unlikely(ret != 0))
-		goto out_no_reloc;
+		if (rcache->valid && rcache->res == res) {
+			rcache->valid_handle = true;
+			rcache->handle = *id_loc;
+		}
+	}
 
+	ret = vmw_resource_relocation_add(sw_context, res,
+					  vmw_ptr_diff(sw_context->buf_start,
+						       id_loc),
+					  vmw_res_rel_normal);
 	if (p_res)
 		*p_res = res;
 
-	if (rcache->valid && rcache->res == res) {
-		rcache->valid_handle = true;
-		rcache->handle = *id_loc;
-	}
-
-	vmw_resource_unreference(&res);
 	return 0;
-
-out_no_reloc:
-	BUG_ON(sw_context->error_resource != NULL);
-	sw_context->error_resource = res;
-
-	return ret;
 }
 
 /**
@@ -854,9 +863,9 @@ static int vmw_cmd_set_render_target_check(struct vmw_private *dev_priv,
 		return ret;
 
 	ret = vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
-				user_surface_converter,
-				&cmd->body.target.sid, &res);
-	if (unlikely(ret != 0))
+				user_surface_converter, &cmd->body.target.sid,
+				&res);
+	if (unlikely(ret))
 		return ret;
 
 	if (dev_priv->has_mob) {
@@ -890,8 +899,8 @@ static int vmw_cmd_surface_copy_check(struct vmw_private *dev_priv,
 	cmd = container_of(header, struct vmw_sid_cmd, header);
 
 	ret = vmw_cmd_res_check(dev_priv, sw_context, vmw_res_surface,
-				user_surface_converter,
-				&cmd->body.src.sid, NULL);
+					  user_surface_converter,
+					  &cmd->body.src.sid, NULL);
 	if (ret)
 		return ret;
 
@@ -2127,8 +2136,7 @@ static int vmw_cmd_set_shader(struct vmw_private *dev_priv,
 					cmd->body.type);
 
 		if (!IS_ERR(res)) {
-			ret = vmw_cmd_res_reloc_add(dev_priv, sw_context,
-						    &cmd->body.shid, res);
+			ret = vmw_execbuf_res_noctx_val_add(sw_context, res);
 			if (unlikely(ret != 0))
 				return ret;
 		}
@@ -2345,7 +2353,7 @@ static int vmw_cmd_dx_set_shader(struct vmw_private *dev_priv,
 			return PTR_ERR(res);
 		}
 
-		ret = vmw_resource_val_add(sw_context, res);
+		ret = vmw_execbuf_res_noctx_val_add(sw_context, res);
 		if (ret)
 			return ret;
 	}
@@ -2883,13 +2891,12 @@ static int vmw_cmd_dx_bind_shader(struct vmw_private *dev_priv,
 		return PTR_ERR(res);
 	}
 
-	ret = vmw_resource_val_add(sw_context, res);
+	ret = vmw_execbuf_res_noctx_val_add(sw_context, res);
 	if (ret) {
 		DRM_ERROR("Error creating resource validation node.\n");
 		return ret;
 	}
 
-
 	return vmw_cmd_res_switch_backup(dev_priv, sw_context, res,
 					 &cmd->body.mobid,
 					 cmd->body.offsetInBytes);
@@ -3781,28 +3788,33 @@ static int vmw_execbuf_tie_context(struct vmw_private *dev_priv,
 {
 	struct vmw_resource *res;
 	int ret;
+	unsigned int size;
 
 	if (handle == SVGA3D_INVALID_ID)
 		return 0;
 
-	ret = vmw_user_resource_lookup_handle(dev_priv, sw_context->fp->tfile,
-					      handle, user_context_converter,
-					      &res);
-	if (unlikely(ret != 0)) {
+	size = vmw_execbuf_res_size(dev_priv, vmw_res_dx_context);
+	ret = vmw_validation_preload_res(sw_context->ctx, size);
+	if (ret)
+		return ret;
+
+	res = vmw_user_resource_noref_lookup_handle
+		(dev_priv, sw_context->fp->tfile, handle,
+		 user_context_converter);
+	if (unlikely(IS_ERR(res))) {
 		DRM_ERROR("Could not find or user DX context 0x%08x.\n",
 			  (unsigned) handle);
-		return ret;
+		return PTR_ERR(res);
 	}
 
-	ret = vmw_resource_val_add(sw_context, res);
+	ret = vmw_execbuf_res_noref_val_add(sw_context, res);
 	if (unlikely(ret != 0))
-		goto out_err;
+		return ret;
 
 	sw_context->dx_ctx_node = vmw_execbuf_info_from_res(sw_context, res);
 	sw_context->man = vmw_context_res_man(res);
-out_err:
-	vmw_resource_unreference(&res);
-	return ret;
+
+	return 0;
 }
 
 int vmw_execbuf_process(struct drm_file *file_priv,
@@ -3818,7 +3830,6 @@ int vmw_execbuf_process(struct drm_file *file_priv,
 {
 	struct vmw_sw_context *sw_context = &dev_priv->ctx;
 	struct vmw_fence_obj *fence = NULL;
-	struct vmw_resource *error_resource;
 	struct vmw_cmdbuf_header *header;
 	uint32_t handle;
 	int ret;
@@ -4028,8 +4039,6 @@ int vmw_execbuf_process(struct drm_file *file_priv,
 		     !dev_priv->query_cid_valid))
 		__vmw_execbuf_release_pinned_bo(dev_priv, NULL);
 out_unlock:
-	error_resource = sw_context->error_resource;
-	sw_context->error_resource = NULL;
 	vmw_cmdbuf_res_revert(&sw_context->staged_cmd_res);
 	vmw_validation_drop_ht(&val_ctx);
 	WARN_ON(!list_empty(&sw_context->ctx_list));
@@ -4040,8 +4049,6 @@ int vmw_execbuf_process(struct drm_file *file_priv,
 	 * avoid deadlocks in resource destruction paths.
 	 */
 	vmw_validation_unref_lists(&val_ctx);
-	if (unlikely(error_resource != NULL))
-		vmw_resource_unreference(&error_resource);
 out_free_header:
 	if (header)
 		vmw_cmdbuf_header_free(header);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index cf48d0b157f6..8a029bade32a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -230,6 +230,41 @@ int vmw_user_resource_lookup_handle(struct vmw_private *dev_priv,
 	return ret;
 }
 
+/**
+ * vmw_user_resource_lookup_handle - lookup a struct resource from a
+ * TTM user-space handle and perform basic type checks
+ *
+ * @dev_priv:     Pointer to a device private struct
+ * @tfile:        Pointer to a struct ttm_object_file identifying the caller
+ * @handle:       The TTM user-space handle
+ * @converter:    Pointer to an object describing the resource type
+ * @p_res:        On successful return the location pointed to will contain
+ *                a pointer to a refcounted struct vmw_resource.
+ *
+ * If the handle can't be found or is associated with an incorrect resource
+ * type, -EINVAL will be returned.
+ */
+struct vmw_resource *
+vmw_user_resource_noref_lookup_handle(struct vmw_private *dev_priv,
+				      struct ttm_object_file *tfile,
+				      uint32_t handle,
+				      const struct vmw_user_resource_conv
+				      *converter)
+{
+	struct ttm_base_object *base;
+
+	base = ttm_base_object_noref_lookup(tfile, handle);
+	if (!base)
+		return ERR_PTR(-ESRCH);
+
+	if (unlikely(ttm_base_object_type(base) != converter->object_type)) {
+		ttm_base_object_noref_release();
+		return ERR_PTR(-EINVAL);
+	}
+
+	return converter->base_obj_to_res(base);
+}
+
 /**
  * Helper function that looks either a surface or bo.
  *
-- 
2.14.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
