+ 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 <[email protected]> > Suggested-by: Matthew Brost <[email protected]> > Cc: Thomas Hellström <[email protected]> > Cc: Michal Wajdeczko <[email protected]> > Cc: Matthew Auld <[email protected]> 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);
