| Message ID | 20210216122624.27940-3-dafna.hirschfeld@collabora.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Dafna, On Tue, Feb 16, 2021 at 01:26:23PM +0100, Dafna Hirschfeld wrote: > The IPA of rkisp1 relay some of the camera's controls. > Therefore it can't work if those controls don't exist. > Return a negative int in that case. > Also return error from the 'configure' method of the pipeline > handler if the IPA configure fails. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> Looks good to me. Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > include/libcamera/ipa/rkisp1.mojom | 2 +- > src/ipa/rkisp1/rkisp1.cpp | 11 ++++++----- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > 3 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > index 9270f9c7..95fa0d93 100644 > --- a/include/libcamera/ipa/rkisp1.mojom > +++ b/include/libcamera/ipa/rkisp1.mojom > @@ -31,7 +31,7 @@ interface IPARkISP1Interface { > > configure(CameraSensorInfo sensorInfo, > map<uint32, IPAStream> streamConfig, > - map<uint32, ControlInfoMap> entityControls) => (); > + map<uint32, ControlInfoMap> entityControls) => (int32 ret); > > mapBuffers(array<IPABuffer> buffers); > unmapBuffers(array<uint32> ids); > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 67bac986..b0641faf 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -38,7 +38,7 @@ public: > int start() override { return 0; } > void stop() override {} > > - void configure(const CameraSensorInfo &info, > + int configure(const CameraSensorInfo &info, > const std::map<uint32_t, IPAStream> &streamConfig, > const std::map<uint32_t, ControlInfoMap> &entityControls) override; > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > @@ -75,25 +75,25 @@ private: > * assemble one. Make sure the reported sensor information are relevant > * before accessing them. > */ > -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, > +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, > [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig, > const std::map<uint32_t, ControlInfoMap> &entityControls) > { > if (entityControls.empty()) > - return; > + return -1; > > ctrls_ = entityControls.at(0); > > const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); > if (itExp == ctrls_.end()) { > LOG(IPARkISP1, Error) << "Can't find exposure control"; > - return; > + return -1; > } > > const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); > if (itGain == ctrls_.end()) { > LOG(IPARkISP1, Error) << "Can't find gain control"; > - return; > + return -1; > } > > autoExposure_ = true; > @@ -111,6 +111,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, > << " Gain: " << minGain_ << "-" << maxGain_; > > setControls(0); > + return 0; > } > > void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 1dd878fc..dad8e5d4 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -653,8 +653,11 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > std::map<uint32_t, ControlInfoMap> entityControls; > entityControls.emplace(0, data->sensor_->controls()); > > - data->ipa_->configure(sensorInfo, streamConfig, entityControls); > - > + ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); > + if (ret) { > + LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")"; > + return ret; > + } > return 0; > } > > -- > 2.17.1 >
diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom index 9270f9c7..95fa0d93 100644 --- a/include/libcamera/ipa/rkisp1.mojom +++ b/include/libcamera/ipa/rkisp1.mojom @@ -31,7 +31,7 @@ interface IPARkISP1Interface { configure(CameraSensorInfo sensorInfo, map<uint32, IPAStream> streamConfig, - map<uint32, ControlInfoMap> entityControls) => (); + map<uint32, ControlInfoMap> entityControls) => (int32 ret); mapBuffers(array<IPABuffer> buffers); unmapBuffers(array<uint32> ids); diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 67bac986..b0641faf 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -38,7 +38,7 @@ public: int start() override { return 0; } void stop() override {} - void configure(const CameraSensorInfo &info, + int configure(const CameraSensorInfo &info, const std::map<uint32_t, IPAStream> &streamConfig, const std::map<uint32_t, ControlInfoMap> &entityControls) override; void mapBuffers(const std::vector<IPABuffer> &buffers) override; @@ -75,25 +75,25 @@ private: * assemble one. Make sure the reported sensor information are relevant * before accessing them. */ -void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, +int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, [[maybe_unused]] const std::map<uint32_t, IPAStream> &streamConfig, const std::map<uint32_t, ControlInfoMap> &entityControls) { if (entityControls.empty()) - return; + return -1; ctrls_ = entityControls.at(0); const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); if (itExp == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find exposure control"; - return; + return -1; } const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); if (itGain == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find gain control"; - return; + return -1; } autoExposure_ = true; @@ -111,6 +111,7 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, << " Gain: " << minGain_ << "-" << maxGain_; setControls(0); + return 0; } void IPARkISP1::mapBuffers(const std::vector<IPABuffer> &buffers) diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 1dd878fc..dad8e5d4 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -653,8 +653,11 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) std::map<uint32_t, ControlInfoMap> entityControls; entityControls.emplace(0, data->sensor_->controls()); - data->ipa_->configure(sensorInfo, streamConfig, entityControls); - + ret = data->ipa_->configure(sensorInfo, streamConfig, entityControls); + if (ret) { + LOG(RkISP1, Error) << "failed configuring IPA (" << ret << ")"; + return ret; + } return 0; }
The IPA of rkisp1 relay some of the camera's controls. Therefore it can't work if those controls don't exist. Return a negative int in that case. Also return error from the 'configure' method of the pipeline handler if the IPA configure fails. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- include/libcamera/ipa/rkisp1.mojom | 2 +- src/ipa/rkisp1/rkisp1.cpp | 11 ++++++----- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- 3 files changed, 12 insertions(+), 8 deletions(-)