| Message ID | 20240120211508.14742-1-mail@eliasnaur.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Elias, Thank you for the patch. On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > Vc4CameraData::findMatchBuffers may return successfully without setting > the embedded buffer. Make sure to initialize the buffer and id to avoid > accessing garbage data. How so ? The function starts with if (bayerQueue_.empty()) return false; /* * Find the embedded data buffer with a matching timestamp to pass to * the IPA. Any embedded buffers with a timestamp lower than the * current bayer buffer will be removed and re-queued to the driver. */ uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; embeddedBuffer = nullptr; so I don't see how it can leave embeddedBuffer unset if it returns successfully. > Without this change, libcamera v0.2.0 usually crashes with an assertion > error: > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() I've seen this indeed, and will test your patch. Maybe it's due to the second change, where you set params.buffers.embedded to 0 ? > Signed-off-by: Elias Naur <mail@eliasnaur.com> > --- > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index 26102ea7..d76389f3 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > void Vc4CameraData::tryRunPipeline() > { > - FrameBuffer *embeddedBuffer; > + FrameBuffer *embeddedBuffer = nullptr; > BayerFrame bayerFrame; > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > params.requestControls = request->controls(); > params.ipaContext = request->sequence(); > params.delayContext = bayerFrame.delayContext; > + params.buffers.embedded = 0; > > if (embeddedBuffer) { > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Elias, > > Thank you for the patch. > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > > Vc4CameraData::findMatchBuffers may return successfully without setting > > the embedded buffer. Make sure to initialize the buffer and id to avoid > > accessing garbage data. > > How so ? The function starts with > > if (bayerQueue_.empty()) > return false; > > /* > * Find the embedded data buffer with a matching timestamp to pass to > * the IPA. Any embedded buffers with a timestamp lower than the > * current bayer buffer will be removed and re-queued to the driver. > */ > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > embeddedBuffer = nullptr; > > so I don't see how it can leave embeddedBuffer unset if it returns > successfully. > > > Without this change, libcamera v0.2.0 usually crashes with an assertion > > error: > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() > > I've seen this indeed, and will test your patch. Maybe it's due to the > second change, where you set params.buffers.embedded to 0 ? > Correct: the fix is the second change; the first change is defensive in case findMatchBuffers for some reason doesn't set embeddedBuffer in case none is found. > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > --- > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > index 26102ea7..d76389f3 100644 > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > > > void Vc4CameraData::tryRunPipeline() > > { > > - FrameBuffer *embeddedBuffer; > > + FrameBuffer *embeddedBuffer = nullptr; > > BayerFrame bayerFrame; > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > > params.requestControls = request->controls(); > > params.ipaContext = request->sequence(); > > params.delayContext = bayerFrame.delayContext; > > + params.buffers.embedded = 0; > > > > if (embeddedBuffer) { > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > -- > Regards, > > Laurent Pinchart
Hi Elias, On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > > > Vc4CameraData::findMatchBuffers may return successfully without setting > > > the embedded buffer. Make sure to initialize the buffer and id to avoid > > > accessing garbage data. > > > > How so ? The function starts with > > > > if (bayerQueue_.empty()) > > return false; > > > > /* > > * Find the embedded data buffer with a matching timestamp to pass to > > * the IPA. Any embedded buffers with a timestamp lower than the > > * current bayer buffer will be removed and re-queued to the driver. > > */ > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > > embeddedBuffer = nullptr; > > > > so I don't see how it can leave embeddedBuffer unset if it returns > > successfully. > > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion > > > error: > > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() > > > > I've seen this indeed, and will test your patch. Maybe it's due to the > > second change, where you set params.buffers.embedded to 0 ? > > Correct: the fix is the second change; Could you please update the commit message to reflect this ? I read it as indicating that embeddedBuffer could be null, while the issue is that params.buffers.embedded is not initialized. > the first change is defensive in case > findMatchBuffers for some reason doesn't set embeddedBuffer in case none > is found. I'm not sure I'd keep that change. I'll let Naush and David decide what they like best. > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > > --- > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > index 26102ea7..d76389f3 100644 > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > > > > > void Vc4CameraData::tryRunPipeline() > > > { > > > - FrameBuffer *embeddedBuffer; > > > + FrameBuffer *embeddedBuffer = nullptr; > > > BayerFrame bayerFrame; > > > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > > > params.requestControls = request->controls(); > > > params.ipaContext = request->sequence(); > > > params.delayContext = bayerFrame.delayContext; > > > + params.buffers.embedded = 0; > > > > > > if (embeddedBuffer) { > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
Hi all, On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel < libcamera-devel@lists.libcamera.org> wrote: > > Hi Elias, > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > > > > Vc4CameraData::findMatchBuffers may return successfully without setting > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid > > > > accessing garbage data. > > > > > > How so ? The function starts with > > > > > > if (bayerQueue_.empty()) > > > return false; > > > > > > /* > > > * Find the embedded data buffer with a matching timestamp to pass to > > > * the IPA. Any embedded buffers with a timestamp lower than the > > > * current bayer buffer will be removed and re-queued to the driver. > > > */ > > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > > > embeddedBuffer = nullptr; > > > > > > so I don't see how it can leave embeddedBuffer unset if it returns > > > successfully. > > > > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion > > > > error: > > > > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() > > > > > > I've seen this indeed, and will test your patch. Maybe it's due to the > > > second change, where you set params.buffers.embedded to 0 ? > > > > Correct: the fix is the second change; > > Could you please update the commit message to reflect this ? I read it > as indicating that embeddedBuffer could be null, while the issue is that > params.buffers.embedded is not initialized. > > > the first change is defensive in case > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none > > is found. > > I'm not sure I'd keep that change. I'll let Naush and David decide what > they like best. I agree, let's just keep the change to initialising params.buffers.embedded. With this, the change itself looks reasonable so Reviewed-by: Naushir Patuck <naush@raspberrypi.com> However... I could have sworn the mojom auto generated class file for ipa::RPi::PrepareParams would initialize all member variables to 0 (including buffers.embedded) on default construction. I'm away right now so I cannot check this myself, but could anybody else check? I'm somewhat surprised that this has not been reported more widely... Thanks, Naush > > > > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > > > --- > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > index 26102ea7..d76389f3 100644 > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > > > > > > > void Vc4CameraData::tryRunPipeline() > > > > { > > > > - FrameBuffer *embeddedBuffer; > > > > + FrameBuffer *embeddedBuffer = nullptr; > > > > BayerFrame bayerFrame; > > > > > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > > > > params.requestControls = request->controls(); > > > > params.ipaContext = request->sequence(); > > > > params.delayContext = bayerFrame.delayContext; > > > > + params.buffers.embedded = 0; > > > > > > > > if (embeddedBuffer) { > > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > -- > Regards, > > Laurent Pinchart
On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote: > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote: > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > > > > > Vc4CameraData::findMatchBuffers may return successfully without setting > > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid > > > > > accessing garbage data. > > > > > > > > How so ? The function starts with > > > > > > > > if (bayerQueue_.empty()) > > > > return false; > > > > > > > > /* > > > > * Find the embedded data buffer with a matching timestamp to pass to > > > > * the IPA. Any embedded buffers with a timestamp lower than the > > > > * current bayer buffer will be removed and re-queued to the driver. > > > > */ > > > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > > > > embeddedBuffer = nullptr; > > > > > > > > so I don't see how it can leave embeddedBuffer unset if it returns > > > > successfully. > > > > > > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion > > > > > error: > > > > > > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() > > > > > > > > I've seen this indeed, and will test your patch. Maybe it's due to the > > > > second change, where you set params.buffers.embedded to 0 ? > > > > > > Correct: the fix is the second change; > > > > Could you please update the commit message to reflect this ? I read it > > as indicating that embeddedBuffer could be null, while the issue is that > > params.buffers.embedded is not initialized. > > > > > the first change is defensive in case > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none > > > is found. > > > > I'm not sure I'd keep that change. I'll let Naush and David decide what > > they like best. > > I agree, let's just keep the change to initialising params.buffers.embedded. > With this, the change itself looks reasonable so > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> Elias, would you be able to send a v2 ? > However... I could have sworn the mojom auto generated class file for > ipa::RPi::PrepareParams would initialize all member variables to 0 (including > buffers.embedded) on default construction. I'm away right now so I cannot > check this myself, but could anybody else check? I'm somewhat surprised that > this has not been reported more widely... I checked, and the constructor doesn't initialize all fields to 0. I'm pretty surprised too to be honest. > > > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > > > > --- > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > index 26102ea7..d76389f3 100644 > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > > > > > > > > > void Vc4CameraData::tryRunPipeline() > > > > > { > > > > > - FrameBuffer *embeddedBuffer; > > > > > + FrameBuffer *embeddedBuffer = nullptr; > > > > > BayerFrame bayerFrame; > > > > > > > > > > /* If any of our request or buffer queues are empty, we cannot proceed. */ > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > > > > > params.requestControls = request->controls(); > > > > > params.ipaContext = request->sequence(); > > > > > params.delayContext = bayerFrame.delayContext; > > > > > + params.buffers.embedded = 0; > > > > > > > > > > if (embeddedBuffer) { > > > > > unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
On Mon, 22 Jan 2024 at 05:31, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote: > > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote: > > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: > > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: > > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via libcamera-devel wrote: > > > > > > Vc4CameraData::findMatchBuffers may return successfully without setting > > > > > > the embedded buffer. Make sure to initialize the buffer and id to avoid > > > > > > accessing garbage data. > > > > > > > > > > How so ? The function starts with > > > > > > > > > > if (bayerQueue_.empty()) > > > > > return false; > > > > > > > > > > /* > > > > > * Find the embedded data buffer with a matching timestamp to pass to > > > > > * the IPA. Any embedded buffers with a timestamp lower than the > > > > > * current bayer buffer will be removed and re-queued to the driver. > > > > > */ > > > > > uint64_t ts = bayerQueue_.front().buffer->metadata().timestamp; > > > > > embeddedBuffer = nullptr; > > > > > > > > > > so I don't see how it can leave embeddedBuffer unset if it returns > > > > > successfully. > > > > > > > > > > > Without this change, libcamera v0.2.0 usually crashes with an assertion > > > > > > error: > > > > > > > > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() > > > > > > > > > > I've seen this indeed, and will test your patch. Maybe it's due to the > > > > > second change, where you set params.buffers.embedded to 0 ? > > > > > > > > Correct: the fix is the second change; > > > > > > Could you please update the commit message to reflect this ? I read it > > > as indicating that embeddedBuffer could be null, while the issue is that > > > params.buffers.embedded is not initialized. > > > > > > > the first change is defensive in case > > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case none > > > > is found. > > > > > > I'm not sure I'd keep that change. I'll let Naush and David decide what > > > they like best. > > I don't like uninitialized values in general, but I suppose fixing those is better done by, say, compiler flags. > > I agree, let's just keep the change to initialising params.buffers.embedded. > > With this, the change itself looks reasonable so > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > Elias, would you be able to send a v2 ? > Done. Elias
On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote: > > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote: > > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: > > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: > > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via > libcamera-devel wrote: > > > > > > Vc4CameraData::findMatchBuffers may return successfully without > setting > > > > > > the embedded buffer. Make sure to initialize the buffer and id > to avoid > > > > > > accessing garbage data. > > > > > > > > > > How so ? The function starts with > > > > > > > > > > if (bayerQueue_.empty()) > > > > > return false; > > > > > > > > > > /* > > > > > * Find the embedded data buffer with a matching timestamp > to pass to > > > > > * the IPA. Any embedded buffers with a timestamp lower > than the > > > > > * current bayer buffer will be removed and re-queued to > the driver. > > > > > */ > > > > > uint64_t ts = > bayerQueue_.front().buffer->metadata().timestamp; > > > > > embeddedBuffer = nullptr; > > > > > > > > > > so I don't see how it can leave embeddedBuffer unset if it returns > > > > > successfully. > > > > > > > > > > > Without this change, libcamera v0.2.0 usually crashes with an > assertion > > > > > > error: > > > > > > > > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in > prepareIsp() > > > > > > > > > > I've seen this indeed, and will test your patch. Maybe it's due to > the > > > > > second change, where you set params.buffers.embedded to 0 ? > > > > > > > > Correct: the fix is the second change; > > > > > > Could you please update the commit message to reflect this ? I read it > > > as indicating that embeddedBuffer could be null, while the issue is > that > > > params.buffers.embedded is not initialized. > > > > > > > the first change is defensive in case > > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case > none > > > > is found. > > > > > > I'm not sure I'd keep that change. I'll let Naush and David decide what > > > they like best. > > > > I agree, let's just keep the change to initialising > params.buffers.embedded. > > With this, the change itself looks reasonable so > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > Elias, would you be able to send a v2 ? > > > However... I could have sworn the mojom auto generated class file for > > ipa::RPi::PrepareParams would initialize all member variables to 0 > (including > > buffers.embedded) on default construction. I'm away right now so I > cannot > > check this myself, but could anybody else check? I'm somewhat surprised > that > > this has not been reported more widely... > > I checked, and the constructor doesn't initialize all fields to 0. I'm > pretty surprised too to be honest. > Curiosity got the better of me. In the https://git.libcamera.org/libcamera/libcamera.git/ build tree, the following bit of code gets generated in raspberrpyi_ipa_interface.h: --- struct BufferIds { public: #ifndef __DOXYGEN__ BufferIds() { } BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats) : bayer(_bayer), embedded(_embedded), stats(_stats) { } #endif uint32_t bayer; uint32_t embedded; uint32_t stats; }; --- However, building from https://github.com/raspberrypi/libcamera/ generates the following: --- struct BufferIds { public: #ifndef __DOXYGEN__ BufferIds() : bayer(0), embedded(0), stats(0) { } BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats) : bayer(_bayer), embedded(_embedded), stats(_stats) { } #endif uint32_t bayer; uint32_t embedded; uint32_t stats; }; --- 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. Regards, Naush > > > > > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> > > > > > > --- > > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- > > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > > index 26102ea7..d76389f3 100644 > > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > > > > > > @@ -906,7 +906,7 @@ void > Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) > > > > > > > > > > > > void Vc4CameraData::tryRunPipeline() > > > > > > { > > > > > > - FrameBuffer *embeddedBuffer; > > > > > > + FrameBuffer *embeddedBuffer = nullptr; > > > > > > BayerFrame bayerFrame; > > > > > > > > > > > > /* If any of our request or buffer queues are empty, we > cannot proceed. */ > > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() > > > > > > params.requestControls = request->controls(); > > > > > > params.ipaContext = request->sequence(); > > > > > > params.delayContext = bayerFrame.delayContext; > > > > > > + params.buffers.embedded = 0; > > > > > > > > > > > > if (embeddedBuffer) { > > > > > > unsigned int embeddedId = > unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); > > -- > Regards, > > Laurent Pinchart >
On Wed, 24 Jan 2024 at 09:17, Naushir Patuck <naush@raspberrypi.com> wrote: > > > On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart < > laurent.pinchart@ideasonboard.com> wrote: > >> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote: >> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel >> wrote: >> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote: >> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote: >> > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via >> libcamera-devel wrote: >> > > > > > Vc4CameraData::findMatchBuffers may return successfully without >> setting >> > > > > > the embedded buffer. Make sure to initialize the buffer and id >> to avoid >> > > > > > accessing garbage data. >> > > > > >> > > > > How so ? The function starts with >> > > > > >> > > > > if (bayerQueue_.empty()) >> > > > > return false; >> > > > > >> > > > > /* >> > > > > * Find the embedded data buffer with a matching >> timestamp to pass to >> > > > > * the IPA. Any embedded buffers with a timestamp lower >> than the >> > > > > * current bayer buffer will be removed and re-queued to >> the driver. >> > > > > */ >> > > > > uint64_t ts = >> bayerQueue_.front().buffer->metadata().timestamp; >> > > > > embeddedBuffer = nullptr; >> > > > > >> > > > > so I don't see how it can leave embeddedBuffer unset if it returns >> > > > > successfully. >> > > > > >> > > > > > Without this change, libcamera v0.2.0 usually crashes with an >> assertion >> > > > > > error: >> > > > > > >> > > > > > ipa_base.cpp:397 assertion "it != buffers_.end()" failed in >> prepareIsp() >> > > > > >> > > > > I've seen this indeed, and will test your patch. Maybe it's due >> to the >> > > > > second change, where you set params.buffers.embedded to 0 ? >> > > > >> > > > Correct: the fix is the second change; >> > > >> > > Could you please update the commit message to reflect this ? I read it >> > > as indicating that embeddedBuffer could be null, while the issue is >> that >> > > params.buffers.embedded is not initialized. >> > > >> > > > the first change is defensive in case >> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case >> none >> > > > is found. >> > > >> > > I'm not sure I'd keep that change. I'll let Naush and David decide >> what >> > > they like best. >> > >> > I agree, let's just keep the change to initialising >> params.buffers.embedded. >> > With this, the change itself looks reasonable so >> > >> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> >> >> Elias, would you be able to send a v2 ? >> >> > However... I could have sworn the mojom auto generated class file for >> > ipa::RPi::PrepareParams would initialize all member variables to 0 >> (including >> > buffers.embedded) on default construction. I'm away right now so I >> cannot >> > check this myself, but could anybody else check? I'm somewhat >> surprised that >> > this has not been reported more widely... >> >> I checked, and the constructor doesn't initialize all fields to 0. I'm >> pretty surprised too to be honest. >> > > Curiosity got the better of me. In the > https://git.libcamera.org/libcamera/libcamera.git/ build tree, > the following bit of code gets generated in raspberrpyi_ipa_interface.h: > > --- > struct BufferIds > { > public: > #ifndef __DOXYGEN__ > BufferIds() > { > } > > BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats) > : bayer(_bayer), embedded(_embedded), stats(_stats) > { > } > #endif > > uint32_t bayer; > uint32_t embedded; > uint32_t stats; > }; > --- > > However, building from https://github.com/raspberrypi/libcamera/ > generates the following: > > --- > struct BufferIds > { > public: > #ifndef __DOXYGEN__ > BufferIds() > : bayer(0), embedded(0), stats(0) > { > } > > BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats) > : bayer(_bayer), embedded(_embedded), stats(_stats) > { > } > #endif > > uint32_t bayer; > uint32_t embedded; > uint32_t stats; > }; > --- > > 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. > And that points to the commit: commit d17de86904f03f1d5a4d5d20af518e70c4758969 Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Date: Thu Jan 4 17:15:48 2024 +0200 utils: ipc: Update mojo Update mojo from commit 9be4263648d7d1a04bb78be75df53f56449a5e3a "Updating trunk VERSION from 6225.0 to 6226.0" from the Chromium repository. The update-mojo.sh script was used for this update. Bug: https://bugs.libcamera.org/show_bug.cgi?id=206 Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Milan Zamazal <mzamazal@redhat.com> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Regards, > Naush > > > > >> >> > > > > > Signed-off-by: Elias Naur <mail@eliasnaur.com> >> > > > > > --- >> > > > > > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- >> > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) >> > > > > > >> > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> > > > > > index 26102ea7..d76389f3 100644 >> > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> > > > > > @@ -906,7 +906,7 @@ void >> Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) >> > > > > > >> > > > > > void Vc4CameraData::tryRunPipeline() >> > > > > > { >> > > > > > - FrameBuffer *embeddedBuffer; >> > > > > > + FrameBuffer *embeddedBuffer = nullptr; >> > > > > > BayerFrame bayerFrame; >> > > > > > >> > > > > > /* If any of our request or buffer queues are empty, we >> cannot proceed. */ >> > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() >> > > > > > params.requestControls = request->controls(); >> > > > > > params.ipaContext = request->sequence(); >> > > > > > params.delayContext = bayerFrame.delayContext; >> > > > > > + params.buffers.embedded = 0; >> > > > > > >> > > > > > if (embeddedBuffer) { >> > > > > > unsigned int embeddedId = >> unicam_[Unicam::Embedded].getBufferId(embeddedBuffer); >> >> -- >> Regards, >> >> Laurent Pinchart >> >
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index 26102ea7..d76389f3 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -906,7 +906,7 @@ void Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs) void Vc4CameraData::tryRunPipeline() { - FrameBuffer *embeddedBuffer; + FrameBuffer *embeddedBuffer = nullptr; BayerFrame bayerFrame; /* If any of our request or buffer queues are empty, we cannot proceed. */ @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline() params.requestControls = request->controls(); params.ipaContext = request->sequence(); params.delayContext = bayerFrame.delayContext; + params.buffers.embedded = 0; if (embeddedBuffer) { unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
Vc4CameraData::findMatchBuffers may return successfully without setting the embedded buffer. Make sure to initialize the buffer and id to avoid accessing garbage data. Without this change, libcamera v0.2.0 usually crashes with an assertion error: ipa_base.cpp:397 assertion "it != buffers_.end()" failed in prepareIsp() Signed-off-by: Elias Naur <mail@eliasnaur.com> --- src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)