[PATCH 0/9 v2] drm/i915: Remove legacy struct_mutex usage

[PATCH 0/9 v2] drm/i915: Remove legacy struct_mutex usage

[Luiz Otavio Mello]

This patch series completes the long-standing effort to eliminate usage
of the legacy struct_mutex from i915 driver.

Historically, struct_mutex was used to serialize access to global driver
state across the DRM subsystem. Over time, it has been gradually
replaced by more fine-grained and localized locking mechanism. The i915
driver was the last remaining user of this lock, and even there, its
usage had become redundant or outdated.

Specifically, the mutex was only still used in two places: i915_irq.c
and intel_guc_log.c. In both cases, the lock could either be removed or
replaced with a more appropriate lock.

This series performs the following steps:

* Moves struct_mutex from drm_device to drm_i915_private, since i915 was
  its only remaining user.
* Removes or replaces all remaining uses of struct_mutex in i915 driver.
* Updates or removes comments referring to struct_mutex to prevent
  future confusion.
* Deletes the lock entirely from the i915 driver once no longer in use.
* Cleans up the corresponding TODO entry in Documentation/gpu/todo.rst
  and comments about struct_mutex in Documentation/gpu/i915.rst.

Some additional notes:

* In intel_guc_log.c, a missing destructor for a lock was identified.
  Since the series introduces a new lock in that area, this issue was
  addressed first, to the two locks do not lack memory in kernel. 
 
* Comments referencing struct_mutex were spread across various parts of
  i915 codebase. To improve clarity, they were cleaned up across three
  separate patches.

The only remianig reference to struct_mutex is in a comment in
i915_gem_execbuffer.c, inside the eb_relocate_vma() function. It was
kept because the intended locking mechanism for that case is unclear.

Changes since v1:
* Rebased onto latest drm-tip as requested for CI compatibility
* No changes to code/content

Luiz Otavio Mello (9):
  drm/i915: Move struct_mutex to drm_i915_private
  drm/i915: Remove struct_mutex in i915_irq.c
  drm/i915: Change mutex initialization in intel_guc_log
  drm/i915: Replace struct_mutex with guc_lock in intel_guc_log
  drm/i915: change comments about legacy struct_mutex 1/3
  drm/i915: change comments about legacy struct_mutex 2/3
  drm/i915: change comments about legacy struct_mutex 3/3
  Remove unused struct_mutex from drm_i915_private
  Remove todo and comments about struct_mutex

 Documentation/gpu/i915.rst                    |  7 ------
 Documentation/gpu/todo.rst                    | 25 -------------------
 drivers/gpu/drm/drm_drv.c                     |  2 --
 drivers/gpu/drm/i915/display/intel_fbc.c      |  6 +----
 .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.c    |  4 +--
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c  |  4 +--
 drivers/gpu/drm/i915/gem/i915_gem_wait.c      |  8 +++---
 drivers/gpu/drm/i915/gt/intel_reset_types.h   |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.c    | 12 ++++++---
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.h    |  8 ++++++
 drivers/gpu/drm/i915/i915_drv.h               |  6 +++--
 drivers/gpu/drm/i915/i915_gem.c               |  3 +--
 drivers/gpu/drm/i915/i915_irq.c               |  6 -----
 include/drm/drm_device.h                      | 10 --------
 15 files changed, 33 insertions(+), 72 deletions(-)

-- 
2.50.1

[PATCH 1/9 v2] drm/i915: Move struct_mutex to drm_i915_private

[Luiz Otavio Mello]

Move legacy BKL struct_mutex from drm_device to drm_i915_private, which
is the last remaining user.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/drm_drv.c                  |  2 --
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.c |  4 ++--
 drivers/gpu/drm/i915/i915_driver.c         |  2 ++
 drivers/gpu/drm/i915/i915_drv.h            | 11 +++++++++++
 drivers/gpu/drm/i915/i915_irq.c            |  4 ++--
 include/drm/drm_device.h                   | 10 ----------
 6 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index cdd591b11488..ad3aee354ba3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -694,7 +694,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
 	mutex_destroy(&dev->master_mutex);
 	mutex_destroy(&dev->clientlist_mutex);
 	mutex_destroy(&dev->filelist_mutex);
-	mutex_destroy(&dev->struct_mutex);
 }
 
 static int drm_dev_init(struct drm_device *dev,
@@ -735,7 +734,6 @@ static int drm_dev_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&dev->vblank_event_list);
 
 	spin_lock_init(&dev->event_lock);
-	mutex_init(&dev->struct_mutex);
 	mutex_init(&dev->filelist_mutex);
 	mutex_init(&dev->clientlist_mutex);
 	mutex_init(&dev->master_mutex);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
index e8a04e476c57..7135fdb0ebb4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
@@ -678,7 +678,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
 	if (level < GUC_LOG_LEVEL_DISABLED || level > GUC_LOG_LEVEL_MAX)
 		return -EINVAL;
 
-	mutex_lock(&i915->drm.struct_mutex);
+	mutex_lock(&i915->struct_mutex);
 
 	if (log->level == level)
 		goto out_unlock;
@@ -696,7 +696,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
 	log->level = level;
 
 out_unlock:
-	mutex_unlock(&i915->drm.struct_mutex);
+	mutex_unlock(&i915->struct_mutex);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index c6263c6d3384..d1559fd8ad3d 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -233,6 +233,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 
 	intel_sbi_init(display);
 	vlv_iosf_sb_init(dev_priv);
+	mutex_init(&dev_priv->struct_mutex);
 	mutex_init(&dev_priv->sb_lock);
 
 	i915_memcpy_init_early(dev_priv);
@@ -291,6 +292,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
 	i915_workqueues_cleanup(dev_priv);
 
 	mutex_destroy(&dev_priv->sb_lock);
+	mutex_destroy(&dev_priv->struct_mutex);
 	vlv_iosf_sb_fini(dev_priv);
 	intel_sbi_fini(display);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e4e89746aa6..6093dbaf4009 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -222,6 +222,17 @@ struct drm_i915_private {
 
 	bool irqs_enabled;
 
+	/*
+	 * Currently, struct_mutex is only used by the i915 driver as a replacement
+	 * for BKL. 
+	 * 
+	 * For this reason, it is no longer part of struct drm_device.
+	*/
+	struct mutex struct_mutex;
+
+	/* LPT/WPT IOSF sideband protection */
+	struct mutex sbi_lock;
+
 	/* VLV/CHV IOSF sideband */
 	struct {
 		struct mutex lock; /* protect sideband access */
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 191ed8bb1d9c..cdfb09464134 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -167,7 +167,7 @@ static void ivb_parity_work(struct work_struct *work)
 	 * In order to prevent a get/put style interface, acquire struct mutex
 	 * any time we access those registers.
 	 */
-	mutex_lock(&dev_priv->drm.struct_mutex);
+	mutex_lock(&dev_priv->struct_mutex);
 
 	/* If we've screwed up tracking, just let the interrupt fire again */
 	if (drm_WARN_ON(&dev_priv->drm, !dev_priv->l3_parity.which_slice))
@@ -225,7 +225,7 @@ static void ivb_parity_work(struct work_struct *work)
 	gen5_gt_enable_irq(gt, GT_PARITY_ERROR(dev_priv));
 	spin_unlock_irq(gt->irq_lock);
 
-	mutex_unlock(&dev_priv->drm.struct_mutex);
+	mutex_unlock(&dev_priv->struct_mutex);
 }
 
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index a33aedd5e9ec..016df5529d46 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -188,16 +188,6 @@ struct drm_device {
 	/** @unique: Unique name of the device */
 	char *unique;
 
-	/**
-	 * @struct_mutex:
-	 *
-	 * Lock for others (not &drm_minor.master and &drm_file.is_master)
-	 *
-	 * TODO: This lock used to be the BKL of the DRM subsystem. Move the
-	 *       lock into i915, which is the only remaining user.
-	 */
-	struct mutex struct_mutex;
-
 	/**
 	 * @master_mutex:
 	 *
-- 
2.50.1

[PATCH 2/9 v2] drm/i915: Remove struct_mutex in i915_irq.c

[Luiz Otavio Mello]

Remove struct_mutex from ivb_parity_work() function.

The ivb_parity_work runs in a workqueue so it cannot race with itself.

Also, it is not protecting anything with the other remaining usage of
struct_mutex.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index cdfb09464134..83b08dacd194 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -163,11 +163,6 @@ static void ivb_parity_work(struct work_struct *work)
 	u32 misccpctl;
 	u8 slice = 0;
 
-	/* We must turn off DOP level clock gating to access the L3 registers.
-	 * In order to prevent a get/put style interface, acquire struct mutex
-	 * any time we access those registers.
-	 */
-	mutex_lock(&dev_priv->struct_mutex);
 
 	/* If we've screwed up tracking, just let the interrupt fire again */
 	if (drm_WARN_ON(&dev_priv->drm, !dev_priv->l3_parity.which_slice))
@@ -225,7 +220,6 @@ static void ivb_parity_work(struct work_struct *work)
 	gen5_gt_enable_irq(gt, GT_PARITY_ERROR(dev_priv));
 	spin_unlock_irq(gt->irq_lock);
 
-	mutex_unlock(&dev_priv->struct_mutex);
 }
 
 static irqreturn_t valleyview_irq_handler(int irq, void *arg)
-- 
2.50.1

[PATCH 3/9 v2] drm/i915: Change mutex initialization in intel_guc_log

[Luiz Otavio Mello]

The intel_guc_log->relay.lock is currently initialized in
intel_guc_log_init_early(), but it lacks a corresponding destructor,
which can lead to a memory leak.

This patch replaces the use of mutex_init() with drmm_mutex_init(),
which ensures the lock is properly destroyed when the driver is
unloaded.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
index 7135fdb0ebb4..469173791394 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
@@ -6,6 +6,8 @@
 #include <linux/debugfs.h>
 #include <linux/string_helpers.h>
 
+#include <drm/drm_managed.h>
+
 #include "gt/intel_gt.h"
 #include "i915_drv.h"
 #include "i915_irq.h"
@@ -512,7 +514,10 @@ static void guc_log_relay_unmap(struct intel_guc_log *log)
 
 void intel_guc_log_init_early(struct intel_guc_log *log)
 {
-	mutex_init(&log->relay.lock);
+	struct intel_guc *guc = log_to_guc(log);
+	struct drm_i915_private *i915 = guc_to_i915(guc);
+
+	drmm_mutex_init(&i915->drm, &log->relay.lock);
 	INIT_WORK(&log->relay.flush_work, copy_debug_logs_work);
 	log->relay.started = false;
 }
-- 
2.50.1

[PATCH 4/9 v2] drm/i915: Replace struct_mutex in intel_guc_log.c

[Luiz Otavio Mello]

Remove the use of struct_mutex from intel_guc_log.c and replace it with
a dedicated lock, guc_lock, defined within the intel_guc_log struct.
    
The struct_mutex was previously used to protect concurrent access and
modification of intel_guc_log->level in intel_guc_log_set_level().
However, it was suggested that the lock should reside within the
intel_guc_log struct itself.
    
Initialize the new guc_lock in intel_guc_log_init_early(), alongside the
existing relay.lock. The lock is initialized using drmm_mutex_init(),
which also ensures it is properly destroyed when the driver is unloaded.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 5 +++--
 drivers/gpu/drm/i915/gt/uc/intel_guc_log.h | 8 ++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
index 469173791394..0104fffd5852 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
@@ -518,6 +518,7 @@ void intel_guc_log_init_early(struct intel_guc_log *log)
 	struct drm_i915_private *i915 = guc_to_i915(guc);
 
 	drmm_mutex_init(&i915->drm, &log->relay.lock);
+	drmm_mutex_init(&i915->drm, &log->guc_lock);
 	INIT_WORK(&log->relay.flush_work, copy_debug_logs_work);
 	log->relay.started = false;
 }
@@ -683,7 +684,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
 	if (level < GUC_LOG_LEVEL_DISABLED || level > GUC_LOG_LEVEL_MAX)
 		return -EINVAL;
 
-	mutex_lock(&i915->struct_mutex);
+	mutex_lock(&log->guc_lock);
 
 	if (log->level == level)
 		goto out_unlock;
@@ -701,7 +702,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
 	log->level = level;
 
 out_unlock:
-	mutex_unlock(&i915->struct_mutex);
+	mutex_unlock(&log->guc_lock);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
index 02127703be80..13cb93ad0710 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
@@ -42,6 +42,14 @@ enum {
 struct intel_guc_log {
 	u32 level;
 
+	/*
+	 * Protects concurrent access and modification of intel_guc_log->level.
+	 *
+	 * This lock replaces the legacy struct_mutex usage in
+	 * intel_guc_log system.
+	 */
+	struct mutex guc_lock;
+
 	/* Allocation settings */
 	struct {
 		s32 bytes;	/* Size in bytes */
-- 
2.50.1

[PATCH 5/9 v2] drm/i915/gem: Clean-up outdated struct_mutex comments

[Luiz Otavio Mello]

The struct_mutex will be removed from the DRM subsystem, as it was a
legacy BKL that was only used by i915 driver. After review, it was
concluded that its usage was no longer necessary

This patch updates various comments in the i915/gem and i915/gt
codebase to either remove or clarify references to struct_mutex, in
order to prevent future misunderstandings.

* i915_gem_execbuffer.c: Replace reference to struct_mutex with
  vm->mutex, as noted in the eb_reserve() function, which states that
  vm->mutex handles deadlocks.
* i915_gem_object.c: Change struct_mutex by
  drm_i915_gem_object->vma.lock. i915_gem_object_unbind() in i915_gem.c
  states that this lock is who actually protects the unbind.
* i915_gem_shrinker.c: The correct lock is actually i915->mm.obj, as
  already documented in its declaration.
* i915_gem_wait.c: The existing comment already mentioned that
  struct_mutex was no longer necessary. Updated to refer to a generic
  global lock instead.
* intel_reset_types.h: Cleaned up the comment text. Updated to refer to
  a generic global lock instead.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
 drivers/gpu/drm/i915/gem/i915_gem_object.c     | 4 ++--
 drivers/gpu/drm/i915/gem/i915_gem_shrinker.c   | 4 ++--
 drivers/gpu/drm/i915/gem/i915_gem_wait.c       | 8 ++++----
 drivers/gpu/drm/i915/gt/intel_reset_types.h    | 2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index f243f8a5215d..39c7c32e1e74 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -182,7 +182,7 @@ enum {
  * the object. Simple! ... The relocation entries are stored in user memory
  * and so to access them we have to copy them into a local buffer. That copy
  * has to avoid taking any pagefaults as they may lead back to a GEM object
- * requiring the struct_mutex (i.e. recursive deadlock). So once again we split
+ * requiring the vm->mutex (i.e. recursive deadlock). So once again we split
  * the relocation into multiple passes. First we try to do everything within an
  * atomic context (avoid the pagefaults) which requires that we never wait. If
  * we detect that we may wait, or if we need to fault, then we have to fallback
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 1f38e367c60b..478011e5ecb3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -459,8 +459,8 @@ static void i915_gem_free_object(struct drm_gem_object *gem_obj)
 	atomic_inc(&i915->mm.free_count);
 
 	/*
-	 * Since we require blocking on struct_mutex to unbind the freed
-	 * object from the GPU before releasing resources back to the
+	 * Since we require blocking on drm_i915_gem_object->vma.lock to unbind
+	 * the freed object from the GPU before releasing resources back to the
 	 * system, we can not do that directly from the RCU callback (which may
 	 * be a softirq context), but must instead then defer that work onto a
 	 * kthread. We use the RCU callback rather than move the freed object
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index b81e67504bbe..7a3e74a6676e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -170,7 +170,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww,
 	 * Also note that although these lists do not hold a reference to
 	 * the object we can safely grab one here: The final object
 	 * unreferencing and the bound_list are both protected by the
-	 * dev->struct_mutex and so we won't ever be able to observe an
+	 * i915->mm.obj_lock and so we won't ever be able to observe an
 	 * object on the bound_list with a reference count equals 0.
 	 */
 	for (phase = phases; phase->list; phase++) {
@@ -185,7 +185,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww,
 
 		/*
 		 * We serialize our access to unreferenced objects through
-		 * the use of the struct_mutex. While the objects are not
+		 * the use of the obj_lock. While the objects are not
 		 * yet freed (due to RCU then a workqueue) we still want
 		 * to be able to shrink their pages, so they remain on
 		 * the unbound/bound list until actually freed.
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
index 991666fd9f85..54829801d3f7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
@@ -217,10 +217,10 @@ static unsigned long to_wait_timeout(s64 timeout_ns)
  *
  * The wait ioctl with a timeout of 0 reimplements the busy ioctl. With any
  * non-zero timeout parameter the wait ioctl will wait for the given number of
- * nanoseconds on an object becoming unbusy. Since the wait itself does so
- * without holding struct_mutex the object may become re-busied before this
- * function completes. A similar but shorter * race condition exists in the busy
- * ioctl
+ * nanoseconds on an object becoming unbusy. Since the wait occurs without
+ * holding a global or exclusive lock the object may become re-busied before
+ * this function completes. A similar but shorter * race condition exists
+ * in the busy ioctl
  */
 int
 i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset_types.h b/drivers/gpu/drm/i915/gt/intel_reset_types.h
index 4f5fd393af6f..ee4eb574a219 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_reset_types.h
@@ -20,7 +20,7 @@ struct intel_reset {
 	 * FENCE registers).
 	 *
 	 * #I915_RESET_ENGINE[num_engines] - Since the driver doesn't need to
-	 * acquire the struct_mutex to reset an engine, we need an explicit
+	 * acquire a global lock to reset an engine, we need an explicit
 	 * flag to prevent two concurrent reset attempts in the same engine.
 	 * As the number of engines continues to grow, allocate the flags from
 	 * the most significant bits.
-- 
2.50.1

[PATCH 6/9 v2] drm/i915/display: Remove outdated struct_mutex comments

[Luiz Otavio Mello]

The struct_mutex will be removed from the DRM subsystem, as it was a
legacy BKL that was only used by i915 driver. After review, it was
concluded that its usage was no longer necessary

This patch update a comment about struct_mutex in i915/display, in order
to prevent future misunderstandings.

* intel_fbc.c: Removed the statement that intel_fbc->lock is the inner
  lock when overlapping with struct_mutex, since struct_mutex is no
  longer used anywhere in the driver.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
index d4c5deff9cbe..aaaabba77b88 100644
--- a/drivers/gpu/drm/i915/display/intel_fbc.c
+++ b/drivers/gpu/drm/i915/display/intel_fbc.c
@@ -98,11 +98,7 @@ struct intel_fbc {
 	struct intel_display *display;
 	const struct intel_fbc_funcs *funcs;
 
-	/*
-	 * This is always the inner lock when overlapping with
-	 * struct_mutex and it's the outer lock when overlapping
-	 * with stolen_lock.
-	 */
+	/* This is always the outer lock when overlapping with stolen_lock */
 	struct mutex lock;
 	unsigned int busy_bits;
 
-- 
2.50.1

[PATCH 7/9 v2] drm/i915: Clean-up outdated struct_mutex comments

[Luiz Otavio Mello]

The struct_mutex will be removed from the DRM subsystem, as it was a
legacy BKL that was only used by i915 driver. After review, it was
concluded that its usage was no longer necessary

This patch updates various comments in the i915 codebase to
either remove or clarify references to struct_mutex, in order to
prevent future misunderstandings.

* i915_drv.h: Removed the statement that stolen_lock is the inner lock
  when overlaps with struct_mutex, since struct_mutex is no longer used
  in the driver.
* i915_gem.c: Removed parentheses suggesting usage of struct_mutex, which
  which is no longer used.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +--
 drivers/gpu/drm/i915/i915_gem.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6093dbaf4009..e8cb94962482 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -114,8 +114,7 @@ struct i915_gem_mm {
 	struct intel_memory_region *stolen_region;
 	/** Memory allocator for GTT stolen memory */
 	struct drm_mm stolen;
-	/** Protects the usage of the GTT stolen memory allocator. This is
-	 * always the inner lock when overlapping with struct_mutex. */
+	/** Protects the usage of the GTT stolen memory allocator */
 	struct mutex stolen_lock;
 
 	/* Protects bound_list/unbound_list and #drm_i915_gem_object.mm.link */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8c8d43451f35..e14a0c3db999 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -847,8 +847,7 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
 	/*
 	 * Only called during RPM suspend. All users of the userfault_list
 	 * must be holding an RPM wakeref to ensure that this can not
-	 * run concurrently with themselves (and use the struct_mutex for
-	 * protection between themselves).
+	 * run concurrently with themselves.
 	 */
 
 	list_for_each_entry_safe(obj, on,
-- 
2.50.1

[PATCH 8/9 v2] drm/i915: Remove unused struct_mutex from drm_i915_private

[Luiz Otavio Mello]

The struct_mutex field in drm_i915_private is no longer used anywhere in
the driver. This patch removes it completely to clean up unused code and
avoid confusion.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 drivers/gpu/drm/i915/i915_driver.c | 2 --
 drivers/gpu/drm/i915/i915_drv.h    | 8 --------
 2 files changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index d1559fd8ad3d..c6263c6d3384 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -233,7 +233,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 
 	intel_sbi_init(display);
 	vlv_iosf_sb_init(dev_priv);
-	mutex_init(&dev_priv->struct_mutex);
 	mutex_init(&dev_priv->sb_lock);
 
 	i915_memcpy_init_early(dev_priv);
@@ -292,7 +291,6 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
 	i915_workqueues_cleanup(dev_priv);
 
 	mutex_destroy(&dev_priv->sb_lock);
-	mutex_destroy(&dev_priv->struct_mutex);
 	vlv_iosf_sb_fini(dev_priv);
 	intel_sbi_fini(display);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e8cb94962482..5a8ac1a52849 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -221,14 +221,6 @@ struct drm_i915_private {
 
 	bool irqs_enabled;
 
-	/*
-	 * Currently, struct_mutex is only used by the i915 driver as a replacement
-	 * for BKL. 
-	 * 
-	 * For this reason, it is no longer part of struct drm_device.
-	*/
-	struct mutex struct_mutex;
-
 	/* LPT/WPT IOSF sideband protection */
 	struct mutex sbi_lock;
 
-- 
2.50.1

[PATCH 9/9 v2] drm/i915: Remove todo and comments about struct_mutex

[Luiz Otavio Mello]

This patch completes the removal of struct_mutex from the driver.

Remove the related TODO item, as the transition away from struct_mutex
is now complete.

Also clean up references to struct_mutex in i915.rst to avoid outdated
documentation.

Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
---
 Documentation/gpu/i915.rst |  7 -------
 Documentation/gpu/todo.rst | 25 -------------------------
 2 files changed, 32 deletions(-)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 72932fa31b8d..eba09c3ddce4 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -358,8 +358,6 @@ Locking Guidelines
 #. All locking rules and interface contracts with cross-driver interfaces
    (dma-buf, dma_fence) need to be followed.
 
-#. No struct_mutex anywhere in the code
-
 #. dma_resv will be the outermost lock (when needed) and ww_acquire_ctx
    is to be hoisted at highest level and passed down within i915_gem_ctx
    in the call chain
@@ -367,11 +365,6 @@ Locking Guidelines
 #. While holding lru/memory manager (buddy, drm_mm, whatever) locks
    system memory allocations are not allowed
 
-	* Enforce this by priming lockdep (with fs_reclaim). If we
-	  allocate memory while holding these looks we get a rehash
-	  of the shrinker vs. struct_mutex saga, and that would be
-	  real bad.
-
 #. Do not nest different lru/memory manager locks within each other.
    Take them in turn to update memory allocations, relying on the object’s
    dma_resv ww_mutex to serialize against other operations.
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 92db80793bba..b5f58b4274b1 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -173,31 +173,6 @@ Contact: Simona Vetter
 
 Level: Intermediate
 
-Get rid of dev->struct_mutex from GEM drivers
----------------------------------------------
-
-``dev->struct_mutex`` is the Big DRM Lock from legacy days and infested
-everything. Nowadays in modern drivers the only bit where it's mandatory is
-serializing GEM buffer object destruction. Which unfortunately means drivers
-have to keep track of that lock and either call ``unreference`` or
-``unreference_locked`` depending upon context.
-
-Core GEM doesn't have a need for ``struct_mutex`` any more since kernel 4.8,
-and there's a GEM object ``free`` callback for any drivers which are
-entirely ``struct_mutex`` free.
-
-For drivers that need ``struct_mutex`` it should be replaced with a driver-
-private lock. The tricky part is the BO free functions, since those can't
-reliably take that lock any more. Instead state needs to be protected with
-suitable subordinate locks or some cleanup work pushed to a worker thread. For
-performance-critical drivers it might also be better to go with a more
-fine-grained per-buffer object and per-context lockings scheme. Currently only
-the ``msm`` and `i915` drivers use ``struct_mutex``.
-
-Contact: Simona Vetter, respective driver maintainers
-
-Level: Advanced
-
 Move Buffer Object Locking to dma_resv_lock()
 ---------------------------------------------
 
-- 
2.50.1

Re: [PATCH 1/9 v2] drm/i915: Move struct_mutex to drm_i915_private

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:00PM -0300, Luiz Otavio Mello wrote:
> Move legacy BKL struct_mutex from drm_device to drm_i915_private, which
> is the last remaining user.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
> ---
>  drivers/gpu/drm/drm_drv.c                  |  2 --

misc Maintainers, since this touches drm, ack on getting this through
drm-intel-next?

>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c |  4 ++--
>  drivers/gpu/drm/i915/i915_driver.c         |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h            | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_irq.c            |  4 ++--
>  include/drm/drm_device.h                   | 10 ----------
>  6 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index cdd591b11488..ad3aee354ba3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -694,7 +694,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
>  	mutex_destroy(&dev->master_mutex);
>  	mutex_destroy(&dev->clientlist_mutex);
>  	mutex_destroy(&dev->filelist_mutex);
> -	mutex_destroy(&dev->struct_mutex);
>  }
>  
>  static int drm_dev_init(struct drm_device *dev,
> @@ -735,7 +734,6 @@ static int drm_dev_init(struct drm_device *dev,
>  	INIT_LIST_HEAD(&dev->vblank_event_list);
>  
>  	spin_lock_init(&dev->event_lock);
> -	mutex_init(&dev->struct_mutex);
>  	mutex_init(&dev->filelist_mutex);
>  	mutex_init(&dev->clientlist_mutex);
>  	mutex_init(&dev->master_mutex);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index e8a04e476c57..7135fdb0ebb4 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -678,7 +678,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
>  	if (level < GUC_LOG_LEVEL_DISABLED || level > GUC_LOG_LEVEL_MAX)
>  		return -EINVAL;
>  
> -	mutex_lock(&i915->drm.struct_mutex);
> +	mutex_lock(&i915->struct_mutex);
>  
>  	if (log->level == level)
>  		goto out_unlock;
> @@ -696,7 +696,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
>  	log->level = level;
>  
>  out_unlock:
> -	mutex_unlock(&i915->drm.struct_mutex);
> +	mutex_unlock(&i915->struct_mutex);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index c6263c6d3384..d1559fd8ad3d 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -233,6 +233,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  
>  	intel_sbi_init(display);
>  	vlv_iosf_sb_init(dev_priv);
> +	mutex_init(&dev_priv->struct_mutex);
>  	mutex_init(&dev_priv->sb_lock);
>  
>  	i915_memcpy_init_early(dev_priv);
> @@ -291,6 +292,7 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
>  	i915_workqueues_cleanup(dev_priv);
>  
>  	mutex_destroy(&dev_priv->sb_lock);
> +	mutex_destroy(&dev_priv->struct_mutex);
>  	vlv_iosf_sb_fini(dev_priv);
>  	intel_sbi_fini(display);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e4e89746aa6..6093dbaf4009 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -222,6 +222,17 @@ struct drm_i915_private {
>  
>  	bool irqs_enabled;
>  
> +	/*
> +	 * Currently, struct_mutex is only used by the i915 driver as a replacement
> +	 * for BKL. 
> +	 * 
> +	 * For this reason, it is no longer part of struct drm_device.
> +	*/

The text in this block is not good imho. It doesn't need to state why it is not
part of the drm_device, but why it yet exists or a mention that it is going to
get removed, but I don't be picky with the content itself since it is getting
removed in a next patch in this same series.

But one thing you need is to fix the spurious space and alignment and ensure that
it is passing the checkpatch.

Right after rebasing into tip/drm-tip you can do this to check all your patches:
$ git show --no-use-mailmap --pretty=email tip/drm-tip..HEAD | ./scripts/checkpatch.pl -q --emacs --strict --show-types --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER

Or you can install our maintainer-tools and use dim checkpatch command.
https://drm.pages.freedesktop.org/maintainer-tools/index.html

With dim you have the right check-patch rules for each of the drm targets.

And yes, I have fixed it on the series I resent for CI, so with this fixed:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +	struct mutex struct_mutex;
> +
> +	/* LPT/WPT IOSF sideband protection */
> +	struct mutex sbi_lock;
> +
>  	/* VLV/CHV IOSF sideband */
>  	struct {
>  		struct mutex lock; /* protect sideband access */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 191ed8bb1d9c..cdfb09464134 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -167,7 +167,7 @@ static void ivb_parity_work(struct work_struct *work)
>  	 * In order to prevent a get/put style interface, acquire struct mutex
>  	 * any time we access those registers.
>  	 */
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> +	mutex_lock(&dev_priv->struct_mutex);
>  
>  	/* If we've screwed up tracking, just let the interrupt fire again */
>  	if (drm_WARN_ON(&dev_priv->drm, !dev_priv->l3_parity.which_slice))
> @@ -225,7 +225,7 @@ static void ivb_parity_work(struct work_struct *work)
>  	gen5_gt_enable_irq(gt, GT_PARITY_ERROR(dev_priv));
>  	spin_unlock_irq(gt->irq_lock);
>  
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> +	mutex_unlock(&dev_priv->struct_mutex);
>  }
>  
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index a33aedd5e9ec..016df5529d46 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -188,16 +188,6 @@ struct drm_device {
>  	/** @unique: Unique name of the device */
>  	char *unique;
>  
> -	/**
> -	 * @struct_mutex:
> -	 *
> -	 * Lock for others (not &drm_minor.master and &drm_file.is_master)
> -	 *
> -	 * TODO: This lock used to be the BKL of the DRM subsystem. Move the
> -	 *       lock into i915, which is the only remaining user.
> -	 */
> -	struct mutex struct_mutex;
> -
>  	/**
>  	 * @master_mutex:
>  	 *
> -- 
> 2.50.1
> 

Re: [PATCH 2/9 v2] drm/i915: Remove struct_mutex in i915_irq.c

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:01PM -0300, Luiz Otavio Mello wrote:
> Remove struct_mutex from ivb_parity_work() function.
> 
> The ivb_parity_work runs in a workqueue so it cannot race with itself.
> 
> Also, it is not protecting anything with the other remaining usage of
> struct_mutex.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
> Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index cdfb09464134..83b08dacd194 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -163,11 +163,6 @@ static void ivb_parity_work(struct work_struct *work)
>  	u32 misccpctl;
>  	u8 slice = 0;
>  
> -	/* We must turn off DOP level clock gating to access the L3 registers.
> -	 * In order to prevent a get/put style interface, acquire struct mutex
> -	 * any time we access those registers.
> -	 */
> -	mutex_lock(&dev_priv->struct_mutex);
>  
>  	/* If we've screwed up tracking, just let the interrupt fire again */
>  	if (drm_WARN_ON(&dev_priv->drm, !dev_priv->l3_parity.which_slice))
> @@ -225,7 +220,6 @@ static void ivb_parity_work(struct work_struct *work)
>  	gen5_gt_enable_irq(gt, GT_PARITY_ERROR(dev_priv));
>  	spin_unlock_irq(gt->irq_lock);
>  
> -	mutex_unlock(&dev_priv->struct_mutex);
>  }
>  
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> -- 
> 2.50.1
> 

Re: [PATCH 3/9 v2] drm/i915: Change mutex initialization in intel_guc_log

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:02PM -0300, Luiz Otavio Mello wrote:
> The intel_guc_log->relay.lock is currently initialized in
> intel_guc_log_init_early(), but it lacks a corresponding destructor,
> which can lead to a memory leak.
> 
> This patch replaces the use of mutex_init() with drmm_mutex_init(),
> which ensures the lock is properly destroyed when the driver is
> unloaded.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 7135fdb0ebb4..469173791394 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -6,6 +6,8 @@
>  #include <linux/debugfs.h>
>  #include <linux/string_helpers.h>
>  
> +#include <drm/drm_managed.h>
> +
>  #include "gt/intel_gt.h"
>  #include "i915_drv.h"
>  #include "i915_irq.h"
> @@ -512,7 +514,10 @@ static void guc_log_relay_unmap(struct intel_guc_log *log)
>  
>  void intel_guc_log_init_early(struct intel_guc_log *log)
>  {
> -	mutex_init(&log->relay.lock);
> +	struct intel_guc *guc = log_to_guc(log);
> +	struct drm_i915_private *i915 = guc_to_i915(guc);
> +
> +	drmm_mutex_init(&i915->drm, &log->relay.lock);
>  	INIT_WORK(&log->relay.flush_work, copy_debug_logs_work);
>  	log->relay.started = false;
>  }
> -- 
> 2.50.1
> 

Re: [PATCH 4/9 v2] drm/i915: Replace struct_mutex in intel_guc_log.c

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:03PM -0300, Luiz Otavio Mello wrote:
> Remove the use of struct_mutex from intel_guc_log.c and replace it with
> a dedicated lock, guc_lock, defined within the intel_guc_log struct.
>     
> The struct_mutex was previously used to protect concurrent access and
> modification of intel_guc_log->level in intel_guc_log_set_level().
> However, it was suggested that the lock should reside within the
> intel_guc_log struct itself.
>     
> Initialize the new guc_lock in intel_guc_log_init_early(), alongside the
> existing relay.lock. The lock is initialized using drmm_mutex_init(),
> which also ensures it is properly destroyed when the driver is unloaded.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
> Suggested-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 5 +++--
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.h | 8 ++++++++
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 469173791394..0104fffd5852 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -518,6 +518,7 @@ void intel_guc_log_init_early(struct intel_guc_log *log)
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
>  
>  	drmm_mutex_init(&i915->drm, &log->relay.lock);
> +	drmm_mutex_init(&i915->drm, &log->guc_lock);
>  	INIT_WORK(&log->relay.flush_work, copy_debug_logs_work);
>  	log->relay.started = false;
>  }
> @@ -683,7 +684,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
>  	if (level < GUC_LOG_LEVEL_DISABLED || level > GUC_LOG_LEVEL_MAX)
>  		return -EINVAL;
>  
> -	mutex_lock(&i915->struct_mutex);
> +	mutex_lock(&log->guc_lock);
>  
>  	if (log->level == level)
>  		goto out_unlock;
> @@ -701,7 +702,7 @@ int intel_guc_log_set_level(struct intel_guc_log *log, u32 level)
>  	log->level = level;
>  
>  out_unlock:
> -	mutex_unlock(&i915->struct_mutex);
> +	mutex_unlock(&log->guc_lock);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> index 02127703be80..13cb93ad0710 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.h
> @@ -42,6 +42,14 @@ enum {
>  struct intel_guc_log {
>  	u32 level;
>  
> +	/*
> +	 * Protects concurrent access and modification of intel_guc_log->level.
> +	 *
> +	 * This lock replaces the legacy struct_mutex usage in
> +	 * intel_guc_log system.
> +	 */
> +	struct mutex guc_lock;
> +
>  	/* Allocation settings */
>  	struct {
>  		s32 bytes;	/* Size in bytes */
> -- 
> 2.50.1
> 

Re: [PATCH 5/9 v2] drm/i915/gem: Clean-up outdated struct_mutex comments

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:04PM -0300, Luiz Otavio Mello wrote:
> The struct_mutex will be removed from the DRM subsystem, as it was a
> legacy BKL that was only used by i915 driver. After review, it was
> concluded that its usage was no longer necessary
> 
> This patch updates various comments in the i915/gem and i915/gt
> codebase to either remove or clarify references to struct_mutex, in
> order to prevent future misunderstandings.
> 
> * i915_gem_execbuffer.c: Replace reference to struct_mutex with
>   vm->mutex, as noted in the eb_reserve() function, which states that
>   vm->mutex handles deadlocks.
> * i915_gem_object.c: Change struct_mutex by
>   drm_i915_gem_object->vma.lock. i915_gem_object_unbind() in i915_gem.c
>   states that this lock is who actually protects the unbind.
> * i915_gem_shrinker.c: The correct lock is actually i915->mm.obj, as
>   already documented in its declaration.
> * i915_gem_wait.c: The existing comment already mentioned that
>   struct_mutex was no longer necessary. Updated to refer to a generic
>   global lock instead.
> * intel_reset_types.h: Cleaned up the comment text. Updated to refer to
>   a generic global lock instead.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
>  drivers/gpu/drm/i915/gem/i915_gem_object.c     | 4 ++--
>  drivers/gpu/drm/i915/gem/i915_gem_shrinker.c   | 4 ++--
>  drivers/gpu/drm/i915/gem/i915_gem_wait.c       | 8 ++++----
>  drivers/gpu/drm/i915/gt/intel_reset_types.h    | 2 +-
>  5 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index f243f8a5215d..39c7c32e1e74 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -182,7 +182,7 @@ enum {
>   * the object. Simple! ... The relocation entries are stored in user memory
>   * and so to access them we have to copy them into a local buffer. That copy
>   * has to avoid taking any pagefaults as they may lead back to a GEM object
> - * requiring the struct_mutex (i.e. recursive deadlock). So once again we split
> + * requiring the vm->mutex (i.e. recursive deadlock). So once again we split
>   * the relocation into multiple passes. First we try to do everything within an
>   * atomic context (avoid the pagefaults) which requires that we never wait. If
>   * we detect that we may wait, or if we need to fault, then we have to fallback
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index 1f38e367c60b..478011e5ecb3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -459,8 +459,8 @@ static void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  	atomic_inc(&i915->mm.free_count);
>  
>  	/*
> -	 * Since we require blocking on struct_mutex to unbind the freed
> -	 * object from the GPU before releasing resources back to the
> +	 * Since we require blocking on drm_i915_gem_object->vma.lock to unbind
> +	 * the freed object from the GPU before releasing resources back to the
>  	 * system, we can not do that directly from the RCU callback (which may
>  	 * be a softirq context), but must instead then defer that work onto a
>  	 * kthread. We use the RCU callback rather than move the freed object
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> index b81e67504bbe..7a3e74a6676e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> @@ -170,7 +170,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww,
>  	 * Also note that although these lists do not hold a reference to
>  	 * the object we can safely grab one here: The final object
>  	 * unreferencing and the bound_list are both protected by the
> -	 * dev->struct_mutex and so we won't ever be able to observe an
> +	 * i915->mm.obj_lock and so we won't ever be able to observe an
>  	 * object on the bound_list with a reference count equals 0.
>  	 */
>  	for (phase = phases; phase->list; phase++) {
> @@ -185,7 +185,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww,
>  
>  		/*
>  		 * We serialize our access to unreferenced objects through
> -		 * the use of the struct_mutex. While the objects are not
> +		 * the use of the obj_lock. While the objects are not
>  		 * yet freed (due to RCU then a workqueue) we still want
>  		 * to be able to shrink their pages, so they remain on
>  		 * the unbound/bound list until actually freed.
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> index 991666fd9f85..54829801d3f7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
> @@ -217,10 +217,10 @@ static unsigned long to_wait_timeout(s64 timeout_ns)
>   *
>   * The wait ioctl with a timeout of 0 reimplements the busy ioctl. With any
>   * non-zero timeout parameter the wait ioctl will wait for the given number of
> - * nanoseconds on an object becoming unbusy. Since the wait itself does so
> - * without holding struct_mutex the object may become re-busied before this
> - * function completes. A similar but shorter * race condition exists in the busy
> - * ioctl
> + * nanoseconds on an object becoming unbusy. Since the wait occurs without
> + * holding a global or exclusive lock the object may become re-busied before
> + * this function completes. A similar but shorter * race condition exists
> + * in the busy ioctl
>   */
>  int
>  i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset_types.h b/drivers/gpu/drm/i915/gt/intel_reset_types.h
> index 4f5fd393af6f..ee4eb574a219 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_reset_types.h
> @@ -20,7 +20,7 @@ struct intel_reset {
>  	 * FENCE registers).
>  	 *
>  	 * #I915_RESET_ENGINE[num_engines] - Since the driver doesn't need to
> -	 * acquire the struct_mutex to reset an engine, we need an explicit
> +	 * acquire a global lock to reset an engine, we need an explicit
>  	 * flag to prevent two concurrent reset attempts in the same engine.
>  	 * As the number of engines continues to grow, allocate the flags from
>  	 * the most significant bits.
> -- 
> 2.50.1
> 

Re: [PATCH 6/9 v2] drm/i915/display: Remove outdated struct_mutex comments

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:05PM -0300, Luiz Otavio Mello wrote:
> The struct_mutex will be removed from the DRM subsystem, as it was a
> legacy BKL that was only used by i915 driver. After review, it was
> concluded that its usage was no longer necessary
> 
> This patch update a comment about struct_mutex in i915/display, in order
> to prevent future misunderstandings.
> 
> * intel_fbc.c: Removed the statement that intel_fbc->lock is the inner
>   lock when overlapping with struct_mutex, since struct_mutex is no
>   longer used anywhere in the driver.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index d4c5deff9cbe..aaaabba77b88 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -98,11 +98,7 @@ struct intel_fbc {
>  	struct intel_display *display;
>  	const struct intel_fbc_funcs *funcs;
>  
> -	/*
> -	 * This is always the inner lock when overlapping with
> -	 * struct_mutex and it's the outer lock when overlapping
> -	 * with stolen_lock.
> -	 */
> +	/* This is always the outer lock when overlapping with stolen_lock */
>  	struct mutex lock;
>  	unsigned int busy_bits;
>  
> -- 
> 2.50.1
> 

Re: [PATCH 7/9 v2] drm/i915: Clean-up outdated struct_mutex comments

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:06PM -0300, Luiz Otavio Mello wrote:
> The struct_mutex will be removed from the DRM subsystem, as it was a
> legacy BKL that was only used by i915 driver. After review, it was
> concluded that its usage was no longer necessary
> 
> This patch updates various comments in the i915 codebase to
> either remove or clarify references to struct_mutex, in order to
> prevent future misunderstandings.
> 
> * i915_drv.h: Removed the statement that stolen_lock is the inner lock
>   when overlaps with struct_mutex, since struct_mutex is no longer used
>   in the driver.
> * i915_gem.c: Removed parentheses suggesting usage of struct_mutex, which
>   which is no longer used.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 3 +--
>  drivers/gpu/drm/i915/i915_gem.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6093dbaf4009..e8cb94962482 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -114,8 +114,7 @@ struct i915_gem_mm {
>  	struct intel_memory_region *stolen_region;
>  	/** Memory allocator for GTT stolen memory */
>  	struct drm_mm stolen;
> -	/** Protects the usage of the GTT stolen memory allocator. This is
> -	 * always the inner lock when overlapping with struct_mutex. */
> +	/** Protects the usage of the GTT stolen memory allocator */
>  	struct mutex stolen_lock;
>  
>  	/* Protects bound_list/unbound_list and #drm_i915_gem_object.mm.link */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8c8d43451f35..e14a0c3db999 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -847,8 +847,7 @@ void i915_gem_runtime_suspend(struct drm_i915_private *i915)
>  	/*
>  	 * Only called during RPM suspend. All users of the userfault_list
>  	 * must be holding an RPM wakeref to ensure that this can not
> -	 * run concurrently with themselves (and use the struct_mutex for
> -	 * protection between themselves).
> +	 * run concurrently with themselves.
>  	 */
>  
>  	list_for_each_entry_safe(obj, on,
> -- 
> 2.50.1
> 

Re: [PATCH 8/9 v2] drm/i915: Remove unused struct_mutex from drm_i915_private

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:07PM -0300, Luiz Otavio Mello wrote:
> The struct_mutex field in drm_i915_private is no longer used anywhere in
> the driver. This patch removes it completely to clean up unused code and
> avoid confusion.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 2 --
>  drivers/gpu/drm/i915/i915_drv.h    | 8 --------
>  2 files changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index d1559fd8ad3d..c6263c6d3384 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -233,7 +233,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
>  
>  	intel_sbi_init(display);
>  	vlv_iosf_sb_init(dev_priv);
> -	mutex_init(&dev_priv->struct_mutex);
>  	mutex_init(&dev_priv->sb_lock);
>  
>  	i915_memcpy_init_early(dev_priv);
> @@ -292,7 +291,6 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
>  	i915_workqueues_cleanup(dev_priv);
>  
>  	mutex_destroy(&dev_priv->sb_lock);
> -	mutex_destroy(&dev_priv->struct_mutex);
>  	vlv_iosf_sb_fini(dev_priv);
>  	intel_sbi_fini(display);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e8cb94962482..5a8ac1a52849 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -221,14 +221,6 @@ struct drm_i915_private {
>  
>  	bool irqs_enabled;
>  
> -	/*
> -	 * Currently, struct_mutex is only used by the i915 driver as a replacement
> -	 * for BKL. 
> -	 * 
> -	 * For this reason, it is no longer part of struct drm_device.
> -	*/

This will need to be rebased like I did in my resubmission for CI once the
other patch removes this trailing spaces and fix the alignement. But with that
change, this patch is:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> -	struct mutex struct_mutex;
> -
>  	/* LPT/WPT IOSF sideband protection */
>  	struct mutex sbi_lock;
>  
> -- 
> 2.50.1
> 

Re: [PATCH 9/9 v2] drm/i915: Remove todo and comments about struct_mutex

[Rodrigo Vivi]

On Thu, Aug 07, 2025 at 02:02:08PM -0300, Luiz Otavio Mello wrote:
> This patch completes the removal of struct_mutex from the driver.
> 
> Remove the related TODO item, as the transition away from struct_mutex
> is now complete.
> 
> Also clean up references to struct_mutex in i915.rst to avoid outdated
> documentation.
> 
> Signed-off-by: Luiz Otavio Mello <luiz.mello@estudante.ufscar.br>
> ---
>  Documentation/gpu/i915.rst |  7 -------
>  Documentation/gpu/todo.rst | 25 -------------------------

misc Maintainers, I also need your ack here to get this through drm-intel-next
please.

I wondered if it would be better to have a split patch to clean up the
i915 doc separate of the drm one, but I believe this single all in one
patch for the doc part is okay...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  2 files changed, 32 deletions(-)
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index 72932fa31b8d..eba09c3ddce4 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -358,8 +358,6 @@ Locking Guidelines
>  #. All locking rules and interface contracts with cross-driver interfaces
>     (dma-buf, dma_fence) need to be followed.
>  
> -#. No struct_mutex anywhere in the code
> -
>  #. dma_resv will be the outermost lock (when needed) and ww_acquire_ctx
>     is to be hoisted at highest level and passed down within i915_gem_ctx
>     in the call chain
> @@ -367,11 +365,6 @@ Locking Guidelines
>  #. While holding lru/memory manager (buddy, drm_mm, whatever) locks
>     system memory allocations are not allowed
>  
> -	* Enforce this by priming lockdep (with fs_reclaim). If we
> -	  allocate memory while holding these looks we get a rehash
> -	  of the shrinker vs. struct_mutex saga, and that would be
> -	  real bad.
> -
>  #. Do not nest different lru/memory manager locks within each other.
>     Take them in turn to update memory allocations, relying on the object’s
>     dma_resv ww_mutex to serialize against other operations.
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 92db80793bba..b5f58b4274b1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -173,31 +173,6 @@ Contact: Simona Vetter
>  
>  Level: Intermediate
>  
> -Get rid of dev->struct_mutex from GEM drivers
> ----------------------------------------------
> -
> -``dev->struct_mutex`` is the Big DRM Lock from legacy days and infested
> -everything. Nowadays in modern drivers the only bit where it's mandatory is
> -serializing GEM buffer object destruction. Which unfortunately means drivers
> -have to keep track of that lock and either call ``unreference`` or
> -``unreference_locked`` depending upon context.
> -
> -Core GEM doesn't have a need for ``struct_mutex`` any more since kernel 4.8,
> -and there's a GEM object ``free`` callback for any drivers which are
> -entirely ``struct_mutex`` free.
> -
> -For drivers that need ``struct_mutex`` it should be replaced with a driver-
> -private lock. The tricky part is the BO free functions, since those can't
> -reliably take that lock any more. Instead state needs to be protected with
> -suitable subordinate locks or some cleanup work pushed to a worker thread. For
> -performance-critical drivers it might also be better to go with a more
> -fine-grained per-buffer object and per-context lockings scheme. Currently only
> -the ``msm`` and `i915` drivers use ``struct_mutex``.
> -
> -Contact: Simona Vetter, respective driver maintainers
> -
> -Level: Advanced
> -
>  Move Buffer Object Locking to dma_resv_lock()
>  ---------------------------------------------
>  
> -- 
> 2.50.1
>