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

[Jacopo Mondi]

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

> > @@ -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
-------------- 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/20191005/76675eb4/attachment.sig>