| Message ID | 20211005054414.873711-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thanks for the patch ! On 05/10/2021 07:44, Hirokazu Honda wrote: > This enables ipu3 pipeline handler to apply the test pattern mode > per frame. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++++++++++++++-- > 1 file changed, 36 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 92e86925..07fda65f 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -69,6 +69,7 @@ public: > void statBufferReady(FrameBuffer *buffer); > void queuePendingRequests(); > void cancelPendingRequests(); > + void frameStart(uint32_t sequence); > > CIO2Device cio2_; > ImgUDevice *imgu_; > @@ -88,6 +89,7 @@ public: > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; > > std::queue<Request *> pendingRequests_; > + std::queue<Request *> processingRequests_; > > ControlInfoMap ipaControls_; > > @@ -568,6 +570,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > cio2->sensor()->sensorInfo(&sensorInfo); > data->cropRegion_ = sensorInfo.analogCrop; > > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff); There is a dependency on "[PATCH] libcamera: camera_sensor: Enable to set a test pattern mode" (20211005043723.568685-1-hiroh@chromium.org), right ? Could you make it a series please :-) ? > + > /* > * Configure the H/V flip controls based on the combination of > * the sensor and user transform. > @@ -801,6 +805,9 @@ void PipelineHandlerIPU3::stop(Camera *camera) > > ret |= data->imgu_->stop(); > ret |= data->cio2_.stop(); > + > + data->processingRequests_ = {}; Shouldn't we do that in cancelPendingRequests() as a mirror of the push which is done in queuePendingRequests() ? I am a bit worry that it is done after cio2_->stop()... > + > if (ret) > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > > @@ -851,6 +858,8 @@ void IPU3CameraData::queuePendingRequests() > > info->rawBuffer = rawBuffer; > > + processingRequests_.push(request); > + > ipa::ipu3::IPU3Event ev; > ev.op = ipa::ipu3::EventProcessControls; > ev.frame = info->id; > @@ -1107,8 +1116,8 @@ int PipelineHandlerIPU3::registerCameras() > data->delayedCtrls_ = > std::make_unique<DelayedControls>(cio2->sensor()->device(), > params); > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(), > - &DelayedControls::applyControls); > + data->cio2_.frameStart().connect(data.get(), > + &IPU3CameraData::frameStart); The IPA will receive the ipa::ipu3::EventProcessControls immediately, so it could be notified to stop trying to apply the algorithm for instance (to be discussed). I think this is good :-). > > /* Convert the sensor rotation to a transformation */ > int32_t rotation = 0; > @@ -1399,6 +1408,31 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > ipa_->processEvent(ev); > } > > +void IPU3CameraData::frameStart(uint32_t sequence) > +{ > + if (!processingRequests_.empty()) { Add a LOG(IPU3, Debug) here ? > + /* Assumes applying the test pattern mode affects immediately. */ > + Request *request = processingRequests_.front(); > + processingRequests_.pop(); > + > + if (request->controls().contains(controls::draft::TestPatternMode)) { Same here, to see which test pattern is applied ? > + const int32_t testPatternMode = request->controls().get( > + controls::draft::TestPatternMode); > + > + int ret = cio2_.sensor()->setTestPatternMode(testPatternMode); > + if (ret) { > + LOG(IPU3, Error) << "Failed to set test pattern mode: " > + << ret; > + } else { > + request->metadata().set(controls::draft::TestPatternMode, > + testPatternMode); > + } > + } > + } > + And a small comment for this line ? > + delayedCtrls_->applyControls(sequence); > +} > + > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) > > } /* namespace libcamera */ > Now, this patch makes the per-frame control on the IPA point of view more important... How will we handle those ? This is still an open question... Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Hi Jean-Michel, On Mon, Oct 25, 2021 at 3:30 PM Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> wrote: > > Hi Hiro, > > Thanks for the patch ! > > On 05/10/2021 07:44, Hirokazu Honda wrote: > > This enables ipu3 pipeline handler to apply the test pattern mode > > per frame. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++++++++++++++-- > > 1 file changed, 36 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 92e86925..07fda65f 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -69,6 +69,7 @@ public: > > void statBufferReady(FrameBuffer *buffer); > > void queuePendingRequests(); > > void cancelPendingRequests(); > > + void frameStart(uint32_t sequence); > > > > CIO2Device cio2_; > > ImgUDevice *imgu_; > > @@ -88,6 +89,7 @@ public: > > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; > > > > std::queue<Request *> pendingRequests_; > > + std::queue<Request *> processingRequests_; > > > > ControlInfoMap ipaControls_; > > > > @@ -568,6 +570,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > > cio2->sensor()->sensorInfo(&sensorInfo); > > data->cropRegion_ = sensorInfo.analogCrop; > > > > + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff); > > There is a dependency on "[PATCH] libcamera: camera_sensor: Enable to > set a test pattern mode" (20211005043723.568685-1-hiroh@chromium.org), > right ? > Could you make it a series please :-) ? > > > + > > /* > > * Configure the H/V flip controls based on the combination of > > * the sensor and user transform. > > @@ -801,6 +805,9 @@ void PipelineHandlerIPU3::stop(Camera *camera) > > > > ret |= data->imgu_->stop(); > > ret |= data->cio2_.stop(); > > + > > + data->processingRequests_ = {}; > > Shouldn't we do that in cancelPendingRequests() as a mirror of the push > which is done in queuePendingRequests() ? > > I am a bit worry that it is done after cio2_->stop()... > > > + > > if (ret) > > LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); > > > > @@ -851,6 +858,8 @@ void IPU3CameraData::queuePendingRequests() > > > > info->rawBuffer = rawBuffer; > > > > + processingRequests_.push(request); > > + > > ipa::ipu3::IPU3Event ev; > > ev.op = ipa::ipu3::EventProcessControls; > > ev.frame = info->id; > > @@ -1107,8 +1116,8 @@ int PipelineHandlerIPU3::registerCameras() > > data->delayedCtrls_ = > > std::make_unique<DelayedControls>(cio2->sensor()->device(), > > params); > > - data->cio2_.frameStart().connect(data->delayedCtrls_.get(), > > - &DelayedControls::applyControls); > > + data->cio2_.frameStart().connect(data.get(), > > + &IPU3CameraData::frameStart); > > The IPA will receive the ipa::ipu3::EventProcessControls immediately, so > it could be notified to stop trying to apply the algorithm for instance > (to be discussed). I think this is good :-). > > > > > /* Convert the sensor rotation to a transformation */ > > int32_t rotation = 0; > > @@ -1399,6 +1408,31 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > > ipa_->processEvent(ev); > > } > > > > +void IPU3CameraData::frameStart(uint32_t sequence) > > +{ > > + if (!processingRequests_.empty()) { > > Add a LOG(IPU3, Debug) here ? > I would not do it because processingReqeusts_ is often not empty and therefore adding LOG here is verbose. Thanks, -Hiro > > + /* Assumes applying the test pattern mode affects immediately. */ > > + Request *request = processingRequests_.front(); > > + processingRequests_.pop(); > > + > > + if (request->controls().contains(controls::draft::TestPatternMode)) { > > Same here, to see which test pattern is applied ? > > > + const int32_t testPatternMode = request->controls().get( > > + controls::draft::TestPatternMode); > > + > > + int ret = cio2_.sensor()->setTestPatternMode(testPatternMode); > > + if (ret) { > > + LOG(IPU3, Error) << "Failed to set test pattern mode: " > > + << ret; > > + } else { > > + request->metadata().set(controls::draft::TestPatternMode, > > + testPatternMode); > > + } > > + } > > + } > > + > > And a small comment for this line ? > > > + delayedCtrls_->applyControls(sequence); > > +} > > + > > REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) > > > > } /* namespace libcamera */ > > > > Now, this patch makes the per-frame control on the IPA point of view > more important... How will we handle those ? This is still an open > question... > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 92e86925..07fda65f 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -69,6 +69,7 @@ public: void statBufferReady(FrameBuffer *buffer); void queuePendingRequests(); void cancelPendingRequests(); + void frameStart(uint32_t sequence); CIO2Device cio2_; ImgUDevice *imgu_; @@ -88,6 +89,7 @@ public: std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; std::queue<Request *> pendingRequests_; + std::queue<Request *> processingRequests_; ControlInfoMap ipaControls_; @@ -568,6 +570,8 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) cio2->sensor()->sensorInfo(&sensorInfo); data->cropRegion_ = sensorInfo.analogCrop; + cio2->sensor()->setTestPatternMode(controls::draft::TestPatternModeOff); + /* * Configure the H/V flip controls based on the combination of * the sensor and user transform. @@ -801,6 +805,9 @@ void PipelineHandlerIPU3::stop(Camera *camera) ret |= data->imgu_->stop(); ret |= data->cio2_.stop(); + + data->processingRequests_ = {}; + if (ret) LOG(IPU3, Warning) << "Failed to stop camera " << camera->id(); @@ -851,6 +858,8 @@ void IPU3CameraData::queuePendingRequests() info->rawBuffer = rawBuffer; + processingRequests_.push(request); + ipa::ipu3::IPU3Event ev; ev.op = ipa::ipu3::EventProcessControls; ev.frame = info->id; @@ -1107,8 +1116,8 @@ int PipelineHandlerIPU3::registerCameras() data->delayedCtrls_ = std::make_unique<DelayedControls>(cio2->sensor()->device(), params); - data->cio2_.frameStart().connect(data->delayedCtrls_.get(), - &DelayedControls::applyControls); + data->cio2_.frameStart().connect(data.get(), + &IPU3CameraData::frameStart); /* Convert the sensor rotation to a transformation */ int32_t rotation = 0; @@ -1399,6 +1408,31 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) ipa_->processEvent(ev); } +void IPU3CameraData::frameStart(uint32_t sequence) +{ + if (!processingRequests_.empty()) { + /* Assumes applying the test pattern mode affects immediately. */ + Request *request = processingRequests_.front(); + processingRequests_.pop(); + + if (request->controls().contains(controls::draft::TestPatternMode)) { + const int32_t testPatternMode = request->controls().get( + controls::draft::TestPatternMode); + + int ret = cio2_.sensor()->setTestPatternMode(testPatternMode); + if (ret) { + LOG(IPU3, Error) << "Failed to set test pattern mode: " + << ret; + } else { + request->metadata().set(controls::draft::TestPatternMode, + testPatternMode); + } + } + } + + delayedCtrls_->applyControls(sequence); +} + REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3) } /* namespace libcamera */
This enables ipu3 pipeline handler to apply the test pattern mode per frame. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 38 ++++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-)