[libcamera-devel] [PATCH 07/10] libcamera: pipeline_handler: Split request queueing
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Wed Nov 10 11:02:33 CET 2021
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 at 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
> > > >
More information about the libcamera-devel
mailing list