Re: [PATCH 1/7] drm/shmem: Add a drm_gem_shmem_sync_mmap() helper

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

Hi Adrian,

On Mon, 1 Sep 2025 09:18:49 +0100
Adrian Larumbe <adrian.larumbe@collabora.com> wrote:

> Hi Faith,
> 
> On 22.08.2025 10:29, Faith Ekstrand wrote:
> > This enables syncing mapped GEM objects between the CPU and GPU via calls
> > to dma_sync_*().  It's a bit annoying as it requires walking the sg_table
> > so it's best if every driver doesn't hand-roll it.
> >
> > Signed-off-by: Faith Ekstrand <faith.ekstrand@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 64 ++++++++++++++++++++++++++
> >  include/drm/drm_gem_shmem_helper.h     |  3 ++
> >  2 files changed, 67 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 8ac0b1fa5287..50907c1fa11f 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -658,6 +658,70 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
> >  }
> >  EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
> >
> > +/**
> > + * drm_gem_shmem_sync_mmap - Sync memor-mapped data to/from the device
> > + * @shmem: shmem GEM object
> > + * @offset: Offset into the GEM object
> > + * @size: Size of the area to sync
> > + *
> > + * Returns:
> > + * 0 on success or a negative error code on failure.
> > + */
> > +int
> > +drm_gem_shmem_sync_mmap(struct drm_gem_shmem_object *shmem,
> > +			size_t offset, size_t size,
> > +			enum dma_data_direction dir)
> > +{
> > +	struct drm_device *dev = shmem->base.dev;
> > +	struct sg_table *sgt;
> > +	struct scatterlist *sgl;
> > +	unsigned int count;
> > +
> > +	if (dir == DMA_NONE)
> > +		return 0;
> > +
> > +	/* Don't bother if it's WC-mapped */
> > +	if (shmem->map_wc)
> > +		return 0;
> > +
> > +	if (size == 0)
> > +		return 0;
> > +
> > +	if (offset + size < offset || offset + size > shmem->base.size)
> > +		return -EINVAL;
> > +
> > +	sgt = drm_gem_shmem_get_pages_sgt(shmem);
> > +	if (IS_ERR(sgt))
> > +		return PTR_ERR(sgt);  
> 
> This will allocate pages when the BO had no backing storage yet, otherwise it'll increase the
> refcnt on those pages, but seems to me this will leave the reference count imbalanced.

That may be how we want things to be, but that's not the case in
practice. At the moment, drm_gem_shmem_get_pages_sgt() only increases
the pages refcount if no sgt exists and one needs to be created,
otherwise it simply returns the sgt attached to drm_gem_shmem_object.
This implicit ref on pages is only released when the GEM object is
destroyed, meaning the sgt/pages lifetime is bound to the GEM object
lifetime. That's definitely something we need to address if we want to
have a generic GEM-shmem shrinker, but I don't think this is needed
here.

> 
> I'd say running this function on an object with no storage backing should be considered a no-op:
> 
> ```
> 	if (!shmem->pages)
> 		return 0;
> ```

The mmap()/vmap() implementations call get_pages(), so I would expect
the !shmem->pages case to return an error, not zero. Of course, it
no longer stands if drivers overload the mmap() implementation and call
get_pages() in their fault handler path, in which case, I agree,
returning zero is better, but that's probably something I would let the
drivers check themselves before calling drm_gem_shmem_sync_mmap().

> 
> On top of that, there are code paths in which we allocate pages, but produce no sgtable for them,
> like drm_gem_shmem_mmap(), so maybe we could do as follows:
> 
> ```
> 	if (!shmem->sgt) {
> 		int ret;
> 
> 		ret = dma_resv_lock_interruptible(shmem->base.resv, NULL);
> 		if (ret)
> 			return ret;
> 
> 		sgt = drm_gem_shmem_get_sg_table(shmem);
> 		if (IS_ERR(sgt))
> 			return ret;
> 
> 		shmem->sgt = sgt;
> 	}
> ```

I'm not too sure we want to force an sgt creation if the driver doesn't
need it. What we want to do though is force drivers who need an sgt
to explicitly call _get_pages() instead of relying on the implicit
pages ref currently held by shmem::sgt. But it's all orthogonal to the
changes being proposed in this patch, I think.

Regards,

Boris