| Message ID | 20210316101842.18674-2-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jean-Micheal, On Tue, Mar 16, 2021 at 11:18:38AM +0100, Jean-Michel Hautbois wrote: > The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to > calculate the statistics grids. > This patch makes it possible for PipelineHandlerIPU3::start() to access > the BDS configuration and later pass the rectangle to the IPU3 IPA > configure method. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > v2: > - move pipe configuration calculation from validate to configure > - move ipa configuration from start to configure This are really 2 (or maybe even 3) separate patches. Also, when applied on master, this patch does not compile here ../src/libcamera/pipeline/ipu3/ipu3.cpp:633:40: error: too many arguments to function call, expected single argument 'entityControls', have 2 arguments data->ipa_->configure(entityControls, config->pipeConfig_.bds); ~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~ include/libcamera/ipa/ipu3_ipa_proxy.h:44:14: note: 'configure' declared here void configure( > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 45 +++++++++++++--------------- > 1 file changed, 20 insertions(+), 25 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2ea13ec9..3f49ded3 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -97,21 +97,23 @@ public: > Status validate() override; > > const StreamConfiguration &cio2Format() const { return cio2Configuration_; } > - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } > > /* Cache the combinedTransform_ that will be applied to the sensor */ > Transform combinedTransform_; > > + /* Store the pipe configuration */ > + ImgUDevice::PipeConfig pipeConfig_; > + ImgUDevice::Pipe pipe_{}; Is the initialization needed ? I would rather zero-initialize pipe_ at every validate() call. > + > private: > /* > * The IPU3CameraData instance is guaranteed to be valid as long as the > * corresponding Camera instance is valid. In order to borrow a > * reference to the camera data, store a new reference to the camera. > */ > - const IPU3CameraData *data_; > + IPU3CameraData *data_; You don't need this change anymore > > StreamConfiguration cio2Configuration_; > - ImgUDevice::PipeConfig pipeConfig_; > }; > > class PipelineHandlerIPU3 : public PipelineHandler > @@ -272,8 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > > LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); > > - ImgUDevice::Pipe pipe{}; > - pipe.input = cio2Configuration_.size; > + pipe_.input = cio2Configuration_.size; > > /* > * Adjust the configurations if needed and assign streams while > @@ -349,15 +350,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > cfg->setStream(const_cast<Stream *>(&data_->outStream_)); > mainOutputAvailable = false; > > - pipe.main = cfg->size; > + pipe_.main = cfg->size; > if (yuvCount == 1) > - pipe.viewfinder = pipe.main; > + pipe_.viewfinder = pipe_.main; > > LOG(IPU3, Debug) << "Assigned " << cfg->toString() > << " to the main output"; > } else { > cfg->setStream(const_cast<Stream *>(&data_->vfStream_)); > - pipe.viewfinder = cfg->size; > + pipe_.viewfinder = cfg->size; > > LOG(IPU3, Debug) << "Assigned " << cfg->toString() > << " to the viewfinder output"; > @@ -373,16 +374,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() > } > } > > - /* Only compute the ImgU configuration if a YUV stream has been requested. */ > - if (yuvCount) { > - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); > - if (pipeConfig_.isNull()) { > - LOG(IPU3, Error) << "Failed to calculate pipe configuration: " > - << "unsupported resolutions."; > - return Invalid; > - } > - } > - > return status; > } > > @@ -576,11 +567,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > * stream has been requested: return here to skip the ImgU configuration > * part. > */ > - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); > - if (imguConfig.isNull()) You can't drop this, as you the comment explain if not imguConfig was calculated in validate() it's because !yuvCount, and we should stop here in configure. You can keep collecting the pipe_ sizes in validate() and replace this check with if (pipe_.main.isNull() && pipe_.viewfinder.isNull()) return 0; > + > + config->pipeConfig_ = imgu->calculatePipeConfig(&config->pipe_); > + if (config->pipeConfig_.isNull()) { > + LOG(IPU3, Error) << "Failed to calculate pipe configuration: " > + << "unsupported resolutions."; Otherwise this will always fail in case of RAW-only capture. > return 0; > + } > > - ret = imgu->configure(imguConfig, &cio2Format); > + ret = imgu->configure(config->pipeConfig_, &cio2Format); Then you can move the ImgU pipe configuration to ImgUDevice::configure() > if (ret) > return ret; > > @@ -633,6 +628,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > return ret; > } > > + std::map<uint32_t, ControlInfoMap> entityControls; > + entityControls.emplace(0, data->cio2_.sensor()->controls()); > + data->ipa_->configure(entityControls, config->pipeConfig_.bds); > + Moving ipa_->configure() here is worth a separate patch Adding the BDS rectange to IPA::configure() is worth a separate patch Thanks j > return 0; > } > > @@ -717,7 +716,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) > > int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) > { > - std::map<uint32_t, ControlInfoMap> entityControls; > IPU3CameraData *data = cameraData(camera); > CIO2Device *cio2 = &data->cio2_; > ImgUDevice *imgu = data->imgu_; > @@ -744,9 +742,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret) > goto error; > > - entityControls.emplace(0, data->cio2_.sensor()->controls()); > - data->ipa_->configure(entityControls); > - > return 0; > > error: > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 2ea13ec9..3f49ded3 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -97,21 +97,23 @@ public: Status validate() override; const StreamConfiguration &cio2Format() const { return cio2Configuration_; } - const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; } /* Cache the combinedTransform_ that will be applied to the sensor */ Transform combinedTransform_; + /* Store the pipe configuration */ + ImgUDevice::PipeConfig pipeConfig_; + ImgUDevice::Pipe pipe_{}; + private: /* * The IPU3CameraData instance is guaranteed to be valid as long as the * corresponding Camera instance is valid. In order to borrow a * reference to the camera data, store a new reference to the camera. */ - const IPU3CameraData *data_; + IPU3CameraData *data_; StreamConfiguration cio2Configuration_; - ImgUDevice::PipeConfig pipeConfig_; }; class PipelineHandlerIPU3 : public PipelineHandler @@ -272,8 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString(); - ImgUDevice::Pipe pipe{}; - pipe.input = cio2Configuration_.size; + pipe_.input = cio2Configuration_.size; /* * Adjust the configurations if needed and assign streams while @@ -349,15 +350,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() cfg->setStream(const_cast<Stream *>(&data_->outStream_)); mainOutputAvailable = false; - pipe.main = cfg->size; + pipe_.main = cfg->size; if (yuvCount == 1) - pipe.viewfinder = pipe.main; + pipe_.viewfinder = pipe_.main; LOG(IPU3, Debug) << "Assigned " << cfg->toString() << " to the main output"; } else { cfg->setStream(const_cast<Stream *>(&data_->vfStream_)); - pipe.viewfinder = cfg->size; + pipe_.viewfinder = cfg->size; LOG(IPU3, Debug) << "Assigned " << cfg->toString() << " to the viewfinder output"; @@ -373,16 +374,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate() } } - /* Only compute the ImgU configuration if a YUV stream has been requested. */ - if (yuvCount) { - pipeConfig_ = data_->imgu_->calculatePipeConfig(&pipe); - if (pipeConfig_.isNull()) { - LOG(IPU3, Error) << "Failed to calculate pipe configuration: " - << "unsupported resolutions."; - return Invalid; - } - } - return status; } @@ -576,11 +567,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) * stream has been requested: return here to skip the ImgU configuration * part. */ - ImgUDevice::PipeConfig imguConfig = config->imguConfig(); - if (imguConfig.isNull()) + + config->pipeConfig_ = imgu->calculatePipeConfig(&config->pipe_); + if (config->pipeConfig_.isNull()) { + LOG(IPU3, Error) << "Failed to calculate pipe configuration: " + << "unsupported resolutions."; return 0; + } - ret = imgu->configure(imguConfig, &cio2Format); + ret = imgu->configure(config->pipeConfig_, &cio2Format); if (ret) return ret; @@ -633,6 +628,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) return ret; } + std::map<uint32_t, ControlInfoMap> entityControls; + entityControls.emplace(0, data->cio2_.sensor()->controls()); + data->ipa_->configure(entityControls, config->pipeConfig_.bds); + return 0; } @@ -717,7 +716,6 @@ int PipelineHandlerIPU3::freeBuffers(Camera *camera) int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlList *controls) { - std::map<uint32_t, ControlInfoMap> entityControls; IPU3CameraData *data = cameraData(camera); CIO2Device *cio2 = &data->cio2_; ImgUDevice *imgu = data->imgu_; @@ -744,9 +742,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret) goto error; - entityControls.emplace(0, data->cio2_.sensor()->controls()); - data->ipa_->configure(entityControls); - return 0; error:
The IPU3 IPA needs to know the BayerDownScaler (BDS) configuration to calculate the statistics grids. This patch makes it possible for PipelineHandlerIPU3::start() to access the BDS configuration and later pass the rectangle to the IPU3 IPA configure method. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- v2: - move pipe configuration calculation from validate to configure - move ipa configuration from start to configure --- src/libcamera/pipeline/ipu3/ipu3.cpp | 45 +++++++++++++--------------- 1 file changed, 20 insertions(+), 25 deletions(-)