| Message ID | 20220818064923.2573060-7-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Thu, Aug 18, 2022 at 03:49:21PM +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> > > --- > 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..6bf39a1c 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; Should we do for enums the same thing we do for integers, and pass by value instead of const reference ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > 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 153cf849..983bd514 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 3c0df843..b9fa15cd 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;
On Fri, Aug 19, 2022 at 03:55:04AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Thu, Aug 18, 2022 at 03:49:21PM +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> > > > > --- > > 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..6bf39a1c 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; > > Should we do for enums the same thing we do for integers, and pass by > value instead of const reference ? Hm, tbh I think we should. I guess I have to fix that too then :/ > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> That means that... this is approved for merging...? I wasn't sure how we felt about modifying the vimc IPA interface in such dummy-test ways Paul > > > > > 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 153cf849..983bd514 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 3c0df843..b9fa15cd 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;
Hi Paul, On Fri, Aug 19, 2022 at 04:44:56PM +0900, paul.elder@ideasonboard.com wrote: > On Fri, Aug 19, 2022 at 03:55:04AM +0300, Laurent Pinchart wrote: > > On Thu, Aug 18, 2022 at 03:49:21PM +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> > > > > > > --- > > > 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..6bf39a1c 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; > > > > Should we do for enums the same thing we do for integers, and pass by > > value instead of const reference ? > > Hm, tbh I think we should. I guess I have to fix that too then :/ > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > That means that... this is approved for merging...? I wasn't sure how we > felt about modifying the vimc IPA interface in such dummy-test ways I'm fine with that, the vimc API is meant for this purpose. I expect we'll refactor it later to make it look less "dummy", but we'll keep an anum and a Flags somewhere in any case for the purpose of testing. For now, this is fine. > > > 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 153cf849..983bd514 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 3c0df843..b9fa15cd 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;
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..6bf39a1c 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 153cf849..983bd514 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 3c0df843..b9fa15cd 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;
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> --- 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(-)