Re: [PATCH v6 01/16] drm/prime: Simplify life of drivers needing custom dma_buf_ops

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.11.25 um 09:42 schrieb Thomas Zimmermann:
> Hi
>
> Am 27.11.25 um 09:34 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 26.11.25 um 13:44 schrieb Boris Brezillon:
>>> drm_gem_is_prime_exported_dma_buf() checks the dma_buf->ops against
>>> drm_gem_prime_dmabuf_ops, which makes it impossible to use if the
>>> driver implements custom dma_buf_ops. Instead of duplicating a bunch
>>> of helpers to work around it, let's provide a way for drivers to
>>> expose their custom dma_buf_ops so the core prime helpers can rely on
>>> that instead of hardcoding &drm_gem_prime_dmabuf_ops.
>>
>> This can't go in as-is. I've spent an awful amount of patches on 
>> removing buffer callbacks from struct drm_driver. Let's please not go 
>> back to that.
>>
>>>
>>> v5:
>>> - New patch
>>>
>>> v6:
>>> - Pass custom dma_buf_ops directly instead of through a getter
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
>>> ---
>>>   drivers/gpu/drm/drm_prime.c | 10 ++++++++--
>>>   include/drm/drm_drv.h       |  8 ++++++++
>>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 21809a82187b..86fd95f0c105 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -904,6 +904,12 @@ unsigned long 
>>> drm_prime_get_contiguous_size(struct sg_table *sgt)
>>>   }
>>>   EXPORT_SYMBOL(drm_prime_get_contiguous_size);
>>>   +static const struct dma_buf_ops *
>>> +drm_gem_prime_get_dma_buf_ops(struct drm_device *dev)
>>> +{
>>> +    return dev->driver->dma_buf_ops ?: &drm_gem_prime_dmabuf_ops;
>>> +}
>>> +
>>>   /**
>>>    * drm_gem_prime_export - helper library implementation of the 
>>> export callback
>>>    * @obj: GEM object to export
>>> @@ -920,7 +926,7 @@ struct dma_buf *drm_gem_prime_export(struct 
>>> drm_gem_object *obj,
>>>       struct dma_buf_export_info exp_info = {
>>>           .exp_name = KBUILD_MODNAME, /* white lie for debug */
>>>           .owner = dev->driver->fops->owner,
>>> -        .ops = &drm_gem_prime_dmabuf_ops,
>>> +        .ops = drm_gem_prime_get_dma_buf_ops(dev),
>>
>> Rather provide a new function drm_gem_prime_export_with_ops() that 
>> takes an additional dma_ops instance. The current 
>> drm_gem_prime_export() would call it with &drm_gem_prime_dmabuf_ops.
>>
>> If this really does not work, you could add a pointer to dma_buf_ops 
>> to drm_gem_object_funcs and fetch that from drm_gem_prime_export(). 
>> We already vm_ops there.
>>
>> Other drivers, such as amdgpu, would also benefit from such a change
>>
>>>           .size = obj->size,
>>>           .flags = flags,
>>>           .priv = obj,
>>> @@ -947,7 +953,7 @@ bool drm_gem_is_prime_exported_dma_buf(struct 
>>> drm_device *dev,
>>>   {
>>>       struct drm_gem_object *obj = dma_buf->priv;
>>>   -    return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && 
>>> (obj->dev == dev);
>>> +    return dma_buf->ops == drm_gem_prime_get_dma_buf_ops(dev) && 
>>> obj->dev == dev;
>
> On a second thought, we probably cannot be sure that dma_buf->priv 
> really is a GEM object until we tested the ops field. :/  IIRC that's 
> why the ops test goes first and the test for obj->dev goes second. So 
> neither solution works.

I think, instead of looking at the ops field, the test could look at 
dma_buf->owner == dev->driver->fops->owner.  This will tell if the 
dma_buf comes from the same driver and hence is a GEM object. In the 
next step, do obj->dev == dev as before.  This will also allow drivers 
like amdgpu to use the helper for testing. See [1].

[1] 
https://elixir.bootlin.com/linux/v6.17.9/source/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c#L512

Best regards
Thomas

>
> Best regards
> Thomas
>
>>
>> This is a bit more complicated and the test has been a pain point 
>> before. For this case, I think we should add a GEM callback for this
>>
>> struct drm_gem_object_funcs {
>>     bool (*exported_by)(struct drm_gem_object *obj, struct drm_device 
>> *dev)
>> }
>>
>> next to the existing export callback.
>>
>> And drm_gem_is_prime_exported_dma_buf would then do
>>
>> {
>>     if (obj->funcs->exported_by)
>>        return obj->funcs-<exported_by(obj, dev)
>>
>>    return /* what we currently test */
>> }
>>
>> IIRC amdgpu would again benefit from this.
>>
>> These changes will isolate dma_buf handling near GEM code, keep 
>> drm_driver clean, and even allow for a driver to have different 
>> implementations of dma_buf_ops.
>>
>> Best regards
>> Thomas
>>
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_is_prime_exported_dma_buf);
>>>   diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index 42fc085f986d..1c6dae60d523 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -431,6 +431,14 @@ struct drm_driver {
>>>        * some examples.
>>>        */
>>>       const struct file_operations *fops;
>>> +
>>> +    /**
>>> +     * @dma_buf_ops:
>>> +     *
>>> +     * dma_buf_ops to use for buffers exported by this driver. When 
>>> NULL,
>>> +     * the drm_prime logic defaults to &drm_gem_prime_dmabuf_ops.
>>> +     */
>>> +    const struct dma_buf_ops *dma_buf_ops;
>>>   };
>>>     void *__devm_drm_dev_alloc(struct device *parent,
>>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)