[libcamera-devel] [PATCH 1/2] libcamera: pipeline_handler: Prepare requests

[Jacopo Mondi]

Hi Kieran,

On Fri, Jan 07, 2022 at 03:07:48PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2022-01-07 14:58:18)
> > Hi Kieran,
> >
> > 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
> > > 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.
>
> >
> > > ---
> > >  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);
> > >       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()
>
> 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);
> > >  }
> > >
> > > --
> > > 2.32.0
> > >