[libcamera-devel] [PATCH 5/5] ipa: vimc: Add support for test back channel

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Oct 03, 2019 at 05:20:37PM +0200, Jacopo Mondi wrote:
> Add support to the dummy VIMC IPA for communication with the IPA
> interface test unit.
> 
> Use the test channel, if available, to send a confirmation code when
> the init() operation is called.

I would move this patch before 4/5, to avoid introducing a test that
would fail in 4/5.

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/ipa/ipa_vimc.cpp | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/src/ipa/ipa_vimc.cpp b/src/ipa/ipa_vimc.cpp
> index abc06e7f5fd5..781f5796b082 100644
> --- a/src/ipa/ipa_vimc.cpp
> +++ b/src/ipa/ipa_vimc.cpp
> @@ -7,20 +7,74 @@
>  
>  #include <iostream>
>  

No need for a blank line (and iostream should then go after fcntl.h).

> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
>  #include <ipa/ipa_interface.h>
>  #include <ipa/ipa_module_info.h>
>  
>  namespace libcamera {
>  
> +/* Keep these in sync with the IPA Interface test unit. */
> +static const char *vimcFifoPath = "/tmp/vimc_ipa_fifo";

I wonder if the path to the FIFO should be passed to the IPA module
through an environment variable.

> +#define IPA_INIT_CODE 0x01

How about moving this to include/ipa/vimc.h ?

> +
>  class IPAVimc : public IPAInterface
>  {
>  public:
> +	IPAVimc();
> +	~IPAVimc();
> +
>  	int init();
> +
> +private:
> +	unsigned int fd_;

fds are int, not unsigned int.

>  };
>  
> +IPAVimc::IPAVimc()
> +	: fd_(0)

0 is a valid value, it should be initialised to -1.

> +{
> +	/* Set up the test unit back-channel, if available. */
> +	struct stat fifoStat;
> +	int ret = stat(vimcFifoPath, &fifoStat);
> +	if (ret)
> +		return;
> +
> +	ret = ::open(vimcFifoPath, O_WRONLY);
> +	if (ret < 0) {
> +		std::cerr << "Failed to open vimc IPA test fifo at: "
> +			  << vimcFifoPath << ": " << strerror(errno)
> +			  << std::endl;

How about using LOG() ?

> +		return;
> +	}
> +	fd_ = ret;
> +}
> +
> +IPAVimc::~IPAVimc()
> +{
> +	if (fd_)
> +		::close(fd_);
> +}
> +
>  int IPAVimc::init()
>  {
>  	std::cout << "initializing vimc IPA!" << std::endl;
> +
> +	if (!fd_)
> +		return 0;
> +
> +	int8_t data = IPA_INIT_CODE;
> +	int ret = ::write(fd_, &data, 1);
> +	if (ret < 0) {
> +		ret = -errno;
> +		std::cerr << "Failed to write to vimc IPA test fifo at: "
> +			  << vimcFifoPath << ": " << strerror(-ret)
> +			  << std::endl;
> +		return ret;
> +	}

This will be enough for now, but to prepare for the future, should the
write to FIFO be moved to a separate method that will later be called
from the other IPAInterface operations ? I would then turn IPA_INIT_CODE
into an

enum IPAMethod {
	IPAMethodInit,
	...
};

and call

int IPAVimc::init()
{
	trace(IPAMethodInit);

	std:cout << ...
}

(bikeshedding the trace method name is allowed :-)).

> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart