[libcamera-devel] [PATCH v2 5/6] ipa: vimc: Add support for tracing operations

[Laurent Pinchart]

Hi Jacopo,

On Sat, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:
> On Fri, Oct 04, 2019 at 06:37:33PM +0200, Jacopo Mondi wrote:
> > Add support to the dummy VIMC IPA for tracing operation by using a FIFO
> > channel that will be used by the IPA Interface test to verify
> > communications with the IPA.
> > 
> > At the moment only add support for the init() operation as it's the only
> > defined one.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/ipa/ipa_vimc.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++
> >  src/ipa/ipa_vimc.h   | 21 ++++++++++++++
> >  2 files changed, 87 insertions(+)
> >  create mode 100644 src/ipa/ipa_vimc.h
> > 
> > diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp
> > index abc06e7f5fd5..5fb62129fef9 100644
> > --- a/src/ipa/ipa_vimc.cpp
> > +++ b/src/ipa/ipa_vimc.cpp
> > @@ -5,22 +5,88 @@
> >   * ipa_vimc.cpp - Vimc Image Processing Algorithm module
> >   */
> >  
> > +#include "ipa_vimc.h"
> > +
> >  #include <iostream>
> >  
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +#include <unistd.h>

Btw, if you want to split the C and C++ headers, the C headers should
come first according to the style guide.

> > +
> >  #include <ipa/ipa_interface.h>
> >  #include <ipa/ipa_module_info.h>
> >  
> > +#include "log.h"
> > +
> >  namespace libcamera {
> >  
> > +LOG_DEFINE_CATEGORY(IPAVimc)
> > +
> >  class IPAVimc : public IPAInterface
> >  {
> >  public:
> > +	IPAVimc();
> > +	~IPAVimc();
> > +
> >  	int init();
> > +
> > +private:
> > +	void initTrace();
> > +	int trace(enum IPAOperationCodes operation);
> > +
> > +	int fd_;
> >  };
> >  
> > +IPAVimc::IPAVimc()
> > +	: fd_(-1)
> > +{
> > +	initTrace();
> > +}
> > +
> > +IPAVimc::~IPAVimc()
> > +{
> > +	if (fd_)
> > +		::close(fd_);
> > +}
> > +
> >  int IPAVimc::init()
> >  {
> >  	std::cout << "initializing vimc IPA!" << std::endl;
> 
> While at it could you switch this to LOG() ?
> 
> > +
> > +	return trace(IPAOperationInit);
> 
> I would move this to the first line of the function, and ignore the
> return value. You can't really return a value from an IPA operation as
> calls are potentially asynchronous. I would thus make the trace()
> method void.
> 
> > +}
> > +
> > +void IPAVimc::initTrace()
> > +{
> > +	struct stat fifoStat;
> > +	int ret = stat(vimcFifoPath, &fifoStat);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = ::open(vimcFifoPath, O_WRONLY);
> > +	if (ret < 0) {
> > +		LOG(IPAVimc, Debug) << "Failed to open vimc IPA test FIFO: "
> > +				    << strerror(errno) << std::endl;
> 
> No need for std::endl with LOG().
> 
> 		LOG(IPAVimc, Debug)
> 			<< "Failed to open vimc IPA test FIFO: "
> 			<< strerror(errno);
> 
> > +		return;
> > +	}
> > +
> > +	fd_ = ret;
> > +}
> > +
> > +int IPAVimc::trace(enum IPAOperationCodes operation)
> > +{
> > +	if (fd_ < 0)
> > +		return 0;
> > +
> > +	int ret = ::write(fd_, &operation, sizeof(operation));
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		LOG(IPAVimc, Debug) << "Failed to write to vimc IPA test FIFO: "
> > +				    << strerror(-ret) << std::endl;
> 
> Same here.
> 
> > +		return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/src/ipa/ipa_vimc.h b/src/ipa/ipa_vimc.h
> > new file mode 100644
> > index 000000000000..62e40c5a6714
> > --- /dev/null
> > +++ b/src/ipa/ipa_vimc.h
> 
> Shouldn't this live in include/ipa/ ?
> 
> > @@ -0,0 +1,21 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * ipa_vimc.h - Vimc Image Processing Algorithm module
> > + */
> > +
> > +#ifndef __LIBCAMERA_IPA_VIMC_H__
> > +#define __LIBCAMERA_IPA_VIMC_H__
> > +
> > +namespace libcamera {
> > +
> > +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo";
> 
> Should we pass this through an environment variable in order to support
> parallel running of tests ?
> 
> > +enum IPAOperationCodes {
> 
> s/IPAOperationCodes/IPAOperationCode/
> 
> > +	IPAOperationNone,
> > +	IPAOperationInit,
> > +};
> > +
> > +}; /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */

-- 
Regards,

Laurent Pinchart