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

[Laurent Pinchart]

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

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.

-- 
Regards,

Laurent Pinchart