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

[Kieran Bingham]

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...

What about 
	PipelineHandler::registerRequest()

To state that the request is 'registered' with a single pipeline
handler?

> 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.

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.
	 */

> > +      * 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
> >