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

[Jacopo Mondi]

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