[1/2] libcamera: mali-c55: Set bytesused appropriately
diff mbox series

Message ID 20250717220539.2434556-1-dan.scally@ideasonboard.com
State Accepted
Commit 2cc66bb547a222d3cc3f9e73de054d537324c825
Headers show
Series
  • [1/2] libcamera: mali-c55: Set bytesused appropriately
Related show

Commit Message

Dan Scally July 17, 2025, 10:05 p.m. UTC 
At the moment the mali-c55 pipeline handler sets bytesused for a
buffer to be the maximum possible size (i.e. the size of a struct
mali_c55_params_buffer). This is not really in keeping with the goal
of the extensible parameters formats, and will not work with the new
framework for those formats. Update the IPA module and pipeline
handler to set bytesused to the size of the parameters that were
actually supplied rather than the maximum possible size.

Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
 include/libcamera/ipa/mali-c55.mojom         | 2 +-
 src/ipa/mali-c55/mali-c55.cpp                | 3 ++-
 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +++----
 3 files changed, 6 insertions(+), 6 deletions(-)

Comments

Kieran Bingham July 18, 2025, 7:25 a.m. UTC | #1 
Quoting Daniel Scally (2025-07-17 23:05:38)
> At the moment the mali-c55 pipeline handler sets bytesused for a
> buffer to be the maximum possible size (i.e. the size of a struct
> mali_c55_params_buffer). This is not really in keeping with the goal
> of the extensible parameters formats, and will not work with the new
> framework for those formats. Update the IPA module and pipeline
> handler to set bytesused to the size of the parameters that were
> actually supplied rather than the maximum possible size.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>

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

> ---
>  include/libcamera/ipa/mali-c55.mojom         | 2 +-
>  src/ipa/mali-c55/mali-c55.cpp                | 3 ++-
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +++----
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/ipa/mali-c55.mojom b/include/libcamera/ipa/mali-c55.mojom
> index 5d7eb4ee..39b7f1f1 100644
> --- a/include/libcamera/ipa/mali-c55.mojom
> +++ b/include/libcamera/ipa/mali-c55.mojom
> @@ -28,7 +28,7 @@ interface IPAMaliC55Interface {
>  };
>  
>  interface IPAMaliC55EventInterface {
> -       paramsComputed(uint32 request);
> +       paramsComputed(uint32 request, uint32 bytesused);
>         statsProcessed(uint32 request, libcamera.ControlList metadata);
>         setSensorControls(libcamera.ControlList sensorControls);
>  };
> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
> index c6941a95..5877f299 100644
> --- a/src/ipa/mali-c55/mali-c55.cpp
> +++ b/src/ipa/mali-c55/mali-c55.cpp
> @@ -346,7 +346,8 @@ void IPAMaliC55::fillParams(unsigned int request,
>                 ASSERT(params->total_size <= MALI_C55_PARAMS_MAX_SIZE);
>         }
>  
> -       paramsComputed.emit(request);
> +       size_t bytesused = offsetof(struct mali_c55_params_buffer, data) + params->total_size;
> +       paramsComputed.emit(request, bytesused);
>  }
>  
>  void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 4acc091b..17ad2055 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -618,7 +618,7 @@ public:
>         void imageBufferReady(FrameBuffer *buffer);
>         void paramsBufferReady(FrameBuffer *buffer);
>         void statsBufferReady(FrameBuffer *buffer);
> -       void paramsComputed(unsigned int requestId);
> +       void paramsComputed(unsigned int requestId, uint32_t bytesused);
>         void statsProcessed(unsigned int requestId, const ControlList &metadata);
>  
>         bool match(DeviceEnumerator *enumerator) override;
> @@ -1494,7 +1494,7 @@ void PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)
>                                  sensorControls);
>  }
>  
> -void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
> +void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId, uint32_t bytesused)
>  {
>         MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];
>         Request *request = frameInfo.request;
> @@ -1505,8 +1505,7 @@ void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
>          * video devices.
>          */
>  
> -       frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused =
> -               sizeof(struct mali_c55_params_buffer);
> +       frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
>         params_->queueBuffer(frameInfo.paramBuffer);
>         stats_->queueBuffer(frameInfo.statBuffer);
>  
> -- 
> 2.30.2
>
Isaac Scott July 18, 2025, 9:11 a.m. UTC | #2 
Hi Dan,

Thank you for the patch!

On Thu, 2025-07-17 at 23:05 +0100, Daniel Scally wrote:
> At the moment the mali-c55 pipeline handler sets bytesused for a
> buffer to be the maximum possible size (i.e. the size of a struct
> mali_c55_params_buffer). This is not really in keeping with the goal
> of the extensible parameters formats, and will not work with the new
> framework for those formats. Update the IPA module and pipeline
> handler to set bytesused to the size of the parameters that were
> actually supplied rather than the maximum possible size.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  include/libcamera/ipa/mali-c55.mojom         | 2 +-
>  src/ipa/mali-c55/mali-c55.cpp                | 3 ++-
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 7 +++----
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/ipa/mali-c55.mojom
> b/include/libcamera/ipa/mali-c55.mojom
> index 5d7eb4ee..39b7f1f1 100644
> --- a/include/libcamera/ipa/mali-c55.mojom
> +++ b/include/libcamera/ipa/mali-c55.mojom
> @@ -28,7 +28,7 @@ interface IPAMaliC55Interface {
>  };
>  
>  interface IPAMaliC55EventInterface {
> -	paramsComputed(uint32 request);
> +	paramsComputed(uint32 request, uint32 bytesused);

Good idea!

Reviewed-by: Isaac Scott <isaac.scott@ideasonboard.com>

>  	statsProcessed(uint32 request, libcamera.ControlList
> metadata);
>  	setSensorControls(libcamera.ControlList sensorControls);
>  };
> diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-
> c55.cpp
> index c6941a95..5877f299 100644
> --- a/src/ipa/mali-c55/mali-c55.cpp
> +++ b/src/ipa/mali-c55/mali-c55.cpp
> @@ -346,7 +346,8 @@ void IPAMaliC55::fillParams(unsigned int request,
>  		ASSERT(params->total_size <=
> MALI_C55_PARAMS_MAX_SIZE);
>  	}
>  
> -	paramsComputed.emit(request);
> +	size_t bytesused = offsetof(struct mali_c55_params_buffer,
> data) + params->total_size;
> +	paramsComputed.emit(request, bytesused);
>  }
>  
>  void IPAMaliC55::processStats(unsigned int request, unsigned int
> bufferId,
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 4acc091b..17ad2055 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -618,7 +618,7 @@ public:
>  	void imageBufferReady(FrameBuffer *buffer);
>  	void paramsBufferReady(FrameBuffer *buffer);
>  	void statsBufferReady(FrameBuffer *buffer);
> -	void paramsComputed(unsigned int requestId);
> +	void paramsComputed(unsigned int requestId, uint32_t
> bytesused);
>  	void statsProcessed(unsigned int requestId, const
> ControlList &metadata);
>  
>  	bool match(DeviceEnumerator *enumerator) override;
> @@ -1494,7 +1494,7 @@ void
> PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)
>  				 sensorControls);
>  }
>  
> -void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
> +void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId,
> uint32_t bytesused)
>  {
>  	MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];
>  	Request *request = frameInfo.request;
> @@ -1505,8 +1505,7 @@ void
> PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
>  	 * video devices.
>  	 */
>  
> -	frameInfo.paramBuffer->_d()-
> >metadata().planes()[0].bytesused =
> -		sizeof(struct mali_c55_params_buffer);
> +	frameInfo.paramBuffer->_d()-
> >metadata().planes()[0].bytesused = bytesused;
>  	params_->queueBuffer(frameInfo.paramBuffer);
>  	stats_->queueBuffer(frameInfo.statBuffer);
>  

Best wishes,
Isaac

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/mali-c55.mojom b/include/libcamera/ipa/mali-c55.mojom
index 5d7eb4ee..39b7f1f1 100644
--- a/include/libcamera/ipa/mali-c55.mojom
+++ b/include/libcamera/ipa/mali-c55.mojom
@@ -28,7 +28,7 @@  interface IPAMaliC55Interface {
 };
 
 interface IPAMaliC55EventInterface {
-	paramsComputed(uint32 request);
+	paramsComputed(uint32 request, uint32 bytesused);
 	statsProcessed(uint32 request, libcamera.ControlList metadata);
 	setSensorControls(libcamera.ControlList sensorControls);
 };
diff --git a/src/ipa/mali-c55/mali-c55.cpp b/src/ipa/mali-c55/mali-c55.cpp
index c6941a95..5877f299 100644
--- a/src/ipa/mali-c55/mali-c55.cpp
+++ b/src/ipa/mali-c55/mali-c55.cpp
@@ -346,7 +346,8 @@  void IPAMaliC55::fillParams(unsigned int request,
 		ASSERT(params->total_size <= MALI_C55_PARAMS_MAX_SIZE);
 	}
 
-	paramsComputed.emit(request);
+	size_t bytesused = offsetof(struct mali_c55_params_buffer, data) + params->total_size;
+	paramsComputed.emit(request, bytesused);
 }
 
 void IPAMaliC55::processStats(unsigned int request, unsigned int bufferId,
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index 4acc091b..17ad2055 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -618,7 +618,7 @@  public:
 	void imageBufferReady(FrameBuffer *buffer);
 	void paramsBufferReady(FrameBuffer *buffer);
 	void statsBufferReady(FrameBuffer *buffer);
-	void paramsComputed(unsigned int requestId);
+	void paramsComputed(unsigned int requestId, uint32_t bytesused);
 	void statsProcessed(unsigned int requestId, const ControlList &metadata);
 
 	bool match(DeviceEnumerator *enumerator) override;
@@ -1494,7 +1494,7 @@  void PipelineHandlerMaliC55::statsBufferReady(FrameBuffer *buffer)
 				 sensorControls);
 }
 
-void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
+void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId, uint32_t bytesused)
 {
 	MaliC55FrameInfo &frameInfo = frameInfoMap_[requestId];
 	Request *request = frameInfo.request;
@@ -1505,8 +1505,7 @@  void PipelineHandlerMaliC55::paramsComputed(unsigned int requestId)
 	 * video devices.
 	 */
 
-	frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused =
-		sizeof(struct mali_c55_params_buffer);
+	frameInfo.paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;
 	params_->queueBuffer(frameInfo.paramBuffer);
 	stats_->queueBuffer(frameInfo.statBuffer);