| Message ID | 20211120111313.106621-1-jacopo@jmondi.org |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On Sat, Nov 20, 2021 at 12:13:01PM +0100, Jacopo Mondi wrote: > Marked as WIP as I've not completed the documentation yet, as I wish to > validate the API first. Also, I'm sure I've missed pieces here and there but I > want this out to discuss it before getting it into a mergeable state. > > Reworked the API as suggested and discussed in v1. > > v1->v2: > > Major changes: > - removed notifiers and timers from Fence. A Fence is now an wrapper that owns > a file descriptor > - Add timers and notifiers to Request::Private and add the > Request::Private::prepare interface > > Smaller changes: > - Expand Request::Private to move all internal fields > - Remove Fence move semantic. A Fence lives in a Framebuffer. > - A few minor patches on top > > Fence application API: > > 1 unique_ptr<Fence> fence = make_unique<Fence>(FileDescriptor &fd); > 2 request->addBuffer(..., move(fence)) > 3 frameBuffer.set(fence) > 4 camera->queueRequest(request) > 5 pipelineHandler.queueRequest() > 6 request.prepare() > 7 request.prepared.emit() > 8 pipelineHandler.doQueueRequest() > 9 if (request.privateState.Ready()) > 10 doQueueRequestDevice(request) > 11 else > 12 request->cancel() > 13 requestCompleted() > 14 if buffer.fence() > 15 FileDescriptor fd = std::move(buffer.fence.fd()) > > What I don't like > > - It's a clunky API > > FileDescriptor fenceFd = FileDescriptor(std::move(buffer.fence)); > std::unique_ptr<Fence> fence = std::make_unique<Fence>(fenceFd); > descriptor->request_->addBuffer(cameraStream->stream(), > frameBuffer, std::move(fence)); > > - Fence creation with a FileDescriptor & does not enforce that the > FileDescriptor should not have duplicated the integer file descriptor A unique_ptr-like fd wrapper may help here. > - Fence constructor takes a FileDescriptor &, but moves the FileDescriptor > internally not to duplicate the fd. The FileDescriptor passed as reference is > invalidated but nothing in the API suggests so I had missed the std::move() in Fence::Private::Private() in the review of 02/12. The FileDescriptor isn't moved despite std::move() as the reference passed to the constructor is const. There is however no duplication, as FileDescriptor implements implicit sharing of the fd. > - Request::addBuffer() moves the Fence to the Framebuffer not to the request > even if the function prototype suggest differently That part doesn't really bother me, but the fact that fences that have timed out are returned through FrameBuffer create an asymmetry that would be nice to fix. I'm not sure how though. > - The API to extract the FileDescritptor from a completed request requires > the user to now that it has to move the FileDescritpro out, nothing suggest > that I've commented on that in the review of the corresponding patches. > - Request now has Request::Private::Status and Request::Status Ditto. > - Fence are an empty wrapper around a FileDescriptor. That's ok-ish as it > allows to support more synchronization methods in future without changing the > API towards the Fence class users We'll likely need to create a base Fence class and a derived SyncObjectFence (or similar) class, but that can go on top. > - Timeouts are per-request not per fence. If we need to have it per-fence the > core infrastructure need to be changed I don't think we'll need per-fence timeouts as long as we handle fences in the core. Once pipeline handlers will want to handle them manually, we'll need a different API, and we can't predict yet what that will be. > Buildbot https://buildbot.libcamera.org/#/builders/1/builds/54 > > CTS: > > ======================================================= > =============== Summary =============== > Total Run time: 19m 58s > 1/1 modules completed > Total Tests : 231 > PASSED : 230 > FAILED : 1 > ============== End of Results ============== > ============================================ > > Thanks > j > > Jacopo Mondi (11): > libcamera: fence: Introduce Fence > libcamera: framebuffer: private: Add Fence > libcamera: request: Add Fence to Request::addBuffer() > test: fence: Add test for the Fence class > libcamera: pipeline_handler: Split request queueing > libcamera: pipeline: Introduce stopDevice() > libcamera: request: Add Request::Private::prepare() > libcamera: pipeline_handler: Prepare Request > android: Remove CameraWorker > test: camera: Fix trivial spelling mistaken > libcamera: Add tracing to meson summary > > Laurent Pinchart (1): > libcamera: request: Make Request class Extensible > > include/libcamera/fence.h | 36 ++ > include/libcamera/framebuffer.h | 3 + > include/libcamera/internal/fence.h | 37 ++ > include/libcamera/internal/framebuffer.h | 9 + > include/libcamera/internal/meson.build | 2 + > include/libcamera/internal/pipeline_handler.h | 9 +- > include/libcamera/internal/request.h | 72 +++ > .../libcamera/internal/tracepoints/request.tp | 18 +- > include/libcamera/meson.build | 1 + > include/libcamera/request.h | 22 +- > meson.build | 1 + > src/android/camera_device.cpp | 43 +- > src/android/camera_device.h | 3 - > src/android/camera_request.cpp | 3 +- > src/android/camera_request.h | 3 +- > src/android/camera_worker.cpp | 129 ----- > src/android/camera_worker.h | 72 --- > src/android/meson.build | 1 - > src/libcamera/fence.cpp | 104 ++++ > src/libcamera/framebuffer.cpp | 26 + > src/libcamera/meson.build | 4 + > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 4 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +- > src/libcamera/pipeline/simple/simple.cpp | 4 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 4 +- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > src/libcamera/pipeline_handler.cpp | 97 +++- > src/libcamera/request.cpp | 462 ++++++++++++++---- > test/camera/capture.cpp | 2 +- > test/fence.cpp | 339 +++++++++++++ > test/meson.build | 1 + > 32 files changed, 1160 insertions(+), 363 deletions(-) > create mode 100644 include/libcamera/fence.h > create mode 100644 include/libcamera/internal/fence.h > create mode 100644 include/libcamera/internal/request.h > delete mode 100644 src/android/camera_worker.cpp > delete mode 100644 src/android/camera_worker.h > create mode 100644 src/libcamera/fence.cpp > create mode 100644 test/fence.cpp