On Wed, Oct 15, 2025 at 12:04 PM Boris Brezillon <[email protected]> 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 > + */ > +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), > + > + /** @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, > +}; > + > /** > * struct drm_gem_object_funcs - GEM object functions > */ > @@ -191,6 +218,21 @@ struct drm_gem_object_funcs { > */ > int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma); > > + /** > + * @sync: > + * > + * Prepare for CPU/device access. This can involve migration of > + * a buffer to the system-RAM/VRAM, or for UMA, flushing/invalidating > + * the CPU caches. The range can be used to optimize the > synchronization > + * when possible.
This has gone in a very different direction from the version I sent out and the added generality makes me really nervous. The idea of sync involving migration and that the range is a mere hint are antithetical with Vulkan. It's a very GLish design that assumes that a BO is exclusively used by one of the CPU or the GPU at the same time. This simply isn't the case in modern APIs. Older DRM uAPIs (as well as dma-buf itself) are littered with such ioctls and we're in the process of deleting them all. If the BO needs to be migrated in order to be accessed from the CPU, that needs to happen on map, not on some sort of begin/end. Or better yet, just disallow mapping such buffers. Once the client has a map, they are free to access from the CPU while stuff is running on the GPU. They have to be careful, of course, not to cause data races, but accessing the same BO from the CPU and GPU or even the same range is totally okay if you aren't racing. As a corollary, just don't map PRIME buffers. And the range really shouldn't be just a hint. With Vulkan, clients are regularly sub-allocating from larger memory objects. If they ask to flush 64B and end up flushing 64M, that's pretty bad. All we need is something which lets us trap through to the kernel for CPU cache management. That's all we need and that's really all it should do. ~Faith > + * > + * Returns 0 on success, -errno otherwise. > + * > + * This callback is optional. > + */ > + int (*sync)(struct drm_gem_object *obj, size_t offset, size_t size, > + enum drm_gem_object_access_flags access); > + > /** > * @evict: > * > @@ -559,6 +601,9 @@ void drm_gem_unlock(struct drm_gem_object *obj); > int drm_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map); > void drm_gem_vunmap(struct drm_gem_object *obj, struct iosys_map *map); > > +int drm_gem_sync(struct drm_gem_object *obj, size_t offset, size_t size, > + enum drm_gem_object_access_flags access); > + > int drm_gem_objects_lookup(struct drm_file *filp, void __user *bo_handles, > int count, struct drm_gem_object ***objs_out); > struct drm_gem_object *drm_gem_object_lookup(struct drm_file *filp, u32 > handle); > -- > 2.51.0 >
