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

[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 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
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;
> >>  }
> >>

-- 
Regards,

Laurent Pinchart