On Mon, Jan 12, 2026 at 3:40 PM Boris Brezillon
<[email protected]> wrote:
>
> On Mon, 12 Jan 2026 12:33:33 +0000
> Steven Price <[email protected]> wrote:
>
> > On 09/01/2026 13:08, Boris Brezillon wrote:
> > > This will be used to order things by reclaimability.
> > >
> > > Signed-off-by: Boris Brezillon <[email protected]>
> > > ---
> > > 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'm pretty sure that refcount_dec() is fine with 1->0. Did you mean
refcount_inc() and 0->1?
> > 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)".
Why not just use atomic_t and use the atomic inc/dec operations? They
don't have saturation, but also do not require treating zero
specially.
Alice
