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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 27 Nov 2025 13:32:30 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Hi Thomas,
> 
> On Thu, 27 Nov 2025 11:44:40 +0100
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> 
> > On Thu, 27 Nov 2025 09:58:55 +0100
> > Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >   
> > > 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].    
> > 
> > Except this doesn't work when the driver is linked statically (not
> > enabled as a module), because THIS_MODULE is NULL in that case.  
> 
> Couple more alternatives, if someone is interested in pursing in that
> path:
> 
> - Have a drm_device::dmabuf_ops and a drm_dev_set_dmabuf_ops() helper
>   to attach the driver dma_buf_ops to the device and allow a direct:
> 
> 	dmabuf->ops == dev->dmabuf_ops
> 
>   test
> - Have a dev field (struct device *) added to dma_buf, and have a
> 
> 	dmabuf->dev == drm_dev_dma_dev(dev)
> 
>   test
> 
> On my side, I'll just drop all the drm_gem[_shmem] changes in this
> series and duplicate the logic in panthor/panfrost for now, because it
> seems there's no consensus on this code-sharing proposal, and I want the
> cached CPU mapping stuff merged.
> 
> Just to be clear, I still think the proposed code sharing is
> valuable to
> 
> - avoid simple mistakes in drivers (it's very easy to get something
>   wrong in the import/export sequence)
> - ease propagation of fixes (all drivers using the common bits get the
>   fix automatically)
> - ease refactoring of code (it's easier to patch one common helper than
>   a half a dozen drivers)
> 
> Let alone the fact it could remove a bunch of boilerplate code in
> various drivers. This being said, I'm not willing to spend time on
> something that's likely to be rejected because of postures on
> philosophical design decisions (which I understand, but not necessarily
> agree with ;-)).

Quick update based on the discussion that happened on IRC between Thomas
and I. Thomas suggestion would be to add an optional

	bool (*gem_prime_dev_is_exporter)(struct drm_device *dev,
					  struct dma_buf *dmabuf);

callback instead of the drm_driver::dma_buf_ops field added in this patch.
This means driver would simply have to provide their own ops, and
implement their own drm_gem_object_funcs::export().

Christian, would you be happy with that?