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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
>