| Message ID | 20211028111520.256612-8-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi (2021-10-28 12:15:17) > In order to prepare to handle synchronization fences at Request > queueing time, split the PipelineHandler::queueRequest() function in > two, by creating a list of waiting requests and introducing a new > doQueueDevice() function that queues Requests to the device in the > order the pipeline has received them. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/pipeline_handler.h | 7 ++++ > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 41cba44d990f..afb7990ea21b 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -7,7 +7,9 @@ > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > +#include <list> > #include <memory> > +#include <mutex> > #include <set> > #include <string> > #include <sys/types.h> > @@ -76,9 +78,14 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > + void doQueueRequest(); > + > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > std::vector<std::weak_ptr<Camera>> cameras_; > > + std::mutex waitingRequestsMutex_; > + std::list<Request *> waitingRequests_; > + > const char *name_; > > friend class PipelineHandlerFactory; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index cada864687ff..38edd00cebad 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -10,6 +10,7 @@ > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/thread.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); Should we add a new TRACEPOINT for when the request gets queued to the device now? > > + { > + MutexLocker lock(waitingRequestsMutex_); > + waitingRequests_.push_back(request); > + } > + > + doQueueRequest(); > +} > + > +/** > + * \brief Queue a request to the device > + */ > +void PipelineHandler::doQueueRequest() > +{ > + Request *request = nullptr; > + { > + MutexLocker lock(waitingRequestsMutex_); > + > + if (!waitingRequests_.size()) > + return; > + > + request = waitingRequests_.front(); > + waitingRequests_.pop_front(); > + } > + > + /* Queue Request to the pipeline handler. */ > Camera *camera = request->_d()->camera(); > - Camera::Private *data = camera->_d(); > - data->queuedRequests_.push_back(request); > + Camera::Private *camData = camera->_d(); Is this rename data -> camData required? it would simplify the diff if it was done separately. Otherwise - perhaps mention in the commit-message. > > - request->sequence_ = data->requestSequence_++; > + request->sequence_ = camData->requestSequence_++; > + camData->queuedRequests_.push_back(request); > > int ret = queueRequestDevice(camera, request); So we have a queue before we queue now... ;-) We probably need to update/create some new block diagrams for the Pipeline Handler documentation to show how the internal queuing works ... So I think this is now queueRequest(); // Add to the queue -> doQueueRequest(); // See if we're allowed to queue -> queueRequestDevice(); // Add to the kernel/hw queue ... > if (ret) { > request->cancel(); > completeRequest(request); > } > + > + /* Try to queue the next Request in the queue, if ready. */ > + doQueueRequest(); Hrm... I get a little scared/pessimistic when I see recursion. Is this better handled as a loop some how? > } > > /** > -- > 2.33.1 >
Hi Kieran On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-10-28 12:15:17) > > In order to prepare to handle synchronization fences at Request > > queueing time, split the PipelineHandler::queueRequest() function in > > two, by creating a list of waiting requests and introducing a new > > doQueueDevice() function that queues Requests to the device in the > > order the pipeline has received them. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/internal/pipeline_handler.h | 7 ++++ > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 41cba44d990f..afb7990ea21b 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -7,7 +7,9 @@ > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > +#include <list> > > #include <memory> > > +#include <mutex> > > #include <set> > > #include <string> > > #include <sys/types.h> > > @@ -76,9 +78,14 @@ private: > > void mediaDeviceDisconnected(MediaDevice *media); > > virtual void disconnect(); > > > > + void doQueueRequest(); > > + > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > std::vector<std::weak_ptr<Camera>> cameras_; > > > > + std::mutex waitingRequestsMutex_; > > + std::list<Request *> waitingRequests_; > > + > > const char *name_; > > > > friend class PipelineHandlerFactory; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index cada864687ff..38edd00cebad 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -10,6 +10,7 @@ > > #include <sys/sysmacros.h> > > > > #include <libcamera/base/log.h> > > +#include <libcamera/base/thread.h> > > #include <libcamera/base/utils.h> > > > > #include <libcamera/camera.h> > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > > { > > LIBCAMERA_TRACEPOINT(request_queue, request); > > Should we add a new TRACEPOINT for when the request gets queued to the > device now? > Dunno, this seems internal stuff, while I assumed tracepoints are meant to track public API interactions ? Am I mistaken ? > > > > + { > > + MutexLocker lock(waitingRequestsMutex_); > > + waitingRequests_.push_back(request); > > + } > > + > > + doQueueRequest(); > > +} > > + > > +/** > > + * \brief Queue a request to the device > > + */ > > +void PipelineHandler::doQueueRequest() > > +{ > > + Request *request = nullptr; > > + { > > + MutexLocker lock(waitingRequestsMutex_); > > + > > + if (!waitingRequests_.size()) > > + return; > > + > > + request = waitingRequests_.front(); > > + waitingRequests_.pop_front(); > > + } > > + > > + /* Queue Request to the pipeline handler. */ > > Camera *camera = request->_d()->camera(); > > - Camera::Private *data = camera->_d(); > > - data->queuedRequests_.push_back(request); > > + Camera::Private *camData = camera->_d(); > > Is this rename data -> camData required? it would simplify the diff if > it was done separately. Otherwise - perhaps mention in the > commit-message. It's probably a leftover, I'll keep using 'data'. > > > > > > - request->sequence_ = data->requestSequence_++; > > + request->sequence_ = camData->requestSequence_++; > > + camData->queuedRequests_.push_back(request); > > > > int ret = queueRequestDevice(camera, request); > > So we have a queue before we queue now... ;-) > > We probably need to update/create some new block diagrams for the > Pipeline Handler documentation to show how the internal queuing works ... > > So I think this is now > queueRequest(); // Add to the queue > -> doQueueRequest(); // See if we're allowed to queue > -> queueRequestDevice(); // Add to the kernel/hw queue ... > it's more like queueRequest(); if (ready) doQueueRequest() queueRequestDevice() else waitOnFences() if (fencesDone) doQueueRequest() queueRequestDevice() > > if (ret) { > > request->cancel(); > > completeRequest(request); > > } > > + > > + /* Try to queue the next Request in the queue, if ready. */ > > + doQueueRequest(); > > Hrm... I get a little scared/pessimistic when I see recursion. Is this > better handled as a loop some how? > Why so ? It's one more level of indendation and I'm not sure what benefits it would bring :) > > > } > > > > /** > > -- > > 2.33.1 > >
Quoting Jacopo Mondi (2021-11-09 17:42:54) > Hi Kieran > > On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2021-10-28 12:15:17) > > > In order to prepare to handle synchronization fences at Request > > > queueing time, split the PipelineHandler::queueRequest() function in > > > two, by creating a list of waiting requests and introducing a new > > > doQueueDevice() function that queues Requests to the device in the > > > order the pipeline has received them. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/internal/pipeline_handler.h | 7 ++++ > > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > index 41cba44d990f..afb7990ea21b 100644 > > > --- a/include/libcamera/internal/pipeline_handler.h > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > @@ -7,7 +7,9 @@ > > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > > > +#include <list> > > > #include <memory> > > > +#include <mutex> > > > #include <set> > > > #include <string> > > > #include <sys/types.h> > > > @@ -76,9 +78,14 @@ private: > > > void mediaDeviceDisconnected(MediaDevice *media); > > > virtual void disconnect(); > > > > > > + void doQueueRequest(); > > > + > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > > std::vector<std::weak_ptr<Camera>> cameras_; > > > > > > + std::mutex waitingRequestsMutex_; > > > + std::list<Request *> waitingRequests_; > > > + > > > const char *name_; > > > > > > friend class PipelineHandlerFactory; > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index cada864687ff..38edd00cebad 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -10,6 +10,7 @@ > > > #include <sys/sysmacros.h> > > > > > > #include <libcamera/base/log.h> > > > +#include <libcamera/base/thread.h> > > > #include <libcamera/base/utils.h> > > > > > > #include <libcamera/camera.h> > > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > > > { > > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > Should we add a new TRACEPOINT for when the request gets queued to the > > device now? > > > > Dunno, this seems internal stuff, while I assumed tracepoints are > meant to track public API interactions ? Am I mistaken ? I would expect them to track any internal actions and events that a tracepoint helps to identify ... Perhaps one for Paul to chime in on... The Tracepoint now tracks when the request is queued, but not when it gets sent to the hardware so I would expect that to be useful information for tracking the flow with the tracing mechanisms. > > > > > > > + { > > > + MutexLocker lock(waitingRequestsMutex_); > > > + waitingRequests_.push_back(request); > > > + } > > > + > > > + doQueueRequest(); > > > +} > > > + > > > +/** > > > + * \brief Queue a request to the device > > > + */ > > > +void PipelineHandler::doQueueRequest() > > > +{ > > > + Request *request = nullptr; > > > + { > > > + MutexLocker lock(waitingRequestsMutex_); > > > + > > > + if (!waitingRequests_.size()) > > > + return; > > > + > > > + request = waitingRequests_.front(); > > > + waitingRequests_.pop_front(); > > > + } > > > + > > > + /* Queue Request to the pipeline handler. */ > > > Camera *camera = request->_d()->camera(); > > > - Camera::Private *data = camera->_d(); > > > - data->queuedRequests_.push_back(request); > > > + Camera::Private *camData = camera->_d(); > > > > Is this rename data -> camData required? it would simplify the diff if > > it was done separately. Otherwise - perhaps mention in the > > commit-message. > > It's probably a leftover, I'll keep using 'data'. > > > > > > > > > > > - request->sequence_ = data->requestSequence_++; > > > + request->sequence_ = camData->requestSequence_++; > > > + camData->queuedRequests_.push_back(request); > > > > > > int ret = queueRequestDevice(camera, request); > > > > So we have a queue before we queue now... ;-) > > > > We probably need to update/create some new block diagrams for the > > Pipeline Handler documentation to show how the internal queuing works ... > > > > So I think this is now > > queueRequest(); // Add to the queue > > -> doQueueRequest(); // See if we're allowed to queue > > -> queueRequestDevice(); // Add to the kernel/hw queue ... > > > > it's more like > > queueRequest(); > if (ready) > doQueueRequest() > queueRequestDevice() > else > waitOnFences() > if (fencesDone) > doQueueRequest() > queueRequestDevice() > > > > > if (ret) { > > > request->cancel(); > > > completeRequest(request); > > > } > > > + > > > + /* Try to queue the next Request in the queue, if ready. */ > > > + doQueueRequest(); > > > > Hrm... I get a little scared/pessimistic when I see recursion. Is this > > better handled as a loop some how? > > > > Why so ? It's one more level of indendation and I'm not sure what > benefits it would bring :) Ok, lets keep it your way then. I'd refactor it - but it's your code ;-) -- Kieran > > > > > } > > > > > > /** > > > -- > > > 2.33.1 > > >
Hello, On Wed, Nov 10, 2021 at 09:48:10AM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2021-11-09 17:42:54) > > Hi Kieran > > > > On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote: > > > Quoting Jacopo Mondi (2021-10-28 12:15:17) > > > > In order to prepare to handle synchronization fences at Request > > > > queueing time, split the PipelineHandler::queueRequest() function in > > > > two, by creating a list of waiting requests and introducing a new > > > > doQueueDevice() function that queues Requests to the device in the > > > > order the pipeline has received them. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > include/libcamera/internal/pipeline_handler.h | 7 ++++ > > > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > > > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > index 41cba44d990f..afb7990ea21b 100644 > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > @@ -7,7 +7,9 @@ > > > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > > > > > +#include <list> > > > > #include <memory> > > > > +#include <mutex> > > > > #include <set> > > > > #include <string> > > > > #include <sys/types.h> > > > > @@ -76,9 +78,14 @@ private: > > > > void mediaDeviceDisconnected(MediaDevice *media); > > > > virtual void disconnect(); > > > > > > > > + void doQueueRequest(); > > > > + > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > > > std::vector<std::weak_ptr<Camera>> cameras_; > > > > > > > > + std::mutex waitingRequestsMutex_; > > > > + std::list<Request *> waitingRequests_; > > > > + > > > > const char *name_; > > > > > > > > friend class PipelineHandlerFactory; > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > index cada864687ff..38edd00cebad 100644 > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > @@ -10,6 +10,7 @@ > > > > #include <sys/sysmacros.h> > > > > > > > > #include <libcamera/base/log.h> > > > > +#include <libcamera/base/thread.h> > > > > #include <libcamera/base/utils.h> > > > > > > > > #include <libcamera/camera.h> > > > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > > > > { > > > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > > > Should we add a new TRACEPOINT for when the request gets queued to the > > > device now? > > > > > > > Dunno, this seems internal stuff, while I assumed tracepoints are > > meant to track public API interactions ? Am I mistaken ? > > I would expect them to track any internal actions and events that a > tracepoint helps to identify ... > > Perhaps one for Paul to chime in on... Chiming in here. Tracepoints can go anywhere. I'd put them in places where we'd want information on the flow, but that are hit so frequently that putting them in debug messages isn't practical. (Speaking of which, I think a bunch of per-request IPA messages should be moved to tracepoints) > > The Tracepoint now tracks when the request is queued, but not when it > gets sent to the hardware so I would expect that to be useful > information for tracking the flow with the tracing mechanisms. Yeah it definitely would. Paul > > > > > > > > > > > + { > > > > + MutexLocker lock(waitingRequestsMutex_); > > > > + waitingRequests_.push_back(request); > > > > + } > > > > + > > > > + doQueueRequest(); > > > > +} > > > > + > > > > +/** > > > > + * \brief Queue a request to the device > > > > + */ > > > > +void PipelineHandler::doQueueRequest() > > > > +{ > > > > + Request *request = nullptr; > > > > + { > > > > + MutexLocker lock(waitingRequestsMutex_); > > > > + > > > > + if (!waitingRequests_.size()) > > > > + return; > > > > + > > > > + request = waitingRequests_.front(); > > > > + waitingRequests_.pop_front(); > > > > + } > > > > + > > > > + /* Queue Request to the pipeline handler. */ > > > > Camera *camera = request->_d()->camera(); > > > > - Camera::Private *data = camera->_d(); > > > > - data->queuedRequests_.push_back(request); > > > > + Camera::Private *camData = camera->_d(); > > > > > > Is this rename data -> camData required? it would simplify the diff if > > > it was done separately. Otherwise - perhaps mention in the > > > commit-message. > > > > It's probably a leftover, I'll keep using 'data'. > > > > > > > > > > > > > > > > - request->sequence_ = data->requestSequence_++; > > > > + request->sequence_ = camData->requestSequence_++; > > > > + camData->queuedRequests_.push_back(request); > > > > > > > > int ret = queueRequestDevice(camera, request); > > > > > > So we have a queue before we queue now... ;-) > > > > > > We probably need to update/create some new block diagrams for the > > > Pipeline Handler documentation to show how the internal queuing works ... > > > > > > So I think this is now > > > queueRequest(); // Add to the queue > > > -> doQueueRequest(); // See if we're allowed to queue > > > -> queueRequestDevice(); // Add to the kernel/hw queue ... > > > > > > > it's more like > > > > queueRequest(); > > if (ready) > > doQueueRequest() > > queueRequestDevice() > > else > > waitOnFences() > > if (fencesDone) > > doQueueRequest() > > queueRequestDevice() > > > > > > > > if (ret) { > > > > request->cancel(); > > > > completeRequest(request); > > > > } > > > > + > > > > + /* Try to queue the next Request in the queue, if ready. */ > > > > + doQueueRequest(); > > > > > > Hrm... I get a little scared/pessimistic when I see recursion. Is this > > > better handled as a loop some how? > > > > > > > Why so ? It's one more level of indendation and I'm not sure what > > benefits it would bring :) > > Ok, lets keep it your way then. I'd refactor it - but it's your code ;-) > -- > Kieran > > > > > > > > > } > > > > > > > > /** > > > > -- > > > > 2.33.1 > > > >
Hi Jacopo, Thank you for the patch. One question below On 10/28/21 4:45 PM, Jacopo Mondi wrote: > In order to prepare to handle synchronization fences at Request > queueing time, split the PipelineHandler::queueRequest() function in > two, by creating a list of waiting requests and introducing a new > doQueueDevice() function that queues Requests to the device in the > order the pipeline has received them. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/pipeline_handler.h | 7 ++++ > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 41cba44d990f..afb7990ea21b 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -7,7 +7,9 @@ > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > +#include <list> > #include <memory> > +#include <mutex> > #include <set> > #include <string> > #include <sys/types.h> > @@ -76,9 +78,14 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > + void doQueueRequest(); > + > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > std::vector<std::weak_ptr<Camera>> cameras_; > > + std::mutex waitingRequestsMutex_; > + std::list<Request *> waitingRequests_; > + > const char *name_; > > friend class PipelineHandlerFactory; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index cada864687ff..38edd00cebad 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -10,6 +10,7 @@ > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/thread.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > + { > + MutexLocker lock(waitingRequestsMutex_); > + waitingRequests_.push_back(request); Why do we need to lock this? Are there multiple threads accessing PipelineHandler::queueRequest() The documentation of the function states \context This function is called from the CameraManager thread. which I believe is all synchronous at this point. > + } > + > + doQueueRequest(); > +} > + > +/** > + * \brief Queue a request to the device > + */ > +void PipelineHandler::doQueueRequest() > +{ > + Request *request = nullptr; > + { > + MutexLocker lock(waitingRequestsMutex_); > + > + if (!waitingRequests_.size()) > + return; > + > + request = waitingRequests_.front(); > + waitingRequests_.pop_front(); > + } > + > + /* Queue Request to the pipeline handler. */ > Camera *camera = request->_d()->camera(); > - Camera::Private *data = camera->_d(); > - data->queuedRequests_.push_back(request); > + Camera::Private *camData = camera->_d(); > > - request->sequence_ = data->requestSequence_++; > + request->sequence_ = camData->requestSequence_++; > + 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 Umang On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote: > Hi Jacopo, > > Thank you for the patch. One question below > > On 10/28/21 4:45 PM, Jacopo Mondi wrote: > > In order to prepare to handle synchronization fences at Request > > queueing time, split the PipelineHandler::queueRequest() function in > > two, by creating a list of waiting requests and introducing a new > > doQueueDevice() function that queues Requests to the device in the > > order the pipeline has received them. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/internal/pipeline_handler.h | 7 ++++ > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > index 41cba44d990f..afb7990ea21b 100644 > > --- a/include/libcamera/internal/pipeline_handler.h > > +++ b/include/libcamera/internal/pipeline_handler.h > > @@ -7,7 +7,9 @@ > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > +#include <list> > > #include <memory> > > +#include <mutex> > > #include <set> > > #include <string> > > #include <sys/types.h> > > @@ -76,9 +78,14 @@ private: > > void mediaDeviceDisconnected(MediaDevice *media); > > virtual void disconnect(); > > + void doQueueRequest(); > > + > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > std::vector<std::weak_ptr<Camera>> cameras_; > > + std::mutex waitingRequestsMutex_; > > + std::list<Request *> waitingRequests_; > > + > > const char *name_; > > friend class PipelineHandlerFactory; > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > index cada864687ff..38edd00cebad 100644 > > --- a/src/libcamera/pipeline_handler.cpp > > +++ b/src/libcamera/pipeline_handler.cpp > > @@ -10,6 +10,7 @@ > > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > > +#include <libcamera/base/thread.h> > > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > > { > > LIBCAMERA_TRACEPOINT(request_queue, request); > > + { > > + MutexLocker lock(waitingRequestsMutex_); > > + waitingRequests_.push_back(request); > > > Why do we need to lock this? Are there multiple threads accessing > PipelineHandler::queueRequest() > > The documentation of the function states > > \context This function is called from the CameraManager thread. > > which I believe is all synchronous at this point. The waitingRequests_ queue is accessed by the doQueueRequest() function which might get called in response to fence completion event. But now that I've said so, the EventDispatcher that sends fence completions is the one running in the current thread, which is the same as the CameraManager one, so it might be safe to remove the locking maybe ? Thanks j > > > + } > > + > > + doQueueRequest(); > > +} > > + > > +/** > > + * \brief Queue a request to the device > > + */ > > +void PipelineHandler::doQueueRequest() > > +{ > > + Request *request = nullptr; > > + { > > + MutexLocker lock(waitingRequestsMutex_); > > + > > + if (!waitingRequests_.size()) > > + return; > > + > > + request = waitingRequests_.front(); > > + waitingRequests_.pop_front(); > > + } > > + > > + /* Queue Request to the pipeline handler. */ > > Camera *camera = request->_d()->camera(); > > - Camera::Private *data = camera->_d(); > > - data->queuedRequests_.push_back(request); > > + Camera::Private *camData = camera->_d(); > > - request->sequence_ = data->requestSequence_++; > > + request->sequence_ = camData->requestSequence_++; > > + 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 On 11/10/21 6:38 PM, Jacopo Mondi wrote: > Hi Umang > > On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote: >> Hi Jacopo, >> >> Thank you for the patch. One question below >> >> On 10/28/21 4:45 PM, Jacopo Mondi wrote: >>> In order to prepare to handle synchronization fences at Request >>> queueing time, split the PipelineHandler::queueRequest() function in >>> two, by creating a list of waiting requests and introducing a new >>> doQueueDevice() function that queues Requests to the device in the >>> order the pipeline has received them. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> include/libcamera/internal/pipeline_handler.h | 7 ++++ >>> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- >>> 2 files changed, 39 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h >>> index 41cba44d990f..afb7990ea21b 100644 >>> --- a/include/libcamera/internal/pipeline_handler.h >>> +++ b/include/libcamera/internal/pipeline_handler.h >>> @@ -7,7 +7,9 @@ >>> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ >>> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ >>> +#include <list> >>> #include <memory> >>> +#include <mutex> >>> #include <set> >>> #include <string> >>> #include <sys/types.h> >>> @@ -76,9 +78,14 @@ private: >>> void mediaDeviceDisconnected(MediaDevice *media); >>> virtual void disconnect(); >>> + void doQueueRequest(); >>> + >>> std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; >>> std::vector<std::weak_ptr<Camera>> cameras_; >>> + std::mutex waitingRequestsMutex_; >>> + std::list<Request *> waitingRequests_; >>> + >>> const char *name_; >>> friend class PipelineHandlerFactory; >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>> index cada864687ff..38edd00cebad 100644 >>> --- a/src/libcamera/pipeline_handler.cpp >>> +++ b/src/libcamera/pipeline_handler.cpp >>> @@ -10,6 +10,7 @@ >>> #include <sys/sysmacros.h> >>> #include <libcamera/base/log.h> >>> +#include <libcamera/base/thread.h> >>> #include <libcamera/base/utils.h> >>> #include <libcamera/camera.h> >>> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) >>> { >>> LIBCAMERA_TRACEPOINT(request_queue, request); >>> + { >>> + MutexLocker lock(waitingRequestsMutex_); >>> + waitingRequests_.push_back(request); >> >> Why do we need to lock this? Are there multiple threads accessing >> PipelineHandler::queueRequest() >> >> The documentation of the function states >> >> \context This function is called from the CameraManager thread. >> >> which I believe is all synchronous at this point. > The waitingRequests_ queue is accessed by the doQueueRequest() > function which might get called in response to fence completion event. > > But now that I've said so, the EventDispatcher that sends fence > completions is the one running in the current thread, which is the > same as the CameraManager one, so it might be safe to remove the > locking maybe ? That's what I am contemplating too, the slot when triggered will run in the same thread (which is the camera-manager one). I see no harm in removing the lock but I maybe wrong too. Uptill now, in my limited reading around queueRequest and the PH class, I didn't see any separate thread angle coming up so... I'll keep an lookout. > > Thanks > j > >>> + } >>> + >>> + doQueueRequest(); >>> +} >>> + >>> +/** >>> + * \brief Queue a request to the device >>> + */ >>> +void PipelineHandler::doQueueRequest() >>> +{ >>> + Request *request = nullptr; >>> + { >>> + MutexLocker lock(waitingRequestsMutex_); >>> + >>> + if (!waitingRequests_.size()) >>> + return; >>> + >>> + request = waitingRequests_.front(); >>> + waitingRequests_.pop_front(); >>> + } >>> + >>> + /* Queue Request to the pipeline handler. */ >>> Camera *camera = request->_d()->camera(); >>> - Camera::Private *data = camera->_d(); >>> - data->queuedRequests_.push_back(request); >>> + Camera::Private *camData = camera->_d(); >>> - request->sequence_ = data->requestSequence_++; >>> + request->sequence_ = camData->requestSequence_++; >>> + 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(); >>> } >>> /**
Hello, On Wed, Nov 10, 2021 at 06:47:01PM +0530, Umang Jain wrote: > On 11/10/21 6:38 PM, Jacopo Mondi wrote: > > On Wed, Nov 10, 2021 at 06:20:01PM +0530, Umang Jain wrote: > >> On 10/28/21 4:45 PM, Jacopo Mondi wrote: > >>> In order to prepare to handle synchronization fences at Request > >>> queueing time, split the PipelineHandler::queueRequest() function in > >>> two, by creating a list of waiting requests and introducing a new > >>> doQueueDevice() function that queues Requests to the device in the > >>> order the pipeline has received them. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> include/libcamera/internal/pipeline_handler.h | 7 ++++ > >>> src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > >>> 2 files changed, 39 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > >>> index 41cba44d990f..afb7990ea21b 100644 > >>> --- a/include/libcamera/internal/pipeline_handler.h > >>> +++ b/include/libcamera/internal/pipeline_handler.h > >>> @@ -7,7 +7,9 @@ > >>> #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > >>> #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > >>> +#include <list> > >>> #include <memory> > >>> +#include <mutex> > >>> #include <set> > >>> #include <string> > >>> #include <sys/types.h> > >>> @@ -76,9 +78,14 @@ private: > >>> void mediaDeviceDisconnected(MediaDevice *media); > >>> virtual void disconnect(); > >>> + void doQueueRequest(); > >>> + > >>> std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > >>> std::vector<std::weak_ptr<Camera>> cameras_; > >>> + std::mutex waitingRequestsMutex_; > >>> + std::list<Request *> waitingRequests_; > >>> + > >>> const char *name_; > >>> friend class PipelineHandlerFactory; > >>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > >>> index cada864687ff..38edd00cebad 100644 > >>> --- a/src/libcamera/pipeline_handler.cpp > >>> +++ b/src/libcamera/pipeline_handler.cpp > >>> @@ -10,6 +10,7 @@ > >>> #include <sys/sysmacros.h> > >>> #include <libcamera/base/log.h> > >>> +#include <libcamera/base/thread.h> > >>> #include <libcamera/base/utils.h> > >>> #include <libcamera/camera.h> > >>> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > >>> { > >>> LIBCAMERA_TRACEPOINT(request_queue, request); > >>> + { > >>> + MutexLocker lock(waitingRequestsMutex_); > >>> + waitingRequests_.push_back(request); > >> > >> Why do we need to lock this? Are there multiple threads accessing > >> PipelineHandler::queueRequest() > >> > >> The documentation of the function states > >> > >> \context This function is called from the CameraManager thread. > >> > >> which I believe is all synchronous at this point. > > > > The waitingRequests_ queue is accessed by the doQueueRequest() > > function which might get called in response to fence completion event. > > > > But now that I've said so, the EventDispatcher that sends fence > > completions is the one running in the current thread, which is the > > same as the CameraManager one, so it might be safe to remove the > > locking maybe ? > > That's what I am contemplating too, the slot when triggered will run in > the same thread (which is the camera-manager one). I see no harm in > removing the lock but I maybe wrong too. Uptill now, in my limited > reading around queueRequest and the PH class, I didn't see any separate > thread angle coming up so... I'll keep an lookout. As mentioned in a separate e-mail in this thread, pipeline handlers are single-threaded, so there's no need for a lock. > >>> + } > >>> + > >>> + doQueueRequest(); > >>> +} > >>> + > >>> +/** > >>> + * \brief Queue a request to the device > >>> + */ > >>> +void PipelineHandler::doQueueRequest() > >>> +{ > >>> + Request *request = nullptr; > >>> + { > >>> + MutexLocker lock(waitingRequestsMutex_); > >>> + > >>> + if (!waitingRequests_.size()) > >>> + return; > >>> + > >>> + request = waitingRequests_.front(); > >>> + waitingRequests_.pop_front(); > >>> + } > >>> + > >>> + /* Queue Request to the pipeline handler. */ > >>> Camera *camera = request->_d()->camera(); > >>> - Camera::Private *data = camera->_d(); > >>> - data->queuedRequests_.push_back(request); > >>> + Camera::Private *camData = camera->_d(); > >>> - request->sequence_ = data->requestSequence_++; > >>> + request->sequence_ = camData->requestSequence_++; > >>> + 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(); > >>> } > >>> /**
Hello, On Wed, Nov 10, 2021 at 07:02:33PM +0900, paul.elder@ideasonboard.com wrote: > On Wed, Nov 10, 2021 at 09:48:10AM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2021-11-09 17:42:54) > > > On Tue, Nov 09, 2021 at 02:39:31PM +0000, Kieran Bingham wrote: > > > > Quoting Jacopo Mondi (2021-10-28 12:15:17) > > > > > In order to prepare to handle synchronization fences at Request > > > > > queueing time, split the PipelineHandler::queueRequest() function in > > > > > two, by creating a list of waiting requests and introducing a new > > > > > doQueueDevice() function that queues Requests to the device in the > > > > > order the pipeline has received them. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > include/libcamera/internal/pipeline_handler.h | 7 ++++ > > > > > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > > > > > 2 files changed, 39 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > > > > > index 41cba44d990f..afb7990ea21b 100644 > > > > > --- a/include/libcamera/internal/pipeline_handler.h > > > > > +++ b/include/libcamera/internal/pipeline_handler.h > > > > > @@ -7,7 +7,9 @@ > > > > > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > > > > > > > > > +#include <list> > > > > > #include <memory> > > > > > +#include <mutex> > > > > > #include <set> > > > > > #include <string> > > > > > #include <sys/types.h> > > > > > @@ -76,9 +78,14 @@ private: > > > > > void mediaDeviceDisconnected(MediaDevice *media); > > > > > virtual void disconnect(); > > > > > > > > > > + void doQueueRequest(); > > > > > + > > > > > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > > > > > std::vector<std::weak_ptr<Camera>> cameras_; > > > > > > > > > > + std::mutex waitingRequestsMutex_; > > > > > + std::list<Request *> waitingRequests_; > > > > > + > > > > > const char *name_; > > > > > > > > > > friend class PipelineHandlerFactory; > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > index cada864687ff..38edd00cebad 100644 > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > @@ -10,6 +10,7 @@ > > > > > #include <sys/sysmacros.h> > > > > > > > > > > #include <libcamera/base/log.h> > > > > > +#include <libcamera/base/thread.h> > > > > > #include <libcamera/base/utils.h> > > > > > > > > > > #include <libcamera/camera.h> > > > > > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > > > > > { > > > > > LIBCAMERA_TRACEPOINT(request_queue, request); > > > > > > > > Should we add a new TRACEPOINT for when the request gets queued to the > > > > device now? > > > > > > Dunno, this seems internal stuff, while I assumed tracepoints are > > > meant to track public API interactions ? Am I mistaken ? > > > > I would expect them to track any internal actions and events that a > > tracepoint helps to identify ... > > > > Perhaps one for Paul to chime in on... > > Chiming in here. > > Tracepoints can go anywhere. I'd put them in places where we'd want > information on the flow, but that are hit so frequently that putting > them in debug messages isn't practical. > > (Speaking of which, I think a bunch of per-request IPA messages should > be moved to tracepoints) > > > The Tracepoint now tracks when the request is queued, but not when it > > gets sent to the hardware so I would expect that to be useful > > information for tracking the flow with the tracing mechanisms. > > Yeah it definitely would. Agreed, knowing when libcamera receives the request and when it passes it to the pipeline handler are both useful pieces of information, now that they're separate events. > > > > > + { > > > > > + MutexLocker lock(waitingRequestsMutex_); > > > > > + waitingRequests_.push_back(request); > > > > > + } > > > > > + > > > > > + doQueueRequest(); > > > > > +} > > > > > + > > > > > +/** > > > > > + * \brief Queue a request to the device > > > > > + */ > > > > > +void PipelineHandler::doQueueRequest() > > > > > +{ > > > > > + Request *request = nullptr; > > > > > + { > > > > > + MutexLocker lock(waitingRequestsMutex_); > > > > > + > > > > > + if (!waitingRequests_.size()) empty() > > > > > + return; > > > > > + > > > > > + request = waitingRequests_.front(); > > > > > + waitingRequests_.pop_front(); > > > > > + } > > > > > + > > > > > + /* Queue Request to the pipeline handler. */ > > > > > Camera *camera = request->_d()->camera(); > > > > > - Camera::Private *data = camera->_d(); > > > > > - data->queuedRequests_.push_back(request); > > > > > + Camera::Private *camData = camera->_d(); > > > > > > > > Is this rename data -> camData required? it would simplify the diff if > > > > it was done separately. Otherwise - perhaps mention in the > > > > commit-message. > > > > > > It's probably a leftover, I'll keep using 'data'. > > > > > > > > > > > > > - request->sequence_ = data->requestSequence_++; > > > > > + request->sequence_ = camData->requestSequence_++; > > > > > + camData->queuedRequests_.push_back(request); > > > > > > > > > > int ret = queueRequestDevice(camera, request); > > > > > > > > So we have a queue before we queue now... ;-) > > > > > > > > We probably need to update/create some new block diagrams for the > > > > Pipeline Handler documentation to show how the internal queuing works ... > > > > > > > > So I think this is now > > > > queueRequest(); // Add to the queue > > > > -> doQueueRequest(); // See if we're allowed to queue > > > > -> queueRequestDevice(); // Add to the kernel/hw queue ... > > > > > > > > > > it's more like > > > > > > queueRequest(); > > > if (ready) > > > doQueueRequest() > > > queueRequestDevice() > > > else > > > waitOnFences() > > > if (fencesDone) > > > doQueueRequest() > > > queueRequestDevice() > > > > > > > > > > > if (ret) { > > > > > request->cancel(); > > > > > completeRequest(request); > > > > > } > > > > > + > > > > > + /* Try to queue the next Request in the queue, if ready. */ > > > > > + doQueueRequest(); > > > > > > > > Hrm... I get a little scared/pessimistic when I see recursion. Is this > > > > better handled as a loop some how? I'd like a loop better too, it's easier to understand the stop condition. > > > Why so ? It's one more level of indendation and I'm not sure what > > > benefits it would bring :) > > > > Ok, lets keep it your way then. I'd refactor it - but it's your code ;-) > > > > > > > } > > > > > > > > > > /**
Hi Jacopo, Thank you for the patch. One additional comment below. On Thu, Oct 28, 2021 at 01:15:17PM +0200, Jacopo Mondi wrote: > In order to prepare to handle synchronization fences at Request > queueing time, split the PipelineHandler::queueRequest() function in > two, by creating a list of waiting requests and introducing a new > doQueueDevice() function that queues Requests to the device in the > order the pipeline has received them. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/pipeline_handler.h | 7 ++++ > src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- > 2 files changed, 39 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 41cba44d990f..afb7990ea21b 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -7,7 +7,9 @@ > #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ > > +#include <list> > #include <memory> > +#include <mutex> > #include <set> > #include <string> > #include <sys/types.h> > @@ -76,9 +78,14 @@ private: > void mediaDeviceDisconnected(MediaDevice *media); > virtual void disconnect(); > > + void doQueueRequest(); > + > std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; > std::vector<std::weak_ptr<Camera>> cameras_; > > + std::mutex waitingRequestsMutex_; > + std::list<Request *> waitingRequests_; Should this be a std::queue ? > + > const char *name_; > > friend class PipelineHandlerFactory; > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index cada864687ff..38edd00cebad 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -10,6 +10,7 @@ > #include <sys/sysmacros.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/thread.h> > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) > { > LIBCAMERA_TRACEPOINT(request_queue, request); > > + { > + MutexLocker lock(waitingRequestsMutex_); > + waitingRequests_.push_back(request); > + } > + > + doQueueRequest(); > +} > + > +/** > + * \brief Queue a request to the device > + */ > +void PipelineHandler::doQueueRequest() > +{ > + Request *request = nullptr; > + { > + MutexLocker lock(waitingRequestsMutex_); > + > + if (!waitingRequests_.size()) > + return; > + > + request = waitingRequests_.front(); > + waitingRequests_.pop_front(); > + } > + > + /* Queue Request to the pipeline handler. */ > Camera *camera = request->_d()->camera(); > - Camera::Private *data = camera->_d(); > - data->queuedRequests_.push_back(request); > + Camera::Private *camData = camera->_d(); > > - request->sequence_ = data->requestSequence_++; > + request->sequence_ = camData->requestSequence_++; > + 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 41cba44d990f..afb7990ea21b 100644 --- a/include/libcamera/internal/pipeline_handler.h +++ b/include/libcamera/internal/pipeline_handler.h @@ -7,7 +7,9 @@ #ifndef __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ #define __LIBCAMERA_INTERNAL_PIPELINE_HANDLER_H__ +#include <list> #include <memory> +#include <mutex> #include <set> #include <string> #include <sys/types.h> @@ -76,9 +78,14 @@ private: void mediaDeviceDisconnected(MediaDevice *media); virtual void disconnect(); + void doQueueRequest(); + std::vector<std::shared_ptr<MediaDevice>> mediaDevices_; std::vector<std::weak_ptr<Camera>> cameras_; + std::mutex waitingRequestsMutex_; + std::list<Request *> waitingRequests_; + const char *name_; friend class PipelineHandlerFactory; diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index cada864687ff..38edd00cebad 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -10,6 +10,7 @@ #include <sys/sysmacros.h> #include <libcamera/base/log.h> +#include <libcamera/base/thread.h> #include <libcamera/base/utils.h> #include <libcamera/camera.h> @@ -312,17 +313,45 @@ void PipelineHandler::queueRequest(Request *request) { LIBCAMERA_TRACEPOINT(request_queue, request); + { + MutexLocker lock(waitingRequestsMutex_); + waitingRequests_.push_back(request); + } + + doQueueRequest(); +} + +/** + * \brief Queue a request to the device + */ +void PipelineHandler::doQueueRequest() +{ + Request *request = nullptr; + { + MutexLocker lock(waitingRequestsMutex_); + + if (!waitingRequests_.size()) + return; + + request = waitingRequests_.front(); + waitingRequests_.pop_front(); + } + + /* Queue Request to the pipeline handler. */ Camera *camera = request->_d()->camera(); - Camera::Private *data = camera->_d(); - data->queuedRequests_.push_back(request); + Camera::Private *camData = camera->_d(); - request->sequence_ = data->requestSequence_++; + request->sequence_ = camData->requestSequence_++; + 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(); } /**
In order to prepare to handle synchronization fences at Request queueing time, split the PipelineHandler::queueRequest() function in two, by creating a list of waiting requests and introducing a new doQueueDevice() function that queues Requests to the device in the order the pipeline has received them. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/internal/pipeline_handler.h | 7 ++++ src/libcamera/pipeline_handler.cpp | 35 +++++++++++++++++-- 2 files changed, 39 insertions(+), 3 deletions(-)