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

[Jacopo Mondi]

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

> > +#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
-------------- 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/456ae606/attachment.sig>