<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 21 Mar 2022 at 11:55, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</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">Quoting Kieran Bingham (2022-03-21 11:51:37)<br>
> Quoting Naushir Patuck via libcamera-devel (2022-03-21 10:27:01)<br>
> > The original use of RPi::Stream::reset() was to clear the external flag state<br>
> > and free/clear out the framebuffers for the stream. However, the latter is now<br>
> > done through PipelineHandlerRPi::configure(). Rework<br>
> > PipelineHandlerRPi::configure() to call RPi::Stream::setExternal() instead of<br>
> > RPi::Stream::reset() to achieve the same thing.<br>
> > <br>
> > Repurpose RPi::Stream::reset() to instead reset the state of the buffer handling<br>
> > logic, where all internally allocated buffers are put back into the queue of<br>
> > available buffers. As such, rename the function to RPi::Stream::resetbuffers()<br>
> > This resetbuffers() is now called from PipelineHandlerRPi::start(), allowing the<br>
> <br>
> /resetbuffers/resetBuffers/ twice above. But could be handled while<br>
> applying.<br>
> <br>
> <br>
> > pipeline handler to correctly deal with start()/stop()/start() sequences and<br>
> > reusing the buffers allocated on the first start().<br>
> > <br>
> > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > Tested-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > ---<br>
> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++--<br>
> > src/libcamera/pipeline/raspberrypi/rpi_stream.cpp | 13 ++++++-------<br>
> > src/libcamera/pipeline/raspberrypi/rpi_stream.h | 2 +-<br>
> > 3 files changed, 12 insertions(+), 10 deletions(-)<br>
> > <br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > index 92043962494b..0ff6acdceb66 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > @@ -693,7 +693,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)<br>
> > /* Start by freeing all buffers and reset the Unicam and ISP stream states. */<br>
> > data->freeBuffers();<br>
> > for (auto const stream : data->streams_)<br>
> > - stream->reset();<br>
> > + stream->setExternal(false);<br>
> > <br>
> > BayerFormat::Packing packing = BayerFormat::Packing::CSI2;<br>
> > Size maxSize, sensorSize;<br>
> > @@ -991,6 +991,9 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> > RPiCameraData *data = cameraData(camera);<br>
> > int ret;<br>
> > <br>
> > + for (auto const stream : data->streams_)<br>
> > + stream->resetBuffers();<br>
> > +<br>
> > if (!data->buffersAllocated_) {<br>
> > /* Allocate buffers for internal pipeline usage. */<br>
> > ret = prepareBuffers(camera);<br>
> > @@ -1031,7 +1034,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
> > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);<br>
> > <br>
> > /*<br>
> > - * Reset the delayed controls with the gain and exposure values set by<br>
> > + * resetBuffers the delayed controls with the gain and exposure values set by<br>
> <br>
> Possibly a little too much of a match on the find/replace?<br>
> <br>
> Could easily be dropped while applying.<br></blockquote><div><br></div><div><div>:-(</div><div><br></div><div>Sorry, my brain is functioning like it's still Sunday this morning. I'll fix this up and</div><div>post an update.</div><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>
> <br>
> > * the IPA.<br>
> > */<br>
> > data->delayedCtrls_->reset();<br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > index f446e1ce66c7..7a93efaa29da 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp<br>
> > @@ -26,10 +26,12 @@ std::string Stream::name() const<br>
> > return name_;<br>
> > }<br>
> > <br>
> > -void Stream::reset()<br>
> > +void Stream::resetBuffers()<br>
> > {<br>
> > - external_ = false;<br>
> > - clearBuffers();<br>
> > + /* Add all internal buffers to the queue of usable buffers. */<br>
> > + availableBuffers_ = {};<br>
> <br>
> I think this is ok, but I get a bit worried when I see blanket buffer<br>
> state changes like that.<br>
> <br>
> > + for (auto const &buffer : internalBuffers_)<br>
> > + availableBuffers_.push(buffer.get());<br>
> <br>
> Is there any other action that is required to update the state of the<br>
> buffer if it was currently in use when this gets called? (Currently I<br>
> think the only call site is below so it's fine).<br>
<br>
Aha, it's not the only call site, there's the one up in start() as well<br>
of course.<br>
<br>
So at start() time, is there any possiblity that some buffer isn't<br>
already in availableBuffers_ ? and if not ... why not ?<br></blockquote><div><br></div><div>At start time, all internal buffers would have been allocated, so any/every</div><div>buffer will be listed in availableBuffers_.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Is there some worry about buffer state on calls to stop() not moving<br>
buffers into the availableBuffers_ list?<br></blockquote><div><br></div><div>I think this is ok, as the resetBuffers() call on start() will (as it says on the tin)</div><div>reset all buffer states and mark them as available again, if that makes sense.</div><div><br></div><div>Regards,</div><div>Naush</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>
--<br>
Kieran<br>
<br>
<br>
> With the above fixes (that can be fixed while applying if suitable)<br>
> <br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> > }<br>
> > <br>
> > void Stream::setExternal(bool external)<br>
> > @@ -97,10 +99,7 @@ int Stream::prepareBuffers(unsigned int count)<br>
> > <br>
> > /* Add these exported buffers to the internal/external buffer list. */<br>
> > setExportedBuffers(&internalBuffers_);<br>
> > -<br>
> > - /* Add these buffers to the queue of internal usable buffers. */<br>
> > - for (auto const &buffer : internalBuffers_)<br>
> > - availableBuffers_.push(buffer.get());<br>
> > + resetBuffers();<br>
> > }<br>
> > <br>
> > /* We must import all internal/external exported buffers. */<br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h<br>
> > index d6f49d34f8c8..c37f7e82eef6 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h<br>
> > @@ -45,7 +45,7 @@ public:<br>
> > V4L2VideoDevice *dev() const;<br>
> > std::string name() const;<br>
> > bool isImporter() const;<br>
> > - void reset();<br>
> > + void resetBuffers();<br>
> > <br>
> > void setExternal(bool external);<br>
> > bool isExternal() const;<br>
> > -- <br>
> > 2.25.1<br>
> ><br>
</blockquote></div></div>