[libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test back channel

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



-- 
Regards,

Laurent Pinchart