On 8/12/26 01:13, Taimuraz Kaitmazov wrote: > SYNC_BO carries an offset and a size, but amdxdna_flush_bo() honours them > only on the vmap path. An imported BO is tested for first and flushes its > whole scatterlist,
Absolutely clear NAK to that from a DMA-buf maintainer side. Flushing on imported scatterlist of a DMA-buf is a really big NO-GO. If DMA-buf imports are used with the device then the device needs to be able to coherently access the underlying memory. In other words you *CAN'T* call drm_clflush_pages() on imported memory. Regards, Christian. > so a sync costs what the BO is worth rather than what > the caller asked to maintain: on npu4 an imported 64 MiB BO cost 1056 us > to sync at every size from 4 KiB up. Patch 5 reorders the arms so the > vmap path is tried first, and indexes the page-array fallback from the > requested offset. > > The four before it are the ground that has to be solid first. Patch 1 > refuses an I/O memory mapping, which the driver currently stores as if it > were an ordinary kernel address. Patch 2 adds a probe that does not log, > so patch 5 does not make an exporter without a vmap op print on every > ioctl. Patches 3 and 4 fix two ways the ioctl mishandles its own range: a > zero length reaching drm_clflush_virt_range(), and an offset and size > added to the BO address without an overflow check, one level above a > function that checks the same arithmetic. All four stand on their own and > can be taken separately; only patch 5 depends on them. > > v1 did not reach dri-devel, so this is the first version visible there. > It is on lore via the other lists it was copied to: > https://lore.kernel.org/lkml/[email protected]/ > > Changes in v2: > - patch 2: take the device from the GEM object rather than abo->client. > amdxdna_gem_obj_close() clears that pointer under abo->lock, which the > pre-split code held across the log and the split did not. > - new patch 3: return early from a zero-length flush. > - new patch 4: check the sync range for overflow on a device BO. > - patch 5: say why the persistent mapping adds no pin. > > The measurements in patch 5 were taken with the equivalent change in > AMD's out-of-tree xdna-driver, where this merged as #1541. That version > and this one differ only in a page-array fallback mainline has no field > for, reached when the mapping fails and the BO is neither imported nor > shmem backed, and in the name of the mapping helper. The flush and the > helper are otherwise identical. This version is compile-tested; it has > not been booted. > > Patch 1 is from inspection rather than a reproducer. The exporter I can > test against is amdgpu, and amdgpu is the case that cannot reach it: it > implements .pin, so a non peer to peer attachment like this driver's > forces the buffer to GTT before anything maps it. Reproducing it needs a > GPU whose exporter has no .pin, which I do not have paired with an NPU > here. > > Taimuraz Kaitmazov (5): > accel/amdxdna: refuse an I/O memory mapping of an imported BO > accel/amdxdna: add a quiet variant of amdxdna_gem_vmap() > accel/amdxdna: return early from a zero-length flush > accel/amdxdna: check the sync range for overflow on a device BO > accel/amdxdna: flush only the requested range in amdxdna_flush_bo > > drivers/accel/amdxdna/amdxdna_gem.c | 66 +++++++++++++++++++++-------- > 1 file changed, 49 insertions(+), 17 deletions(-) > > -- > 2.55.0 >
