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

[Laurent Pinchart]

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.

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