| Message ID | 20191006200852.11775-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Sun, Oct 06, 2019 at 10:08:51PM +0200, Jacopo Mondi wrote: > Add support to the dummy VIMC IPA for tracing operation by using a FIFO > channel that will be used by the IPA Interface test to verify > communications with the IPA. > > At the moment only add support for the init() operation as it's the only > defined one. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/ipa/ipa_vimc.h | 21 +++++++++++++ > src/ipa/ipa_vimc.cpp | 68 +++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 88 insertions(+), 1 deletion(-) > create mode 100644 include/ipa/ipa_vimc.h > > diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h > new file mode 100644 > index 000000000000..f4a3ef4f32a4 > --- /dev/null > +++ b/include/ipa/ipa_vimc.h > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * ipa_vimc.h - Vimc Image Processing Algorithm module > + */ > + > +#ifndef __LIBCAMERA_IPA_VIMC_H__ > +#define __LIBCAMERA_IPA_VIMC_H__ > + > +namespace libcamera { > + > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; This should be a #define, otherwise every time this file is included you will end up creating a symbol. You also need a blank line here. With this fixed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +enum IPAOperationCode { > + IPAOperationNone, > + IPAOperationInit, > +}; > + > +}; /* namespace libcamera */ > + > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */ > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > index abc06e7f5fd5..0bc0b73c2d3f 100644 > --- a/src/ipa/ipa_vimc.cpp > +++ b/src/ipa/ipa_vimc.cpp > @@ -5,25 +5,91 @@ > * ipa_vimc.cpp - Vimc Image Processing Algorithm module > */ > > +#include <ipa/ipa_vimc.h> > + > +#include <fcntl.h> > +#include <string.h> > +#include <sys/stat.h> > +#include <unistd.h> > + > #include <iostream> > > #include <ipa/ipa_interface.h> > #include <ipa/ipa_module_info.h> > > +#include "log.h" > + > namespace libcamera { > > +LOG_DEFINE_CATEGORY(IPAVimc) > + > class IPAVimc : public IPAInterface > { > public: > + IPAVimc(); > + ~IPAVimc(); > + > int init(); > + > +private: > + void initTrace(); > + void trace(enum IPAOperationCode operation); > + > + int fd_; > }; > > +IPAVimc::IPAVimc() > + : fd_(-1) > +{ > + initTrace(); > +} > + > +IPAVimc::~IPAVimc() > +{ > + if (fd_) > + ::close(fd_); > +} > + > int IPAVimc::init() > { > - std::cout << "initializing vimc IPA!" << std::endl; > + trace(IPAOperationInit); > + > + LOG(IPAVimc, Debug) << "initializing vimc IPA!"; > + > return 0; > } > > +void IPAVimc::initTrace() > +{ > + struct stat fifoStat; > + int ret = stat(vimcFifoPath, &fifoStat); > + if (ret) > + return; > + > + ret = ::open(vimcFifoPath, O_WRONLY); > + if (ret < 0) { > + ret = -errno; > + LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: " > + << strerror(-ret); > + return; > + } > + > + fd_ = ret; > +} > + > +void IPAVimc::trace(enum IPAOperationCode operation) > +{ > + if (fd_ < 0) > + return; > + > + int ret = ::write(fd_, &operation, sizeof(operation)); > + if (ret < 0) { > + ret = -errno; > + LOG(IPAVimc, Error) << "Failed to write to vimc IPA test FIFO: " > + << strerror(-ret); > + } > +} > + > /* > * External IPA module interface > */
Hi Laurent, On Mon, Oct 07, 2019 at 07:45:53AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Sun, Oct 06, 2019 at 10:08:51PM +0200, Jacopo Mondi wrote: > > Add support to the dummy VIMC IPA for tracing operation by using a FIFO > > channel that will be used by the IPA Interface test to verify > > communications with the IPA. > > > > At the moment only add support for the init() operation as it's the only > > defined one. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/ipa/ipa_vimc.h | 21 +++++++++++++ > > src/ipa/ipa_vimc.cpp | 68 +++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 88 insertions(+), 1 deletion(-) > > create mode 100644 include/ipa/ipa_vimc.h > > > > diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h > > new file mode 100644 > > index 000000000000..f4a3ef4f32a4 > > --- /dev/null > > +++ b/include/ipa/ipa_vimc.h > > @@ -0,0 +1,21 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * ipa_vimc.h - Vimc Image Processing Algorithm module > > + */ > > + > > +#ifndef __LIBCAMERA_IPA_VIMC_H__ > > +#define __LIBCAMERA_IPA_VIMC_H__ > > + > > +namespace libcamera { > > + > > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > This should be a #define, otherwise every time this file is included you > will end up creating a symbol. You also need a blank line here. > Correct, each compilation unit will have its own copy. Although, macros are heavily discouraged everywhere, and specifically I'm not at ease in putting them in public headers, unless very well prefixed. I cannot find anywhere clearly defined if constexpr expressions behaves any differently than regular variables in regards to their allocation, but being them variables I assume there's nothing different, as a symbol has to be created for each file unit that includes the header. I'll go with a macro, but I'm not happy with it, and I rather pay the price of having the symbol reserved twice (which only happens when running tests though). Thanks j > With this fixed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > +enum IPAOperationCode { > > + IPAOperationNone, > > + IPAOperationInit, > > +}; > > + > > +}; /* namespace libcamera */ > > + > > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */ > > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > > index abc06e7f5fd5..0bc0b73c2d3f 100644 > > --- a/src/ipa/ipa_vimc.cpp > > +++ b/src/ipa/ipa_vimc.cpp > > @@ -5,25 +5,91 @@ > > * ipa_vimc.cpp - Vimc Image Processing Algorithm module > > */ > > > > +#include <ipa/ipa_vimc.h> > > + > > +#include <fcntl.h> > > +#include <string.h> > > +#include <sys/stat.h> > > +#include <unistd.h> > > + > > #include <iostream> > > > > #include <ipa/ipa_interface.h> > > #include <ipa/ipa_module_info.h> > > > > +#include "log.h" > > + > > namespace libcamera { > > > > +LOG_DEFINE_CATEGORY(IPAVimc) > > + > > class IPAVimc : public IPAInterface > > { > > public: > > + IPAVimc(); > > + ~IPAVimc(); > > + > > int init(); > > + > > +private: > > + void initTrace(); > > + void trace(enum IPAOperationCode operation); > > + > > + int fd_; > > }; > > > > +IPAVimc::IPAVimc() > > + : fd_(-1) > > +{ > > + initTrace(); > > +} > > + > > +IPAVimc::~IPAVimc() > > +{ > > + if (fd_) > > + ::close(fd_); > > +} > > + > > int IPAVimc::init() > > { > > - std::cout << "initializing vimc IPA!" << std::endl; > > + trace(IPAOperationInit); > > + > > + LOG(IPAVimc, Debug) << "initializing vimc IPA!"; > > + > > return 0; > > } > > > > +void IPAVimc::initTrace() > > +{ > > + struct stat fifoStat; > > + int ret = stat(vimcFifoPath, &fifoStat); > > + if (ret) > > + return; > > + > > + ret = ::open(vimcFifoPath, O_WRONLY); > > + if (ret < 0) { > > + ret = -errno; > > + LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: " > > + << strerror(-ret); > > + return; > > + } > > + > > + fd_ = ret; > > +} > > + > > +void IPAVimc::trace(enum IPAOperationCode operation) > > +{ > > + if (fd_ < 0) > > + return; > > + > > + int ret = ::write(fd_, &operation, sizeof(operation)); > > + if (ret < 0) { > > + ret = -errno; > > + LOG(IPAVimc, Error) << "Failed to write to vimc IPA test FIFO: " > > + << strerror(-ret); > > + } > > +} > > + > > /* > > * External IPA module interface > > */ > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Oct 07, 2019 at 10:15:42AM +0200, Jacopo Mondi wrote: > On Mon, Oct 07, 2019 at 07:45:53AM +0300, Laurent Pinchart wrote: > > On Sun, Oct 06, 2019 at 10:08:51PM +0200, Jacopo Mondi wrote: > >> Add support to the dummy VIMC IPA for tracing operation by using a FIFO > >> channel that will be used by the IPA Interface test to verify > >> communications with the IPA. > >> > >> At the moment only add support for the init() operation as it's the only > >> defined one. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> include/ipa/ipa_vimc.h | 21 +++++++++++++ > >> src/ipa/ipa_vimc.cpp | 68 +++++++++++++++++++++++++++++++++++++++++- > >> 2 files changed, 88 insertions(+), 1 deletion(-) > >> create mode 100644 include/ipa/ipa_vimc.h > >> > >> diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h > >> new file mode 100644 > >> index 000000000000..f4a3ef4f32a4 > >> --- /dev/null > >> +++ b/include/ipa/ipa_vimc.h > >> @@ -0,0 +1,21 @@ > >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > >> +/* > >> + * Copyright (C) 2019, Google Inc. > >> + * > >> + * ipa_vimc.h - Vimc Image Processing Algorithm module > >> + */ > >> + > >> +#ifndef __LIBCAMERA_IPA_VIMC_H__ > >> +#define __LIBCAMERA_IPA_VIMC_H__ > >> + > >> +namespace libcamera { > >> + > >> +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > > > This should be a #define, otherwise every time this file is included you > > will end up creating a symbol. You also need a blank line here. > > Correct, each compilation unit will have its own copy. > > Although, macros are heavily discouraged everywhere, and specifically I'm > not at ease in putting them in public headers, unless very well > prefixed. I cannot find anywhere clearly defined if constexpr > expressions behaves any differently than regular variables in regards to > their allocation, but being them variables I assume there's nothing > different, as a symbol has to be created for each file unit that > includes the header. Note that you're declaring a const variable, not a constexpr variable. I wonder if that makes a difference when it comes to optimisation. Maybe the compiler will always remove the symbol if it doesn't get used ? If that's the case we could avoid a macro. > I'll go with a macro, but I'm not happy with it, and I rather pay the > price of having the symbol reserved twice (which only happens when > running tests though). > > > With this fixed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> +enum IPAOperationCode { > >> + IPAOperationNone, > >> + IPAOperationInit, > >> +}; > >> + > >> +}; /* namespace libcamera */ > >> + > >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */ > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > >> index abc06e7f5fd5..0bc0b73c2d3f 100644 > >> --- a/src/ipa/ipa_vimc.cpp > >> +++ b/src/ipa/ipa_vimc.cpp > >> @@ -5,25 +5,91 @@ > >> * ipa_vimc.cpp - Vimc Image Processing Algorithm module > >> */ > >> > >> +#include <ipa/ipa_vimc.h> > >> + > >> +#include <fcntl.h> > >> +#include <string.h> > >> +#include <sys/stat.h> > >> +#include <unistd.h> > >> + > >> #include <iostream> > >> > >> #include <ipa/ipa_interface.h> > >> #include <ipa/ipa_module_info.h> > >> > >> +#include "log.h" > >> + > >> namespace libcamera { > >> > >> +LOG_DEFINE_CATEGORY(IPAVimc) > >> + > >> class IPAVimc : public IPAInterface > >> { > >> public: > >> + IPAVimc(); > >> + ~IPAVimc(); > >> + > >> int init(); > >> + > >> +private: > >> + void initTrace(); > >> + void trace(enum IPAOperationCode operation); > >> + > >> + int fd_; > >> }; > >> > >> +IPAVimc::IPAVimc() > >> + : fd_(-1) > >> +{ > >> + initTrace(); > >> +} > >> + > >> +IPAVimc::~IPAVimc() > >> +{ > >> + if (fd_) > >> + ::close(fd_); > >> +} > >> + > >> int IPAVimc::init() > >> { > >> - std::cout << "initializing vimc IPA!" << std::endl; > >> + trace(IPAOperationInit); > >> + > >> + LOG(IPAVimc, Debug) << "initializing vimc IPA!"; > >> + > >> return 0; > >> } > >> > >> +void IPAVimc::initTrace() > >> +{ > >> + struct stat fifoStat; > >> + int ret = stat(vimcFifoPath, &fifoStat); > >> + if (ret) > >> + return; > >> + > >> + ret = ::open(vimcFifoPath, O_WRONLY); > >> + if (ret < 0) { > >> + ret = -errno; > >> + LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: " > >> + << strerror(-ret); > >> + return; > >> + } > >> + > >> + fd_ = ret; > >> +} > >> + > >> +void IPAVimc::trace(enum IPAOperationCode operation) > >> +{ > >> + if (fd_ < 0) > >> + return; > >> + > >> + int ret = ::write(fd_, &operation, sizeof(operation)); > >> + if (ret < 0) { > >> + ret = -errno; > >> + LOG(IPAVimc, Error) << "Failed to write to vimc IPA test FIFO: " > >> + << strerror(-ret); > >> + } > >> +} > >> + > >> /* > >> * External IPA module interface > >> */
diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h new file mode 100644 index 000000000000..f4a3ef4f32a4 --- /dev/null +++ b/include/ipa/ipa_vimc.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * ipa_vimc.h - Vimc Image Processing Algorithm module + */ + +#ifndef __LIBCAMERA_IPA_VIMC_H__ +#define __LIBCAMERA_IPA_VIMC_H__ + +namespace libcamera { + +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; +enum IPAOperationCode { + IPAOperationNone, + IPAOperationInit, +}; + +}; /* namespace libcamera */ + +#endif /* __LIBCAMERA_IPA_VIMC_H__ */ diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp index abc06e7f5fd5..0bc0b73c2d3f 100644 --- a/src/ipa/ipa_vimc.cpp +++ b/src/ipa/ipa_vimc.cpp @@ -5,25 +5,91 @@ * ipa_vimc.cpp - Vimc Image Processing Algorithm module */ +#include <ipa/ipa_vimc.h> + +#include <fcntl.h> +#include <string.h> +#include <sys/stat.h> +#include <unistd.h> + #include <iostream> #include <ipa/ipa_interface.h> #include <ipa/ipa_module_info.h> +#include "log.h" + namespace libcamera { +LOG_DEFINE_CATEGORY(IPAVimc) + class IPAVimc : public IPAInterface { public: + IPAVimc(); + ~IPAVimc(); + int init(); + +private: + void initTrace(); + void trace(enum IPAOperationCode operation); + + int fd_; }; +IPAVimc::IPAVimc() + : fd_(-1) +{ + initTrace(); +} + +IPAVimc::~IPAVimc() +{ + if (fd_) + ::close(fd_); +} + int IPAVimc::init() { - std::cout << "initializing vimc IPA!" << std::endl; + trace(IPAOperationInit); + + LOG(IPAVimc, Debug) << "initializing vimc IPA!"; + return 0; } +void IPAVimc::initTrace() +{ + struct stat fifoStat; + int ret = stat(vimcFifoPath, &fifoStat); + if (ret) + return; + + ret = ::open(vimcFifoPath, O_WRONLY); + if (ret < 0) { + ret = -errno; + LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: " + << strerror(-ret); + return; + } + + fd_ = ret; +} + +void IPAVimc::trace(enum IPAOperationCode operation) +{ + if (fd_ < 0) + return; + + int ret = ::write(fd_, &operation, sizeof(operation)); + if (ret < 0) { + ret = -errno; + LOG(IPAVimc, Error) << "Failed to write to vimc IPA test FIFO: " + << strerror(-ret); + } +} + /* * External IPA module interface */
Add support to the dummy VIMC IPA for tracing operation by using a FIFO channel that will be used by the IPA Interface test to verify communications with the IPA. At the moment only add support for the init() operation as it's the only defined one. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/ipa/ipa_vimc.h | 21 +++++++++++++ src/ipa/ipa_vimc.cpp | 68 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 include/ipa/ipa_vimc.h