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

[Lizhi Hou <lizhi.hou@amd.com>]

Hi Christian,

Thanks for pointing this out.

Taimuraz, this is not introduced by your patch. And the current code 
violates dma-buf protocol. It look we need to unconditionally return 
-EOPNOTSUPP for imported BO at the beginning of amdxdna_flush_bo(). 
Could you help to modify your patch 1 for this if it makes sense?


Thanks,

Lizhi

On 8/12/26 01:57, Christian König wrote:
> 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@kaitmazov.com/
>>
>> 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
>>