Re: [PATCH v1 8/9] drm/panthor: Track the number of mmap on a BO

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 12 Jan 2026 12:33:33 +0000
Steven Price <steven.price@arm.com> wrote:

> On 09/01/2026 13:08, Boris Brezillon wrote:
> > This will be used to order things by reclaimability.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_gem.c | 44 +++++++++++++++++++++++++--
> >  drivers/gpu/drm/panthor/panthor_gem.h |  3 ++
> >  2 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > index 44f05bd957e7..458d22380e96 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -484,6 +484,7 @@ static void panthor_gem_print_info(struct drm_printer *p, unsigned int indent,
> >  	drm_printf_indent(p, indent, "vmap_use_count=%u\n",
> >  			  refcount_read(&bo->cmap.vaddr_use_count));
> >  	drm_printf_indent(p, indent, "vaddr=%p\n", bo->cmap.vaddr);
> > +	drm_printf_indent(p, indent, "mmap_count=%u\n", refcount_read(&bo->cmap.mmap_count));
> >  }
> >  
> >  static int panthor_gem_pin_locked(struct drm_gem_object *obj)
> > @@ -600,6 +601,13 @@ static int panthor_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *v
> >  	if (is_cow_mapping(vma->vm_flags))
> >  		return -EINVAL;
> >  
> > +	if (!refcount_inc_not_zero(&bo->cmap.mmap_count)) {
> > +		dma_resv_lock(obj->resv, NULL);
> > +		if (!refcount_inc_not_zero(&bo->cmap.mmap_count))
> > +			refcount_set(&bo->cmap.mmap_count, 1);
> > +		dma_resv_unlock(obj->resv);
> > +	}
> > +
> >  	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> >  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> >  	if (should_map_wc(bo))
> > @@ -732,10 +740,42 @@ static vm_fault_t panthor_gem_fault(struct vm_fault *vmf)
> >  	return blocking_page_setup(vmf, bo, page_offset, true);
> >  }
> >  
> > +static void panthor_gem_vm_open(struct vm_area_struct *vma)
> > +{
> > +	struct panthor_gem_object *bo = to_panthor_bo(vma->vm_private_data);
> > +
> > +	/* mmap_count must have been incremented at mmap time, so it can't be
> > +	 * zero here.
> > +	 */
> > +	if (!drm_gem_is_imported(&bo->base))
> > +		drm_WARN_ON(bo->base.dev, !refcount_inc_not_zero(&bo->cmap.mmap_count));
> > +
> > +	drm_gem_vm_open(vma);
> > +}
> > +
> > +static void panthor_gem_vm_close(struct vm_area_struct *vma)
> > +{
> > +	struct panthor_gem_object *bo = to_panthor_bo(vma->vm_private_data);
> > +
> > +	if (drm_gem_is_imported(&bo->base))
> > +		goto out;
> > +
> > +	if (refcount_dec_not_one(&bo->cmap.mmap_count))
> > +		goto out;
> > +
> > +	dma_resv_lock(bo->base.resv, NULL);
> > +	if (!refcount_dec_not_one(&bo->cmap.mmap_count))
> > +		refcount_set(&bo->cmap.mmap_count, 0);
> > +	dma_resv_unlock(bo->base.resv);  
> 
> I don't think this logic is safe. Holding the resv_lock doesn't protect
> against another thread doing a refcount_inc_not_zero() without holding
> the lock.
> 
> I think you can just replace the if() part with a refcount_dec() call,
> the lock AFAICT is needed because the following patch wants to be sure
> that !!mmap_count is stable when resv_lock is held.

I wish I could, but refcount_dec() doesn't let me do the 1 -> 0 without
complaining :P.

> 
> I also feel you should invert the conditino for refcount_dec_not_one,
> leading to the following which I feel is easier to read:
> 
> static void panthor_gem_vm_close(struct vm_area_struct *vma)
> {
> 	[...]
> 
> 	if (!refcount_dec_not_one(&bo->cmap.mmap_count)) {
> 		dma_resv_lock(bo->base.resv, NULL);
> 		refcount_dec(&bo->cmap.mmap_count);
> 		dma_resv_unlock(bo->base.resv);
> 	}

The best I can do is:

 	if (!refcount_dec_not_one(&bo->cmap.mmap_count)) {
 		dma_resv_lock(bo->base.resv, NULL);
 		if (!refcount_dec_not_one(&bo->cmap.mmap_count))
			refcount_set(&bo->cmap.mmap_count, 0);
 		dma_resv_unlock(bo->base.resv);
 	}

so we only take the lock when absolutely needed, but the 1 -> 0
transition still has to be done with "if (dec_not_one) set(0)".

> 
> 	drm_gem_object_put(&bo->base);
> }