[libcamera-devel,v2,14/14] libcamera: ipa: Merge controls and v4l2controls in IPAOperationData
diff mbox series

Message ID 20191012184407.31684-15-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Use ControlList for both libcamera and V4L2 controls
Related show

Commit Message

Laurent Pinchart Oct. 12, 2019, 6:44 p.m. UTC 
Now that the V4L2ControlList is merely a helper to construct a
ControlList for V4L2 controls, without any data member, all controls can
be transferred between pipeline handlers and IPAs using ControlList
only. Remove the v4l2controls member for IPAOperationData and use the
control member instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/ipa/ipa_interface.h              | 1 -
 src/ipa/rkisp1/rkisp1.cpp                | 2 +-
 src/libcamera/ipa_interface.cpp          | 8 --------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++---
 4 files changed, 4 insertions(+), 13 deletions(-)

Comments

Jacopo Mondi Oct. 13, 2019, 3:02 p.m. UTC | #1 
Hi Laurent

On Sat, Oct 12, 2019 at 09:44:07PM +0300, Laurent Pinchart wrote:
> Now that the V4L2ControlList is merely a helper to construct a
> ControlList for V4L2 controls, without any data member, all controls can
> be transferred between pipeline handlers and IPAs using ControlList
> only. Remove the v4l2controls member for IPAOperationData and use the
> control member instead.

Great to see this simplified!
We should aim to do the same with the V4L2ControlInfoMap!

This all seems good!

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/ipa/ipa_interface.h              | 1 -
>  src/ipa/rkisp1/rkisp1.cpp                | 2 +-
>  src/libcamera/ipa_interface.cpp          | 8 --------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++---
>  4 files changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index c393b64f6aa1..dfb1bcbee516 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -33,7 +33,6 @@ struct IPAOperationData {
>  	unsigned int operation;
>  	std::vector<uint32_t> data;
>  	std::vector<ControlList> controls;
> -	std::vector<V4L2ControlList> v4l2controls;
>  };
>
>  class IPAInterface
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 69ced840585f..13059d99036c 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -215,7 +215,7 @@ void IPARkISP1::setControls(unsigned int frame)
>  	V4L2ControlList ctrls(ctrls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -	op.v4l2controls.push_back(ctrls);
> +	op.controls.push_back(ctrls);
>
>  	queueFrameAction.emit(frame, op);
>  }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 4fb178298f4c..0571b8e6bf8b 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -120,14 +120,6 @@ namespace libcamera {
>   * by the IPA protocol.
>   */
>
> -/**
> - * \var IPAOperationData::v4l2controls
> - * \brief Operation V4L2 controls data
> - *
> - * The interpretation and position of different values in the array are defined
> - * by the IPA protocol.
> - */
> -
>  /**
>   * \class IPAInterface
>   * \brief Interface for IPA implementation
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 32b023730009..9b19bde8a274 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -324,7 +324,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)
>  class RkISP1ActionSetSensor : public FrameAction
>  {
>  public:
> -	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, V4L2ControlList controls)
> +	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)
>  		: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}
>
>  protected:
> @@ -335,7 +335,7 @@ protected:
>
>  private:
>  	CameraSensor *sensor_;
> -	V4L2ControlList controls_;
> +	ControlList controls_;
>  };
>
>  class RkISP1ActionQueueBuffers : public FrameAction
> @@ -387,7 +387,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  {
>  	switch (action.operation) {
>  	case RKISP1_IPA_ACTION_V4L2_SET: {
> -		V4L2ControlList controls = action.v4l2controls[0];
> +		const ControlList &controls = action.controls[0];
>  		timeline_.scheduleAction(utils::make_unique<RkISP1ActionSetSensor>(frame,
>  										   sensor_,
>  										   controls));
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Oct. 13, 2019, 4:29 p.m. UTC | #2 
Hi Laurent,

Thanks for your patch.

On 2019-10-12 21:44:07 +0300, Laurent Pinchart wrote:
> Now that the V4L2ControlList is merely a helper to construct a
> ControlList for V4L2 controls, without any data member, all controls can
> be transferred between pipeline handlers and IPAs using ControlList
> only. Remove the v4l2controls member for IPAOperationData and use the
> control member instead.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/ipa/ipa_interface.h              | 1 -
>  src/ipa/rkisp1/rkisp1.cpp                | 2 +-
>  src/libcamera/ipa_interface.cpp          | 8 --------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++---
>  4 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
> index c393b64f6aa1..dfb1bcbee516 100644
> --- a/include/ipa/ipa_interface.h
> +++ b/include/ipa/ipa_interface.h
> @@ -33,7 +33,6 @@ struct IPAOperationData {
>  	unsigned int operation;
>  	std::vector<uint32_t> data;
>  	std::vector<ControlList> controls;
> -	std::vector<V4L2ControlList> v4l2controls;
>  };
>  
>  class IPAInterface
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 69ced840585f..13059d99036c 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -215,7 +215,7 @@ void IPARkISP1::setControls(unsigned int frame)
>  	V4L2ControlList ctrls(ctrls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> -	op.v4l2controls.push_back(ctrls);
> +	op.controls.push_back(ctrls);
>  
>  	queueFrameAction.emit(frame, op);
>  }
> diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> index 4fb178298f4c..0571b8e6bf8b 100644
> --- a/src/libcamera/ipa_interface.cpp
> +++ b/src/libcamera/ipa_interface.cpp
> @@ -120,14 +120,6 @@ namespace libcamera {
>   * by the IPA protocol.
>   */
>  
> -/**
> - * \var IPAOperationData::v4l2controls
> - * \brief Operation V4L2 controls data
> - *
> - * The interpretation and position of different values in the array are defined
> - * by the IPA protocol.
> - */
> -
>  /**
>   * \class IPAInterface
>   * \brief Interface for IPA implementation
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 32b023730009..9b19bde8a274 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -324,7 +324,7 @@ RkISP1FrameInfo *RkISP1Frames::find(Request *request)
>  class RkISP1ActionSetSensor : public FrameAction
>  {
>  public:
> -	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, V4L2ControlList controls)
> +	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)
>  		: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}
>  
>  protected:
> @@ -335,7 +335,7 @@ protected:
>  
>  private:
>  	CameraSensor *sensor_;
> -	V4L2ControlList controls_;
> +	ControlList controls_;
>  };
>  
>  class RkISP1ActionQueueBuffers : public FrameAction
> @@ -387,7 +387,7 @@ void RkISP1CameraData::queueFrameAction(unsigned int frame,
>  {
>  	switch (action.operation) {
>  	case RKISP1_IPA_ACTION_V4L2_SET: {
> -		V4L2ControlList controls = action.v4l2controls[0];
> +		const ControlList &controls = action.controls[0];
>  		timeline_.scheduleAction(utils::make_unique<RkISP1ActionSetSensor>(frame,
>  										   sensor_,
>  										   controls));
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h
index c393b64f6aa1..dfb1bcbee516 100644
--- a/include/ipa/ipa_interface.h
+++ b/include/ipa/ipa_interface.h
@@ -33,7 +33,6 @@  struct IPAOperationData {
 	unsigned int operation;
 	std::vector<uint32_t> data;
 	std::vector<ControlList> controls;
-	std::vector<V4L2ControlList> v4l2controls;
 };
 
 class IPAInterface
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 69ced840585f..13059d99036c 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -215,7 +215,7 @@  void IPARkISP1::setControls(unsigned int frame)
 	V4L2ControlList ctrls(ctrls_);
 	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
 	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
-	op.v4l2controls.push_back(ctrls);
+	op.controls.push_back(ctrls);
 
 	queueFrameAction.emit(frame, op);
 }
diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
index 4fb178298f4c..0571b8e6bf8b 100644
--- a/src/libcamera/ipa_interface.cpp
+++ b/src/libcamera/ipa_interface.cpp
@@ -120,14 +120,6 @@  namespace libcamera {
  * by the IPA protocol.
  */
 
-/**
- * \var IPAOperationData::v4l2controls
- * \brief Operation V4L2 controls data
- *
- * The interpretation and position of different values in the array are defined
- * by the IPA protocol.
- */
-
 /**
  * \class IPAInterface
  * \brief Interface for IPA implementation
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 32b023730009..9b19bde8a274 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -324,7 +324,7 @@  RkISP1FrameInfo *RkISP1Frames::find(Request *request)
 class RkISP1ActionSetSensor : public FrameAction
 {
 public:
-	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, V4L2ControlList controls)
+	RkISP1ActionSetSensor(unsigned int frame, CameraSensor *sensor, const ControlList &controls)
 		: FrameAction(frame, SetSensor), sensor_(sensor), controls_(controls) {}
 
 protected:
@@ -335,7 +335,7 @@  protected:
 
 private:
 	CameraSensor *sensor_;
-	V4L2ControlList controls_;
+	ControlList controls_;
 };
 
 class RkISP1ActionQueueBuffers : public FrameAction
@@ -387,7 +387,7 @@  void RkISP1CameraData::queueFrameAction(unsigned int frame,
 {
 	switch (action.operation) {
 	case RKISP1_IPA_ACTION_V4L2_SET: {
-		V4L2ControlList controls = action.v4l2controls[0];
+		const ControlList &controls = action.controls[0];
 		timeline_.scheduleAction(utils::make_unique<RkISP1ActionSetSensor>(frame,
 										   sensor_,
 										   controls));