| Message ID | 20210512084744.1499469-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | f1b7b68d209aa2fb074f39be7ca0d7e2c9edd631 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch! On Wed, 12 May 2021 at 09:47, Naushir Patuck <naush@raspberrypi.com> wrote: > > Write the controls::SensorTimestamp value in the Request metadata when > the request is popped from the queue ready to run the pipeline. This > ensures that the timestamp is written to the correct Request item, > which may not be at the top of the queue when the Unicam buffer dequeue > occurs. > > Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++-------- > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 6fbdba0487bf..eb6d31670567 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -221,6 +221,8 @@ public: > > private: > void checkRequestCompleted(); > + void fillRequestMetadata(const ControlList &bufferControls, > + Request *request); > void tryRunPipeline(); > bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer); > > @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > if (stream == &unicam_[Unicam::Image]) { > - /* > - * Record the sensor timestamp in the Request. > - * > - * \todo Do not assume the request in the front of the queue > - * is the correct one > - */ > - Request *request = requestQueue_.front(); > - ASSERT(request); > - > - request->metadata().set(controls::SensorTimestamp, > - buffer->metadata().timestamp); > - Indeed, I've actually been seeing segmentation faults at this line, maybe ~10% of the time when running with the ov5647. I've applied these patches and set the system running in a loop - all seems to be good now! > /* > * Lookup the sensor controls used for this frame sequence from > * DelayedControl and queue them along with the frame buffer. > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls) > } > } > > +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, > + Request *request) > +{ > + request->metadata().set(controls::SensorTimestamp, > + bufferControls.get(controls::SensorTimestamp)); > +} I did wonder why this is worth a whole new function, but I see the next commit adds something more to it! > + > void RPiCameraData::tryRunPipeline() > { > FrameBuffer *embeddedBuffer; > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline() > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > + /* > + * Clear the request metadata and fill it with some initial non-IPA > + * related controls. We clear it first because the request metadata > + * may have been populated if we have dropped the previous frame. > + */ > + request->metadata().clear(); Ah yes, thank you. That's been most annoying since the ControlList::merge function has started spitting out warnings! Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > + fillRequestMetadata(bayerFrame.controls, request); > + > /* > * Process all the user controls by the IPA. Once this is complete, we > * queue the ISP output buffer listed in the request to start the HW > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi, Gentle ping to get some feedback on this patch series. It fixes a possible segfault that has been reported, and would be nice to get in soon. Regards, Naush On Wed, 12 May 2021 at 09:47, Naushir Patuck <naush@raspberrypi.com> wrote: > Write the controls::SensorTimestamp value in the Request metadata when > the request is popped from the queue ready to run the pipeline. This > ensures that the timestamp is written to the correct Request item, > which may not be at the top of the queue when the Unicam buffer dequeue > occurs. > > Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++-------- > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 6fbdba0487bf..eb6d31670567 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -221,6 +221,8 @@ public: > > private: > void checkRequestCompleted(); > + void fillRequestMetadata(const ControlList &bufferControls, > + Request *request); > void tryRunPipeline(); > bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer > *&embeddedBuffer); > > @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > *buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > if (stream == &unicam_[Unicam::Image]) { > - /* > - * Record the sensor timestamp in the Request. > - * > - * \todo Do not assume the request in the front of the > queue > - * is the correct one > - */ > - Request *request = requestQueue_.front(); > - ASSERT(request); > - > - request->metadata().set(controls::SensorTimestamp, > - buffer->metadata().timestamp); > - > /* > * Lookup the sensor controls used for this frame sequence > from > * DelayedControl and queue them along with the frame > buffer. > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const > ControlList &controls) > } > } > > +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, > + Request *request) > +{ > + request->metadata().set(controls::SensorTimestamp, > + > bufferControls.get(controls::SensorTimestamp)); > +} > + > void RPiCameraData::tryRunPipeline() > { > FrameBuffer *embeddedBuffer; > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline() > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > + /* > + * Clear the request metadata and fill it with some initial non-IPA > + * related controls. We clear it first because the request metadata > + * may have been populated if we have dropped the previous frame. > + */ > + request->metadata().clear(); > + fillRequestMetadata(bayerFrame.controls, request); > + > /* > * Process all the user controls by the IPA. Once this is > complete, we > * queue the ISP output buffer listed in the request to start the > HW > -- > 2.25.1 > >
Hi Naush, On 12/05/2021 09:47, Naushir Patuck wrote: > Write the controls::SensorTimestamp value in the Request metadata when > the request is popped from the queue ready to run the pipeline. This > ensures that the timestamp is written to the correct Request item, > which may not be at the top of the queue when the Unicam buffer dequeue > occurs. > > Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++-------- > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 6fbdba0487bf..eb6d31670567 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -221,6 +221,8 @@ public: > > private: > void checkRequestCompleted(); > + void fillRequestMetadata(const ControlList &bufferControls, > + Request *request); > void tryRunPipeline(); > bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer); > > @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > if (stream == &unicam_[Unicam::Image]) { > - /* > - * Record the sensor timestamp in the Request. > - * > - * \todo Do not assume the request in the front of the queue > - * is the correct one > - */ > - Request *request = requestQueue_.front(); > - ASSERT(request); > - > - request->metadata().set(controls::SensorTimestamp, > - buffer->metadata().timestamp); > - > /* > * Lookup the sensor controls used for this frame sequence from > * DelayedControl and queue them along with the frame buffer. > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls) > } > } > > +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, > + Request *request) > +{ > + request->metadata().set(controls::SensorTimestamp, > + bufferControls.get(controls::SensorTimestamp)); > +} > + > void RPiCameraData::tryRunPipeline() > { > FrameBuffer *embeddedBuffer; > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline() > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > + /* > + * Clear the request metadata and fill it with some initial non-IPA > + * related controls. We clear it first because the request metadata > + * may have been populated if we have dropped the previous frame. > + */ Aha, I was going to say is this the right place to clear this, and then I re-read that comment and now I understand why it's here. So I think this is fine. Is there anything else that would have to be cleared down if a request gets 're-used' internally? I suspect not at the moment, but it may be that we need a specific call on a request to clean it up more generically. This should be fine though Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Hrm, thinking about it more, i think this comes down to the ControlList->merge() doesn't it ... if the entry is already there it won't be updated? I almost feel like in this case - it should be updated, because it's newer and more correct information... > + request->metadata().clear(); > + fillRequestMetadata(bayerFrame.controls, request); > + > /* > * Process all the user controls by the IPA. Once this is complete, we > * queue the ISP output buffer listed in the request to start the HW >
Hi Kieran, Thank you for your feedback. On Mon, 17 May 2021 at 13:40, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Naush, > > On 12/05/2021 09:47, Naushir Patuck wrote: > > Write the controls::SensorTimestamp value in the Request metadata when > > the request is popped from the queue ready to run the pipeline. This > > ensures that the timestamp is written to the correct Request item, > > which may not be at the top of the queue when the Unicam buffer dequeue > > occurs. > > > > Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++-------- > > 1 file changed, 17 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 6fbdba0487bf..eb6d31670567 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -221,6 +221,8 @@ public: > > > > private: > > void checkRequestCompleted(); > > + void fillRequestMetadata(const ControlList &bufferControls, > > + Request *request); > > void tryRunPipeline(); > > bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer > *&embeddedBuffer); > > > > @@ -1416,18 +1418,6 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > << ", timestamp: " << buffer->metadata().timestamp; > > > > if (stream == &unicam_[Unicam::Image]) { > > - /* > > - * Record the sensor timestamp in the Request. > > - * > > - * \todo Do not assume the request in the front of the > queue > > - * is the correct one > > - */ > > - Request *request = requestQueue_.front(); > > - ASSERT(request); > > - > > - request->metadata().set(controls::SensorTimestamp, > > - buffer->metadata().timestamp); > > - > > /* > > * Lookup the sensor controls used for this frame sequence > from > > * DelayedControl and queue them along with the frame > buffer. > > @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const > ControlList &controls) > > } > > } > > > > +void RPiCameraData::fillRequestMetadata(const ControlList > &bufferControls, > > + Request *request) > > +{ > > + request->metadata().set(controls::SensorTimestamp, > > + > bufferControls.get(controls::SensorTimestamp)); > > +} > > + > > void RPiCameraData::tryRunPipeline() > > { > > FrameBuffer *embeddedBuffer; > > @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline() > > /* See if a new ScalerCrop value needs to be applied. */ > > applyScalerCrop(request->controls()); > > > > + /* > > + * Clear the request metadata and fill it with some initial non-IPA > > + * related controls. We clear it first because the request metadata > > + * may have been populated if we have dropped the previous frame. > > + */ > > Aha, I was going to say is this the right place to clear this, and then > I re-read that comment and now I understand why it's here. > > So I think this is fine. > > Is there anything else that would have to be cleared down if a request > gets 're-used' internally? > I think that's it.... for now :-) > > I suspect not at the moment, but it may be that we need a specific call > on a request to clean it up more generically. > > This should be fine though > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Hrm, thinking about it more, i think this comes down to the > ControlList->merge() doesn't it ... if the entry is already there it > won't be updated? I almost feel like in this case - it should be > updated, because it's newer and more correct information... > Correct. It's a consequence of doing the ControlList::merge() now. I think ControlList::merge() follows the convention of std::map::merge() where it does not merge if duplicate keys exist between the two maps. Regards, Naush > > > > > + request->metadata().clear(); > > + fillRequestMetadata(bayerFrame.controls, request); > > + > > /* > > * Process all the user controls by the IPA. Once this is > complete, we > > * queue the ISP output buffer listed in the request to start the > HW > > > > -- > Regards > -- > Kieran >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 6fbdba0487bf..eb6d31670567 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -221,6 +221,8 @@ public: private: void checkRequestCompleted(); + void fillRequestMetadata(const ControlList &bufferControls, + Request *request); void tryRunPipeline(); bool findMatchingBuffers(BayerFrame &bayerFrame, FrameBuffer *&embeddedBuffer); @@ -1416,18 +1418,6 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) << ", timestamp: " << buffer->metadata().timestamp; if (stream == &unicam_[Unicam::Image]) { - /* - * Record the sensor timestamp in the Request. - * - * \todo Do not assume the request in the front of the queue - * is the correct one - */ - Request *request = requestQueue_.front(); - ASSERT(request); - - request->metadata().set(controls::SensorTimestamp, - buffer->metadata().timestamp); - /* * Lookup the sensor controls used for this frame sequence from * DelayedControl and queue them along with the frame buffer. @@ -1689,6 +1679,13 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls) } } +void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, + Request *request) +{ + request->metadata().set(controls::SensorTimestamp, + bufferControls.get(controls::SensorTimestamp)); +} + void RPiCameraData::tryRunPipeline() { FrameBuffer *embeddedBuffer; @@ -1708,6 +1705,14 @@ void RPiCameraData::tryRunPipeline() /* See if a new ScalerCrop value needs to be applied. */ applyScalerCrop(request->controls()); + /* + * Clear the request metadata and fill it with some initial non-IPA + * related controls. We clear it first because the request metadata + * may have been populated if we have dropped the previous frame. + */ + request->metadata().clear(); + fillRequestMetadata(bayerFrame.controls, request); + /* * Process all the user controls by the IPA. Once this is complete, we * queue the ISP output buffer listed in the request to start the HW
Write the controls::SensorTimestamp value in the Request metadata when the request is popped from the queue ready to run the pipeline. This ensures that the timestamp is written to the correct Request item, which may not be at the top of the queue when the Unicam buffer dequeue occurs. Fixes: fcfb1dc02a6b ("libcamera: raspberry: Report sensor timestamp") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-)