| Message ID | 20191003152037.74617-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote: > Add support to the dummy VIMC IPA for communication with the IPA > interface test unit. > > Use the test channel, if available, to send a confirmation code when > the init() operation is called. I would move this patch before 4/5, to avoid introducing a test that would fail in 4/5. > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > index abc06e7f5fd5..781f5796b082 100644 > --- a/src/ipa/ipa_vimc.cpp > +++ b/src/ipa/ipa_vimc.cpp > @@ -7,20 +7,74 @@ > > #include <iostream> > No need for a blank line (and iostream should then go after fcntl.h). > +#include <fcntl.h> > +#include <string.h> > +#include <sys/stat.h> > +#include <unistd.h> > + > #include <ipa/ipa_interface.h> > #include <ipa/ipa_module_info.h> > > namespace libcamera { > > +/* Keep these in sync with the IPA Interface test unit. */ > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; I wonder if the path to the FIFO should be passed to the IPA module through an environment variable. > +#define IPA_INIT_CODE 0x01 How about moving this to include/ipa/vimc.h ? > + > class IPAVimc : public IPAInterface > { > public: > + IPAVimc(); > + ~IPAVimc(); > + > int init(); > + > +private: > + unsigned int fd_; fds are int, not unsigned int. > }; > > +IPAVimc::IPAVimc() > + : fd_(0) 0 is a valid value, it should be initialised to -1. > +{ > + /* Set up the test unit back-channel, if available. */ > + struct stat fifoStat; > + int ret = stat(vimcFifoPath, &fifoStat); > + if (ret) > + return; > + > + ret = ::open(vimcFifoPath, O_WRONLY); > + if (ret < 0) { > + std::cerr << "Failed to open vimc IPA test fifo at: " > + << vimcFifoPath << ": " << strerror(errno) > + << std::endl; How about using LOG() ? > + return; > + } > + fd_ = ret; > +} > + > +IPAVimc::~IPAVimc() > +{ > + if (fd_) > + ::close(fd_); > +} > + > int IPAVimc::init() > { > std::cout << "initializing vimc IPA!" << std::endl; > + > + if (!fd_) > + return 0; > + > + int8_t data = IPA_INIT_CODE; > + int ret = ::write(fd_, &data, 1); > + if (ret < 0) { > + ret = -errno; > + std::cerr << "Failed to write to vimc IPA test fifo at: " > + << vimcFifoPath << ": " << strerror(-ret) > + << std::endl; > + return ret; > + } This will be enough for now, but to prepare for the future, should the write to FIFO be moved to a separate method that will later be called from the other IPAInterface operations ? I would then turn IPA_INIT_CODE into an enum IPAMethod { IPAMethodInit, ... }; and call int IPAVimc::init() { trace(IPAMethodInit); std:cout << ... } (bikeshedding the trace method name is allowed :-)). > + > return 0; > } >
Hi Laurent, On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote: > > Add support to the dummy VIMC IPA for communication with the IPA > > interface test unit. > > > > Use the test channel, if available, to send a confirmation code when > > the init() operation is called. > > I would move this patch before 4/5, to avoid introducing a test that > would fail in 4/5. > Yeah, better! > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > > > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > > index abc06e7f5fd5..781f5796b082 100644 > > --- a/src/ipa/ipa_vimc.cpp > > +++ b/src/ipa/ipa_vimc.cpp > > @@ -7,20 +7,74 @@ > > > > #include <iostream> > > > > No need for a blank line (and iostream should then go after fcntl.h). > Don't we keep c++ and c headers inclusion directives separate ? > > +#include <fcntl.h> > > +#include <string.h> > > +#include <sys/stat.h> > > +#include <unistd.h> > > + > > #include <ipa/ipa_interface.h> > > #include <ipa/ipa_module_info.h> > > > > namespace libcamera { > > > > +/* Keep these in sync with the IPA Interface test unit. */ > > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > I wonder if the path to the FIFO should be passed to the IPA module > through an environment variable. > I thought how to avoid having to keep the two in sync, including a shared header > > +#define IPA_INIT_CODE 0x01 > > How about moving this to include/ipa/vimc.h ? > ...as you suggest here. It would however contain mostly definitions for the testing infrastructure, and I was not sure how welcome that would be. I'll go ahead with this since it seems there are no objections > > + > > class IPAVimc : public IPAInterface > > { > > public: > > + IPAVimc(); > > + ~IPAVimc(); > > + > > int init(); > > + > > +private: > > + unsigned int fd_; > > fds are int, not unsigned int. > ? Only if they are used to store error codes. Negative file descriptors ? > > }; > > > > +IPAVimc::IPAVimc() > > + : fd_(0) > > 0 is a valid value, it should be initialised to -1. > Are we using stdin ? > > +{ > > + /* Set up the test unit back-channel, if available. */ > > + struct stat fifoStat; > > + int ret = stat(vimcFifoPath, &fifoStat); > > + if (ret) > > + return; > > + > > + ret = ::open(vimcFifoPath, O_WRONLY); > > + if (ret < 0) { > > + std::cerr << "Failed to open vimc IPA test fifo at: " > > + << vimcFifoPath << ": " << strerror(errno) > > + << std::endl; > > How about using LOG() ? > I saw cout/cerr where used an I thought IPA should avoid using LOG(), but there are no reasons for doing so, as this IPA is linked against libcamera anyhow > > + return; > > + } > > + fd_ = ret; > > +} > > + > > +IPAVimc::~IPAVimc() > > +{ > > + if (fd_) > > + ::close(fd_); > > +} > > + > > int IPAVimc::init() > > { > > std::cout << "initializing vimc IPA!" << std::endl; > > + > > + if (!fd_) > > + return 0; > > + > > + int8_t data = IPA_INIT_CODE; > > + int ret = ::write(fd_, &data, 1); > > + if (ret < 0) { > > + ret = -errno; > > + std::cerr << "Failed to write to vimc IPA test fifo at: " > > + << vimcFifoPath << ": " << strerror(-ret) > > + << std::endl; > > + return ret; > > + } > > This will be enough for now, but to prepare for the future, should the > write to FIFO be moved to a separate method that will later be called > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE > into an > > enum IPAMethod { > IPAMethodInit, > ... Could we wait to see how the interface looks like before defining the enum I however agree a separate method makes sense already. > }; > > and call > > int IPAVimc::init() > { > trace(IPAMethodInit); > > std:cout << ... > } > > (bikeshedding the trace method name is allowed :-)). > trace is fine, or 'report', 'signal'... > > + > > return 0; > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote: > On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote: > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote: > >> Add support to the dummy VIMC IPA for communication with the IPA > >> interface test unit. > >> > >> Use the test channel, if available, to send a confirmation code when > >> the init() operation is called. > > > > I would move this patch before 4/5, to avoid introducing a test that > > would fail in 4/5. > > Yeah, better! > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 54 insertions(+) > >> > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > >> index abc06e7f5fd5..781f5796b082 100644 > >> --- a/src/ipa/ipa_vimc.cpp > >> +++ b/src/ipa/ipa_vimc.cpp > >> @@ -7,20 +7,74 @@ > >> > >> #include <iostream> > >> > > > > No need for a blank line (and iostream should then go after fcntl.h). > > Don't we keep c++ and c headers inclusion directives separate ? We do in a few files, and mix them in other files. I think we should pick one. I have so far favoured mixing them, but if we want to separate them that's OK with me too, as long as we're consistent. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes documents splitting the headers. Is that your preference too ? If so, could you submit a patch to fix it library-wide ? > >> +#include <fcntl.h> > >> +#include <string.h> > >> +#include <sys/stat.h> > >> +#include <unistd.h> > >> + > >> #include <ipa/ipa_interface.h> > >> #include <ipa/ipa_module_info.h> > >> > >> namespace libcamera { > >> > >> +/* Keep these in sync with the IPA Interface test unit. */ > >> +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > > > I wonder if the path to the FIFO should be passed to the IPA module > > through an environment variable. > > I thought how to avoid having to keep the two in sync, including a > shared header Passing it through an environment variable would also allow us to make the name dynamic, which could be useful for running multiple tests in parallel. > >> +#define IPA_INIT_CODE 0x01 > > > > How about moving this to include/ipa/vimc.h ? > > ...as you suggest here. It would however contain mostly definitions for > the testing infrastructure, and I was not sure how welcome that would > be. I'll go ahead with this since it seems there are no objections Please do :-) > >> + > >> class IPAVimc : public IPAInterface > >> { > >> public: > >> + IPAVimc(); > >> + ~IPAVimc(); > >> + > >> int init(); > >> + > >> +private: > >> + unsigned int fd_; > > > > fds are int, not unsigned int. > > ? Only if they are used to store error codes. Negative file > descriptors ? Negative means invalid, which is useful to be able to store. The C library functions that deal with fds use signed values, we should do here too. > >> }; > >> > >> +IPAVimc::IPAVimc() > >> + : fd_(0) > > > > 0 is a valid value, it should be initialised to -1. > > Are we using stdin ? Whether 0 will end up being a possible case in practice depends on multiple factors. When the IPA is isolated stdin is closed for instance. Let's be safe. > >> +{ > >> + /* Set up the test unit back-channel, if available. */ > >> + struct stat fifoStat; > >> + int ret = stat(vimcFifoPath, &fifoStat); > >> + if (ret) > >> + return; > >> + > >> + ret = ::open(vimcFifoPath, O_WRONLY); > >> + if (ret < 0) { > >> + std::cerr << "Failed to open vimc IPA test fifo at: " > >> + << vimcFifoPath << ": " << strerror(errno) > >> + << std::endl; > > > > How about using LOG() ? > > I saw cout/cerr where used an I thought IPA should avoid using LOG(), > but there are no reasons for doing so, as this IPA is linked against > libcamera anyhow Could you please convert the existing cout/cerr to LOG too ? > >> + return; > >> + } > >> + fd_ = ret; > >> +} > >> + > >> +IPAVimc::~IPAVimc() > >> +{ > >> + if (fd_) > >> + ::close(fd_); > >> +} > >> + > >> int IPAVimc::init() > >> { > >> std::cout << "initializing vimc IPA!" << std::endl; > >> + > >> + if (!fd_) > >> + return 0; > >> + > >> + int8_t data = IPA_INIT_CODE; > >> + int ret = ::write(fd_, &data, 1); > >> + if (ret < 0) { > >> + ret = -errno; > >> + std::cerr << "Failed to write to vimc IPA test fifo at: " > >> + << vimcFifoPath << ": " << strerror(-ret) > >> + << std::endl; > >> + return ret; > >> + } > > > > This will be enough for now, but to prepare for the future, should the > > write to FIFO be moved to a separate method that will later be called > > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE > > into an > > > > enum IPAMethod { > > IPAMethodInit, > > ... > > Could we wait to see how the interface looks like before defining the > enum We can, but even if we have a single value for the enum now, is there a drawback in using it ? It makes the API clearer as the type is then explicit. > I however agree a separate method makes sense already. > > > }; > > > > and call > > > > int IPAVimc::init() > > { > > trace(IPAMethodInit); > > > > std:cout << ... > > } > > > > (bikeshedding the trace method name is allowed :-)). > > > > trace is fine, or 'report', 'signal'... > > >> + > >> return 0; > >> } > >>
Hi Laurent, On Fri, Oct 04, 2019 at 06:56:09PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote: > > On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote: > > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote: > > >> Add support to the dummy VIMC IPA for communication with the IPA > > >> interface test unit. > > >> > > >> Use the test channel, if available, to send a confirmation code when > > >> the init() operation is called. > > > > > > I would move this patch before 4/5, to avoid introducing a test that > > > would fail in 4/5. > > > > Yeah, better! > > > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >> --- > > >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ > > >> 1 file changed, 54 insertions(+) > > >> > > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > > >> index abc06e7f5fd5..781f5796b082 100644 > > >> --- a/src/ipa/ipa_vimc.cpp > > >> +++ b/src/ipa/ipa_vimc.cpp > > >> @@ -7,20 +7,74 @@ > > >> > > >> #include <iostream> > > >> > > > > > > No need for a blank line (and iostream should then go after fcntl.h). > > > > Don't we keep c++ and c headers inclusion directives separate ? > > We do in a few files, and mix them in other files. I think we should > pick one. I have so far favoured mixing them, but if we want to separate > them that's OK with me too, as long as we're consistent. > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes That's where my preference comes from. > documents splitting the headers. Is that your preference too ? If so, > could you submit a patch to fix it library-wide ? I'm not sure it's top priority at the moment, but let's keep it in mind. But let's pick one direction from now on, and I would stick wil the cppguide > > > >> +#include <fcntl.h> > > >> +#include <string.h> > > >> +#include <sys/stat.h> > > >> +#include <unistd.h> > > >> + > > >> #include <ipa/ipa_interface.h> > > >> #include <ipa/ipa_module_info.h> > > >> > > >> namespace libcamera { > > >> > > >> +/* Keep these in sync with the IPA Interface test unit. */ > > >> +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > > > > > I wonder if the path to the FIFO should be passed to the IPA module > > > through an environment variable. > > > > I thought how to avoid having to keep the two in sync, including a > > shared header > > Passing it through an environment variable would also allow us to make > the name dynamic, which could be useful for running multiple tests in > parallel. > I've put it in the vimc_ipa.h header > > >> +#define IPA_INIT_CODE 0x01 > > > > > > How about moving this to include/ipa/vimc.h ? which I have however added in src/ipa/ It's actually only used by vimc_ipa.c and the test. Is it worth placing it in the public includes directory ? > > > > ...as you suggest here. It would however contain mostly definitions for > > the testing infrastructure, and I was not sure how welcome that would > > be. I'll go ahead with this since it seems there are no objections > > Please do :-) > > > >> + > > >> class IPAVimc : public IPAInterface > > >> { > > >> public: > > >> + IPAVimc(); > > >> + ~IPAVimc(); > > >> + > > >> int init(); > > >> + > > >> +private: > > >> + unsigned int fd_; > > > > > > fds are int, not unsigned int. > > > > ? Only if they are used to store error codes. Negative file > > descriptors ? > > Negative means invalid, which is useful to be able to store. The C > library functions that deal with fds use signed values, we should do > here too. > > > >> }; > > >> > > >> +IPAVimc::IPAVimc() > > >> + : fd_(0) > > > > > > 0 is a valid value, it should be initialised to -1. > > > > Are we using stdin ? > > Whether 0 will end up being a possible case in practice depends on > multiple factors. When the IPA is isolated stdin is closed for instance. > Let's be safe. Agreed! Also the C library uses signed ints, I assumed they were unsigned. > > > >> +{ > > >> + /* Set up the test unit back-channel, if available. */ > > >> + struct stat fifoStat; > > >> + int ret = stat(vimcFifoPath, &fifoStat); > > >> + if (ret) > > >> + return; > > >> + > > >> + ret = ::open(vimcFifoPath, O_WRONLY); > > >> + if (ret < 0) { > > >> + std::cerr << "Failed to open vimc IPA test fifo at: " > > >> + << vimcFifoPath << ": " << strerror(errno) > > >> + << std::endl; > > > > > > How about using LOG() ? > > > > I saw cout/cerr where used an I thought IPA should avoid using LOG(), > > but there are no reasons for doing so, as this IPA is linked against > > libcamera anyhow > > Could you please convert the existing cout/cerr to LOG too ? Yup > > > >> + return; > > >> + } > > >> + fd_ = ret; > > >> +} > > >> + > > >> +IPAVimc::~IPAVimc() > > >> +{ > > >> + if (fd_) > > >> + ::close(fd_); > > >> +} > > >> + > > >> int IPAVimc::init() > > >> { > > >> std::cout << "initializing vimc IPA!" << std::endl; > > >> + > > >> + if (!fd_) > > >> + return 0; > > >> + > > >> + int8_t data = IPA_INIT_CODE; > > >> + int ret = ::write(fd_, &data, 1); > > >> + if (ret < 0) { > > >> + ret = -errno; > > >> + std::cerr << "Failed to write to vimc IPA test fifo at: " > > >> + << vimcFifoPath << ": " << strerror(-ret) > > >> + << std::endl; > > >> + return ret; > > >> + } > > > > > > This will be enough for now, but to prepare for the future, should the > > > write to FIFO be moved to a separate method that will later be called > > > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE > > > into an > > > > > > enum IPAMethod { > > > IPAMethodInit, > > > ... > > > > Could we wait to see how the interface looks like before defining the > > enum > > We can, but even if we have a single value for the enum now, is there a > drawback in using it ? It makes the API clearer as the type is then > explicit. Yeah, I was afraid adding operations would have changed the way we want to return tracing codes, but for now let's use an enum > > > I however agree a separate method makes sense already. > > > > > }; > > > > > > and call > > > > > > int IPAVimc::init() > > > { > > > trace(IPAMethodInit); > > > > > > std:cout << ... > > > } > > > > > > (bikeshedding the trace method name is allowed :-)). > > > > > > > trace is fine, or 'report', 'signal'... > > > > >> + > > >> return 0; > > >> } > > >> > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Fri, Oct 04, 2019 at 06:20:51PM +0200, Jacopo Mondi wrote: > On Fri, Oct 04, 2019 at 06:56:09PM +0300, Laurent Pinchart wrote: > > On Fri, Oct 04, 2019 at 10:31:15AM +0200, Jacopo Mondi wrote: > > > On Thu, Oct 03, 2019 at 09:47:41PM +0300, Laurent Pinchart wrote: > > > > On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote: > > > >> Add support to the dummy VIMC IPA for communication with the IPA > > > >> interface test unit. > > > >> > > > >> Use the test channel, if available, to send a confirmation code when > > > >> the init() operation is called. > > > > > > > > I would move this patch before 4/5, to avoid introducing a test that > > > > would fail in 4/5. > > > > > > Yeah, better! > > > > > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > >> --- > > > >> src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ > > > >> 1 file changed, 54 insertions(+) > > > >> > > > >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp > > > >> index abc06e7f5fd5..781f5796b082 100644 > > > >> --- a/src/ipa/ipa_vimc.cpp > > > >> +++ b/src/ipa/ipa_vimc.cpp > > > >> @@ -7,20 +7,74 @@ > > > >> > > > >> #include <iostream> > > > >> > > > > > > > > No need for a blank line (and iostream should then go after fcntl.h). > > > > > > Don't we keep c++ and c headers inclusion directives separate ? > > > > We do in a few files, and mix them in other files. I think we should > > pick one. I have so far favoured mixing them, but if we want to separate > > them that's OK with me too, as long as we're consistent. > > > > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > That's where my preference comes from. > > > documents splitting the headers. Is that your preference too ? If so, > > could you submit a patch to fix it library-wide ? > > I'm not sure it's top priority at the moment, but let's keep it in > mind. But let's pick one direction from now on, and I would stick wil > the cppguide > > > > >> +#include <fcntl.h> > > > >> +#include <string.h> > > > >> +#include <sys/stat.h> > > > >> +#include <unistd.h> > > > >> + > > > >> #include <ipa/ipa_interface.h> > > > >> #include <ipa/ipa_module_info.h> > > > >> > > > >> namespace libcamera { > > > >> > > > >> +/* Keep these in sync with the IPA Interface test unit. */ > > > >> +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; > > > > > > > > I wonder if the path to the FIFO should be passed to the IPA module > > > > through an environment variable. > > > > > > I thought how to avoid having to keep the two in sync, including a > > > shared header > > > > Passing it through an environment variable would also allow us to make > > the name dynamic, which could be useful for running multiple tests in > > parallel. > > I've put it in the vimc_ipa.h header > > > > >> +#define IPA_INIT_CODE 0x01 > > > > > > > > How about moving this to include/ipa/vimc.h ? > > which I have however added in src/ipa/ > > It's actually only used by vimc_ipa.c and the test. Is it worth > placing it in the public includes directory ? I think so. include/ipa/ is meant for headers that define the interface between an IPA and a pipeline handler. This case is slightly different, but it's still an IPA interface, so I think adding it there makes sense. The VIMC IPA is special anyway, and using include/ipa/ will avoid adding another include directory to the search path. > > > ...as you suggest here. It would however contain mostly definitions for > > > the testing infrastructure, and I was not sure how welcome that would > > > be. I'll go ahead with this since it seems there are no objections > > > > Please do :-) > > > > > >> + > > > >> class IPAVimc : public IPAInterface > > > >> { > > > >> public: > > > >> + IPAVimc(); > > > >> + ~IPAVimc(); > > > >> + > > > >> int init(); > > > >> + > > > >> +private: > > > >> + unsigned int fd_; > > > > > > > > fds are int, not unsigned int. > > > > > > ? Only if they are used to store error codes. Negative file > > > descriptors ? > > > > Negative means invalid, which is useful to be able to store. The C > > library functions that deal with fds use signed values, we should do > > here too. > > > > > >> }; > > > >> > > > >> +IPAVimc::IPAVimc() > > > >> + : fd_(0) > > > > > > > > 0 is a valid value, it should be initialised to -1. > > > > > > Are we using stdin ? > > > > Whether 0 will end up being a possible case in practice depends on > > multiple factors. When the IPA is isolated stdin is closed for instance. > > Let's be safe. > > Agreed! Also the C library uses signed ints, I assumed they were > unsigned. > > > > > > >> +{ > > > >> + /* Set up the test unit back-channel, if available. */ > > > >> + struct stat fifoStat; > > > >> + int ret = stat(vimcFifoPath, &fifoStat); > > > >> + if (ret) > > > >> + return; > > > >> + > > > >> + ret = ::open(vimcFifoPath, O_WRONLY); > > > >> + if (ret < 0) { > > > >> + std::cerr << "Failed to open vimc IPA test fifo at: " > > > >> + << vimcFifoPath << ": " << strerror(errno) > > > >> + << std::endl; > > > > > > > > How about using LOG() ? > > > > > > I saw cout/cerr where used an I thought IPA should avoid using LOG(), > > > but there are no reasons for doing so, as this IPA is linked against > > > libcamera anyhow > > > > Could you please convert the existing cout/cerr to LOG too ? > > Yup > > > > > > >> + return; > > > >> + } > > > >> + fd_ = ret; > > > >> +} > > > >> + > > > >> +IPAVimc::~IPAVimc() > > > >> +{ > > > >> + if (fd_) > > > >> + ::close(fd_); > > > >> +} > > > >> + > > > >> int IPAVimc::init() > > > >> { > > > >> std::cout << "initializing vimc IPA!" << std::endl; > > > >> + > > > >> + if (!fd_) > > > >> + return 0; > > > >> + > > > >> + int8_t data = IPA_INIT_CODE; > > > >> + int ret = ::write(fd_, &data, 1); > > > >> + if (ret < 0) { > > > >> + ret = -errno; > > > >> + std::cerr << "Failed to write to vimc IPA test fifo at: " > > > >> + << vimcFifoPath << ": " << strerror(-ret) > > > >> + << std::endl; > > > >> + return ret; > > > >> + } > > > > > > > > This will be enough for now, but to prepare for the future, should the > > > > write to FIFO be moved to a separate method that will later be called > > > > from the other IPAInterface operations ? I would then turn IPA_INIT_CODE > > > > into an > > > > > > > > enum IPAMethod { > > > > IPAMethodInit, > > > > ... > > > > > > Could we wait to see how the interface looks like before defining the > > > enum > > > > We can, but even if we have a single value for the enum now, is there a > > drawback in using it ? It makes the API clearer as the type is then > > explicit. > > Yeah, I was afraid adding operations would have changed the way we > want to return tracing codes, but for now let's use an enum > > > > > > I however agree a separate method makes sense already. > > > > > > > }; > > > > > > > > and call > > > > > > > > int IPAVimc::init() > > > > { > > > > trace(IPAMethodInit); > > > > > > > > std:cout << ... > > > > } > > > > > > > > (bikeshedding the trace method name is allowed :-)). > > > > > > > > > > trace is fine, or 'report', 'signal'... > > > > > > >> + > > > >> return 0; > > > >> } > > > >> > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp index abc06e7f5fd5..781f5796b082 100644 --- a/src/ipa/ipa_vimc.cpp +++ b/src/ipa/ipa_vimc.cpp @@ -7,20 +7,74 @@ #include <iostream> +#include <fcntl.h> +#include <string.h> +#include <sys/stat.h> +#include <unistd.h> + #include <ipa/ipa_interface.h> #include <ipa/ipa_module_info.h> namespace libcamera { +/* Keep these in sync with the IPA Interface test unit. */ +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo"; +#define IPA_INIT_CODE 0x01 + class IPAVimc : public IPAInterface { public: + IPAVimc(); + ~IPAVimc(); + int init(); + +private: + unsigned int fd_; }; +IPAVimc::IPAVimc() + : fd_(0) +{ + /* Set up the test unit back-channel, if available. */ + struct stat fifoStat; + int ret = stat(vimcFifoPath, &fifoStat); + if (ret) + return; + + ret = ::open(vimcFifoPath, O_WRONLY); + if (ret < 0) { + std::cerr << "Failed to open vimc IPA test fifo at: " + << vimcFifoPath << ": " << strerror(errno) + << std::endl; + return; + } + fd_ = ret; +} + +IPAVimc::~IPAVimc() +{ + if (fd_) + ::close(fd_); +} + int IPAVimc::init() { std::cout << "initializing vimc IPA!" << std::endl; + + if (!fd_) + return 0; + + int8_t data = IPA_INIT_CODE; + int ret = ::write(fd_, &data, 1); + if (ret < 0) { + ret = -errno; + std::cerr << "Failed to write to vimc IPA test fifo at: " + << vimcFifoPath << ": " << strerror(-ret) + << std::endl; + return ret; + } + return 0; }
Add support to the dummy VIMC IPA for communication with the IPA interface test unit. Use the test channel, if available, to send a confirmation code when the init() operation is called. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)