Message ID | 20241220162724.756494-5-stefan.klug@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Stefan, Thank you for the patch. The subject line is incorrect, this patch isn't about the pipeline handler but the IPA module. On Fri, Dec 20, 2024 at 05:26:50PM +0100, Stefan Klug wrote: > IPARkISP1::setControls() is called when new sensor controls shall be > queued in the pipeline handler. It constructs a list of sensor controls > and then emits the setSensorControls signal. > > To be able to return initial sensor controls from the IPARkISP1::start() > function, similar functionality will be needed. Prepare for that by > changing the setControls() function to a getSensorControls() and by moving > the setSensorControls.emit() out of the function. > > This patch contains no functional changes. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 2ffdd99b158a..45a539a61fb1 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -76,7 +76,7 @@ private: > void updateControls(const IPACameraSensorInfo &sensorInfo, > const ControlInfoMap &sensorControls, > ControlInfoMap *ipaControls); > - void setControls(unsigned int frame); > + ControlList getSensorControls(unsigned int frame); > > std::map<unsigned int, FrameBuffer> buffers_; > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > @@ -211,7 +211,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > int IPARkISP1::start() > { > - setControls(0); > + ControlList ctrls = getSensorControls(0); Will this work fine before patch 5/7 ? Or at least with no regression (such as crashes) ? > + setSensorControls.emit(0, ctrls); > > return 0; > } > @@ -378,7 +379,12 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > algo->process(context_, frame, frameContext, stats, metadata); > } > > - setControls(frame); > + /* > + * \todo: Here we should do a lookahead that takes the sensor delays The doxygen tag is '\todo', not '\todo:'. > + * into account. > + */ I'm not sure how accurate that is, but I suppose it's not worse than the existing comment. I'd keep the mention of frame number though. > + ControlList ctrls = getSensorControls(frame); > + setSensorControls.emit(frame, ctrls); > > context_.debugMetadata.moveEntries(metadata); > metadataReady.emit(frame, metadata); > @@ -443,13 +449,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > } > > -void IPARkISP1::setControls(unsigned int frame) > +ControlList IPARkISP1::getSensorControls(unsigned int frame) > { > - /* > - * \todo The frame number is most likely wrong here, we need to take > - * internal sensor delays and other timing parameters into account. > - */ > - > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > uint32_t exposure = frameContext.agc.exposure; > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > @@ -457,8 +458,7 @@ void IPARkISP1::setControls(unsigned int frame) > ControlList ctrls(sensorControls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > - > - setSensorControls.emit(frame, ctrls); > + return ctrls; > } > > } /* namespace ipa::rkisp1 */
On Mon, Jan 13, 2025 at 12:09:18AM +0200, Laurent Pinchart wrote: > Hi Stefan, > > Thank you for the patch. > > The subject line is incorrect, this patch isn't about the pipeline > handler but the IPA module. > > On Fri, Dec 20, 2024 at 05:26:50PM +0100, Stefan Klug wrote: > > IPARkISP1::setControls() is called when new sensor controls shall be > > queued in the pipeline handler. It constructs a list of sensor controls > > and then emits the setSensorControls signal. > > > > To be able to return initial sensor controls from the IPARkISP1::start() > > function, similar functionality will be needed. Prepare for that by > > changing the setControls() function to a getSensorControls() and by moving > > the setSensorControls.emit() out of the function. > > > > This patch contains no functional changes. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++----------- > > 1 file changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 2ffdd99b158a..45a539a61fb1 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -76,7 +76,7 @@ private: > > void updateControls(const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > ControlInfoMap *ipaControls); > > - void setControls(unsigned int frame); > > + ControlList getSensorControls(unsigned int frame); > > > > std::map<unsigned int, FrameBuffer> buffers_; > > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > @@ -211,7 +211,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > > > int IPARkISP1::start() > > { > > - setControls(0); > > + ControlList ctrls = getSensorControls(0); > > Will this work fine before patch 5/7 ? Or at least with no regression > (such as crashes) ? This doesn't change functionality (when combined with the line below) so I expect it to be fine. Paul > > > + setSensorControls.emit(0, ctrls); > > > > return 0; > > } > > @@ -378,7 +379,12 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > > algo->process(context_, frame, frameContext, stats, metadata); > > } > > > > - setControls(frame); > > + /* > > + * \todo: Here we should do a lookahead that takes the sensor delays > > The doxygen tag is '\todo', not '\todo:'. > > > + * into account. > > + */ > > I'm not sure how accurate that is, but I suppose it's not worse than the > existing comment. I'd keep the mention of frame number though. > > > + ControlList ctrls = getSensorControls(frame); > > + setSensorControls.emit(frame, ctrls); > > > > context_.debugMetadata.moveEntries(metadata); > > metadataReady.emit(frame, metadata); > > @@ -443,13 +449,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > } > > > > -void IPARkISP1::setControls(unsigned int frame) > > +ControlList IPARkISP1::getSensorControls(unsigned int frame) > > { > > - /* > > - * \todo The frame number is most likely wrong here, we need to take > > - * internal sensor delays and other timing parameters into account. > > - */ > > - > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > uint32_t exposure = frameContext.agc.exposure; > > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > @@ -457,8 +458,7 @@ void IPARkISP1::setControls(unsigned int frame) > > ControlList ctrls(sensorControls_); > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > - > > - setSensorControls.emit(frame, ctrls); > > + return ctrls; > > } > > > > } /* namespace ipa::rkisp1 */ > > -- > Regards, > > Laurent Pinchart
On Fri, Dec 20, 2024 at 05:26:50PM +0100, Stefan Klug wrote: > IPARkISP1::setControls() is called when new sensor controls shall be > queued in the pipeline handler. It constructs a list of sensor controls > and then emits the setSensorControls signal. > > To be able to return initial sensor controls from the IPARkISP1::start() > function, similar functionality will be needed. Prepare for that by > changing the setControls() function to a getSensorControls() and by moving > the setSensorControls.emit() out of the function. > > This patch contains no functional changes. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> With Laurent's comments addressed, Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 2ffdd99b158a..45a539a61fb1 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -76,7 +76,7 @@ private: > void updateControls(const IPACameraSensorInfo &sensorInfo, > const ControlInfoMap &sensorControls, > ControlInfoMap *ipaControls); > - void setControls(unsigned int frame); > + ControlList getSensorControls(unsigned int frame); > > std::map<unsigned int, FrameBuffer> buffers_; > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > @@ -211,7 +211,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > int IPARkISP1::start() > { > - setControls(0); > + ControlList ctrls = getSensorControls(0); > + setSensorControls.emit(0, ctrls); > > return 0; > } > @@ -378,7 +379,12 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, > algo->process(context_, frame, frameContext, stats, metadata); > } > > - setControls(frame); > + /* > + * \todo: Here we should do a lookahead that takes the sensor delays > + * into account. > + */ > + ControlList ctrls = getSensorControls(frame); > + setSensorControls.emit(frame, ctrls); > > context_.debugMetadata.moveEntries(metadata); > metadataReady.emit(frame, metadata); > @@ -443,13 +449,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > } > > -void IPARkISP1::setControls(unsigned int frame) > +ControlList IPARkISP1::getSensorControls(unsigned int frame) > { > - /* > - * \todo The frame number is most likely wrong here, we need to take > - * internal sensor delays and other timing parameters into account. > - */ > - > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > uint32_t exposure = frameContext.agc.exposure; > uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > @@ -457,8 +458,7 @@ void IPARkISP1::setControls(unsigned int frame) > ControlList ctrls(sensorControls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > - > - setSensorControls.emit(frame, ctrls); > + return ctrls; > } > > } /* namespace ipa::rkisp1 */ > -- > 2.43.0 >
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 2ffdd99b158a..45a539a61fb1 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -76,7 +76,7 @@ private: void updateControls(const IPACameraSensorInfo &sensorInfo, const ControlInfoMap &sensorControls, ControlInfoMap *ipaControls); - void setControls(unsigned int frame); + ControlList getSensorControls(unsigned int frame); std::map<unsigned int, FrameBuffer> buffers_; std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; @@ -211,7 +211,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, int IPARkISP1::start() { - setControls(0); + ControlList ctrls = getSensorControls(0); + setSensorControls.emit(0, ctrls); return 0; } @@ -378,7 +379,12 @@ void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId, algo->process(context_, frame, frameContext, stats, metadata); } - setControls(frame); + /* + * \todo: Here we should do a lookahead that takes the sensor delays + * into account. + */ + ControlList ctrls = getSensorControls(frame); + setSensorControls.emit(frame, ctrls); context_.debugMetadata.moveEntries(metadata); metadataReady.emit(frame, metadata); @@ -443,13 +449,8 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); } -void IPARkISP1::setControls(unsigned int frame) +ControlList IPARkISP1::getSensorControls(unsigned int frame) { - /* - * \todo The frame number is most likely wrong here, we need to take - * internal sensor delays and other timing parameters into account. - */ - IPAFrameContext &frameContext = context_.frameContexts.get(frame); uint32_t exposure = frameContext.agc.exposure; uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); @@ -457,8 +458,7 @@ void IPARkISP1::setControls(unsigned int frame) ControlList ctrls(sensorControls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); - - setSensorControls.emit(frame, ctrls); + return ctrls; } } /* namespace ipa::rkisp1 */
IPARkISP1::setControls() is called when new sensor controls shall be queued in the pipeline handler. It constructs a list of sensor controls and then emits the setSensorControls signal. To be able to return initial sensor controls from the IPARkISP1::start() function, similar functionality will be needed. Prepare for that by changing the setControls() function to a getSensorControls() and by moving the setSensorControls.emit() out of the function. This patch contains no functional changes. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/rkisp1.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)