[PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

TTM provides a means to assign eviction priorities to buffer object. This
means that all buffer objects with a lower priority will be evicted first
on memory pressure.
Use this to make sure surfaces and in particular non-dirty surfaces are
evicted first. Evicting in particular shaders, cotables and contexts imply
a significant performance hit on vmwgfx, so make sure these resources are
evicted last.
Some buffer objects are sub-allocated in user-space which means we can have
many resources attached to a single buffer object or resource. In that case
the buffer object is given the highest priority of the attached resources.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_bo.c            |  2 +
 drivers/gpu/drm/vmwgfx/vmwgfx_context.c       |  4 ++
 drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c       | 13 ++--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h           | 72 +++++++++++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c      | 56 +++++++++++----
 drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h |  2 +
 drivers/gpu/drm/vmwgfx/vmwgfx_shader.c        |  8 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c       |  4 ++
 8 files changed, 141 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 5d5c2bce01f3..c0829d50eecc 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -510,6 +510,8 @@ int vmw_bo_init(struct vmw_private *dev_priv,
 
 	acc_size = vmw_bo_acc_size(dev_priv, size, user);
 	memset(vmw_bo, 0, sizeof(*vmw_bo));
+	BUILD_BUG_ON(TTM_MAX_BO_PRIORITY <= 3);
+	vmw_bo->base.priority = 3;
 
 	INIT_LIST_HEAD(&vmw_bo->res_list);
 
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
index 63f111068a44..a56c9d802382 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_context.c
@@ -88,6 +88,8 @@ static const struct vmw_res_func vmw_gb_context_func = {
 	.res_type = vmw_res_context,
 	.needs_backup = true,
 	.may_evict = true,
+	.prio = 3,
+	.dirty_prio = 3,
 	.type_name = "guest backed contexts",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_gb_context_create,
@@ -100,6 +102,8 @@ static const struct vmw_res_func vmw_dx_context_func = {
 	.res_type = vmw_res_dx_context,
 	.needs_backup = true,
 	.may_evict = true,
+	.prio = 3,
+	.dirty_prio = 3,
 	.type_name = "dx contexts",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_dx_context_create,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
index b4f6e1217c9d..8c699cb2565b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
@@ -116,6 +116,8 @@ static const struct vmw_res_func vmw_cotable_func = {
 	.res_type = vmw_res_cotable,
 	.needs_backup = true,
 	.may_evict = true,
+	.prio = 3,
+	.dirty_prio = 3,
 	.type_name = "context guest backed object tables",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_cotable_create,
@@ -307,7 +309,7 @@ static int vmw_cotable_unbind(struct vmw_resource *res,
 	struct ttm_buffer_object *bo = val_buf->bo;
 	struct vmw_fence_obj *fence;
 
-	if (list_empty(&res->mob_head))
+	if (!vmw_resource_mob_attached(res))
 		return 0;
 
 	WARN_ON_ONCE(bo->mem.mem_type != VMW_PL_MOB);
@@ -453,6 +455,7 @@ static int vmw_cotable_resize(struct vmw_resource *res, size_t new_size)
 		goto out_wait;
 	}
 
+	vmw_resource_mob_detach(res);
 	res->backup = buf;
 	res->backup_size = new_size;
 	vcotbl->size_read_back = cur_size_read_back;
@@ -467,12 +470,12 @@ static int vmw_cotable_resize(struct vmw_resource *res, size_t new_size)
 		res->backup = old_buf;
 		res->backup_size = old_size;
 		vcotbl->size_read_back = old_size_read_back;
+		vmw_resource_mob_attach(res);
 		goto out_wait;
 	}
 
+	vmw_resource_mob_attach(res);
 	/* Let go of the old mob. */
-	list_del(&res->mob_head);
-	list_add_tail(&res->mob_head, &buf->res_list);
 	vmw_bo_unreference(&old_buf);
 	res->id = vcotbl->type;
 
@@ -496,7 +499,7 @@ static int vmw_cotable_resize(struct vmw_resource *res, size_t new_size)
  * is called before bind() in the validation sequence is instead used for two
  * things.
  * 1) Unscrub the cotable if it is scrubbed and still attached to a backup
- *    buffer, that is, if @res->mob_head is non-empty.
+ *    buffer.
  * 2) Resize the cotable if needed.
  */
 static int vmw_cotable_create(struct vmw_resource *res)
@@ -512,7 +515,7 @@ static int vmw_cotable_create(struct vmw_resource *res)
 		new_size *= 2;
 
 	if (likely(new_size <= res->backup_size)) {
-		if (vcotbl->scrubbed && !list_empty(&res->mob_head)) {
+		if (vcotbl->scrubbed && vmw_resource_mob_attached(res)) {
 			ret = vmw_cotable_unscrub(res);
 			if (ret)
 				return ret;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 366dcfc1f9bb..7c935c72d368 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -86,6 +86,15 @@ struct vmw_fpriv {
 	bool gb_aware; /* user-space is guest-backed aware */
 };
 
+/**
+ * struct vmw_buffer_object - TTM buffer object with vmwgfx additions
+ * @base: The TTM buffer object
+ * @res_list: List of resources using this buffer object as a backing MOB
+ * @pin_count: pin depth
+ * @dx_query_ctx: DX context if this buffer object is used as a DX query MOB
+ * @map: Kmap object for semi-persistent mappings
+ * @res_prios: Eviction priority counts for attached resources
+ */
 struct vmw_buffer_object {
 	struct ttm_buffer_object base;
 	struct list_head res_list;
@@ -94,6 +103,7 @@ struct vmw_buffer_object {
 	struct vmw_resource *dx_query_ctx;
 	/* Protected by reservation */
 	struct ttm_bo_kmap_obj map;
+	u32 res_prios[TTM_MAX_BO_PRIORITY];
 };
 
 /**
@@ -145,6 +155,7 @@ struct vmw_resource {
 	struct kref kref;
 	struct vmw_private *dev_priv;
 	int id;
+	u32 used_prio;
 	unsigned long backup_size;
 	bool res_dirty;
 	bool backup_dirty;
@@ -709,6 +720,19 @@ extern void vmw_query_move_notify(struct ttm_buffer_object *bo,
 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);
+void vmw_resource_mob_attach(struct vmw_resource *res);
+void vmw_resource_mob_detach(struct vmw_resource *res);
+
+/**
+ * vmw_resource_mob_attached - Whether a resource currently has a mob attached
+ * @res: The resource
+ *
+ * Return: true if the resource has a mob attached, false otherwise.
+ */
+static inline bool vmw_resource_mob_attached(const struct vmw_resource *res)
+{
+	return !list_empty(&res->mob_head);
+}
 
 /**
  * vmw_user_resource_noref_release - release a user resource pointer looked up
@@ -787,6 +811,54 @@ static inline void vmw_user_bo_noref_release(void)
 	ttm_base_object_noref_release();
 }
 
+/**
+ * vmw_bo_adjust_prio - Adjust the buffer object eviction priority
+ * according to attached resources
+ * @vbo: The struct vmw_buffer_object
+ */
+static inline void vmw_bo_prio_adjust(struct vmw_buffer_object *vbo)
+{
+	int i = ARRAY_SIZE(vbo->res_prios);
+
+	while (i--) {
+		if (vbo->res_prios[i]) {
+			vbo->base.priority = i;
+			return;
+		}
+	}
+
+	vbo->base.priority = 3;
+}
+
+/**
+ * vmw_bo_prio_add - Notify a buffer object of a newly attached resource
+ * eviction priority
+ * @vbo: The struct vmw_buffer_object
+ * @prio: The resource priority
+ *
+ * After being notified, the code assigns the highest resource eviction priority
+ * to the backing buffer object (mob).
+ */
+static inline void vmw_bo_prio_add(struct vmw_buffer_object *vbo, int prio)
+{
+	if (vbo->res_prios[prio]++ == 0)
+		vmw_bo_prio_adjust(vbo);
+}
+
+/**
+ * vmw_bo_prio_del - Notify a buffer object of a resource with a certain
+ * priority being removed
+ * @vbo: The struct vmw_buffer_object
+ * @prio: The resource priority
+ *
+ * After being notified, the code assigns the highest resource eviction priority
+ * to the backing buffer object (mob).
+ */
+static inline void vmw_bo_prio_del(struct vmw_buffer_object *vbo, int prio)
+{
+	if (--vbo->res_prios[prio] == 0)
+		vmw_bo_prio_adjust(vbo);
+}
 
 /**
  * Misc Ioctl functionality - vmwgfx_ioctl.c
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index 1d38a8b2f2ec..be7d4149a129 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -34,6 +34,37 @@
 
 #define VMW_RES_EVICT_ERR_COUNT 10
 
+/**
+ * vmw_resource_mob_attach - Mark a resource as attached to its backing mob
+ * @res: The resource
+ */
+void vmw_resource_mob_attach(struct vmw_resource *res)
+{
+	struct vmw_buffer_object *backup = res->backup;
+
+	lockdep_assert_held(&backup->base.resv->lock.base);
+	res->used_prio = (res->res_dirty) ? res->func->dirty_prio :
+		res->func->prio;
+	list_add_tail(&res->mob_head, &backup->res_list);
+	vmw_bo_prio_add(backup, res->used_prio);
+}
+
+/**
+ * vmw_resource_mob_detach - Mark a resource as detached from its backing mob
+ * @res: The resource
+ */
+void vmw_resource_mob_detach(struct vmw_resource *res)
+{
+	struct vmw_buffer_object *backup = res->backup;
+
+	lockdep_assert_held(&backup->base.resv->lock.base);
+	if (vmw_resource_mob_attached(res)) {
+		list_del_init(&res->mob_head);
+		vmw_bo_prio_del(backup, res->used_prio);
+	}
+}
+
+
 struct vmw_resource *vmw_resource_reference(struct vmw_resource *res)
 {
 	kref_get(&res->kref);
@@ -80,7 +111,7 @@ static void vmw_resource_release(struct kref *kref)
 		struct ttm_buffer_object *bo = &res->backup->base;
 
 		ttm_bo_reserve(bo, false, false, NULL);
-		if (!list_empty(&res->mob_head) &&
+		if (vmw_resource_mob_attached(res) &&
 		    res->func->unbind != NULL) {
 			struct ttm_validate_buffer val_buf;
 
@@ -89,7 +120,7 @@ static void vmw_resource_release(struct kref *kref)
 			res->func->unbind(res, false, &val_buf);
 		}
 		res->backup_dirty = false;
-		list_del_init(&res->mob_head);
+		vmw_resource_mob_detach(res);
 		ttm_bo_unreserve(bo);
 		vmw_bo_unreference(&res->backup);
 	}
@@ -179,6 +210,7 @@ int vmw_resource_init(struct vmw_private *dev_priv, struct vmw_resource *res,
 	res->backup_offset = 0;
 	res->backup_dirty = false;
 	res->res_dirty = false;
+	res->used_prio = 3;
 	if (delay_id)
 		return 0;
 	else
@@ -355,14 +387,14 @@ static int vmw_resource_do_validate(struct vmw_resource *res,
 	}
 
 	if (func->bind &&
-	    ((func->needs_backup && list_empty(&res->mob_head) &&
+	    ((func->needs_backup && !vmw_resource_mob_attached(res) &&
 	      val_buf->bo != NULL) ||
 	     (!func->needs_backup && val_buf->bo != NULL))) {
 		ret = func->bind(res, val_buf);
 		if (unlikely(ret != 0))
 			goto out_bind_failed;
 		if (func->needs_backup)
-			list_add_tail(&res->mob_head, &res->backup->res_list);
+			vmw_resource_mob_attach(res);
 	}
 
 	return 0;
@@ -402,15 +434,13 @@ void vmw_resource_unreserve(struct vmw_resource *res,
 
 	if (switch_backup && new_backup != res->backup) {
 		if (res->backup) {
-			lockdep_assert_held(&res->backup->base.resv->lock.base);
-			list_del_init(&res->mob_head);
+			vmw_resource_mob_detach(res);
 			vmw_bo_unreference(&res->backup);
 		}
 
 		if (new_backup) {
 			res->backup = vmw_bo_reference(new_backup);
-			lockdep_assert_held(&new_backup->base.resv->lock.base);
-			list_add_tail(&res->mob_head, &new_backup->res_list);
+			vmw_resource_mob_attach(res);
 		} else {
 			res->backup = NULL;
 		}
@@ -469,7 +499,7 @@ vmw_resource_check_buffer(struct ww_acquire_ctx *ticket,
 	if (unlikely(ret != 0))
 		goto out_no_reserve;
 
-	if (res->func->needs_backup && list_empty(&res->mob_head))
+	if (res->func->needs_backup && !vmw_resource_mob_attached(res))
 		return 0;
 
 	backup_dirty = res->backup_dirty;
@@ -574,11 +604,11 @@ static int vmw_resource_do_evict(struct ww_acquire_ctx *ticket,
 		return ret;
 
 	if (unlikely(func->unbind != NULL &&
-		     (!func->needs_backup || !list_empty(&res->mob_head)))) {
+		     (!func->needs_backup || vmw_resource_mob_attached(res)))) {
 		ret = func->unbind(res, res->res_dirty, &val_buf);
 		if (unlikely(ret != 0))
 			goto out_no_unbind;
-		list_del_init(&res->mob_head);
+		vmw_resource_mob_detach(res);
 	}
 	ret = func->destroy(res);
 	res->backup_dirty = true;
@@ -660,7 +690,7 @@ int vmw_resource_validate(struct vmw_resource *res, bool intr)
 	if (unlikely(ret != 0))
 		goto out_no_validate;
 	else if (!res->func->needs_backup && res->backup) {
-		list_del_init(&res->mob_head);
+		WARN_ON_ONCE(vmw_resource_mob_attached(res));
 		vmw_bo_unreference(&res->backup);
 	}
 
@@ -699,7 +729,7 @@ void vmw_resource_unbind_list(struct vmw_buffer_object *vbo)
 		(void) res->func->unbind(res, res->res_dirty, &val_buf);
 		res->backup_dirty = true;
 		res->res_dirty = false;
-		list_del_init(&res->mob_head);
+		vmw_resource_mob_detach(res);
 	}
 
 	(void) ttm_bo_wait(&vbo->base, false, false);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
index 7e19eba0b0b8..984e588c62ca 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h
@@ -78,6 +78,8 @@ struct vmw_res_func {
 	const char *type_name;
 	struct ttm_placement *backup_placement;
 	bool may_evict;
+	u32 prio;
+	u32 dirty_prio;
 
 	int (*create) (struct vmw_resource *res);
 	int (*destroy) (struct vmw_resource *res);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
index d310d21f0d54..e139fdfd1635 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
@@ -95,6 +95,8 @@ static const struct vmw_res_func vmw_gb_shader_func = {
 	.res_type = vmw_res_shader,
 	.needs_backup = true,
 	.may_evict = true,
+	.prio = 3,
+	.dirty_prio = 3,
 	.type_name = "guest backed shaders",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_gb_shader_create,
@@ -106,7 +108,9 @@ static const struct vmw_res_func vmw_gb_shader_func = {
 static const struct vmw_res_func vmw_dx_shader_func = {
 	.res_type = vmw_res_shader,
 	.needs_backup = true,
-	.may_evict = false,
+	.may_evict = true,
+	.prio = 3,
+	.dirty_prio = 3,
 	.type_name = "dx shaders",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_dx_shader_create,
@@ -423,7 +427,7 @@ static int vmw_dx_shader_create(struct vmw_resource *res)
 
 	WARN_ON_ONCE(!shader->committed);
 
-	if (!list_empty(&res->mob_head)) {
+	if (vmw_resource_mob_attached(res)) {
 		mutex_lock(&dev_priv->binding_mutex);
 		ret = vmw_dx_shader_unscrub(res);
 		mutex_unlock(&dev_priv->binding_mutex);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 219471903bc1..c40d44f4d9af 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -112,6 +112,8 @@ static const struct vmw_res_func vmw_legacy_surface_func = {
 	.res_type = vmw_res_surface,
 	.needs_backup = false,
 	.may_evict = true,
+	.prio = 1,
+	.dirty_prio = 1,
 	.type_name = "legacy surfaces",
 	.backup_placement = &vmw_srf_placement,
 	.create = &vmw_legacy_srf_create,
@@ -124,6 +126,8 @@ static const struct vmw_res_func vmw_gb_surface_func = {
 	.res_type = vmw_res_surface,
 	.needs_backup = true,
 	.may_evict = true,
+	.prio = 1,
+	.dirty_prio = 2,
 	.type_name = "guest backed surfaces",
 	.backup_placement = &vmw_mob_placement,
 	.create = vmw_gb_surface_create,
-- 
2.20.1

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

[PATCH 2/4] drm/vmwgfx: Add debug message for layout change ioctl

[Thomas Hellström (VMware)]

From: Deepak Rawat <drawat@vmware.com>

Add debug code to check user-space layout change request.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 8 ++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index 5971c0d47507..b4c8817dc267 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1424,6 +1424,14 @@ int vmw_host_log(const char *log);
 #define VMW_DEBUG_USER(fmt, ...)                                              \
 	DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__)
 
+/**
+ * VMW_DEBUG_KMS - Debug output for kernel mode-setting
+ *
+ * This macro is for debugging vmwgfx mode-setting code.
+ */
+#define VMW_DEBUG_KMS(fmt, ...)                                               \
+	DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__)
+
 /* Resource dirtying - vmwgfx_page_dirty.c */
 void vmw_bo_dirty_scan(struct vmw_buffer_object *vbo);
 int vmw_bo_dirty_add(struct vmw_buffer_object *vbo);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index b97bc8e5944b..7f9264a72e1d 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2347,6 +2347,9 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 
 	if (!arg->num_outputs) {
 		struct drm_rect def_rect = {0, 0, 800, 600};
+		VMW_DEBUG_KMS("Default layout x1 = %d y1 = %d x2 = %d y2 = %d\n",
+			      def_rect.x1, def_rect.y1,
+			      def_rect.x2, def_rect.y2);
 		vmw_du_update_layout(dev_priv, 1, &def_rect);
 		return 0;
 	}
@@ -2367,6 +2370,7 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 
 	drm_rects = (struct drm_rect *)rects;
 
+	VMW_DEBUG_KMS("Layout count = %u\n", arg->num_outputs);
 	for (i = 0; i < arg->num_outputs; i++) {
 		struct drm_vmw_rect curr_rect;
 
@@ -2383,6 +2387,10 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 		drm_rects[i].x2 = curr_rect.x + curr_rect.w;
 		drm_rects[i].y2 = curr_rect.y + curr_rect.h;
 
+		VMW_DEBUG_KMS("  x1 = %d y1 = %d x2 = %d y2 = %d\n",
+			      drm_rects[i].x1, drm_rects[i].y1,
+			      drm_rects[i].x2, drm_rects[i].y2);
+
 		/*
 		 * Currently this check is limiting the topology within
 		 * mode_config->max (which actually is max texture size
-- 
2.20.1

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

[PATCH 3/4] drm/vmwgfx: Use VMW_DEBUG_KMS for vmwgfx mode-setting user errors

[Thomas Hellström (VMware)]

From: Deepak Rawat <drawat@vmware.com>

For errors during layout change ioctl use VMW_DEBUG_KMS instead of
DRM_ERROR.

Signed-off-by: Deepak Rawat <drawat@vmware.com>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 7f9264a72e1d..e7222fa2cfdf 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1462,7 +1462,7 @@ static int vmw_kms_check_display_memory(struct drm_device *dev,
 		if (dev_priv->active_display_unit == vmw_du_screen_target &&
 		    (drm_rect_width(&rects[i]) > dev_priv->stdu_max_width ||
 		     drm_rect_height(&rects[i]) > dev_priv->stdu_max_height)) {
-			DRM_ERROR("Screen size not supported.\n");
+			VMW_DEBUG_KMS("Screen size not supported.\n");
 			return -EINVAL;
 		}
 
@@ -1486,7 +1486,7 @@ static int vmw_kms_check_display_memory(struct drm_device *dev,
 	 * limit on primary bounding box
 	 */
 	if (pixel_mem > dev_priv->prim_bb_mem) {
-		DRM_ERROR("Combined output size too large.\n");
+		VMW_DEBUG_KMS("Combined output size too large.\n");
 		return -EINVAL;
 	}
 
@@ -1496,7 +1496,7 @@ static int vmw_kms_check_display_memory(struct drm_device *dev,
 		bb_mem = (u64) bounding_box.x2 * bounding_box.y2 * 4;
 
 		if (bb_mem > dev_priv->prim_bb_mem) {
-			DRM_ERROR("Topology is beyond supported limits.\n");
+			VMW_DEBUG_KMS("Topology is beyond supported limits.\n");
 			return -EINVAL;
 		}
 	}
@@ -1645,6 +1645,7 @@ static int vmw_kms_check_topology(struct drm_device *dev,
 		struct vmw_connector_state *vmw_conn_state;
 
 		if (!du->pref_active && new_crtc_state->enable) {
+			VMW_DEBUG_KMS("Enabling a disabled display unit\n");
 			ret = -EINVAL;
 			goto clean;
 		}
@@ -1701,8 +1702,10 @@ vmw_kms_atomic_check_modeset(struct drm_device *dev,
 		return ret;
 
 	ret = vmw_kms_check_implicit(dev, state);
-	if (ret)
+	if (ret) {
+		VMW_DEBUG_KMS("Invalid implicit state\n");
 		return ret;
+	}
 
 	if (!state->allow_modeset)
 		return ret;
@@ -2401,7 +2404,9 @@ int vmw_kms_update_layout_ioctl(struct drm_device *dev, void *data,
 		if (drm_rects[i].x1 < 0 ||  drm_rects[i].y1 < 0 ||
 		    drm_rects[i].x2 > mode_config->max_width ||
 		    drm_rects[i].y2 > mode_config->max_height) {
-			DRM_ERROR("Invalid GUI layout.\n");
+			VMW_DEBUG_KMS("Invalid layout %d %d %d %d\n",
+				      drm_rects[i].x1, drm_rects[i].y1,
+				      drm_rects[i].x2, drm_rects[i].y2);
 			ret = -EINVAL;
 			goto out_free;
 		}
-- 
2.20.1

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

[PATCH 4/4] drm/vmwgfx: Kill unneeded legacy security features

[Thomas Hellström (VMware)]

From: Thomas Hellstrom <thellstrom@vmware.com>

At one point, the GPU command verifier and user-space handle manager
couldn't properly protect GPU clients from accessing each other's data.
Instead there was an elaborate mechanism to make sure only the active
master's primary clients could render. The other clients were either
put to sleep or even killed (if the master had exited). VRAM was
evicted on master switch. With the advent of render-node functionality,
we relaxed the VRAM eviction, but the other mechanisms stayed in place.

Now that the GPU  command verifier and ttm object manager properly
isolates primary clients from different master realms we can remove the
master switch related code and drop those legacy features.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Brian Paul <brianp@vmware.com>
Reviewed-by: Deepak Rawat <drawat@vmware.com>
---
 drivers/gpu/drm/vmwgfx/ttm_lock.c       | 100 ---------------
 drivers/gpu/drm/vmwgfx/ttm_lock.h       |  30 -----
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     | 162 +-----------------------
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h     |  16 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c |   6 -
 5 files changed, 3 insertions(+), 311 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/ttm_lock.c b/drivers/gpu/drm/vmwgfx/ttm_lock.c
index 16b2083cb9d4..5971c72e6d10 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_lock.c
+++ b/drivers/gpu/drm/vmwgfx/ttm_lock.c
@@ -29,7 +29,6 @@
  * Authors: Thomas Hellstrom <thellstrom-at-vmware-dot-com>
  */
 
-#include <drm/ttm/ttm_module.h>
 #include <linux/atomic.h>
 #include <linux/errno.h>
 #include <linux/wait.h>
@@ -49,8 +48,6 @@ void ttm_lock_init(struct ttm_lock *lock)
 	init_waitqueue_head(&lock->queue);
 	lock->rw = 0;
 	lock->flags = 0;
-	lock->kill_takers = false;
-	lock->signal = SIGKILL;
 }
 
 void ttm_read_unlock(struct ttm_lock *lock)
@@ -66,11 +63,6 @@ static bool __ttm_read_lock(struct ttm_lock *lock)
 	bool locked = false;
 
 	spin_lock(&lock->lock);
-	if (unlikely(lock->kill_takers)) {
-		send_sig(lock->signal, current, 0);
-		spin_unlock(&lock->lock);
-		return false;
-	}
 	if (lock->rw >= 0 && lock->flags == 0) {
 		++lock->rw;
 		locked = true;
@@ -98,11 +90,6 @@ static bool __ttm_read_trylock(struct ttm_lock *lock, bool *locked)
 	*locked = false;
 
 	spin_lock(&lock->lock);
-	if (unlikely(lock->kill_takers)) {
-		send_sig(lock->signal, current, 0);
-		spin_unlock(&lock->lock);
-		return false;
-	}
 	if (lock->rw >= 0 && lock->flags == 0) {
 		++lock->rw;
 		block = false;
@@ -147,11 +134,6 @@ static bool __ttm_write_lock(struct ttm_lock *lock)
 	bool locked = false;
 
 	spin_lock(&lock->lock);
-	if (unlikely(lock->kill_takers)) {
-		send_sig(lock->signal, current, 0);
-		spin_unlock(&lock->lock);
-		return false;
-	}
 	if (lock->rw == 0 && ((lock->flags & ~TTM_WRITE_LOCK_PENDING) == 0)) {
 		lock->rw = -1;
 		lock->flags &= ~TTM_WRITE_LOCK_PENDING;
@@ -182,88 +164,6 @@ int ttm_write_lock(struct ttm_lock *lock, bool interruptible)
 	return ret;
 }
 
-static int __ttm_vt_unlock(struct ttm_lock *lock)
-{
-	int ret = 0;
-
-	spin_lock(&lock->lock);
-	if (unlikely(!(lock->flags & TTM_VT_LOCK)))
-		ret = -EINVAL;
-	lock->flags &= ~TTM_VT_LOCK;
-	wake_up_all(&lock->queue);
-	spin_unlock(&lock->lock);
-
-	return ret;
-}
-
-static void ttm_vt_lock_remove(struct ttm_base_object **p_base)
-{
-	struct ttm_base_object *base = *p_base;
-	struct ttm_lock *lock = container_of(base, struct ttm_lock, base);
-	int ret;
-
-	*p_base = NULL;
-	ret = __ttm_vt_unlock(lock);
-	BUG_ON(ret != 0);
-}
-
-static bool __ttm_vt_lock(struct ttm_lock *lock)
-{
-	bool locked = false;
-
-	spin_lock(&lock->lock);
-	if (lock->rw == 0) {
-		lock->flags &= ~TTM_VT_LOCK_PENDING;
-		lock->flags |= TTM_VT_LOCK;
-		locked = true;
-	} else {
-		lock->flags |= TTM_VT_LOCK_PENDING;
-	}
-	spin_unlock(&lock->lock);
-	return locked;
-}
-
-int ttm_vt_lock(struct ttm_lock *lock,
-		bool interruptible,
-		struct ttm_object_file *tfile)
-{
-	int ret = 0;
-
-	if (interruptible) {
-		ret = wait_event_interruptible(lock->queue,
-					       __ttm_vt_lock(lock));
-		if (unlikely(ret != 0)) {
-			spin_lock(&lock->lock);
-			lock->flags &= ~TTM_VT_LOCK_PENDING;
-			wake_up_all(&lock->queue);
-			spin_unlock(&lock->lock);
-			return ret;
-		}
-	} else
-		wait_event(lock->queue, __ttm_vt_lock(lock));
-
-	/*
-	 * Add a base-object, the destructor of which will
-	 * make sure the lock is released if the client dies
-	 * while holding it.
-	 */
-
-	ret = ttm_base_object_init(tfile, &lock->base, false,
-				   ttm_lock_type, &ttm_vt_lock_remove, NULL);
-	if (ret)
-		(void)__ttm_vt_unlock(lock);
-	else
-		lock->vt_holder = tfile;
-
-	return ret;
-}
-
-int ttm_vt_unlock(struct ttm_lock *lock)
-{
-	return ttm_ref_object_base_unref(lock->vt_holder,
-					 lock->base.handle, TTM_REF_USAGE);
-}
-
 void ttm_suspend_unlock(struct ttm_lock *lock)
 {
 	spin_lock(&lock->lock);
diff --git a/drivers/gpu/drm/vmwgfx/ttm_lock.h b/drivers/gpu/drm/vmwgfx/ttm_lock.h
index 0c3af9836863..3d454e8b491f 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_lock.h
+++ b/drivers/gpu/drm/vmwgfx/ttm_lock.h
@@ -63,8 +63,6 @@
  * @lock: Spinlock protecting some lock members.
  * @rw: Read-write lock counter. Protected by @lock.
  * @flags: Lock state. Protected by @lock.
- * @kill_takers: Boolean whether to kill takers of the lock.
- * @signal: Signal to send when kill_takers is true.
  */
 
 struct ttm_lock {
@@ -73,9 +71,6 @@ struct ttm_lock {
 	spinlock_t lock;
 	int32_t rw;
 	uint32_t flags;
-	bool kill_takers;
-	int signal;
-	struct ttm_object_file *vt_holder;
 };
 
 
@@ -220,29 +215,4 @@ extern void ttm_write_unlock(struct ttm_lock *lock);
  */
 extern int ttm_write_lock(struct ttm_lock *lock, bool interruptible);
 
-/**
- * ttm_lock_set_kill
- *
- * @lock: Pointer to a struct ttm_lock
- * @val: Boolean whether to kill processes taking the lock.
- * @signal: Signal to send to the process taking the lock.
- *
- * The kill-when-taking-lock functionality is used to kill processes that keep
- * on using the TTM functionality when its resources has been taken down, for
- * example when the X server exits. A typical sequence would look like this:
- * - X server takes lock in write mode.
- * - ttm_lock_set_kill() is called with @val set to true.
- * - As part of X server exit, TTM resources are taken down.
- * - X server releases the lock on file release.
- * - Another dri client wants to render, takes the lock and is killed.
- *
- */
-static inline void ttm_lock_set_kill(struct ttm_lock *lock, bool val,
-				     int signal)
-{
-	lock->kill_takers = val;
-	if (val)
-		lock->signal = signal;
-}
-
 #endif
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index d59c474be38e..8349a6cc126f 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -254,7 +254,6 @@ static int vmw_restrict_dma_mask;
 static int vmw_assume_16bpp;
 
 static int vmw_probe(struct pci_dev *, const struct pci_device_id *);
-static void vmw_master_init(struct vmw_master *);
 static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
 			      void *ptr);
 
@@ -762,10 +761,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 	DRM_INFO("MMIO at 0x%08x size is %u kiB\n",
 		 dev_priv->mmio_start, dev_priv->mmio_size / 1024);
 
-	vmw_master_init(&dev_priv->fbdev_master);
-	ttm_lock_set_kill(&dev_priv->fbdev_master.lock, false, SIGTERM);
-	dev_priv->active_master = &dev_priv->fbdev_master;
-
 	dev_priv->mmio_virt = memremap(dev_priv->mmio_start,
 				       dev_priv->mmio_size, MEMREMAP_WB);
 
@@ -1012,18 +1007,7 @@ static void vmw_driver_unload(struct drm_device *dev)
 static void vmw_postclose(struct drm_device *dev,
 			 struct drm_file *file_priv)
 {
-	struct vmw_fpriv *vmw_fp;
-
-	vmw_fp = vmw_fpriv(file_priv);
-
-	if (vmw_fp->locked_master) {
-		struct vmw_master *vmaster =
-			vmw_master(vmw_fp->locked_master);
-
-		ttm_lock_set_kill(&vmaster->lock, true, SIGTERM);
-		ttm_vt_unlock(&vmaster->lock);
-		drm_master_put(&vmw_fp->locked_master);
-	}
+	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
 
 	ttm_object_file_release(&vmw_fp->tfile);
 	kfree(vmw_fp);
@@ -1052,55 +1036,6 @@ static int vmw_driver_open(struct drm_device *dev, struct drm_file *file_priv)
 	return ret;
 }
 
-static struct vmw_master *vmw_master_check(struct drm_device *dev,
-					   struct drm_file *file_priv,
-					   unsigned int flags)
-{
-	int ret;
-	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
-	struct vmw_master *vmaster;
-
-	if (!drm_is_primary_client(file_priv) || !(flags & DRM_AUTH))
-		return NULL;
-
-	ret = mutex_lock_interruptible(&dev->master_mutex);
-	if (unlikely(ret != 0))
-		return ERR_PTR(-ERESTARTSYS);
-
-	if (drm_is_current_master(file_priv)) {
-		mutex_unlock(&dev->master_mutex);
-		return NULL;
-	}
-
-	/*
-	 * Check if we were previously master, but now dropped. In that
-	 * case, allow at least render node functionality.
-	 */
-	if (vmw_fp->locked_master) {
-		mutex_unlock(&dev->master_mutex);
-
-		if (flags & DRM_RENDER_ALLOW)
-			return NULL;
-
-		DRM_ERROR("Dropped master trying to access ioctl that "
-			  "requires authentication.\n");
-		return ERR_PTR(-EACCES);
-	}
-	mutex_unlock(&dev->master_mutex);
-
-	/*
-	 * Take the TTM lock. Possibly sleep waiting for the authenticating
-	 * master to become master again, or for a SIGTERM if the
-	 * authenticating master exits.
-	 */
-	vmaster = vmw_master(file_priv->master);
-	ret = ttm_read_lock(&vmaster->lock, true);
-	if (unlikely(ret != 0))
-		vmaster = ERR_PTR(ret);
-
-	return vmaster;
-}
-
 static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 			      unsigned long arg,
 			      long (*ioctl_func)(struct file *, unsigned int,
@@ -1109,7 +1044,6 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 	struct drm_file *file_priv = filp->private_data;
 	struct drm_device *dev = file_priv->minor->dev;
 	unsigned int nr = DRM_IOCTL_NR(cmd);
-	struct vmw_master *vmaster;
 	unsigned int flags;
 	long ret;
 
@@ -1145,21 +1079,7 @@ static long vmw_generic_ioctl(struct file *filp, unsigned int cmd,
 	} else if (!drm_ioctl_flags(nr, &flags))
 		return -EINVAL;
 
-	vmaster = vmw_master_check(dev, file_priv, flags);
-	if (IS_ERR(vmaster)) {
-		ret = PTR_ERR(vmaster);
-
-		if (ret != -ERESTARTSYS)
-			DRM_INFO("IOCTL ERROR Command %d, Error %ld.\n",
-				 nr, ret);
-		return ret;
-	}
-
-	ret = ioctl_func(filp, cmd, arg);
-	if (vmaster)
-		ttm_read_unlock(&vmaster->lock);
-
-	return ret;
+	return ioctl_func(filp, cmd, arg);
 
 out_io_encoding:
 	DRM_ERROR("Invalid command format, ioctl %d\n",
@@ -1186,65 +1106,10 @@ static void vmw_lastclose(struct drm_device *dev)
 {
 }
 
-static void vmw_master_init(struct vmw_master *vmaster)
-{
-	ttm_lock_init(&vmaster->lock);
-}
-
-static int vmw_master_create(struct drm_device *dev,
-			     struct drm_master *master)
-{
-	struct vmw_master *vmaster;
-
-	vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
-	if (unlikely(!vmaster))
-		return -ENOMEM;
-
-	vmw_master_init(vmaster);
-	ttm_lock_set_kill(&vmaster->lock, true, SIGTERM);
-	master->driver_priv = vmaster;
-
-	return 0;
-}
-
-static void vmw_master_destroy(struct drm_device *dev,
-			       struct drm_master *master)
-{
-	struct vmw_master *vmaster = vmw_master(master);
-
-	master->driver_priv = NULL;
-	kfree(vmaster);
-}
-
 static int vmw_master_set(struct drm_device *dev,
 			  struct drm_file *file_priv,
 			  bool from_open)
 {
-	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
-	struct vmw_master *active = dev_priv->active_master;
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
-	int ret = 0;
-
-	if (active) {
-		BUG_ON(active != &dev_priv->fbdev_master);
-		ret = ttm_vt_lock(&active->lock, false, vmw_fp->tfile);
-		if (unlikely(ret != 0))
-			return ret;
-
-		ttm_lock_set_kill(&active->lock, true, SIGTERM);
-		dev_priv->active_master = NULL;
-	}
-
-	ttm_lock_set_kill(&vmaster->lock, false, SIGTERM);
-	if (!from_open) {
-		ttm_vt_unlock(&vmaster->lock);
-		BUG_ON(vmw_fp->locked_master != file_priv->master);
-		drm_master_put(&vmw_fp->locked_master);
-	}
-
-	dev_priv->active_master = vmaster;
-
 	/*
 	 * Inform a new master that the layout may have changed while
 	 * it was gone.
@@ -1259,31 +1124,10 @@ static void vmw_master_drop(struct drm_device *dev,
 			    struct drm_file *file_priv)
 {
 	struct vmw_private *dev_priv = vmw_priv(dev);
-	struct vmw_fpriv *vmw_fp = vmw_fpriv(file_priv);
-	struct vmw_master *vmaster = vmw_master(file_priv->master);
-	int ret;
-
-	/**
-	 * Make sure the master doesn't disappear while we have
-	 * it locked.
-	 */
 
-	vmw_fp->locked_master = drm_master_get(file_priv->master);
-	ret = ttm_vt_lock(&vmaster->lock, false, vmw_fp->tfile);
 	vmw_kms_legacy_hotspot_clear(dev_priv);
-	if (unlikely((ret != 0))) {
-		DRM_ERROR("Unable to lock TTM at VT switch.\n");
-		drm_master_put(&vmw_fp->locked_master);
-	}
-
-	ttm_lock_set_kill(&vmaster->lock, false, SIGTERM);
-
 	if (!dev_priv->enable_fb)
 		vmw_svga_disable(dev_priv);
-
-	dev_priv->active_master = &dev_priv->fbdev_master;
-	ttm_lock_set_kill(&dev_priv->fbdev_master.lock, false, SIGTERM);
-	ttm_vt_unlock(&dev_priv->fbdev_master.lock);
 }
 
 /**
@@ -1562,8 +1406,6 @@ static struct drm_driver driver = {
 	.disable_vblank = vmw_disable_vblank,
 	.ioctls = vmw_ioctls,
 	.num_ioctls = ARRAY_SIZE(vmw_ioctls),
-	.master_create = vmw_master_create,
-	.master_destroy = vmw_master_destroy,
 	.master_set = vmw_master_set,
 	.master_drop = vmw_master_drop,
 	.open = vmw_driver_open,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index b4c8817dc267..3a358a5495e4 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -81,7 +81,6 @@
 #define VMW_RES_SHADER ttm_driver_type4
 
 struct vmw_fpriv {
-	struct drm_master *locked_master;
 	struct ttm_object_file *tfile;
 	bool gb_aware; /* user-space is guest-backed aware */
 };
@@ -393,10 +392,6 @@ struct vmw_sw_context{
 struct vmw_legacy_display;
 struct vmw_overlay;
 
-struct vmw_master {
-	struct ttm_lock lock;
-};
-
 struct vmw_vga_topology_state {
 	uint32_t width;
 	uint32_t height;
@@ -559,11 +554,8 @@ struct vmw_private {
 	spinlock_t svga_lock;
 
 	/**
-	 * Master management.
+	 * PM management.
 	 */
-
-	struct vmw_master *active_master;
-	struct vmw_master fbdev_master;
 	struct notifier_block pm_nb;
 	bool refuse_hibernation;
 	bool suspend_locked;
@@ -632,11 +624,6 @@ static inline struct vmw_fpriv *vmw_fpriv(struct drm_file *file_priv)
 	return (struct vmw_fpriv *)file_priv->driver_priv;
 }
 
-static inline struct vmw_master *vmw_master(struct drm_master *master)
-{
-	return (struct vmw_master *) master->driver_priv;
-}
-
 /*
  * The locking here is fine-grained, so that it is performed once
  * for every read- and write operation. This is of course costly, but we
@@ -1102,7 +1089,6 @@ void vmw_kms_cursor_snoop(struct vmw_surface *srf,
 int vmw_kms_write_svga(struct vmw_private *vmw_priv,
 		       unsigned width, unsigned height, unsigned pitch,
 		       unsigned bpp, unsigned depth);
-void vmw_kms_idle_workqueues(struct vmw_master *vmaster);
 bool vmw_kms_validate_mode_vram(struct vmw_private *dev_priv,
 				uint32_t pitch,
 				uint32_t height);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 637043f1befa..862ca44680ca 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -946,12 +946,6 @@ vmw_surface_handle_reference(struct vmw_private *dev_priv,
 		if (unlikely(drm_is_render_client(file_priv)))
 			require_exist = true;
 
-		if (READ_ONCE(vmw_fpriv(file_priv)->locked_master)) {
-			DRM_ERROR("Locked master refused legacy "
-				  "surface reference.\n");
-			return -EACCES;
-		}
-
 		handle = u_handle;
 	}
 
-- 
2.20.1

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

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Emil Velikov]

Hi Thomas,

On 2019/06/18, Thomas Hellström (VMware) wrote:
> From: Thomas Hellstrom <thellstrom@vmware.com>
> 
> TTM provides a means to assign eviction priorities to buffer object. This
> means that all buffer objects with a lower priority will be evicted first
> on memory pressure.
> Use this to make sure surfaces and in particular non-dirty surfaces are
> evicted first. Evicting in particular shaders, cotables and contexts imply
> a significant performance hit on vmwgfx, so make sure these resources are
> evicted last.
> Some buffer objects are sub-allocated in user-space which means we can have
> many resources attached to a single buffer object or resource. In that case
> the buffer object is given the highest priority of the attached resources.
> 
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Deepak Rawat <drawat@vmware.com>
Fwiw patches 1-3 are:
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Patch 4 is:
Acked-by: Emil Velikov <emil.velikov@collabora.com>

Huge thanks for sorting this out.
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Daniel Vetter]

On Tue, Jun 18, 2019 at 11:54:08AM +0100, Emil Velikov wrote:
> Hi Thomas,
> 
> On 2019/06/18, Thomas Hellström (VMware) wrote:
> > From: Thomas Hellstrom <thellstrom@vmware.com>
> > 
> > TTM provides a means to assign eviction priorities to buffer object. This
> > means that all buffer objects with a lower priority will be evicted first
> > on memory pressure.
> > Use this to make sure surfaces and in particular non-dirty surfaces are
> > evicted first. Evicting in particular shaders, cotables and contexts imply
> > a significant performance hit on vmwgfx, so make sure these resources are
> > evicted last.
> > Some buffer objects are sub-allocated in user-space which means we can have
> > many resources attached to a single buffer object or resource. In that case
> > the buffer object is given the highest priority of the attached resources.
> > 
> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > Reviewed-by: Deepak Rawat <drawat@vmware.com>
> Fwiw patches 1-3 are:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> Patch 4 is:
> Acked-by: Emil Velikov <emil.velikov@collabora.com>
> 
> Huge thanks for sorting this out.

Oh, does this mean we can remove the varios master* callbacks from
drm_driver now? Iirc vmwgfx was the only user, and those callbacks seem
very tempting to various folks for implementing questionable driver hacks
... Happy to type the patches, but maybe simpler if you do that since all
this gets merged through the vmwgfx tree.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Thomas Hellstrom]

On 6/18/19 2:19 PM, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 11:54:08AM +0100, Emil Velikov wrote:
>> Hi Thomas,
>>
>> On 2019/06/18, Thomas Hellström (VMware) wrote:
>>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>>
>>> TTM provides a means to assign eviction priorities to buffer object. This
>>> means that all buffer objects with a lower priority will be evicted first
>>> on memory pressure.
>>> Use this to make sure surfaces and in particular non-dirty surfaces are
>>> evicted first. Evicting in particular shaders, cotables and contexts imply
>>> a significant performance hit on vmwgfx, so make sure these resources are
>>> evicted last.
>>> Some buffer objects are sub-allocated in user-space which means we can have
>>> many resources attached to a single buffer object or resource. In that case
>>> the buffer object is given the highest priority of the attached resources.
>>>
>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>> Reviewed-by: Deepak Rawat <drawat@vmware.com>
>> Fwiw patches 1-3 are:
>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>>
>> Patch 4 is:
>> Acked-by: Emil Velikov <emil.velikov@collabora.com>
>>
>> Huge thanks for sorting this out.
> Oh, does this mean we can remove the varios master* callbacks from
> drm_driver now? Iirc vmwgfx was the only user, and those callbacks seem
> very tempting to various folks for implementing questionable driver hacks
> ... Happy to type the patches, but maybe simpler if you do that since all
> this gets merged through the vmwgfx tree.
>
> Cheers, Daniel

In case someone follow this, I'll paste in the commit message of 4/4 
which is the relevant one here..

8<--------------------------------------------

At one point, the GPU command verifier and user-space handle manager
couldn't properly protect GPU clients from accessing each other's data.
Instead there was an elaborate mechanism to make sure only the active
master's primary clients could render. The other clients were either
put to sleep or even killed (if the master had exited). VRAM was
evicted on master switch. With the advent of render-node functionality,
we relaxed the VRAM eviction, but the other mechanisms stayed in place.

Now that the GPU  command verifier and ttm object manager properly
isolates primary clients from different master realms we can remove the
master switch related code and drop those legacy features.

8<-------------------------------------------

I think we can at least take a look. I'm out on a fairly long vacation 
soon so in any case it won't be before August or so.

One use we still have for master_set() is that if a master is switched 
away, and then the mode list changes, and then the master is switched 
back, it will typically not remember to act on the sysfs event received 
while switched out, and come back in an incorrect mode. Since mode-list 
changes happen quite frequently with virtual display adapters that's bad.

But perhaps we can consider moving that to core, if that's what needed 
to get rid of the master switch callbacks.

/Thomas



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

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Thomas Hellstrom]

On 6/18/19 12:54 PM, Emil Velikov wrote:
> Hi Thomas,
>
> On 2019/06/18, Thomas Hellström (VMware) wrote:
>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>
>> TTM provides a means to assign eviction priorities to buffer object. This
>> means that all buffer objects with a lower priority will be evicted first
>> on memory pressure.
>> Use this to make sure surfaces and in particular non-dirty surfaces are
>> evicted first. Evicting in particular shaders, cotables and contexts imply
>> a significant performance hit on vmwgfx, so make sure these resources are
>> evicted last.
>> Some buffer objects are sub-allocated in user-space which means we can have
>> many resources attached to a single buffer object or resource. In that case
>> the buffer object is given the highest priority of the attached resources.
>>
>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Reviewed-by: Deepak Rawat <drawat@vmware.com>
> Fwiw patches 1-3 are:
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>
> Patch 4 is:
> Acked-by: Emil Velikov <emil.velikov@collabora.com>
>
> Huge thanks for sorting this out.
> Emil

Thanks for reviewing, Emil.

/Thomas


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

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Daniel Vetter]

On Tue, Jun 18, 2019 at 03:08:01PM +0200, Thomas Hellstrom wrote:
> On 6/18/19 2:19 PM, Daniel Vetter wrote:
> > On Tue, Jun 18, 2019 at 11:54:08AM +0100, Emil Velikov wrote:
> > > Hi Thomas,
> > > 
> > > On 2019/06/18, Thomas Hellström (VMware) wrote:
> > > > From: Thomas Hellstrom <thellstrom@vmware.com>
> > > > 
> > > > TTM provides a means to assign eviction priorities to buffer object. This
> > > > means that all buffer objects with a lower priority will be evicted first
> > > > on memory pressure.
> > > > Use this to make sure surfaces and in particular non-dirty surfaces are
> > > > evicted first. Evicting in particular shaders, cotables and contexts imply
> > > > a significant performance hit on vmwgfx, so make sure these resources are
> > > > evicted last.
> > > > Some buffer objects are sub-allocated in user-space which means we can have
> > > > many resources attached to a single buffer object or resource. In that case
> > > > the buffer object is given the highest priority of the attached resources.
> > > > 
> > > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > > Reviewed-by: Deepak Rawat <drawat@vmware.com>
> > > Fwiw patches 1-3 are:
> > > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Patch 4 is:
> > > Acked-by: Emil Velikov <emil.velikov@collabora.com>
> > > 
> > > Huge thanks for sorting this out.
> > Oh, does this mean we can remove the varios master* callbacks from
> > drm_driver now? Iirc vmwgfx was the only user, and those callbacks seem
> > very tempting to various folks for implementing questionable driver hacks
> > ... Happy to type the patches, but maybe simpler if you do that since all
> > this gets merged through the vmwgfx tree.
> > 
> > Cheers, Daniel
> 
> In case someone follow this, I'll paste in the commit message of 4/4 which
> is the relevant one here..
> 
> 8<--------------------------------------------
> 
> At one point, the GPU command verifier and user-space handle manager
> couldn't properly protect GPU clients from accessing each other's data.
> Instead there was an elaborate mechanism to make sure only the active
> master's primary clients could render. The other clients were either
> put to sleep or even killed (if the master had exited). VRAM was
> evicted on master switch. With the advent of render-node functionality,
> we relaxed the VRAM eviction, but the other mechanisms stayed in place.
> 
> Now that the GPU  command verifier and ttm object manager properly
> isolates primary clients from different master realms we can remove the
> master switch related code and drop those legacy features.
> 
> 8<-------------------------------------------
> 
> I think we can at least take a look. I'm out on a fairly long vacation soon
> so in any case it won't be before August or so.

Ah don't worry, if this all lands in the 5.3 merge window I can take a
look in a few weeks.

> One use we still have for master_set() is that if a master is switched away,
> and then the mode list changes, and then the master is switched back, it
> will typically not remember to act on the sysfs event received while
> switched out, and come back in an incorrect mode. Since mode-list changes
> happen quite frequently with virtual display adapters that's bad.
> 
> But perhaps we can consider moving that to core, if that's what needed to
> get rid of the master switch callbacks.

Hm, this sounds a bit like papering over userspace bugs, at least if
you're referring to drm_sysfs_hotplug_event(). Userspace is supposed to
either keep listening or to re-acquire all the kms output state and do the
hotplugg processing in one go when becoming active again.

Ofc it exists, so we can't just remove it. I wouldn't want to make this
part of the uapi though, feels like duct-taping around sloppy userspace.
Maybe we could work on a gradual plan to deprecate this, with limiting it
only to older vmwgfx versions as a start?

These kind of tiny but important differences in how drivers implement kms
is why I'd much, much prefer it's not even possible to do stuff like this.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Thomas Hellstrom]

On 6/18/19 3:27 PM, Daniel Vetter wrote:
> On Tue, Jun 18, 2019 at 03:08:01PM +0200, Thomas Hellstrom wrote:
>> On 6/18/19 2:19 PM, Daniel Vetter wrote:
>>> On Tue, Jun 18, 2019 at 11:54:08AM +0100, Emil Velikov wrote:
>>>> Hi Thomas,
>>>>
>>>> On 2019/06/18, Thomas Hellström (VMware) wrote:
>>>>> From: Thomas Hellstrom <thellstrom@vmware.com>
>>>>>
>>>>> TTM provides a means to assign eviction priorities to buffer object. This
>>>>> means that all buffer objects with a lower priority will be evicted first
>>>>> on memory pressure.
>>>>> Use this to make sure surfaces and in particular non-dirty surfaces are
>>>>> evicted first. Evicting in particular shaders, cotables and contexts imply
>>>>> a significant performance hit on vmwgfx, so make sure these resources are
>>>>> evicted last.
>>>>> Some buffer objects are sub-allocated in user-space which means we can have
>>>>> many resources attached to a single buffer object or resource. In that case
>>>>> the buffer object is given the highest priority of the attached resources.
>>>>>
>>>>> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
>>>>> Reviewed-by: Deepak Rawat <drawat@vmware.com>
>>>> Fwiw patches 1-3 are:
>>>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>>>>
>>>> Patch 4 is:
>>>> Acked-by: Emil Velikov <emil.velikov@collabora.com>
>>>>
>>>> Huge thanks for sorting this out.
>>> Oh, does this mean we can remove the varios master* callbacks from
>>> drm_driver now? Iirc vmwgfx was the only user, and those callbacks seem
>>> very tempting to various folks for implementing questionable driver hacks
>>> ... Happy to type the patches, but maybe simpler if you do that since all
>>> this gets merged through the vmwgfx tree.
>>>
>>> Cheers, Daniel
>> In case someone follow this, I'll paste in the commit message of 4/4 which
>> is the relevant one here..
>>
>> 8<--------------------------------------------
>>
>> At one point, the GPU command verifier and user-space handle manager
>> couldn't properly protect GPU clients from accessing each other's data.
>> Instead there was an elaborate mechanism to make sure only the active
>> master's primary clients could render. The other clients were either
>> put to sleep or even killed (if the master had exited). VRAM was
>> evicted on master switch. With the advent of render-node functionality,
>> we relaxed the VRAM eviction, but the other mechanisms stayed in place.
>>
>> Now that the GPU  command verifier and ttm object manager properly
>> isolates primary clients from different master realms we can remove the
>> master switch related code and drop those legacy features.
>>
>> 8<-------------------------------------------
>>
>> I think we can at least take a look. I'm out on a fairly long vacation soon
>> so in any case it won't be before August or so.
> Ah don't worry, if this all lands in the 5.3 merge window I can take a
> look in a few weeks.
>
>> One use we still have for master_set() is that if a master is switched away,
>> and then the mode list changes, and then the master is switched back, it
>> will typically not remember to act on the sysfs event received while
>> switched out, and come back in an incorrect mode. Since mode-list changes
>> happen quite frequently with virtual display adapters that's bad.
>>
>> But perhaps we can consider moving that to core, if that's what needed to
>> get rid of the master switch callbacks.
> Hm, this sounds a bit like papering over userspace bugs, at least if
> you're referring to drm_sysfs_hotplug_event(). Userspace is supposed to
> either keep listening or to re-acquire all the kms output state and do the
> hotplugg processing in one go when becoming active again.
>
> Ofc it exists, so we can't just remove it. I wouldn't want to make this
> part of the uapi though, feels like duct-taping around sloppy userspace.
> Maybe we could work on a gradual plan to deprecate this, with limiting it
> only to older vmwgfx versions as a start?

Sounds ok with me. First I guess I need to figure out what compositors / 
user-space drivers actually suffer from this. If there are many, it 
might be a pain trying to fix them all.

Thanks,

Thomas


>
> These kind of tiny but important differences in how drivers implement kms
> is why I'd much, much prefer it's not even possible to do stuff like this.
>
> Thanks, Daniel


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

Re: [PATCH 1/4] drm/vmwgfx: Assign eviction priorities to resources

[Daniel Vetter]

On Tue, Jun 18, 2019 at 04:14:27PM +0200, Thomas Hellstrom wrote:
> On 6/18/19 3:27 PM, Daniel Vetter wrote:
> > On Tue, Jun 18, 2019 at 03:08:01PM +0200, Thomas Hellstrom wrote:
> > > On 6/18/19 2:19 PM, Daniel Vetter wrote:
> > > > On Tue, Jun 18, 2019 at 11:54:08AM +0100, Emil Velikov wrote:
> > > > > Hi Thomas,
> > > > > 
> > > > > On 2019/06/18, Thomas Hellström (VMware) wrote:
> > > > > > From: Thomas Hellstrom <thellstrom@vmware.com>
> > > > > > 
> > > > > > TTM provides a means to assign eviction priorities to buffer object. This
> > > > > > means that all buffer objects with a lower priority will be evicted first
> > > > > > on memory pressure.
> > > > > > Use this to make sure surfaces and in particular non-dirty surfaces are
> > > > > > evicted first. Evicting in particular shaders, cotables and contexts imply
> > > > > > a significant performance hit on vmwgfx, so make sure these resources are
> > > > > > evicted last.
> > > > > > Some buffer objects are sub-allocated in user-space which means we can have
> > > > > > many resources attached to a single buffer object or resource. In that case
> > > > > > the buffer object is given the highest priority of the attached resources.
> > > > > > 
> > > > > > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > > > > > Reviewed-by: Deepak Rawat <drawat@vmware.com>
> > > > > Fwiw patches 1-3 are:
> > > > > Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> > > > > 
> > > > > Patch 4 is:
> > > > > Acked-by: Emil Velikov <emil.velikov@collabora.com>
> > > > > 
> > > > > Huge thanks for sorting this out.
> > > > Oh, does this mean we can remove the varios master* callbacks from
> > > > drm_driver now? Iirc vmwgfx was the only user, and those callbacks seem
> > > > very tempting to various folks for implementing questionable driver hacks
> > > > ... Happy to type the patches, but maybe simpler if you do that since all
> > > > this gets merged through the vmwgfx tree.
> > > > 
> > > > Cheers, Daniel
> > > In case someone follow this, I'll paste in the commit message of 4/4 which
> > > is the relevant one here..
> > > 
> > > 8<--------------------------------------------
> > > 
> > > At one point, the GPU command verifier and user-space handle manager
> > > couldn't properly protect GPU clients from accessing each other's data.
> > > Instead there was an elaborate mechanism to make sure only the active
> > > master's primary clients could render. The other clients were either
> > > put to sleep or even killed (if the master had exited). VRAM was
> > > evicted on master switch. With the advent of render-node functionality,
> > > we relaxed the VRAM eviction, but the other mechanisms stayed in place.
> > > 
> > > Now that the GPU  command verifier and ttm object manager properly
> > > isolates primary clients from different master realms we can remove the
> > > master switch related code and drop those legacy features.
> > > 
> > > 8<-------------------------------------------
> > > 
> > > I think we can at least take a look. I'm out on a fairly long vacation soon
> > > so in any case it won't be before August or so.
> > Ah don't worry, if this all lands in the 5.3 merge window I can take a
> > look in a few weeks.
> > 
> > > One use we still have for master_set() is that if a master is switched away,
> > > and then the mode list changes, and then the master is switched back, it
> > > will typically not remember to act on the sysfs event received while
> > > switched out, and come back in an incorrect mode. Since mode-list changes
> > > happen quite frequently with virtual display adapters that's bad.
> > > 
> > > But perhaps we can consider moving that to core, if that's what needed to
> > > get rid of the master switch callbacks.
> > Hm, this sounds a bit like papering over userspace bugs, at least if
> > you're referring to drm_sysfs_hotplug_event(). Userspace is supposed to
> > either keep listening or to re-acquire all the kms output state and do the
> > hotplugg processing in one go when becoming active again.
> > 
> > Ofc it exists, so we can't just remove it. I wouldn't want to make this
> > part of the uapi though, feels like duct-taping around sloppy userspace.
> > Maybe we could work on a gradual plan to deprecate this, with limiting it
> > only to older vmwgfx versions as a start?
> 
> Sounds ok with me. First I guess I need to figure out what compositors /
> user-space drivers actually suffer from this. If there are many, it might be
> a pain trying to fix them all.

Yeah if this shipped already sailed for most compositors, then I guess we
need to upgrade this to cross-driver behavior. If it's just some vmwgfx
thing, then we should try to phase it out slowly somehow.

Either way I much prefer consistent behaviour for anything that is
relevant for kms clients and compositors.

Anyway, great progress on the other master callbacks, thanks a lot for
doing that!

Cheers, Daniel

> 
> Thanks,
> 
> Thomas
> 
> 
> > 
> > These kind of tiny but important differences in how drivers implement kms
> > is why I'd much, much prefer it's not even possible to do stuff like this.
> > 
> > Thanks, Daniel
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel