| Message ID | 20220107125506.1477795-2-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> Nice, this sounds better than disconnecting after the signal has been emitted. > --- > 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 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 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() > + * 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 >
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@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 > >
Hi Kieran Thanks for looking into this. I've tried it on the example in the original bug report and can confirm that it is fixed and now works properly: Tested-by: David Plowman <david.plowman@raspberrypi.com> David On Fri, 7 Jan 2022 at 15:07, Kieran Bingham <kieran.bingham@ideasonboard.com> 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@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 > > >
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@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 > > >
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@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); > > > > } > > > >
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 */ + 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 + * 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); }
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@ideasonboard.com> --- 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(-)