[libcamera-devel] [PATCH 04/11] libcamera: ipu3: imgu: Add parameters video device
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Nov 5 12:23:16 CET 2020
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, ¶mFormat);
> + 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
More information about the libcamera-devel
mailing list