[ESI][PyCDE] Callback service #7153
Conversation
Call functions that the host software registers from hardware.
teqdruid
force-pushed
the
teqdruid/esi-callbacks
branch
from
1079c9d
to
8b09774
Compare
| class EsiTesterTop(Module): | ||
| clk = Clock() | ||
| rst = Reset() | ||
|
|
||
| @generator | ||
| def construct(ports): | ||
| PrintfExample(clk=ports.clk, rst=ports.rst) |
There was a problem hiding this comment.
Is it necessary to have EsiTesterTop here? Can't we just instantiate PrintfExample in the pycde.System? It just looks a bit redundant to have a secondary top module which does nothing but instantiate the first.
There was a problem hiding this comment.
The idea is that this test would expand to test multiple new advanced features.
| @@ -0,0 +1,53 @@ | |||
| # REQUIRES: esi-runtime, esi-cosim, rtl-sim, esitester | |||
There was a problem hiding this comment.
esitester seems very vague for the purpose of this file. I presume that this file is intended to test the callback service, and callback service only? I'd really like to the see the testing story for ESI be elaborated, and also made easier to wrap ones' head around. Part of this - in my opinion - is to properly split out test files into more meaningfully named things (e.g. would this be more meaningfully named PyCDE/integration_test/esi/test_callbackservice.py?).
There was a problem hiding this comment.
I lean the opposite direction. I would like to see a monolithic test for ESI features which could be synthesized to a single FPGA image. Medium term, it'd probably be useful for board and BSP developers to test correctness and performance. This test was named with that intention in mind.
| """Expose a bundle to the host as a function. Bundle _must_ have 'arg' and | ||
| 'result' channels going FROM the server and TO the server, respectively.""" |
There was a problem hiding this comment.
There's a lot of this host/server/... split brain terminology that needs to be refactored in our upcoming work 👀... Seeing as we're trending towards supporting everything-can-call-everything in the system, alternatives could be source/target,caller/calle, or just unify on using from/to.
There was a problem hiding this comment.
Yeah, I agree. We'll have to decide on appropriate terms, which actually won't be easy given the multiple processes we'll be developing for CAM.
| def CallServiceDeclOp : ESI_Op<"service.std.call", | ||
| [SingleBlock, NoTerminator, HasParent<"::mlir::ModuleOp">, | ||
| Symbol, DeclareOpInterfaceMethods<ServiceDeclOpInterface>]> { | ||
| let summary = "Service against which hardware can call into software"; |
There was a problem hiding this comment.
Is this necessarily hardware calling software? Again, this might be refactored in upcoming work, but in theory this service could be served by things other than software, no?
There was a problem hiding this comment.
The calls which this service exposes could be implemented in hardware, but it's intended for software. For making calls in hardware, I suspect this service wouldn't make a lot of sense. I think directly connecting function calls to functions makes more sense.
| std::lock_guard<std::mutex> g(listenerMutex); | ||
| for (auto &[channel, cb] : listeners) { | ||
| assert(channel && "Null channel in listener list"); | ||
| if (channel->read(data)) | ||
| portUnlockWorkList.emplace_back(channel, cb, std::move(data)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Based on the implementation of ReadChannelPort's read functions, nothings going to be particularly performant here. However, i was wondering whether it'd be more futureproof to base your implementation here on ReadChannelPort::readAsync, and then instead of looping over the listeners map, you'd signal a condition variable whenever a new listener was added, modify readAsync s.t. it also takes an optional callback, and in that callback, assign portUnlockWorkList. This, with the goal of making AcceleratorServiceThread::Impl::loop the final implementation, which will be performant, once readAsync is properly implemented.
There was a problem hiding this comment.
Actually, I'm considering re-implementing the other PR using this service loop.
| @@ -0,0 +1,86 @@ | |||
| //===- esitester.cpp - ESI accelerator test/example tool ------------------===// | |||
There was a problem hiding this comment.
as above... esitester makes it sound like this is applicable for testing any ESI application, but it seems like a callback-specific thing.
Call software functions from hardware.
Call software functions from hardware.