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

[Jacopo Mondi]

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

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
-------------- 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/20191006/ee774b71/attachment.sig>