| Message ID | 20220331163058.171418-3-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. s/Inlink/Inline/ in the subject line (same for 3/6). It looks like I wrote "inlinking" instead of "inlining" in my review of v3, sorry about that :-) On Thu, Mar 31, 2022 at 10:00:54PM +0530, Umang Jain via libcamera-devel wrote: > Since we have moved away from switch/case on the operation ID, > there's little reason to split the operation in two functions. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 47 +++++++++++++++++-------------------------- > 1 file changed, 18 insertions(+), 29 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 7779ad58..23a9033e 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -157,7 +157,6 @@ private: > ControlInfoMap *ipaControls); > void updateSessionConfiguration(const ControlInfoMap &sensorControls); > > - void fillParams(unsigned int frame, ipu3_uapi_params *params); > void parseStatistics(unsigned int frame, > int64_t frameTimestamp, > const ipu3_uapi_stats_3a *stats); > @@ -514,6 +513,9 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids) > * \brief Fill and return a buffer with ISP processing parameters for a frame > * \param[in] frame The frame number > * \param[in] bufferId ID of the parameter buffer to fill > + * > + * Algorithms are expected to fill the IPU3 parameter buffer for the next > + * frame given their most recent processing of the ImgU statistics. > */ > void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > { > @@ -527,7 +529,21 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) > ipu3_uapi_params *params = > reinterpret_cast<ipu3_uapi_params *>(mem.data()); > > - fillParams(frame, params); > + /* > + * The incoming params buffer may contain uninitialised data, or the > + * parameters of previously queued frames. Clearing the entire buffer > + * may be an expensive operation, and the kernel will only read from > + * structures which have their associated use-flag set. > + * > + * It is the responsibility of the algorithms to set the use flags > + * accordingly for any data structure they update during prepare(). > + */ > + params->use = {}; > + > + for (auto const &algo : algorithms_) > + algo->prepare(context_, params); > + > + paramsBufferReady.emit(frame); > } > > /** > @@ -570,33 +586,6 @@ void IPAIPU3::queueRequest(const uint32_t frame, > /* \todo Start processing for 'frame' based on 'controls'. */ > } > > -/** > - * \brief Fill the ImgU parameter buffer for the next frame > - * \param[in] frame The number of the latest frame processed > - * \param[out] params The parameter buffer to fill > - * > - * Algorithms are expected to fill the IPU3 parameter buffer for the next > - * frame given their most recent processing of the ImgU statistics. > - */ > -void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) > -{ > - /* > - * The incoming params buffer may contain uninitialised data, or the > - * parameters of previously queued frames. Clearing the entire buffer > - * may be an expensive operation, and the kernel will only read from > - * structures which have their associated use-flag set. > - * > - * It is the responsibility of the algorithms to set the use flags > - * accordingly for any data structure they update during prepare(). > - */ > - params->use = {}; > - > - for (auto const &algo : algorithms_) > - algo->prepare(context_, params); > - > - paramsBufferReady.emit(frame); > -} > - > /** > * \brief Process the statistics generated by the ImgU > * \param[in] frame The number of the latest frame processed
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 7779ad58..23a9033e 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -157,7 +157,6 @@ private: ControlInfoMap *ipaControls); void updateSessionConfiguration(const ControlInfoMap &sensorControls); - void fillParams(unsigned int frame, ipu3_uapi_params *params); void parseStatistics(unsigned int frame, int64_t frameTimestamp, const ipu3_uapi_stats_3a *stats); @@ -514,6 +513,9 @@ void IPAIPU3::unmapBuffers(const std::vector<unsigned int> &ids) * \brief Fill and return a buffer with ISP processing parameters for a frame * \param[in] frame The frame number * \param[in] bufferId ID of the parameter buffer to fill + * + * Algorithms are expected to fill the IPU3 parameter buffer for the next + * frame given their most recent processing of the ImgU statistics. */ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) { @@ -527,7 +529,21 @@ void IPAIPU3::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) ipu3_uapi_params *params = reinterpret_cast<ipu3_uapi_params *>(mem.data()); - fillParams(frame, params); + /* + * The incoming params buffer may contain uninitialised data, or the + * parameters of previously queued frames. Clearing the entire buffer + * may be an expensive operation, and the kernel will only read from + * structures which have their associated use-flag set. + * + * It is the responsibility of the algorithms to set the use flags + * accordingly for any data structure they update during prepare(). + */ + params->use = {}; + + for (auto const &algo : algorithms_) + algo->prepare(context_, params); + + paramsBufferReady.emit(frame); } /** @@ -570,33 +586,6 @@ void IPAIPU3::queueRequest(const uint32_t frame, /* \todo Start processing for 'frame' based on 'controls'. */ } -/** - * \brief Fill the ImgU parameter buffer for the next frame - * \param[in] frame The number of the latest frame processed - * \param[out] params The parameter buffer to fill - * - * Algorithms are expected to fill the IPU3 parameter buffer for the next - * frame given their most recent processing of the ImgU statistics. - */ -void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params) -{ - /* - * The incoming params buffer may contain uninitialised data, or the - * parameters of previously queued frames. Clearing the entire buffer - * may be an expensive operation, and the kernel will only read from - * structures which have their associated use-flag set. - * - * It is the responsibility of the algorithms to set the use flags - * accordingly for any data structure they update during prepare(). - */ - params->use = {}; - - for (auto const &algo : algorithms_) - algo->prepare(context_, params); - - paramsBufferReady.emit(frame); -} - /** * \brief Process the statistics generated by the ImgU * \param[in] frame The number of the latest frame processed
Since we have moved away from switch/case on the operation ID, there's little reason to split the operation in two functions. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 47 +++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)