[libcamera-devel] [PATCH] libcamera: rkisp1: Generate configuration from main path if only one role

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Mar 24, 2022 at 07:52:20PM +0900, Paul Elder via libcamera-devel wrote:
> The current logic for generating configurations assumes that we have
> multiple roles. The consequence of this is that if only one role is
> requested, and it is for viewfinder or recording, then the self path's
> configuration generator would be used instead of the main path's. This
> is what causes the default resolution on the rkisp1 pipeline handler to
> be 1920x1920 (since it's the max resolution of the self path). Note that
> the main path is still used for streaming, just that it is using self
> path's default configuraion (if it isn't changed by the application).

s/configuraion/configuration/

> This patch skips all the logic for determining which path to assign to
> which role in the event that only one role is requested. In this case,
> we simply generate the configuration from the math path. This makes the

s/math/main/

I like the concept of a math path better though :-)

> default resolution for a single stream 2592x1944.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index e6fc582b..6c39494e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -515,6 +515,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() == 1) {
> +		StreamConfiguration cfg = data->mainPath_->generateConfiguration(
> +			data->sensor_->resolution());
> +
> +		config->addConfiguration(cfg);
> +		config->validate();
> +
> +		return config;
> +	}

Given that the ISP has two outputs only, we can have either one or two
roles. It looks like the code below could then be simplified. Should
this function be rewritten ?

Let's also note that the main and self paths don't only differ in the
maximum resolution they support. The main path can capture YUV 4:2:2 or
4:2:0 in packed, semi-planar or planar variants (plus raw formats,
unprocessed), while the self path can additionally capture YUV 4:4:4 and
4:0:0 (but doesn't support packed YUV formats), RGB 565, 666 and 888,
and can also support flipping and rotation. We may thus need a more
complex logic here.

> +
>  	bool mainPathAvailable = true;
>  	bool selfPathAvailable = true;
>  	for (const StreamRole role : roles) {

-- 
Regards,

Laurent Pinchart