| Message ID | 20211028111520.256612-10-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2021-10-28 12:15:19) > Before queueing a request to the device, any synchronization fence from > the Request framebuffers has to be waited on. > > Start by moving the fences to the Request, in order to reset the ones > in the FrameBuffers and then enable all of them one by one. Aha, ok so now I'm starting to understand the lifetime of the fence. If a Buffer is added with a fence to protect it until it's ready, do we introduce any kind of race when enabling if the buffer fence has completed /before/ we enable it? I.e. could we end up sitting on a fence waiting for a completion that has already occured, and then timeout - and cause the request to be incorrectly cancelled? > When a fence completes, make sure all fences in the Request have been > waited on or have expired and only after that actually queue the Request > to the device. > > If any fence in the Request has expired cancel the request and do not > queue it to the device. Sounds good... > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/pipeline_handler.h | 2 + > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++- > 2 files changed, 77 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 5c3c0bc5a8b3..5b98011ac4f6 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -29,6 +29,7 @@ class CameraConfiguration; > class CameraManager; > class DeviceEnumerator; > class DeviceMatch; > +class Fence; > class FrameBuffer; > class MediaDevice; > class PipelineHandler; > @@ -79,6 +80,7 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence); > void doQueueRequest(); > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 0f9fec1b618f..39cb680e5386 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -18,6 +18,7 @@ > #include <libcamera/framebuffer.h> > > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/request.h" > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > + Request::Private *data = request->_d(); > + > { > MutexLocker lock(waitingRequestsMutex_); > waitingRequests_.push_back(request); > } > > + for (FrameBuffer *buffer : request->pending_) { > + if (buffer->fence() == -1) > + continue; > + > + /* > + * Move the Fence into the Request::Private list of > + * fences. This resets the file descriptor fence in the > + * FrameBuffer to -1. > + */ > + data->addFence(std::move(buffer->_d()->fence())); > + > + Fence *fence = &data->fences().back(); > + fence->complete.connect(this, > + [this, request, buffer, fence]() { > + fenceCompleted(request, buffer, fence); > + }); > + } I can't help but wonder if this should be handled in the Request, and not by moving the fences to the camera data.... The request could signal that it is ready for consumption, without having to move the ownership of the fence from the buffer to the camera... Then the PipelineHandler wouldn't have to be digging around in the internals of the Request buffers. Of course it already has access to those, but I'm not 100% convinced, this is a good reason to dig in there, and not expose an "I am ready" signal from the request. But maybe that adds more layering on the requst that is undesirable? That's a fair bit of redesign though - so if you say "No, it's better this way" I'll just move on ;-) > + > + /* If no fences to wait on, we can queue the request immediately. */ > + if (!data->pendingFences()) { > + doQueueRequest(); > + > + return; > + } > + > + /* > + * Now that we have added all fences, enable them one by one. > + * Enabling fences while adding them to the Request would race on the > + * number of pending fences. > + */ > + for (Fence &fence : data->fences()) > + fence.enable(); > +} > + > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer, > + Fence *fence) > +{ > + Request::Private *data = request->_d(); > + > + if (fence->expired()) { > + /* > + * Move the fence back to the framebuffer, so that it doesn't > + * get closed and the file descriptor number is made > + * available again to applications. > + */ > + FrameBuffer::Private *bufferData = buffer->_d(); > + bufferData->fence() = std::move(*fence); > + > + data->fenceExpired(); > + } else { > + data->fenceCompleted(); > + } > + > + if (data->pendingFences()) > + return; > + > + data->clearFences(); > doQueueRequest(); > } > > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest() > return; > > request = waitingRequests_.front(); > + if (request->_d()->pendingFences()) > + return; > + > waitingRequests_.pop_front(); > } > > - /* Queue Request to the pipeline handler. */ > Camera *camera = request->_d()->camera(); > Camera::Private *camData = camera->_d(); > > request->sequence_ = camData->requestSequence_++; > - camData->queuedRequests_.push_back(request); > > + /* Cancel the request if one of the fences has failed. */ > + if (request->_d()->expiredFences()) { > + request->cancel(); > + completeRequest(request); > + > + doQueueRequest(); This makes me really think this would be better handled in a loop rather than recursion... :-S > + > + return; > + } > + > + /* Queue Request to the pipeline handler. */ > + camData->queuedRequests_.push_back(request); > int ret = queueRequestDevice(camera, request); > if (ret) { > request->cancel(); > completeRequest(request); > } > > - /* Try to queue the next Request in the queue, if ready. */ > doQueueRequest(); > } > > -- > 2.33.1 >
Hi Kieran On Tue, Nov 09, 2021 at 04:07:43PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-10-28 12:15:19) > > Before queueing a request to the device, any synchronization fence from > > the Request framebuffers has to be waited on. > > > > Start by moving the fences to the Request, in order to reset the ones > > in the FrameBuffers and then enable all of them one by one. > > > Aha, ok so now I'm starting to understand the lifetime of the fence. > > If a Buffer is added with a fence to protect it until it's ready, do we > introduce any kind of race when enabling if the buffer fence has > completed /before/ we enable it? > > I.e. could we end up sitting on a fence waiting for a completion that > has already occured, and then timeout - and cause the request to be > incorrectly cancelled? > depends :) For sofware fences I think if a fence gets signalled before we get to poll() on it, I think poll() would immediately return signalling the fence is read. I assume hw fences should work the same, but I can't tell for sure > > > When a fence completes, make sure all fences in the Request have been > > waited on or have expired and only after that actually queue the Request > > to the device. > > > > If any fence in the Request has expired cancel the request and do not > > queue it to the device. > > Sounds good... > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/internal/pipeline_handler.h | 2 + > > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++- > > 2 files changed, 77 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 5c3c0bc5a8b3..5b98011ac4f6 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -29,6 +29,7 @@ class CameraConfiguration; > > class CameraManager; > > class DeviceEnumerator; > > class DeviceMatch; > > +class Fence; > > class FrameBuffer; > > class MediaDevice; > > class PipelineHandler; > > @@ -79,6 +80,7 @@ private: > > void mediaDeviceDisconnected(MediaDevice *media); > > virtual void disconnect(); > > > > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence); > > void doQueueRequest(); > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index 0f9fec1b618f..39cb680e5386 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -18,6 +18,7 @@ > > #include <libcamera/framebuffer.h> > > > > #include "libcamera/internal/camera.h" > > +#include "libcamera/internal/framebuffer.h" > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/request.h" > > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request) > > { > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > + Request::Private *data = request->_d(); > > + > > { > > MutexLocker lock(waitingRequestsMutex_); > > waitingRequests_.push_back(request); > > } > > > > + for (FrameBuffer *buffer : request->pending_) { > > + if (buffer->fence() == -1) > > + continue; > > + > > + /* > > + * Move the Fence into the Request::Private list of > > + * fences. This resets the file descriptor fence in the > > + * FrameBuffer to -1. > > + */ > > + data->addFence(std::move(buffer->_d()->fence())); > > + > > + Fence *fence = &data->fences().back(); > > + fence->complete.connect(this, > > + [this, request, buffer, fence]() { > > + fenceCompleted(request, buffer, fence); > > + }); > > + } > > I can't help but wonder if this should be handled in the Request, and > not by moving the fences to the camera data.... They get moved to Request::Private Request::Private *data = request->_d(); for (buffer : request->pending) data->addFence(std::move(buffer->_d()->fence())); > > The request could signal that it is ready for consumption, without > having to move the ownership of the fence from the buffer to the > camera... However I got your point, the Request could be auto-counting, ie we connect the Fence::complete signal to a slot in Request::Private and basically move the fence counting implemented in PipelineHandler::fenceCompleted() there. I initially wanted this but I got discouraged by the need to have the Request then queue itself when it's ready. Now that I look at it again, we could add to Request::Private a 'done' (or 'ready' or whatever) signal to which we connect a slot in PipelineHandler. This would also simplify the Request::Private::fence*() interface that can be made private to Request::Private. > > Then the PipelineHandler wouldn't have to be digging around in the > internals of the Request buffers. I'll experiment with that, but please note that fences will anyway have to be moved to Request::Private here anyhow, as doing so at Request::addBuffer() time would invalidate the fence as seen from application much earlier than Camera::queueRequest() time, something I would rather not do. > > Of course it already has access to those, but I'm not 100% convinced, > this is a good reason to dig in there, and not expose an "I am ready" > signal from the request. But maybe that adds more layering on the requst > that is undesirable? Let's see how it looks like. I didn't want the request to call PipelineHandler::doQueueRequest() when it's ready, but maybe with a Signal it gets nicer. > > > That's a fair bit of redesign though - so if you say "No, it's better > this way" I'll just move on ;-) Ouch, I already acknoleded you comment. Can I take it back and say "No, it's better this way" ? :D > > > > > + > > + /* If no fences to wait on, we can queue the request immediately. */ > > + if (!data->pendingFences()) { > > + doQueueRequest(); > > + > > + return; > > + } > > + > > + /* > > + * Now that we have added all fences, enable them one by one. > > + * Enabling fences while adding them to the Request would race on the > > + * number of pending fences. > > + */ > > + for (Fence &fence : data->fences()) > > + fence.enable(); > > +} > > + > > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer, > > + Fence *fence) > > +{ > > + Request::Private *data = request->_d(); > > + > > + if (fence->expired()) { > > + /* > > + * Move the fence back to the framebuffer, so that it doesn't > > + * get closed and the file descriptor number is made > > + * available again to applications. > > + */ > > + FrameBuffer::Private *bufferData = buffer->_d(); > > + bufferData->fence() = std::move(*fence); > > + > > + data->fenceExpired(); > > + } else { > > + data->fenceCompleted(); > > + } > > + > > + if (data->pendingFences()) > > + return; > > + > > + data->clearFences(); > > doQueueRequest(); > > } > > > > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest() > > return; > > > > request = waitingRequests_.front(); > > + if (request->_d()->pendingFences()) > > + return; > > + > > waitingRequests_.pop_front(); > > } > > > > - /* Queue Request to the pipeline handler. */ > > Camera *camera = request->_d()->camera(); > > Camera::Private *camData = camera->_d(); > > > > request->sequence_ = camData->requestSequence_++; > > - camData->queuedRequests_.push_back(request); > > > > + /* Cancel the request if one of the fences has failed. */ > > + if (request->_d()->expiredFences()) { > > + request->cancel(); > > + completeRequest(request); > > + > > + doQueueRequest(); > > This makes me really think this would be better handled in a loop rather > than recursion... :-S > Still not convinced, mostly for a style point of view of having one more indentation layer. Thanks j > > + > > + return; > > + } > > + > > + /* Queue Request to the pipeline handler. */ > > + camData->queuedRequests_.push_back(request); > > int ret = queueRequestDevice(camera, request); > > if (ret) { > > request->cancel(); > > completeRequest(request); > > } > > > > - /* Try to queue the next Request in the queue, if ready. */ > > doQueueRequest(); > > } > > > > -- > > 2.33.1 > >
Hi Jacopo, Thank you for the patch On 10/28/21 4:45 PM, Jacopo Mondi wrote: > Before queueing a request to the device, any synchronization fence from > the Request framebuffers has to be waited on. > > Start by moving the fences to the Request, in order to reset the ones > in the FrameBuffers and then enable all of them one by one. > > When a fence completes, make sure all fences in the Request have been > waited on or have expired and only after that actually queue the Request > to the device. Here it's said: waited on OR expired => queue the Request to device > > If any fence in the Request has expired cancel the request and do not > queue it to the device. And here it's said: If fence expired => Cancel the request and do not queue. Am I missing something? The patch looks good to me overall. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/pipeline_handler.h | 2 + > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++- > 2 files changed, 77 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 5c3c0bc5a8b3..5b98011ac4f6 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -29,6 +29,7 @@ class CameraConfiguration; > class CameraManager; > class DeviceEnumerator; > class DeviceMatch; > +class Fence; > class FrameBuffer; > class MediaDevice; > class PipelineHandler; > @@ -79,6 +80,7 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence); > void doQueueRequest(); > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 0f9fec1b618f..39cb680e5386 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -18,6 +18,7 @@ > #include <libcamera/framebuffer.h> > > #include "libcamera/internal/camera.h" > +#include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/request.h" > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > + Request::Private *data = request->_d(); > + > { > MutexLocker lock(waitingRequestsMutex_); > waitingRequests_.push_back(request); > } > > + for (FrameBuffer *buffer : request->pending_) { > + if (buffer->fence() == -1) > + continue; > + > + /* > + * Move the Fence into the Request::Private list of > + * fences. This resets the file descriptor fence in the > + * FrameBuffer to -1. > + */ > + data->addFence(std::move(buffer->_d()->fence())); > + > + Fence *fence = &data->fences().back(); > + fence->complete.connect(this, > + [this, request, buffer, fence]() { > + fenceCompleted(request, buffer, fence); > + }); > + } > + > + /* If no fences to wait on, we can queue the request immediately. */ > + if (!data->pendingFences()) { > + doQueueRequest(); > + > + return; > + } > + > + /* > + * Now that we have added all fences, enable them one by one. > + * Enabling fences while adding them to the Request would race on the > + * number of pending fences. > + */ > + for (Fence &fence : data->fences()) > + fence.enable(); > +} > + > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer, > + Fence *fence) > +{ > + Request::Private *data = request->_d(); > + > + if (fence->expired()) { > + /* > + * Move the fence back to the framebuffer, so that it doesn't > + * get closed and the file descriptor number is made > + * available again to applications. > + */ > + FrameBuffer::Private *bufferData = buffer->_d(); > + bufferData->fence() = std::move(*fence); > + > + data->fenceExpired(); > + } else { > + data->fenceCompleted(); > + } > + > + if (data->pendingFences()) > + return; > + > + data->clearFences(); > doQueueRequest(); > } > > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest() > return; > > request = waitingRequests_.front(); > + if (request->_d()->pendingFences()) > + return; > + > waitingRequests_.pop_front(); > } > > - /* Queue Request to the pipeline handler. */ > Camera *camera = request->_d()->camera(); > Camera::Private *camData = camera->_d(); > > request->sequence_ = camData->requestSequence_++; > - camData->queuedRequests_.push_back(request); > > + /* Cancel the request if one of the fences has failed. */ > + if (request->_d()->expiredFences()) { > + request->cancel(); > + completeRequest(request); > + > + doQueueRequest(); A bit-off to read doQueueRequest() in this codepath here, but okay I guess > + > + return; > + } > + > + /* Queue Request to the pipeline handler. */ > + camData->queuedRequests_.push_back(request); > int ret = queueRequestDevice(camera, request); > if (ret) { > request->cancel(); > completeRequest(request); > } > > - /* Try to queue the next Request in the queue, if ready. */ > doQueueRequest(); > } >
Hi Jacopo, Thank you for the patch. On Tue, Nov 09, 2021 at 06:57:24PM +0100, Jacopo Mondi wrote: > On Tue, Nov 09, 2021 at 04:07:43PM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2021-10-28 12:15:19) > > > Before queueing a request to the device, any synchronization fence from > > > the Request framebuffers has to be waited on. > > > > > > Start by moving the fences to the Request, in order to reset the ones > > > in the FrameBuffers and then enable all of them one by one. > > > > Aha, ok so now I'm starting to understand the lifetime of the fence. > > > > If a Buffer is added with a fence to protect it until it's ready, do we > > introduce any kind of race when enabling if the buffer fence has > > completed /before/ we enable it? > > > > I.e. could we end up sitting on a fence waiting for a completion that > > has already occured, and then timeout - and cause the request to be > > incorrectly cancelled? > > depends :) For sofware fences I think if a fence gets signalled before > we get to poll() on it, I think poll() would immediately return > signalling the fence is read. > > I assume hw fences should work the same, but I can't tell for sure That's how the fence API is meant to behave, yes. > > > When a fence completes, make sure all fences in the Request have been > > > waited on or have expired and only after that actually queue the Request > > > to the device. > > > > > > If any fence in the Request has expired cancel the request and do not > > > queue it to the device. > > > > Sounds good... > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/internal/pipeline_handler.h | 2 + > > > src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++- > > > 2 files changed, 77 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > index 5c3c0bc5a8b3..5b98011ac4f6 100644 > > > --- a/include/libcamera/internal/pipeline_handler.h > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > @@ -29,6 +29,7 @@ class CameraConfiguration; > > > class CameraManager; > > > class DeviceEnumerator; > > > class DeviceMatch; > > > +class Fence; > > > class FrameBuffer; > > > class MediaDevice; > > > class PipelineHandler; > > > @@ -79,6 +80,7 @@ private: > > > void mediaDeviceDisconnected(MediaDevice *media); > > > virtual void disconnect(); > > > > > > + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence); > > > void doQueueRequest(); > > > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index 0f9fec1b618f..39cb680e5386 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -18,6 +18,7 @@ > > > #include <libcamera/framebuffer.h> > > > > > > #include "libcamera/internal/camera.h" > > > +#include "libcamera/internal/framebuffer.h" > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/request.h" > > > @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request) > > > { > > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > > > + Request::Private *data = request->_d(); > > > + > > > { > > > MutexLocker lock(waitingRequestsMutex_); > > > waitingRequests_.push_back(request); > > > } > > > > > > + for (FrameBuffer *buffer : request->pending_) { > > > + if (buffer->fence() == -1) > > > + continue; > > > + > > > + /* > > > + * Move the Fence into the Request::Private list of > > > + * fences. This resets the file descriptor fence in the > > > + * FrameBuffer to -1. > > > + */ > > > + data->addFence(std::move(buffer->_d()->fence())); > > > + > > > + Fence *fence = &data->fences().back(); > > > + fence->complete.connect(this, > > > + [this, request, buffer, fence]() { > > > + fenceCompleted(request, buffer, fence); > > > + }); > > > + } > > > > I can't help but wonder if this should be handled in the Request, and > > not by moving the fences to the camera data.... > > They get moved to Request::Private > > Request::Private *data = request->_d(); > > for (buffer : request->pending) > data->addFence(std::move(buffer->_d()->fence())); > > > The request could signal that it is ready for consumption, without > > having to move the ownership of the fence from the buffer to the > > camera... > > However I got your point, the Request could be auto-counting, ie we > connect the Fence::complete signal to a slot in Request::Private and > basically move the fence counting implemented in > PipelineHandler::fenceCompleted() there. I initially wanted this but I > got discouraged by the need to have the Request then queue itself when > it's ready. Now that I look at it again, we could add to > Request::Private a 'done' (or 'ready' or whatever) signal to which we > connect a slot in PipelineHandler. This would also simplify the > Request::Private::fence*() interface that can be made private to > Request::Private. > > > Then the PipelineHandler wouldn't have to be digging around in the > > internals of the Request buffers. > > I'll experiment with that, but please note that fences will anyway > have to be moved to Request::Private here anyhow, as doing so at > Request::addBuffer() time would invalidate the fence as seen from > application much earlier than Camera::queueRequest() time, something I > would rather not do. > > > Of course it already has access to those, but I'm not 100% convinced, > > this is a good reason to dig in there, and not expose an "I am ready" > > signal from the request. But maybe that adds more layering on the requst > > that is undesirable? > > Let's see how it looks like. I didn't want the request to call > PipelineHandler::doQueueRequest() when it's ready, but maybe with a > Signal it gets nicer. > > > That's a fair bit of redesign though - so if you say "No, it's better > > this way" I'll just move on ;-) > > Ouch, I already acknoleded you comment. Can I take it back and say > "No, it's better this way" ? :D > > > > + > > > + /* If no fences to wait on, we can queue the request immediately. */ > > > + if (!data->pendingFences()) { > > > + doQueueRequest(); > > > + > > > + return; > > > + } > > > + > > > + /* > > > + * Now that we have added all fences, enable them one by one. > > > + * Enabling fences while adding them to the Request would race on the > > > + * number of pending fences. > > > + */ > > > + for (Fence &fence : data->fences()) > > > + fence.enable(); > > > +} > > > + > > > +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer, > > > + Fence *fence) > > > +{ > > > + Request::Private *data = request->_d(); > > > + > > > + if (fence->expired()) { > > > + /* > > > + * Move the fence back to the framebuffer, so that it doesn't > > > + * get closed and the file descriptor number is made > > > + * available again to applications. > > > + */ > > > + FrameBuffer::Private *bufferData = buffer->_d(); > > > + bufferData->fence() = std::move(*fence); > > > + > > > + data->fenceExpired(); > > > + } else { > > > + data->fenceCompleted(); > > > + } > > > + > > > + if (data->pendingFences()) > > > + return; > > > + > > > + data->clearFences(); > > > doQueueRequest(); > > > } > > > > > > @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest() > > > return; > > > > > > request = waitingRequests_.front(); > > > + if (request->_d()->pendingFences()) > > > + return; > > > + > > > waitingRequests_.pop_front(); > > > } > > > > > > - /* Queue Request to the pipeline handler. */ > > > Camera *camera = request->_d()->camera(); > > > Camera::Private *camData = camera->_d(); > > > > > > request->sequence_ = camData->requestSequence_++; > > > - camData->queuedRequests_.push_back(request); > > > > > > + /* Cancel the request if one of the fences has failed. */ > > > + if (request->_d()->expiredFences()) { > > > + request->cancel(); > > > + completeRequest(request); > > > + > > > + doQueueRequest(); > > > > This makes me really think this would be better handled in a loop rather > > than recursion... :-S > > Still not convinced, mostly for a style point of view of having one > more indentation layer. You can split the function in two, doQueueRequest() and doQueueRequests(), with the latter implementing the loop and calling the former. > > > + > > > + return; > > > + } > > > + > > > + /* Queue Request to the pipeline handler. */ > > > + camData->queuedRequests_.push_back(request); > > > int ret = queueRequestDevice(camera, request); > > > if (ret) { > > > request->cancel(); > > > completeRequest(request); > > > } > > > > > > - /* Try to queue the next Request in the queue, if ready. */ > > > doQueueRequest(); > > > } > > >
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h index 5c3c0bc5a8b3..5b98011ac4f6 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -29,6 +29,7 @@ class CameraConfiguration; class CameraManager; class DeviceEnumerator; class DeviceMatch; +class Fence; class FrameBuffer; class MediaDevice; class PipelineHandler; @@ -79,6 +80,7 @@ private: void mediaDeviceDisconnected(MediaDevice *media); virtual void disconnect(); + void fenceCompleted(Request *request, FrameBuffer *buffer, Fence *fence); void doQueueRequest(); std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index 0f9fec1b618f..39cb680e5386 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -18,6 +18,7 @@ #include <libcamera/framebuffer.h> #include "libcamera/internal/camera.h" +#include "libcamera/internal/framebuffer.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/request.h" @@ -336,11 +337,70 @@ void PipelineHandler::queueRequest(Request *request) { LIBCAMERA_TRACEPOINT(request_queue, request); + Request::Private *data = request->_d(); + { MutexLocker lock(waitingRequestsMutex_); waitingRequests_.push_back(request); } + for (FrameBuffer *buffer : request->pending_) { + if (buffer->fence() == -1) + continue; + + /* + * Move the Fence into the Request::Private list of + * fences. This resets the file descriptor fence in the + * FrameBuffer to -1. + */ + data->addFence(std::move(buffer->_d()->fence())); + + Fence *fence = &data->fences().back(); + fence->complete.connect(this, + [this, request, buffer, fence]() { + fenceCompleted(request, buffer, fence); + }); + } + + /* If no fences to wait on, we can queue the request immediately. */ + if (!data->pendingFences()) { + doQueueRequest(); + + return; + } + + /* + * Now that we have added all fences, enable them one by one. + * Enabling fences while adding them to the Request would race on the + * number of pending fences. + */ + for (Fence &fence : data->fences()) + fence.enable(); +} + +void PipelineHandler::fenceCompleted(Request *request, FrameBuffer *buffer, + Fence *fence) +{ + Request::Private *data = request->_d(); + + if (fence->expired()) { + /* + * Move the fence back to the framebuffer, so that it doesn't + * get closed and the file descriptor number is made + * available again to applications. + */ + FrameBuffer::Private *bufferData = buffer->_d(); + bufferData->fence() = std::move(*fence); + + data->fenceExpired(); + } else { + data->fenceCompleted(); + } + + if (data->pendingFences()) + return; + + data->clearFences(); doQueueRequest(); } @@ -357,23 +417,35 @@ void PipelineHandler::doQueueRequest() return; request = waitingRequests_.front(); + if (request->_d()->pendingFences()) + return; + waitingRequests_.pop_front(); } - /* Queue Request to the pipeline handler. */ Camera *camera = request->_d()->camera(); Camera::Private *camData = camera->_d(); request->sequence_ = camData->requestSequence_++; - camData->queuedRequests_.push_back(request); + /* Cancel the request if one of the fences has failed. */ + if (request->_d()->expiredFences()) { + request->cancel(); + completeRequest(request); + + doQueueRequest(); + + return; + } + + /* Queue Request to the pipeline handler. */ + camData->queuedRequests_.push_back(request); int ret = queueRequestDevice(camera, request); if (ret) { request->cancel(); completeRequest(request); } - /* Try to queue the next Request in the queue, if ready. */ doQueueRequest(); }
Before queueing a request to the device, any synchronization fence from the Request framebuffers has to be waited on. Start by moving the fences to the Request, in order to reset the ones in the FrameBuffers and then enable all of them one by one. When a fence completes, make sure all fences in the Request have been waited on or have expired and only after that actually queue the Request to the device. If any fence in the Request has expired cancel the request and do not queue it to the device. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/internal/pipeline_handler.h | 2 + src/libcamera/pipeline_handler.cpp | 78 ++++++++++++++++++- 2 files changed, 77 insertions(+), 3 deletions(-)