| Message ID | 20210603124345.2492885-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi all, Just a nudge on this one.... Thanks, Naush On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote: > With the addition of FrameBuffer::cancel(), the logic to clear and return > pending requests can be simplified by not having to queue all the request > buffers to the device before calling streamOff(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > 1 file changed, 8 insertions(+), 38 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a65b4568256c..887f8d0f7404 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > - /* This also stops the streams. */ > + for (auto const stream : data->streams_) > + stream->dev()->streamOff(); > + > data->clearIncompleteRequests(); > data->bayerQueue_ = {}; > data->embeddedQueue_ = {}; > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer > *buffer) > > void RPiCameraData::clearIncompleteRequests() > { > - /* > - * Queue up any buffers passed in the request. > - * This is needed because streamOff() will then mark the buffers as > - * cancelled. > - */ > - for (auto const request : requestQueue_) { > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > - > - FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer) > - stream->queueBuffer(buffer); > - } > - } > - > - /* Stop all streams. */ > - for (auto const stream : streams_) > - stream->dev()->streamOff(); > - > /* > * All outstanding requests (and associated buffers) must be > returned > - * back to the pipeline. The buffers would have been marked as > - * cancelled by the call to streamOff() earlier. > + * back to the pipeline. > */ > while (!requestQueue_.empty()) { > Request *request = requestQueue_.front(); > - /* > - * A request could be partially complete, > - * i.e. we have returned some buffers, but still waiting > - * for others or waiting for metadata. > - */ > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > > - FrameBuffer *buffer = request->findBuffer(stream); > - /* > - * Has the buffer already been handed back to the > - * request? If not, do so now. > - */ > - if (buffer && buffer->request()) > - pipe_->completeBuffer(request, buffer); > + for (auto &b : request->buffers()) { > + FrameBuffer *buffer = b.second; > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > } > > pipe_->completeRequest(request); > -- > 2.25.1 > >
Hi Naush Thanks for the tidy-up! I can't say I understand the details of this sufficiently to give a very meaningful review, however, I get the point that you can simply stop the streams without having to queue buffers back first. Nevertheless, I've been running with this for a while now with no ill effects, hence: Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote: > > With the addition of FrameBuffer::cancel(), the logic to clear and return > pending requests can be simplified by not having to queue all the request > buffers to the device before calling streamOff(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > 1 file changed, 8 insertions(+), 38 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a65b4568256c..887f8d0f7404 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > - /* This also stops the streams. */ > + for (auto const stream : data->streams_) > + stream->dev()->streamOff(); > + > data->clearIncompleteRequests(); > data->bayerQueue_ = {}; > data->embeddedQueue_ = {}; > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > void RPiCameraData::clearIncompleteRequests() > { > - /* > - * Queue up any buffers passed in the request. > - * This is needed because streamOff() will then mark the buffers as > - * cancelled. > - */ > - for (auto const request : requestQueue_) { > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > - > - FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer) > - stream->queueBuffer(buffer); > - } > - } > - > - /* Stop all streams. */ > - for (auto const stream : streams_) > - stream->dev()->streamOff(); > - > /* > * All outstanding requests (and associated buffers) must be returned > - * back to the pipeline. The buffers would have been marked as > - * cancelled by the call to streamOff() earlier. > + * back to the pipeline. > */ > while (!requestQueue_.empty()) { > Request *request = requestQueue_.front(); > - /* > - * A request could be partially complete, > - * i.e. we have returned some buffers, but still waiting > - * for others or waiting for metadata. > - */ > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > > - FrameBuffer *buffer = request->findBuffer(stream); > - /* > - * Has the buffer already been handed back to the > - * request? If not, do so now. > - */ > - if (buffer && buffer->request()) > - pipe_->completeBuffer(request, buffer); > + for (auto &b : request->buffers()) { > + FrameBuffer *buffer = b.second; > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > } > > pipe_->completeRequest(request); > -- > 2.25.1 >
Hi Naush, On 03/06/2021 13:43, Naushir Patuck wrote: > With the addition of FrameBuffer::cancel(), the logic to clear and return > pending requests can be simplified by not having to queue all the request > buffers to the device before calling streamOff(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > 1 file changed, 8 insertions(+), 38 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a65b4568256c..887f8d0f7404 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > - /* This also stops the streams. */ > + for (auto const stream : data->streams_) > + stream->dev()->streamOff(); That's nicer than hiding it in clearIncompleteRequests... > + > data->clearIncompleteRequests(); > data->bayerQueue_ = {}; > data->embeddedQueue_ = {}; > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) > > void RPiCameraData::clearIncompleteRequests() > { > - /* > - * Queue up any buffers passed in the request. > - * This is needed because streamOff() will then mark the buffers as > - * cancelled. > - */ > - for (auto const request : requestQueue_) { > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > - > - FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer) > - stream->queueBuffer(buffer); > - } > - } > - > - /* Stop all streams. */ > - for (auto const stream : streams_) > - stream->dev()->streamOff(); > - > /* > * All outstanding requests (and associated buffers) must be returned > - * back to the pipeline. The buffers would have been marked as > - * cancelled by the call to streamOff() earlier. > + * back to the pipeline. > */ > while (!requestQueue_.empty()) { > Request *request = requestQueue_.front(); > - /* > - * A request could be partially complete, > - * i.e. we have returned some buffers, but still waiting > - * for others or waiting for metadata. > - */ > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > > - FrameBuffer *buffer = request->findBuffer(stream); > - /* > - * Has the buffer already been handed back to the > - * request? If not, do so now. > - */ > - if (buffer && buffer->request()) > - pipe_->completeBuffer(request, buffer); > + for (auto &b : request->buffers()) { > + FrameBuffer *buffer = b.second; > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); Looks good to me. As far as I can tell, items on the requestQueue haven't been handed to any hardware yet, is that correct? (Which means there's no risk of any other action occurring from a stream on these buffers). And in fact, all streams are called with streamOff() before this anyway - so it should be good. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > pipe_->completeRequest(request); >
Hi all, Another gentle nudge. This needs one more R-b tag to get merged. Thanks, Naush On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote: > With the addition of FrameBuffer::cancel(), the logic to clear and return > pending requests can be simplified by not having to queue all the request > buffers to the device before calling streamOff(). > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > 1 file changed, 8 insertions(+), 38 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a65b4568256c..887f8d0f7404 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > /* Disable SOF event generation. */ > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > - /* This also stops the streams. */ > + for (auto const stream : data->streams_) > + stream->dev()->streamOff(); > + > data->clearIncompleteRequests(); > data->bayerQueue_ = {}; > data->embeddedQueue_ = {}; > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer > *buffer) > > void RPiCameraData::clearIncompleteRequests() > { > - /* > - * Queue up any buffers passed in the request. > - * This is needed because streamOff() will then mark the buffers as > - * cancelled. > - */ > - for (auto const request : requestQueue_) { > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > - > - FrameBuffer *buffer = request->findBuffer(stream); > - if (buffer) > - stream->queueBuffer(buffer); > - } > - } > - > - /* Stop all streams. */ > - for (auto const stream : streams_) > - stream->dev()->streamOff(); > - > /* > * All outstanding requests (and associated buffers) must be > returned > - * back to the pipeline. The buffers would have been marked as > - * cancelled by the call to streamOff() earlier. > + * back to the pipeline. > */ > while (!requestQueue_.empty()) { > Request *request = requestQueue_.front(); > - /* > - * A request could be partially complete, > - * i.e. we have returned some buffers, but still waiting > - * for others or waiting for metadata. > - */ > - for (auto const stream : streams_) { > - if (!stream->isExternal()) > - continue; > > - FrameBuffer *buffer = request->findBuffer(stream); > - /* > - * Has the buffer already been handed back to the > - * request? If not, do so now. > - */ > - if (buffer && buffer->request()) > - pipe_->completeBuffer(request, buffer); > + for (auto &b : request->buffers()) { > + FrameBuffer *buffer = b.second; > + buffer->cancel(); > + pipe_->completeBuffer(request, buffer); > } > > pipe_->completeRequest(request); > -- > 2.25.1 > >
Hi Naush, On Tue, Jun 22, 2021 at 09:36:43AM +0100, Naushir Patuck wrote: > Hi all, > > Another gentle nudge. This needs one more R-b tag to get merged. Sorry for being late. This looks good to me! One minor to be fixed when applying below > > Thanks, > Naush > > On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote: > > > With the addition of FrameBuffer::cancel(), the logic to clear and return > > pending requests can be simplified by not having to queue all the request > > buffers to the device before calling streamOff(). > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > > 1 file changed, 8 insertions(+), 38 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index a65b4568256c..887f8d0f7404 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > /* Disable SOF event generation. */ > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > > - /* This also stops the streams. */ > > + for (auto const stream : data->streams_) > > + stream->dev()->streamOff(); > > + > > data->clearIncompleteRequests(); > > data->bayerQueue_ = {}; > > data->embeddedQueue_ = {}; > > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer > > *buffer) > > > > void RPiCameraData::clearIncompleteRequests() > > { > > - /* > > - * Queue up any buffers passed in the request. > > - * This is needed because streamOff() will then mark the buffers as > > - * cancelled. > > - */ > > - for (auto const request : requestQueue_) { > > - for (auto const stream : streams_) { > > - if (!stream->isExternal()) > > - continue; > > - > > - FrameBuffer *buffer = request->findBuffer(stream); > > - if (buffer) > > - stream->queueBuffer(buffer); > > - } > > - } > > - > > - /* Stop all streams. */ > > - for (auto const stream : streams_) > > - stream->dev()->streamOff(); > > - > > /* > > * All outstanding requests (and associated buffers) must be > > returned > > - * back to the pipeline. The buffers would have been marked as > > - * cancelled by the call to streamOff() earlier. > > + * back to the pipeline. back to the 'camera' ? back to the 'application' ? we're actually in the pipeline here :) Apart from this: Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > */ > > while (!requestQueue_.empty()) { > > Request *request = requestQueue_.front(); > > - /* > > - * A request could be partially complete, > > - * i.e. we have returned some buffers, but still waiting > > - * for others or waiting for metadata. > > - */ > > - for (auto const stream : streams_) { > > - if (!stream->isExternal()) > > - continue; > > > > - FrameBuffer *buffer = request->findBuffer(stream); > > - /* > > - * Has the buffer already been handed back to the > > - * request? If not, do so now. > > - */ > > - if (buffer && buffer->request()) > > - pipe_->completeBuffer(request, buffer); > > + for (auto &b : request->buffers()) { > > + FrameBuffer *buffer = b.second; > > + buffer->cancel(); > > + pipe_->completeBuffer(request, buffer); > > } > > > > pipe_->completeRequest(request); > > -- > > 2.25.1 > > > >
Hi Jacopo, Thank you for your review. On Wed, 23 Jun 2021 at 14:20, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Naush, > > On Tue, Jun 22, 2021 at 09:36:43AM +0100, Naushir Patuck wrote: > > Hi all, > > > > Another gentle nudge. This needs one more R-b tag to get merged. > > Sorry for being late. > This looks good to me! One minor to be fixed when applying below > > > > > > Thanks, > > Naush > > > > On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > > With the addition of FrameBuffer::cancel(), the logic to clear and > return > > > pending requests can be simplified by not having to queue all the > request > > > buffers to the device before calling streamOff(). > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > > > 1 file changed, 8 insertions(+), 38 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index a65b4568256c..887f8d0f7404 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > > /* Disable SOF event generation. */ > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > > > > - /* This also stops the streams. */ > > > + for (auto const stream : data->streams_) > > > + stream->dev()->streamOff(); > > > + > > > data->clearIncompleteRequests(); > > > data->bayerQueue_ = {}; > > > data->embeddedQueue_ = {}; > > > @@ -1477,49 +1479,17 @@ void > RPiCameraData::ispOutputDequeue(FrameBuffer > > > *buffer) > > > > > > void RPiCameraData::clearIncompleteRequests() > > > { > > > - /* > > > - * Queue up any buffers passed in the request. > > > - * This is needed because streamOff() will then mark the > buffers as > > > - * cancelled. > > > - */ > > > - for (auto const request : requestQueue_) { > > > - for (auto const stream : streams_) { > > > - if (!stream->isExternal()) > > > - continue; > > > - > > > - FrameBuffer *buffer = > request->findBuffer(stream); > > > - if (buffer) > > > - stream->queueBuffer(buffer); > > > - } > > > - } > > > - > > > - /* Stop all streams. */ > > > - for (auto const stream : streams_) > > > - stream->dev()->streamOff(); > > > - > > > /* > > > * All outstanding requests (and associated buffers) must be > > > returned > > > - * back to the pipeline. The buffers would have been marked as > > > - * cancelled by the call to streamOff() earlier. > > > + * back to the pipeline. > > back to the 'camera' ? back to the 'application' ? we're actually in > the pipeline here :) > "back to the application" is probably the right term here. Would you be able to change this before submitting, or should I post an updated patch? Thanks! Naush > > Apart from this: > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > > */ > > > while (!requestQueue_.empty()) { > > > Request *request = requestQueue_.front(); > > > - /* > > > - * A request could be partially complete, > > > - * i.e. we have returned some buffers, but still > waiting > > > - * for others or waiting for metadata. > > > - */ > > > - for (auto const stream : streams_) { > > > - if (!stream->isExternal()) > > > - continue; > > > > > > - FrameBuffer *buffer = > request->findBuffer(stream); > > > - /* > > > - * Has the buffer already been handed back to > the > > > - * request? If not, do so now. > > > - */ > > > - if (buffer && buffer->request()) > > > - pipe_->completeBuffer(request, buffer); > > > + for (auto &b : request->buffers()) { > > > + FrameBuffer *buffer = b.second; > > > + buffer->cancel(); > > > + pipe_->completeBuffer(request, buffer); > > > } > > > > > > pipe_->completeRequest(request); > > > -- > > > 2.25.1 > > > > > > >
No worries, we'll fix when applying Thanks j On Wed, Jun 23, 2021 at 02:31:13PM +0100, Naushir Patuck wrote: > Hi Jacopo, > > Thank you for your review. > > On Wed, 23 Jun 2021 at 14:20, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hi Naush, > > > > On Tue, Jun 22, 2021 at 09:36:43AM +0100, Naushir Patuck wrote: > > > Hi all, > > > > > > Another gentle nudge. This needs one more R-b tag to get merged. > > > > Sorry for being late. > > This looks good to me! One minor to be fixed when applying below > > > > > > > > > > Thanks, > > > Naush > > > > > > On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> > > wrote: > > > > > > > With the addition of FrameBuffer::cancel(), the logic to clear and > > return > > > > pending requests can be simplified by not having to queue all the > > request > > > > buffers to the device before calling streamOff(). > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- > > > > 1 file changed, 8 insertions(+), 38 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index a65b4568256c..887f8d0f7404 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) > > > > /* Disable SOF event generation. */ > > > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); > > > > > > > > - /* This also stops the streams. */ > > > > + for (auto const stream : data->streams_) > > > > + stream->dev()->streamOff(); > > > > + > > > > data->clearIncompleteRequests(); > > > > data->bayerQueue_ = {}; > > > > data->embeddedQueue_ = {}; > > > > @@ -1477,49 +1479,17 @@ void > > RPiCameraData::ispOutputDequeue(FrameBuffer > > > > *buffer) > > > > > > > > void RPiCameraData::clearIncompleteRequests() > > > > { > > > > - /* > > > > - * Queue up any buffers passed in the request. > > > > - * This is needed because streamOff() will then mark the > > buffers as > > > > - * cancelled. > > > > - */ > > > > - for (auto const request : requestQueue_) { > > > > - for (auto const stream : streams_) { > > > > - if (!stream->isExternal()) > > > > - continue; > > > > - > > > > - FrameBuffer *buffer = > > request->findBuffer(stream); > > > > - if (buffer) > > > > - stream->queueBuffer(buffer); > > > > - } > > > > - } > > > > - > > > > - /* Stop all streams. */ > > > > - for (auto const stream : streams_) > > > > - stream->dev()->streamOff(); > > > > - > > > > /* > > > > * All outstanding requests (and associated buffers) must be > > > > returned > > > > - * back to the pipeline. The buffers would have been marked as > > > > - * cancelled by the call to streamOff() earlier. > > > > + * back to the pipeline. > > > > back to the 'camera' ? back to the 'application' ? we're actually in > > the pipeline here :) > > > > "back to the application" is probably the right term here. > Would you be able to change this before submitting, or should I post > an updated patch? > > Thanks! > Naush > > > > > > > Apart from this: > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Thanks > > j > > > > > > */ > > > > while (!requestQueue_.empty()) { > > > > Request *request = requestQueue_.front(); > > > > - /* > > > > - * A request could be partially complete, > > > > - * i.e. we have returned some buffers, but still > > waiting > > > > - * for others or waiting for metadata. > > > > - */ > > > > - for (auto const stream : streams_) { > > > > - if (!stream->isExternal()) > > > > - continue; > > > > > > > > - FrameBuffer *buffer = > > request->findBuffer(stream); > > > > - /* > > > > - * Has the buffer already been handed back to > > the > > > > - * request? If not, do so now. > > > > - */ > > > > - if (buffer && buffer->request()) > > > > - pipe_->completeBuffer(request, buffer); > > > > + for (auto &b : request->buffers()) { > > > > + FrameBuffer *buffer = b.second; > > > > + buffer->cancel(); > > > > + pipe_->completeBuffer(request, buffer); > > > > } > > > > > > > > pipe_->completeRequest(request); > > > > -- > > > > 2.25.1 > > > > > > > > > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index a65b4568256c..887f8d0f7404 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera) /* Disable SOF event generation. */ data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false); - /* This also stops the streams. */ + for (auto const stream : data->streams_) + stream->dev()->streamOff(); + data->clearIncompleteRequests(); data->bayerQueue_ = {}; data->embeddedQueue_ = {}; @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer) void RPiCameraData::clearIncompleteRequests() { - /* - * Queue up any buffers passed in the request. - * This is needed because streamOff() will then mark the buffers as - * cancelled. - */ - for (auto const request : requestQueue_) { - for (auto const stream : streams_) { - if (!stream->isExternal()) - continue; - - FrameBuffer *buffer = request->findBuffer(stream); - if (buffer) - stream->queueBuffer(buffer); - } - } - - /* Stop all streams. */ - for (auto const stream : streams_) - stream->dev()->streamOff(); - /* * All outstanding requests (and associated buffers) must be returned - * back to the pipeline. The buffers would have been marked as - * cancelled by the call to streamOff() earlier. + * back to the pipeline. */ while (!requestQueue_.empty()) { Request *request = requestQueue_.front(); - /* - * A request could be partially complete, - * i.e. we have returned some buffers, but still waiting - * for others or waiting for metadata. - */ - for (auto const stream : streams_) { - if (!stream->isExternal()) - continue; - FrameBuffer *buffer = request->findBuffer(stream); - /* - * Has the buffer already been handed back to the - * request? If not, do so now. - */ - if (buffer && buffer->request()) - pipe_->completeBuffer(request, buffer); + for (auto &b : request->buffers()) { + FrameBuffer *buffer = b.second; + buffer->cancel(); + pipe_->completeBuffer(request, buffer); } pipe_->completeRequest(request);
With the addition of FrameBuffer::cancel(), the logic to clear and return pending requests can be simplified by not having to queue all the request buffers to the device before calling streamOff(). Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 46 ++++--------------- 1 file changed, 8 insertions(+), 38 deletions(-)