[libcamera-devel] [PATCH v2 05/10] pipeline: raspberrypi: Disable StreamOn for ISP Output0/1 when dropping frames

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Tue, Nov 29, 2022 at 01:45:29PM +0000, Naushir Patuck via libcamera-devel wrote:
> If the pipeline handler is required to drop startup frames by the IPA, do not
> call StreamOn on the ISP Output0 and Output1 device nodes from
> PipelineHandlerRPi::start. This stops the ISP from generating output on those
> channels giving improving latency, and more crucially does not require internal
> buffers to be allocated to deal with this condition.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/data/default.json    |  2 +-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 34 ++++++++++++++++---
>  2 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/data/default.json b/src/libcamera/pipeline/raspberrypi/data/default.json
> index d709e31850af..a7ea735c87f4 100644
> --- a/src/libcamera/pipeline/raspberrypi/data/default.json
> +++ b/src/libcamera/pipeline/raspberrypi/data/default.json
> @@ -14,7 +14,7 @@
>                  #                             min_total_unicam_buffers - external buffer count)
>                  "min_total_unicam_buffers": 4,
>                  
> -                # The number of internal buffers used for ISP Output0. This must be set to 1.
> +                # The number of internal buffers used for ISP Output0.

Can you explain this change in the commit message ?

>                  "num_output0_buffers": 1
>          }
>  }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index ce411453db0a..86eb43a3a3c5 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1094,8 +1094,18 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  
>  	data->state_ = RPiCameraData::State::Idle;
>  
> -	/* Start all streams. */
> +	/*
> +	 * Start all streams with the exception of ISP Output0 and Output1 if
> +	 * we are dropping some start-up frames. This saves a tiny bit of latency
> +	 * and avoids the need for allocating an Output0 buffer only to handle
> +	 * startup drop frame conditions.
> +	 */

I'm not sure to follow you here. Wouldn't it be sufficient to drop the
buffers at unicam's output without pushing them to the ISP ?  Why does
delaying streamOn() help ?

>  	for (auto const stream : data->streams_) {
> +		if (data->dropFrameCount_ &&
> +		    (stream == &data->isp_[Isp::Output0] ||
> +		     stream == &data->isp_[Isp::Output1]))
> +			continue;
> +
>  		ret = stream->dev()->streamOn();
>  		if (ret) {
>  			stop(camera);
> @@ -1500,6 +1510,13 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)
>  			if (ret < 0)
>  				return ret;
>  		} else {
> +			/*
> +			 * We don't enable streaming for Output0 and Output1 for
> +			 * startup frame drops, so don't queue any buffers.
> +			 */
> +			if (stream == &data->isp_[Isp::Output0] ||
> +			    stream == &data->isp_[Isp::Output1])
> +				continue;
>  			/*
>  			 * For external streams, we must queue up a set of internal
>  			 * buffers to handle the number of drop frames requested by
> @@ -2169,16 +2186,25 @@ void RPiCameraData::checkRequestCompleted()
>  	}
>  
>  	/*
> -	 * Make sure we have three outputs completed in the case of a dropped
> -	 * frame.
> +	 * Only the ISP statistics output is generated when we are dropping
> +	 * frames on startup.
>  	 */
>  	if (state_ == State::IpaComplete &&
> -	    ((ispOutputCount_ == 3 && dropFrameCount_) || requestCompleted)) {
> +	    ((ispOutputCount_ == 1 && dropFrameCount_) || requestCompleted)) {
>  		state_ = State::Idle;
>  		if (dropFrameCount_) {
>  			dropFrameCount_--;
>  			LOG(RPI, Debug) << "Dropping frame at the request of the IPA ("
>  					<< dropFrameCount_ << " left)";
> +			/*
> +			 * If we have finished dropping startup frames, start
> +			 * streaming on the ISP Output0 and Output1 nodes for
> +			 * normal operation.
> +			 */
> +			if (!dropFrameCount_) {
> +				isp_[Isp::Output0].dev()->streamOn();
> +				isp_[Isp::Output1].dev()->streamOn();
> +			}
>  		}
>  	}
>  }

-- 
Regards,

Laurent Pinchart