Re: [PATCH v7 05/10] accel/rocket: Add IOCTLs for synchronizing memory accesses

[Robin Murphy <robin.murphy@arm.com>]

On 2025-06-06 7:28 am, Tomeu Vizoso wrote:
> The NPU cores have their own access to the memory bus, and this isn't
> cache coherent with the CPUs.
> 
> Add IOCTLs so userspace can mark when the caches need to be flushed, and
> also when a writer job needs to be waited for before the buffer can be
> accessed from the CPU.
> 
> Initially based on the same IOCTLs from the Etnaviv driver.
> 
> v2:
> - Don't break UABI by reordering the IOCTL IDs (Jeff Hugo)
> 
> v3:
> - Check that padding fields in IOCTLs are zero (Jeff Hugo)
> 
> v6:
> - Fix conversion logic to make sure we use DMA_BIDIRECTIONAL when needed
>    (Lucas Stach)
> 
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
> ---
>   drivers/accel/rocket/rocket_drv.c |  2 +
>   drivers/accel/rocket/rocket_gem.c | 82 +++++++++++++++++++++++++++++++++++++++
>   drivers/accel/rocket/rocket_gem.h |  5 +++
>   include/uapi/drm/rocket_accel.h   | 37 ++++++++++++++++++
>   4 files changed, 126 insertions(+)
> 
> diff --git a/drivers/accel/rocket/rocket_drv.c b/drivers/accel/rocket/rocket_drv.c
> index 4ab78193c186dfcfc3e323f16c588e85e6a8a334..eb9284ee2511f730afe6a532225c2706ce0e2822 100644
> --- a/drivers/accel/rocket/rocket_drv.c
> +++ b/drivers/accel/rocket/rocket_drv.c
> @@ -62,6 +62,8 @@ static const struct drm_ioctl_desc rocket_drm_driver_ioctls[] = {
>   
>   	ROCKET_IOCTL(CREATE_BO, create_bo),
>   	ROCKET_IOCTL(SUBMIT, submit),
> +	ROCKET_IOCTL(PREP_BO, prep_bo),
> +	ROCKET_IOCTL(FINI_BO, fini_bo),
>   };
>   
>   DEFINE_DRM_ACCEL_FOPS(rocket_accel_driver_fops);
> diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c
> index 61b7f970a6885aa13784daa1222611a02aa10dee..07024b6e71bf544dc7f00b008b9afb74b0c4e802 100644
> --- a/drivers/accel/rocket/rocket_gem.c
> +++ b/drivers/accel/rocket/rocket_gem.c
> @@ -113,3 +113,85 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *
>   
>   	return ret;
>   }
> +
> +static inline enum dma_data_direction rocket_op_to_dma_dir(u32 op)
> +{
> +	op &= ROCKET_PREP_READ | ROCKET_PREP_WRITE;
> +
> +	if (op == ROCKET_PREP_READ)
> +		return DMA_FROM_DEVICE;
> +	else if (op == ROCKET_PREP_WRITE)
> +		return DMA_TO_DEVICE;
> +	else
> +		return DMA_BIDIRECTIONAL;
> +}
> +
> +int rocket_ioctl_prep_bo(struct drm_device *dev, void *data, struct drm_file *file)
> +{
> +	struct drm_rocket_prep_bo *args = data;
> +	unsigned long timeout = drm_timeout_abs_to_jiffies(args->timeout_ns);
> +	struct rocket_device *rdev = to_rocket_device(dev);
> +	struct drm_gem_object *gem_obj;
> +	struct drm_gem_shmem_object *shmem_obj;
> +	bool write = !!(args->op & ROCKET_PREP_WRITE);
> +	long ret = 0;
> +
> +	if (args->op & ~(ROCKET_PREP_READ | ROCKET_PREP_WRITE))
> +		return -EINVAL;
> +
> +	gem_obj = drm_gem_object_lookup(file, args->handle);
> +	if (!gem_obj)
> +		return -ENOENT;
> +
> +	ret = dma_resv_wait_timeout(gem_obj->resv, dma_resv_usage_rw(write),
> +				    true, timeout);
> +	if (!ret)
> +		ret = timeout ? -ETIMEDOUT : -EBUSY;
> +
> +	shmem_obj = &to_rocket_bo(gem_obj)->base;
> +
> +	for (unsigned int core = 1; core < rdev->num_cores; core++) {

Huh? If you need to sync the BO memory ever, then you need to sync it 
for the same device it was mapped, and certainly not 0 or 2+ times 
depending on how may cores happen to be enabled. Please throw 
CONFIG_DMA_API_DEBUG at this.

> +		dma_sync_sgtable_for_cpu(rdev->cores[core].dev, shmem_obj->sgt,
> +					 rocket_op_to_dma_dir(args->op));

Hmm, the intent of the API is really that the direction for sync should 
match the direction for map and unmap too; if it was mapped 
DMA_BIDIRECTIONAL then it should be synced DMA_BIDIRECTIONAL. If you 
have BOs which are really only used for one-directional purposes then 
they should be mapped as such at creation.

Does anything actually prevent one thread form trying to read from a 
buffer while another thread is writing it, and thus the read 
inintuitively destroying newly-written data (and/or the write 
unwittingly destroying its own data in FINI_BO because last_cpu_prep_op 
got overwritten)? Unless there's a significant measurable benefit to 
trying to be clever here (of which I'm somewhjat doubtful), I would be 
strongly inclined to just keep things simple and straightforward.

Thanks,
Robin.

> +	}
> +
> +	to_rocket_bo(gem_obj)->last_cpu_prep_op = args->op;
> +
> +	drm_gem_object_put(gem_obj);
> +
> +	return ret;
> +}
> +
> +int rocket_ioctl_fini_bo(struct drm_device *dev, void *data, struct drm_file *file)
> +{
> +	struct rocket_device *rdev = to_rocket_device(dev);
> +	struct drm_rocket_fini_bo *args = data;
> +	struct drm_gem_shmem_object *shmem_obj;
> +	struct rocket_gem_object *rkt_obj;
> +	struct drm_gem_object *gem_obj;
> +
> +	if (args->reserved != 0) {
> +		drm_dbg(dev, "Reserved field in drm_rocket_fini_bo struct should be 0.\n");
> +		return -EINVAL;
> +	}
> +
> +	gem_obj = drm_gem_object_lookup(file, args->handle);
> +	if (!gem_obj)
> +		return -ENOENT;
> +
> +	rkt_obj = to_rocket_bo(gem_obj);
> +	shmem_obj = &rkt_obj->base;
> +
> +	WARN_ON(rkt_obj->last_cpu_prep_op == 0);
> +
> +	for (unsigned int core = 1; core < rdev->num_cores; core++) {
> +		dma_sync_sgtable_for_device(rdev->cores[core].dev, shmem_obj->sgt,
> +					    rocket_op_to_dma_dir(rkt_obj->last_cpu_prep_op));
> +	}
> +
> +	rkt_obj->last_cpu_prep_op = 0;
> +
> +	drm_gem_object_put(gem_obj);
> +
> +	return 0;
> +}
> diff --git a/drivers/accel/rocket/rocket_gem.h b/drivers/accel/rocket/rocket_gem.h
> index e8a4d6213fd80419be2ec8af04583a67fb1a4b75..a52a63cd78339a6150b99592ab5f94feeeb51fde 100644
> --- a/drivers/accel/rocket/rocket_gem.h
> +++ b/drivers/accel/rocket/rocket_gem.h
> @@ -12,12 +12,17 @@ struct rocket_gem_object {
>   	struct iommu_domain *domain;
>   	size_t size;
>   	u32 offset;
> +	u32 last_cpu_prep_op;
>   };
>   
>   struct drm_gem_object *rocket_gem_create_object(struct drm_device *dev, size_t size);
>   
>   int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *file);
>   
> +int rocket_ioctl_prep_bo(struct drm_device *dev, void *data, struct drm_file *file);
> +
> +int rocket_ioctl_fini_bo(struct drm_device *dev, void *data, struct drm_file *file);
> +
>   static inline
>   struct  rocket_gem_object *to_rocket_bo(struct drm_gem_object *obj)
>   {
> diff --git a/include/uapi/drm/rocket_accel.h b/include/uapi/drm/rocket_accel.h
> index cb1b5934c201160e7650aabd1b3a2b1c77b1fd7b..b5c80dd767be56e9720b51e4a82617a425a881a1 100644
> --- a/include/uapi/drm/rocket_accel.h
> +++ b/include/uapi/drm/rocket_accel.h
> @@ -13,9 +13,13 @@ extern "C" {
>   
>   #define DRM_ROCKET_CREATE_BO			0x00
>   #define DRM_ROCKET_SUBMIT			0x01
> +#define DRM_ROCKET_PREP_BO			0x02
> +#define DRM_ROCKET_FINI_BO			0x03
>   
>   #define DRM_IOCTL_ROCKET_CREATE_BO		DRM_IOWR(DRM_COMMAND_BASE + DRM_ROCKET_CREATE_BO, struct drm_rocket_create_bo)
>   #define DRM_IOCTL_ROCKET_SUBMIT			DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_SUBMIT, struct drm_rocket_submit)
> +#define DRM_IOCTL_ROCKET_PREP_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_PREP_BO, struct drm_rocket_prep_bo)
> +#define DRM_IOCTL_ROCKET_FINI_BO		DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_FINI_BO, struct drm_rocket_fini_bo)
>   
>   /**
>    * struct drm_rocket_create_bo - ioctl argument for creating Rocket BOs.
> @@ -39,6 +43,39 @@ struct drm_rocket_create_bo {
>   	__u64 offset;
>   };
>   
> +#define ROCKET_PREP_READ        0x01
> +#define ROCKET_PREP_WRITE       0x02
> +
> +/**
> + * struct drm_rocket_prep_bo - ioctl argument for starting CPU ownership of the BO.
> + *
> + * Takes care of waiting for any NPU jobs that might still use the NPU and performs cache
> + * synchronization.
> + */
> +struct drm_rocket_prep_bo {
> +	/** Input: GEM handle of the buffer object. */
> +	__u32 handle;
> +
> +	/** Input: mask of ROCKET_PREP_x, direction of the access. */
> +	__u32 op;
> +
> +	/** Input: Amount of time to wait for NPU jobs. */
> +	__s64 timeout_ns;
> +};
> +
> +/**
> + * struct drm_rocket_fini_bo - ioctl argument for finishing CPU ownership of the BO.
> + *
> + * Synchronize caches for NPU access.
> + */
> +struct drm_rocket_fini_bo {
> +	/** Input: GEM handle of the buffer object. */
> +	__u32 handle;
> +
> +	/** Reserved, must be zero. */
> +	__u32 reserved;
> +};
> +
>   /**
>    * struct drm_rocket_task - A task to be run on the NPU
>    *
>