On Wed, 29 Oct 2025 10:30:56 +0100 Maxime Ripard <[email protected]> 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 <[email protected]> wrote: > > > > > 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. > > > > 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.
