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

Message ID 20210309063829.8710-4-dafna.hirschfeld@collabora.com
State Accepted
Headers show
Series
  • rkisp1: add an initial support to different hw revision
Related show

Commit Message

Dafna Hirschfeld March 9, 2021, 6:38 a.m. UTC
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(-)

Comments

Laurent Pinchart March 9, 2021, 10:39 p.m. UTC | #1
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.
Dafna Hirschfeld March 10, 2021, 6:47 a.m. UTC | #2
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
>
Laurent Pinchart March 10, 2021, 8:32 a.m. UTC | #3
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.

Patch
diff mbox series

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;