[libcamera-devel] Revert "libcamera: ipu3: imgu: Add pipe calculation debug"
diff mbox series

Message ID 20210527120920.28936-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel] Revert "libcamera: ipu3: imgu: Add pipe calculation debug"
Related show

Commit Message

Jacopo Mondi May 27, 2021, 12:09 p.m. UTC 
This reverts commit 5b015e96ccdbcd87b4ba6484199652fec5cdb38a.

The ImgU pipe configuration debug is useful to test the correctness
of the parameters computation against the Intel Python script.

However, the number of debug messages which is printed out by the
configuration procedure is so high it floods the logs, up to the point
that starting the Android camera3 HAL, which tests several configurations
at startup, becomes so slow it is barely usable.

Revert the patch that adds the excessive debug statements, which are mostly
useful only when testing the configuration procedure.

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

---
 src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

--
2.31.1

Comments

Umang Jain May 27, 2021, 1:47 p.m. UTC | #1 
Hi Jacopo

On 5/27/21 5:39 PM, Jacopo Mondi wrote:
> This reverts commit 5b015e96ccdbcd87b4ba6484199652fec5cdb38a.
>
> The ImgU pipe configuration debug is useful to test the correctness
> of the parameters computation against the Intel Python script.
>
> However, the number of debug messages which is printed out by the
> configuration procedure is so high it floods the logs, up to the point
> that starting the Android camera3 HAL, which tests several configurations
> at startup, becomes so slow it is barely usable.
>
> Revert the patch that adds the excessive debug statements, which are mostly
> useful only when testing the configuration procedure.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Acked-by: Umang Jain <umang.jain@ideasonboard.com>
>
> ---
>   src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++------------------------
>   1 file changed, 4 insertions(+), 30 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 6bfd23bee3ca..3e517ac67962 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -23,7 +23,6 @@
>   namespace libcamera {
>
>   LOG_DECLARE_CATEGORY(IPU3)
> -LOG_DEFINE_CATEGORY(ImgUPipe)
>
>   namespace {
>
> @@ -129,8 +128,6 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>   	unsigned int ifHeight;
>   	float bdsHeight;
>
> -	LOG(ImgUPipe, Debug) << "BDS sf: " << bdsSF << ", BDS width: " << bdsWidth;
> -
>   	if (!isSameRatio(pipe->input, gdc)) {
>   		unsigned int foundIfHeight = 0;
>   		float estIFHeight = (iif.width * gdc.height) /
> @@ -138,9 +135,6 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>   		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
>
>   		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> -		LOG(ImgUPipe, Debug) << "Estimated IF Height: " << estIFHeight
> -				     << ", IF Height: " << ifHeight;
> -
>   		while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
>   		       ifHeight / bdsSF >= minBDSHeight) {
>
> @@ -176,15 +170,9 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>
>   		if (foundIfHeight) {
>   			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> -			Size foundIf{ iif.width, foundIfHeight };
> -			Size foundBds{ bdsWidth, bdsIntHeight };
>
> -			LOG(ImgUPipe, Debug)
> -				<< "IF: " << foundIf.toString()
> -				<< ", BDS: " << foundBds.toString()
> -				<< ", GDC: " << gdc.toString();
> -
> -			pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
> +			pipeConfigs.push_back({ bdsSF, { iif.width, foundIfHeight },
> +						{ bdsWidth, bdsIntHeight }, gdc });
>   			return;
>   		}
>   	} else {
> @@ -197,15 +185,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>
>   				if (!(ifHeight % IF_ALIGN_H) &&
>   				    !(bdsIntHeight % BDS_ALIGN_H)) {
> -					Size foundIf{ iif.width, ifHeight };
> -					Size foundBds{ bdsWidth, bdsIntHeight };
> -
> -					LOG(ImgUPipe, Debug)
> -						<< "IF: " << foundIf.toString()
> -						<< ", BDS: " << foundBds.toString()
> -						<< ", GDC: " << gdc.toString();
> -
> -					pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
> +					pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
> +								{ bdsWidth, bdsIntHeight }, gdc });
>   				}
>   			}
>
> @@ -283,8 +264,6 @@ Size calculateGDC(ImgUDevice::Pipe *pipe)
>   	gdc.width = main.width * sf;
>   	gdc.height = main.height * sf;
>
> -	LOG(ImgUPipe, Debug) << "GDC: " << gdc.toString();
> -
>   	return gdc;
>   }
>
> @@ -302,11 +281,6 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
>   	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
>   	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
>
> -	LOG(ImgUPipe, Debug)
> -		<< "IF (" << pipe.iif.toString() << ") - BDS ("
> -		<< pipe.bds.toString() << ") - GDC (" << pipe.gdc.toString()
> -		<< ") -> FOV: " << fov.w << "x" << fov.h;
> -
>   	return fov;
>   }
>
> --
> 2.31.1
>
Laurent Pinchart May 27, 2021, 2:11 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Thu, May 27, 2021 at 02:09:20PM +0200, Jacopo Mondi wrote:
> This reverts commit 5b015e96ccdbcd87b4ba6484199652fec5cdb38a.
> 
> The ImgU pipe configuration debug is useful to test the correctness
> of the parameters computation against the Intel Python script.
> 
> However, the number of debug messages which is printed out by the
> configuration procedure is so high it floods the logs, up to the point
> that starting the Android camera3 HAL, which tests several configurations
> at startup, becomes so slow it is barely usable.
> 
> Revert the patch that adds the excessive debug statements, which are mostly
> useful only when testing the configuration procedure.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  src/libcamera/pipeline/ipu3/imgu.cpp | 34 ++++------------------------
>  1 file changed, 4 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 6bfd23bee3ca..3e517ac67962 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -23,7 +23,6 @@
>  namespace libcamera {
> 
>  LOG_DECLARE_CATEGORY(IPU3)
> -LOG_DEFINE_CATEGORY(ImgUPipe)
> 
>  namespace {
> 
> @@ -129,8 +128,6 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>  	unsigned int ifHeight;
>  	float bdsHeight;
> 
> -	LOG(ImgUPipe, Debug) << "BDS sf: " << bdsSF << ", BDS width: " << bdsWidth;
> -
>  	if (!isSameRatio(pipe->input, gdc)) {
>  		unsigned int foundIfHeight = 0;
>  		float estIFHeight = (iif.width * gdc.height) /
> @@ -138,9 +135,6 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
>  		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);
> 
>  		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> -		LOG(ImgUPipe, Debug) << "Estimated IF Height: " << estIFHeight
> -				     << ", IF Height: " << ifHeight;
> -
>  		while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
>  		       ifHeight / bdsSF >= minBDSHeight) {
> 
> @@ -176,15 +170,9 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> 
>  		if (foundIfHeight) {
>  			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> -			Size foundIf{ iif.width, foundIfHeight };
> -			Size foundBds{ bdsWidth, bdsIntHeight };
> 
> -			LOG(ImgUPipe, Debug)
> -				<< "IF: " << foundIf.toString()
> -				<< ", BDS: " << foundBds.toString()
> -				<< ", GDC: " << gdc.toString();
> -
> -			pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
> +			pipeConfigs.push_back({ bdsSF, { iif.width, foundIfHeight },
> +						{ bdsWidth, bdsIntHeight }, gdc });
>  			return;
>  		}
>  	} else {
> @@ -197,15 +185,8 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> 
>  				if (!(ifHeight % IF_ALIGN_H) &&
>  				    !(bdsIntHeight % BDS_ALIGN_H)) {
> -					Size foundIf{ iif.width, ifHeight };
> -					Size foundBds{ bdsWidth, bdsIntHeight };
> -
> -					LOG(ImgUPipe, Debug)
> -						<< "IF: " << foundIf.toString()
> -						<< ", BDS: " << foundBds.toString()
> -						<< ", GDC: " << gdc.toString();
> -
> -					pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
> +					pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
> +								{ bdsWidth, bdsIntHeight }, gdc });
>  				}
>  			}
> 
> @@ -283,8 +264,6 @@ Size calculateGDC(ImgUDevice::Pipe *pipe)
>  	gdc.width = main.width * sf;
>  	gdc.height = main.height * sf;
> 
> -	LOG(ImgUPipe, Debug) << "GDC: " << gdc.toString();
> -
>  	return gdc;
>  }
> 
> @@ -302,11 +281,6 @@ FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
>  	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
>  	fov.h = (inH - (ifCropH + gdcCropH)) / inH;
> 
> -	LOG(ImgUPipe, Debug)
> -		<< "IF (" << pipe.iif.toString() << ") - BDS ("
> -		<< pipe.bds.toString() << ") - GDC (" << pipe.gdc.toString()
> -		<< ") -> FOV: " << fov.w << "x" << fov.h;
> -
>  	return fov;
>  }
> 
> --
> 2.31.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
index 6bfd23bee3ca..3e517ac67962 100644
--- a/src/libcamera/pipeline/ipu3/imgu.cpp
+++ b/src/libcamera/pipeline/ipu3/imgu.cpp
@@ -23,7 +23,6 @@ 
 namespace libcamera {

 LOG_DECLARE_CATEGORY(IPU3)
-LOG_DEFINE_CATEGORY(ImgUPipe)

 namespace {

@@ -129,8 +128,6 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 	unsigned int ifHeight;
 	float bdsHeight;

-	LOG(ImgUPipe, Debug) << "BDS sf: " << bdsSF << ", BDS width: " << bdsWidth;
-
 	if (!isSameRatio(pipe->input, gdc)) {
 		unsigned int foundIfHeight = 0;
 		float estIFHeight = (iif.width * gdc.height) /
@@ -138,9 +135,6 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
 		estIFHeight = std::clamp<float>(estIFHeight, minIFHeight, iif.height);

 		ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
-		LOG(ImgUPipe, Debug) << "Estimated IF Height: " << estIFHeight
-				     << ", IF Height: " << ifHeight;
-
 		while (ifHeight >= minIFHeight && ifHeight <= iif.height &&
 		       ifHeight / bdsSF >= minBDSHeight) {

@@ -176,15 +170,9 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc

 		if (foundIfHeight) {
 			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
-			Size foundIf{ iif.width, foundIfHeight };
-			Size foundBds{ bdsWidth, bdsIntHeight };

-			LOG(ImgUPipe, Debug)
-				<< "IF: " << foundIf.toString()
-				<< ", BDS: " << foundBds.toString()
-				<< ", GDC: " << gdc.toString();
-
-			pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
+			pipeConfigs.push_back({ bdsSF, { iif.width, foundIfHeight },
+						{ bdsWidth, bdsIntHeight }, gdc });
 			return;
 		}
 	} else {
@@ -197,15 +185,8 @@  void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc

 				if (!(ifHeight % IF_ALIGN_H) &&
 				    !(bdsIntHeight % BDS_ALIGN_H)) {
-					Size foundIf{ iif.width, ifHeight };
-					Size foundBds{ bdsWidth, bdsIntHeight };
-
-					LOG(ImgUPipe, Debug)
-						<< "IF: " << foundIf.toString()
-						<< ", BDS: " << foundBds.toString()
-						<< ", GDC: " << gdc.toString();
-
-					pipeConfigs.push_back({ bdsSF, foundIf, foundBds, gdc });
+					pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },
+								{ bdsWidth, bdsIntHeight }, gdc });
 				}
 			}

@@ -283,8 +264,6 @@  Size calculateGDC(ImgUDevice::Pipe *pipe)
 	gdc.width = main.width * sf;
 	gdc.height = main.height * sf;

-	LOG(ImgUPipe, Debug) << "GDC: " << gdc.toString();
-
 	return gdc;
 }

@@ -302,11 +281,6 @@  FOV calcFOV(const Size &in, const ImgUDevice::PipeConfig &pipe)
 	fov.w = (inW - (ifCropW + gdcCropW)) / inW;
 	fov.h = (inH - (ifCropH + gdcCropH)) / inH;

-	LOG(ImgUPipe, Debug)
-		<< "IF (" << pipe.iif.toString() << ") - BDS ("
-		<< pipe.bds.toString() << ") - GDC (" << pipe.gdc.toString()
-		<< ") -> FOV: " << fov.w << "x" << fov.h;
-
 	return fov;
 }