Re: [PATCH v2 0/5] accel/amdxdna: honour the SYNC_BO range

[Christian König]

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/20260811204556.875037-1-taimuraz@xxxxxxxxxxxxx/
> 
> 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
>