[libcamera-devel,v3,7/9] ipu3: Assign |outCaptureStream| to StillCapture configuration
diff mbox series

Message ID 20220629103018.4025635-8-chenghaoyang@google.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • Use two imgus in ipu3 pipeline handler
Related show

Commit Message

Harvey Yang June 29, 2022, 10:30 a.m. UTC 
When StillCapture and other non-raw configurations are requested,
assigns |outCaptureStream| instead of |outStream| to the StillCapture
configuration.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 50 +++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 8 deletions(-)

Comments

Umang Jain Aug. 2, 2022, 7:53 a.m. UTC | #1 
Hi Harvey,

On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
> When StillCapture and other non-raw configurations are requested,
> assigns |outCaptureStream| instead of |outStream| to the StillCapture
> configuration.
>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>   src/libcamera/pipeline/ipu3/ipu3.cpp | 50 +++++++++++++++++++++++-----
>   1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ec9d14d1..e26c2736 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -254,8 +254,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>   	 * \todo Clarify the IF and BDS margins requirements.
>   	 */
>   	unsigned int rawCount = 0;
> -	unsigned int yuvCount = 0;
> +	unsigned int videoCount = 0;
> +	unsigned int stillCount = 0;
>   	Size maxYuvSize;
> +	Size maxVideoSize;
>   	Size rawSize;
>   
>   	for (const StreamConfiguration &cfg : config_) {
> @@ -264,16 +266,29 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>   		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
>   			rawCount++;
>   			rawSize.expandTo(cfg.size);
> +		} else if (cfg.streamRole == StreamRole::StillCapture) {
> +			if (stillCount != 0)
> +				return Invalid;
> +
> +			stillCount++;
> +			maxYuvSize.expandTo(cfg.size);
>   		} else {
> -			yuvCount++;
> +			videoCount++;
>   			maxYuvSize.expandTo(cfg.size);
> +			maxVideoSize.expandTo(cfg.size);
>   		}
>   	}
>   
> -	if (rawCount > 1 || yuvCount > 2) {
> +	if (videoCount == 0 && stillCount == 1) {
> +		stillCount = 0;
> +		videoCount = 1;
> +		maxVideoSize.expandTo(maxYuvSize);
> +	}


Maybe you can explain this if block with a brief comment on the case why 
we invert stillCount and videoCount.

> +
> +	if (rawCount > 1 || videoCount > 2) {
>   		LOG(IPU3, Debug) << "Camera configuration not supported";
>   		return Invalid;
> -	} else if (rawCount && !yuvCount) {
> +	} else if (rawCount && !stillCount && !videoCount) {
>   		/*
>   		 * Disallow raw-only camera configuration. Currently, ImgU does
>   		 * not get configured for raw-only streams and has early return
> @@ -316,6 +331,9 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>   	ImgUDevice::Pipe pipe{};
>   	pipe.input = cio2Configuration_.size;
>   
> +	ImgUDevice::Pipe pipe1{};
> +	pipe1.input = cio2Configuration_.size;
> +
>   	/*
>   	 * Adjust the configurations if needed and assign streams while
>   	 * iterating them.
> @@ -380,18 +398,34 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>   			cfg->stride = info.stride(cfg->size.width, 0, 1);
>   			cfg->frameSize = info.frameSize(cfg->size, 1);
>   
> +			if (stillCount == 1 && cfg->streamRole == StreamRole::StillCapture) {


Maybe check both is redundant? Can stillCount be converted to 
ASSERT(stillCount != 0) inside the block?

> +				LOG(IPU3, Debug) << "Assigned "
> +						 << cfg->toString()
> +						 << " to the imgu1 main output";
> +				cfg->setStream(const_cast<Stream *>(&data_->outCaptureStream_));
> +
> +				pipe1.main = cfg->size;
> +				pipe1.viewfinder = pipe1.main;
> +
> +				pipeConfig1_ = data_->imgu1_->calculatePipeConfig(&pipe1);
> +				if (pipeConfig1_.isNull()) {
> +					LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
> +							 << "unsupported resolutions.";
> +					return Invalid;
> +				}
>   			/*
>   			 * Use the main output stream in case only one stream is
>   			 * requested or if the current configuration is the one
>   			 * with the maximum YUV output size.
>   			 */
> -			if (mainOutputAvailable &&
> -			    (originalCfg.size == maxYuvSize || yuvCount == 1)) {
> +			} else if (mainOutputAvailable &&
> +				   (originalCfg.size == maxVideoSize ||
> +				    videoCount == 1)) {
>   				cfg->setStream(const_cast<Stream *>(&data_->outStream_));
>   				mainOutputAvailable = false;
>   
>   				pipe.main = cfg->size;
> -				if (yuvCount == 1)
> +				if (videoCount == 1)
>   					pipe.viewfinder = pipe.main;
>   
>   				LOG(IPU3, Debug) << "Assigned " << cfg->toString()
> @@ -415,7 +449,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>   	}
>   
>   	/* Only compute the ImgU configuration if a YUV stream has been requested. */
> -	if (yuvCount) {
> +	if (videoCount) {
>   		pipeConfig0_ = data_->imgu0_->calculatePipeConfig(&pipe);
>   		if (pipeConfig0_.isNull()) {
>   			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
Harvey Yang Aug. 2, 2022, 10:31 a.m. UTC | #2 
Hi Umang,

On Tue, Aug 2, 2022 at 3:53 PM Umang Jain <umang.jain@ideasonboard.com>
wrote:

> Hi Harvey,
>
> On 6/29/22 16:00, Harvey Yang via libcamera-devel wrote:
> > When StillCapture and other non-raw configurations are requested,
> > assigns |outCaptureStream| instead of |outStream| to the StillCapture
> > configuration.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >   src/libcamera/pipeline/ipu3/ipu3.cpp | 50 +++++++++++++++++++++++-----
> >   1 file changed, 42 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ec9d14d1..e26c2736 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -254,8 +254,10 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >        * \todo Clarify the IF and BDS margins requirements.
> >        */
> >       unsigned int rawCount = 0;
> > -     unsigned int yuvCount = 0;
> > +     unsigned int videoCount = 0;
> > +     unsigned int stillCount = 0;
> >       Size maxYuvSize;
> > +     Size maxVideoSize;
> >       Size rawSize;
> >
> >       for (const StreamConfiguration &cfg : config_) {
> > @@ -264,16 +266,29 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >               if (info.colourEncoding ==
> PixelFormatInfo::ColourEncodingRAW) {
> >                       rawCount++;
> >                       rawSize.expandTo(cfg.size);
> > +             } else if (cfg.streamRole == StreamRole::StillCapture) {
> > +                     if (stillCount != 0)
> > +                             return Invalid;
> > +
> > +                     stillCount++;
> > +                     maxYuvSize.expandTo(cfg.size);
> >               } else {
> > -                     yuvCount++;
> > +                     videoCount++;
> >                       maxYuvSize.expandTo(cfg.size);
> > +                     maxVideoSize.expandTo(cfg.size);
> >               }
> >       }
> >
> > -     if (rawCount > 1 || yuvCount > 2) {
> > +     if (videoCount == 0 && stillCount == 1) {
> > +             stillCount = 0;
> > +             videoCount = 1;
> > +             maxVideoSize.expandTo(maxYuvSize);
> > +     }
>
>
> Maybe you can explain this if block with a brief comment on the case why
> we invert stillCount and videoCount.
>
>
Added a simple one. Please check.


> > +
> > +     if (rawCount > 1 || videoCount > 2) {
> >               LOG(IPU3, Debug) << "Camera configuration not supported";
> >               return Invalid;
> > -     } else if (rawCount && !yuvCount) {
> > +     } else if (rawCount && !stillCount && !videoCount) {
> >               /*
> >                * Disallow raw-only camera configuration. Currently, ImgU
> does
> >                * not get configured for raw-only streams and has early
> return
> > @@ -316,6 +331,9 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >       ImgUDevice::Pipe pipe{};
> >       pipe.input = cio2Configuration_.size;
> >
> > +     ImgUDevice::Pipe pipe1{};
> > +     pipe1.input = cio2Configuration_.size;
> > +
> >       /*
> >        * Adjust the configurations if needed and assign streams while
> >        * iterating them.
> > @@ -380,18 +398,34 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >                       cfg->stride = info.stride(cfg->size.width, 0, 1);
> >                       cfg->frameSize = info.frameSize(cfg->size, 1);
> >
> > +                     if (stillCount == 1 && cfg->streamRole ==
> StreamRole::StillCapture) {
>
>
> Maybe check both is redundant? Can stillCount be converted to
> ASSERT(stillCount != 0) inside the block?
>
>
Sure. Use |ASSERT(stillCount == 1)| instead, as it only supports one
StillCapture stream.


> > +                             LOG(IPU3, Debug) << "Assigned "
> > +                                              << cfg->toString()
> > +                                              << " to the imgu1 main
> output";
> > +                             cfg->setStream(const_cast<Stream
> *>(&data_->outCaptureStream_));
> > +
> > +                             pipe1.main = cfg->size;
> > +                             pipe1.viewfinder = pipe1.main;
> > +
> > +                             pipeConfig1_ =
> data_->imgu1_->calculatePipeConfig(&pipe1);
> > +                             if (pipeConfig1_.isNull()) {
> > +                                     LOG(IPU3, Error) << "Failed to
> calculate pipe configuration: "
> > +                                                      << "unsupported
> resolutions.";
> > +                                     return Invalid;
> > +                             }
> >                       /*
> >                        * Use the main output stream in case only one
> stream is
> >                        * requested or if the current configuration is
> the one
> >                        * with the maximum YUV output size.
> >                        */
> > -                     if (mainOutputAvailable &&
> > -                         (originalCfg.size == maxYuvSize || yuvCount ==
> 1)) {
> > +                     } else if (mainOutputAvailable &&
> > +                                (originalCfg.size == maxVideoSize ||
> > +                                 videoCount == 1)) {
> >                               cfg->setStream(const_cast<Stream
> *>(&data_->outStream_));
> >                               mainOutputAvailable = false;
> >
> >                               pipe.main = cfg->size;
> > -                             if (yuvCount == 1)
> > +                             if (videoCount == 1)
> >                                       pipe.viewfinder = pipe.main;
> >
> >                               LOG(IPU3, Debug) << "Assigned " <<
> cfg->toString()
> > @@ -415,7 +449,7 @@ CameraConfiguration::Status
> IPU3CameraConfiguration::validate()
> >       }
> >
> >       /* Only compute the ImgU configuration if a YUV stream has been
> requested. */
> > -     if (yuvCount) {
> > +     if (videoCount) {
> >               pipeConfig0_ = data_->imgu0_->calculatePipeConfig(&pipe);
> >               if (pipeConfig0_.isNull()) {
> >                       LOG(IPU3, Error) << "Failed to calculate pipe
> configuration: "
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index ec9d14d1..e26c2736 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -254,8 +254,10 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	 * \todo Clarify the IF and BDS margins requirements.
 	 */
 	unsigned int rawCount = 0;
-	unsigned int yuvCount = 0;
+	unsigned int videoCount = 0;
+	unsigned int stillCount = 0;
 	Size maxYuvSize;
+	Size maxVideoSize;
 	Size rawSize;
 
 	for (const StreamConfiguration &cfg : config_) {
@@ -264,16 +266,29 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {
 			rawCount++;
 			rawSize.expandTo(cfg.size);
+		} else if (cfg.streamRole == StreamRole::StillCapture) {
+			if (stillCount != 0)
+				return Invalid;
+
+			stillCount++;
+			maxYuvSize.expandTo(cfg.size);
 		} else {
-			yuvCount++;
+			videoCount++;
 			maxYuvSize.expandTo(cfg.size);
+			maxVideoSize.expandTo(cfg.size);
 		}
 	}
 
-	if (rawCount > 1 || yuvCount > 2) {
+	if (videoCount == 0 && stillCount == 1) {
+		stillCount = 0;
+		videoCount = 1;
+		maxVideoSize.expandTo(maxYuvSize);
+	}
+
+	if (rawCount > 1 || videoCount > 2) {
 		LOG(IPU3, Debug) << "Camera configuration not supported";
 		return Invalid;
-	} else if (rawCount && !yuvCount) {
+	} else if (rawCount && !stillCount && !videoCount) {
 		/*
 		 * Disallow raw-only camera configuration. Currently, ImgU does
 		 * not get configured for raw-only streams and has early return
@@ -316,6 +331,9 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	ImgUDevice::Pipe pipe{};
 	pipe.input = cio2Configuration_.size;
 
+	ImgUDevice::Pipe pipe1{};
+	pipe1.input = cio2Configuration_.size;
+
 	/*
 	 * Adjust the configurations if needed and assign streams while
 	 * iterating them.
@@ -380,18 +398,34 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 			cfg->stride = info.stride(cfg->size.width, 0, 1);
 			cfg->frameSize = info.frameSize(cfg->size, 1);
 
+			if (stillCount == 1 && cfg->streamRole == StreamRole::StillCapture) {
+				LOG(IPU3, Debug) << "Assigned "
+						 << cfg->toString()
+						 << " to the imgu1 main output";
+				cfg->setStream(const_cast<Stream *>(&data_->outCaptureStream_));
+
+				pipe1.main = cfg->size;
+				pipe1.viewfinder = pipe1.main;
+
+				pipeConfig1_ = data_->imgu1_->calculatePipeConfig(&pipe1);
+				if (pipeConfig1_.isNull()) {
+					LOG(IPU3, Error) << "Failed to calculate pipe configuration: "
+							 << "unsupported resolutions.";
+					return Invalid;
+				}
 			/*
 			 * Use the main output stream in case only one stream is
 			 * requested or if the current configuration is the one
 			 * with the maximum YUV output size.
 			 */
-			if (mainOutputAvailable &&
-			    (originalCfg.size == maxYuvSize || yuvCount == 1)) {
+			} else if (mainOutputAvailable &&
+				   (originalCfg.size == maxVideoSize ||
+				    videoCount == 1)) {
 				cfg->setStream(const_cast<Stream *>(&data_->outStream_));
 				mainOutputAvailable = false;
 
 				pipe.main = cfg->size;
-				if (yuvCount == 1)
+				if (videoCount == 1)
 					pipe.viewfinder = pipe.main;
 
 				LOG(IPU3, Debug) << "Assigned " << cfg->toString()
@@ -415,7 +449,7 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	}
 
 	/* Only compute the ImgU configuration if a YUV stream has been requested. */
-	if (yuvCount) {
+	if (videoCount) {
 		pipeConfig0_ = data_->imgu0_->calculatePipeConfig(&pipe);
 		if (pipeConfig0_.isNull()) {
 			LOG(IPU3, Error) << "Failed to calculate pipe configuration: "