[libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Prepare requests
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 9 02:29:25 CET 2022
Hello,
On Fri, Jan 07, 2022 at 04:53:13PM +0100, Jacopo Mondi wrote:
> On Fri, Jan 07, 2022 at 03:07:48PM +0000, Kieran Bingham wrote:
> > Quoting Jacopo Mondi (2022-01-07 14:58:18)
> > > On Fri, Jan 07, 2022 at 12:55:05PM +0000, Kieran Bingham wrote:
> > > > Provide a call allowing requests to be prepared and associated with the
> > > > pipeline handler after being constructed by the camera.
> > > >
> > > > This provides an opportunity for the Pipeline Handler to connect any
> > > > signals it may be interested in receiveing for the request such as
s/receiveing/receiving/
> > > > getting notifications when the request is ready for processing when
> > > > using a fence.
> > > >
> > > > While here, update the existing usage of the d pointer in
> > > > Camera::createRequest() to match the style of other functions.
> > > >
> > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > > Nice, this sounds better than disconnecting after the signal has been
> > > emitted.
> >
> > That's what I thought, I couldn't bear to send a patch that disconnected
> > the signal on every request to have it reconnect again.
Avoiding constant connections and disconnections is certainly nice (but
note that we will connect and disconnect the EventNotifier::activated
signal for each fence every time a request is queued, we can't do much
about that). I'm not thrilled about connecting the Request::prepared
signal when creating the request though, it feels wrong somehow, but as
it's internal I can live with it.
> > > > ---
> > > > include/libcamera/internal/pipeline_handler.h | 1 +
> > > > src/libcamera/camera.cpp | 14 ++++++++++---
> > > > src/libcamera/pipeline_handler.cpp | 20 ++++++++++++++++---
> > > > 3 files changed, 29 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > > index e5b8ffb4db3d..ef7e1390560f 100644
> > > > --- a/include/libcamera/internal/pipeline_handler.h
> > > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > > @@ -59,6 +59,7 @@ public:
> > > > void stop(Camera *camera);
> > > > bool hasPendingRequests(const Camera *camera) const;
> > > >
> > > > + void prepareRequest(Request *request);
I don't like the name, given that the request has a prepared signal that
is emitted when the request is queued, which is completely unrelated to
this "prepare" operation. associateRequest() could be better, I'm sure
there are even better names.
> > > > void queueRequest(Request *request);
> > > >
> > > > bool completeBuffer(Request *request, FrameBuffer *buffer);
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index 86d84ac0a77d..1a00b34d9d9d 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -1074,12 +1074,20 @@ int Camera::configure(CameraConfiguration *config)
> > > > */
> > > > std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > > > {
> > > > - int ret = _d()->isAccessAllowed(Private::CameraConfigured,
> > > > - Private::CameraRunning);
> > > > + Private *const d = _d();
> > > > +
> > > > + int ret = d->isAccessAllowed(Private::CameraConfigured,
> > > > + Private::CameraRunning);
> > > > if (ret < 0)
> > > > return nullptr;
> > > >
> > > > - return std::make_unique<Request>(this, cookie);
> > > > + std::unique_ptr<Request> request = std::make_unique<Request>(this, cookie);
> > > > +
> > > > + /* Associate the request with the pipeline handler */
> > >
> > > It doesn't really associate it, but rather initialize it
> >
> > "Intialise the request with the pipeline handler" doesn't sound right
> > though... ?
> >
> > > I would also be tempted to suggest to rename the function to
> > > PipelineHandler::createRequest() to make sure it is immediately clear
> > > where the function is meant to be called from and when, as all the
> >
> > That would bother me as the PipelineHandler is not createing the
> > request...
>
> No it's not, but it's in the creation call path.
>
> > What about
> > PipelineHandler::registerRequest()
Ah, there we go :-)
> > To state that the request is 'registered' with a single pipeline
> > handler?
>
> Works for met
>
> > > other 'prepare' actions happen at request queue time, not at request
> > > creation time.
> > >
> > > > + if (request)
> > > > + d->pipe_->prepareRequest(request.get());
> > > > +
> > > > + return request;
> > > > }
> > > >
> > > > /**
> > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > > index 03e4b9e66aa6..18b59ef27fb6 100644
> > > > --- a/src/libcamera/pipeline_handler.cpp
> > > > +++ b/src/libcamera/pipeline_handler.cpp
> > > > @@ -337,6 +337,23 @@ bool PipelineHandler::hasPendingRequests(const Camera *camera) const
> > > > return !camera->_d()->queuedRequests_.empty();
> > > > }
> > > >
> > > > +/**
> > > > + * \fn PipelineHandler::prepareRequest()
> > > > + * \brief Prepare a request for use by the Pipeline Handler
> > > > + * \param[in] request The request to prepare
> > > > + */
> > > > +void PipelineHandler::prepareRequest(Request *request)
> > > > +{
> > > > + /*
> > > > + * Connect the request prepared signal to the pipeline handler
> > > > + * to manage fence based preparations allowing the request
> > >
> > > I would drop this statement. Fences handling is only potentially one
> > > the preparation steps we might want to handle in Request::prepare()
> >
> > I can drop it, but the reason for having the comment here is to delcare
> > what this connection is for. This function (which could be renamed to
> > registerRequest()) could do other things when registering the request
> > too.
>
> Sorry, I wasn't clear, I was suggestin to just drop
>
> + * to manage fence based preparations allowing the request
>
> > Is this better?
> >
> > /*
> > * Connect the request prepared signal to the pipeline handler
> > * to notify the pipeline handler when a request is ready to be
> > * processed.
> > */
>
> With one of "the pipeline handler" dropped, yes :)
>
> > > > + * to inform us when it is ready to be processed by hardware.
> > > > + */
> > > > + request->_d()->prepared.connect(this, [this]() {
> > > > + doQueueRequests();
> > > > + });
> > > > +}
> > > > +
> > > > /**
> > > > * \fn PipelineHandler::queueRequest()
> > > > * \brief Queue a request
> > > > @@ -366,9 +383,6 @@ void PipelineHandler::queueRequest(Request *request)
> > > >
> > > > waitingRequests_.push(request);
> > > >
> > > > - request->_d()->prepared.connect(this, [this]() {
> > > > - doQueueRequests();
> > > > - });
> > > > request->_d()->prepare(300ms);
> > > > }
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list