[libcamera-devel,v3,2/3] src: rkisp1: return error from ipa's configure method if it fails
diff mbox series

Message ID 20210309063829.8710-3-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 
The IPA of rkisp1 relies on some of the camera's controls.
Therefore it can't work if those controls are not given.
Return -EINVAL from 'configure' in that case.
Also return error from the pipeline's 'configure' method
if the IPA configure fails.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Laurent Pinchart March 9, 2021, 10:24 p.m. UTC | #1 
Hi Dafna,

Thank you for the patch.

The subject line should start with "ipa: rkisp1: ...".

On Tue, Mar 09, 2021 at 07:38:28AM +0100, Dafna Hirschfeld wrote:
> The IPA of rkisp1 relies on some of the camera's controls.
> Therefore it can't work if those controls are not given.
> Return -EINVAL from 'configure' in that case.
> Also return error from the pipeline's 'configure' method
> if the IPA configure fails.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 f11aeb40..0b0f31e4 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;

Indentation should be adjusted here.

>  	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)

And here too.

>  {
>  	if (entityControls.empty())
> -		return;
> +		return -EINVAL;
>  
>  	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 -EINVAL;
>  	}
>  
>  	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>  	if (itGain == ctrls_.end()) {
>  		LOG(IPARkISP1, Error) << "Can't find gain control";
> -		return;
> +		return -EINVAL;
>  	}
>  
>  	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 538c0139..34814f62 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;
>  }
>
Dafna Hirschfeld March 10, 2021, 6:36 a.m. UTC | #2 
On 09.03.21 23:24, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> The subject line should start with "ipa: rkisp1: ...".

I changed both the ipa and the pipeline in this patch so
I thought just 'ipa: rkisp1:' is wrong.

thanks,
Dafna

> 
> On Tue, Mar 09, 2021 at 07:38:28AM +0100, Dafna Hirschfeld wrote:
>> The IPA of rkisp1 relies on some of the camera's controls.
>> Therefore it can't work if those controls are not given.
>> Return -EINVAL from 'configure' in that case.
>> Also return error from the pipeline's 'configure' method
>> if the IPA configure fails.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 f11aeb40..0b0f31e4 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;
> 
> Indentation should be adjusted here.
> 
>>   	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)
> 
> And here too.
> 
>>   {
>>   	if (entityControls.empty())
>> -		return;
>> +		return -EINVAL;
>>   
>>   	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 -EINVAL;
>>   	}
>>   
>>   	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
>>   	if (itGain == ctrls_.end()) {
>>   		LOG(IPARkISP1, Error) << "Can't find gain control";
>> -		return;
>> +		return -EINVAL;
>>   	}
>>   
>>   	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 538c0139..34814f62 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;
>>   }
>>   
>
Laurent Pinchart March 10, 2021, 8:30 a.m. UTC | #3 
Hi Dafna,

On Wed, Mar 10, 2021 at 07:36:11AM +0100, Dafna Hirschfeld wrote:
> On 09.03.21 23:24, Laurent Pinchart wrote:
> > Hi Dafna,
> > 
> > Thank you for the patch.
> > 
> > The subject line should start with "ipa: rkisp1: ...".
> 
> I changed both the ipa and the pipeline in this patch so
> I thought just 'ipa: rkisp1:' is wrong.

Right, as the patch touches both sides, there's no clear cut. Picking
the side that we consider most relevant is one option. We don't
usually have a "src:" prefix in commit messages, but it's no big deal,
it would be fine too.

> > On Tue, Mar 09, 2021 at 07:38:28AM +0100, Dafna Hirschfeld wrote:
> >> The IPA of rkisp1 relies on some of the camera's controls.
> >> Therefore it can't work if those controls are not given.
> >> Return -EINVAL from 'configure' in that case.
> >> Also return error from the pipeline's 'configure' method
> >> if the IPA configure fails.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@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 f11aeb40..0b0f31e4 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;
> > 
> > Indentation should be adjusted here.
> > 
> >>   	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)
> > 
> > And here too.
> > 
> >>   {
> >>   	if (entityControls.empty())
> >> -		return;
> >> +		return -EINVAL;
> >>   
> >>   	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 -EINVAL;
> >>   	}
> >>   
> >>   	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
> >>   	if (itGain == ctrls_.end()) {
> >>   		LOG(IPARkISP1, Error) << "Can't find gain control";
> >> -		return;
> >> +		return -EINVAL;
> >>   	}
> >>   
> >>   	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 538c0139..34814f62 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;
> >>   }
> >>

Patch
diff mbox series 

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 f11aeb40..0b0f31e4 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 -EINVAL;
 
 	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 -EINVAL;
 	}
 
 	const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN);
 	if (itGain == ctrls_.end()) {
 		LOG(IPARkISP1, Error) << "Can't find gain control";
-		return;
+		return -EINVAL;
 	}
 
 	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 538c0139..34814f62 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;
 }