[libcamera-devel] [PATCH v2 7/7] [TEST] ipa: vimc: Add Flags to parameters
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Aug 19 03:02:53 CEST 2022
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 at 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 at 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;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list