Re: [PATCH v5 02/16] drm/shmem: Provide a generic {begin,end}_cpu_access() implementation

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

On Thu, 30 Oct 2025 15:31:44 +0100
Christian König <christian.koenig@amd.com> wrote:

> On 10/30/25 15:05, Boris Brezillon wrote:
> > The default implementation simply takes care of invalidating/flushing
> > caches around CPU accesses. It takes care of both the exporter and
> > the importers, which forces us to overload the default
> > ::[un]map_dma_buf() implementation provided by drm_gem.c to store the
> > sgt.
> > 
> > v5:
> > - New patch
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 114 +++++++++++++++++++++++++
> >  include/drm/drm_gem_shmem_helper.h     |  10 +++
> >  2 files changed, 124 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index dc94a27710e5..e49c75739c20 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -893,6 +893,120 @@ struct drm_gem_object *drm_gem_shmem_prime_import_no_map(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_no_map);
> >  
> > +/**
> > + * drm_gem_shmem_prime_unmap_dma_buf - Default unmap_dma_buf() for exported buffers
> > + * @attach: attachment
> > + * @sgt: SG table to unmap
> > + * @dir: type of access done by this attachment
> > + *
> > + * Default implementation for dma_buf_ops::map_dma_buf(). This is just a wrapper
> > + * around drm_gem_map_dma_buf() that lets us set the dma_buf_attachment::priv
> > + * to the sgt so that drm_gem_shmem_prime_{begin,end}_cpu_access() can sync
> > + * around CPU accesses.
> > + */
> > +struct sg_table *
> > +drm_gem_shmem_prime_map_dma_buf(struct dma_buf_attachment *attach,
> > +				enum dma_data_direction dir)
> > +{
> > +	struct sg_table *sgt = drm_gem_map_dma_buf(attach, dir);
> > +
> > +	if (!IS_ERR(sgt))
> > +		attach->priv = sgt;
> > +
> > +	return sgt;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_map_dma_buf);
> > +
> > +/**
> > + * drm_gem_shmem_prime_unmap_dma_buf - Default unmap_dma_buf() for exported buffers
> > + * @attach: attachment
> > + * @sgt: SG table to unmap
> > + * @dir: type of access done by this attachment
> > + *
> > + * Default implementation for dma_buf_ops::unmap_dma_buf(). This is just a wrapper
> > + * around drm_gem_unmap_dma_buf() that lets us reset the dma_buf_attachment::priv
> > + * field so that drm_gem_shmem_prime_{begin,end}_cpu_access() don't consider it
> > + * as a mapped attachment to sync against.
> > + */
> > +void drm_gem_shmem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
> > +				       struct sg_table *sgt,
> > +				       enum dma_data_direction dir)
> > +{
> > +	attach->priv = NULL;
> > +	drm_gem_unmap_dma_buf(attach, sgt, dir);
> > +}
> > +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_unmap_dma_buf);
> > +
> > +/**
> > + * drm_gem_shmem_prime_begin_cpu_access - Default end_cpu_access() for exported buffers
> > + * @dma_buf: The exported DMA buffer this acts on
> > + * @dir: direction of the access
> > + *
> > + * Default implementation for dma_buf_ops::begin_cpu_access(). This only takes care of
> > + * cache maintenance.
> > + */
> > +int drm_gem_shmem_prime_begin_cpu_access(struct dma_buf *dma_buf,
> > +					 enum dma_data_direction dir)
> > +{
> > +	struct drm_gem_object *obj = dma_buf->priv;
> > +	struct drm_device *dev = obj->dev;
> > +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > +	struct dma_buf_attachment *attach;
> > +
> > +	dma_resv_lock(obj->resv, NULL);
> > +	if (shmem->sgt)
> > +		dma_sync_sgtable_for_cpu(dev->dev, shmem->sgt, dir);
> > +
> > +	if (shmem->vaddr)
> > +		invalidate_kernel_vmap_range(shmem->vaddr, shmem->base.size);
> > +  
> 
> 
> > +	list_for_each_entry(attach, &dma_buf->attachments, node) {
> > +		struct sg_table *sgt = attach->priv;
> > +
> > +		if (sgt)
> > +			dma_sync_sgtable_for_cpu(attach->dev, sgt, dir);
> > +	}  
> 
> This conflicts with the debugging hack in DMA-buf.
> 
> I've recently send out a patch to fix that, but it hasn't been pushed yet.

I guess you're talking about [1]. I'm not sure this is a problem
actually, because we don't pass the exporter's sg_table, we allocate a
new one each time. As for the importer's sg_table we attach to
dma_buf_attach::priv, it would still be mangled, and I believe it's
fine, because the dma_sync_xxx helpers manipulate dma_addr_t not pages.
Am I missing something?

[1]https://lists.linaro.org/archives/list/linaro-mm-sig@lists.linaro.org/thread/HZDQ7SFOG4722BISB6BEEA34B7QESM2O/