[v3,15/23] libcamera: software_isp: Call Algorithm::queueRequest
diff mbox series

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

Commit Message

Milan Zamazal July 17, 2024, 8:54 a.m. UTC 
This patch adds Algorithm::queueRequest call for the defined algorithms.
As there are currently no control knobs in software ISP nor the
corresponding queueRequest call chain, the patch also introduces the
queueRequest methods and calls from the pipeline to the IPA.

This is preparation only since there are currently no Algorithm based
algorithms defined and no current software ISP algorithms support
control knobs.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.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

Dan Scally Aug. 13, 2024, 8:50 a.m. UTC | #1 
Hi Milan

On 17/07/2024 09:54, Milan Zamazal wrote:
> This patch adds Algorithm::queueRequest call for the defined algorithms.
> As there are currently no control knobs in software ISP nor the
> corresponding queueRequest call chain, the patch also introduces the
> queueRequest methods and calls from the pipeline to the IPA.
>
> This is preparation only since there are currently no Algorithm based
> algorithms defined and no current software ISP algorithms support
> control knobs.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.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 c5d5a46f..a3e3a9da 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -73,6 +73,7 @@ public:
>   	int start();
>   	void stop();
>   
> +	void queueRequest(const uint32_t frame, const ControlList &controls);

As mentioned before, I think we should move away from calling this variable "frame". It's not the 
frame number from the driver, in most implementations (including this one) it's the request sequence 
number, so I think naming it after that somehow is the better approach.


Other than the variable name, patch looks fine to me:


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

>   	int queueBuffers(uint32_t frame, FrameBuffer *input,
>   			 const std::map<const Stream *, FrameBuffer *> &outputs);
>   
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index 5e124016..34b91657 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -23,6 +23,7 @@ interface IPASoftInterface {
>   	configure(IPAConfigInfo configInfo)
>   		=> (int32 ret);
>   
> +	[async] 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 4326d14c..984c3a77 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1429,6 +1429,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 9793df18..0b9c5fdf 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -279,6 +279,16 @@ int SoftwareIsp::exportBuffers(const Stream *stream, 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 Aug. 13, 2024, 10:58 a.m. UTC | #2 
Hi Dan,

thank you for review.

Dan Scally <dan.scally@ideasonboard.com> writes:

> Hi Milan
>
> On 17/07/2024 09:54, Milan Zamazal wrote:
>> This patch adds Algorithm::queueRequest call for the defined algorithms.
>> As there are currently no control knobs in software ISP nor the
>> corresponding queueRequest call chain, the patch also introduces the
>> queueRequest methods and calls from the pipeline to the IPA.
>>
>> This is preparation only since there are currently no Algorithm based
>> algorithms defined and no current software ISP algorithms support
>> control knobs.
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.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 c5d5a46f..a3e3a9da 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -73,6 +73,7 @@ public:
>>   	int start();
>>   	void stop();
>>   +	void queueRequest(const uint32_t frame, const ControlList &controls);
>
> As mentioned before, I think we should move away from calling this variable "frame". It's not the frame number from the driver, in most
> implementations (including this one) it's the request sequence number, so I think naming it after that somehow is the better approach.

I agree but I wouldn't like to diverge from the other pipelines.  Is it
preferable to change the variable name in this series and possibly later
also in the other pipelines, or to keep `frame' here for now and then
to make mass-rename everywhere?

> Other than the variable name, patch looks fine to me:
>
>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>
>>   	int queueBuffers(uint32_t frame, FrameBuffer *input,
>>   			 const std::map<const Stream *, FrameBuffer *> &outputs);
>>   diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> index 5e124016..34b91657 100644
>> --- a/include/libcamera/ipa/soft.mojom
>> +++ b/include/libcamera/ipa/soft.mojom
>> @@ -23,6 +23,7 @@ interface IPASoftInterface {
>>   	configure(IPAConfigInfo configInfo)
>>   		=> (int32 ret);
>>   +	[async] 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 4326d14c..984c3a77 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -1429,6 +1429,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 9793df18..0b9c5fdf 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -279,6 +279,16 @@ int SoftwareIsp::exportBuffers(const Stream *stream, 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 c5d5a46f..a3e3a9da 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -73,6 +73,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<const Stream *, FrameBuffer *> &outputs);
 
diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
index 5e124016..34b91657 100644
--- a/include/libcamera/ipa/soft.mojom
+++ b/include/libcamera/ipa/soft.mojom
@@ -23,6 +23,7 @@  interface IPASoftInterface {
 	configure(IPAConfigInfo configInfo)
 		=> (int32 ret);
 
+	[async] 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 4326d14c..984c3a77 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1429,6 +1429,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 9793df18..0b9c5fdf 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -279,6 +279,16 @@  int SoftwareIsp::exportBuffers(const Stream *stream, 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