Re: [PATCH v2 1/3] drm/sa: Split drm_suballoc_new() into SA alloc and init helpers

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

+ dri-devel, Christian

On Wed, 2026-02-04 at 16:46 +0000, Satyanarayana K V P wrote:
> drm_suballoc_new() currently both allocates the SA object using
> kmalloc()
> and searches for a suitable hole in the sub-allocator for the
> requested
> size. Since kmalloc() may take internal locks, performing memory
> allocation while holding the sub-allocator mutex can trigger lockdep
> deadlock warnings.

... As Matt mentioned, This is to be able to suballocate under a
reclaim-tainted lock: Allocation can be done outside the lock, and init
inside the lock.

> 
> Fix this by splitting SA allocation from drm_suballoc_new(),
> separating
> object allocation from sub-allocator initialization and hole
> selection.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>

This must be CC'd dri-devel as well and AMD maintainers.

Below is in addition to MBrost's comments.

> 
> ---
> V1 -> V2:
> - Splitted drm_suballoc_new() into drm_suballoc_alloc() and
> drm_suballoc_init() (Thomas).
> ---
>  drivers/gpu/drm/drm_suballoc.c | 135 +++++++++++++++++++++++++------
> --
>  include/drm/drm_suballoc.h     |   8 ++
>  2 files changed, 112 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_suballoc.c
> b/drivers/gpu/drm/drm_suballoc.c
> index 879ea33dbbc4..6f21f9e048d6 100644
> --- a/drivers/gpu/drm/drm_suballoc.c
> +++ b/drivers/gpu/drm/drm_suballoc.c
> @@ -123,7 +123,7 @@ static void drm_suballoc_remove_locked(struct
> drm_suballoc *sa)
>  	list_del_init(&sa->olist);
>  	list_del_init(&sa->flist);
>  	dma_fence_put(sa->fence);
> -	kfree(sa);
> +	drm_suballoc_release(sa);
>  }
>  
>  static void drm_suballoc_try_free(struct drm_suballoc_manager
> *sa_manager)
> @@ -293,45 +293,75 @@ static bool drm_suballoc_next_hole(struct
> drm_suballoc_manager *sa_manager,
>  }
>  
>  /**
> - * drm_suballoc_new() - Make a suballocation.
> + * drm_suballoc_alloc() - Allocate uninitialized suballoc object.
> + * @gfp: gfp flags used for memory allocation.
> + *
> + * Allocate memory for an uninitialized suballoc object. Intended
> usage is
> + * allocate memory for suballoc object outside of a reclaim tainted
> context
> + * and then be initialized at a later time in a reclaim tainted
> context.
> + *
> + * @drm_suballoc_release should be used to release the memory if
> returned
> + * suballoc object is in uninitialized state.
> + *
> + * Return: a new uninitialized suballoc object, or an ERR_PTR(-
> ENOMEM).
> + */
> +struct drm_suballoc *drm_suballoc_alloc(gfp_t gfp)
> +{
> +	struct drm_suballoc *sa;
> +
> +	sa = kmalloc(sizeof(*sa), gfp);
> +	if (!sa)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return sa;
> +}
> +EXPORT_SYMBOL(drm_suballoc_alloc);
> +
> +/**
> + * drm_suballoc_release() - Release memory for suballocation.
> + * @sa: The struct drm_suballoc.
> + */
> +void drm_suballoc_release(struct drm_suballoc *sa)
> +{
> +	kfree(sa);
> +}
> +EXPORT_SYMBOL(drm_suballoc_release);
> +
> +/**
> + * drm_suballoc_init() - Initialize a suballocation.
>   * @sa_manager: pointer to the sa_manager
> + * @sa: The struct drm_suballoc.
>   * @size: number of bytes we want to suballocate.
> - * @gfp: gfp flags used for memory allocation. Typically GFP_KERNEL
> but
> - *       the argument is provided for suballocations from reclaim
> context or
> - *       where the caller wants to avoid pipelining rather than wait
> for
> - *       reclaim.
>   * @intr: Whether to perform waits interruptible. This should
> typically
>   *        always be true, unless the caller needs to propagate a
>   *        non-interruptible context from above layers.
>   * @align: Alignment. Must not exceed the default manager alignment.
>   *         If @align is zero, then the manager alignment is used.
> + * @timeout: Time to a wait suballocation to become available.

I think we should split out the timeout to a separate patch, since that
is not really related to the problem of the lockdep splat, and it would
require a separate motivation, since typically the CTRL-C in
combination with fence timeouts are sufficient. We need to explain why
that's not the case for this particuar use-case. 

Also that would make the -fixes patches for the reclaim lockdep splat
smaller.


Otherwise LGTM.
Thomas


>   *
> - * Try to make a suballocation of size @size, which will be rounded
> - * up to the alignment specified in specified in
> drm_suballoc_manager_init().
> + * Try to make a suballocation on a pre-allocated suballoc object of
> size @size,
> + * which will be rounded up to the alignment specified in specified
> in
> + * drm_suballoc_manager_init().
>   *
> - * Return: a new suballocated bo, or an ERR_PTR.
> + * Return: zero on success, errno on failure.
>   */
> -struct drm_suballoc *
> -drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
> size,
> -		 gfp_t gfp, bool intr, size_t align)
> +int drm_suballoc_init(struct drm_suballoc_manager *sa_manager,
> +		      struct drm_suballoc *sa, size_t size,
> +		      bool intr, size_t align, signed long timeout)
>  {
>  	struct dma_fence *fences[DRM_SUBALLOC_MAX_QUEUES];
>  	unsigned int tries[DRM_SUBALLOC_MAX_QUEUES];
>  	unsigned int count;
>  	int i, r;
> -	struct drm_suballoc *sa;
>  
>  	if (WARN_ON_ONCE(align > sa_manager->align))
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  	if (WARN_ON_ONCE(size > sa_manager->size || !size))
> -		return ERR_PTR(-EINVAL);
> +		return -EINVAL;
>  
>  	if (!align)
>  		align = sa_manager->align;
>  
> -	sa = kmalloc(sizeof(*sa), gfp);
> -	if (!sa)
> -		return ERR_PTR(-ENOMEM);
>  	sa->manager = sa_manager;
>  	sa->fence = NULL;
>  	INIT_LIST_HEAD(&sa->olist);
> @@ -339,6 +369,8 @@ drm_suballoc_new(struct drm_suballoc_manager
> *sa_manager, size_t size,
>  
>  	spin_lock(&sa_manager->wq.lock);
>  	do {
> +		long t;
> +
>  		for (i = 0; i < DRM_SUBALLOC_MAX_QUEUES; ++i)
>  			tries[i] = 0;
>  
> @@ -348,7 +380,7 @@ drm_suballoc_new(struct drm_suballoc_manager
> *sa_manager, size_t size,
>  			if (drm_suballoc_try_alloc(sa_manager, sa,
>  						   size, align)) {
>  				spin_unlock(&sa_manager->wq.lock);
> -				return sa;
> +				return 0;
>  			}
>  
>  			/* see if we can skip over some allocations
> */
> @@ -359,34 +391,75 @@ drm_suballoc_new(struct drm_suballoc_manager
> *sa_manager, size_t size,
>  				fences[count++] =
> dma_fence_get(fences[i]);
>  
>  		if (count) {
> -			long t;
> -
>  			spin_unlock(&sa_manager->wq.lock);
>  			t = dma_fence_wait_any_timeout(fences,
> count, intr,
> -						      
> MAX_SCHEDULE_TIMEOUT,
> -						       NULL);
> +						       timeout,
> NULL);
>  			for (i = 0; i < count; ++i)
>  				dma_fence_put(fences[i]);
>  
> -			r = (t > 0) ? 0 : t;
>  			spin_lock(&sa_manager->wq.lock);
>  		} else if (intr) {
> +			spin_unlock(&sa_manager->wq.lock);
>  			/* if we have nothing to wait for block */
> -			r = wait_event_interruptible_locked
> +			r = wait_event_interruptible_timeout
>  				(sa_manager->wq,
> -				 __drm_suballoc_event(sa_manager,
> size, align));
> +				 __drm_suballoc_event(sa_manager,
> size, align),
> +				 timeout);
> +			spin_lock(&sa_manager->wq.lock);
>  		} else {
>  			spin_unlock(&sa_manager->wq.lock);
> -			wait_event(sa_manager->wq,
> -				   drm_suballoc_event(sa_manager,
> size, align));
> -			r = 0;
> +			t = wait_event_timeout
> +				(sa_manager->wq,
> +				 drm_suballoc_event(sa_manager,
> size, align),
> +				 timeout);
>  			spin_lock(&sa_manager->wq.lock);
>  		}
> +		r = (t > 0) ? 0 : !r ? -ETIME : t;
>  	} while (!r);
>  
>  	spin_unlock(&sa_manager->wq.lock);
> -	kfree(sa);
> -	return ERR_PTR(r);
> +	return r;
> +}
> +EXPORT_SYMBOL(drm_suballoc_init);
> +
> +/**
> + * drm_suballoc_new() - Make a suballocation.
> + * @sa_manager: pointer to the sa_manager
> + * @size: number of bytes we want to suballocate.
> + * @gfp: gfp flags used for memory allocation. Typically GFP_KERNEL
> but
> + *       the argument is provided for suballocations from reclaim
> context or
> + *       where the caller wants to avoid pipelining rather than wait
> for
> + *       reclaim.
> + * @intr: Whether to perform waits interruptible. This should
> typically
> + *        always be true, unless the caller needs to propagate a
> + *        non-interruptible context from above layers.
> + * @align: Alignment. Must not exceed the default manager alignment.
> + *         If @align is zero, then the manager alignment is used.
> + *
> + * Try to make a suballocation of size @size, which will be rounded
> + * up to the alignment specified in specified in
> drm_suballoc_manager_init().
> + *
> + * Return: a new suballocated bo, or an ERR_PTR.
> + */
> +struct drm_suballoc *
> +drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
> size,
> +		 gfp_t gfp, bool intr, size_t align)
> +{
> +	struct drm_suballoc *sa;
> +	int err;
> +
> +	sa = drm_suballoc_alloc(gfp);
> +	if (IS_ERR(sa))
> +		return sa;
> +
> +	err = drm_suballoc_init(sa_manager, sa, size, intr, align,
> +				MAX_SCHEDULE_TIMEOUT);
> +	if (err) {
> +		drm_suballoc_release(sa);
> +		return ERR_PTR(err);
> +	}
> +
> +	return sa;
>  }
>  EXPORT_SYMBOL(drm_suballoc_new);
>  
> diff --git a/include/drm/drm_suballoc.h b/include/drm/drm_suballoc.h
> index 7ba72a81a808..cff0e14556d1 100644
> --- a/include/drm/drm_suballoc.h
> +++ b/include/drm/drm_suballoc.h
> @@ -53,6 +53,14 @@ void drm_suballoc_manager_init(struct
> drm_suballoc_manager *sa_manager,
>  
>  void drm_suballoc_manager_fini(struct drm_suballoc_manager
> *sa_manager);
>  
> +struct drm_suballoc *drm_suballoc_alloc(gfp_t gfp);
> +
> +void drm_suballoc_release(struct drm_suballoc *sa);
> +
> +int drm_suballoc_init(struct drm_suballoc_manager *sa_manager,
> +		      struct drm_suballoc *sa, size_t size, bool
> intr,
> +		      size_t align, signed long timeout);
> +
>  struct drm_suballoc *
>  drm_suballoc_new(struct drm_suballoc_manager *sa_manager, size_t
> size,
>  		 gfp_t gfp, bool intr, size_t align);