Re: [PATCH v4 02/14] drm/gem: Add a drm_gem_object_funcs::sync() and a drm_gem_sync() helper

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

On Wed, 29 Oct 2025 10:30:56 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> On Thu, Oct 16, 2025 at 03:07:04PM +0200, Boris Brezillon wrote:
> > On Thu, 16 Oct 2025 14:57:08 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >   
> > > On Thu, 16 Oct 2025 10:13:25 +0200
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > On Wed, Oct 15, 2025 at 06:03:14PM +0200, Boris Brezillon wrote:    
> > > > > Prepare things for standardizing synchronization around CPU accesses
> > > > > of GEM buffers. This will be used to provide default
> > > > > drm_gem_dmabuf_{begin,end}_cpu_access() implementations, and provide
> > > > > a way for drivers to add their own ioctls to synchronize CPU
> > > > > writes/reads when they can't do it directly from userland.
> > > > > 
> > > > > v2:
> > > > > - New commit
> > > > > 
> > > > > v3:
> > > > > - No changes
> > > > > 
> > > > > v4:
> > > > > - Add Steve's R-b
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Reviewed-by: Steven Price <steven.price@arm.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_gem.c | 10 +++++++++
> > > > >  include/drm/drm_gem.h     | 45 +++++++++++++++++++++++++++++++++++++++
> > > > >  2 files changed, 55 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > > index a1a9c828938b..a1431e4f2404 100644
> > > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > > @@ -1333,6 +1333,16 @@ void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map)
> > > > >  }
> > > > >  EXPORT_SYMBOL(drm_gem_vunmap);
> > > > >  
> > > > > +int drm_gem_sync(struct drm_gem_object *obj, size_t offset, size_t size,
> > > > > +		 enum drm_gem_object_access_flags access)
> > > > > +{
> > > > > +	if (obj->funcs->sync)
> > > > > +		return obj->funcs->sync(obj, offset, size, access);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +EXPORT_SYMBOL(drm_gem_sync);
> > > > > +
> > > > >  /**
> > > > >   * drm_gem_lock_reservations - Sets up the ww context and acquires
> > > > >   * the lock on an array of GEM objects.
> > > > > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > > > > index 8d48d2af2649..1c33e59ab305 100644
> > > > > --- a/include/drm/drm_gem.h
> > > > > +++ b/include/drm/drm_gem.h
> > > > > @@ -66,6 +66,33 @@ enum drm_gem_object_status {
> > > > >  	DRM_GEM_OBJECT_ACTIVE    = BIT(2),
> > > > >  };
> > > > >  
> > > > > +/**
> > > > > + * enum drm_gem_object_status - bitmask describing GEM access types to prepare for      
> > > > 
> > > > Treating an enum as a bitmask is a bit weird to me. I'd say either have
> > > > a bitmask with BIT(enum values), or no enum at all.    
> > > 
> > > I'll drop the enum and make it pure defines.
> > >   
> > > >     
> > > > > + */
> > > > > +enum drm_gem_object_access_flags {
> > > > > +	/** @DRM_GEM_OBJECT_CPU_ACCESS: Prepare for a CPU access. */
> > > > > +	DRM_GEM_OBJECT_CPU_ACCESS = 0,
> > > > > +
> > > > > +	/** @DRM_GEM_OBJECT_DEV_ACCESS: Prepare for a device access. */
> > > > > +	DRM_GEM_OBJECT_DEV_ACCESS = BIT(0),
> > > > > +
> > > > > +	/** @DRM_GEM_OBJECT_ACCESSOR_MASK: Mask used to check the entity doing the access. */
> > > > > +	DRM_GEM_OBJECT_ACCESSOR_MASK = BIT(0),      
> > > > 
> > > > Do we really want to have to variants with the same discriminant? If so,
> > > > we should document why it's something we want.
> > > >     
> > > > > +	/** @DRM_GEM_OBJECT_READ_ACCESS: Prepare for read-only accesses. */
> > > > > +	DRM_GEM_OBJECT_READ_ACCESS = BIT(1),
> > > > > +
> > > > > +	/** @DRM_GEM_OBJECT_WRITE_ACCESS: Prepare for write-only accesses. */
> > > > > +	DRM_GEM_OBJECT_WRITE_ACCESS = BIT(2),
> > > > > +
> > > > > +	/** @DRM_GEM_OBJECT_RW_ACCESS: Prepare for a read/write accesses. */
> > > > > +	DRM_GEM_OBJECT_RW_ACCESS = DRM_GEM_OBJECT_READ_ACCESS |
> > > > > +				   DRM_GEM_OBJECT_WRITE_ACCESS,
> > > > > +
> > > > > +	/** @DRM_GEM_OBJECT_ACCESS_TYPE_MASK: Mask used to check the access type. */
> > > > > +	DRM_GEM_OBJECT_ACCESS_TYPE_MASK = DRM_GEM_OBJECT_RW_ACCESS,      
> > > > 
> > > > Same thing.
> > > > 
> > > > Or is it that you encode both the direction and access type, and have a
> > > > mask to isolate each?    
> > > 
> > > This ^.
> > >   
> > > > 
> > > > If so, we should really move it out from an enum into defines, or treat each
> > > > separately like dma_sync_does.    
> > > 
> > > Sure, I can do that.  
> > 
> > Actually, looking at the enum just above the one added in this patch
> > (drm_gem_object_status), it seems that it has the same flaws, and I
> > think it was the reason I went for this enum-based approach, because I
> > tend to be consistent with the code base I'm modifying.
> > 
> > Now, I get that defining flags with an enum and then composing those to
> > then pass the composition to some helper pretending it's still an enum
> > only works in C (because with C you can do anything you want :D), and
> > probably not if you're in pedantic mode. But if we want to enforce
> > that, we should probably fix the existing code base, otherwise this
> > will keep happening ;-). And no, before you ask, I'm not volunteering
> > for this :P.  
> 
> I'm fine with it not being totally consistent with the other enums
> around, if anything because those aren't consistent with the typical way
> we use enums in C.

I ended up dropping the generic ::sync() hook, and the enum that goes
with it. There's a new enum at the drm_gem_shmem level, but this time
it's a true enum, not a bitmask.