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 ;-)). Regards, Boris
