Re: [PATCH v3 11/16] accel/thames: Add IOCTL for memory synchronization

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel/thames: thames_ioctl_bo_prep() syncs caches when wait fails
- [Medium] accel/thames: DMA direction mismatch when syncing contiguous BOs
--

commit 85aeec13be820dd1a0897aae141a1833daad8267
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Add IOCTL for memory synchronization

This commit adds DRM_THAMES_BO_PREP and DRM_THAMES_BO_FINI ioctls to the
thames accel driver so userspace can wait for in-flight jobs and synchronize
CPU caches with non-coherent DSP memory.

> diff --git a/drivers/accel/thames/thames_gem.c b/drivers/accel/thames/thames_gem.c
> index c9fd08c7aaf4d..5a4dc497768f8 100644
> --- a/drivers/accel/thames/thames_gem.c
> +++ b/drivers/accel/thames/thames_gem.c
> @@ -942,3 +942,66 @@ int thames_ioctl_bo_mmap_offset(struct drm_device *ddev, void *data, struct drm_
>  	return 0;
>  }
>  
> +int thames_ioctl_bo_prep(struct drm_device *ddev, void *data, struct drm_file *file)
> +{
> +	struct drm_thames_bo_prep *args = data;
> +	struct thames_gem_object *bo;
> +	struct drm_gem_object *gem_obj;
> +	unsigned long timeout = drm_timeout_abs_to_jiffies(args->timeout_ns);
> +	long ret = 0;
> +
> +	if (args->reserved != 0)
> +		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_READ, true, timeout);
> +	if (ret == 0)
> +		ret = timeout ? -ETIMEDOUT : -EBUSY;
> +	else if (ret > 0)
> +		ret = 0;
> +
> +	bo = to_thames_bo(gem_obj);
> +	if (bo->flags & DRM_THAMES_BO_CONTIGUOUS)
> +		dma_sync_single_for_cpu(ddev->dev, bo->base.dma_addr, gem_obj->size,
> +					DMA_BIDIRECTIONAL);
> +	else
> +		dma_sync_sgtable_for_cpu(ddev->dev, bo->shmem.sgt, DMA_BIDIRECTIONAL);

[Severity: High]
Should thames_ioctl_bo_prep() skip the cache invalidation when
dma_resv_wait_timeout() fails?

If the wait times out (-ETIMEDOUT), is polled while fences are still active
(-EBUSY), or is interrupted by a signal (-ERESTARTSYS), ret is negative, yet
the code still calls dma_sync_single_for_cpu() or dma_sync_sgtable_for_cpu().

This transfers DMA buffer ownership to the CPU and invalidates CPU cache
lines while the DSP or DRU DMA engine is still actively accessing the buffer.
It also performs full-buffer cache maintenance on every non-blocking poll or
signal-interrupted retry, and contradicts the struct drm_thames_bo_prep
comment in include/uapi/drm/thames_accel.h:

include/uapi/drm/thames_accel.h:
 * Waits for the DSP to be done with the buffer and invalidates the CPU caches,
 * so that what the DSP wrote is visible to the CPU.  The DSP is not coherent
 * with the CPU, and the wait and the cache invalidation are deliberately the
 * same operation: userspace has no way of skipping the wait and keeping the
 * invalidation, so a mapping is never handed out with stale cache lines.

> +	/* Order the cache maintenance before the CPU reads. */
> +	mb();
> +
> +	drm_gem_object_put(gem_obj);
> +
> +	return ret;
> +}
> +
> +int thames_ioctl_bo_fini(struct drm_device *ddev, void *data, struct drm_file *file)
> +{
> +	struct drm_thames_bo_fini *args = data;
> +	struct thames_gem_object *bo;
> +	struct drm_gem_object *gem_obj;
> +
> +	if (args->reserved != 0)
> +		return -EINVAL;
> +
> +	gem_obj = drm_gem_object_lookup(file, args->handle);
> +	if (!gem_obj)
> +		return -ENOENT;
> +
> +	bo = to_thames_bo(gem_obj);
> +	if (bo->flags & DRM_THAMES_BO_CONTIGUOUS) {
> +		dma_sync_single_for_device(ddev->dev, bo->base.dma_addr, gem_obj->size,
> +					   DMA_BIDIRECTIONAL);

[Severity: Medium]
Does syncing contiguous buffer objects with DMA_BIDIRECTIONAL in
thames_ioctl_bo_prep() and thames_ioctl_bo_fini() mismatch the direction used
when allocating them?

In drivers/accel/thames/thames_gem.c, contiguous buffers are allocated and
initially synced in thames_gem_create() with DMA_TO_DEVICE:

drivers/accel/thames/thames_gem.c:thames_gem_create() {
	...
	bo->alloc_vaddr = dma_alloc_noncoherent(dev, bo->alloc_size, &bo->alloc_dma,
						DMA_TO_DEVICE, GFP_KERNEL | __GFP_NOWARN);
	...
	dma_sync_single_for_device(dev, bo->alloc_dma, bo->alloc_size, DMA_TO_DEVICE);
}

And freed in thames_gem_bo_free() with DMA_TO_DEVICE:

drivers/accel/thames/thames_gem.c:thames_gem_bo_free() {
	...
	if (bo->alloc_vaddr)
		dma_free_noncoherent(drm_dev_dma_dev(obj->dev), bo->alloc_size, bo->alloc_vaddr,
				     bo->alloc_dma, DMA_TO_DEVICE);
}

When CONFIG_DMA_API_DEBUG is enabled, calling dma_sync_single_for_cpu() and
dma_sync_single_for_device() with DMA_BIDIRECTIONAL on an allocation created
with DMA_TO_DEVICE triggers check_sync() warnings for direction mismatch and
for syncing device read-only DMA memory for the CPU.

Should thames_gem_create() and thames_gem_bo_free() use DMA_BIDIRECTIONAL for
contiguous BOs as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=11