| Message ID | 20211028100319.1097720-2-hanlinchen@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Han-Lin Chen (2021-10-28 11:03:17) > The Intel close sourced IPA requires the effective controls applied to > the sensor when the stastistics are generated. Report effective sensor controls > with the stastistics to IPA. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> > --- > src/libcamera/pipeline/ipu3/frames.h | 3 +++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++++ > 2 files changed, 8 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > index 3ef7e445..a897e307 100644 > --- a/src/libcamera/pipeline/ipu3/frames.h > +++ b/src/libcamera/pipeline/ipu3/frames.h > @@ -12,6 +12,7 @@ > #include <queue> > #include <vector> > > +#include <libcamera/controls.h> > #include <libcamera/base/signal.h> > > namespace libcamera { > @@ -34,6 +35,8 @@ public: > FrameBuffer *paramBuffer; > FrameBuffer *statBuffer; > > + ControlList effectiveSensorControls; > + > bool paramDequeued; > bool metadataProcessed; > }; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 8816efc5..6a7f5b9a 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -667,6 +667,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > + data->delayedCtrls_->reset(); > + > return updateControls(data); > } > > @@ -1363,6 +1365,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > > + info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > + Are the delayedControls and the metadata sequence numbers directly correlated? Or are they both just sequentially increasing sequences... I see we're resetting the delayed controls above, and that makes me worry that this 'works' by resetting the delayed controls, and assumeing that the capture device will always give the same sequence numbers. But I'm not sure they always do that. .. I'm sure I've seen some continue sequences after start stops. Hopefully the V4L2 API will specify what is correct here. > if (request->findBuffer(&rawStream_)) > pipe()->completeBuffer(request, buffer); > > @@ -1419,6 +1423,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > ev.frame = info->id; > ev.bufferId = info->statBuffer->cookie(); > ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp); > + ev.sensorControls = info->effectiveSensorControls; > ipa_->processEvent(ev); > } > > -- > 2.33.1.1089.g2158813163f-goog >
Hi Kieran, Thanks for the review. On Thu, Oct 28, 2021 at 9:34 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Han-Lin Chen (2021-10-28 11:03:17) > > The Intel close sourced IPA requires the effective controls applied to > > the sensor when the stastistics are generated. Report effective sensor controls > > with the stastistics to IPA. > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> > > --- > > src/libcamera/pipeline/ipu3/frames.h | 3 +++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++++ > > 2 files changed, 8 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h > > index 3ef7e445..a897e307 100644 > > --- a/src/libcamera/pipeline/ipu3/frames.h > > +++ b/src/libcamera/pipeline/ipu3/frames.h > > @@ -12,6 +12,7 @@ > > #include <queue> > > #include <vector> > > > > +#include <libcamera/controls.h> > > #include <libcamera/base/signal.h> > > > > namespace libcamera { > > @@ -34,6 +35,8 @@ public: > > FrameBuffer *paramBuffer; > > FrameBuffer *statBuffer; > > > > + ControlList effectiveSensorControls; > > + > > bool paramDequeued; > > bool metadataProcessed; > > }; > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 8816efc5..6a7f5b9a 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -667,6 +667,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > return ret; > > } > > > > + data->delayedCtrls_->reset(); > > + > > return updateControls(data); > > } > > > > @@ -1363,6 +1365,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > request->metadata().set(controls::SensorTimestamp, > > buffer->metadata().timestamp); > > > > + info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > > + > > Are the delayedControls and the metadata sequence numbers directly > correlated? Or are they both just sequentially increasing sequences... > > I see we're resetting the delayed controls above, and that makes me > worry that this 'works' by resetting the delayed controls, and assumeing > that the capture device will always give the same sequence numbers. But > I'm not sure they always do that. .. I'm sure I've seen some continue > sequences after start stops. The DelayedControl uses the first sequence after reset() as the "first_sequence" and uses the diff between it and the current sequence as index to it's internal ring buffer. I suppose it's designed this way, so it works as long as the following sequence is increasing. The V4L2 API seems not to specify whether the sequence should be reset after stops, but it should be increasing after start. This is why I suppose resetting it before start is a safer bet. Another reason is to clear the internal ring buffer in the DelayedControls. > > Hopefully the V4L2 API will specify what is correct here. > > > > if (request->findBuffer(&rawStream_)) > > pipe()->completeBuffer(request, buffer); > > > > @@ -1419,6 +1423,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > > ev.frame = info->id; > > ev.bufferId = info->statBuffer->cookie(); > > ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp); > > + ev.sensorControls = info->effectiveSensorControls; > > ipa_->processEvent(ev); > > } > > > > -- > > 2.33.1.1089.g2158813163f-goog > >
diff --git a/src/libcamera/pipeline/ipu3/frames.h b/src/libcamera/pipeline/ipu3/frames.h index 3ef7e445..a897e307 100644 --- a/src/libcamera/pipeline/ipu3/frames.h +++ b/src/libcamera/pipeline/ipu3/frames.h @@ -12,6 +12,7 @@ #include <queue> #include <vector> +#include <libcamera/controls.h> #include <libcamera/base/signal.h> namespace libcamera { @@ -34,6 +35,8 @@ public: FrameBuffer *paramBuffer; FrameBuffer *statBuffer; + ControlList effectiveSensorControls; + bool paramDequeued; bool metadataProcessed; }; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 8816efc5..6a7f5b9a 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -667,6 +667,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; } + data->delayedCtrls_->reset(); + return updateControls(data); } @@ -1363,6 +1365,8 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); + info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); + if (request->findBuffer(&rawStream_)) pipe()->completeBuffer(request, buffer); @@ -1419,6 +1423,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) ev.frame = info->id; ev.bufferId = info->statBuffer->cookie(); ev.frameTimestamp = request->metadata().get(controls::SensorTimestamp); + ev.sensorControls = info->effectiveSensorControls; ipa_->processEvent(ev); }
The Intel close sourced IPA requires the effective controls applied to the sensor when the stastistics are generated. Report effective sensor controls with the stastistics to IPA. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> --- src/libcamera/pipeline/ipu3/frames.h | 3 +++ src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++++ 2 files changed, 8 insertions(+)