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

[Jacopo Mondi]

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 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 ?

> >
> > ...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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191004/e094effc/attachment.sig>