| Message ID | 20201105001546.1690179-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
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@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@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; >
Hi Niklas, On Thu, Nov 05, 2020 at 01:15:39AM +0100, 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. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; I guess this answers my previous question, so format and size are fixed on the stat and param nodes, and only the ImgU subdevice's source pad needs configuration. Looks good (also for the previous patch): Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > *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; > + } > + > /* > * 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_; > 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; > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Thu, Nov 05, 2020 at 01:15:39AM +0100, 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@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")); Depending on which series is merged first (see "[PATCH 2/2] libcamera: v4l2_device: Return a unique pointer from fromEntityName()"), you could write this param_ = 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. */ Maybe this is a better candidate than 03/11 to add a small explanation why this is the case. Could you check if we even need to set the format on the subdev pad ? It seems ignored in the driver. > + if (dev == param_.get() || dev == stat_.get()) > return 0; > > *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; > + } > + > /* > * 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_; I would also move this between input and output, but it's not big deal. Reviewed-by: Laurent Pinchart <laurent.pinchart@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;
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; *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; + } + /* * 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_; 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;
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. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/imgu.cpp | 38 ++++++++++++++++++++++++++-- src/libcamera/pipeline/ipu3/imgu.h | 3 ++- 2 files changed, 38 insertions(+), 3 deletions(-)