On Thu, 16 Oct 2025 10:13:25 +0200
Maxime Ripard <[email protected]> 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 <[email protected]>
> > Reviewed-by: Steven Price <[email protected]>
> > ---
> > 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.
>
> Maxime
