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

[Laurent Pinchart]

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 at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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