[libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if hw revision is not RKISP1_V10

[Laurent Pinchart]

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 at 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 at 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.

-- 
Regards,

Laurent Pinchart