[v2,11/19] libcamera: software_isp: Call Algorithm::queueRequest
diff mbox series

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

Commit Message

Milan Zamazal July 3, 2024, 5:51 p.m. UTC
This patch adds Algorithm::queueRequest 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>
---
 include/libcamera/internal/software_isp/software_isp.h |  1 +
 include/libcamera/ipa/soft.mojom                       |  1 +
 src/ipa/simple/soft_simple.cpp                         |  9 +++++++++
 src/libcamera/pipeline/simple/simple.cpp               |  2 ++
 src/libcamera/software_isp/software_isp.cpp            | 10 ++++++++++
 5 files changed, 23 insertions(+)

Comments

Umang Jain July 13, 2024, 3:37 p.m. UTC | #1
Hi Milan,


On 03/07/24 11:21 pm, Milan Zamazal wrote:
> This patch adds Algorithm::queueRequest call for the defined algorithms.
> This is preparation only since there are currently no Algorithm based
> algorithms defined.

I think the patch targets adding queueRequest() call to soft ISP and 
softIPA and Algorithms here.

So this has to be reworded to reflect that.

> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   include/libcamera/internal/software_isp/software_isp.h |  1 +
>   include/libcamera/ipa/soft.mojom                       |  1 +
>   src/ipa/simple/soft_simple.cpp                         |  9 +++++++++
>   src/libcamera/pipeline/simple/simple.cpp               |  2 ++
>   src/libcamera/software_isp/software_isp.cpp            | 10 ++++++++++
>   5 files changed, 23 insertions(+)
>
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index ff26b1d4..1ba9eb62 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -72,6 +72,7 @@ public:
>   	int start();
>   	void stop();
>   
> +	void queueRequest(const uint32_t frame, const ControlList &controls);
>   	int queueBuffers(uint32_t frame, FrameBuffer *input,
>   			 const std::map<unsigned int, FrameBuffer *> &outputs);
>   
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index 5e124016..426fee18 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -23,6 +23,7 @@ interface IPASoftInterface {
>   	configure(IPAConfigInfo configInfo)
>   		=> (int32 ret);
>   
> +	queueRequest(uint32 frame, libcamera.ControlList sensorControls);

Usually this is [async] , but I suppose that's not the case with Soft 
ISP here?

AS this is only queuing later on to Algorithms, I guess it's fine for now.

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>   	[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 49fff312..bd0dea5f 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -77,6 +77,7 @@ public:
>   	int start() override;
>   	void stop() override;
>   
> +	void queueRequest(const uint32_t frame, const ControlList &controls) override;
>   	void processStats(const uint32_t frame, const uint32_t bufferId,
>   			  const ControlList &sensorControls) override;
>   
> @@ -269,6 +270,14 @@ void IPASoftSimple::stop()
>   {
>   }
>   
> +void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
> +{
> +	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
> +
> +	for (auto const &algo : algorithms())
> +		algo->queueRequest(context_, frame, frameContext, controls);
> +}
> +
>   void IPASoftSimple::processStats(
>   	[[maybe_unused]] const uint32_t frame,
>   	[[maybe_unused]] const uint32_t bufferId,
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index e0c9fe5c..6fb80209 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1430,6 +1430,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>   	if (data->useConversion_)
>   		data->conversionQueue_.push(std::move(buffers));
>   
> +	data->swIsp_->queueRequest(request->sequence(), request->controls());
> +
>   	return 0;
>   }
>   
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 2bbb86da..4e6b3a2e 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -279,6 +279,16 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
>   	return count;
>   }
>   
> +/**
> + * \brief Queue a request and process the control list from the application
> + * \param[in] frame The number of the frame which will be processed next
> + * \param[in] controls The controls for the \a frame
> + */
> +void SoftwareIsp::queueRequest(const uint32_t frame, const ControlList &controls)
> +{
> +	ipa_->queueRequest(frame, controls);
> +}
> +
>   /**
>    * \brief Queue buffers to Software ISP
>    * \param[in] frame The frame number
Milan Zamazal July 13, 2024, 7:48 p.m. UTC | #2
Hi Umang,

thank you for review.

Umang Jain <umang.jain@ideasonboard.com> writes:

> Hi Milan,
>
>
> On 03/07/24 11:21 pm, Milan Zamazal wrote:
>> This patch adds Algorithm::queueRequest call for the defined algorithms.
>> This is preparation only since there are currently no Algorithm based
>> algorithms defined.
>
> I think the patch targets adding queueRequest() call to soft ISP and softIPA and Algorithms here.
>
> So this has to be reworded to reflect that.

OK, will do.

>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   include/libcamera/internal/software_isp/software_isp.h |  1 +
>>   include/libcamera/ipa/soft.mojom                       |  1 +
>>   src/ipa/simple/soft_simple.cpp                         |  9 +++++++++
>>   src/libcamera/pipeline/simple/simple.cpp               |  2 ++
>>   src/libcamera/software_isp/software_isp.cpp            | 10 ++++++++++
>>   5 files changed, 23 insertions(+)
>>
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index ff26b1d4..1ba9eb62 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -72,6 +72,7 @@ public:
>>   	int start();
>>   	void stop();
>>   +	void queueRequest(const uint32_t frame, const ControlList &controls);
>>   	int queueBuffers(uint32_t frame, FrameBuffer *input,
>>   			 const std::map<unsigned int, FrameBuffer *> &outputs);
>>   diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> index 5e124016..426fee18 100644
>> --- a/include/libcamera/ipa/soft.mojom
>> +++ b/include/libcamera/ipa/soft.mojom
>> @@ -23,6 +23,7 @@ interface IPASoftInterface {
>>   	configure(IPAConfigInfo configInfo)
>>   		=> (int32 ret);
>>   +	queueRequest(uint32 frame, libcamera.ControlList sensorControls);
>
> Usually this is [async] , but I suppose that's not the case with Soft ISP here?

I'm not sure.  I don't know what's the reason to make it async as it
looks like the algorithms just look at control values and assign their
counterparts; maybe because it is called at a sensitive moment?  I'll
change it to async to be safe and consistent.

> AS this is only queuing later on to Algorithms, I guess it's fine for now.
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>>   	[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 49fff312..bd0dea5f 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -77,6 +77,7 @@ public:
>>   	int start() override;
>>   	void stop() override;
>>   +	void queueRequest(const uint32_t frame, const ControlList &controls) override;
>>   	void processStats(const uint32_t frame, const uint32_t bufferId,
>>   			  const ControlList &sensorControls) override;
>>   @@ -269,6 +270,14 @@ void IPASoftSimple::stop()
>>   {
>>   }
>>   +void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
>> +{
>> +	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
>> +
>> +	for (auto const &algo : algorithms())
>> +		algo->queueRequest(context_, frame, frameContext, controls);
>> +}
>> +
>>   void IPASoftSimple::processStats(
>>   	[[maybe_unused]] const uint32_t frame,
>>   	[[maybe_unused]] const uint32_t bufferId,
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index e0c9fe5c..6fb80209 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -1430,6 +1430,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>>   	if (data->useConversion_)
>>   		data->conversionQueue_.push(std::move(buffers));
>>   +	data->swIsp_->queueRequest(request->sequence(), request->controls());
>> +
>>   	return 0;
>>   }
>>   diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 2bbb86da..4e6b3a2e 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -279,6 +279,16 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
>>   	return count;
>>   }
>>   +/**
>> + * \brief Queue a request and process the control list from the application
>> + * \param[in] frame The number of the frame which will be processed next
>> + * \param[in] controls The controls for the \a frame
>> + */
>> +void SoftwareIsp::queueRequest(const uint32_t frame, const ControlList &controls)
>> +{
>> +	ipa_->queueRequest(frame, controls);
>> +}
>> +
>>   /**
>>    * \brief Queue buffers to Software ISP
>>    * \param[in] frame The frame number

Patch
diff mbox series

diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
index ff26b1d4..1ba9eb62 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -72,6 +72,7 @@  public:
 	int start();
 	void stop();
 
+	void queueRequest(const uint32_t frame, const ControlList &controls);
 	int queueBuffers(uint32_t frame, FrameBuffer *input,
 			 const std::map<unsigned int, FrameBuffer *> &outputs);
 
diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
index 5e124016..426fee18 100644
--- a/include/libcamera/ipa/soft.mojom
+++ b/include/libcamera/ipa/soft.mojom
@@ -23,6 +23,7 @@  interface IPASoftInterface {
 	configure(IPAConfigInfo configInfo)
 		=> (int32 ret);
 
+	queueRequest(uint32 frame, libcamera.ControlList sensorControls);
 	[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 49fff312..bd0dea5f 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -77,6 +77,7 @@  public:
 	int start() override;
 	void stop() override;
 
+	void queueRequest(const uint32_t frame, const ControlList &controls) override;
 	void processStats(const uint32_t frame, const uint32_t bufferId,
 			  const ControlList &sensorControls) override;
 
@@ -269,6 +270,14 @@  void IPASoftSimple::stop()
 {
 }
 
+void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
+{
+	IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
+
+	for (auto const &algo : algorithms())
+		algo->queueRequest(context_, frame, frameContext, controls);
+}
+
 void IPASoftSimple::processStats(
 	[[maybe_unused]] const uint32_t frame,
 	[[maybe_unused]] const uint32_t bufferId,
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index e0c9fe5c..6fb80209 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1430,6 +1430,8 @@  int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
 	if (data->useConversion_)
 		data->conversionQueue_.push(std::move(buffers));
 
+	data->swIsp_->queueRequest(request->sequence(), request->controls());
+
 	return 0;
 }
 
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index 2bbb86da..4e6b3a2e 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -279,6 +279,16 @@  int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,
 	return count;
 }
 
+/**
+ * \brief Queue a request and process the control list from the application
+ * \param[in] frame The number of the frame which will be processed next
+ * \param[in] controls The controls for the \a frame
+ */
+void SoftwareIsp::queueRequest(const uint32_t frame, const ControlList &controls)
+{
+	ipa_->queueRequest(frame, controls);
+}
+
 /**
  * \brief Queue buffers to Software ISP
  * \param[in] frame The frame number