| Message ID | 20210309063829.8710-4-dafna.hirschfeld@collabora.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Dafna, Thank you for the patch. On Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote: > In kernel 5.11 the rkisp1 uapi had changed to support > different hardware revisions. Currently only revision 10 > is supported by the rkisp1 IPA and therefore 'init' > should fail if the revision is not 10. I think it's important here to state that this change will require a kernel upgrade to v5.11 or newer. Maybe as follow ? This changes depends on the kernel driver reporting the hardware revision, and thus requires the rkisp1 driver from v5.11 or newer. > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > include/libcamera/ipa/rkisp1.mojom | 2 +- > src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++---- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------ > 3 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > index 95fa0d93..29f726e1 100644 > --- a/include/libcamera/ipa/rkisp1.mojom > +++ b/include/libcamera/ipa/rkisp1.mojom > @@ -25,7 +25,7 @@ struct RkISP1Action { > }; > > interface IPARkISP1Interface { > - init(IPASettings settings) => (int32 ret); > + init(uint32 hwRevision) => (int32 ret); > start() => (int32 ret); > stop(); > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 0b0f31e4..197c2389 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1) > class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface > { > public: > - int init([[maybe_unused]] const IPASettings &settings) override > - { > - return 0; > - } > + int init(unsigned int hwRevision) override; I expect we'll need IPA settings in the future, but that's fine, we can add it back later when/if needed. > int start() override { return 0; } > void stop() override {} > > @@ -69,6 +66,18 @@ private: > uint32_t maxGain_; > }; > > +int IPARkISP1::init(unsigned int hwRevision) > +{ > + /* todo add support for other revisions */ This should be \todo. > + if (hwRevision != RKISP1_V10) { > + LOG(IPARkISP1, Error) << "Hardware version " << hwRevision << s/version/revision/ ? > + " is currently not supported"; Here's how we usually format multi-line log statements: LOG(IPARkISP1, Error) << "Hardware version " << hwRevision << " is currently not supported"; > + return -ENODEV; > + } > + LOG(IPARkISP1, Info) << "Hardware revision is " << hwRevision; I'd turn this into a debug message, as we support a single revision, it's not very important information at this point. > + return 0; > +} > +; No need for a semicolon here. > /** > * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo > * if the connected sensor does not provide enough information to properly > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 34814f62..24c622a8 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -85,7 +85,7 @@ public: > { > } > > - int loadIPA(); > + int loadIPA(unsigned int hwRevision); > > Stream mainPathStream_; > Stream selfPathStream_; > @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > return nullptr; > } > > -int RkISP1CameraData::loadIPA() > +int RkISP1CameraData::loadIPA(unsigned int hwRevision) > { > ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > if (!ipa_) > @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA() > ipa_->queueFrameAction.connect(this, > &RkISP1CameraData::queueFrameAction); > > - ipa_->init(IPASettings{}); > - > - return 0; > + return ipa_->init(hwRevision); How about adding an error message here ? int ret = ipa_->init(hwRevision); if (ret < 0) { LOG(RkISP1, Error) << "IPA initialization failure"; return ret; } return 0; Otherwise the pipeline handler will fail silently, and it may be hard to debug the problem. > } > > void RkISP1CameraData::queueFrameAction(unsigned int frame, > @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > isp_->frameStart.connect(data->delayedCtrls_.get(), > &DelayedControls::applyControls); > > - ret = data->loadIPA(); > + ret = data->loadIPA(media_->hwRevision()); > if (ret) > return ret; > I've thought a bit more about version checks, and I'd propose adding if (!media_->hwRevision()) { LOG(RkISP1, Error) << "The rkisp1 driver is too old, v5.11 or newer is required"; return false; } to the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1 driver is too old (and v5.11 being quite recent, we will have developers running into this problem), the error messages that will be printed won't give a clear indication of how to solve the issue. What do you think ? With these small issues addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> If you agree with all the comments, you can just let me know, and I'll make changes when applying, there's no need to resubmit.
Hi, On 09.03.21 23:39, Laurent Pinchart wrote: > Hi Dafna, > > Thank you for the patch. > > On Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote: >> In kernel 5.11 the rkisp1 uapi had changed to support >> different hardware revisions. Currently only revision 10 >> is supported by the rkisp1 IPA and therefore 'init' >> should fail if the revision is not 10. > > I think it's important here to state that this change will require a > kernel upgrade to v5.11 or newer. Maybe as follow ? > > This changes depends on the kernel driver reporting the hardware > revision, and thus requires the rkisp1 driver from v5.11 or newer. > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> include/libcamera/ipa/rkisp1.mojom | 2 +- >> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++---- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------ >> 3 files changed, 18 insertions(+), 11 deletions(-) >> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom >> index 95fa0d93..29f726e1 100644 >> --- a/include/libcamera/ipa/rkisp1.mojom >> +++ b/include/libcamera/ipa/rkisp1.mojom >> @@ -25,7 +25,7 @@ struct RkISP1Action { >> }; >> >> interface IPARkISP1Interface { >> - init(IPASettings settings) => (int32 ret); >> + init(uint32 hwRevision) => (int32 ret); >> start() => (int32 ret); >> stop(); >> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index 0b0f31e4..197c2389 100644 >> --- a/src/ipa/rkisp1/rkisp1.cpp >> +++ b/src/ipa/rkisp1/rkisp1.cpp >> @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1) >> class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface >> { >> public: >> - int init([[maybe_unused]] const IPASettings &settings) override >> - { >> - return 0; >> - } >> + int init(unsigned int hwRevision) override; > > I expect we'll need IPA settings in the future, but that's fine, we can > add it back later when/if needed. > >> int start() override { return 0; } >> void stop() override {} >> >> @@ -69,6 +66,18 @@ private: >> uint32_t maxGain_; >> }; >> >> +int IPARkISP1::init(unsigned int hwRevision) >> +{ >> + /* todo add support for other revisions */ > > This should be \todo. > >> + if (hwRevision != RKISP1_V10) { >> + LOG(IPARkISP1, Error) << "Hardware version " << hwRevision << > > s/version/revision/ ? > >> + " is currently not supported"; > > Here's how we usually format multi-line log statements: > > LOG(IPARkISP1, Error) > << "Hardware version " << hwRevision > << " is currently not supported"; > >> + return -ENODEV; >> + } >> + LOG(IPARkISP1, Info) << "Hardware revision is " << hwRevision; > > I'd turn this into a debug message, as we support a single revision, > it's not very important information at this point. > >> + return 0; >> +} >> +; > > No need for a semicolon here. > >> /** >> * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo >> * if the connected sensor does not provide enough information to properly >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> index 34814f62..24c622a8 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >> @@ -85,7 +85,7 @@ public: >> { >> } >> >> - int loadIPA(); >> + int loadIPA(unsigned int hwRevision); >> >> Stream mainPathStream_; >> Stream selfPathStream_; >> @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) >> return nullptr; >> } >> >> -int RkISP1CameraData::loadIPA() >> +int RkISP1CameraData::loadIPA(unsigned int hwRevision) >> { >> ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); >> if (!ipa_) >> @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA() >> ipa_->queueFrameAction.connect(this, >> &RkISP1CameraData::queueFrameAction); >> >> - ipa_->init(IPASettings{}); >> - >> - return 0; >> + return ipa_->init(hwRevision); > > How about adding an error message here ? > > int ret = ipa_->init(hwRevision); > if (ret < 0) { > LOG(RkISP1, Error) << "IPA initialization failure"; > return ret; > } > > return 0; > > Otherwise the pipeline handler will fail silently, and it may be hard to > debug the problem. > >> } >> >> void RkISP1CameraData::queueFrameAction(unsigned int frame, >> @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) >> isp_->frameStart.connect(data->delayedCtrls_.get(), >> &DelayedControls::applyControls); >> >> - ret = data->loadIPA(); >> + ret = data->loadIPA(media_->hwRevision()); >> if (ret) >> return ret; >> > > I've thought a bit more about version checks, and I'd propose adding > > if (!media_->hwRevision()) { > LOG(RkISP1, Error) > << "The rkisp1 driver is too old, v5.11 or newer is required"; > return false; > } > > to the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1 > driver is too old (and v5.11 being quite recent, we will have developers > running into this problem), the error messages that will be printed > won't give a clear indication of how to solve the issue. What do you > think ? I don't mind adding this check. The rkisp1 was a in staging until v5.11 so probably didn't have much users. > > With these small issues addressed, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > If you agree with all the comments, you can just let me know, and I'll > make changes when applying, there's no need to resubmit. okay, I agree to the comments, thank:), you mean the whole patchset? Thanks, Dafna >
Hi Dafna, On Wed, Mar 10, 2021 at 07:47:49AM +0100, Dafna Hirschfeld wrote: > On 09.03.21 23:39, Laurent Pinchart wrote: > > On Tue, Mar 09, 2021 at 07:38:29AM +0100, Dafna Hirschfeld wrote: > >> In kernel 5.11 the rkisp1 uapi had changed to support > >> different hardware revisions. Currently only revision 10 > >> is supported by the rkisp1 IPA and therefore 'init' > >> should fail if the revision is not 10. > > > > I think it's important here to state that this change will require a > > kernel upgrade to v5.11 or newer. Maybe as follow ? > > > > This changes depends on the kernel driver reporting the hardware > > revision, and thus requires the rkisp1 driver from v5.11 or newer. > > > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> --- > >> include/libcamera/ipa/rkisp1.mojom | 2 +- > >> src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++---- > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------ > >> 3 files changed, 18 insertions(+), 11 deletions(-) > >> > >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > >> index 95fa0d93..29f726e1 100644 > >> --- a/include/libcamera/ipa/rkisp1.mojom > >> +++ b/include/libcamera/ipa/rkisp1.mojom > >> @@ -25,7 +25,7 @@ struct RkISP1Action { > >> }; > >> > >> interface IPARkISP1Interface { > >> - init(IPASettings settings) => (int32 ret); > >> + init(uint32 hwRevision) => (int32 ret); > >> start() => (int32 ret); > >> stop(); > >> > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >> index 0b0f31e4..197c2389 100644 > >> --- a/src/ipa/rkisp1/rkisp1.cpp > >> +++ b/src/ipa/rkisp1/rkisp1.cpp > >> @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1) > >> class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface > >> { > >> public: > >> - int init([[maybe_unused]] const IPASettings &settings) override > >> - { > >> - return 0; > >> - } > >> + int init(unsigned int hwRevision) override; > > > > I expect we'll need IPA settings in the future, but that's fine, we can > > add it back later when/if needed. > > > >> int start() override { return 0; } > >> void stop() override {} > >> > >> @@ -69,6 +66,18 @@ private: > >> uint32_t maxGain_; > >> }; > >> > >> +int IPARkISP1::init(unsigned int hwRevision) > >> +{ > >> + /* todo add support for other revisions */ > > > > This should be \todo. > > > >> + if (hwRevision != RKISP1_V10) { > >> + LOG(IPARkISP1, Error) << "Hardware version " << hwRevision << > > > > s/version/revision/ ? > > > >> + " is currently not supported"; > > > > Here's how we usually format multi-line log statements: > > > > LOG(IPARkISP1, Error) > > << "Hardware version " << hwRevision > > << " is currently not supported"; > > > >> + return -ENODEV; > >> + } > >> + LOG(IPARkISP1, Info) << "Hardware revision is " << hwRevision; > > > > I'd turn this into a debug message, as we support a single revision, > > it's not very important information at this point. > > > >> + return 0; > >> +} > >> +; > > > > No need for a semicolon here. > > > >> /** > >> * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo > >> * if the connected sensor does not provide enough information to properly > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> index 34814f62..24c622a8 100644 > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >> @@ -85,7 +85,7 @@ public: > >> { > >> } > >> > >> - int loadIPA(); > >> + int loadIPA(unsigned int hwRevision); > >> > >> Stream mainPathStream_; > >> Stream selfPathStream_; > >> @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) > >> return nullptr; > >> } > >> > >> -int RkISP1CameraData::loadIPA() > >> +int RkISP1CameraData::loadIPA(unsigned int hwRevision) > >> { > >> ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); > >> if (!ipa_) > >> @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA() > >> ipa_->queueFrameAction.connect(this, > >> &RkISP1CameraData::queueFrameAction); > >> > >> - ipa_->init(IPASettings{}); > >> - > >> - return 0; > >> + return ipa_->init(hwRevision); > > > > How about adding an error message here ? > > > > int ret = ipa_->init(hwRevision); > > if (ret < 0) { > > LOG(RkISP1, Error) << "IPA initialization failure"; > > return ret; > > } > > > > return 0; > > > > Otherwise the pipeline handler will fail silently, and it may be hard to > > debug the problem. > > > >> } > >> > >> void RkISP1CameraData::queueFrameAction(unsigned int frame, > >> @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > >> isp_->frameStart.connect(data->delayedCtrls_.get(), > >> &DelayedControls::applyControls); > >> > >> - ret = data->loadIPA(); > >> + ret = data->loadIPA(media_->hwRevision()); > >> if (ret) > >> return ret; > >> > > > > I've thought a bit more about version checks, and I'd propose adding > > > > if (!media_->hwRevision()) { > > LOG(RkISP1, Error) > > << "The rkisp1 driver is too old, v5.11 or newer is required"; > > return false; > > } > > > > to the PipelineHandlerRkISP1::match() function. Otherwise, if the rkisp1 > > driver is too old (and v5.11 being quite recent, we will have developers > > running into this problem), the error messages that will be printed > > won't give a clear indication of how to solve the issue. What do you > > think ? > > I don't mind adding this check. The rkisp1 was a in staging until > v5.11 so probably didn't have much users. That's true, but even if there are few users, Kieran would scold me for not taking good care of them :-) > > With these small issues addressed, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > If you agree with all the comments, you can just let me know, and I'll > > make changes when applying, there's no need to resubmit. > > okay, I agree to the comments, thank:), you mean the whole patchset? Yes. I'll test it today, and then apply.
diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom index 95fa0d93..29f726e1 100644 --- a/include/libcamera/ipa/rkisp1.mojom +++ b/include/libcamera/ipa/rkisp1.mojom @@ -25,7 +25,7 @@ struct RkISP1Action { }; interface IPARkISP1Interface { - init(IPASettings settings) => (int32 ret); + init(uint32 hwRevision) => (int32 ret); start() => (int32 ret); stop(); diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 0b0f31e4..197c2389 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -31,10 +31,7 @@ LOG_DEFINE_CATEGORY(IPARkISP1) class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface { public: - int init([[maybe_unused]] const IPASettings &settings) override - { - return 0; - } + int init(unsigned int hwRevision) override; int start() override { return 0; } void stop() override {} @@ -69,6 +66,18 @@ private: uint32_t maxGain_; }; +int IPARkISP1::init(unsigned int hwRevision) +{ + /* todo add support for other revisions */ + if (hwRevision != RKISP1_V10) { + LOG(IPARkISP1, Error) << "Hardware version " << hwRevision << + " is currently not supported"; + return -ENODEV; + } + LOG(IPARkISP1, Info) << "Hardware revision is " << hwRevision; + return 0; +} +; /** * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo * if the connected sensor does not provide enough information to properly diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 34814f62..24c622a8 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -85,7 +85,7 @@ public: { } - int loadIPA(); + int loadIPA(unsigned int hwRevision); Stream mainPathStream_; Stream selfPathStream_; @@ -300,7 +300,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request) return nullptr; } -int RkISP1CameraData::loadIPA() +int RkISP1CameraData::loadIPA(unsigned int hwRevision) { ipa_ = IPAManager::createIPA<ipa::rkisp1::IPAProxyRkISP1>(pipe_, 1, 1); if (!ipa_) @@ -309,9 +309,7 @@ int RkISP1CameraData::loadIPA() ipa_->queueFrameAction.connect(this, &RkISP1CameraData::queueFrameAction); - ipa_->init(IPASettings{}); - - return 0; + return ipa_->init(hwRevision); } void RkISP1CameraData::queueFrameAction(unsigned int frame, @@ -952,7 +950,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) isp_->frameStart.connect(data->delayedCtrls_.get(), &DelayedControls::applyControls); - ret = data->loadIPA(); + ret = data->loadIPA(media_->hwRevision()); if (ret) return ret;
In kernel 5.11 the rkisp1 uapi had changed to support different hardware revisions. Currently only revision 10 is supported by the rkisp1 IPA and therefore 'init' should fail if the revision is not 10. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- include/libcamera/ipa/rkisp1.mojom | 2 +- src/ipa/rkisp1/rkisp1.cpp | 17 +++++++++++++---- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++------ 3 files changed, 18 insertions(+), 11 deletions(-)