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

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

-- 
Regards,

Laurent Pinchart