[libcamera-devel] [PATCH v4] pipeline-rkisp1-Fix-sensor-ISP-format-mismatch
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Mon Feb 15 21:11:17 CET 2021
Hi
On 15.02.21 14:00, Helen Koike wrote:
> Hi Sebastian,
>
>
> On 2/7/21 9:58 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. Additionally, make sure that the media bus code
>> of the format is valid on the ISP.
>>
>> 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 at posteo.net>
>
> This patch doesn't compile, I believe things changed since you submitted.
> Could you submit a fixed version please?
>
> Thanks
> Helen
>
>> ---
>>
>> v1: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015113.html
>> v2: https://lists.libcamera.org/pipermail/libcamera-devel/2020-November/015136.html
>> v3: https://patchwork.libcamera.org/patch/10660/
>>
>> Changelog:
>>
>> Changes since v3:
>>
>> * Introduce a helper method for finding the correct sensor format:
>> `findBestFitFormat`
>> * Besides checking for a valid format resolution also check for a
>> matching media bus code
>> * Return -EINVAL on an error instead of -1
>> * Add documentation for the `findBestFitFormat` method
>>
>> 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
>>
>> [6:41:00.383092648] [28903] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>> [6:41:00.383284855] [28903] INFO Camera camera.cpp:890 configuring streams: (0) 1920x1920-NV12
>> ...
>> [6:41:00.384053394] [28904] INFO RkISP1 rkisp1.cpp:573 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>> [6:41:00.384124851] [28904] DEBUG RkISP1 rkisp1.cpp:637 Sensor configured with 2112x1568-SBGGR10_1X10
>> [6:41:00.384184351] [28904] DEBUG RkISP1 rkisp1.cpp:648 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>> [6:41:00.384214684] [28904] DEBUG RkISP1 rkisp1.cpp:654 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:41:00.384252309] [28904] DEBUG RkISP1 rkisp1.cpp:666 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:41:00.384295184] [28904] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:41:00.384324059] [28904] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
>> [6:41:00.384351475] [28904] 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
>>
>> [6:51:11.037976397] [28922] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>> [6:51:11.038439562] [28922] INFO Camera camera.cpp:890 configuring streams: (0) 900x600-NV12
>> ...
>> [6:51:11.040661843] [28923] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>> [6:51:11.040884092] [28923] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>> [6:51:11.041015633] [28923] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:51:11.041182757] [28923] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:51:11.041374089] [28923] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:51:11.041498339] [28923] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 900x600-YUYV8_2X8
>> [6:51:11.041619380] [28923] 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
>>
>> [6:52:41.156286265] [28926] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>> [6:52:41.156612639] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
>> [6:52:41.156745055] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 1920x1920-NV12
>> [6:52:41.156849471] [28926] DEBUG RkISP1 rkisp1_path.cpp:94 Adjusting format from 4500x3500-NV12 to 4416x3312-NV12
>> [6:52:41.156959137] [28926] DEBUG CameraSensor camera_sensor.cpp:560 'ov13850 1-0010': No supported format or size found
>> Camera configuration adjusted
>> [6:52:41.157176136] [28926] DEBUG CameraSensor camera_sensor.cpp:560 'ov13850 1-0010': No supported format or size found
>> [6:52:41.157245844] [28926] INFO Camera camera.cpp:890 configuring streams: (0) 4416x3312-NV12
>> ...
>> [6:52:41.159387000] [28927] INFO RkISP1 rkisp1.cpp:585 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>> [6:52:41.159552958] [28927] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>> [6:52:41.159752749] [28927] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>> [6:52:41.159883123] [28927] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:52:41.160129289] [28927] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:52:41.160322079] [28927] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:52:41.160451870] [28927] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 4416x3312-YUYV8_2X8
>> [6:52:41.160574370] [28927] 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
>>
>> [6:53:43.547487714] [28930] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>> [6:53:43.547902462] [28930] INFO Camera camera.cpp:890 configuring streams: (0) 40x30-NV12
>> ...
>> [6:53:43.550449033] [28931] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>> [6:53:43.550650282] [28931] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>> [6:53:43.550773657] [28931] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:53:43.550932323] [28931] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:53:43.551103822] [28931] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:53:43.551222530] [28931] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 40x30-YUYV8_2X8
>> [6:53:43.551334237] [28931] 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
>>
>> [6:54:43.212517584] [28933] DEBUG Camera camera.cpp:831 streams configuration: (0) 1920x1920-NV12
>> [6:54:43.212946040] [28933] INFO Camera camera.cpp:890 configuring streams: (0) 3450x2456-NV12
>> ...
>> [6:54:43.215050739] [28934] INFO RkISP1 rkisp1.cpp:585 Sensor format 4224x3136 is not supported by the ISP (maximum: 4032x3024), trying to re-configure to a smaller sensor format
>> [6:54:43.215213779] [28934] DEBUG RkISP1 rkisp1.cpp:609 Sensor configured with 2112x1568-SBGGR10_1X10
>> [6:54:43.215416487] [28934] DEBUG RkISP1 rkisp1.cpp:620 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
>> [6:54:43.215545986] [28934] DEBUG RkISP1 rkisp1.cpp:626 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:54:43.215710485] [28934] DEBUG RkISP1 rkisp1.cpp:638 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:54:43.215891026] [28934] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
>> [6:54:43.216104233] [28934] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3450x2456-YUYV8_2X8
>> [6:54:43.216246274] [28934] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3450x2456-YUYV8_1_5X8
>>
>> ---
>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 67 ++++++++++++++++++++++++
>> 1 file changed, 67 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index e85979a7..55b1a10d 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -163,6 +163,8 @@ private:
>> void paramReady(FrameBuffer *buffer);
>> void statReady(FrameBuffer *buffer);
>> void frameStart(uint32_t sequence);
>> + int findBestFitFormat(V4L2SubdeviceFormat *format,
>> + CameraSensor *sensor);
>> int allocateBuffers(Camera *camera);
>> int freeBuffers(Camera *camera);
>> @@ -551,6 +553,67 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>> return config;
>> }
>> +/**
>> + * \brief Find the closest possible match to the desired configuration format,
>> + * that is valid on the RkISP1.
>> + * \param[out] format The configuration format for the image sensor
>> + * \param[in] sensor The camera sensor object
>> + * \return 0 on success, -1 on failure
>> + */
>> +int PipelineHandlerRkISP1::findBestFitFormat(
>> + V4L2SubdeviceFormat *format, CameraSensor *sensor)
>> +{
>> + V4L2Subdevice::Formats ispFormats = isp_->formats(0);
>> + if (ispFormats.empty()) {
>> + LOG(RkISP1, Error) << "Unable to fetch ISP formats.";
>> + return -1;
>> + }
>> + /*
>> + * 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;
>> +
>> + 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()) {
The sizes are sorted in increasing order and you want to get the max size
so it make sense to iterate it in reverse and break once a valid size is found.
>> + if (size.width > ispMaximum.width ||
>> + size.height > ispMaximum.height)
>> + continue;
>> + maxSize = std::max(maxSize, size);
>> + }
I think you can add this loop to the validate function just before the
sensor->getFormat call.
Then the 'maxSize' can be the maxSize sent to the sensor->getFormat
Thanks,
Dafna
>> + if (maxSize == Size(0, 0)) {
>> + LOG(RkISP1, Error) << "No avail. sensor resolution <= "
>> + << format->toString();
>> + return -1;
>> + }
>> + format->size = maxSize;
>> + }
>> +
>> + auto mbusCodeMatch = ispFormats.find(format->mbus_code);
>> + if (mbusCodeMatch == ispFormats.end()) {
>> + format->mbus_code = 0;
>> + for (unsigned int mbusCode : sensor->mbusCodes()) {
>> + mbusCodeMatch = ispFormats.find(mbusCode);
>> + if (mbusCodeMatch != ispFormats.end()) {
>> + format->mbus_code = mbusCode;
>> + break;
>> + }
>> + }
>> + if (format->mbus_code == 0) {
>> + LOG(RkISP1, Error) << "No valid sensor mbus-code found";
>> + return -1;
>> + }
>> + }
>> + return 0;
>> +}
>> +
>> int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>> {
>> RkISP1CameraConfiguration *config =
>> @@ -570,6 +633,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>> V4L2SubdeviceFormat format = config->sensorFormat();
>> LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
>> + ret = findBestFitFormat(&format, sensor);
>> + if (ret < 0)
>> + return -EINVAL;
>> +
>> ret = sensor->setFormat(&format);
>> if (ret < 0)
>> return ret;
>>
More information about the libcamera-devel
mailing list