[libcamera-devel] [PATCH v3] pipeline-rkisp1-Fix-sensor-ISP-format-mismatch
Helen Koike
helen.koike at collabora.com
Mon Dec 14 13:04:50 CET 2020
Hi Sebastian,
On 12/13/20 10:42 AM, Sebastian Fricke wrote:
> This patch fixes a mismatch of image formats during the pipeline
> creation of the RkISP1. The mismatch happens because the current code
> does not check if the configured format exceeds the maximum viable
> resolution of the ISP.
>
> Make sure to use a sensor format resolution that is smaller or equal to
> the maximum allowed resolution for the RkISP1. The maximum resolution
> is defined within the `rkisp1-common.h` file as:
> define RKISP1_ISP_MAX_WIDTH 4032
> define RKISP1_ISP_MAX_HEIGHT 3024
>
> Enumerate the frame-sizes of the ISP entity and compare the maximum with
> the configured resolution.
>
> This means that some camera sensors can never operate with their maximum
> resolution, for example on my OV13850 camera sensor, there are two
> possible resolutions: 4224x3136 & 2112x1568, the first of those two will
> never be picked as it surpasses the maximum of the ISP.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
> ---
>
> Attention: This patch depends on the following patch for the Linux
> Kernel media subsystem.
> https://patchwork.kernel.org/project/linux-media/patch/20201212185306.19135-1-sebastian.fricke.linux@gmail.com/
>
> v1: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015113.html
> v2: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015136.html
>
> Changelog:
>
> Changes since v2:
>
> * Replace the act of attempting to set the ISP format before negotiating
> the actual format for both the ISP input pad and the sensor in order
> to get the maximum frame size. With a logic that involves enumerating
> the maximum size directly from the subdevice and using that size for
> the negotiation process.
> * Improve the log messages
>
> Changes since v1:
>
> * Change snake_case variable names to camelCase
> * Use the request comment style
> * Correct the scope of the newly implemented variables
> * Correct the subject of the debug log for the ISP format configuration
> * Update the comment above the ISP format configuration
> * Check if the original format is not equal to the configured ISP format
> instead of checking if it is greater, this denies a false positive
> where the height exceeds the maximum while the width is smaller.
> If the configured format does not exceed the maximum resolution of the
> ISP, it will stay untouched so the inequality always means that we
> have to reconfigure the format.
> * Adjust the comparison of the ISP format size with the available sensor
> formats, to detect a false-positive were the width is smaller while
> the height exceeds the maximum
> * Use the standard function `max`
>
> -----
>
> The following tests were all able to create a working camera pipeline:
> 1. Without stream configuration
> 2. With a normal stream configuration
> 3. With a stream configuration that exceeds the maximum of the ISP
> 4. With a very small resolution stream configuration
> 5. With a configuration that is closer to the upper than to the lower
> resolution
>
> -----
>
> 1. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3
>
> [7:56:14.564557994] [9200] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
> [7:56:14.565294449] [9200] INFO Camera camera.cpp:890 configuring streams: (0) 1920x1920-NV12
> ...
> [7:56:14.567584898] [9201] INFO RkISP1 rkisp1.cpp:689 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
> [7:56:14.567738605] [9201] DEBUG RkISP1 rkisp1.cpp:713 Sensor configured with 2112x1568-SBGGR10_1X10
> [7:56:14.567941896] [9201] DEBUG RkISP1 rkisp1.cpp:724 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [7:56:14.568196812] [9201] DEBUG RkISP1 rkisp1.cpp:730 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:56:14.568360144] [9201] DEBUG RkISP1 rkisp1.cpp:742 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:56:14.568529602] [9201] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:56:14.568647435] [9201] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
> [7:56:14.568762351] [9201] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8
>
> 2. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=900,height=600,pixelformat=NV12,role=video
>
> [7:57:58.012056371] [9204] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
> [7:57:58.012524202] [9204] INFO Camera camera.cpp:890 configuring streams: (0) 900x600-NV12
> ...
> [7:57:58.014565860] [9205] DEBUG RkISP1 rkisp1.cpp:713 Sensor configured with 2112x1568-SBGGR10_1X10
> [7:57:58.014776442] [9205] DEBUG RkISP1 rkisp1.cpp:724 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [7:57:58.014913816] [9205] DEBUG RkISP1 rkisp1.cpp:730 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:57:58.015064607] [9205] DEBUG RkISP1 rkisp1.cpp:742 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:57:58.015230273] [9205] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:57:58.015348689] [9205] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 900x600-YUYV8_2X8
> [7:57:58.015463314] [9205] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 900x600-YUYV8_1_5X8
>
> 3. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=4500,height=3500,pixelformat=NV12,role=video
>
> [7:59:52.764968202] [9210] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
> [7:59:52.765415033] [9210] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
> [7:59:52.765671115] [9210] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 1920x1920-NV12
> [7:59:52.765905613] [9210] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
> [7:59:52.766147987] [9210] DEBUG CameraSensor camera_sensor.cpp:436 'ov13850 1-0010': No supported format or size found
> Camera configuration adjusted
> [7:59:52.766558943] [9210] DEBUG CameraSensor camera_sensor.cpp:436 'ov13850 1-0010': No supported format or size found
> [7:59:52.766702151] [9210] INFO Camera camera.cpp:890 configuring streams: (0) 4416x3312-NV12
> ...
> [7:59:52.768968097] [9211] INFO RkISP1 rkisp1.cpp:689 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
> [7:59:52.769120347] [9211] DEBUG RkISP1 rkisp1.cpp:713 Sensor configured with 2112x1568-SBGGR10_1X10
> [7:59:52.769320721] [9211] DEBUG RkISP1 rkisp1.cpp:724 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [7:59:52.769455762] [9211] DEBUG RkISP1 rkisp1.cpp:730 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:59:52.769606552] [9211] DEBUG RkISP1 rkisp1.cpp:742 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:59:52.769771927] [9211] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [7:59:52.769888301] [9211] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 4416x3312-YUYV8_2X8
> [7:59:52.770003217] [9211] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 4416x3312-YUYV8_1_5X8
>
> 4. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=40,height=30,pixelformat=NV12,role=video
>
> [8:03:00.496627233] [9218] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
> [8:03:00.497419103] [9218] INFO Camera camera.cpp:890 configuring streams: (0) 40x30-NV12
> ...
> [8:03:00.499715966] [9219] DEBUG RkISP1 rkisp1.cpp:713 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:03:00.499924215] [9219] DEBUG RkISP1 rkisp1.cpp:724 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [8:03:00.500168047] [9219] DEBUG RkISP1 rkisp1.cpp:730 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:03:00.500336630] [9219] DEBUG RkISP1 rkisp1.cpp:742 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:03:00.500513087] [9219] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:03:00.500632962] [9219] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 40x30-YUYV8_2X8
> [8:03:00.500745836] [9219] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 40x30-YUYV8_1_5X8
>
> 5. LIBCAMERA_LOG_LEVELS=0 cam -c 1 --capture=3 -s width=3450,height=2456,pixelformat=NV12,role=video
>
> [8:01:23.689086810] [9215] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
> [8:01:23.689893848] [9215] INFO Camera camera.cpp:890 configuring streams: (0) 3450x2456-NV12
> ...
> [8:01:23.692257211] [9216] INFO RkISP1 rkisp1.cpp:689 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
> [8:01:23.692415876] [9216] DEBUG RkISP1 rkisp1.cpp:713 Sensor configured with 2112x1568-SBGGR10_1X10
> [8:01:23.692615084] [9216] DEBUG RkISP1 rkisp1.cpp:724 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [8:01:23.692753041] [9216] DEBUG RkISP1 rkisp1.cpp:730 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:01:23.692901499] [9216] DEBUG RkISP1 rkisp1.cpp:742 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:01:23.693066581] [9216] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [8:01:23.693183831] [9216] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3450x2456-YUYV8_2X8
> [8:01:23.693297289] [9216] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3450x2456-YUYV8_1_5X8
>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 34 +++++++++++++++++++++++-
> 1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4d98c902..d6fc624c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -667,12 +667,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> if (ret)
> return ret;
>
> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> + if (ispFormats.empty())
> + return -1;
Returning -1 doesn't seem appropriated. Please check how other functions return errors.
I would guess -EINVAL.
> +
> + /*
> + * The maximum resolution is identical for all media bus codes on
> + * the RkISP1 isp entity. Therefore take the first available resolution.
> + */
> + Size ispMaximum = ispFormats.begin()->second[0].max;
> +
> /*
> * Configure the format on the sensor output and propagate it through
> * the pipeline.
> */
> V4L2SubdeviceFormat format = config->sensorFormat();
> - LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
> +
> + if (ispMaximum.width < format.size.width ||
> + ispMaximum.height < format.size.height) {
> + Size maxSize;
> + LOG(RkISP1, Info) << "Sensor format " << format.size.toString()
> + << " is not supported by the ISP (maximum: "
> + << ispMaximum.toString() << "), trying to "
> + << "re-configure to a smaller sensor format";
> +
> + for (const Size &size : sensor->sizes()) {
> + if (size.width > ispMaximum.width ||
> + size.height > ispMaximum.height)
> + continue;
> + maxSize = std::max(maxSize, size);
> + }
> + if (maxSize == Size(0, 0)) {
> + LOG(RkISP1, Error) << "No available sensor resolution"
> + "that is smaller or equal to "
> + << format.toString();
> + return -1;
Returning -1 doesn't seem appropriated. Please check how other functions return errors.
I would guess -EINVAL.
With this change:
Reviewed-by: Helen Koike <helen.koike at collabora.com>
Thanks
Helen
> + }
> + format.size = maxSize;
> + }
>
> ret = sensor->setFormat(&format);
> if (ret < 0)
>
More information about the libcamera-devel
mailing list