[libcamera-devel,v5,6/9] ipa: vimc: Add IPAOperationCode to init() parameter list
diff mbox series

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

Commit Message

Paul Elder Oct. 11, 2022, 10:58 a.m. UTC 
For the purpose of testing serializing/deserializing enums in function
parameters, add IPAOperationCode to the parameter list of init().

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
No change in v5

Changes in v4:
- remove [TEST] from commit subject

Changes in v3:
- pass enum by value as opposed to by const reference

No changes in v2
---
 include/libcamera/ipa/vimc.mojom     | 2 +-
 src/ipa/vimc/vimc.cpp                | 6 ++++--
 src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
 test/ipa/ipa_interface_test.cpp      | 2 +-
 4 files changed, 7 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi Oct. 12, 2022, 1:41 p.m. UTC | #1 
I understand the VIMC IPA doesn't do anything useful per se, but it
seems we're using it as test module rather than a skeleton IPA.

IOW do we need this if enum serialization is already tested ? Won't it
confuse IPA implementer that might look into VIMC as a reference ?

On Tue, Oct 11, 2022 at 07:58:56PM +0900, Paul Elder via libcamera-devel wrote:
> For the purpose of testing serializing/deserializing enums in function
> parameters, add IPAOperationCode to the parameter list of init().
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> No change in v5
>
> Changes in v4:
> - remove [TEST] from commit subject
>
> Changes in v3:
> - pass enum by value as opposed to by const reference
>
> No changes in v2
> ---
>  include/libcamera/ipa/vimc.mojom     | 2 +-
>  src/ipa/vimc/vimc.cpp                | 6 ++++--
>  src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
>  test/ipa/ipa_interface_test.cpp      | 2 +-
>  4 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> index 718b9674..16149787 100644
> --- a/include/libcamera/ipa/vimc.mojom
> +++ b/include/libcamera/ipa/vimc.mojom
> @@ -18,7 +18,7 @@ enum IPAOperationCode {
>  };
>
>  interface IPAVimcInterface {
> -	init(libcamera.IPASettings settings) => (int32 ret);
> +	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
>
>  	configure(libcamera.IPACameraSensorInfo sensorInfo,
>  		  map<uint32, libcamera.IPAStream> streamConfig,
> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> index 85afb279..5d494b63 100644
> --- a/src/ipa/vimc/vimc.cpp
> +++ b/src/ipa/vimc/vimc.cpp
> @@ -31,7 +31,7 @@ public:
>  	IPAVimc();
>  	~IPAVimc();
>
> -	int init(const IPASettings &settings) override;
> +	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code) override;
>
>  	int start() override;
>  	void stop() override;
> @@ -66,7 +66,7 @@ IPAVimc::~IPAVimc()
>  		::close(fd_);
>  }
>
> -int IPAVimc::init(const IPASettings &settings)
> +int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code)
>  {
>  	trace(ipa::vimc::IPAOperationInit);
>
> @@ -74,6 +74,8 @@ int IPAVimc::init(const IPASettings &settings)
>  		<< "initializing vimc IPA with configuration file "
>  		<< settings.configurationFile;
>
> +	LOG(IPAVimc, Debug) << "Got opcode " << code;
> +
>  	File conf(settings.configurationFile);
>  	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
>  		LOG(IPAVimc, Error) << "Failed to open configuration file";
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index d2f2e460..df749bf7 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -471,7 +471,7 @@ 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() });
> +	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
>
>  	/* Create and register the camera. */
>  	std::set<Stream *> streams{ &data->stream_ };
> diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> index 6b93e976..cd20348a 100644
> --- a/test/ipa/ipa_interface_test.cpp
> +++ b/test/ipa/ipa_interface_test.cpp
> @@ -106,7 +106,7 @@ protected:
>
>  		/* Test initialization of IPA module. */
>  		std::string conf = ipa_->configurationFile("vimc.conf");
> -		int ret = ipa_->init(IPASettings{ conf, "vimc" });
> +		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
>  		if (ret < 0) {
>  			cerr << "IPA interface init() failed" << endl;
>  			return TestFail;
> --
> 2.30.2
>
Laurent Pinchart Oct. 16, 2022, 2:11 a.m. UTC | #2 
On Wed, Oct 12, 2022 at 03:41:46PM +0200, Jacopo Mondi via libcamera-devel wrote:
> I understand the VIMC IPA doesn't do anything useful per se, but it
> seems we're using it as test module rather than a skeleton IPA.
> 
> IOW do we need this if enum serialization is already tested ? Won't it
> confuse IPA implementer that might look into VIMC as a reference ?

Unless I'm mistaken, we don't have any other test for enums in function
parameters. I think we could implement a test that wouldn't require vimc
and would instead implement everything in the test itself, but that
would be lots of code. We've used vimc for the purpose of testing the
IPA interface, so I don't see this as a new issue. I however agree that
it would be nice if we could clean up the vimc IPA interface and bring
it closer to something real, while keeping the features we need for
tests.

> On Tue, Oct 11, 2022 at 07:58:56PM +0900, Paul Elder via libcamera-devel wrote:
> > For the purpose of testing serializing/deserializing enums in function
> > parameters, add IPAOperationCode to the parameter list of init().
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > ---
> > No change in v5
> >
> > Changes in v4:
> > - remove [TEST] from commit subject
> >
> > Changes in v3:
> > - pass enum by value as opposed to by const reference
> >
> > No changes in v2
> > ---
> >  include/libcamera/ipa/vimc.mojom     | 2 +-
> >  src/ipa/vimc/vimc.cpp                | 6 ++++--
> >  src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> >  test/ipa/ipa_interface_test.cpp      | 2 +-
> >  4 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> > index 718b9674..16149787 100644
> > --- a/include/libcamera/ipa/vimc.mojom
> > +++ b/include/libcamera/ipa/vimc.mojom
> > @@ -18,7 +18,7 @@ enum IPAOperationCode {
> >  };
> >
> >  interface IPAVimcInterface {
> > -	init(libcamera.IPASettings settings) => (int32 ret);
> > +	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
> >
> >  	configure(libcamera.IPACameraSensorInfo sensorInfo,
> >  		  map<uint32, libcamera.IPAStream> streamConfig,
> > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > index 85afb279..5d494b63 100644
> > --- a/src/ipa/vimc/vimc.cpp
> > +++ b/src/ipa/vimc/vimc.cpp
> > @@ -31,7 +31,7 @@ public:
> >  	IPAVimc();
> >  	~IPAVimc();
> >
> > -	int init(const IPASettings &settings) override;
> > +	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code) override;
> >
> >  	int start() override;
> >  	void stop() override;
> > @@ -66,7 +66,7 @@ IPAVimc::~IPAVimc()
> >  		::close(fd_);
> >  }
> >
> > -int IPAVimc::init(const IPASettings &settings)
> > +int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code)
> >  {
> >  	trace(ipa::vimc::IPAOperationInit);
> >
> > @@ -74,6 +74,8 @@ int IPAVimc::init(const IPASettings &settings)
> >  		<< "initializing vimc IPA with configuration file "
> >  		<< settings.configurationFile;
> >
> > +	LOG(IPAVimc, Debug) << "Got opcode " << code;
> > +
> >  	File conf(settings.configurationFile);
> >  	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
> >  		LOG(IPAVimc, Error) << "Failed to open configuration file";
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index d2f2e460..df749bf7 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -471,7 +471,7 @@ 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() });
> > +	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
> >
> >  	/* Create and register the camera. */
> >  	std::set<Stream *> streams{ &data->stream_ };
> > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > index 6b93e976..cd20348a 100644
> > --- a/test/ipa/ipa_interface_test.cpp
> > +++ b/test/ipa/ipa_interface_test.cpp
> > @@ -106,7 +106,7 @@ protected:
> >
> >  		/* Test initialization of IPA module. */
> >  		std::string conf = ipa_->configurationFile("vimc.conf");
> > -		int ret = ipa_->init(IPASettings{ conf, "vimc" });
> > +		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
> >  		if (ret < 0) {
> >  			cerr << "IPA interface init() failed" << endl;
> >  			return TestFail;
Nicolas Dufresne via libcamera-devel Oct. 18, 2022, 9:17 a.m. UTC | #3 
On Sun, Oct 16, 2022 at 05:11:50AM +0300, Laurent Pinchart wrote:
> On Wed, Oct 12, 2022 at 03:41:46PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > I understand the VIMC IPA doesn't do anything useful per se, but it
> > seems we're using it as test module rather than a skeleton IPA.
> > 
> > IOW do we need this if enum serialization is already tested ? Won't it
> > confuse IPA implementer that might look into VIMC as a reference ?
> 
> Unless I'm mistaken, we don't have any other test for enums in function
> parameters. I think we could implement a test that wouldn't require vimc

Yeah, that's the deal; we don't otherwise have a test for enums and
flags in function parameters.


Paul

> and would instead implement everything in the test itself, but that
> would be lots of code. We've used vimc for the purpose of testing the
> IPA interface, so I don't see this as a new issue. I however agree that
> it would be nice if we could clean up the vimc IPA interface and bring
> it closer to something real, while keeping the features we need for
> tests.
> 
> > On Tue, Oct 11, 2022 at 07:58:56PM +0900, Paul Elder via libcamera-devel wrote:
> > > For the purpose of testing serializing/deserializing enums in function
> > > parameters, add IPAOperationCode to the parameter list of init().
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > ---
> > > No change in v5
> > >
> > > Changes in v4:
> > > - remove [TEST] from commit subject
> > >
> > > Changes in v3:
> > > - pass enum by value as opposed to by const reference
> > >
> > > No changes in v2
> > > ---
> > >  include/libcamera/ipa/vimc.mojom     | 2 +-
> > >  src/ipa/vimc/vimc.cpp                | 6 ++++--
> > >  src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> > >  test/ipa/ipa_interface_test.cpp      | 2 +-
> > >  4 files changed, 7 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
> > > index 718b9674..16149787 100644
> > > --- a/include/libcamera/ipa/vimc.mojom
> > > +++ b/include/libcamera/ipa/vimc.mojom
> > > @@ -18,7 +18,7 @@ enum IPAOperationCode {
> > >  };
> > >
> > >  interface IPAVimcInterface {
> > > -	init(libcamera.IPASettings settings) => (int32 ret);
> > > +	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
> > >
> > >  	configure(libcamera.IPACameraSensorInfo sensorInfo,
> > >  		  map<uint32, libcamera.IPAStream> streamConfig,
> > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
> > > index 85afb279..5d494b63 100644
> > > --- a/src/ipa/vimc/vimc.cpp
> > > +++ b/src/ipa/vimc/vimc.cpp
> > > @@ -31,7 +31,7 @@ public:
> > >  	IPAVimc();
> > >  	~IPAVimc();
> > >
> > > -	int init(const IPASettings &settings) override;
> > > +	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code) override;
> > >
> > >  	int start() override;
> > >  	void stop() override;
> > > @@ -66,7 +66,7 @@ IPAVimc::~IPAVimc()
> > >  		::close(fd_);
> > >  }
> > >
> > > -int IPAVimc::init(const IPASettings &settings)
> > > +int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code)
> > >  {
> > >  	trace(ipa::vimc::IPAOperationInit);
> > >
> > > @@ -74,6 +74,8 @@ int IPAVimc::init(const IPASettings &settings)
> > >  		<< "initializing vimc IPA with configuration file "
> > >  		<< settings.configurationFile;
> > >
> > > +	LOG(IPAVimc, Debug) << "Got opcode " << code;
> > > +
> > >  	File conf(settings.configurationFile);
> > >  	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
> > >  		LOG(IPAVimc, Error) << "Failed to open configuration file";
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index d2f2e460..df749bf7 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -471,7 +471,7 @@ 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() });
> > > +	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
> > >
> > >  	/* Create and register the camera. */
> > >  	std::set<Stream *> streams{ &data->stream_ };
> > > diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
> > > index 6b93e976..cd20348a 100644
> > > --- a/test/ipa/ipa_interface_test.cpp
> > > +++ b/test/ipa/ipa_interface_test.cpp
> > > @@ -106,7 +106,7 @@ protected:
> > >
> > >  		/* Test initialization of IPA module. */
> > >  		std::string conf = ipa_->configurationFile("vimc.conf");
> > > -		int ret = ipa_->init(IPASettings{ conf, "vimc" });
> > > +		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
> > >  		if (ret < 0) {
> > >  			cerr << "IPA interface init() failed" << endl;
> > >  			return TestFail;
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom
index 718b9674..16149787 100644
--- a/include/libcamera/ipa/vimc.mojom
+++ b/include/libcamera/ipa/vimc.mojom
@@ -18,7 +18,7 @@  enum IPAOperationCode {
 };
 
 interface IPAVimcInterface {
-	init(libcamera.IPASettings settings) => (int32 ret);
+	init(libcamera.IPASettings settings, IPAOperationCode code) => (int32 ret);
 
 	configure(libcamera.IPACameraSensorInfo sensorInfo,
 		  map<uint32, libcamera.IPAStream> streamConfig,
diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp
index 85afb279..5d494b63 100644
--- a/src/ipa/vimc/vimc.cpp
+++ b/src/ipa/vimc/vimc.cpp
@@ -31,7 +31,7 @@  public:
 	IPAVimc();
 	~IPAVimc();
 
-	int init(const IPASettings &settings) override;
+	int init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code) override;
 
 	int start() override;
 	void stop() override;
@@ -66,7 +66,7 @@  IPAVimc::~IPAVimc()
 		::close(fd_);
 }
 
-int IPAVimc::init(const IPASettings &settings)
+int IPAVimc::init(const IPASettings &settings, const ipa::vimc::IPAOperationCode code)
 {
 	trace(ipa::vimc::IPAOperationInit);
 
@@ -74,6 +74,8 @@  int IPAVimc::init(const IPASettings &settings)
 		<< "initializing vimc IPA with configuration file "
 		<< settings.configurationFile;
 
+	LOG(IPAVimc, Debug) << "Got opcode " << code;
+
 	File conf(settings.configurationFile);
 	if (!conf.open(File::OpenModeFlag::ReadOnly)) {
 		LOG(IPAVimc, Error) << "Failed to open configuration file";
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index d2f2e460..df749bf7 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -471,7 +471,7 @@  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() });
+	data->ipa_->init(IPASettings{ conf, data->sensor_->model() }, ipa::vimc::IPAOperationInit);
 
 	/* Create and register the camera. */
 	std::set<Stream *> streams{ &data->stream_ };
diff --git a/test/ipa/ipa_interface_test.cpp b/test/ipa/ipa_interface_test.cpp
index 6b93e976..cd20348a 100644
--- a/test/ipa/ipa_interface_test.cpp
+++ b/test/ipa/ipa_interface_test.cpp
@@ -106,7 +106,7 @@  protected:
 
 		/* Test initialization of IPA module. */
 		std::string conf = ipa_->configurationFile("vimc.conf");
-		int ret = ipa_->init(IPASettings{ conf, "vimc" });
+		int ret = ipa_->init(IPASettings{ conf, "vimc" }, ipa::vimc::IPAOperationInit);
 		if (ret < 0) {
 			cerr << "IPA interface init() failed" << endl;
 			return TestFail;