On Thu, 27 Nov 2025 13:32:30 +0100 Boris Brezillon <[email protected]> wrote:
> Hi Thomas, > > On Thu, 27 Nov 2025 11:44:40 +0100 > Boris Brezillon <[email protected]> wrote: > > > On Thu, 27 Nov 2025 09:58:55 +0100 > > Thomas Zimmermann <[email protected]> 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 <[email protected]> > > > >>> --- > > > >>> 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?
