[libcamera-devel] [PATCH v3 3/3] ipa: rkisp1: fail on init if hw revision is not RKISP1_V10
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Wed Mar 10 07:47:49 CET 2021
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 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.
>
> 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?
Thanks,
Dafna
>
More information about the libcamera-devel
mailing list