[libcamera-devel] [PATCH] ipa: rkisp1: Do not set controls during configure

[Sebastian Fricke]

Hey Laurent,

On 26.03.2021 05:04, Laurent Pinchart wrote:
>Hi Sebastian,
>
>One more comment.
>
>On Fri, Mar 26, 2021 at 05:00:46AM +0200, Laurent Pinchart wrote:
>> On Thu, Mar 25, 2021 at 06:42:54PM +0100, Sebastian Fricke wrote:
>> > The configure operation is synchronous and should not send events back
>> > to the pipeline handler.
>> >
>> > If information needs to be returned from configure it should be handled
>> > through the interface directly.
>> >
>> > Move the initial call to setControls() out of configure() and into the
>> > start() method which is called after the IPA running_ state is updated.
>> >
>> > Signed-off-by: Sebastian Fricke <sebastian.fricke at posteo.net>
>> > ---
>> >  src/ipa/rkisp1/rkisp1.cpp | 10 ++++++++--
>> >  1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> > index d2a10bb9..b0698903 100644
>> > --- a/src/ipa/rkisp1/rkisp1.cpp
>> > +++ b/src/ipa/rkisp1/rkisp1.cpp
>> > @@ -32,7 +32,7 @@ class IPARkISP1 : public ipa::rkisp1::IPARkISP1Interface
>> >  {
>> >  public:
>> >  	int init(unsigned int hwRevision) override;
>> > -	int start() override { return 0; }
>> > +	int start() override;
>> >  	void stop() override {}
>> >
>> >  	int configure(const CameraSensorInfo &info,
>> > @@ -80,6 +80,13 @@ int IPARkISP1::init(unsigned int hwRevision)
>> >  	return 0;
>> >  }
>> >
>> > +int IPARkISP1::start()
>> > +{
>> > +        setControls(0);
>
>Tabs for indentation. Didn't checkstyle.py warn you ?

Ah... I have set up a new image on my board and forgot to insert the git
hooks. Sorry ... I will fix that right away. I should probably add that
to my ansible playbook, so that the hook is set up automatically.

>
>>
>> The IPA will send the queueFrameAction event to the pipeline handler
>> before the start() function completes. This will cause different
>> behaviours in the isolated and non-isolated cases.
>>
>> In the isolated case, the message corresponding to queueFrameAction will
>> be send to the pipeline handler before the response to start(), and will
>> thus be processed first. In the non-isolated case, the deferred call
>> related to queueFrameAction will be queued to the pipeline handler
>> thread, but will only be processed once control returns to the event
>> loop, which will happen after start() completes.
>>
>> I'm OK with this patch as a short term fix, as it fixes a real issue and
>> we only run the rkisp1 IPA without isolation at this point.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> We however need to figure out a correct fix for the IPC case. Paul,
>> what's your opinion on this, how should we handle it ?
>>
>> > +
>> > +        return 0;
>> > +}
>> > +
>> >  /**
>> >   * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo
>> >   * if the connected sensor does not provide enough information to properly
>> > @@ -121,7 +128,6 @@ int IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info,
>> >  		<< "Exposure: " << minExposure_ << "-" << maxExposure_
>> >  		<< " Gain: " << minGain_ << "-" << maxGain_;
>> >
>> > -	setControls(0);
>> >  	return 0;
>> >  }
>> >
>
>-- 
>Regards,
>
>Laurent Pinchart