[libcamera-devel,v2,7/7,TEST] ipa: vimc: Add Flags to parameters
diff mbox series

Message ID 20220818064923.2573060-8-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • utils: ipc: Add support for enums and Flags
Related show

Commit Message

Paul Elder Aug. 18, 2022, 6:49 a.m. UTC 
For the purpose of testing serializing/deserializing Flags in function
parameters, add an enum class TestFlags and Flags<TestFlags> to some
function parameters, both for input and output and Signals.

While at it, update the ipa_interface_test.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- use new attribute-based mojom definition for Flags
---
 include/libcamera/ipa/vimc.mojom     | 14 ++++++++++++--
 src/ipa/vimc/vimc.cpp                | 21 +++++++++++++++++----
 src/libcamera/pipeline/vimc/vimc.cpp | 16 +++++++++++++---
 test/ipa/ipa_interface_test.cpp      |  6 +++++-
 4 files changed, 47 insertions(+), 10 deletions(-)

Comments

Laurent Pinchart Aug. 19, 2022, 1:02 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Thu, Aug 18, 2022 at 03:49:22PM +0900, Paul Elder via libcamera-devel wrote:
> For the purpose of testing serializing/deserializing Flags in function
> parameters, add an enum class TestFlags and Flags<TestFlags> to some
> function parameters, both for input and output and Signals.
> 
> While at it, update the ipa_interface_test.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - use new attribute-based mojom definition for Flags
> ---
>  include/libcamera/ipa/vimc.mojom     | 14 ++++++++++++--
>  src/ipa/vimc/vimc.cpp                | 21 +++++++++++++++++----
>  src/libcamera/pipeline/vimc/vimc.cpp | 16 +++++++++++++---
>  test/ipa/ipa_interface_test.cpp      |  6 +++++-
>  4 files changed, 47 insertions(+), 10 deletions(-)
> 
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 16149787..15948d77 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -17,8 +17,18 @@ enum IPAOperationCode {
>  	IPAOperationStop,
>  };
>  
> +[Flags] enum TestFlags {

This should be TestFlag as each enumerator is a single flag. We do the
same in C++ code, and then create a type alias

	using TestFlags = Flags<TestFlag>;

We could actually try to do something similar in mojo, by defining a
type that aliases to Flags<T> and repurposing the [flags] attribute to
qualify the type definition, instead of qualifying every usage of the
enum in structures or function parameters, but that's probably overkill.

> +	Flag1 = 0x1,
> +	Flag2 = 0x2,
> +	Flag3 = 0x4,
> +	Flag4 = 0x8,
> +};
> +
>  interface IPAVimcInterface {
> -	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
> +	init(libcamera.IPASettings settings,
> +	     IPAOperationCode code,
> +	     [Flags] TestFlags inFlags)
> +	=> (int32 ret, [Flags] TestFlags outFlags);
>  
>  	configure(libcamera.IPACameraSensorInfo sensorInfo,
>  		  map<uint32, libcamera.IPAStream> streamConfig,
> @@ -41,5 +51,5 @@ interface IPAVimcInterface {
>  };
>  
>  interface IPAVimcEventInterface {
> -	paramsBufferReady(uint32 bufferId);
> +	paramsBufferReady(uint32 bufferId, [Flags] TestFlags flags);
>  };
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 6bf39a1c..bb4ca355 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -31,7 +31,10 @@ public:
>  	IPAVimc();
>  	~IPAVimc();
>  
> -	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code) override;
> +	int init(const IPASettings &settings,
> +		 const ipa::vimc::IPAOperationCode &code,
> +		 const Flags<ipa::vimc::TestFlags> &inFlags,

Same question as in 6/7, should this be passed by value ?

> +		 Flags<ipa::vimc::TestFlags> *outFlags) override;
>  
>  	int start() override;
>  	void stop() override;
> @@ -65,8 +68,10 @@ IPAVimc::~IPAVimc()
>  	if (fd_ != -1)
>  		::close(fd_);
>  }
> -
> -int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code)
> +int IPAVimc::init(const IPASettings &settings,
> +		  const ipa::vimc::IPAOperationCode &code,
> +		  const Flags<ipa::vimc::TestFlags> &inFlags,
> +		  Flags<ipa::vimc::TestFlags> *outFlags)
>  {
>  	trace(ipa::vimc::IPAOperationInit);
>  
> @@ -76,6 +81,13 @@ int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode
>  
>  	LOG(IPAVimc, Debug) << "Got opcode " << code;
>  
> +	LOG(IPAVimc, Debug)
> +		<< "Flag 2 was "
> +		<< ((inFlags & ipa::vimc::TestFlags::Flag2) ? "" : "not ")

I think you can drop the inner parentheses.

> +		<< "set";
> +
> +	*outFlags |= ipa::vimc::TestFlags::Flag1;
> +
>  	File conf(settings.configurationFile);
>  	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
>  		LOG(IPAVimc, Error) << "Failed to open configuration file";
> @@ -144,7 +156,8 @@ void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferI
>  		return;
>  	}
>  
> -	paramsBufferReady.emit(bufferId);
> +	Flags<ipa::vimc::TestFlags> flags;
> +	paramsBufferReady.emit(bufferId, flags);
>  }
>  
>  void IPAVimc::initTrace()
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 983bd514..e74a8679 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -54,7 +54,7 @@ public:
>  	int init();
>  	int allocateMockIPABuffers();
>  	void bufferReady(FrameBuffer *buffer);
> -	void paramsBufferReady(unsigned int id);
> +	void paramsBufferReady(unsigned int id, const Flags<ipa::vimc::TestFlags> &flags);
>  
>  	MediaDevice *media_;
>  	std::unique_ptr<CameraSensor> sensor_;
> @@ -471,7 +471,16 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
>  	data->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);
>  
>  	std::string conf = data->ipa_->configurationFile("vimc.conf");
> -	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
> +	Flags<ipa::vimc::TestFlags> inFlags;
> +	Flags<ipa::vimc::TestFlags> outFlags;
> +	inFlags |= ipa::vimc::TestFlags::Flag2;

This could be set when construcing inFlags.

> +	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
> +			 ipa::vimc::IPAOperationInit, inFlags, &outFlags);
> +
> +	LOG(VIMC, Debug)
> +		<< "Flag 1 was "
> +		<< ((outFlags & ipa::vimc::TestFlags::Flag1) ? "" : "not ")

You can drop the inner parentheses here too.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		<< "set";
>  
>  	/* Create and register the camera. */
>  	std::set<Stream *> streams{ &data->stream_ };
> @@ -608,7 +617,8 @@ int VimcCameraData::allocateMockIPABuffers()
>  	return video_->exportBuffers(kBufCount, &mockIPABufs_);
>  }
>  
> -void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)
> +void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id,
> +				       [[maybe_unused]] const Flags<ipa::vimc::TestFlags> &flags)
>  {
>  }
>  
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index b9fa15cd..47c7352b 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -106,7 +106,11 @@ protected:
>  
>  		/* Test initialization of IPA module. */
>  		std::string conf = ipa_->configurationFile("vimc.conf");
> -		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
> +		Flags<ipa::vimc::TestFlags> inFlags;
> +		Flags<ipa::vimc::TestFlags> outFlags;
> +		int ret = ipa_->init(IPASettings{ conf, "vimc" },
> +				     ipa::vimc::IPAOperationInit,
> +				     inFlags, &outFlags);
>  		if (ret < 0) {
>  			cerr << "IPA interface init() failed" << endl;
>  			return TestFail;

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
index 16149787..15948d77 100644
--- a/include/libcamera/ipa/vimc.mojom
+++ b/include/libcamera/ipa/vimc.mojom
@@ -17,8 +17,18 @@  enum IPAOperationCode {
 	IPAOperationStop,
 };
 
+[Flags] enum TestFlags {
+	Flag1 = 0x1,
+	Flag2 = 0x2,
+	Flag3 = 0x4,
+	Flag4 = 0x8,
+};
+
 interface IPAVimcInterface {
-	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
+	init(libcamera.IPASettings settings,
+	     IPAOperationCode code,
+	     [Flags] TestFlags inFlags)
+	=> (int32 ret, [Flags] TestFlags outFlags);
 
 	configure(libcamera.IPACameraSensorInfo sensorInfo,
 		  map<uint32, libcamera.IPAStream> streamConfig,
@@ -41,5 +51,5 @@  interface IPAVimcInterface {
 };
 
 interface IPAVimcEventInterface {
-	paramsBufferReady(uint32 bufferId);
+	paramsBufferReady(uint32 bufferId, [Flags] TestFlags flags);
 };
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index 6bf39a1c..bb4ca355 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -31,7 +31,10 @@  public:
 	IPAVimc();
 	~IPAVimc();
 
-	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code) override;
+	int init(const IPASettings &settings,
+		 const ipa::vimc::IPAOperationCode &code,
+		 const Flags<ipa::vimc::TestFlags> &inFlags,
+		 Flags<ipa::vimc::TestFlags> *outFlags) override;
 
 	int start() override;
 	void stop() override;
@@ -65,8 +68,10 @@  IPAVimc::~IPAVimc()
 	if (fd_ != -1)
 		::close(fd_);
 }
-
-int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode &code)
+int IPAVimc::init(const IPASettings &settings,
+		  const ipa::vimc::IPAOperationCode &code,
+		  const Flags<ipa::vimc::TestFlags> &inFlags,
+		  Flags<ipa::vimc::TestFlags> *outFlags)
 {
 	trace(ipa::vimc::IPAOperationInit);
 
@@ -76,6 +81,13 @@  int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode
 
 	LOG(IPAVimc, Debug) << "Got opcode " << code;
 
+	LOG(IPAVimc, Debug)
+		<< "Flag 2 was "
+		<< ((inFlags & ipa::vimc::TestFlags::Flag2) ? "" : "not ")
+		<< "set";
+
+	*outFlags |= ipa::vimc::TestFlags::Flag1;
+
 	File conf(settings.configurationFile);
 	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
 		LOG(IPAVimc, Error) << "Failed to open configuration file";
@@ -144,7 +156,8 @@  void IPAVimc::fillParamsBuffer([[maybe_unused]] uint32_t frame, uint32_t bufferI
 		return;
 	}
 
-	paramsBufferReady.emit(bufferId);
+	Flags<ipa::vimc::TestFlags> flags;
+	paramsBufferReady.emit(bufferId, flags);
 }
 
 void IPAVimc::initTrace()
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 983bd514..e74a8679 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -54,7 +54,7 @@  public:
 	int init();
 	int allocateMockIPABuffers();
 	void bufferReady(FrameBuffer *buffer);
-	void paramsBufferReady(unsigned int id);
+	void paramsBufferReady(unsigned int id, const Flags<ipa::vimc::TestFlags> &flags);
 
 	MediaDevice *media_;
 	std::unique_ptr<CameraSensor> sensor_;
@@ -471,7 +471,16 @@  bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
 	data->ipa_->paramsBufferReady.connect(data.get(), &VimcCameraData::paramsBufferReady);
 
 	std::string conf = data->ipa_->configurationFile("vimc.conf");
-	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
+	Flags<ipa::vimc::TestFlags> inFlags;
+	Flags<ipa::vimc::TestFlags> outFlags;
+	inFlags |= ipa::vimc::TestFlags::Flag2;
+	data->ipa_->init(IPASettings{ conf, data->sensor_->model() },
+			 ipa::vimc::IPAOperationInit, inFlags, &outFlags);
+
+	LOG(VIMC, Debug)
+		<< "Flag 1 was "
+		<< ((outFlags & ipa::vimc::TestFlags::Flag1) ? "" : "not ")
+		<< "set";
 
 	/* Create and register the camera. */
 	std::set<Stream *> streams{ &data->stream_ };
@@ -608,7 +617,8 @@  int VimcCameraData::allocateMockIPABuffers()
 	return video_->exportBuffers(kBufCount, &mockIPABufs_);
 }
 
-void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id)
+void VimcCameraData::paramsBufferReady([[maybe_unused]] unsigned int id,
+				       [[maybe_unused]] const Flags<ipa::vimc::TestFlags> &flags)
 {
 }
 
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index b9fa15cd..47c7352b 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -106,7 +106,11 @@  protected:
 
 		/* Test initialization of IPA module. */
 		std::string conf = ipa_->configurationFile("vimc.conf");
-		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
+		Flags<ipa::vimc::TestFlags> inFlags;
+		Flags<ipa::vimc::TestFlags> outFlags;
+		int ret = ipa_->init(IPASettings{ conf, "vimc" },
+				     ipa::vimc::IPAOperationInit,
+				     inFlags, &outFlags);
 		if (ret < 0) {
 			cerr << "IPA interface init() failed" << endl;
 			return TestFail;