[libcamera-devel,1/2] ipa: rkisp1: Split queuing of request and parameter filling
diff mbox series

Message ID 20220331165725.188364-2-umang.jain@ideasonboard.com
State Accepted
Delegated to: Umang Jain
Headers show
Series
  • Synchronise IPA interfaces
Related show

Commit Message

Umang Jain March 31, 2022, 4:57 p.m. UTC 
Queuing of request (i.e. passing of controls to the IPA)
and filling of the parameters buffer are two separate operations.
Treat them as such by splitting them into two functions in
the rkisp1 IPA interface.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 include/libcamera/ipa/rkisp1.mojom       |  4 ++--
 src/ipa/rkisp1/rkisp1.cpp                | 11 ++++++++---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  5 +++--
 3 files changed, 13 insertions(+), 7 deletions(-)

Comments

Nicolas Dufresne via libcamera-devel April 1, 2022, 8:31 a.m. UTC | #1 
Hi Umang,

On Thu, Mar 31, 2022 at 10:27:24PM +0530, Umang Jain via libcamera-devel wrote:
> Queuing of request (i.e. passing of controls to the IPA)
> and filling of the parameters buffer are two separate operations.
> Treat them as such by splitting them into two functions in
> the rkisp1 IPA interface.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

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

> ---
>  include/libcamera/ipa/rkisp1.mojom       |  4 ++--
>  src/ipa/rkisp1/rkisp1.cpp                | 11 ++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  5 +++--
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index f50f1e11..e3537385 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -23,8 +23,8 @@ interface IPARkISP1Interface {
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
>  
> -	[async] queueRequest(uint32 frame, uint32 bufferId,
> -			     libcamera.ControlList reqControls);
> +	[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);
> +	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>  	[async] processStatsBuffer(uint32 frame, uint32 bufferId,
>  				   libcamera.ControlList sensorControls);
>  };
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 13ab1cdc..ef1f0d56 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -54,8 +54,8 @@ public:
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> -	void queueRequest(const uint32_t frame, const uint32_t bufferId,
> -			  const ControlList &controls) override;
> +	void queueRequest(const uint32_t frame, const ControlList &controls) override;
> +	void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
>  	void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>  				const ControlList &sensorControls) override;
>  private:
> @@ -235,8 +235,13 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> -void IPARkISP1::queueRequest(const uint32_t frame, const uint32_t bufferId,
> +void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,
>  			     [[maybe_unused]] const ControlList &controls)
> +{
> +	/* \todo Start processing for 'frame' based on 'controls'. */
> +}
> +
> +void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>  {
>  	rkisp1_params_cfg *params =
>  		reinterpret_cast<rkisp1_params_cfg *>(
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e6fc582b..1624e2ec 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -855,8 +855,9 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  	if (!info)
>  		return -ENOENT;
>  
> -	data->ipa_->queueRequest(data->frame_, info->paramBuffer->cookie(),
> -				 request->controls());
> +	data->ipa_->queueRequest(data->frame_, request->controls());
> +	data->ipa_->fillParamsBuffer(data->frame_, info->paramBuffer->cookie());
> +
>  	data->frame_++;
>  
>  	return 0;
> -- 
> 2.31.0
>
Laurent Pinchart April 1, 2022, 12:04 p.m. UTC | #2 
Hi Umang,

Thank you for the patch.

On Thu, Mar 31, 2022 at 10:27:24PM +0530, Umang Jain via libcamera-devel wrote:
> Queuing of request (i.e. passing of controls to the IPA)
> and filling of the parameters buffer are two separate operations.
> Treat them as such by splitting them into two functions in
> the rkisp1 IPA interface.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

It's a bit useless at the moment as queueRequest() is a stub, but it
prepares for the future.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  include/libcamera/ipa/rkisp1.mojom       |  4 ++--
>  src/ipa/rkisp1/rkisp1.cpp                | 11 ++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  5 +++--
>  3 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> index f50f1e11..e3537385 100644
> --- a/include/libcamera/ipa/rkisp1.mojom
> +++ b/include/libcamera/ipa/rkisp1.mojom
> @@ -23,8 +23,8 @@ interface IPARkISP1Interface {
>  	mapBuffers(array<libcamera.IPABuffer> buffers);
>  	unmapBuffers(array<uint32> ids);
>  
> -	[async] queueRequest(uint32 frame, uint32 bufferId,
> -			     libcamera.ControlList reqControls);
> +	[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);
> +	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>  	[async] processStatsBuffer(uint32 frame, uint32 bufferId,
>  				   libcamera.ControlList sensorControls);
>  };
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 13ab1cdc..ef1f0d56 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -54,8 +54,8 @@ public:
>  	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
>  	void unmapBuffers(const std::vector<unsigned int> &ids) override;
>  
> -	void queueRequest(const uint32_t frame, const uint32_t bufferId,
> -			  const ControlList &controls) override;
> +	void queueRequest(const uint32_t frame, const ControlList &controls) override;
> +	void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
>  	void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>  				const ControlList &sensorControls) override;
>  private:
> @@ -235,8 +235,13 @@ void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
>  	}
>  }
>  
> -void IPARkISP1::queueRequest(const uint32_t frame, const uint32_t bufferId,
> +void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,
>  			     [[maybe_unused]] const ControlList &controls)
> +{
> +	/* \todo Start processing for 'frame' based on 'controls'. */
> +}
> +
> +void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>  {
>  	rkisp1_params_cfg *params =
>  		reinterpret_cast<rkisp1_params_cfg *>(
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e6fc582b..1624e2ec 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -855,8 +855,9 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>  	if (!info)
>  		return -ENOENT;
>  
> -	data->ipa_->queueRequest(data->frame_, info->paramBuffer->cookie(),
> -				 request->controls());
> +	data->ipa_->queueRequest(data->frame_, request->controls());
> +	data->ipa_->fillParamsBuffer(data->frame_, info->paramBuffer->cookie());
> +
>  	data->frame_++;
>

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
index f50f1e11..e3537385 100644
--- a/include/libcamera/ipa/rkisp1.mojom
+++ b/include/libcamera/ipa/rkisp1.mojom
@@ -23,8 +23,8 @@  interface IPARkISP1Interface {
 	mapBuffers(array<libcamera.IPABuffer> buffers);
 	unmapBuffers(array<uint32> ids);
 
-	[async] queueRequest(uint32 frame, uint32 bufferId,
-			     libcamera.ControlList reqControls);
+	[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);
+	[async] fillParamsBuffer(uint32 frame, uint32 bufferId);
 	[async] processStatsBuffer(uint32 frame, uint32 bufferId,
 				   libcamera.ControlList sensorControls);
 };
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 13ab1cdc..ef1f0d56 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -54,8 +54,8 @@  public:
 	void mapBuffers(const std::vector<IPABuffer> &buffers) override;
 	void unmapBuffers(const std::vector<unsigned int> &ids) override;
 
-	void queueRequest(const uint32_t frame, const uint32_t bufferId,
-			  const ControlList &controls) override;
+	void queueRequest(const uint32_t frame, const ControlList &controls) override;
+	void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
 	void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
 				const ControlList &sensorControls) override;
 private:
@@ -235,8 +235,13 @@  void IPARkISP1::unmapBuffers(const std::vector<unsigned int> &ids)
 	}
 }
 
-void IPARkISP1::queueRequest(const uint32_t frame, const uint32_t bufferId,
+void IPARkISP1::queueRequest([[maybe_unused]] const uint32_t frame,
 			     [[maybe_unused]] const ControlList &controls)
+{
+	/* \todo Start processing for 'frame' based on 'controls'. */
+}
+
+void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
 {
 	rkisp1_params_cfg *params =
 		reinterpret_cast<rkisp1_params_cfg *>(
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index e6fc582b..1624e2ec 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -855,8 +855,9 @@  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
 	if (!info)
 		return -ENOENT;
 
-	data->ipa_->queueRequest(data->frame_, info->paramBuffer->cookie(),
-				 request->controls());
+	data->ipa_->queueRequest(data->frame_, request->controls());
+	data->ipa_->fillParamsBuffer(data->frame_, info->paramBuffer->cookie());
+
 	data->frame_++;
 
 	return 0;