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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Oct 07, 2019 at 10:15:42AM +0200, Jacopo Mondi wrote:
> On Mon, Oct 07, 2019 at 07:45:53AM +0300, Laurent Pinchart wrote:
> > On Sun, Oct 06, 2019 at 10:08:51PM +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>
> >> ---
> >>  include/ipa/ipa_vimc.h | 21 +++++++++++++
> >>  src/ipa/ipa_vimc.cpp   | 68 +++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 88 insertions(+), 1 deletion(-)
> >>  create mode 100644 include/ipa/ipa_vimc.h
> >>
> >> diff --git a/include/ipa/ipa_vimc.h b/include/ipa/ipa_vimc.h
> >> new file mode 100644
> >> index 000000000000..f4a3ef4f32a4
> >> --- /dev/null
> >> +++ b/include/ipa/ipa_vimc.h
> >> @@ -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";
> >
> > This should be a #define, otherwise every time this file is included you
> > will end up creating a symbol. You also need a blank line here.
> 
> Correct, each compilation unit will have its own copy.
> 
> Although, macros are heavily discouraged everywhere, and specifically I'm
> not at ease in putting them in public headers, unless very well
> prefixed. I cannot find anywhere clearly defined if constexpr
> expressions behaves any differently than regular variables in regards to
> their allocation, but being them variables I assume there's nothing
> different, as a symbol has to be created for each file unit that
> includes the header.

Note that you're declaring a const variable, not a constexpr variable. I
wonder if that makes a difference when it comes to optimisation. Maybe
the compiler will always remove the symbol if it doesn't get used ? If
that's the case we could avoid a macro.

> I'll go with a macro, but I'm not happy with it, and I rather pay the
> price of having the symbol reserved twice (which only happens when
> running tests though).
> 
> > With this fixed,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> >> +enum IPAOperationCode {
> >> +	IPAOperationNone,
> >> +	IPAOperationInit,
> >> +};
> >> +
> >> +}; /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */
> >> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp
> >> index abc06e7f5fd5..0bc0b73c2d3f 100644
> >> --- a/src/ipa/ipa_vimc.cpp
> >> +++ b/src/ipa/ipa_vimc.cpp
> >> @@ -5,25 +5,91 @@
> >>   * ipa_vimc.cpp - Vimc Image Processing Algorithm module
> >>   */
> >>
> >> +#include <ipa/ipa_vimc.h>
> >> +
> >> +#include <fcntl.h>
> >> +#include <string.h>
> >> +#include <sys/stat.h>
> >> +#include <unistd.h>
> >> +
> >>  #include <iostream>
> >>
> >>  #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();
> >> +	void trace(enum IPAOperationCode operation);
> >> +
> >> +	int fd_;
> >>  };
> >>
> >> +IPAVimc::IPAVimc()
> >> +	: fd_(-1)
> >> +{
> >> +	initTrace();
> >> +}
> >> +
> >> +IPAVimc::~IPAVimc()
> >> +{
> >> +	if (fd_)
> >> +		::close(fd_);
> >> +}
> >> +
> >>  int IPAVimc::init()
> >>  {
> >> -	std::cout << "initializing vimc IPA!" << std::endl;
> >> +	trace(IPAOperationInit);
> >> +
> >> +	LOG(IPAVimc, Debug) << "initializing vimc IPA!";
> >> +
> >>  	return 0;
> >>  }
> >>
> >> +void IPAVimc::initTrace()
> >> +{
> >> +	struct stat fifoStat;
> >> +	int ret = stat(vimcFifoPath, &fifoStat);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = ::open(vimcFifoPath, O_WRONLY);
> >> +	if (ret < 0) {
> >> +		ret = -errno;
> >> +		LOG(IPAVimc, Error) << "Failed to open vimc IPA test FIFO: "
> >> +				    << strerror(-ret);
> >> +		return;
> >> +	}
> >> +
> >> +	fd_ = ret;
> >> +}
> >> +
> >> +void IPAVimc::trace(enum IPAOperationCode operation)
> >> +{
> >> +	if (fd_ < 0)
> >> +		return;
> >> +
> >> +	int ret = ::write(fd_, &operation, sizeof(operation));
> >> +	if (ret < 0) {
> >> +		ret = -errno;
> >> +		LOG(IPAVimc, Error) << "Failed to write to vimc IPA test FIFO: "
> >> +				    << strerror(-ret);
> >> +	}
> >> +}
> >> +
> >>  /*
> >>   * External IPA module interface
> >>   */

-- 
Regards,

Laurent Pinchart