[libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add parameters video device

[Kieran Bingham]

Hi Niklas,

On 05/11/2020 00:15, Niklas Söderlund wrote:
> Add the parameters video device to the data structure of the ImgUDevice.
> Even if the video device is configured, prepared to import buffers and
> started no buffers are ever queued to it so it does not yet effect the
> capture. Nor is does this change hinder the current capture mode to
> function.
> 
> This is done in preparation to attache an IPA to the IPU3 pipeline.
> 

s/attache/attach/


> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 38 ++++++++++++++++++++++++++--
>  src/libcamera/pipeline/ipu3/imgu.h   |  3 ++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 0a3bf62020fd23fb..547a9e00325e7519 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -365,6 +365,11 @@ int ImgUDevice::init(MediaDevice *media, unsigned int index)
>  	if (ret)
>  		return ret;
>  
> +	param_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " parameters"));
> +	ret = param_->open();
> +	if (ret)
> +		return ret;
> +
>  	stat_.reset(V4L2VideoDevice::fromEntityName(media, name_ + " 3a stat"));
>  	ret = stat_->open();
>  	if (ret)
> @@ -477,6 +482,13 @@ int ImgUDevice::configure(const PipeConfig &pipeConfig, V4L2DeviceFormat *inputF
>  
>  	LOG(IPU3, Debug) << "ImgU GDC format = " << gdcFormat.toString();
>  
> +	StreamConfiguration paramCfg = {};
> +	paramCfg.size = inputFormat->size;
> +	V4L2DeviceFormat paramFormat;
> +	ret = configureVideoDevice(param_.get(), PAD_PARAM, paramCfg, &paramFormat);
> +	if (ret)
> +		return ret;
> +
>  	StreamConfiguration statCfg = {};
>  	statCfg.size = inputFormat->size;
>  	V4L2DeviceFormat statFormat;
> @@ -507,8 +519,8 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
>  	if (ret)
>  		return ret;
>  
> -	/* No need to apply format to the stat node. */
> -	if (dev == stat_.get())
> +	/* No need to apply format to the param or stat video devices. */
> +	if (dev == param_.get() || dev == stat_.get())
>  		return 0;

Ayee, I'm not sure I like this 'intrinsic knowledge of what device it's
operating on' in this function, but perhaps it's not better than
duplicating code. (and it was here before this patch anyway).


>  
>  	*outputFormat = {};
> @@ -539,6 +551,12 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount)
>  		return ret;
>  	}
>  
> +	ret = param_->importBuffers(bufferCount);
> +	if (ret < 0) {
> +		LOG(IPU3, Error) << "Failed to allocate ImgU param buffers";
> +		goto error;
> +	}

Will you allocate parameter buffers on the parameter node?

(Yes, I see that happen later... I wonder why it doesn't just happen
here, but it's fine for now).


> +
>  	/*
>  	 * The kernel fails to start if buffers are not either imported or
>  	 * allocated for the statistics video device. As statistics buffers are
> @@ -588,6 +606,10 @@ void ImgUDevice::freeBuffers()
>  	if (ret)
>  		LOG(IPU3, Error) << "Failed to release ImgU output buffers";
>  
> +	ret = param_->releaseBuffers();
> +	if (ret)
> +		LOG(IPU3, Error) << "Failed to release ImgU param buffers";
> +
>  	ret = stat_->releaseBuffers();
>  	if (ret)
>  		LOG(IPU3, Error) << "Failed to release ImgU stat buffers";
> @@ -618,6 +640,12 @@ int ImgUDevice::start()
>  		return ret;
>  	}
>  
> +	ret = param_->streamOn();
> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to start ImgU param";
> +		return ret;
> +	}
> +
>  	ret = stat_->streamOn();
>  	if (ret) {
>  		LOG(IPU3, Error) << "Failed to start ImgU stat";
> @@ -639,6 +667,7 @@ int ImgUDevice::stop()
>  
>  	ret = output_->streamOff();
>  	ret |= viewfinder_->streamOff();
> +	ret |= param_->streamOff();
>  	ret |= stat_->streamOff();
>  	ret |= input_->streamOff();
>  
> @@ -680,6 +709,7 @@ int ImgUDevice::linkSetup(const std::string &source, unsigned int sourcePad,
>  int ImgUDevice::enableLinks(bool enable)
>  {
>  	std::string viewfinderName = name_ + " viewfinder";
> +	std::string paramName = name_ + " parameters";
>  	std::string outputName = name_ + " output";
>  	std::string statName = name_ + " 3a stat";
>  	std::string inputName = name_ + " input";
> @@ -697,6 +727,10 @@ int ImgUDevice::enableLinks(bool enable)
>  	if (ret)
>  		return ret;
>  
> +	ret = linkSetup(paramName, 0, name_, PAD_PARAM, enable);
> +	if (ret)
> +		return ret;
> +
>  	return linkSetup(name_, PAD_STAT, statName, 0, enable);
>  }
>  
> diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h
> index 37f5ae77c99ff8fe..1388d07a45b28590 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.h
> +++ b/src/libcamera/pipeline/ipu3/imgu.h
> @@ -73,11 +73,12 @@ public:
>  	std::unique_ptr<V4L2VideoDevice> input_;
>  	std::unique_ptr<V4L2VideoDevice> output_;
>  	std::unique_ptr<V4L2VideoDevice> viewfinder_;
> +	std::unique_ptr<V4L2VideoDevice> param_;

How about keeping these in the same order as the pad values?
(Put this between input/output)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>  	std::unique_ptr<V4L2VideoDevice> stat_;
> -	/* \todo Add param video device for 3A tuning */
>  
>  private:
>  	static constexpr unsigned int PAD_INPUT = 0;
> +	static constexpr unsigned int PAD_PARAM = 1;
>  	static constexpr unsigned int PAD_OUTPUT = 2;
>  	static constexpr unsigned int PAD_VF = 3;
>  	static constexpr unsigned int PAD_STAT = 4;
> 

-- 
Regards
--
Kieran