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

[Jacopo Mondi]

Hi Laurent,

On Mon, Oct 07, 2019 at 07:45:53AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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.

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

Thanks
   j

> 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
-------------- 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/20191007/dcbc11d5/attachment.sig>