[v6,11/18] libcamera: software_isp: Call Algorithm::prepare
diff mbox series

Message ID 20240906120927.4071508-12-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP refactoring
Related show

Commit Message

Milan Zamazal Sept. 6, 2024, 12:09 p.m. UTC 
This patch adds Algorithm::prepare call for the defined algorithms.
This is preparation only since there are currently no Algorithm based
algorithms defined.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/ipa/soft.mojom            | 1 +
 src/ipa/simple/soft_simple.cpp              | 8 ++++++++
 src/libcamera/software_isp/software_isp.cpp | 1 +
 3 files changed, 10 insertions(+)

Comments

Dan Scally Sept. 9, 2024, 8:21 a.m. UTC | #1 
Hi Milan

On 06/09/2024 13:09, Milan Zamazal wrote:
> This patch adds Algorithm::prepare call for the defined algorithms.
> This is preparation only since there are currently no Algorithm based
> algorithms defined.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   include/libcamera/ipa/soft.mojom            | 1 +
>   src/ipa/simple/soft_simple.cpp              | 8 ++++++++
>   src/libcamera/software_isp/software_isp.cpp | 1 +
>   3 files changed, 10 insertions(+)
>
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index ddccd154..347fd69b 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -24,6 +24,7 @@ interface IPASoftInterface {
>   		=> (int32 ret);
>   
>   	[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
> +	[async] fillParamsBuffer(uint32 frame);
>   	[async] processStats(uint32 frame,
>   			     uint32 bufferId,
>   			     libcamera.ControlList sensorControls);
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index eb3bbd92..3a0cb6e0 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -79,6 +79,7 @@ public:
>   	void stop() override;
>   
>   	void queueRequest(const uint32_t frame, const ControlList &controls) override;
> +	void fillParamsBuffer(const uint32_t frame) override;
>   	void processStats(const uint32_t frame, const uint32_t bufferId,
>   			  const ControlList &sensorControls) override;
>   
> @@ -279,6 +280,13 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
>   		algo->queueRequest(context_, frame, frameContext, controls);
>   }
>   
> +void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
> +{
> +	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> +	for (auto const &algo : algorithms())
> +		algo->prepare(context_, frame, frameContext, params_);
> +}
> +
>   void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
>   				 [[maybe_unused]] const uint32_t bufferId,
>   				 const ControlList &sensorControls)
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index eda18947..564e44e8 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -352,6 +352,7 @@ void SoftwareIsp::stop()
>    */
>   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>   {
> +	ipa_->fillParamsBuffer(frame);
>   	debayer_->invokeMethod(&DebayerCpu::process,
>   			       ConnectionTypeQueued, frame, input, output, debayerParams_);
>   }
Kieran Bingham Sept. 9, 2024, 8:09 p.m. UTC | #2 
Quoting Dan Scally (2024-09-09 09:21:45)
> Hi Milan
> 
> On 06/09/2024 13:09, Milan Zamazal wrote:
> > This patch adds Algorithm::prepare call for the defined algorithms.
> > This is preparation only since there are currently no Algorithm based
> > algorithms defined.
> >
> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> >   include/libcamera/ipa/soft.mojom            | 1 +
> >   src/ipa/simple/soft_simple.cpp              | 8 ++++++++
> >   src/libcamera/software_isp/software_isp.cpp | 1 +
> >   3 files changed, 10 insertions(+)
> >
> > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> > index ddccd154..347fd69b 100644
> > --- a/include/libcamera/ipa/soft.mojom
> > +++ b/include/libcamera/ipa/soft.mojom
> > @@ -24,6 +24,7 @@ interface IPASoftInterface {
> >               => (int32 ret);
> >   
> >       [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
> > +     [async] fillParamsBuffer(uint32 frame);
> >       [async] processStats(uint32 frame,
> >                            uint32 bufferId,
> >                            libcamera.ControlList sensorControls);
> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > index eb3bbd92..3a0cb6e0 100644
> > --- a/src/ipa/simple/soft_simple.cpp
> > +++ b/src/ipa/simple/soft_simple.cpp
> > @@ -79,6 +79,7 @@ public:
> >       void stop() override;
> >   
> >       void queueRequest(const uint32_t frame, const ControlList &controls) override;
> > +     void fillParamsBuffer(const uint32_t frame) override;
> >       void processStats(const uint32_t frame, const uint32_t bufferId,
> >                         const ControlList &sensorControls) override;
> >   
> > @@ -279,6 +280,13 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
> >               algo->queueRequest(context_, frame, frameContext, controls);
> >   }
> >   
> > +void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
> > +{
> > +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > +     for (auto const &algo : algorithms())
> > +             algo->prepare(context_, frame, frameContext, params_);

This doesn't fill a parameter buffer. Do we anticipate having a
structured buffer to pass to the SoftISP components?

Otherwise perhaps we should just call this
	'IPASoftSimple::prepare(const uint32_t frame)'


> > +}
> > +
> >   void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
> >                                [[maybe_unused]] const uint32_t bufferId,
> >                                const ControlList &sensorControls)
> > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> > index eda18947..564e44e8 100644
> > --- a/src/libcamera/software_isp/software_isp.cpp
> > +++ b/src/libcamera/software_isp/software_isp.cpp
> > @@ -352,6 +352,7 @@ void SoftwareIsp::stop()
> >    */
> >   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
> >   {
> > +     ipa_->fillParamsBuffer(frame);

Oh... are we mixing prepare and process calls?

I'll have to take a look at the bigger picture ....

> >       debayer_->invokeMethod(&DebayerCpu::process,
> >                              ConnectionTypeQueued, frame, input, output, debayerParams_);
> >   }
Kieran Bingham Sept. 9, 2024, 8:32 p.m. UTC | #3 
Quoting Kieran Bingham (2024-09-09 21:09:20)
> Quoting Dan Scally (2024-09-09 09:21:45)
> > Hi Milan
> > 
> > On 06/09/2024 13:09, Milan Zamazal wrote:
> > > This patch adds Algorithm::prepare call for the defined algorithms.
> > > This is preparation only since there are currently no Algorithm based
> > > algorithms defined.
> > >
> > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > 
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > >   include/libcamera/ipa/soft.mojom            | 1 +
> > >   src/ipa/simple/soft_simple.cpp              | 8 ++++++++
> > >   src/libcamera/software_isp/software_isp.cpp | 1 +
> > >   3 files changed, 10 insertions(+)
> > >
> > > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> > > index ddccd154..347fd69b 100644
> > > --- a/include/libcamera/ipa/soft.mojom
> > > +++ b/include/libcamera/ipa/soft.mojom
> > > @@ -24,6 +24,7 @@ interface IPASoftInterface {
> > >               => (int32 ret);
> > >   
> > >       [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
> > > +     [async] fillParamsBuffer(uint32 frame);
> > >       [async] processStats(uint32 frame,
> > >                            uint32 bufferId,
> > >                            libcamera.ControlList sensorControls);
> > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> > > index eb3bbd92..3a0cb6e0 100644
> > > --- a/src/ipa/simple/soft_simple.cpp
> > > +++ b/src/ipa/simple/soft_simple.cpp
> > > @@ -79,6 +79,7 @@ public:
> > >       void stop() override;
> > >   
> > >       void queueRequest(const uint32_t frame, const ControlList &controls) override;
> > > +     void fillParamsBuffer(const uint32_t frame) override;
> > >       void processStats(const uint32_t frame, const uint32_t bufferId,
> > >                         const ControlList &sensorControls) override;
> > >   
> > > @@ -279,6 +280,13 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
> > >               algo->queueRequest(context_, frame, frameContext, controls);
> > >   }
> > >   
> > > +void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
> > > +{
> > > +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > +     for (auto const &algo : algorithms())
> > > +             algo->prepare(context_, frame, frameContext, params_);
> 
> This doesn't fill a parameter buffer. Do we anticipate having a
> structured buffer to pass to the SoftISP components?
> 
> Otherwise perhaps we should just call this
>         'IPASoftSimple::prepare(const uint32_t frame)'
> 
> 
> > > +}
> > > +
> > >   void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
> > >                                [[maybe_unused]] const uint32_t bufferId,
> > >                                const ControlList &sensorControls)
> > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> > > index eda18947..564e44e8 100644
> > > --- a/src/libcamera/software_isp/software_isp.cpp
> > > +++ b/src/libcamera/software_isp/software_isp.cpp
> > > @@ -352,6 +352,7 @@ void SoftwareIsp::stop()
> > >    */
> > >   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
> > >   {
> > > +     ipa_->fillParamsBuffer(frame);
> 
> Oh... are we mixing prepare and process calls?
> 
> I'll have to take a look at the bigger picture ....

Aha - no - this does make sense because this is the 'process' stages of
the SoftISP which is the 'running' process.

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

> 
> > >       debayer_->invokeMethod(&DebayerCpu::process,
> > >                              ConnectionTypeQueued, frame, input, output, debayerParams_);
> > >   }
Milan Zamazal Sept. 19, 2024, 5:47 p.m. UTC | #4 
Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Kieran Bingham (2024-09-09 21:09:20)
>> Quoting Dan Scally (2024-09-09 09:21:45)
>> > Hi Milan
>
>> > 
>> > On 06/09/2024 13:09, Milan Zamazal wrote:
>> > > This patch adds Algorithm::prepare call for the defined algorithms.
>> > > This is preparation only since there are currently no Algorithm based
>> > > algorithms defined.
>> > >
>> > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>> > > ---
>> > 
>> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>> > 
>> > >   include/libcamera/ipa/soft.mojom            | 1 +
>> > >   src/ipa/simple/soft_simple.cpp              | 8 ++++++++
>> > >   src/libcamera/software_isp/software_isp.cpp | 1 +
>> > >   3 files changed, 10 insertions(+)
>> > >
>> > > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> > > index ddccd154..347fd69b 100644
>> > > --- a/include/libcamera/ipa/soft.mojom
>> > > +++ b/include/libcamera/ipa/soft.mojom
>> > > @@ -24,6 +24,7 @@ interface IPASoftInterface {
>> > >               => (int32 ret);
>> > >   
>> > >       [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
>> > > +     [async] fillParamsBuffer(uint32 frame);
>> > >       [async] processStats(uint32 frame,
>> > >                            uint32 bufferId,
>> > >                            libcamera.ControlList sensorControls);
>> > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> > > index eb3bbd92..3a0cb6e0 100644
>> > > --- a/src/ipa/simple/soft_simple.cpp
>> > > +++ b/src/ipa/simple/soft_simple.cpp
>> > > @@ -79,6 +79,7 @@ public:
>> > >       void stop() override;
>> > >   
>> > >       void queueRequest(const uint32_t frame, const ControlList &controls) override;
>> > > +     void fillParamsBuffer(const uint32_t frame) override;
>> > >       void processStats(const uint32_t frame, const uint32_t bufferId,
>> > >                         const ControlList &sensorControls) override;
>> > >   
>> > > @@ -279,6 +280,13 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
>> > >               algo->queueRequest(context_, frame, frameContext, controls);
>> > >   }
>> > >   
>> > > +void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
>> > > +{
>> > > +     IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> > > +     for (auto const &algo : algorithms())
>> > > +             algo->prepare(context_, frame, frameContext, params_);
>> 
>> This doesn't fill a parameter buffer. Do we anticipate having a
>> structured buffer to pass to the SoftISP components?
>> 
>> Otherwise perhaps we should just call this
>>         'IPASoftSimple::prepare(const uint32_t frame)'

It has been renamed from IPASoftSimple::prepare to
IPASoftSimple::fillParamsBuffer on Laurent's request, for consistency
with the other pipelines.  params_ is a mmaped structure, a single slot
currently but it will be selected from a ring in a followup patch
series; so I think "buffer" in the name is not completely off.

>> > > +}
>> > > +
>> > >   void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
>> > >                                [[maybe_unused]] const uint32_t bufferId,
>> > >                                const ControlList &sensorControls)
>> > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> > > index eda18947..564e44e8 100644
>> > > --- a/src/libcamera/software_isp/software_isp.cpp
>> > > +++ b/src/libcamera/software_isp/software_isp.cpp
>> > > @@ -352,6 +352,7 @@ void SoftwareIsp::stop()
>> > >    */
>> > >   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>> > >   {
>> > > +     ipa_->fillParamsBuffer(frame);
>> 
>> Oh... are we mixing prepare and process calls?
>> 
>> I'll have to take a look at the bigger picture ....
>
> Aha - no - this does make sense because this is the 'process' stages of
> the SoftISP which is the 'running' process.

Yes.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> 
>> > >       debayer_->invokeMethod(&DebayerCpu::process,
>> > >                              ConnectionTypeQueued, frame, input, output, debayerParams_);
>> > >   }

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
index ddccd154..347fd69b 100644
--- a/include/libcamera/ipa/soft.mojom
+++ b/include/libcamera/ipa/soft.mojom
@@ -24,6 +24,7 @@  interface IPASoftInterface {
 		=> (int32 ret);
 
 	[async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
+	[async] fillParamsBuffer(uint32 frame);
 	[async] processStats(uint32 frame,
 			     uint32 bufferId,
 			     libcamera.ControlList sensorControls);
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index eb3bbd92..3a0cb6e0 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -79,6 +79,7 @@  public:
 	void stop() override;
 
 	void queueRequest(const uint32_t frame, const ControlList &controls) override;
+	void fillParamsBuffer(const uint32_t frame) override;
 	void processStats(const uint32_t frame, const uint32_t bufferId,
 			  const ControlList &sensorControls) override;
 
@@ -279,6 +280,13 @@  void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro
 		algo->queueRequest(context_, frame, frameContext, controls);
 }
 
+void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
+{
+	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
+	for (auto const &algo : algorithms())
+		algo->prepare(context_, frame, frameContext, params_);
+}
+
 void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
 				 [[maybe_unused]] const uint32_t bufferId,
 				 const ControlList &sensorControls)
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index eda18947..564e44e8 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -352,6 +352,7 @@  void SoftwareIsp::stop()
  */
 void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
 {
+	ipa_->fillParamsBuffer(frame);
 	debayer_->invokeMethod(&DebayerCpu::process,
 			       ConnectionTypeQueued, frame, input, output, debayerParams_);
 }