| Message ID | 20260618123844.656396-13-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Thu, Jun 18, 2026 at 02:38:29PM +0200, Barnabás Pőcze wrote: > The pipeline handler base class already tracks the queued requests of a camera > in the `Camera::Private` type, so there is no reason to have another queue > in the derived camera type that does the same. So remove it. > > The only significant difference is that if `queueRequestDevice()` fails, the > request can still stay in `queuedRequests_` if there were successfully queued > requests before it. This, however, does not cause issues as only the oldest > queued request is accessed in the rpi pipeline handler, and that cannot be a > cancelled request (it would have already been completed). This smells like one of those innocent looking changes that triggers some corner case. Maybe it doesn't, but I would only be confident in accepting this with proper testing. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- > .../pipeline/rpi/common/pipeline_base.h | 2 -- > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- > 4 files changed, 12 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index 6a6be5c820..efe06c6807 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > return ret; > } > > - /* Push the request to the back of the queue. */ > - data->requestQueue_.push(request); > data->handleState(); > > return 0; > @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) > > /* Add to the Request metadata buffer what the IPA has provided. */ > /* Last thing to do is to fill up the request metadata. */ > - Request *request = requestQueue_.front(); > + Request *request = queuedRequests_.front(); > request->_d()->metadata().merge(metadata); > > /* > @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() > * All outstanding requests (and associated buffers) must be returned > * back to the application. > */ > - while (!requestQueue_.empty()) { > - pipe()->cancelRequest(requestQueue_.front()); > - requestQueue_.pop(); > - } > + while (!queuedRequests_.empty()) > + pipe()->cancelRequest(queuedRequests_.front()); > } > > void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > * that we actually have one to action, otherwise we just return > * buffer back to the stream. > */ > - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); > + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); > if (request && request->findBuffer(stream) == buffer) { > FrameMetadata &md = buffer->_d()->metadata(); > > @@ -1468,7 +1464,7 @@ void CameraData::handleState() > > void CameraData::checkRequestCompleted() > { > - Request *request = requestQueue_.front(); > + Request *request = queuedRequests_.front(); > if (request->hasPendingBuffers()) > return; > > @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() > << request->sequence(); > > pipe()->completeRequest(request); > - requestQueue_.pop(); > > LOG(RPI, Debug) << "Going into Idle state"; > state_ = State::Idle; > @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon > * in the metadata, being the sequence number of the request whose ControlList > * has just been applied. > */ > - Request *request = requestQueue_.front(); > + Request *request = queuedRequests_.front(); > request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); > > /* > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > index 758155ee0d..4a8a769fd8 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > @@ -129,8 +129,6 @@ public: > return state_ != State::Stopped && state_ != State::Error; > } > > - std::queue<Request *> requestQueue_; > - > /* For handling digital zoom. */ > IPACameraSensorInfo sensorInfo_; > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > index b744c901f7..ef34be8338 100644 > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) > void PiSPCameraData::tryRunPipeline() > { > /* If any of our request or buffer queues are empty, we cannot proceed. */ > - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) > + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) > return; > > CfeJob &job = cfeJobQueue_.front(); > > /* Take the first request from the queue and action the IPA. */ > - Request *request = requestQueue_.front(); > + Request *request = queuedRequests_.front(); > ASSERT(request->metadata().empty()); > > /* See if a new ScalerCrop value needs to be applied. */ > @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() > params.buffers.bayer = RPi::MaskBayerData | bayerId; > params.buffers.stats = RPi::MaskStats | statsId; > params.buffers.embedded = 0; > - params.ipaContext = requestQueue_.front()->sequence(); > + params.ipaContext = request->sequence(); > params.delayContext = job.delayContext; > params.sensorControls = std::move(job.sensorControls); > /* params.requestControls is set by handleControlLists. */ > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 3e9a490589..4c159527be 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > if (stream == &isp_[Isp::Stats]) { > ipa::RPi::ProcessParams params; > params.buffers.stats = index | RPi::MaskStats; > - params.ipaContext = requestQueue_.front()->sequence(); > + params.ipaContext = queuedRequests_.front()->sequence(); > ipa_->processStats(params); > } else { > /* Any other ISP output can be handed back to the application now. */ > @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() > BayerFrame bayerFrame; > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > - if (state_ != State::Idle || requestQueue_.empty() || > + if (state_ != State::Idle || queuedRequests_.empty() || > bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) > return; > > @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() > return; > > /* Take the first request from the queue and action the IPA. */ > - Request *request = requestQueue_.front(); > + Request *request = queuedRequests_.front(); > ASSERT(request->metadata().empty()); > > /* See if a new ScalerCrop value needs to be applied. */ > -- > 2.54.0 >
On Mon, Jun 22, 2026 at 02:58:42PM +0200, Jacopo Mondi wrote: > Hi Barnabás > > On Thu, Jun 18, 2026 at 02:38:29PM +0200, Barnabás Pőcze wrote: > > The pipeline handler base class already tracks the queued requests of a camera > > in the `Camera::Private` type, so there is no reason to have another queue > > in the derived camera type that does the same. So remove it. > > > > The only significant difference is that if `queueRequestDevice()` fails, the > > request can still stay in `queuedRequests_` if there were successfully queued > > requests before it. This, however, does not cause issues as only the oldest > > queued request is accessed in the rpi pipeline handler, and that cannot be a > > cancelled request (it would have already been completed). > > This smells like one of those innocent looking changes that triggers > some corner case. Maybe it doesn't, but I would only be confident in > accepting this with proper testing. It's indeed a possibly tricky change, so I've analyzed the code flow changes in this patch. Please see below for comments to any change that isn't a straight s/requestQueue_/queuedRequest_/ > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > --- > > .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- > > .../pipeline/rpi/common/pipeline_base.h | 2 -- > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- > > 4 files changed, 12 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > index 6a6be5c820..efe06c6807 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > > return ret; > > } > > > > - /* Push the request to the back of the queue. */ > > - data->requestQueue_.push(request); PipelineHandlerBase::queueRequestDevice() is called by PipelineHandler::doQueueRequest() after adding the request to CameraData::queuedRequests_. > > data->handleState(); > > > > return 0; > > @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) > > > > /* Add to the Request metadata buffer what the IPA has provided. */ > > /* Last thing to do is to fill up the request metadata. */ > > - Request *request = requestQueue_.front(); > > + Request *request = queuedRequests_.front(); > > request->_d()->metadata().merge(metadata); > > > > /* > > @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() > > * All outstanding requests (and associated buffers) must be returned > > * back to the application. > > */ > > - while (!requestQueue_.empty()) { > > - pipe()->cancelRequest(requestQueue_.front()); > > - requestQueue_.pop(); > > - } > > + while (!queuedRequests_.empty()) > > + pipe()->cancelRequest(queuedRequests_.front()); PipelineHandler::cancelRequest() calls request->_d()->cancel(); completeRequest(request); The first call sets Request::Private::cancelled_ to true. The second call the call Request::Private::complete(), which sets Request::status_ to RequestCancelled based on cancelled_ being true. PipelineHandler::completeRequest() then iterates over CameraData::queuedRequests_ and remove non-pending requests from the front of the queue. The first request is guaranteed to be non-pending as it was just marked as cancelled, and is removed from the head of the queue. PipelineHandler::completeRequests() may remove further requests from the head of the queue if they are not pending. This can happen if a request was added to the back of the queue in a cancelled status (which I think can only happen if the fence times out). This shouldn't be a problem. Those requests will be completed properly by the function based on their current status (success of cancelled). If they're already cancelled there's no need to call PipelineHandler::cancelRequest() on them again, if they have successfully completed they should be returned as such to the application. > > } > > > > void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > * that we actually have one to action, otherwise we just return > > * buffer back to the stream. > > */ > > - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); > > + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); > > if (request && request->findBuffer(stream) == buffer) { > > FrameMetadata &md = buffer->_d()->metadata(); > > > > @@ -1468,7 +1464,7 @@ void CameraData::handleState() > > > > void CameraData::checkRequestCompleted() > > { > > - Request *request = requestQueue_.front(); > > + Request *request = queuedRequests_.front(); > > if (request->hasPendingBuffers()) > > return; > > > > @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() > > << request->sequence(); > > > > pipe()->completeRequest(request); > > - requestQueue_.pop(); PipelineHandler::completeRequest() removes the request from the front of the queue. > > > > LOG(RPI, Debug) << "Going into Idle state"; > > state_ = State::Idle; > > @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon > > * in the metadata, being the sequence number of the request whose ControlList > > * has just been applied. > > */ > > - Request *request = requestQueue_.front(); > > + Request *request = queuedRequests_.front(); > > request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); > > > > /* > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > index 758155ee0d..4a8a769fd8 100644 > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > @@ -129,8 +129,6 @@ public: > > return state_ != State::Stopped && state_ != State::Error; > > } > > > > - std::queue<Request *> requestQueue_; > > - > > /* For handling digital zoom. */ > > IPACameraSensorInfo sensorInfo_; > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > index b744c901f7..ef34be8338 100644 > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) > > void PiSPCameraData::tryRunPipeline() > > { > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) > > + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) > > return; > > > > CfeJob &job = cfeJobQueue_.front(); > > > > /* Take the first request from the queue and action the IPA. */ > > - Request *request = requestQueue_.front(); > > + Request *request = queuedRequests_.front(); > > ASSERT(request->metadata().empty()); > > > > /* See if a new ScalerCrop value needs to be applied. */ > > @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() > > params.buffers.bayer = RPi::MaskBayerData | bayerId; > > params.buffers.stats = RPi::MaskStats | statsId; > > params.buffers.embedded = 0; > > - params.ipaContext = requestQueue_.front()->sequence(); > > + params.ipaContext = request->sequence(); The request pointer is set above to the front of the queue, and nothing in-between changes it. All the changes in this patch seem to be safe, but how about code touching queuedRequests_ elsewhere ? Requests are added to the queue in PipelineHandler::doQueueRequest() and removed from the queue in PipelineHandler::completeRequest(). Let's start with removal from the queue. completeRequest() is called in rpi/common/pipeline_base.cpp in CameraData::checkRequestCompleted() and CameraData::clearIncompleteRequests(). Both call requestQueue_.pop() right after, and those pop() calls get removed by this patch (see above), so there's no change in behaviour. As for addition to the queue, doQueueRequest() calls data->queuedRequests_.push_back(request); request->_d()->sequence_ = data->requestSequence_++; if (request->_d()->cancelled_) { completeRequest(request); return; } int ret = queueRequestDevice(camera, request); If the request is not cancelled at this point, queueRequestDevice() is called right after adding the request to CameraData::queuedRequests_. This matches the removal of requestQueue_.push() in PipelineHandlerBase::queueRequestDevice(). If the request is cancelled, it will not make it to the pipeline handler, and will stay at the back of queuedRequests_ until all other requests in front complete, at which point it will also complete. As noted above, it shouldn't be an issue. As far as I can tell, the patch should not introduce any change in behaviour. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Nonetheless I second Jacopo's request for thorough testing. > > params.delayContext = job.delayContext; > > params.sensorControls = std::move(job.sensorControls); > > /* params.requestControls is set by handleControlLists. */ > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > index 3e9a490589..4c159527be 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > if (stream == &isp_[Isp::Stats]) { > > ipa::RPi::ProcessParams params; > > params.buffers.stats = index | RPi::MaskStats; > > - params.ipaContext = requestQueue_.front()->sequence(); > > + params.ipaContext = queuedRequests_.front()->sequence(); > > ipa_->processStats(params); > > } else { > > /* Any other ISP output can be handed back to the application now. */ > > @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() > > BayerFrame bayerFrame; > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > - if (state_ != State::Idle || requestQueue_.empty() || > > + if (state_ != State::Idle || queuedRequests_.empty() || > > bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) > > return; > > > > @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() > > return; > > > > /* Take the first request from the queue and action the IPA. */ > > - Request *request = requestQueue_.front(); > > + Request *request = queuedRequests_.front(); > > ASSERT(request->metadata().empty()); > > > > /* See if a new ScalerCrop value needs to be applied. */
Hi Barnabas Thanks for looking into this. On Tue, 23 Jun 2026 at 18:51, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Jun 22, 2026 at 02:58:42PM +0200, Jacopo Mondi wrote: > > Hi Barnabás > > > > On Thu, Jun 18, 2026 at 02:38:29PM +0200, Barnabás Pőcze wrote: > > > The pipeline handler base class already tracks the queued requests of a camera > > > in the `Camera::Private` type, so there is no reason to have another queue > > > in the derived camera type that does the same. So remove it. > > > > > > The only significant difference is that if `queueRequestDevice()` fails, the > > > request can still stay in `queuedRequests_` if there were successfully queued > > > requests before it. This, however, does not cause issues as only the oldest > > > queued request is accessed in the rpi pipeline handler, and that cannot be a > > > cancelled request (it would have already been completed). > > > > This smells like one of those innocent looking changes that triggers > > some corner case. Maybe it doesn't, but I would only be confident in > > accepting this with proper testing. > > It's indeed a possibly tricky change, so I've analyzed the code flow > changes in this patch. Please see below for comments to any change that > isn't a straight s/requestQueue_/queuedRequest_/ > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- > > > .../pipeline/rpi/common/pipeline_base.h | 2 -- > > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- > > > 4 files changed, 12 insertions(+), 19 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > index 6a6be5c820..efe06c6807 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > > > return ret; > > > } > > > > > > - /* Push the request to the back of the queue. */ > > > - data->requestQueue_.push(request); > > PipelineHandlerBase::queueRequestDevice() is called by > PipelineHandler::doQueueRequest() after adding the request to > CameraData::queuedRequests_. > > > > data->handleState(); > > > > > > return 0; > > > @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) > > > > > > /* Add to the Request metadata buffer what the IPA has provided. */ > > > /* Last thing to do is to fill up the request metadata. */ > > > - Request *request = requestQueue_.front(); > > > + Request *request = queuedRequests_.front(); > > > request->_d()->metadata().merge(metadata); > > > > > > /* > > > @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() > > > * All outstanding requests (and associated buffers) must be returned > > > * back to the application. > > > */ > > > - while (!requestQueue_.empty()) { > > > - pipe()->cancelRequest(requestQueue_.front()); > > > - requestQueue_.pop(); > > > - } > > > + while (!queuedRequests_.empty()) > > > + pipe()->cancelRequest(queuedRequests_.front()); > > PipelineHandler::cancelRequest() calls > > request->_d()->cancel(); > completeRequest(request); > > The first call sets Request::Private::cancelled_ to true. The second > call the call Request::Private::complete(), which sets Request::status_ > to RequestCancelled based on cancelled_ being true. > > PipelineHandler::completeRequest() then iterates over > CameraData::queuedRequests_ and remove non-pending requests from the > front of the queue. The first request is guaranteed to be non-pending as > it was just marked as cancelled, and is removed from the head of the > queue. > > PipelineHandler::completeRequests() may remove further requests from the > head of the queue if they are not pending. This can happen if a request > was added to the back of the queue in a cancelled status (which I think > can only happen if the fence times out). This shouldn't be a problem. > Those requests will be completed properly by the function based on their > current status (success of cancelled). If they're already cancelled > there's no need to call PipelineHandler::cancelRequest() on them again, > if they have successfully completed they should be returned as such to > the application. > > > > } > > > > > > void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > > @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > > * that we actually have one to action, otherwise we just return > > > * buffer back to the stream. > > > */ > > > - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); > > > + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); > > > if (request && request->findBuffer(stream) == buffer) { > > > FrameMetadata &md = buffer->_d()->metadata(); > > > > > > @@ -1468,7 +1464,7 @@ void CameraData::handleState() > > > > > > void CameraData::checkRequestCompleted() > > > { > > > - Request *request = requestQueue_.front(); > > > + Request *request = queuedRequests_.front(); > > > if (request->hasPendingBuffers()) > > > return; > > > > > > @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() > > > << request->sequence(); > > > > > > pipe()->completeRequest(request); > > > - requestQueue_.pop(); > > PipelineHandler::completeRequest() removes the request from the front of > the queue. > > > > > > > LOG(RPI, Debug) << "Going into Idle state"; > > > state_ = State::Idle; > > > @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon > > > * in the metadata, being the sequence number of the request whose ControlList > > > * has just been applied. > > > */ > > > - Request *request = requestQueue_.front(); > > > + Request *request = queuedRequests_.front(); > > > request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); > > > > > > /* > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > index 758155ee0d..4a8a769fd8 100644 > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > @@ -129,8 +129,6 @@ public: > > > return state_ != State::Stopped && state_ != State::Error; > > > } > > > > > > - std::queue<Request *> requestQueue_; > > > - > > > /* For handling digital zoom. */ > > > IPACameraSensorInfo sensorInfo_; > > > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > index b744c901f7..ef34be8338 100644 > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) > > > void PiSPCameraData::tryRunPipeline() > > > { > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) > > > + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) > > > return; > > > > > > CfeJob &job = cfeJobQueue_.front(); > > > > > > /* Take the first request from the queue and action the IPA. */ > > > - Request *request = requestQueue_.front(); > > > + Request *request = queuedRequests_.front(); > > > ASSERT(request->metadata().empty()); > > > > > > /* See if a new ScalerCrop value needs to be applied. */ > > > @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() > > > params.buffers.bayer = RPi::MaskBayerData | bayerId; > > > params.buffers.stats = RPi::MaskStats | statsId; > > > params.buffers.embedded = 0; > > > - params.ipaContext = requestQueue_.front()->sequence(); > > > + params.ipaContext = request->sequence(); > > The request pointer is set above to the front of the queue, and nothing > in-between changes it. > > > All the changes in this patch seem to be safe, but how about code > touching queuedRequests_ elsewhere ? Requests are added to the queue in > PipelineHandler::doQueueRequest() and removed from the queue in > PipelineHandler::completeRequest(). > > Let's start with removal from the queue. completeRequest() is called in > rpi/common/pipeline_base.cpp in CameraData::checkRequestCompleted() and > CameraData::clearIncompleteRequests(). Both call requestQueue_.pop() > right after, and those pop() calls get removed by this patch (see > above), so there's no change in behaviour. > > As for addition to the queue, doQueueRequest() calls > > data->queuedRequests_.push_back(request); > > request->_d()->sequence_ = data->requestSequence_++; > > if (request->_d()->cancelled_) { > completeRequest(request); > return; > } > > int ret = queueRequestDevice(camera, request); > > If the request is not cancelled at this point, queueRequestDevice() is > called right after adding the request to CameraData::queuedRequests_. > This matches the removal of requestQueue_.push() in > PipelineHandlerBase::queueRequestDevice(). > > If the request is cancelled, it will not make it to the pipeline > handler, and will stay at the back of queuedRequests_ until all other > requests in front complete, at which point it will also complete. As > noted above, it shouldn't be an issue. > > As far as I can tell, the patch should not introduce any change in > behaviour. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Nonetheless I second Jacopo's request for thorough testing. I did look through it and it certainly looked OK to me. I don't think we ever do anything weird (would it be weird?) like pushing cancelled requests on the queue. I have gone to the trouble of downloading it and running our standard set of tests on my Pi 5 and, as expected, it all seems fine, so it can have a Tested-by: David Plowman <david.plowman@raspberrypi.com> Though I do confess to a few very slight misgivings about fixing stuff that isn't broken. Does it make the Raspberry Pi code simpler if we have to remember that someone else could alter the queue, when previously they couldn't? But I suppose if it's simplifying things in the bigger picture, then that would make sense so I'm OK to add Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Maybe let's wait for Naush's "rb" too. Thanks David > > > > params.delayContext = job.delayContext; > > > params.sensorControls = std::move(job.sensorControls); > > > /* params.requestControls is set by handleControlLists. */ > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > index 3e9a490589..4c159527be 100644 > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > > if (stream == &isp_[Isp::Stats]) { > > > ipa::RPi::ProcessParams params; > > > params.buffers.stats = index | RPi::MaskStats; > > > - params.ipaContext = requestQueue_.front()->sequence(); > > > + params.ipaContext = queuedRequests_.front()->sequence(); > > > ipa_->processStats(params); > > > } else { > > > /* Any other ISP output can be handed back to the application now. */ > > > @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() > > > BayerFrame bayerFrame; > > > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > - if (state_ != State::Idle || requestQueue_.empty() || > > > + if (state_ != State::Idle || queuedRequests_.empty() || > > > bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) > > > return; > > > > > > @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() > > > return; > > > > > > /* Take the first request from the queue and action the IPA. */ > > > - Request *request = requestQueue_.front(); > > > + Request *request = queuedRequests_.front(); > > > ASSERT(request->metadata().empty()); > > > > > > /* See if a new ScalerCrop value needs to be applied. */ > > -- > Regards, > > Laurent Pinchart
On Fri, Jun 26, 2026 at 03:12:09PM +0100, David Plowman wrote: > On Tue, 23 Jun 2026 at 18:51, Laurent Pinchart wrote: > > On Mon, Jun 22, 2026 at 02:58:42PM +0200, Jacopo Mondi wrote: > > > On Thu, Jun 18, 2026 at 02:38:29PM +0200, Barnabás Pőcze wrote: > > > > The pipeline handler base class already tracks the queued requests of a camera > > > > in the `Camera::Private` type, so there is no reason to have another queue > > > > in the derived camera type that does the same. So remove it. > > > > > > > > The only significant difference is that if `queueRequestDevice()` fails, the > > > > request can still stay in `queuedRequests_` if there were successfully queued > > > > requests before it. This, however, does not cause issues as only the oldest > > > > queued request is accessed in the rpi pipeline handler, and that cannot be a > > > > cancelled request (it would have already been completed). > > > > > > This smells like one of those innocent looking changes that triggers > > > some corner case. Maybe it doesn't, but I would only be confident in > > > accepting this with proper testing. > > > > It's indeed a possibly tricky change, so I've analyzed the code flow > > changes in this patch. Please see below for comments to any change that > > isn't a straight s/requestQueue_/queuedRequest_/ > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > --- > > > > .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- > > > > .../pipeline/rpi/common/pipeline_base.h | 2 -- > > > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- > > > > 4 files changed, 12 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > index 6a6be5c820..efe06c6807 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > > > > @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) > > > > return ret; > > > > } > > > > > > > > - /* Push the request to the back of the queue. */ > > > > - data->requestQueue_.push(request); > > > > PipelineHandlerBase::queueRequestDevice() is called by > > PipelineHandler::doQueueRequest() after adding the request to > > CameraData::queuedRequests_. > > > > > > data->handleState(); > > > > > > > > return 0; > > > > @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) > > > > > > > > /* Add to the Request metadata buffer what the IPA has provided. */ > > > > /* Last thing to do is to fill up the request metadata. */ > > > > - Request *request = requestQueue_.front(); > > > > + Request *request = queuedRequests_.front(); > > > > request->_d()->metadata().merge(metadata); > > > > > > > > /* > > > > @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() > > > > * All outstanding requests (and associated buffers) must be returned > > > > * back to the application. > > > > */ > > > > - while (!requestQueue_.empty()) { > > > > - pipe()->cancelRequest(requestQueue_.front()); > > > > - requestQueue_.pop(); > > > > - } > > > > + while (!queuedRequests_.empty()) > > > > + pipe()->cancelRequest(queuedRequests_.front()); > > > > PipelineHandler::cancelRequest() calls > > > > request->_d()->cancel(); > > completeRequest(request); > > > > The first call sets Request::Private::cancelled_ to true. The second > > call the call Request::Private::complete(), which sets Request::status_ > > to RequestCancelled based on cancelled_ being true. > > > > PipelineHandler::completeRequest() then iterates over > > CameraData::queuedRequests_ and remove non-pending requests from the > > front of the queue. The first request is guaranteed to be non-pending as > > it was just marked as cancelled, and is removed from the head of the > > queue. > > > > PipelineHandler::completeRequests() may remove further requests from the > > head of the queue if they are not pending. This can happen if a request > > was added to the back of the queue in a cancelled status (which I think > > can only happen if the fence times out). This shouldn't be a problem. > > Those requests will be completed properly by the function based on their > > current status (success of cancelled). If they're already cancelled > > there's no need to call PipelineHandler::cancelRequest() on them again, > > if they have successfully completed they should be returned as such to > > the application. > > > > > > } > > > > > > > > void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > > > @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) > > > > * that we actually have one to action, otherwise we just return > > > > * buffer back to the stream. > > > > */ > > > > - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); > > > > + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); > > > > if (request && request->findBuffer(stream) == buffer) { > > > > FrameMetadata &md = buffer->_d()->metadata(); > > > > > > > > @@ -1468,7 +1464,7 @@ void CameraData::handleState() > > > > > > > > void CameraData::checkRequestCompleted() > > > > { > > > > - Request *request = requestQueue_.front(); > > > > + Request *request = queuedRequests_.front(); > > > > if (request->hasPendingBuffers()) > > > > return; > > > > > > > > @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() > > > > << request->sequence(); > > > > > > > > pipe()->completeRequest(request); > > > > - requestQueue_.pop(); > > > > PipelineHandler::completeRequest() removes the request from the front of > > the queue. > > > > > > > > > > LOG(RPI, Debug) << "Going into Idle state"; > > > > state_ = State::Idle; > > > > @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon > > > > * in the metadata, being the sequence number of the request whose ControlList > > > > * has just been applied. > > > > */ > > > > - Request *request = requestQueue_.front(); > > > > + Request *request = queuedRequests_.front(); > > > > request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); > > > > > > > > /* > > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > > index 758155ee0d..4a8a769fd8 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h > > > > @@ -129,8 +129,6 @@ public: > > > > return state_ != State::Stopped && state_ != State::Error; > > > > } > > > > > > > > - std::queue<Request *> requestQueue_; > > > > - > > > > /* For handling digital zoom. */ > > > > IPACameraSensorInfo sensorInfo_; > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > index b744c901f7..ef34be8338 100644 > > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) > > > > void PiSPCameraData::tryRunPipeline() > > > > { > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > > - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) > > > > + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) > > > > return; > > > > > > > > CfeJob &job = cfeJobQueue_.front(); > > > > > > > > /* Take the first request from the queue and action the IPA. */ > > > > - Request *request = requestQueue_.front(); > > > > + Request *request = queuedRequests_.front(); > > > > ASSERT(request->metadata().empty()); > > > > > > > > /* See if a new ScalerCrop value needs to be applied. */ > > > > @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() > > > > params.buffers.bayer = RPi::MaskBayerData | bayerId; > > > > params.buffers.stats = RPi::MaskStats | statsId; > > > > params.buffers.embedded = 0; > > > > - params.ipaContext = requestQueue_.front()->sequence(); > > > > + params.ipaContext = request->sequence(); > > > > The request pointer is set above to the front of the queue, and nothing > > in-between changes it. > > > > > > All the changes in this patch seem to be safe, but how about code > > touching queuedRequests_ elsewhere ? Requests are added to the queue in > > PipelineHandler::doQueueRequest() and removed from the queue in > > PipelineHandler::completeRequest(). > > > > Let's start with removal from the queue. completeRequest() is called in > > rpi/common/pipeline_base.cpp in CameraData::checkRequestCompleted() and > > CameraData::clearIncompleteRequests(). Both call requestQueue_.pop() > > right after, and those pop() calls get removed by this patch (see > > above), so there's no change in behaviour. > > > > As for addition to the queue, doQueueRequest() calls > > > > data->queuedRequests_.push_back(request); > > > > request->_d()->sequence_ = data->requestSequence_++; > > > > if (request->_d()->cancelled_) { > > completeRequest(request); > > return; > > } > > > > int ret = queueRequestDevice(camera, request); > > > > If the request is not cancelled at this point, queueRequestDevice() is > > called right after adding the request to CameraData::queuedRequests_. > > This matches the removal of requestQueue_.push() in > > PipelineHandlerBase::queueRequestDevice(). > > > > If the request is cancelled, it will not make it to the pipeline > > handler, and will stay at the back of queuedRequests_ until all other > > requests in front complete, at which point it will also complete. As > > noted above, it shouldn't be an issue. > > > > As far as I can tell, the patch should not introduce any change in > > behaviour. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Nonetheless I second Jacopo's request for thorough testing. > > I did look through it and it certainly looked OK to me. I don't think > we ever do anything weird (would it be weird?) like pushing cancelled > requests on the queue. > > I have gone to the trouble of downloading it and running our standard > set of tests on my Pi 5 and, as expected, it all seems fine, so it can > have a > > Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks for testing. > Though I do confess to a few very slight misgivings about fixing stuff > that isn't broken. Does it make the Raspberry Pi code simpler if we > have to remember that someone else could alter the queue, when > previously they couldn't? But I suppose if it's simplifying things in > the bigger picture, then that would make sense so I'm OK to add Barnab@s is working on splitting buffers from requests, and as far as I understand the custom requestQueue_ in the Raspberry Pi pipeline handler got in the way. It was a candidate to address in this preparatory series to reduce the size of the full upcoming series. > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Maybe let's wait for Naush's "rb" too. > > > > > params.delayContext = job.delayContext; > > > > params.sensorControls = std::move(job.sensorControls); > > > > /* params.requestControls is set by handleControlLists. */ > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > index 3e9a490589..4c159527be 100644 > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) > > > > if (stream == &isp_[Isp::Stats]) { > > > > ipa::RPi::ProcessParams params; > > > > params.buffers.stats = index | RPi::MaskStats; > > > > - params.ipaContext = requestQueue_.front()->sequence(); > > > > + params.ipaContext = queuedRequests_.front()->sequence(); > > > > ipa_->processStats(params); > > > > } else { > > > > /* Any other ISP output can be handed back to the application now. */ > > > > @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() > > > > BayerFrame bayerFrame; > > > > > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > > - if (state_ != State::Idle || requestQueue_.empty() || > > > > + if (state_ != State::Idle || queuedRequests_.empty() || > > > > bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) > > > > return; > > > > > > > > @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() > > > > return; > > > > > > > > /* Take the first request from the queue and action the IPA. */ > > > > - Request *request = requestQueue_.front(); > > > > + Request *request = queuedRequests_.front(); > > > > ASSERT(request->metadata().empty()); > > > > > > > > /* See if a new ScalerCrop value needs to be applied. */
2026. 06. 26. 16:18 keltezéssel, Laurent Pinchart írta: > On Fri, Jun 26, 2026 at 03:12:09PM +0100, David Plowman wrote: >> On Tue, 23 Jun 2026 at 18:51, Laurent Pinchart wrote: >>> On Mon, Jun 22, 2026 at 02:58:42PM +0200, Jacopo Mondi wrote: >>>> On Thu, Jun 18, 2026 at 02:38:29PM +0200, Barnabás Pőcze wrote: >>>>> The pipeline handler base class already tracks the queued requests of a camera >>>>> in the `Camera::Private` type, so there is no reason to have another queue >>>>> in the derived camera type that does the same. So remove it. >>>>> >>>>> The only significant difference is that if `queueRequestDevice()` fails, the >>>>> request can still stay in `queuedRequests_` if there were successfully queued >>>>> requests before it. This, however, does not cause issues as only the oldest >>>>> queued request is accessed in the rpi pipeline handler, and that cannot be a >>>>> cancelled request (it would have already been completed). >>>> >>>> This smells like one of those innocent looking changes that triggers >>>> some corner case. Maybe it doesn't, but I would only be confident in >>>> accepting this with proper testing. >>> >>> It's indeed a possibly tricky change, so I've analyzed the code flow >>> changes in this patch. Please see below for comments to any change that >>> isn't a straight s/requestQueue_/queuedRequest_/ >>> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- >>>>> .../pipeline/rpi/common/pipeline_base.h | 2 -- >>>>> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- >>>>> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- >>>>> 4 files changed, 12 insertions(+), 19 deletions(-) >>>>> >>>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >>>>> index 6a6be5c820..efe06c6807 100644 >>>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >>>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >>>>> @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) >>>>> return ret; >>>>> } >>>>> >>>>> - /* Push the request to the back of the queue. */ >>>>> - data->requestQueue_.push(request); >>> >>> PipelineHandlerBase::queueRequestDevice() is called by >>> PipelineHandler::doQueueRequest() after adding the request to >>> CameraData::queuedRequests_. >>> >>>>> data->handleState(); >>>>> >>>>> return 0; >>>>> @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) >>>>> >>>>> /* Add to the Request metadata buffer what the IPA has provided. */ >>>>> /* Last thing to do is to fill up the request metadata. */ >>>>> - Request *request = requestQueue_.front(); >>>>> + Request *request = queuedRequests_.front(); >>>>> request->_d()->metadata().merge(metadata); >>>>> >>>>> /* >>>>> @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() >>>>> * All outstanding requests (and associated buffers) must be returned >>>>> * back to the application. >>>>> */ >>>>> - while (!requestQueue_.empty()) { >>>>> - pipe()->cancelRequest(requestQueue_.front()); >>>>> - requestQueue_.pop(); >>>>> - } >>>>> + while (!queuedRequests_.empty()) >>>>> + pipe()->cancelRequest(queuedRequests_.front()); >>> >>> PipelineHandler::cancelRequest() calls >>> >>> request->_d()->cancel(); >>> completeRequest(request); >>> >>> The first call sets Request::Private::cancelled_ to true. The second >>> call the call Request::Private::complete(), which sets Request::status_ >>> to RequestCancelled based on cancelled_ being true. >>> >>> PipelineHandler::completeRequest() then iterates over >>> CameraData::queuedRequests_ and remove non-pending requests from the >>> front of the queue. The first request is guaranteed to be non-pending as >>> it was just marked as cancelled, and is removed from the head of the >>> queue. >>> >>> PipelineHandler::completeRequests() may remove further requests from the >>> head of the queue if they are not pending. This can happen if a request >>> was added to the back of the queue in a cancelled status (which I think >>> can only happen if the fence times out). This shouldn't be a problem. >>> Those requests will be completed properly by the function based on their >>> current status (success of cancelled). If they're already cancelled >>> there's no need to call PipelineHandler::cancelRequest() on them again, >>> if they have successfully completed they should be returned as such to >>> the application. >>> >>>>> } >>>>> >>>>> void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) >>>>> @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) >>>>> * that we actually have one to action, otherwise we just return >>>>> * buffer back to the stream. >>>>> */ >>>>> - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); >>>>> + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); >>>>> if (request && request->findBuffer(stream) == buffer) { >>>>> FrameMetadata &md = buffer->_d()->metadata(); >>>>> >>>>> @@ -1468,7 +1464,7 @@ void CameraData::handleState() >>>>> >>>>> void CameraData::checkRequestCompleted() >>>>> { >>>>> - Request *request = requestQueue_.front(); >>>>> + Request *request = queuedRequests_.front(); >>>>> if (request->hasPendingBuffers()) >>>>> return; >>>>> >>>>> @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() >>>>> << request->sequence(); >>>>> >>>>> pipe()->completeRequest(request); >>>>> - requestQueue_.pop(); >>> >>> PipelineHandler::completeRequest() removes the request from the front of >>> the queue. >>> >>>>> >>>>> LOG(RPI, Debug) << "Going into Idle state"; >>>>> state_ = State::Idle; >>>>> @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon >>>>> * in the metadata, being the sequence number of the request whose ControlList >>>>> * has just been applied. >>>>> */ >>>>> - Request *request = requestQueue_.front(); >>>>> + Request *request = queuedRequests_.front(); >>>>> request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); >>>>> >>>>> /* >>>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h >>>>> index 758155ee0d..4a8a769fd8 100644 >>>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h >>>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h >>>>> @@ -129,8 +129,6 @@ public: >>>>> return state_ != State::Stopped && state_ != State::Error; >>>>> } >>>>> >>>>> - std::queue<Request *> requestQueue_; >>>>> - >>>>> /* For handling digital zoom. */ >>>>> IPACameraSensorInfo sensorInfo_; >>>>> >>>>> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp >>>>> index b744c901f7..ef34be8338 100644 >>>>> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp >>>>> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp >>>>> @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) >>>>> void PiSPCameraData::tryRunPipeline() >>>>> { >>>>> /* If any of our request or buffer queues are empty, we cannot proceed. */ >>>>> - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) >>>>> + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) >>>>> return; >>>>> >>>>> CfeJob &job = cfeJobQueue_.front(); >>>>> >>>>> /* Take the first request from the queue and action the IPA. */ >>>>> - Request *request = requestQueue_.front(); >>>>> + Request *request = queuedRequests_.front(); >>>>> ASSERT(request->metadata().empty()); >>>>> >>>>> /* See if a new ScalerCrop value needs to be applied. */ >>>>> @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() >>>>> params.buffers.bayer = RPi::MaskBayerData | bayerId; >>>>> params.buffers.stats = RPi::MaskStats | statsId; >>>>> params.buffers.embedded = 0; >>>>> - params.ipaContext = requestQueue_.front()->sequence(); >>>>> + params.ipaContext = request->sequence(); >>> >>> The request pointer is set above to the front of the queue, and nothing >>> in-between changes it. >>> >>> >>> All the changes in this patch seem to be safe, but how about code >>> touching queuedRequests_ elsewhere ? Requests are added to the queue in >>> PipelineHandler::doQueueRequest() and removed from the queue in >>> PipelineHandler::completeRequest(). >>> >>> Let's start with removal from the queue. completeRequest() is called in >>> rpi/common/pipeline_base.cpp in CameraData::checkRequestCompleted() and >>> CameraData::clearIncompleteRequests(). Both call requestQueue_.pop() >>> right after, and those pop() calls get removed by this patch (see >>> above), so there's no change in behaviour. >>> >>> As for addition to the queue, doQueueRequest() calls >>> >>> data->queuedRequests_.push_back(request); >>> >>> request->_d()->sequence_ = data->requestSequence_++; >>> >>> if (request->_d()->cancelled_) { >>> completeRequest(request); >>> return; >>> } >>> >>> int ret = queueRequestDevice(camera, request); >>> >>> If the request is not cancelled at this point, queueRequestDevice() is >>> called right after adding the request to CameraData::queuedRequests_. >>> This matches the removal of requestQueue_.push() in >>> PipelineHandlerBase::queueRequestDevice(). >>> >>> If the request is cancelled, it will not make it to the pipeline >>> handler, and will stay at the back of queuedRequests_ until all other >>> requests in front complete, at which point it will also complete. As >>> noted above, it shouldn't be an issue. >>> >>> As far as I can tell, the patch should not introduce any change in >>> behaviour. >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>> Nonetheless I second Jacopo's request for thorough testing. >> >> I did look through it and it certainly looked OK to me. I don't think >> we ever do anything weird (would it be weird?) like pushing cancelled >> requests on the queue. >> >> I have gone to the trouble of downloading it and running our standard >> set of tests on my Pi 5 and, as expected, it all seems fine, so it can >> have a >> >> Tested-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks for testing. > >> Though I do confess to a few very slight misgivings about fixing stuff >> that isn't broken. Does it make the Raspberry Pi code simpler if we >> have to remember that someone else could alter the queue, when >> previously they couldn't? But I suppose if it's simplifying things in >> the bigger picture, then that would make sense so I'm OK to add > > Barnab@s is working on splitting buffers from requests, and as far as I > understand the custom requestQueue_ in the Raspberry Pi pipeline handler > got in the way. It was a candidate to address in this preparatory series > to reduce the size of the full upcoming series. Since we're not converting all pipeline handlers in the upcoming changeset, I think I'll just omit this change because in contrary to my earlier expectations it is not actually needed. So sorry for the wasted effort. :( > >> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> >> >> Maybe let's wait for Naush's "rb" too. >> >>>>> params.delayContext = job.delayContext; >>>>> params.sensorControls = std::move(job.sensorControls); >>>>> /* params.requestControls is set by handleControlLists. */ >>>>> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >>>>> index 3e9a490589..4c159527be 100644 >>>>> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >>>>> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >>>>> @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) >>>>> if (stream == &isp_[Isp::Stats]) { >>>>> ipa::RPi::ProcessParams params; >>>>> params.buffers.stats = index | RPi::MaskStats; >>>>> - params.ipaContext = requestQueue_.front()->sequence(); >>>>> + params.ipaContext = queuedRequests_.front()->sequence(); >>>>> ipa_->processStats(params); >>>>> } else { >>>>> /* Any other ISP output can be handed back to the application now. */ >>>>> @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() >>>>> BayerFrame bayerFrame; >>>>> >>>>> /* If any of our request or buffer queues are empty, we cannot proceed. */ >>>>> - if (state_ != State::Idle || requestQueue_.empty() || >>>>> + if (state_ != State::Idle || queuedRequests_.empty() || >>>>> bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) >>>>> return; >>>>> >>>>> @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() >>>>> return; >>>>> >>>>> /* Take the first request from the queue and action the IPA. */ >>>>> - Request *request = requestQueue_.front(); >>>>> + Request *request = queuedRequests_.front(); >>>>> ASSERT(request->metadata().empty()); >>>>> >>>>> /* See if a new ScalerCrop value needs to be applied. */ >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index 6a6be5c820..efe06c6807 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -786,8 +786,6 @@ int PipelineHandlerBase::queueRequestDevice(Camera *camera, Request *request) return ret; } - /* Push the request to the back of the queue. */ - data->requestQueue_.push(request); data->handleState(); return 0; @@ -1241,7 +1239,7 @@ void CameraData::metadataReady(const ControlList &metadata) /* Add to the Request metadata buffer what the IPA has provided. */ /* Last thing to do is to fill up the request metadata. */ - Request *request = requestQueue_.front(); + Request *request = queuedRequests_.front(); request->_d()->metadata().merge(metadata); /* @@ -1400,10 +1398,8 @@ void CameraData::clearIncompleteRequests() * All outstanding requests (and associated buffers) must be returned * back to the application. */ - while (!requestQueue_.empty()) { - pipe()->cancelRequest(requestQueue_.front()); - requestQueue_.pop(); - } + while (!queuedRequests_.empty()) + pipe()->cancelRequest(queuedRequests_.front()); } void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) @@ -1413,7 +1409,7 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream) * that we actually have one to action, otherwise we just return * buffer back to the stream. */ - Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front(); + Request *request = queuedRequests_.empty() ? nullptr : queuedRequests_.front(); if (request && request->findBuffer(stream) == buffer) { FrameMetadata &md = buffer->_d()->metadata(); @@ -1468,7 +1464,7 @@ void CameraData::handleState() void CameraData::checkRequestCompleted() { - Request *request = requestQueue_.front(); + Request *request = queuedRequests_.front(); if (request->hasPendingBuffers()) return; @@ -1480,7 +1476,6 @@ void CameraData::checkRequestCompleted() << request->sequence(); pipe()->completeRequest(request); - requestQueue_.pop(); LOG(RPI, Debug) << "Going into Idle state"; state_ = State::Idle; @@ -1536,7 +1531,7 @@ void CameraData::handleControlLists(uint32_t delayContext, ControlList ¶mCon * in the metadata, being the sequence number of the request whose ControlList * has just been applied. */ - Request *request = requestQueue_.front(); + Request *request = queuedRequests_.front(); request->_d()->metadata().set(controls::rpi::ControlListSequence, delayContext); /* diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h index 758155ee0d..4a8a769fd8 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h @@ -129,8 +129,6 @@ public: return state_ != State::Stopped && state_ != State::Error; } - std::queue<Request *> requestQueue_; - /* For handling digital zoom. */ IPACameraSensorInfo sensorInfo_; diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp index b744c901f7..ef34be8338 100644 --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp @@ -2308,13 +2308,13 @@ void PiSPCameraData::prepareBe(uint32_t bufferId, bool stitchSwapBuffers) void PiSPCameraData::tryRunPipeline() { /* If any of our request or buffer queues are empty, we cannot proceed. */ - if (state_ != State::Idle || requestQueue_.empty() || !cfeJobComplete()) + if (state_ != State::Idle || queuedRequests_.empty() || !cfeJobComplete()) return; CfeJob &job = cfeJobQueue_.front(); /* Take the first request from the queue and action the IPA. */ - Request *request = requestQueue_.front(); + Request *request = queuedRequests_.front(); ASSERT(request->metadata().empty()); /* See if a new ScalerCrop value needs to be applied. */ @@ -2335,7 +2335,7 @@ void PiSPCameraData::tryRunPipeline() params.buffers.bayer = RPi::MaskBayerData | bayerId; params.buffers.stats = RPi::MaskStats | statsId; params.buffers.embedded = 0; - params.ipaContext = requestQueue_.front()->sequence(); + params.ipaContext = request->sequence(); params.delayContext = job.delayContext; params.sensorControls = std::move(job.sensorControls); /* params.requestControls is set by handleControlLists. */ diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index 3e9a490589..4c159527be 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -839,7 +839,7 @@ void Vc4CameraData::ispOutputDequeue(FrameBuffer *buffer) if (stream == &isp_[Isp::Stats]) { ipa::RPi::ProcessParams params; params.buffers.stats = index | RPi::MaskStats; - params.ipaContext = requestQueue_.front()->sequence(); + params.ipaContext = queuedRequests_.front()->sequence(); ipa_->processStats(params); } else { /* Any other ISP output can be handed back to the application now. */ @@ -923,7 +923,7 @@ void Vc4CameraData::tryRunPipeline() BayerFrame bayerFrame; /* If any of our request or buffer queues are empty, we cannot proceed. */ - if (state_ != State::Idle || requestQueue_.empty() || + if (state_ != State::Idle || queuedRequests_.empty() || bayerQueue_.empty() || (embeddedQueue_.empty() && sensorMetadata_)) return; @@ -931,7 +931,7 @@ void Vc4CameraData::tryRunPipeline() return; /* Take the first request from the queue and action the IPA. */ - Request *request = requestQueue_.front(); + Request *request = queuedRequests_.front(); ASSERT(request->metadata().empty()); /* See if a new ScalerCrop value needs to be applied. */
The pipeline handler base class already tracks the queued requests of a camera in the `Camera::Private` type, so there is no reason to have another queue in the derived camera type that does the same. So remove it. The only significant difference is that if `queueRequestDevice()` fails, the request can still stay in `queuedRequests_` if there were successfully queued requests before it. This, however, does not cause issues as only the oldest queued request is accessed in the rpi pipeline handler, and that cannot be a cancelled request (it would have already been completed). Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- .../pipeline/rpi/common/pipeline_base.cpp | 17 ++++++----------- .../pipeline/rpi/common/pipeline_base.h | 2 -- src/libcamera/pipeline/rpi/pisp/pisp.cpp | 6 +++--- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 6 +++--- 4 files changed, 12 insertions(+), 19 deletions(-)