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

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

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

Regards,

Boris