[ESI][Runtime] XRT support for host memory access #8122
Conversation
Adds support for allocating, flushing, and getting the device pointer on XRT platforms.
teqdruid
force-pushed
the
teqdruid/esirt/xrthostmem
branch
from
1c07d92 to
ef7e3c0
Compare
There was a problem hiding this comment.
Don't know much about the XRT intricacies, so someone else would be better suited to review that. A few other style comments... (and I'll never get over the use of raw pointers and new in the ESI runtime used anywhere outside of an immediate wrapping of a smart pointer, but you know that 🤷).
| /// It is recommended to use 'sync' to flush the caches before executing any | ||
| /// DMA. | ||
| virtual void flush() override { bo.sync(XCL_BO_SYNC_BO_TO_DEVICE); } | ||
|
|
There was a problem hiding this comment.
Is there any sense in adding 'dirty'-flags to HostMemRegion s.t. when a write has been performed to it, HostMem implementations can check/verify that flush has been called?
I'm just a bit sceptical about the "it's recommended" phrase (although that probably originates from xrt documentation and not you). I can accept "it's recommended" if it's purely performance related, but if not, it either is required or it isn't.
There was a problem hiding this comment.
There isn't a 'write' method on HostMemRegion. Standard use case is that the application writes directly to the host pointer. Avoids a memcpy, which as you well know is a killer on the host. So I have no way of (quickly) determining whether the buffer is dirty.
As for it's recommended... certain platforms will require it, and certain platforms won't. In the interest of applications writing platform-independent code, I wrote that it's recommended. As for Xrt, I found some documentation saying:
XRT Native API: xrt::bo::sync()) should be used. Though these API will not do any DMA operation, they are used for Cache Invalidate/Flush as the application works on the Cache memory.
so it sounds like it's required. I'll update the comment.
There was a problem hiding this comment.
Actually, I'm surprised that it's not required for the Catapult based platforms on x86. But I don't fully understand how this new user-pointer translation support works. It must go through the cache system.
There was a problem hiding this comment.
Said documentation: https://xilinx.github.io/XRT/master/html/hm.html
teqdruid
force-pushed
the
teqdruid/esirt/xrthostmem
branch
from
6ba4fe8 to
92174df
Compare
Adds support for allocating, flushing, and getting the device pointer on XRT platforms.