<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@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">On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:<br>
> On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:<br>
> > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:<br>
> > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:<br>
> > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote:<br>
> > > > > Vc4CameraData::findMatchBuffers may return successfully without setting<br>
> > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid<br>
> > > > > accessing garbage data.<br>
> > > ><br>
> > > > How so ? The function starts with<br>
> > > ><br>
> > > > if (bayerQueue_.empty())<br>
> > > > return false;<br>
> > > ><br>
> > > > /*<br>
> > > > * Find the embedded data buffer with a matching timestamp to pass to<br>
> > > > * the IPA. Any embedded buffers with a timestamp lower than the<br>
> > > > * current bayer buffer will be removed and re-queued to the driver.<br>
> > > > */<br>
> > > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp;<br>
> > > > embeddedBuffer = nullptr;<br>
> > > ><br>
> > > > so I don't see how it can leave embeddedBuffer unset if it returns<br>
> > > > successfully.<br>
> > > ><br>
> > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion<br>
> > > > > error:<br>
> > > > ><br>
> > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp()<br>
> > > ><br>
> > > > I've seen this indeed, and will test your patch. Maybe it's due to the<br>
> > > > second change, where you set params.buffers.embedded to 0 ?<br>
> > ><br>
> > > Correct: the fix is the second change;<br>
> ><br>
> > Could you please update the commit message to reflect this ? I read it<br>
> > as indicating that embeddedBuffer could be null, while the issue is that<br>
> > params.buffers.embedded is not initialized.<br>
> ><br>
> > > the first change is defensive in case<br>
> > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none<br>
> > > is found.<br>
> ><br>
> > I'm not sure I'd keep that change. I'll let Naush and David decide what<br>
> > they like best.<br>
> <br>
> I agree, let's just keep the change to initialising params.buffers.embedded.<br>
> With this, the change itself looks reasonable so<br>
> <br>
> Reviewed-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
<br>
Elias, would you be able to send a v2 ?<br>
<br>
> However... I could have sworn the mojom auto generated class file for<br>
> ipa::RPi::PrepareParams would initialize all member variables to 0 (including<br>
> buffers.embedded) on default construction. I'm away right now so I cannot<br>
> check this myself, but could anybody else check? I'm somewhat surprised that<br>
> this has not been reported more widely...<br>
<br>
I checked, and the constructor doesn't initialize all fields to 0. I'm<br>
pretty surprised too to be honest.<br></blockquote><div><br></div><div>Curiosity got the better of me. In the <a href="https://git.libcamera.org/libcamera/libcamera.git/">https://git.libcamera.org/libcamera/libcamera.git/</a> build tree,</div><div>the following bit of code gets generated in raspberrpyi_ipa_interface.h:</div><div><br></div><div>---</div><div>struct BufferIds<br>{<br>public:<br>#ifndef __DOXYGEN__<br> BufferIds()<br> {<br> }<br><br> BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)<br> : bayer(_bayer), embedded(_embedded), stats(_stats)<br> {<br> }</div><div>#endif<br><br> uint32_t bayer;<br> uint32_t embedded;<br> uint32_t stats;<br>};<br></div><div>---</div><div><br></div><div>However, building from <a href="https://github.com/raspberrypi/libcamera/">https://github.com/raspberrypi/libcamera/</a> generates the following:</div><div><br></div><div>---</div><div>struct BufferIds<br>{<br>public:<br>#ifndef __DOXYGEN__<br> BufferIds()<br> : bayer(0), embedded(0), stats(0)<br> {<br> }<br><br> BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)<br> : bayer(_bayer), embedded(_embedded), stats(_stats)<br> {<br> }<br>#endif<br><br> uint32_t bayer;<br> uint32_t embedded;<br> uint32_t stats;<br>};<br></div><div>---</div><div><br></div><div>So at some point in time, the fields were explicitly initialised to 0 and things were working as expected. The RPi tree definitely does not have any changes to the mojom generation code, so I can't say what caused this change, but it should hopefully be easy enough to bisect and find the root case.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></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>
> > > > > Signed-off-by: Elias Naur <<a href="mailto:mail@eliasnaur.com" target="_blank">mail@eliasnaur.com</a>><br>
> > > > > ---<br>
> > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-<br>
> > > > > 1 file changed, 2 insertions(+), 1 deletion(-)<br>
> > > > ><br>
> > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> > > > > index 26102ea7..d76389f3 100644<br>
> > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp<br>
> > > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)<br>
> > > > ><br>
> > > > > void Vc4CameraData::tryRunPipeline()<br>
> > > > > {<br>
> > > > > - FrameBuffer *embeddedBuffer;<br>
> > > > > + FrameBuffer *embeddedBuffer = nullptr;<br>
> > > > > BayerFrame bayerFrame;<br>
> > > > ><br>
> > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */<br>
> > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()<br>
> > > > > params.requestControls = request->controls();<br>
> > > > > params.ipaContext = request->sequence();<br>
> > > > > params.delayContext = bayerFrame.delayContext;<br>
> > > > > + params.buffers.embedded = 0;<br>
> > > > ><br>
> > > > > if (embeddedBuffer) {<br>
> > > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>