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

[Naushir Patuck]

Hi Jacopo,

Thank you for your review.

On Wed, 23 Jun 2021 at 14:20, Jacopo Mondi <jacopo at 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 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 :)
>

"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 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
> > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210623/99a9611a/attachment-0001.htm>