Re: [PATCH 1/2] gpu: host1x: Allow entries in BO caches to be freed

[Aaron Kling]

On Thu, May 14, 2026 at 9:35 PM Mikko Perttunen <mperttunen@xxxxxxxxxx> wrote:
>
> When a buffer object is pinned via host1x_bo_pin() with a cache, the
> resulting mapping is kept in the cache so it can be reused on subsequent
> pins. Each mapping held a reference to the underlying host1x_bo (taken
> in tegra_bo_pin / gather_bo_pin), so as long as a mapping was cached,
> the bo itself could not be freed.
>
> However, the only way to remove the cached mapping was through the free
> path of the buffer object. This meant that if a bo got cached, it could
> never get freed again.
>
> Resolve the circularity by holding a weak reference to the bo from the
> cache side. This is done by having the .pin callbacks not bump the bo's
> refcount -- instead the common Host1x bo code does so, except for the
> cache reference.
>
> Also move the remove-cache-mapping-on-free code into a common function
> inside Host1x code. This is only called from the TegraDRM GEM buffers
> since those are the only ones that can be cached at the moment.
>
> Reported-by: Aaron Kling <webgeek1234@xxxxxxxxx>
> Fixes: 1f39b1dfa53c ("drm/tegra: Implement buffer object cache")
> Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/tegra/gem.c    | 13 ++-------
>  drivers/gpu/drm/tegra/submit.c |  3 +--
>  drivers/gpu/host1x/bus.c       | 60 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/host1x.h         |  7 +++++
>  4 files changed, 69 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index d2bae88ad545..2377e2b76397 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -69,7 +69,7 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct device *dev, struct host1x_
>                 return ERR_PTR(-ENOMEM);
>
>         kref_init(&map->ref);
> -       map->bo = host1x_bo_get(bo);
> +       map->bo = bo;
>         map->direction = direction;
>         map->dev = dev;
>
> @@ -170,7 +170,6 @@ static void tegra_bo_unpin(struct host1x_bo_mapping *map)
>                 kfree(map->sgt);
>         }
>
> -       host1x_bo_put(map->bo);
>         kfree(map);
>  }
>
> @@ -509,17 +508,9 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>  void tegra_bo_free_object(struct drm_gem_object *gem)
>  {
>         struct tegra_drm *tegra = gem->dev->dev_private;
> -       struct host1x_bo_mapping *mapping, *tmp;
>         struct tegra_bo *bo = to_tegra_bo(gem);
>
> -       /* remove all mappings of this buffer object from any caches */
> -       list_for_each_entry_safe(mapping, tmp, &bo->base.mappings, list) {
> -               if (mapping->cache)
> -                       host1x_bo_unpin(mapping);
> -               else
> -                       dev_err(gem->dev->dev, "mapping %p stale for device %s\n", mapping,
> -                               dev_name(mapping->dev));
> -       }
> +       host1x_bo_clear_cached_mappings(&bo->base);
>
>         if (tegra->domain) {
>                 tegra_bo_iommu_unmap(tegra, bo);
> diff --git a/drivers/gpu/drm/tegra/submit.c b/drivers/gpu/drm/tegra/submit.c
> index 3009b8b9e619..e5841857c937 100644
> --- a/drivers/gpu/drm/tegra/submit.c
> +++ b/drivers/gpu/drm/tegra/submit.c
> @@ -76,7 +76,7 @@ gather_bo_pin(struct device *dev, struct host1x_bo *bo, enum dma_data_direction
>                 return ERR_PTR(-ENOMEM);
>
>         kref_init(&map->ref);
> -       map->bo = host1x_bo_get(bo);
> +       map->bo = bo;
>         map->direction = direction;
>         map->dev = dev;
>
> @@ -117,7 +117,6 @@ static void gather_bo_unpin(struct host1x_bo_mapping *map)
>         dma_unmap_sgtable(map->dev, map->sgt, map->direction, 0);
>         sg_free_table(map->sgt);
>         kfree(map->sgt);
> -       host1x_bo_put(map->bo);
>
>         kfree(map);
>  }
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index f814eb4941c0..772e05a7b45b 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -887,6 +887,20 @@ int host1x_client_resume(struct host1x_client *client)
>  }
>  EXPORT_SYMBOL(host1x_client_resume);
>
> +/**
> + * host1x_bo_pin() - Create a DMA mapping for the buffer object
> + * @dev: Device onto which DMA map to
> + * @bo: Buffer object to map
> + * @dir: DMA direction
> + * @cache: Cache in which to store mapping, or NULL
> + *
> + * Creates a DMA mapping pointing to @bo for @dev. The refcount of @bo is incremented
> + * until host1x_bo_unpin is called.
> + *
> + * If @cache is specified, the mapping is also stored in the cache and not released
> + * until @bo is freed (refcount drops to zero). This improves performance when a buffer
> + * is pinned and unpinned frequently as in the case of display use.
> + */
>  struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo,
>                                         enum dma_data_direction dir,
>                                         struct host1x_bo_cache *cache)
> @@ -899,6 +913,7 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
>                 list_for_each_entry(mapping, &cache->mappings, entry) {
>                         if (mapping->bo == bo && mapping->direction == dir) {
>                                 kref_get(&mapping->ref);
> +                               host1x_bo_get(bo);
>                                 goto unlock;
>                         }
>                 }
> @@ -908,6 +923,8 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
>         if (IS_ERR(mapping))
>                 goto unlock;
>
> +       host1x_bo_get(bo);
> +
>         spin_lock(&mapping->bo->lock);
>         list_add_tail(&mapping->list, &bo->mappings);
>         spin_unlock(&mapping->bo->lock);
> @@ -918,7 +935,12 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
>
>                 list_add_tail(&mapping->entry, &cache->mappings);
>
> -               /* bump reference count to track the copy in the cache */
> +               /*
> +                * Bump the mapping reference count to track the mapping in the cache,
> +                * but do not bump the BO's refcount. This allows the BO to still get freed,
> +                * triggering the release of the cache mapping through
> +                * host1x_bo_clear_cached_mappings.
> +                */
>                 kref_get(&mapping->ref);
>         }
>
> @@ -948,9 +970,17 @@ static void __host1x_bo_unpin(struct kref *ref)
>         mapping->bo->ops->unpin(mapping);
>  }
>
> +/**
> + * host1x_bo_unpin() - Release an established DMA mapping of a buffer object
> + * @mapping: Mapping to release
> + *
> + * Unmaps the given @mapping, unless it is cached. Decreases the refcount on
> + * the underlying buffer object.
> + */
>  void host1x_bo_unpin(struct host1x_bo_mapping *mapping)
>  {
>         struct host1x_bo_cache *cache = mapping->cache;
> +       struct host1x_bo *bo = mapping->bo;
>
>         if (cache)
>                 mutex_lock(&cache->lock);
> @@ -959,5 +989,33 @@ void host1x_bo_unpin(struct host1x_bo_mapping *mapping)
>
>         if (cache)
>                 mutex_unlock(&cache->lock);
> +
> +       host1x_bo_put(bo);
>  }
>  EXPORT_SYMBOL(host1x_bo_unpin);
> +
> +/**
> + * host1x_bo_clear_cached_mappings() - Remove all cached mappings pointing at a bo
> + * @bo: Buffer object to release mappings of
> + *
> + * Drops references to any mappings pointing to @bo left in any caches. This must
> + * be called by any host1x_bo implementers that may be pinned with caching enabled
> + * before freeing the bo.
> + */
> +void host1x_bo_clear_cached_mappings(struct host1x_bo *bo)
> +{
> +       struct host1x_bo_mapping *mapping, *tmp;
> +       struct host1x_bo_cache *cache;
> +
> +       list_for_each_entry_safe(mapping, tmp, &bo->mappings, list) {
> +               cache = mapping->cache;
> +               if (WARN_ON(!cache))
> +                       continue;
> +
> +               mutex_lock(&mapping->cache->lock);
> +               WARN_ON(kref_read(&mapping->ref) != 1);
> +               __host1x_bo_unpin(&mapping->ref);
> +               mutex_unlock(&mapping->cache->lock);
> +       }
> +}
> +EXPORT_SYMBOL(host1x_bo_clear_cached_mappings);
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 5e7a63143a4a..d8f052a85b75 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -143,6 +143,12 @@ static inline struct host1x_bo_mapping *to_host1x_bo_mapping(struct kref *ref)
>         return container_of(ref, struct host1x_bo_mapping, ref);
>  }
>
> +/**
> + * struct host1x_bo_ops - operations implemented by a host1x_bo provider
> + *
> + * @pin: create a DMA mapping. Implementation must not touch the bo's refcount.
> + * @unpin: destroy a DMA mapping. Implementation must not touch the bo's refcount.
> + */
>  struct host1x_bo_ops {
>         struct host1x_bo *(*get)(struct host1x_bo *bo);
>         void (*put)(struct host1x_bo *bo);
> @@ -181,6 +187,7 @@ struct host1x_bo_mapping *host1x_bo_pin(struct device *dev, struct host1x_bo *bo
>                                         enum dma_data_direction dir,
>                                         struct host1x_bo_cache *cache);
>  void host1x_bo_unpin(struct host1x_bo_mapping *map);
> +void host1x_bo_clear_cached_mappings(struct host1x_bo *bo);
>
>  static inline void *host1x_bo_mmap(struct host1x_bo *bo)
>  {
>
> --
> 2.53.0
>

I have verified this on a Jetson AGX Xavier devkit, a Jetson Xavier NX
devkit, a Jetson TX2 devkit, and a Jetson TX2 NX in a p3509 carrier.
No longer seeing allocation failures nor am I seeing any obvious
regressions. I am currently unable to boot any t210 device to Android
userspace to verify those for various preexisting reasons, and Android
recovery does not reallocate buffers to stress this issue.

Tested-by: Aaron Kling <webgeek1234@xxxxxxxxx>

Aaron