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

2101 diff mbox series

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

Hi Jacopo,

Thank you for the patch.

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@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>
> +
>  #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__ */

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@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__ */

Hi Laurent,

On Sat, Oct 05, 2019 at 12:02:03AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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@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>
> > +
> >  #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.
>

Agreed

> > +}
> > +
> > +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);
>

Sorry, leftover!

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

Good question...
I assumed include/ would contain public headers only, and this is
specific to an IPA module, and only used outside from src/ipa/ for the
test.

I would keep it private and provide tests a way to include it, unless
I'm missing a use case for sharing this with other components...

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

I assume you mean multiple tests on the IPA Interface... Do we expect
more? one per operation? In any case, passing it from environment
variable mean both the VIMC IPA and the test should access it, and if
it's not defined the test would be skip. Or do you mean passing it
from the build environment? Because in that case it would not make
easier to have create different paths for multiple pipes to be used in
parallled by different tests

> > +enum IPAOperationCodes {
>
> s/IPAOperationCodes/IPAOperationCode/
>

I tend to pluralize enumeration types, but we only actually have them
in the singular form, I'll change this.

Thanks
  j

> > +	IPAOperationNone,
> > +	IPAOperationInit,
> > +};
> > +
> > +}; /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPA_VIMC_H__ */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Sat, Oct 05, 2019 at 02:54:23PM +0200, Jacopo Mondi wrote:
> 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@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>
> >> +
> >>  #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.
> 
> Agreed
> 
> >> +}
> >> +
> >> +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);
> >
> 
> Sorry, leftover!
> 
> >> +		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/ ?
> 
> Good question...
> I assumed include/ would contain public headers only, and this is
> specific to an IPA module, and only used outside from src/ipa/ for the
> test.
> 
> I would keep it private and provide tests a way to include it, unless
> I'm missing a use case for sharing this with other components...

As I just explained in a reply to v1, include/ipa/ is meant for headers
that describe the IPA-specific interfaces. The usual use case if for the
interface with pipeline handlers, but I think the interface with tests
for the VIMC IPA makes sense, as that's a special case.

> >> @@ -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 ?
> 
> I assume you mean multiple tests on the IPA Interface... Do we expect
> more? one per operation? In any case, passing it from environment
> variable mean both the VIMC IPA and the test should access it, and if
> it's not defined the test would be skip. Or do you mean passing it
> from the build environment? Because in that case it would not make
> easier to have create different paths for multiple pipes to be used in
> parallled by different tests

I meant the build environment, to avoid multiple tests trying to use the
same FIFO. The name could be constructed as "/tmp/libcamera-test-ipa." +
PID for instance, to make sure it's unique.

> >> +enum IPAOperationCodes {
> >
> > s/IPAOperationCodes/IPAOperationCode/
> 
> I tend to pluralize enumeration types, but we only actually have them
> in the singular form, I'll change this.
> 
> >> +	IPAOperationNone,
> >> +	IPAOperationInit,
> >> +};
> >> +
> >> +}; /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */

Hi Laurent,

On Sat, Oct 05, 2019 at 07:49:17PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Sat, Oct 05, 2019 at 02:54:23PM +0200, Jacopo Mondi wrote:
> > 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@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>
> > >> +
> > >>  #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.
> >
> > Agreed
> >
> > >> +}
> > >> +
> > >> +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);
> > >
> >
> > Sorry, leftover!
> >
> > >> +		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/ ?
> >
> > Good question...
> > I assumed include/ would contain public headers only, and this is
> > specific to an IPA module, and only used outside from src/ipa/ for the
> > test.
> >
> > I would keep it private and provide tests a way to include it, unless
> > I'm missing a use case for sharing this with other components...
>
> As I just explained in a reply to v1, include/ipa/ is meant for headers
> that describe the IPA-specific interfaces. The usual use case if for the
> interface with pipeline handlers, but I think the interface with tests
> for the VIMC IPA makes sense, as that's a special case.
>

Agreed, will move there

> > >> @@ -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 ?
> >
> > I assume you mean multiple tests on the IPA Interface... Do we expect
> > more? one per operation? In any case, passing it from environment
> > variable mean both the VIMC IPA and the test should access it, and if
> > it's not defined the test would be skip. Or do you mean passing it
> > from the build environment? Because in that case it would not make
> > easier to have create different paths for multiple pipes to be used in
> > parallled by different tests
>
> I meant the build environment, to avoid multiple tests trying to use the
> same FIFO. The name could be constructed as "/tmp/libcamera-test-ipa." +
> PID for instance, to make sure it's unique.
>

Good idea! let's do that once we have more tests ;P

Thanks
  j
> > >> +enum IPAOperationCodes {
> > >
> > > s/IPAOperationCodes/IPAOperationCode/
> >
> > I tend to pluralize enumeration types, but we only actually have them
> > in the singular form, I'll change this.
> >
> > >> +	IPAOperationNone,
> > >> +	IPAOperationInit,
> > >> +};
> > >> +
> > >> +}; /* namespace libcamera */
> > >> +
> > >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */
>
> --
> Regards,
>
> Laurent Pinchart