[libcamera-devel] [PATCH] pipeline: raspberrypi: Simplify RPiCameraData::clearIncompleteRequests()

[Jacopo Mondi]

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