[v2,6/6] pipeline: vimc: Don't hardcode scaling factor with recent kernels
diff mbox series

Message ID 20240529154341.10426-7-laurent.pinchart@ideasonboard.com
State Accepted
Commit f7d7a5e294237a50dbdc3aa3e0fa7c3200a08fa7
Headers show
Series
  • vimc scaling improvements
Related show

Commit Message

Laurent Pinchart May 29, 2024, 3:43 p.m. UTC 
Starting in kernel v5.16, the vimc driver stopped hardcoding the scaler
factor. Use this to lift constraints on the camera configuration, and in
particular on the exotic output size alignment to a multiple of 6. As a
result, vimc-based cameras can more easily match common display
resolutions.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/pipeline/vimc/vimc.cpp | 47 +++++++++++++++++++---------
 1 file changed, 33 insertions(+), 14 deletions(-)

Comments

Dan Scally June 1, 2024, 10:44 p.m. UTC | #1 
Hi Laurent

On 29/05/2024 16:43, Laurent Pinchart wrote:
> Starting in kernel v5.16, the vimc driver stopped hardcoding the scaler
> factor. Use this to lift constraints on the camera configuration, and in
> particular on the exotic output size alignment to a multiple of 6. As a
> result, vimc-based cameras can more easily match common display
> resolutions.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Looks good to me too: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   src/libcamera/pipeline/vimc/vimc.cpp | 47 +++++++++++++++++++---------
>   1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index c7650432ddcc..0ec9928eec23 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -114,6 +114,9 @@ static const std::map<PixelFormat, uint32_t> pixelformats{
>   	{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },
>   };
>   
> +static constexpr Size kMinSize{ 16, 16 };
> +static constexpr Size kMaxSize{ 4096, 2160 };
> +
>   } /* namespace */
>   
>   VimcCameraConfiguration::VimcCameraConfiguration(VimcCameraData *data)
> @@ -153,14 +156,20 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>   	const Size size = cfg.size;
>   
>   	/*
> -	 * The scaler hardcodes a x3 scale-up ratio, and the sensor output size
> -	 * is aligned to two pixels in both directions. The output width and
> -	 * height thus have to be multiples of 6.
> +	 * The sensor output size is aligned to two pixels in both directions.
> +	 * Additionally, prior to v5.16, the scaler hardcodes a x3 scale-up
> +	 * ratio, requiring the output width and height to be multiples of 6.
>   	 */
> -	cfg.size.width = std::max(48U, std::min(4096U, cfg.size.width));
> -	cfg.size.height = std::max(48U, std::min(2160U, cfg.size.height));
> -	cfg.size.width -= cfg.size.width % 6;
> -	cfg.size.height -= cfg.size.height % 6;
> +	Size minSize{ kMinSize };
> +	unsigned int alignment = 2;
> +
> +	if (data_->media_->version() < KERNEL_VERSION(5, 16, 0)) {
> +		minSize *= 3;
> +		alignment *= 3;
> +	}
> +
> +	cfg.size.expandTo(minSize).boundTo(kMaxSize)
> +		.alignDownTo(alignment, alignment);
>   
>   	if (cfg.size != size) {
>   		LOG(VIMC, Debug)
> @@ -216,10 +225,12 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,
>   			}
>   		}
>   
> -		/* The scaler hardcodes a x3 scale-up ratio. */
> -		std::vector<SizeRange> sizes{
> -			SizeRange{ { 48, 48 }, { 4096, 2160 } }
> -		};
> +		/* Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. */
> +		Size minSize{ kMinSize };
> +		if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
> +			minSize *= 3;
> +
> +		std::vector<SizeRange> sizes{ { minSize, kMaxSize } };
>   		formats[pixelformat.first] = sizes;
>   	}
>   
> @@ -242,10 +253,18 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>   	StreamConfiguration &cfg = config->at(0);
>   	int ret;
>   
> -	/* The scaler hardcodes a x3 scale-up ratio. */
> +	/*
> +	 * Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. For newer
> +	 * kernels, use a sensor resolution of 1920x1080 and let the scaler
> +	 * produce the requested stream size.
> +	 */
> +	Size sensorSize{ 1920, 1080 };
> +	if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
> +		sensorSize = { cfg.size.width / 3, cfg.size.height / 3 };
> +
>   	V4L2SubdeviceFormat subformat = {};
>   	subformat.code = MEDIA_BUS_FMT_SGRBG8_1X8;
> -	subformat.size = { cfg.size.width / 3, cfg.size.height / 3 };
> +	subformat.size = sensorSize;
>   
>   	ret = data->sensor_->setFormat(&subformat);
>   	if (ret)
> @@ -293,7 +312,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>   	 * vimc driver will fail pipeline validation.
>   	 */
>   	format.fourcc = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8);
> -	format.size = { cfg.size.width / 3, cfg.size.height / 3 };
> +	format.size = sensorSize;
>   
>   	ret = data->raw_->setFormat(&format);
>   	if (ret)
Dan Scally June 1, 2024, 11:17 p.m. UTC | #2 
On 01/06/2024 23:44, Dan Scally wrote:
> Hi Laurent
>
> On 29/05/2024 16:43, Laurent Pinchart wrote:
>> Starting in kernel v5.16, the vimc driver stopped hardcoding the scaler
>> factor. Use this to lift constraints on the camera configuration, and in
>> particular on the exotic output size alignment to a multiple of 6. As a
>> result, vimc-based cameras can more easily match common display
>> resolutions.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
> Looks good to me too: Daniel Scally <dan.scally@ideasonboard.com>


Err, supposed to be a "Reviewed-by: " in there somewhere...

>
>> ---
>>   src/libcamera/pipeline/vimc/vimc.cpp | 47 +++++++++++++++++++---------
>>   1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index c7650432ddcc..0ec9928eec23 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -114,6 +114,9 @@ static const std::map<PixelFormat, uint32_t> pixelformats{
>>       { formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },
>>   };
>>   +static constexpr Size kMinSize{ 16, 16 };
>> +static constexpr Size kMaxSize{ 4096, 2160 };
>> +
>>   } /* namespace */
>> VimcCameraConfiguration::VimcCameraConfiguration(VimcCameraData *data)
>> @@ -153,14 +156,20 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>>       const Size size = cfg.size;
>>         /*
>> -     * The scaler hardcodes a x3 scale-up ratio, and the sensor output size
>> -     * is aligned to two pixels in both directions. The output width and
>> -     * height thus have to be multiples of 6.
>> +     * The sensor output size is aligned to two pixels in both directions.
>> +     * Additionally, prior to v5.16, the scaler hardcodes a x3 scale-up
>> +     * ratio, requiring the output width and height to be multiples of 6.
>>        */
>> -    cfg.size.width = std::max(48U, std::min(4096U, cfg.size.width));
>> -    cfg.size.height = std::max(48U, std::min(2160U, cfg.size.height));
>> -    cfg.size.width -= cfg.size.width % 6;
>> -    cfg.size.height -= cfg.size.height % 6;
>> +    Size minSize{ kMinSize };
>> +    unsigned int alignment = 2;
>> +
>> +    if (data_->media_->version() < KERNEL_VERSION(5, 16, 0)) {
>> +        minSize *= 3;
>> +        alignment *= 3;
>> +    }
>> +
>> +    cfg.size.expandTo(minSize).boundTo(kMaxSize)
>> +        .alignDownTo(alignment, alignment);
>>         if (cfg.size != size) {
>>           LOG(VIMC, Debug)
>> @@ -216,10 +225,12 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,
>>               }
>>           }
>>   -        /* The scaler hardcodes a x3 scale-up ratio. */
>> -        std::vector<SizeRange> sizes{
>> -            SizeRange{ { 48, 48 }, { 4096, 2160 } }
>> -        };
>> +        /* Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. */
>> +        Size minSize{ kMinSize };
>> +        if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
>> +            minSize *= 3;
>> +
>> +        std::vector<SizeRange> sizes{ { minSize, kMaxSize } };
>>           formats[pixelformat.first] = sizes;
>>       }
>>   @@ -242,10 +253,18 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration 
>> *config)
>>       StreamConfiguration &cfg = config->at(0);
>>       int ret;
>>   -    /* The scaler hardcodes a x3 scale-up ratio. */
>> +    /*
>> +     * Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. For newer
>> +     * kernels, use a sensor resolution of 1920x1080 and let the scaler
>> +     * produce the requested stream size.
>> +     */
>> +    Size sensorSize{ 1920, 1080 };
>> +    if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
>> +        sensorSize = { cfg.size.width / 3, cfg.size.height / 3 };
>> +
>>       V4L2SubdeviceFormat subformat = {};
>>       subformat.code = MEDIA_BUS_FMT_SGRBG8_1X8;
>> -    subformat.size = { cfg.size.width / 3, cfg.size.height / 3 };
>> +    subformat.size = sensorSize;
>>         ret = data->sensor_->setFormat(&subformat);
>>       if (ret)
>> @@ -293,7 +312,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>>        * vimc driver will fail pipeline validation.
>>        */
>>       format.fourcc = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8);
>> -    format.size = { cfg.size.width / 3, cfg.size.height / 3 };
>> +    format.size = sensorSize;
>>         ret = data->raw_->setFormat(&format);
>>       if (ret)

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index c7650432ddcc..0ec9928eec23 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -114,6 +114,9 @@  static const std::map<PixelFormat, uint32_t> pixelformats{
 	{ formats::BGR888, MEDIA_BUS_FMT_RGB888_1X24 },
 };
 
+static constexpr Size kMinSize{ 16, 16 };
+static constexpr Size kMaxSize{ 4096, 2160 };
+
 } /* namespace */
 
 VimcCameraConfiguration::VimcCameraConfiguration(VimcCameraData *data)
@@ -153,14 +156,20 @@  CameraConfiguration::Status VimcCameraConfiguration::validate()
 	const Size size = cfg.size;
 
 	/*
-	 * The scaler hardcodes a x3 scale-up ratio, and the sensor output size
-	 * is aligned to two pixels in both directions. The output width and
-	 * height thus have to be multiples of 6.
+	 * The sensor output size is aligned to two pixels in both directions.
+	 * Additionally, prior to v5.16, the scaler hardcodes a x3 scale-up
+	 * ratio, requiring the output width and height to be multiples of 6.
 	 */
-	cfg.size.width = std::max(48U, std::min(4096U, cfg.size.width));
-	cfg.size.height = std::max(48U, std::min(2160U, cfg.size.height));
-	cfg.size.width -= cfg.size.width % 6;
-	cfg.size.height -= cfg.size.height % 6;
+	Size minSize{ kMinSize };
+	unsigned int alignment = 2;
+
+	if (data_->media_->version() < KERNEL_VERSION(5, 16, 0)) {
+		minSize *= 3;
+		alignment *= 3;
+	}
+
+	cfg.size.expandTo(minSize).boundTo(kMaxSize)
+		.alignDownTo(alignment, alignment);
 
 	if (cfg.size != size) {
 		LOG(VIMC, Debug)
@@ -216,10 +225,12 @@  PipelineHandlerVimc::generateConfiguration(Camera *camera,
 			}
 		}
 
-		/* The scaler hardcodes a x3 scale-up ratio. */
-		std::vector<SizeRange> sizes{
-			SizeRange{ { 48, 48 }, { 4096, 2160 } }
-		};
+		/* Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. */
+		Size minSize{ kMinSize };
+		if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
+			minSize *= 3;
+
+		std::vector<SizeRange> sizes{ { minSize, kMaxSize } };
 		formats[pixelformat.first] = sizes;
 	}
 
@@ -242,10 +253,18 @@  int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
 	StreamConfiguration &cfg = config->at(0);
 	int ret;
 
-	/* The scaler hardcodes a x3 scale-up ratio. */
+	/*
+	 * Prior to v5.16, the scaler hardcodes a x3 scale-up ratio. For newer
+	 * kernels, use a sensor resolution of 1920x1080 and let the scaler
+	 * produce the requested stream size.
+	 */
+	Size sensorSize{ 1920, 1080 };
+	if (data->media_->version() < KERNEL_VERSION(5, 16, 0))
+		sensorSize = { cfg.size.width / 3, cfg.size.height / 3 };
+
 	V4L2SubdeviceFormat subformat = {};
 	subformat.code = MEDIA_BUS_FMT_SGRBG8_1X8;
-	subformat.size = { cfg.size.width / 3, cfg.size.height / 3 };
+	subformat.size = sensorSize;
 
 	ret = data->sensor_->setFormat(&subformat);
 	if (ret)
@@ -293,7 +312,7 @@  int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
 	 * vimc driver will fail pipeline validation.
 	 */
 	format.fourcc = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8);
-	format.size = { cfg.size.width / 3, cfg.size.height / 3 };
+	format.size = sensorSize;
 
 	ret = data->raw_->setFormat(&format);
 	if (ret)