All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-17 17:52 ` Nirmoy Das
  0 siblings, 0 replies; 31+ messages in thread
From: Nirmoy Das @ 2023-01-17 17:52 UTC (permalink / raw)
  To: intel-gfx
  Cc: Tvrtko Ursulin, Chris Wilson, dri-devel, Thomas Zimmermann,
	Andi Shyti, Nirmoy Das

Currently there is no easy way for a drm driver to safely check and allow
drm_vma_offset_node for a drm file just once. Allow drm drivers to call
non-refcounted version of drm_vma_node_allow() so that a driver doesn't
need to keep track of each drm_vma_node_allow() to call subsequent
drm_vma_node_revoke() to prevent memory leak.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>

Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
 include/drm/drm_vma_manager.h     |  1 +
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
index 7de37f8c68fd..83229a031af0 100644
--- a/drivers/gpu/drm/drm_vma_manager.c
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
 }
 EXPORT_SYMBOL(drm_vma_offset_remove);
 
-/**
- * drm_vma_node_allow - Add open-file to list of allowed users
- * @node: Node to modify
- * @tag: Tag of file to remove
- *
- * Add @tag to the list of allowed open-files for this node. If @tag is
- * already on this list, the ref-count is incremented.
- *
- * The list of allowed-users is preserved across drm_vma_offset_add() and
- * drm_vma_offset_remove() calls. You may even call it if the node is currently
- * not added to any offset-manager.
- *
- * You must remove all open-files the same number of times as you added them
- * before destroying the node. Otherwise, you will leak memory.
- *
- * This is locked against concurrent access internally.
- *
- * RETURNS:
- * 0 on success, negative error code on internal failure (out-of-mem)
- */
-int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
+static int vma_node_allow(struct drm_vma_offset_node *node,
+			  struct drm_file *tag, bool ref_counted)
 {
 	struct rb_node **iter;
 	struct rb_node *parent = NULL;
@@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
 		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
 
 		if (tag == entry->vm_tag) {
-			entry->vm_count++;
+			if (ref_counted)
+				entry->vm_count++;
 			goto unlock;
 		} else if (tag > entry->vm_tag) {
 			iter = &(*iter)->rb_right;
@@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
 	kfree(new);
 	return ret;
 }
+
+/**
+ * drm_vma_node_allow - Add open-file to list of allowed users
+ * @node: Node to modify
+ * @tag: Tag of file to remove
+ *
+ * Add @tag to the list of allowed open-files for this node. If @tag is
+ * already on this list, the ref-count is incremented.
+ *
+ * The list of allowed-users is preserved across drm_vma_offset_add() and
+ * drm_vma_offset_remove() calls. You may even call it if the node is currently
+ * not added to any offset-manager.
+ *
+ * You must remove all open-files the same number of times as you added them
+ * before destroying the node. Otherwise, you will leak memory.
+ *
+ * This is locked against concurrent access internally.
+ *
+ * RETURNS:
+ * 0 on success, negative error code on internal failure (out-of-mem)
+ */
+int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
+{
+	return vma_node_allow(node, tag, true);
+}
 EXPORT_SYMBOL(drm_vma_node_allow);
 
+/**
+ * drm_vma_node_allow_once - Add open-file to list of allowed users
+ * @node: Node to modify
+ * @tag: Tag of file to remove
+ *
+ * Add @tag to the list of allowed open-files for this node.
+ *
+ * The list of allowed-users is preserved across drm_vma_offset_add() and
+ * drm_vma_offset_remove() calls. You may even call it if the node is currently
+ * not added to any offset-manager.
+ *
+ * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
+ * should only be called once after this.
+ *
+ * This is locked against concurrent access internally.
+ *
+ * RETURNS:
+ * 0 on success, negative error code on internal failure (out-of-mem)
+ */
+int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
+{
+	return vma_node_allow(node, tag, false);
+}
+EXPORT_SYMBOL(drm_vma_node_allow_once);
+
 /**
  * drm_vma_node_revoke - Remove open-file from list of allowed users
  * @node: Node to modify
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 4f8c35206f7c..6c2a2f21dbf0 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
 			   struct drm_vma_offset_node *node);
 
 int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
+int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
 void drm_vma_node_revoke(struct drm_vma_offset_node *node,
 			 struct drm_file *tag);
 bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-17 17:52 ` Nirmoy Das
  0 siblings, 0 replies; 31+ messages in thread
From: Nirmoy Das @ 2023-01-17 17:52 UTC (permalink / raw)
  To: intel-gfx
  Cc: Chris Wilson, Daniel Vetter, Maxime Ripard, dri-devel,
	Thomas Zimmermann, David Airlie, Nirmoy Das

Currently there is no easy way for a drm driver to safely check and allow
drm_vma_offset_node for a drm file just once. Allow drm drivers to call
non-refcounted version of drm_vma_node_allow() so that a driver doesn't
need to keep track of each drm_vma_node_allow() to call subsequent
drm_vma_node_revoke() to prevent memory leak.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>

Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
 include/drm/drm_vma_manager.h     |  1 +
 2 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
index 7de37f8c68fd..83229a031af0 100644
--- a/drivers/gpu/drm/drm_vma_manager.c
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
 }
 EXPORT_SYMBOL(drm_vma_offset_remove);
 
-/**
- * drm_vma_node_allow - Add open-file to list of allowed users
- * @node: Node to modify
- * @tag: Tag of file to remove
- *
- * Add @tag to the list of allowed open-files for this node. If @tag is
- * already on this list, the ref-count is incremented.
- *
- * The list of allowed-users is preserved across drm_vma_offset_add() and
- * drm_vma_offset_remove() calls. You may even call it if the node is currently
- * not added to any offset-manager.
- *
- * You must remove all open-files the same number of times as you added them
- * before destroying the node. Otherwise, you will leak memory.
- *
- * This is locked against concurrent access internally.
- *
- * RETURNS:
- * 0 on success, negative error code on internal failure (out-of-mem)
- */
-int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
+static int vma_node_allow(struct drm_vma_offset_node *node,
+			  struct drm_file *tag, bool ref_counted)
 {
 	struct rb_node **iter;
 	struct rb_node *parent = NULL;
@@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
 		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
 
 		if (tag == entry->vm_tag) {
-			entry->vm_count++;
+			if (ref_counted)
+				entry->vm_count++;
 			goto unlock;
 		} else if (tag > entry->vm_tag) {
 			iter = &(*iter)->rb_right;
@@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
 	kfree(new);
 	return ret;
 }
+
+/**
+ * drm_vma_node_allow - Add open-file to list of allowed users
+ * @node: Node to modify
+ * @tag: Tag of file to remove
+ *
+ * Add @tag to the list of allowed open-files for this node. If @tag is
+ * already on this list, the ref-count is incremented.
+ *
+ * The list of allowed-users is preserved across drm_vma_offset_add() and
+ * drm_vma_offset_remove() calls. You may even call it if the node is currently
+ * not added to any offset-manager.
+ *
+ * You must remove all open-files the same number of times as you added them
+ * before destroying the node. Otherwise, you will leak memory.
+ *
+ * This is locked against concurrent access internally.
+ *
+ * RETURNS:
+ * 0 on success, negative error code on internal failure (out-of-mem)
+ */
+int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
+{
+	return vma_node_allow(node, tag, true);
+}
 EXPORT_SYMBOL(drm_vma_node_allow);
 
+/**
+ * drm_vma_node_allow_once - Add open-file to list of allowed users
+ * @node: Node to modify
+ * @tag: Tag of file to remove
+ *
+ * Add @tag to the list of allowed open-files for this node.
+ *
+ * The list of allowed-users is preserved across drm_vma_offset_add() and
+ * drm_vma_offset_remove() calls. You may even call it if the node is currently
+ * not added to any offset-manager.
+ *
+ * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
+ * should only be called once after this.
+ *
+ * This is locked against concurrent access internally.
+ *
+ * RETURNS:
+ * 0 on success, negative error code on internal failure (out-of-mem)
+ */
+int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
+{
+	return vma_node_allow(node, tag, false);
+}
+EXPORT_SYMBOL(drm_vma_node_allow_once);
+
 /**
  * drm_vma_node_revoke - Remove open-file from list of allowed users
  * @node: Node to modify
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 4f8c35206f7c..6c2a2f21dbf0 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
 			   struct drm_vma_offset_node *node);
 
 int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
+int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
 void drm_vma_node_revoke(struct drm_vma_offset_node *node,
 			 struct drm_file *tag);
 bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
@ 2023-01-17 17:52   ` Nirmoy Das
  -1 siblings, 0 replies; 31+ messages in thread
From: Nirmoy Das @ 2023-01-17 17:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: Tvrtko Ursulin, Andi Shyti, dri-devel, Nirmoy Das

drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.

Call drm_vma_node_allow_once() instead to prevent that memory leak.

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
 	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
 out:
 	if (file)
-		drm_vma_node_allow(&mmo->vma_node, file);
+		drm_vma_node_allow_once(&mmo->vma_node, file);
 	return mmo;
 
 err:
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
@ 2023-01-17 17:52   ` Nirmoy Das
  0 siblings, 0 replies; 31+ messages in thread
From: Nirmoy Das @ 2023-01-17 17:52 UTC (permalink / raw)
  To: intel-gfx; +Cc: dri-devel, Nirmoy Das

drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.

Call drm_vma_node_allow_once() instead to prevent that memory leak.

Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>

Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
 	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
 out:
 	if (file)
-		drm_vma_node_allow(&mmo->vma_node, file);
+		drm_vma_node_allow_once(&mmo->vma_node, file);
 	return mmo;
 
 err:
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
  (?)
  (?)
@ 2023-01-17 21:56 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2023-01-17 21:56 UTC (permalink / raw)
  To: Das, Nirmoy; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
URL   : https://patchwork.freedesktop.org/series/112952/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12594 -> Patchwork_112952v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/index.html

Participating hosts (42 -> 42)
------------------------------

  Additional (2): bat-rpls-2 fi-skl-6700k2 
  Missing    (2): fi-bsw-kefka fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_112952v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-skl-6700k2:      NOTRUN -> [SKIP][1] ([fdo#109271] / [i915#2190])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-skl-6700k2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-skl-6700k2:      NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#4613]) +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-skl-6700k2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@prime_vgem@basic-userptr:
    - fi-skl-6700k2:      NOTRUN -> [SKIP][3] ([fdo#109271]) +14 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-skl-6700k2/igt@prime_vgem@basic-userptr.html

  
#### Possible fixes ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [FAIL][4] ([i915#7229]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - {bat-rplp-1}:       [DMESG-WARN][6] ([i915#2867]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-n3050:       [FAIL][8] ([i915#6298]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-bsw-n3050/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-lvds-1:
    - fi-ctg-p8600:       [FAIL][10] ([fdo#103375]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/fi-ctg-p8600/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-lvds-1.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/fi-ctg-p8600/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-b-lvds-1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828


Build changes
-------------

  * Linux: CI_DRM_12594 -> Patchwork_112952v1

  CI-20190529: 20190529
  CI_DRM_12594: 5cec9cff5436577179bab7b52de0465ba169691a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7121: aa16e81259f59734230d441905b9d0f605e4a4b5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112952v1: 5cec9cff5436577179bab7b52de0465ba169691a @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

fc11bcdba028 drm/i915: Fix a memory leak with reused mmap_offset
bb1d06ef0b8c drm/drm_vma_manager: Add drm_vma_node_allow_once()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/index.html

[-- Attachment #2: Type: text/html, Size: 5255 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-17 17:52   ` [Intel-gfx] " Nirmoy Das
@ 2023-01-18  9:19     ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18  9:19 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: Andi Shyti, dri-devel, Mirsad Todorovac



Hi,

Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.

Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from https://patchwork.freedesktop.org/series/112952/.

Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.

Finally, let me add some tags below:

On 17/01/2023 17:52, Nirmoy Das wrote:
> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
> on each mmap_offset. As the mmap_offset is reused by the client, the
> per-file vm_count may remain non-zero and the rbtree leaked.
> 
> Call drm_vma_node_allow_once() instead to prevent that memory leak.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>

Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Cc: <stable@vger.kernel.org> # v5.7+
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 4f69bff63068..2aac6bf78740 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>   	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>   out:
>   	if (file)
> -		drm_vma_node_allow(&mmo->vma_node, file);
> +		drm_vma_node_allow_once(&mmo->vma_node, file);
>   	return mmo;
>   
>   err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
@ 2023-01-18  9:19     ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18  9:19 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx; +Cc: dri-devel, Mirsad Todorovac



Hi,

Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.

Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from https://patchwork.freedesktop.org/series/112952/.

Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.

Finally, let me add some tags below:

On 17/01/2023 17:52, Nirmoy Das wrote:
> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
> on each mmap_offset. As the mmap_offset is reused by the client, the
> per-file vm_count may remain non-zero and the rbtree leaked.
> 
> Call drm_vma_node_allow_once() instead to prevent that memory leak.
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>

Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
Cc: <stable@vger.kernel.org> # v5.7+
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

> 
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 4f69bff63068..2aac6bf78740 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>   	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>   out:
>   	if (file)
> -		drm_vma_node_allow(&mmo->vma_node, file);
> +		drm_vma_node_allow_once(&mmo->vma_node, file);
>   	return mmo;
>   
>   err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
@ 2023-01-18  9:22   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18  9:22 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx
  Cc: Chris Wilson, Thomas Zimmermann, dri-devel, Andi Shyti


On 17/01/2023 17:52, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>   include/drm/drm_vma_manager.h     |  1 +
>   2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   }
>   EXPORT_SYMBOL(drm_vma_offset_remove);
>   
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>   {
>   	struct rb_node **iter;
>   	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>   
>   		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>   			goto unlock;
>   		} else if (tag > entry->vm_tag) {
>   			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   	kfree(new);
>   	return ret;
>   }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>   EXPORT_SYMBOL(drm_vma_node_allow);
>   
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>   /**
>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>    * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   			   struct drm_vma_offset_node *node);
>   
>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>   			 struct drm_file *tag);
>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

LGTM,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-18  9:22   ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18  9:22 UTC (permalink / raw)
  To: Nirmoy Das, intel-gfx
  Cc: Chris Wilson, Thomas Zimmermann, Maxime Ripard, dri-devel,
	Daniel Vetter, David Airlie


On 17/01/2023 17:52, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>   include/drm/drm_vma_manager.h     |  1 +
>   2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   }
>   EXPORT_SYMBOL(drm_vma_offset_remove);
>   
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>   {
>   	struct rb_node **iter;
>   	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>   
>   		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>   			goto unlock;
>   		} else if (tag > entry->vm_tag) {
>   			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   	kfree(new);
>   	return ret;
>   }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>   EXPORT_SYMBOL(drm_vma_node_allow);
>   
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>   /**
>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>    * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   			   struct drm_vma_offset_node *node);
>   
>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>   			 struct drm_file *tag);
>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

LGTM,

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18  9:19     ` [Intel-gfx] " Tvrtko Ursulin
@ 2023-01-18  9:27       ` Das, Nirmoy
  -1 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-18  9:27 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: Andi Shyti, dri-devel, Mirsad Todorovac

Hi Tvrtko,

On 1/18/2023 10:19 AM, Tvrtko Ursulin wrote:
>
>
> Hi,
>
> Thanks for working on this, it looks good to me and it aligns with how 
> i915 uses the facility.
>
> Copying Mirsad who reported the issue in case he is still happy to 
> give it a quick test. Mirsad, I don't know if you are subscribed to 
> one of the two mailing lists where series was posted. In case not, you 
> can grab both patches from 
> https://patchwork.freedesktop.org/series/112952/.
>
> Nirmoy - we also have an IGT written by Chuansheng - 
> https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A 
> more generic one could be placed in gem_mmap_offset test but this one 
> works too in my testing and is IMO better than nothing.


This looks good to me. let's get this merge and I can look into 
improving it at later point.

>
> Finally, let me add some tags below:
>
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>> on each mmap_offset. As the mmap_offset is reused by the client, the
>> per-file vm_count may remain non-zero and the rbtree leaked.
>>
>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree 
> rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>


Thanks for your review and those extra tags.


Nirmoy

>
> Regards,
>
> Tvrtko
>
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 4f69bff63068..2aac6bf78740 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>   out:
>>       if (file)
>> -        drm_vma_node_allow(&mmo->vma_node, file);
>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>       return mmo;
>>     err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
@ 2023-01-18  9:27       ` Das, Nirmoy
  0 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-18  9:27 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx; +Cc: dri-devel, Mirsad Todorovac

Hi Tvrtko,

On 1/18/2023 10:19 AM, Tvrtko Ursulin wrote:
>
>
> Hi,
>
> Thanks for working on this, it looks good to me and it aligns with how 
> i915 uses the facility.
>
> Copying Mirsad who reported the issue in case he is still happy to 
> give it a quick test. Mirsad, I don't know if you are subscribed to 
> one of the two mailing lists where series was posted. In case not, you 
> can grab both patches from 
> https://patchwork.freedesktop.org/series/112952/.
>
> Nirmoy - we also have an IGT written by Chuansheng - 
> https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A 
> more generic one could be placed in gem_mmap_offset test but this one 
> works too in my testing and is IMO better than nothing.


This looks good to me. let's get this merge and I can look into 
improving it at later point.

>
> Finally, let me add some tags below:
>
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>> on each mmap_offset. As the mmap_offset is reused by the client, the
>> per-file vm_count may remain non-zero and the rbtree leaked.
>>
>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree 
> rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>


Thanks for your review and those extra tags.


Nirmoy

>
> Regards,
>
> Tvrtko
>
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 4f69bff63068..2aac6bf78740 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>   out:
>>       if (file)
>> -        drm_vma_node_allow(&mmo->vma_node, file);
>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>       return mmo;
>>     err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
@ 2023-01-18  9:38   ` Andi Shyti
  -1 siblings, 0 replies; 31+ messages in thread
From: Andi Shyti @ 2023-01-18  9:38 UTC (permalink / raw)
  To: Nirmoy Das
  Cc: Chris Wilson, Daniel Vetter, intel-gfx, Maxime Ripard, dri-devel,
	Thomas Zimmermann, David Airlie

On Tue, Jan 17, 2023 at 06:52:35PM +0100, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 

Next time, please, don't leave any spaces between tags.

> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

> ---
>  drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>  include/drm/drm_vma_manager.h     |  1 +
>  2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>  }
>  EXPORT_SYMBOL(drm_vma_offset_remove);
>  
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>  {
>  	struct rb_node **iter;
>  	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>  		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>  
>  		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>  			goto unlock;
>  		} else if (tag > entry->vm_tag) {
>  			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>  	kfree(new);
>  	return ret;
>  }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>  EXPORT_SYMBOL(drm_vma_node_allow);
>  
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>  /**
>   * drm_vma_node_revoke - Remove open-file from list of allowed users
>   * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>  			   struct drm_vma_offset_node *node);
>  
>  int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>  void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>  			 struct drm_file *tag);
>  bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
> -- 
> 2.39.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-18  9:38   ` Andi Shyti
  0 siblings, 0 replies; 31+ messages in thread
From: Andi Shyti @ 2023-01-18  9:38 UTC (permalink / raw)
  To: Nirmoy Das
  Cc: Tvrtko Ursulin, Chris Wilson, intel-gfx, dri-devel,
	Thomas Zimmermann, Andi Shyti

On Tue, Jan 17, 2023 at 06:52:35PM +0100, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 

Next time, please, don't leave any spaces between tags.

> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

> ---
>  drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>  include/drm/drm_vma_manager.h     |  1 +
>  2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>  }
>  EXPORT_SYMBOL(drm_vma_offset_remove);
>  
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>  {
>  	struct rb_node **iter;
>  	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>  		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>  
>  		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>  			goto unlock;
>  		} else if (tag > entry->vm_tag) {
>  			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>  	kfree(new);
>  	return ret;
>  }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>  EXPORT_SYMBOL(drm_vma_node_allow);
>  
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>  /**
>   * drm_vma_node_revoke - Remove open-file from list of allowed users
>   * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>  			   struct drm_vma_offset_node *node);
>  
>  int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>  void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>  			 struct drm_file *tag);
>  bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
> -- 
> 2.39.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18  9:19     ` [Intel-gfx] " Tvrtko Ursulin
@ 2023-01-18  9:40       ` Andi Shyti
  -1 siblings, 0 replies; 31+ messages in thread
From: Andi Shyti @ 2023-01-18  9:40 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: intel-gfx, Mirsad Todorovac, Andi Shyti, dri-devel, Nirmoy Das

Hi,

On Wed, Jan 18, 2023 at 09:19:40AM +0000, Tvrtko Ursulin wrote:
> 
> 
> Hi,
> 
> Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.
> 
> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from https://patchwork.freedesktop.org/series/112952/.
> 
> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
> 
> Finally, let me add some tags below:
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
> > drm_vma_node_allow() and drm_vma_node_revoke() should be called in
> > balanced pairs. We call drm_vma_node_allow() once per-file everytime a
> > user calls mmap_offset, but only call drm_vma_node_revoke once per-file
> > on each mmap_offset. As the mmap_offset is reused by the client, the
> > per-file vm_count may remain non-zero and the rbtree leaked.
> > 
> > Call drm_vma_node_allow_once() instead to prevent that memory leak.
> > 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Nirmoy, you can also add:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Will this go through the drm branch?

Andi

> Regards,
> 
> Tvrtko
> 
> > 
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 4f69bff63068..2aac6bf78740 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
> >   	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
> >   out:
> >   	if (file)
> > -		drm_vma_node_allow(&mmo->vma_node, file);
> > +		drm_vma_node_allow_once(&mmo->vma_node, file);
> >   	return mmo;
> >   err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
@ 2023-01-18  9:40       ` Andi Shyti
  0 siblings, 0 replies; 31+ messages in thread
From: Andi Shyti @ 2023-01-18  9:40 UTC (permalink / raw)
  To: Tvrtko Ursulin; +Cc: intel-gfx, Mirsad Todorovac, dri-devel, Nirmoy Das

Hi,

On Wed, Jan 18, 2023 at 09:19:40AM +0000, Tvrtko Ursulin wrote:
> 
> 
> Hi,
> 
> Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.
> 
> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from https://patchwork.freedesktop.org/series/112952/.
> 
> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
> 
> Finally, let me add some tags below:
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
> > drm_vma_node_allow() and drm_vma_node_revoke() should be called in
> > balanced pairs. We call drm_vma_node_allow() once per-file everytime a
> > user calls mmap_offset, but only call drm_vma_node_revoke once per-file
> > on each mmap_offset. As the mmap_offset is reused by the client, the
> > per-file vm_count may remain non-zero and the rbtree leaked.
> > 
> > Call drm_vma_node_allow_once() instead to prevent that memory leak.
> > 
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Nirmoy, you can also add:

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Will this go through the drm branch?

Andi

> Regards,
> 
> Tvrtko
> 
> > 
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 4f69bff63068..2aac6bf78740 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
> >   	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
> >   out:
> >   	if (file)
> > -		drm_vma_node_allow(&mmo->vma_node, file);
> > +		drm_vma_node_allow_once(&mmo->vma_node, file);
> >   	return mmo;
> >   err:

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-18  9:38   ` Andi Shyti
@ 2023-01-18  9:45     ` Das, Nirmoy
  -1 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-18  9:45 UTC (permalink / raw)
  To: Andi Shyti, Nirmoy Das
  Cc: Chris Wilson, intel-gfx, dri-devel, Thomas Zimmermann


On 1/18/2023 10:38 AM, Andi Shyti wrote:
> On Tue, Jan 17, 2023 at 06:52:35PM +0100, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>
> Next time, please, don't leave any spaces between tags.


I will keep that in my mind.

>
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>


Thanks,

Nirmoy

>
> Thanks,
> Andi
>
>> ---
>>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>>   include/drm/drm_vma_manager.h     |  1 +
>>   2 files changed, 55 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
>> index 7de37f8c68fd..83229a031af0 100644
>> --- a/drivers/gpu/drm/drm_vma_manager.c
>> +++ b/drivers/gpu/drm/drm_vma_manager.c
>> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>>   }
>>   EXPORT_SYMBOL(drm_vma_offset_remove);
>>   
>> -/**
>> - * drm_vma_node_allow - Add open-file to list of allowed users
>> - * @node: Node to modify
>> - * @tag: Tag of file to remove
>> - *
>> - * Add @tag to the list of allowed open-files for this node. If @tag is
>> - * already on this list, the ref-count is incremented.
>> - *
>> - * The list of allowed-users is preserved across drm_vma_offset_add() and
>> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> - * not added to any offset-manager.
>> - *
>> - * You must remove all open-files the same number of times as you added them
>> - * before destroying the node. Otherwise, you will leak memory.
>> - *
>> - * This is locked against concurrent access internally.
>> - *
>> - * RETURNS:
>> - * 0 on success, negative error code on internal failure (out-of-mem)
>> - */
>> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +static int vma_node_allow(struct drm_vma_offset_node *node,
>> +			  struct drm_file *tag, bool ref_counted)
>>   {
>>   	struct rb_node **iter;
>>   	struct rb_node *parent = NULL;
>> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>>   
>>   		if (tag == entry->vm_tag) {
>> -			entry->vm_count++;
>> +			if (ref_counted)
>> +				entry->vm_count++;
>>   			goto unlock;
>>   		} else if (tag > entry->vm_tag) {
>>   			iter = &(*iter)->rb_right;
>> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>>   	kfree(new);
>>   	return ret;
>>   }
>> +
>> +/**
>> + * drm_vma_node_allow - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node. If @tag is
>> + * already on this list, the ref-count is incremented.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> + * not added to any offset-manager.
>> + *
>> + * You must remove all open-files the same number of times as you added them
>> + * before destroying the node. Otherwise, you will leak memory.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +{
>> +	return vma_node_allow(node, tag, true);
>> +}
>>   EXPORT_SYMBOL(drm_vma_node_allow);
>>   
>> +/**
>> + * drm_vma_node_allow_once - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> + * not added to any offset-manager.
>> + *
>> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
>> + * should only be called once after this.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +{
>> +	return vma_node_allow(node, tag, false);
>> +}
>> +EXPORT_SYMBOL(drm_vma_node_allow_once);
>> +
>>   /**
>>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>>    * @node: Node to modify
>> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
>> index 4f8c35206f7c..6c2a2f21dbf0 100644
>> --- a/include/drm/drm_vma_manager.h
>> +++ b/include/drm/drm_vma_manager.h
>> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>>   			   struct drm_vma_offset_node *node);
>>   
>>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>>   			 struct drm_file *tag);
>>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
>> -- 
>> 2.39.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-18  9:45     ` Das, Nirmoy
  0 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-18  9:45 UTC (permalink / raw)
  To: Andi Shyti, Nirmoy Das
  Cc: Chris Wilson, Daniel Vetter, intel-gfx, Maxime Ripard, dri-devel,
	Thomas Zimmermann, David Airlie


On 1/18/2023 10:38 AM, Andi Shyti wrote:
> On Tue, Jan 17, 2023 at 06:52:35PM +0100, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>
> Next time, please, don't leave any spaces between tags.


I will keep that in my mind.

>
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>


Thanks,

Nirmoy

>
> Thanks,
> Andi
>
>> ---
>>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>>   include/drm/drm_vma_manager.h     |  1 +
>>   2 files changed, 55 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
>> index 7de37f8c68fd..83229a031af0 100644
>> --- a/drivers/gpu/drm/drm_vma_manager.c
>> +++ b/drivers/gpu/drm/drm_vma_manager.c
>> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>>   }
>>   EXPORT_SYMBOL(drm_vma_offset_remove);
>>   
>> -/**
>> - * drm_vma_node_allow - Add open-file to list of allowed users
>> - * @node: Node to modify
>> - * @tag: Tag of file to remove
>> - *
>> - * Add @tag to the list of allowed open-files for this node. If @tag is
>> - * already on this list, the ref-count is incremented.
>> - *
>> - * The list of allowed-users is preserved across drm_vma_offset_add() and
>> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> - * not added to any offset-manager.
>> - *
>> - * You must remove all open-files the same number of times as you added them
>> - * before destroying the node. Otherwise, you will leak memory.
>> - *
>> - * This is locked against concurrent access internally.
>> - *
>> - * RETURNS:
>> - * 0 on success, negative error code on internal failure (out-of-mem)
>> - */
>> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +static int vma_node_allow(struct drm_vma_offset_node *node,
>> +			  struct drm_file *tag, bool ref_counted)
>>   {
>>   	struct rb_node **iter;
>>   	struct rb_node *parent = NULL;
>> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>>   
>>   		if (tag == entry->vm_tag) {
>> -			entry->vm_count++;
>> +			if (ref_counted)
>> +				entry->vm_count++;
>>   			goto unlock;
>>   		} else if (tag > entry->vm_tag) {
>>   			iter = &(*iter)->rb_right;
>> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>>   	kfree(new);
>>   	return ret;
>>   }
>> +
>> +/**
>> + * drm_vma_node_allow - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node. If @tag is
>> + * already on this list, the ref-count is incremented.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> + * not added to any offset-manager.
>> + *
>> + * You must remove all open-files the same number of times as you added them
>> + * before destroying the node. Otherwise, you will leak memory.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +{
>> +	return vma_node_allow(node, tag, true);
>> +}
>>   EXPORT_SYMBOL(drm_vma_node_allow);
>>   
>> +/**
>> + * drm_vma_node_allow_once - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
>> + * not added to any offset-manager.
>> + *
>> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
>> + * should only be called once after this.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
>> +{
>> +	return vma_node_allow(node, tag, false);
>> +}
>> +EXPORT_SYMBOL(drm_vma_node_allow_once);
>> +
>>   /**
>>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>>    * @node: Node to modify
>> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
>> index 4f8c35206f7c..6c2a2f21dbf0 100644
>> --- a/include/drm/drm_vma_manager.h
>> +++ b/include/drm/drm_vma_manager.h
>> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>>   			   struct drm_vma_offset_node *node);
>>   
>>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>>   			 struct drm_file *tag);
>>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
>> -- 
>> 2.39.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18  9:19     ` [Intel-gfx] " Tvrtko Ursulin
@ 2023-01-18 10:26       ` Mirsad Todorovac
  -1 siblings, 0 replies; 31+ messages in thread
From: Mirsad Todorovac @ 2023-01-18 10:26 UTC (permalink / raw)
  To: Tvrtko Ursulin, Nirmoy Das, intel-gfx; +Cc: dri-devel

Hi,

On 1/18/23 10:19, Tvrtko Ursulin wrote:

> Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.
> 
> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed 
> to one of the two mailing lists where series was posted. In case not, you can grab both patches from 
> https://patchwork.freedesktop.org/series/112952/.
> 
> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more 
> generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
> 
> Finally, let me add some tags below:
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>> on each mmap_offset. As the mmap_offset is reused by the client, the
>> per-file vm_count may remain non-zero and the rbtree leaked.
>>
>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
> 
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 4f69bff63068..2aac6bf78740 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>   out:
>>       if (file)
>> -        drm_vma_node_allow(&mmo->vma_node, file);
>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>       return mmo;
>>   err:

The drm/i915 patch seems OK and there are currently no memory leaks as of
reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
the initial bug ...

Will post you if there are any changes.

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
@ 2023-01-18 10:26       ` Mirsad Todorovac
  0 siblings, 0 replies; 31+ messages in thread
From: Mirsad Todorovac @ 2023-01-18 10:26 UTC (permalink / raw)
  To: Tvrtko Ursulin, Nirmoy Das, intel-gfx; +Cc: Andi Shyti, dri-devel

Hi,

On 1/18/23 10:19, Tvrtko Ursulin wrote:

> Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.
> 
> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed 
> to one of the two mailing lists where series was posted. In case not, you can grab both patches from 
> https://patchwork.freedesktop.org/series/112952/.
> 
> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more 
> generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
> 
> Finally, let me add some tags below:
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>> on each mmap_offset. As the mmap_offset is reused by the client, the
>> per-file vm_count may remain non-zero and the rbtree leaked.
>>
>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> Cc: <stable@vger.kernel.org> # v5.7+
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
> 
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 4f69bff63068..2aac6bf78740 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>   out:
>>       if (file)
>> -        drm_vma_node_allow(&mmo->vma_node, file);
>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>       return mmo;
>>   err:

The drm/i915 patch seems OK and there are currently no memory leaks as of
reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
the initial bug ...

Will post you if there are any changes.

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18 10:26       ` Mirsad Todorovac
  (?)
@ 2023-01-18 10:39       ` Das, Nirmoy
  2023-01-18 10:53         ` Mirsad Todorovac
  2023-01-18 22:27         ` Mirsad Goran Todorovac
  -1 siblings, 2 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-18 10:39 UTC (permalink / raw)
  To: Mirsad Todorovac, Tvrtko Ursulin, Nirmoy Das, intel-gfx; +Cc: dri-devel


On 1/18/2023 11:26 AM, Mirsad Todorovac wrote:
> Hi,
>
> On 1/18/23 10:19, Tvrtko Ursulin wrote:
>
>> Thanks for working on this, it looks good to me and it aligns with 
>> how i915 uses the facility.
>>
>> Copying Mirsad who reported the issue in case he is still happy to 
>> give it a quick test. Mirsad, I don't know if you are subscribed to 
>> one of the two mailing lists where series was posted. In case not, 
>> you can grab both patches from 
>> https://patchwork.freedesktop.org/series/112952/.
>>
>> Nirmoy - we also have an IGT written by Chuansheng - 
>> https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. 
>> A more generic one could be placed in gem_mmap_offset test but this 
>> one works too in my testing and is IMO better than nothing.
>>
>> Finally, let me add some tags below:
>>
>> On 17/01/2023 17:52, Nirmoy Das wrote:
>>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>>> on each mmap_offset. As the mmap_offset is reused by the client, the
>>> per-file vm_count may remain non-zero and the rbtree leaked.
>>>
>>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree 
>> rather than a plain list")
>> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
>> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
>> Cc: <stable@vger.kernel.org> # v5.7+
>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> index 4f69bff63068..2aac6bf78740 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>>   out:
>>>       if (file)
>>> -        drm_vma_node_allow(&mmo->vma_node, file);
>>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>>       return mmo;
>>>   err:
>
> The drm/i915 patch seems OK and there are currently no memory leaks as of
> reported by /sys/kernel/debug/kmemleak under the same Chrome load that 
> triggered
> the initial bug ...


Thanks, Mirsad for quickly checking this!


Nirmoy

>
> Will post you if there are any changes.
>
> Regards,
> Mirsad
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18 10:39       ` [Intel-gfx] " Das, Nirmoy
@ 2023-01-18 10:53         ` Mirsad Todorovac
  2023-01-18 22:27         ` Mirsad Goran Todorovac
  1 sibling, 0 replies; 31+ messages in thread
From: Mirsad Todorovac @ 2023-01-18 10:53 UTC (permalink / raw)
  To: Das, Nirmoy, Tvrtko Ursulin, Nirmoy Das, intel-gfx; +Cc: dri-devel

Hi,

On 1/18/23 11:39, Das, Nirmoy wrote:
> 
> On 1/18/2023 11:26 AM, Mirsad Todorovac wrote:
>> Hi,
>>
>> On 1/18/23 10:19, Tvrtko Ursulin wrote:
>>
>>> Thanks for working on this, it looks good to me and it aligns with how i915 uses the facility.
>>>
>>> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are 
>>> subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from 
>>> https://patchwork.freedesktop.org/series/112952/.
>>>
>>> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more 
>>> generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
>>>
>>> Finally, let me add some tags below:
>>>
>>> On 17/01/2023 17:52, Nirmoy Das wrote:
>>>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>>>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>>>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>>>> on each mmap_offset. As the mmap_offset is reused by the client, the
>>>> per-file vm_count may remain non-zero and the rbtree leaked.
>>>>
>>>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>>>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>>
>>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
>>> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
>>> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
>>> Cc: <stable@vger.kernel.org> # v5.7+
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> index 4f69bff63068..2aac6bf78740 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>>>   out:
>>>>       if (file)
>>>> -        drm_vma_node_allow(&mmo->vma_node, file);
>>>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>>>       return mmo;
>>>>   err:
>>
>> The drm/i915 patch seems OK and there are currently no memory leaks as of
>> reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
>> the initial bug ...
> 
> 
> Thanks, Mirsad for quickly checking this!

There was no problem, Nirmoy, everything applied neatly :)

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu

System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
                   ` (4 preceding siblings ...)
  (?)
@ 2023-01-18 11:07 ` Patchwork
  -1 siblings, 0 replies; 31+ messages in thread
From: Patchwork @ 2023-01-18 11:07 UTC (permalink / raw)
  To: Das, Nirmoy; +Cc: intel-gfx

[-- Attachment #1: Type: text/plain, Size: 20026 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
URL   : https://patchwork.freedesktop.org/series/112952/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12594_full -> Patchwork_112952v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/index.html

Participating hosts (13 -> 11)
------------------------------

  Additional (1): shard-rkl0 
  Missing    (3): pig-skl-6260u pig-kbl-iris pig-glk-j5005 

Known issues
------------

  Here are the changes found in Patchwork_112952v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][1] -> [FAIL][2] ([i915#2842]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-glk6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#79])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-glk3/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-pri-indfb-draw-mmap-gtt:
    - shard-glk:          NOTRUN -> [SKIP][5] ([fdo#109271]) +24 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-glk9/igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-pri-indfb-draw-mmap-gtt.html

  
#### Possible fixes ####

  * igt@drm_read@fault-buffer:
    - {shard-rkl}:        [SKIP][6] ([i915#4098]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@drm_read@fault-buffer.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@drm_read@fault-buffer.html

  * igt@fbdev@eof:
    - {shard-rkl}:        [SKIP][8] ([i915#2582]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@fbdev@eof.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@fbdev@eof.html

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - {shard-rkl}:        [SKIP][10] ([fdo#109313]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-6/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-5/igt@gem_exec_flush@basic-batch-kernel-default-cmd.html

  * igt@gem_exec_reloc@basic-wc-read-noreloc:
    - {shard-rkl}:        [SKIP][12] ([i915#3281]) -> [PASS][13] +4 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@gem_exec_reloc@basic-wc-read-noreloc.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-5/igt@gem_exec_reloc@basic-wc-read-noreloc.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [DMESG-WARN][14] ([i915#5566] / [i915#716]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-glk8/igt@gen9_exec_parse@allowed-all.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-glk9/igt@gen9_exec_parse@allowed-all.html

  * igt@gen9_exec_parse@valid-registers:
    - {shard-rkl}:        [SKIP][16] ([i915#2527]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@gen9_exec_parse@valid-registers.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-5/igt@gen9_exec_parse@valid-registers.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a:
    - {shard-dg1}:        [SKIP][18] ([i915#1937]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-dg1-15/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-dg1-14/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html

  * igt@i915_pm_rc6_residency@rc6-idle@rcs0:
    - {shard-dg1}:        [FAIL][20] ([i915#3591]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-dg1-16/igt@i915_pm_rc6_residency@rc6-idle@rcs0.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-dg1-12/igt@i915_pm_rc6_residency@rc6-idle@rcs0.html

  * igt@i915_pm_rpm@i2c:
    - {shard-rkl}:        [SKIP][22] ([fdo#109308]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@i915_pm_rpm@i2c.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@i915_pm_rpm@i2c.html

  * igt@i915_pm_rpm@modeset-lpsp-stress:
    - {shard-tglu}:       [SKIP][24] ([i915#1397]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@i915_pm_rpm@modeset-lpsp-stress.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@i915_pm_rpm@modeset-lpsp-stress.html

  * igt@i915_suspend@basic-s3-without-i915:
    - {shard-rkl}:        [FAIL][26] ([fdo#103375]) -> [PASS][27]
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@i915_suspend@basic-s3-without-i915.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-5/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_big_fb@y-tiled-8bpp-rotate-180:
    - {shard-rkl}:        [SKIP][28] ([i915#1845] / [i915#4098]) -> [PASS][29] +11 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@kms_big_fb@y-tiled-8bpp-rotate-180.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_big_fb@y-tiled-8bpp-rotate-180.html

  * igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip:
    - {shard-tglu}:       [SKIP][30] ([i915#7651]) -> [PASS][31] +15 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@kms_big_fb@y-tiled-max-hw-stride-32bpp-rotate-0-hflip.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
    - {shard-rkl}:        [SKIP][32] ([i915#1849] / [i915#4098]) -> [PASS][33] +13 similar issues
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt:
    - {shard-tglu}:       [SKIP][34] ([i915#1849]) -> [PASS][35] +7 similar issues
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html

  * igt@kms_plane@pixel-format@pipe-b-planes:
    - {shard-rkl}:        [SKIP][36] ([i915#1849]) -> [PASS][37] +1 similar issue
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@kms_plane@pixel-format@pipe-b-planes.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_plane@pixel-format@pipe-b-planes.html

  * igt@kms_psr@cursor_render:
    - {shard-rkl}:        [SKIP][38] ([i915#1072]) -> [PASS][39] +1 similar issue
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@kms_psr@cursor_render.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_psr@cursor_render.html

  * igt@kms_psr_stress_test@flip-primary-invalidate-overlay:
    - {shard-rkl}:        [SKIP][40] ([i915#5461]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-3/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html

  * igt@kms_rotation_crc@cursor-rotation-180:
    - {shard-tglu}:       [SKIP][42] ([i915#1845]) -> [PASS][43]
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@kms_rotation_crc@cursor-rotation-180.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@kms_rotation_crc@cursor-rotation-180.html

  * igt@kms_universal_plane@disable-primary-vs-flip-pipe-a:
    - {shard-tglu}:       [SKIP][44] ([fdo#109274]) -> [PASS][45] +2 similar issues
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html

  * igt@kms_universal_plane@universal-plane-pipe-a-sanity:
    - {shard-rkl}:        [SKIP][46] ([i915#1845] / [i915#4070] / [i915#4098]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-2/igt@kms_universal_plane@universal-plane-pipe-a-sanity.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-6/igt@kms_universal_plane@universal-plane-pipe-a-sanity.html

  * igt@kms_vblank@pipe-c-wait-forked:
    - {shard-tglu}:       [SKIP][48] ([i915#1845] / [i915#7651]) -> [PASS][49] +5 similar issues
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-tglu-6/igt@kms_vblank@pipe-c-wait-forked.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-tglu-3/igt@kms_vblank@pipe-c-wait-forked.html

  * igt@perf@mi-rpc:
    - {shard-rkl}:        [SKIP][50] ([i915#2434]) -> [PASS][51]
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-rkl-6/igt@perf@mi-rpc.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-rkl-5/igt@perf@mi-rpc.html

  * igt@sysfs_heartbeat_interval@precise@vcs1:
    - {shard-dg1}:        [FAIL][52] ([i915#1755]) -> [PASS][53] +1 similar issue
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12594/shard-dg1-16/igt@sysfs_heartbeat_interval@precise@vcs1.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/shard-dg1-15/igt@sysfs_heartbeat_interval@precise@vcs1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
  [fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
  [fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#110542]: https://bugs.freedesktop.org/show_bug.cgi?id=110542
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111644]: https://bugs.freedesktop.org/show_bug.cgi?id=111644
  [fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
  [i915#1755]: https://gitlab.freedesktop.org/drm/intel/issues/1755
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#1937]: https://gitlab.freedesktop.org/drm/intel/issues/1937
  [i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
  [i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
  [i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
  [i915#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#5030]: https://gitlab.freedesktop.org/drm/intel/issues/5030
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5327]: https://gitlab.freedesktop.org/drm/intel/issues/5327
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6117]: https://gitlab.freedesktop.org/drm/intel/issues/6117
  [i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
  [i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
  [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#6334]: https://gitlab.freedesktop.org/drm/intel/issues/6334
  [i915#6412]: https://gitlab.freedesktop.org/drm/intel/issues/6412
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7037]: https://gitlab.freedesktop.org/drm/intel/issues/7037
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
  [i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7582]: https://gitlab.freedesktop.org/drm/intel/issues/7582
  [i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7707]: https://gitlab.freedesktop.org/drm/intel/issues/7707
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79


Build changes
-------------

  * Linux: CI_DRM_12594 -> Patchwork_112952v1
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_12594: 5cec9cff5436577179bab7b52de0465ba169691a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7121: aa16e81259f59734230d441905b9d0f605e4a4b5 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112952v1: 5cec9cff5436577179bab7b52de0465ba169691a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112952v1/index.html

[-- Attachment #2: Type: text/html, Size: 15124 bytes --]

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
@ 2023-01-18 13:19   ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18 13:19 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, dri-devel
  Cc: Andi Shyti, intel-gfx, Chris Wilson, Thomas Zimmermann, Nirmoy Das


Hi Dave & Daniel,

On 17/01/2023 17:52, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>

Okay to take this via drm-intel?

Do we need an additional r-b from the DRM core side?

Regards,

Tvrtko

> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>   include/drm/drm_vma_manager.h     |  1 +
>   2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   }
>   EXPORT_SYMBOL(drm_vma_offset_remove);
>   
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>   {
>   	struct rb_node **iter;
>   	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>   
>   		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>   			goto unlock;
>   		} else if (tag > entry->vm_tag) {
>   			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   	kfree(new);
>   	return ret;
>   }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>   EXPORT_SYMBOL(drm_vma_node_allow);
>   
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>   /**
>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>    * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   			   struct drm_vma_offset_node *node);
>   
>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>   			 struct drm_file *tag);
>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-18 13:19   ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18 13:19 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, dri-devel
  Cc: intel-gfx, Chris Wilson, Maxime Ripard, Thomas Zimmermann, Nirmoy Das


Hi Dave & Daniel,

On 17/01/2023 17:52, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>

Okay to take this via drm-intel?

Do we need an additional r-b from the DRM core side?

Regards,

Tvrtko

> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>   include/drm/drm_vma_manager.h     |  1 +
>   2 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 7de37f8c68fd..83229a031af0 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   }
>   EXPORT_SYMBOL(drm_vma_offset_remove);
>   
> -/**
> - * drm_vma_node_allow - Add open-file to list of allowed users
> - * @node: Node to modify
> - * @tag: Tag of file to remove
> - *
> - * Add @tag to the list of allowed open-files for this node. If @tag is
> - * already on this list, the ref-count is incremented.
> - *
> - * The list of allowed-users is preserved across drm_vma_offset_add() and
> - * drm_vma_offset_remove() calls. You may even call it if the node is currently
> - * not added to any offset-manager.
> - *
> - * You must remove all open-files the same number of times as you added them
> - * before destroying the node. Otherwise, you will leak memory.
> - *
> - * This is locked against concurrent access internally.
> - *
> - * RETURNS:
> - * 0 on success, negative error code on internal failure (out-of-mem)
> - */
> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +static int vma_node_allow(struct drm_vma_offset_node *node,
> +			  struct drm_file *tag, bool ref_counted)
>   {
>   	struct rb_node **iter;
>   	struct rb_node *parent = NULL;
> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   		entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>   
>   		if (tag == entry->vm_tag) {
> -			entry->vm_count++;
> +			if (ref_counted)
> +				entry->vm_count++;
>   			goto unlock;
>   		} else if (tag > entry->vm_tag) {
>   			iter = &(*iter)->rb_right;
> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
>   	kfree(new);
>   	return ret;
>   }
> +
> +/**
> + * drm_vma_node_allow - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node. If @tag is
> + * already on this list, the ref-count is incremented.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * You must remove all open-files the same number of times as you added them
> + * before destroying the node. Otherwise, you will leak memory.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, true);
> +}
>   EXPORT_SYMBOL(drm_vma_node_allow);
>   
> +/**
> + * drm_vma_node_allow_once - Add open-file to list of allowed users
> + * @node: Node to modify
> + * @tag: Tag of file to remove
> + *
> + * Add @tag to the list of allowed open-files for this node.
> + *
> + * The list of allowed-users is preserved across drm_vma_offset_add() and
> + * drm_vma_offset_remove() calls. You may even call it if the node is currently
> + * not added to any offset-manager.
> + *
> + * This is not ref-counted unlike drm_vma_node_allow() hence drm_vma_node_revoke()
> + * should only be called once after this.
> + *
> + * This is locked against concurrent access internally.
> + *
> + * RETURNS:
> + * 0 on success, negative error code on internal failure (out-of-mem)
> + */
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag)
> +{
> +	return vma_node_allow(node, tag, false);
> +}
> +EXPORT_SYMBOL(drm_vma_node_allow_once);
> +
>   /**
>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>    * @node: Node to modify
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4f8c35206f7c..6c2a2f21dbf0 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>   			   struct drm_vma_offset_node *node);
>   
>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct drm_file *tag);
> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct drm_file *tag);
>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>   			 struct drm_file *tag);
>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-18 13:19   ` [Intel-gfx] " Tvrtko Ursulin
@ 2023-01-18 13:34     ` Tvrtko Ursulin
  -1 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18 13:34 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, dri-devel
  Cc: Andi Shyti, intel-gfx, Chris Wilson, Thomas Zimmermann, Nirmoy Das


On 18/01/2023 13:19, Tvrtko Ursulin wrote:
> 
> Hi Dave & Daniel,
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Okay to take this via drm-intel?

Or both patches via drm-misc-fixes so that we avoid drm-intel fixes flow?

Regards,

Tvrtko

> Do we need an additional r-b from the DRM core side?
> 
> Regards,
> 
> Tvrtko
> 
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>>   include/drm/drm_vma_manager.h     |  1 +
>>   2 files changed, 55 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vma_manager.c 
>> b/drivers/gpu/drm/drm_vma_manager.c
>> index 7de37f8c68fd..83229a031af0 100644
>> --- a/drivers/gpu/drm/drm_vma_manager.c
>> +++ b/drivers/gpu/drm/drm_vma_manager.c
>> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct 
>> drm_vma_offset_manager *mgr,
>>   }
>>   EXPORT_SYMBOL(drm_vma_offset_remove);
>> -/**
>> - * drm_vma_node_allow - Add open-file to list of allowed users
>> - * @node: Node to modify
>> - * @tag: Tag of file to remove
>> - *
>> - * Add @tag to the list of allowed open-files for this node. If @tag is
>> - * already on this list, the ref-count is incremented.
>> - *
>> - * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> - * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> - * not added to any offset-manager.
>> - *
>> - * You must remove all open-files the same number of times as you 
>> added them
>> - * before destroying the node. Otherwise, you will leak memory.
>> - *
>> - * This is locked against concurrent access internally.
>> - *
>> - * RETURNS:
>> - * 0 on success, negative error code on internal failure (out-of-mem)
>> - */
>> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +static int vma_node_allow(struct drm_vma_offset_node *node,
>> +              struct drm_file *tag, bool ref_counted)
>>   {
>>       struct rb_node **iter;
>>       struct rb_node *parent = NULL;
>> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node 
>> *node, struct drm_file *tag)
>>           entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>>           if (tag == entry->vm_tag) {
>> -            entry->vm_count++;
>> +            if (ref_counted)
>> +                entry->vm_count++;
>>               goto unlock;
>>           } else if (tag > entry->vm_tag) {
>>               iter = &(*iter)->rb_right;
>> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node 
>> *node, struct drm_file *tag)
>>       kfree(new);
>>       return ret;
>>   }
>> +
>> +/**
>> + * drm_vma_node_allow - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node. If @tag is
>> + * already on this list, the ref-count is incremented.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> + * not added to any offset-manager.
>> + *
>> + * You must remove all open-files the same number of times as you 
>> added them
>> + * before destroying the node. Otherwise, you will leak memory.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +{
>> +    return vma_node_allow(node, tag, true);
>> +}
>>   EXPORT_SYMBOL(drm_vma_node_allow);
>> +/**
>> + * drm_vma_node_allow_once - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> + * not added to any offset-manager.
>> + *
>> + * This is not ref-counted unlike drm_vma_node_allow() hence 
>> drm_vma_node_revoke()
>> + * should only be called once after this.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +{
>> +    return vma_node_allow(node, tag, false);
>> +}
>> +EXPORT_SYMBOL(drm_vma_node_allow_once);
>> +
>>   /**
>>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>>    * @node: Node to modify
>> diff --git a/include/drm/drm_vma_manager.h 
>> b/include/drm/drm_vma_manager.h
>> index 4f8c35206f7c..6c2a2f21dbf0 100644
>> --- a/include/drm/drm_vma_manager.h
>> +++ b/include/drm/drm_vma_manager.h
>> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct 
>> drm_vma_offset_manager *mgr,
>>                  struct drm_vma_offset_node *node);
>>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag);
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct 
>> drm_file *tag);
>>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>>                struct drm_file *tag);
>>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-18 13:34     ` Tvrtko Ursulin
  0 siblings, 0 replies; 31+ messages in thread
From: Tvrtko Ursulin @ 2023-01-18 13:34 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, dri-devel
  Cc: intel-gfx, Chris Wilson, Maxime Ripard, Thomas Zimmermann, Nirmoy Das


On 18/01/2023 13:19, Tvrtko Ursulin wrote:
> 
> Hi Dave & Daniel,
> 
> On 17/01/2023 17:52, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Okay to take this via drm-intel?

Or both patches via drm-misc-fixes so that we avoid drm-intel fixes flow?

Regards,

Tvrtko

> Do we need an additional r-b from the DRM core side?
> 
> Regards,
> 
> Tvrtko
> 
>> Suggested-by: Chris Wilson <chris.p.wilson@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/drm_vma_manager.c | 76 ++++++++++++++++++++++---------
>>   include/drm/drm_vma_manager.h     |  1 +
>>   2 files changed, 55 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vma_manager.c 
>> b/drivers/gpu/drm/drm_vma_manager.c
>> index 7de37f8c68fd..83229a031af0 100644
>> --- a/drivers/gpu/drm/drm_vma_manager.c
>> +++ b/drivers/gpu/drm/drm_vma_manager.c
>> @@ -240,27 +240,8 @@ void drm_vma_offset_remove(struct 
>> drm_vma_offset_manager *mgr,
>>   }
>>   EXPORT_SYMBOL(drm_vma_offset_remove);
>> -/**
>> - * drm_vma_node_allow - Add open-file to list of allowed users
>> - * @node: Node to modify
>> - * @tag: Tag of file to remove
>> - *
>> - * Add @tag to the list of allowed open-files for this node. If @tag is
>> - * already on this list, the ref-count is incremented.
>> - *
>> - * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> - * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> - * not added to any offset-manager.
>> - *
>> - * You must remove all open-files the same number of times as you 
>> added them
>> - * before destroying the node. Otherwise, you will leak memory.
>> - *
>> - * This is locked against concurrent access internally.
>> - *
>> - * RETURNS:
>> - * 0 on success, negative error code on internal failure (out-of-mem)
>> - */
>> -int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +static int vma_node_allow(struct drm_vma_offset_node *node,
>> +              struct drm_file *tag, bool ref_counted)
>>   {
>>       struct rb_node **iter;
>>       struct rb_node *parent = NULL;
>> @@ -282,7 +263,8 @@ int drm_vma_node_allow(struct drm_vma_offset_node 
>> *node, struct drm_file *tag)
>>           entry = rb_entry(*iter, struct drm_vma_offset_file, vm_rb);
>>           if (tag == entry->vm_tag) {
>> -            entry->vm_count++;
>> +            if (ref_counted)
>> +                entry->vm_count++;
>>               goto unlock;
>>           } else if (tag > entry->vm_tag) {
>>               iter = &(*iter)->rb_right;
>> @@ -307,8 +289,58 @@ int drm_vma_node_allow(struct drm_vma_offset_node 
>> *node, struct drm_file *tag)
>>       kfree(new);
>>       return ret;
>>   }
>> +
>> +/**
>> + * drm_vma_node_allow - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node. If @tag is
>> + * already on this list, the ref-count is incremented.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> + * not added to any offset-manager.
>> + *
>> + * You must remove all open-files the same number of times as you 
>> added them
>> + * before destroying the node. Otherwise, you will leak memory.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +{
>> +    return vma_node_allow(node, tag, true);
>> +}
>>   EXPORT_SYMBOL(drm_vma_node_allow);
>> +/**
>> + * drm_vma_node_allow_once - Add open-file to list of allowed users
>> + * @node: Node to modify
>> + * @tag: Tag of file to remove
>> + *
>> + * Add @tag to the list of allowed open-files for this node.
>> + *
>> + * The list of allowed-users is preserved across drm_vma_offset_add() 
>> and
>> + * drm_vma_offset_remove() calls. You may even call it if the node is 
>> currently
>> + * not added to any offset-manager.
>> + *
>> + * This is not ref-counted unlike drm_vma_node_allow() hence 
>> drm_vma_node_revoke()
>> + * should only be called once after this.
>> + *
>> + * This is locked against concurrent access internally.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error code on internal failure (out-of-mem)
>> + */
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct 
>> drm_file *tag)
>> +{
>> +    return vma_node_allow(node, tag, false);
>> +}
>> +EXPORT_SYMBOL(drm_vma_node_allow_once);
>> +
>>   /**
>>    * drm_vma_node_revoke - Remove open-file from list of allowed users
>>    * @node: Node to modify
>> diff --git a/include/drm/drm_vma_manager.h 
>> b/include/drm/drm_vma_manager.h
>> index 4f8c35206f7c..6c2a2f21dbf0 100644
>> --- a/include/drm/drm_vma_manager.h
>> +++ b/include/drm/drm_vma_manager.h
>> @@ -74,6 +74,7 @@ void drm_vma_offset_remove(struct 
>> drm_vma_offset_manager *mgr,
>>                  struct drm_vma_offset_node *node);
>>   int drm_vma_node_allow(struct drm_vma_offset_node *node, struct 
>> drm_file *tag);
>> +int drm_vma_node_allow_once(struct drm_vma_offset_node *node, struct 
>> drm_file *tag);
>>   void drm_vma_node_revoke(struct drm_vma_offset_node *node,
>>                struct drm_file *tag);
>>   bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
  2023-01-18 10:39       ` [Intel-gfx] " Das, Nirmoy
  2023-01-18 10:53         ` Mirsad Todorovac
@ 2023-01-18 22:27         ` Mirsad Goran Todorovac
  1 sibling, 0 replies; 31+ messages in thread
From: Mirsad Goran Todorovac @ 2023-01-18 22:27 UTC (permalink / raw)
  To: Das, Nirmoy, Tvrtko Ursulin, Nirmoy Das, intel-gfx; +Cc: dri-devel

On 18. 01. 2023. 11:39, Das, Nirmoy wrote:
>>>
>>> Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from https://patchwork.freedesktop.org/series/112952/.
>>>
>>> Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
>>>
>>> Finally, let me add some tags below:
>>>
>>> On 17/01/2023 17:52, Nirmoy Das wrote:
>>>> drm_vma_node_allow() and drm_vma_node_revoke() should be called in
>>>> balanced pairs. We call drm_vma_node_allow() once per-file everytime a
>>>> user calls mmap_offset, but only call drm_vma_node_revoke once per-file
>>>> on each mmap_offset. As the mmap_offset is reused by the client, the
>>>> per-file vm_count may remain non-zero and the rbtree leaked.
>>>>
>>>> Call drm_vma_node_allow_once() instead to prevent that memory leak.
>>>>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>>
>>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
>>> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
>>> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
>>> Cc: <stable@vger.kernel.org> # v5.7+
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> index 4f69bff63068..2aac6bf78740 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>>> @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>>>>       GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>>>>   out:
>>>>       if (file)
>>>> -        drm_vma_node_allow(&mmo->vma_node, file);
>>>> +        drm_vma_node_allow_once(&mmo->vma_node, file);
>>>>       return mmo;
>>>>   err:
>>
>> The drm/i915 patch seems OK and there are currently no memory leaks as of
>> reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
>> the initial bug ...
> 
> 
> Thanks, Mirsad for quickly checking this!

P.S.

Dear Nirmoy,

To let me explain, as I work on the Academy of Fine Arts and the Faculty of Graphic Arts,
video and multimedia drivers are of my special interest, so I kinda enjoyed this one.

No need to thank. ;-)

In case of more bugs discovered, I will be glad to test on my available platforms,
considering it a part of my research. I can justify time spent on graphic and multimedia
drivers, so it is not a big deal.

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
 
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
@ 2023-01-19 13:25   ` Maxime Ripard
  -1 siblings, 0 replies; 31+ messages in thread
From: Maxime Ripard @ 2023-01-19 13:25 UTC (permalink / raw)
  To: intel-gfx, Nirmoy Das
  Cc: Chris Wilson, Daniel Vetter, Maxime Ripard, dri-devel,
	Thomas Zimmermann, David Airlie

On Tue, 17 Jan 2023 18:52:35 +0100, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-19 13:25   ` Maxime Ripard
  0 siblings, 0 replies; 31+ messages in thread
From: Maxime Ripard @ 2023-01-19 13:25 UTC (permalink / raw)
  To: intel-gfx, Nirmoy Das; +Cc: Chris Wilson, dri-devel, Thomas Zimmermann

On Tue, 17 Jan 2023 18:52:35 +0100, Nirmoy Das wrote:
> Currently there is no easy way for a drm driver to safely check and allow
> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
> need to keep track of each drm_vma_node_allow() to call subsequent
> drm_vma_node_revoke() to prevent memory leak.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
  2023-01-19 13:25   ` Maxime Ripard
@ 2023-01-19 13:27     ` Das, Nirmoy
  -1 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-19 13:27 UTC (permalink / raw)
  To: Maxime Ripard, intel-gfx; +Cc: Chris Wilson, dri-devel, Thomas Zimmermann


On 1/19/2023 2:25 PM, Maxime Ripard wrote:
> On Tue, 17 Jan 2023 18:52:35 +0100, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> [...]
> Applied to drm/drm-misc (drm-misc-fixes).


Thanks Maxime!

>
> Thanks!
> Maxime

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once()
@ 2023-01-19 13:27     ` Das, Nirmoy
  0 siblings, 0 replies; 31+ messages in thread
From: Das, Nirmoy @ 2023-01-19 13:27 UTC (permalink / raw)
  To: Maxime Ripard, intel-gfx
  Cc: Chris Wilson, Daniel Vetter, Maxime Ripard, dri-devel,
	Thomas Zimmermann, David Airlie


On 1/19/2023 2:25 PM, Maxime Ripard wrote:
> On Tue, 17 Jan 2023 18:52:35 +0100, Nirmoy Das wrote:
>> Currently there is no easy way for a drm driver to safely check and allow
>> drm_vma_offset_node for a drm file just once. Allow drm drivers to call
>> non-refcounted version of drm_vma_node_allow() so that a driver doesn't
>> need to keep track of each drm_vma_node_allow() to call subsequent
>> drm_vma_node_revoke() to prevent memory leak.
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> [...]
> Applied to drm/drm-misc (drm-misc-fixes).


Thanks Maxime!

>
> Thanks!
> Maxime

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2023-01-19 13:27 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-17 17:52 [PATCH 1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once() Nirmoy Das
2023-01-17 17:52 ` [Intel-gfx] " Nirmoy Das
2023-01-17 17:52 ` [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset Nirmoy Das
2023-01-17 17:52   ` [Intel-gfx] " Nirmoy Das
2023-01-18  9:19   ` Tvrtko Ursulin
2023-01-18  9:19     ` [Intel-gfx] " Tvrtko Ursulin
2023-01-18  9:27     ` Das, Nirmoy
2023-01-18  9:27       ` [Intel-gfx] " Das, Nirmoy
2023-01-18  9:40     ` Andi Shyti
2023-01-18  9:40       ` [Intel-gfx] " Andi Shyti
2023-01-18 10:26     ` Mirsad Todorovac
2023-01-18 10:26       ` Mirsad Todorovac
2023-01-18 10:39       ` [Intel-gfx] " Das, Nirmoy
2023-01-18 10:53         ` Mirsad Todorovac
2023-01-18 22:27         ` Mirsad Goran Todorovac
2023-01-17 21:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once() Patchwork
2023-01-18  9:22 ` [PATCH 1/2] " Tvrtko Ursulin
2023-01-18  9:22   ` [Intel-gfx] " Tvrtko Ursulin
2023-01-18  9:38 ` Andi Shyti
2023-01-18  9:38   ` Andi Shyti
2023-01-18  9:45   ` [Intel-gfx] " Das, Nirmoy
2023-01-18  9:45     ` Das, Nirmoy
2023-01-18 11:07 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
2023-01-18 13:19 ` [PATCH 1/2] " Tvrtko Ursulin
2023-01-18 13:19   ` [Intel-gfx] " Tvrtko Ursulin
2023-01-18 13:34   ` Tvrtko Ursulin
2023-01-18 13:34     ` [Intel-gfx] " Tvrtko Ursulin
2023-01-19 13:25 ` Maxime Ripard
2023-01-19 13:25   ` Maxime Ripard
2023-01-19 13:27   ` Das, Nirmoy
2023-01-19 13:27     ` Das, Nirmoy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.