<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 23 Jun 2021 at 14:20, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Tue, Jun 22, 2021 at 09:36:43AM +0100, Naushir Patuck wrote:<br>
> Hi all,<br>
><br>
> Another gentle nudge.  This needs one more R-b tag to get merged.<br>
<br>
Sorry for being late.<br>
This looks good to me! One minor to be fixed when applying below<br>
<br>
<br>
><br>
> Thanks,<br>
> Naush<br>
><br>
> On Thu, 3 Jun 2021 at 13:43, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> > With the addition of FrameBuffer::cancel(), the logic to clear and return<br>
> > pending requests can be simplified by not having to queue all the request<br>
> > buffers to the device before calling streamOff().<br>
> ><br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > ---<br>
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 46 ++++---------------<br>
> >  1 file changed, 8 insertions(+), 38 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > index a65b4568256c..887f8d0f7404 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > @@ -884,7 +884,9 @@ void PipelineHandlerRPi::stop(Camera *camera)<br>
> >         /* Disable SOF event generation. */<br>
> >         data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(false);<br>
> ><br>
> > -       /* This also stops the streams. */<br>
> > +       for (auto const stream : data->streams_)<br>
> > +               stream->dev()->streamOff();<br>
> > +<br>
> >         data->clearIncompleteRequests();<br>
> >         data->bayerQueue_ = {};<br>
> >         data->embeddedQueue_ = {};<br>
> > @@ -1477,49 +1479,17 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer<br>
> > *buffer)<br>
> ><br>
> >  void RPiCameraData::clearIncompleteRequests()<br>
> >  {<br>
> > -       /*<br>
> > -        * Queue up any buffers passed in the request.<br>
> > -        * This is needed because streamOff() will then mark the buffers as<br>
> > -        * cancelled.<br>
> > -        */<br>
> > -       for (auto const request : requestQueue_) {<br>
> > -               for (auto const stream : streams_) {<br>
> > -                       if (!stream->isExternal())<br>
> > -                               continue;<br>
> > -<br>
> > -                       FrameBuffer *buffer = request->findBuffer(stream);<br>
> > -                       if (buffer)<br>
> > -                               stream->queueBuffer(buffer);<br>
> > -               }<br>
> > -       }<br>
> > -<br>
> > -       /* Stop all streams. */<br>
> > -       for (auto const stream : streams_)<br>
> > -               stream->dev()->streamOff();<br>
> > -<br>
> >         /*<br>
> >          * All outstanding requests (and associated buffers) must be<br>
> > returned<br>
> > -        * back to the pipeline. The buffers would have been marked as<br>
> > -        * cancelled by the call to streamOff() earlier.<br>
> > +        * back to the pipeline.<br>
<br>
back to the 'camera' ? back to the 'application' ? we're actually in<br>
the pipeline here :)<br></blockquote><div><br></div><div>"back to the application" is probably the right term here.</div><div>Would you be able to change this before submitting, or should I post</div><div>an updated patch?</div><div><br></div><div>Thanks!</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Apart from this:<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
   j<br>
<br>
> >          */<br>
> >         while (!requestQueue_.empty()) {<br>
> >                 Request *request = requestQueue_.front();<br>
> > -               /*<br>
> > -                * A request could be partially complete,<br>
> > -                * i.e. we have returned some buffers, but still waiting<br>
> > -                * for others or waiting for metadata.<br>
> > -                */<br>
> > -               for (auto const stream : streams_) {<br>
> > -                       if (!stream->isExternal())<br>
> > -                               continue;<br>
> ><br>
> > -                       FrameBuffer *buffer = request->findBuffer(stream);<br>
> > -                       /*<br>
> > -                        * Has the buffer already been handed back to the<br>
> > -                        * request? If not, do so now.<br>
> > -                        */<br>
> > -                       if (buffer && buffer->request())<br>
> > -                               pipe_->completeBuffer(request, buffer);<br>
> > +               for (auto &b : request->buffers()) {<br>
> > +                       FrameBuffer *buffer = b.second;<br>
> > +                       buffer->cancel();<br>
> > +                       pipe_->completeBuffer(request, buffer);<br>
> >                 }<br>
> ><br>
> >                 pipe_->completeRequest(request);<br>
> > --<br>
> > 2.25.1<br>
> ><br>
> ><br>
</blockquote></div></div>