Re: [PATCH v3 1/6] drm/gem: add helper to detect self-imported PRIME dma-bufs

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 16.10.26 um 08:26 schrieb Niranjana Vishwanathapura:
> On Thu, Oct 15, 2026 at 09:27:08AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 14.10.26 um 21:02 schrieb Niranjana Vishwanathapura:
>>> From: Farah Kassabri <farah.kassabri@intel.com>
>>>
>>> Add drm_gem_prime_self_import() to detect and handle the case where a
>>> PRIME dma-buf being imported was exported by the same DRM device.
>>>
>>> When the dma-buf originates from the importing device and uses the
>>> expected dma-buf ops, the helper returns the associated GEM object and
>>> takes an extra reference on it. This allows drivers to bypass the
>>> generic PRIME import path and avoid taking an additional reference on
>>> the dma-buf file.
>>>
>>> The helper simplifies self-import handling and avoids duplication of
>>> this pattern across DRM drivers.
>>>
>>> v2: Move drm_gem_prime_self_import() to drm_gem.h as a static inline
>>>     helper routine
>>
>> Why even bother submitting this? It is the same series that I already 
>> declined with a few cosmetic changes.
>>
>> The abstract entry point for importing is 
>> drm_driver.gem_prime_import. The implementation is driver specific. 
>> No need to over abstract. Keeping drm_gem_object_get() separate from 
>> the test is a feature, not a bug BTW.
>>
>> If you want to improve the status quo, change dma-buf such that prime 
>> works without relying on the vtable and ->priv. I outlined this in my 
>> earlier replies.
>>
>
> Hi Thomas,
>
> As mentioned in other thread, your earlier suggesion of
> '__drm_gem_is_exported_dma_buf(dev, ops, dma_buf)' is not fully helpful
> as it would require drivers to still take object reference. Besides,
> this also depends on 'ops'.

This is a feature. These are two distinct operations. Taking the object 
ref is another of those implementation detail where drivers might want 
to do something else.

>
> To me the 'dma_buf_ops as export identifer' seems to be well accepted
> idiom, though we may say it shouldn't be in stricter sense.
>
> But if we were not to relay on vtable and ->priv, then it looks like
> the best alternative will be to have DRM subsystem maintain a list of
> exported dma-buf objects per drm device. Then during import, parse this
> list to determine if the import is self-import or not.
>
> I am not sure if it is worth to add this list just for this simple helper
> routine. If using dma_buf ops is no go, then yah, let us drop this 
> series,
> keep current code and may be look into this alternative later through 
> an RFC.

As I mention before, the abstraction is drm_driver.gem_prime_import. 
Anything behind that is driver-specific. A real improvement should 
provide a driver-generic implementation of gem_prime_import. This needs 
changes to dma_buf and to the prime helpers. OTOH gem_prime_import is 
already fairly abstract. There are no low-hanging fruits here IMHO.

Best regards
Thomas


>
> Thanks,
> Niranjana
>
>> Best regards
>> Thomas
>>
>>
>>
>>>
>>> Signed-off-by: Farah Kassabri <farah.kassabri@intel.com>
>>> Signed-off-by: Niranjana Vishwanathapura 
>>> <niranjana.vishwanathapura@intel.com>
>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_prime.c | 10 ++-------
>>>  include/drm/drm_gem.h       | 42 +++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 44 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 9b44c78cd77f..7a15c29052eb 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -974,15 +974,9 @@ struct drm_gem_object 
>>> *drm_gem_prime_import_dev(struct drm_device *dev,
>>>      struct drm_gem_object *obj;
>>>      int ret;
>>> -    if (drm_gem_is_prime_exported_dma_buf(dev, dma_buf)) {
>>> -        /*
>>> -         * Importing dmabuf exported from our own gem increases
>>> -         * refcount on gem itself instead of f_count of dmabuf.
>>> -         */
>>> -        obj = dma_buf->priv;
>>> -        drm_gem_object_get(obj);
>>> +    obj = drm_gem_prime_self_import(dev, dma_buf, 
>>> &drm_gem_prime_dmabuf_ops);
>>> +    if (obj)
>>>          return obj;
>>> -    }
>>>      if (!dev->driver->gem_prime_import_sg_table)
>>>          return ERR_PTR(-EINVAL);
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index 885244e375d3..238ec08f0d81 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -646,6 +646,48 @@ static inline bool drm_gem_is_imported(const 
>>> struct drm_gem_object *obj)
>>>      return !!obj->import_attach;
>>>  }
>>> +/**
>>> + * drm_gem_prime_self_import - Attempt to import a dma-buf exported 
>>> by this DRM device
>>> + * @dev: DRM device performing the import
>>> + * @dma_buf: dma-buf to import
>>> + * @expected_ops: dma-buf ops used by this driver
>>> + *
>>> + * If @dma_buf was exported by this DRM device using @expected_ops, 
>>> return the
>>> + * corresponding GEM object and take an extra reference on it. In 
>>> this case,
>>> + * the import avoids taking a reference on the dma-buf file and 
>>> instead bumps
>>> + * the GEM object's refcount directly.
>>> + *
>>> + * Comparing @dma_buf->ops against @expected_ops is plumbing: 
>>> dma-buf has no
>>> + * first-class exporter identifier, so the ops pointer is used as a 
>>> stand-in to
>>> + * recognise a buffer this driver exported. A dedicated exporter 
>>> identity would
>>> + * let this be done without reaching into @dma_buf->ops.
>>> + *
>>> + * Returns:
>>> + * A referenced GEM object on success, or %NULL if the dma-buf was 
>>> not exported
>>> + * by this device or does not match @expected_ops.
>>> + */
>>> +static inline struct drm_gem_object *
>>> +drm_gem_prime_self_import(struct drm_device *dev, struct dma_buf 
>>> *dma_buf,
>>> +              const struct dma_buf_ops *expected_ops)
>>> +{
>>> +    struct drm_gem_object *obj;
>>> +
>>> +    if (dma_buf->ops != expected_ops)
>>> +        return NULL;
>>> +
>>> +    obj = dma_buf->priv;
>>> +    if (obj->dev != dev)
>>> +        return NULL;
>>> +
>>> +    /*
>>> +     * Importing dmabuf exported from the driver's own gem increases
>>> +     * refcount on gem itself instead of f_count of dmabuf.
>>> +     */
>>> +    drm_gem_object_get(obj);
>>> +
>>> +    return obj;
>>> +}
>>> +
>>>  #ifdef CONFIG_LOCKDEP
>>>  #define drm_gem_gpuva_assert_lock_held(gpuvm, obj) \
>>>      lockdep_assert(drm_gpuvm_immediate_mode(gpuvm) ? \
>>
>> -- 
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
>> GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)
>>
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)