[v1,5/8] ipa: rkisp1: Add debug metadata support to the rkisp1
diff mbox series

Message ID 20241002161933.247091-6-stefan.klug@ideasonboard.com
State Superseded
Headers show
Series
  • Add support for IPA debugging metadata
Related show

Commit Message

Stefan Klug Oct. 2, 2024, 4:19 p.m. UTC 
Add a DebugMetadata helper to the context and add the corresponding
plumbing.  This is all that is needed to support debug metadata in an
IPA.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/ipa_context.h | 5 +++++
 src/ipa/rkisp1/rkisp1.cpp    | 4 ++++
 2 files changed, 9 insertions(+)

Comments

Laurent Pinchart Oct. 3, 2024, 5:05 p.m. UTC | #1 
Hi Stefan,

Thank you for the patch.

On Wed, Oct 02, 2024 at 06:19:23PM +0200, Stefan Klug wrote:
> Add a DebugMetadata helper to the context and add the corresponding
> plumbing.  This is all that is needed to support debug metadata in an
> IPA.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/ipa_context.h | 5 +++++
>  src/ipa/rkisp1/rkisp1.cpp    | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index d52e73ad2503..7b93a9e9461d 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -17,8 +17,11 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/geometry.h>
> +
>  #include <libcamera/ipa/core_ipa_interface.h>
>  
> +#include "libcamera/internal/debug_controls.h"
> +
>  #include <libipa/camera_sensor_helper.h>
>  #include <libipa/fc_queue.h>
>  #include <libipa/matrix.h>
> @@ -194,6 +197,8 @@ struct IPAContext {
>  
>  	ControlInfoMap::Map ctrlMap;
>  
> +	DebugMetadata debugMetadata;
> +
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper;
>  };
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index a579f21de56f..5b1ef0c372c6 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -117,6 +117,7 @@ const IPAHwSettings ipaHwSettingsV12{
>  const ControlInfoMap::Map rkisp1Controls{
>  	{ &controls::AwbEnable, ControlInfo(false, true) },
>  	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> +	{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },
>  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
>  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
>  };
> @@ -326,6 +327,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>  void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
>  {
>  	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> +	context_.debugMetadata.checkForEnable(controls);
>  
>  	for (auto const &a : algorithms()) {
>  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> @@ -368,6 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>  		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>  
>  	ControlList metadata(controls::controls);
> +	context_.debugMetadata.assignControlList(&metadata);

What's the advantage of doing this here, instead of...

>  
>  	for (auto const &a : algorithms()) {
>  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> @@ -377,6 +380,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>  	}
>  
>  	setControls(frame);
> +	context_.debugMetadata.assignControlList(nullptr);

... extracting the control list from debugMetadata here and merging it
into metadata ?

>  	metadataReady.emit(frame, metadata);
>  }
Stefan Klug Oct. 3, 2024, 8:13 p.m. UTC | #2 
Hi Laurent,

Thank you for the review. 

On Thu, Oct 03, 2024 at 08:05:58PM +0300, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Wed, Oct 02, 2024 at 06:19:23PM +0200, Stefan Klug wrote:
> > Add a DebugMetadata helper to the context and add the corresponding
> > plumbing.  This is all that is needed to support debug metadata in an
> > IPA.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/ipa_context.h | 5 +++++
> >  src/ipa/rkisp1/rkisp1.cpp    | 4 ++++
> >  2 files changed, 9 insertions(+)
> > 
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index d52e73ad2503..7b93a9e9461d 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -17,8 +17,11 @@
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/geometry.h>
> > +
> >  #include <libcamera/ipa/core_ipa_interface.h>
> >  
> > +#include "libcamera/internal/debug_controls.h"
> > +
> >  #include <libipa/camera_sensor_helper.h>
> >  #include <libipa/fc_queue.h>
> >  #include <libipa/matrix.h>
> > @@ -194,6 +197,8 @@ struct IPAContext {
> >  
> >  	ControlInfoMap::Map ctrlMap;
> >  
> > +	DebugMetadata debugMetadata;
> > +
> >  	/* Interface to the Camera Helper */
> >  	std::unique_ptr<CameraSensorHelper> camHelper;
> >  };
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index a579f21de56f..5b1ef0c372c6 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -117,6 +117,7 @@ const IPAHwSettings ipaHwSettingsV12{
> >  const ControlInfoMap::Map rkisp1Controls{
> >  	{ &controls::AwbEnable, ControlInfo(false, true) },
> >  	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > +	{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },
> >  	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> >  	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> >  };
> > @@ -326,6 +327,7 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
> >  void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
> >  {
> >  	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> > +	context_.debugMetadata.checkForEnable(controls);
> >  
> >  	for (auto const &a : algorithms()) {
> >  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> > @@ -368,6 +370,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >  		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >  
> >  	ControlList metadata(controls::controls);
> > +	context_.debugMetadata.assignControlList(&metadata);
> 
> What's the advantage of doing this here, instead of...
> 
> >  
> >  	for (auto const &a : algorithms()) {
> >  		Algorithm *algo = static_cast<Algorithm *>(a.get());
> > @@ -377,6 +380,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
> >  	}
> >  
> >  	setControls(frame);
> > +	context_.debugMetadata.assignControlList(nullptr);
> 
> ... extracting the control list from debugMetadata here and merging it
> into metadata ?

Oh you are right. I had that initial thought of attaching a ControlList
to a DebugMetadata object for longer than the current function call. But
in practice that case never happens as the Metadata list is short lived.
I'll drop the whole assignControlList part in the next version.

Regards,
Stefan

> 
> >  	metadataReady.emit(frame, metadata);
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index d52e73ad2503..7b93a9e9461d 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -17,8 +17,11 @@ 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 #include <libcamera/geometry.h>
+
 #include <libcamera/ipa/core_ipa_interface.h>
 
+#include "libcamera/internal/debug_controls.h"
+
 #include <libipa/camera_sensor_helper.h>
 #include <libipa/fc_queue.h>
 #include <libipa/matrix.h>
@@ -194,6 +197,8 @@  struct IPAContext {
 
 	ControlInfoMap::Map ctrlMap;
 
+	DebugMetadata debugMetadata;
+
 	/* Interface to the Camera Helper */
 	std::unique_ptr<CameraSensorHelper> camHelper;
 };
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index a579f21de56f..5b1ef0c372c6 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -117,6 +117,7 @@  const IPAHwSettings ipaHwSettingsV12{
 const ControlInfoMap::Map rkisp1Controls{
 	{ &controls::AwbEnable, ControlInfo(false, true) },
 	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
+	{ &controls::DebugMetadataEnable, ControlInfo(false, true, false) },
 	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
 	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
 };
@@ -326,6 +327,7 @@  void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
 void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
 {
 	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
+	context_.debugMetadata.checkForEnable(controls);
 
 	for (auto const &a : algorithms()) {
 		Algorithm *algo = static_cast<Algorithm *>(a.get());
@@ -368,6 +370,7 @@  void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
 		context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
 
 	ControlList metadata(controls::controls);
+	context_.debugMetadata.assignControlList(&metadata);
 
 	for (auto const &a : algorithms()) {
 		Algorithm *algo = static_cast<Algorithm *>(a.get());
@@ -377,6 +380,7 @@  void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
 	}
 
 	setControls(frame);
+	context_.debugMetadata.assignControlList(nullptr);
 
 	metadataReady.emit(frame, metadata);
 }