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.
> 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);
>> }
