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

[Laurent Pinchart]

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.

> +}
> +
> +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);

> +		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/ ?

> @@ -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 ?

> +enum IPAOperationCodes {

s/IPAOperationCodes/IPAOperationCode/

> +	IPAOperationNone,
> +	IPAOperationInit,
> +};
> +
> +}; /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPA_VIMC_H__ */

-- 
Regards,

Laurent Pinchart