Re: [PATCH RFC 02/12] drm/shmem: Provide a generic {begin, end}_cpu_access() implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/shmem: wrong vmap flush order in drm_gem_shmem_prime_end_cpu_access()
--

commit 4543a541900e1a7c8880d3f04fbf76393b153371
Author: Boris Brezillon <boris.brezillon@collabora.com>

drm/shmem: Provide a generic {begin, end}_cpu_access() implementation

This commit adds default dma_buf_ops implementations for map_dma_buf,
unmap_dma_buf, begin_cpu_access, and end_cpu_access to drm_gem_shmem_helper
so that CPU caches and vmap ranges are synchronized across the exporter and
all mapped attachments.

[Severity: Medium]
In drivers/gpu/drm/drm_gem_shmem_helper.c, inside
drm_gem_shmem_prime_end_cpu_access() (around line 1093), should
flush_kernel_vmap_range(shmem->vaddr, shmem->base.size) be called before
iterating over dma_buf->attachments and calling
dma_sync_sgtable_for_device(attach->dev, sgt, dir)?

Currently, drm_gem_shmem_prime_end_cpu_access() syncs each attachment's
sg_table for the device first, then calls flush_kernel_vmap_range() on
shmem->vaddr, and finally calls dma_sync_sgtable_for_device() on shmem->sgt.

When a cacheable shmem GEM object (with map_wc set to false) is exported,
mapped by an importing device via drm_gem_shmem_prime_map_dma_buf(), and
also mapped into kernel virtual address space via drm_gem_shmem_vmap_locked(),
modifications made through shmem->vaddr may sit in a virtually indexed or
aliasing cache line (such as on 32-bit arm with vivt or vipt-aliasing caches
and an outer l2 cache, or mips with primary d-cache aliasing). Calling
dma_sync_sgtable_for_device() on the importer attachments before
flush_kernel_vmap_range() means the virtual alias in shmem->vaddr has not yet
been written back when dma_sync_sgtable_for_device() cleans the physical
mapping, cleans the outer cache, or copies to swiotlb bounce buffers, which
can leave importing devices reading stale data.

As documented in Documentation/core-api/cachetlb.rst and done in other
dma-buf exporters such as system_heap_dma_buf_end_cpu_access() and
cma_heap_dma_buf_end_cpu_access(), flush_kernel_vmap_range() should happen
before all dma_sync_sgtable_for_device() calls.

Similarly, in drm_gem_shmem_prime_begin_cpu_access() (around line 1058),
invalidate_kernel_vmap_range(shmem->vaddr, shmem->base.size) is placed
between syncing shmem->sgt and syncing dma_buf->attachments. Should
invalidate_kernel_vmap_range() be called after all
dma_sync_sgtable_for_cpu() calls have completed so that both exporter and
importer sg_table syncs are ordered consistently with respect to the kernel
vmap range invalidation?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-v3d-cached-maps-v1-0-0a6a6b363382@igalia.com?part=2