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

[paul.elder at ideasonboard.com]

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 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