On 12/01/2026 16:59, Boris Brezillon wrote: > On Mon, 12 Jan 2026 16:49:33 +0000 > Steven Price <[email protected]> wrote: > >> On 12/01/2026 14:39, Boris Brezillon 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. >> >> Yeah, I misread the refcount_dec() code the first time I looked at it. >> >>>> >>>> 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)) >> >> The problem is here - if another thread does an increment from 1 to 2 at >> this point then we lose a reference count. And since you don't have to >> hold the lock for that we have a problem. > > Okay, I see what you mean now. I was considering 0 -> 1, not 1 -> 2. So > we really need a > > if (refcount_dec_and_test(&bo->cmap.mmap_count)) > /* Do nothing */;
Yes - I was just writing a longer email to say that ;) I'm not sure if there's a better way of writing that - it does seem like a missing function in the refcount API. I just worry it points to refcounts being the wrong thing to use. But other than the missing function the refcount API does work fine here. Thanks, Steve >> >>> 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)". >> >> It's not the holding of the lock that's concerning me - it's the fact >> that the increment part might not be holding the lock. >> >> Your suggestion of "(void)refcount_dec_and_test()" would work, but it's >> rather abusing the API. I can't help feeling this points to the refcount >> API being the wrong thing to be using... >> >> Thanks, >> Steve >> >>>> >>>> drm_gem_object_put(&bo->base); >>>> } >> >
