[libcamera-devel] ipa: rkisp1: Do not set controls during configure
diff mbox series

Message ID 20210325174254.7977-1-sebastian.fricke@posteo.net
State Accepted
Headers show
Series
  • [libcamera-devel] ipa: rkisp1: Do not set controls during configure
Related show

Commit Message

Sebastian Fricke March 25, 2021, 5:42 p.m. UTC 
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@posteo.net>
---
 src/ipa/rkisp1/rkisp1.cpp | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Paul Elder March 26, 2021, 2:22 a.m. UTC | #1 
Hi Sebastian,

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.

The reason why the events shouldn't be sent from the IPA to the pipeline
handler is not because configure is synchronous, it's because start()
hasn't been called yet, so the IPA thread hasn't started running yet.

It just happens that all calls before start() are synchronous, and all
calls after start() are asynchronous, so the rules regarding events also
line up :)

> 
> If information needs to be returned from configure it should be handled
> through the interface directly.

I think s/handled through the interface/returned/ ?

> 
> 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@posteo.net>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  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);
> +
> +        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;
>  }
>  
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart March 26, 2021, 3 a.m. UTC | #2 
Hi Sebastian,

Thank you for the patch.

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

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@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;
>  }
>
Laurent Pinchart March 26, 2021, 3:04 a.m. UTC | #3 
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@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 ?

> 
> 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@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;
> >  }
> >
Paul Elder March 26, 2021, 3:19 a.m. UTC | #4 
Hi Laurent,

On Fri, Mar 26, 2021 at 05:00:45AM +0200, Laurent Pinchart wrote:
> Hi Sebastian,
> 
> Thank you for the patch.
> 
> 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@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);
> 
> 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@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 ?

As we discussed on irc, I think a good solution would be to disallow
emitting signals from start(). We've allowed custom return values from
start(), which competes with emitting signals from start(). So I think
if somebody wants to emit signals from the IPA during start(), they
should instead return the values directly, and then the pipeline handler
can call the signal handler on the return values (or partially
reimplment the signal handler).

I guess I'll have to add this to the IPA guide and we'll have to add
some more assertions (with a Starting state?).


Thanks,

Paul

> > +
> > +        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;
> >  }
> >
Sebastian Fricke March 26, 2021, 4:58 a.m. UTC | #5 
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@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@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
Kieran Bingham March 26, 2021, 4:36 p.m. UTC | #6 
On 25/03/2021 17:42, 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@posteo.net>

Given that this matches the IPU3 implementation, and it's known that we
need to update this soon, I think this should be merged.

With the whitespace issues corrected.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Niklas, as this has just hit you, could you apply this and fix the
whitespace and apply after testing please?


> ---
>  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);
> +
> +        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;
>  }
>  
>

Patch
diff mbox series 

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);
+
+        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;
 }