[libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor ISP format mismatch
Sebastian Fricke
sebastian.fricke at posteo.net
Wed Mar 17 08:07:16 CET 2021
Hello Dafna, Laurent and Heiko,
thank you all for your reviews, I have looked further into the problem
and would like to share my thoughts with you.
Comments below...
On 15.03.2021 17:52, Dafna Hirschfeld wrote:
>
>
>On 12.03.21 22:31, Laurent Pinchart wrote:
>>Hi Dafna,
>>
>>On Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:
>>>On 02.03.21 03:39, Laurent Pinchart wrote:
>>>>On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:
>>>>>On 28.02.21 16:49, Laurent Pinchart wrote:
>>>>>>On Sat, Feb 27, 2021 at 07:01:26PM +0100, 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.
>>>>>>
>>>>>>It would have been nice if the ISP had an input crop rectangle :-S It
>>>>>>would have allowed us to crop the input image a little bit to 4032x2992
>>>>>>(keeping the same aspect ratio) or 4032x3024 (4:3). Just
>>>>>>double-checking, is there indeed no way to crop at the input, or could
>>>>>>it be that it's not implemented in the driver ? I can't find the
>>>>>>4032x3024 limit in the documentation I have access to.
>>>>>
>>>>>The isp does support cropping on the sink pad.
>>>>>But currently the function v4l2_subdev_link_validate_default compares
>>>>>the dimensions defined in v4l2_subdev_format which are not the crop
>>>>>dimensions but the ones set by set_fmt. Is that wrong?
>>>>
>>>>I think so. If the ISP supports a larger size than 4032x3024 before
>>>>cropping, it should accept that on its sink pad, with the sink crop
>>>>rectangle being adjusted to never be larger than 4032x3024 (for instance
>>>>when userspace sets the format on the sink pad, the crop rectangle could
>>>>be automatically set to the same size, clamped to 4032x3024 and
>>>>centered). Userspace can then adjust the crop rectangle further if
>>>>necessary.
>>>
>>>In rkisp1-isp.c, there is diagram of the cropping regions.
>>>It says that the 4032x3024 limit is 'for black level'.
>>>Does that means that some sensors might send frames with black pixels in
>>>the edges that need to be cropped?
>>
>>In this context, I believe that black level refers to black level
>>correction (BLC in short), which is the process of subtracting a fixed
>>value from all pixels to account for leakage currents and other
>>parasitic effects in the sensor that makes fully black pixels have a
>>non-zero value. Sensors typically have a set of lines before the image
>>that is physically covered, and those lines can then be transmitted over
>>CSI-2. The average black level will then be computed on the SoC side
>>(either using the CPU, or in the ISP if it supports doing so), and
>>configured in the ISP to subtract it from all pixels. The black lines
>>are cropped out of what the ISP processes further down the pipeline.
>
>The number of black/invalid lines depends on the sensor right?
>So userspace should adjust the cropping according to the sensor.
>If so then why would we want to always clamp to 4032x3024 ?
>Or should it just be the initial value and userspace can then increase
>the crop size?
>
>>
>>>The TRM says "Maximum input resolution of 4416x3312 pixels"
>>>The datasheet then shows that the default values are 4096x3072 for both the
>>>input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).
>>>
>>>So from the docs at least it sound possible to do as you suggested,
>>>limit the input to 4416x3312 and then always set a crop with maximum
>>>value 4032x3024
>>
>>Sounds like it's worth a try at least :-)
Dafna advised me to try and set the MAX and MIN size simply to
4416x3312, so I did and here are the resulting images.
I captured one video with these settings:
`LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=50 --file=/tmp/out_video -s height=600,width=800,pixelformat=NV12,role=video`
libcamera then sets the camera pipeline like this:
```
[0:35:57.915030227] [6669] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 2112x1568-SBGGR10_1X10
[0:35:57.915069602] [6669] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 2112x1568-SBGGR10_1X10
[0:35:57.915125018] [6669] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
[0:35:57.915152143] [6669] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915183643] [6669] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915234684] [6669] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915262101] [6669] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 800x600-YUYV8_2X8
[0:35:57.915284850] [6669] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 800x600-YUYV8_1_5X8
```
And another video with these settings:
`cam --camera=1 --capture=50 --file=/tmp/out_video -s height=2160,width=3840,pixelformat=NV12,role=video`
libcamera sets the camera pipeline like this:
```
[0:52:25.445115742] [8997] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
[0:52:25.445158617] [8997] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
[0:52:25.445214616] [8997] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
[0:52:25.445242616] [8997] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445274991] [8997] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445325449] [8997] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445351116] [8997] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3840x2160-YUYV8_2X8
[0:52:25.445372990] [8997] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3840x2160-YUYV8_1_5X8
```
Here are the results of both captures (I have taken roughly the same
frame number):
https://imgur.com/a/EFtEmB9
The image quality of the higher resolution image is obviously better and
it is also quite a lot brighter. I was not able to detect any defect
pixels in the image so far.
But I noticed the following:
My sensor has a Black Level Calibration (BLC) register (@ 0x5001),
usually, BLC is enabled. If I turn it off and try to capture with a
sensor resolution of 4224x3136, then the pipeline can still be
created, validated and the buffers are queued, but it seems like the sensor
doesn't start anymore. When I use the lower resolution mode of the sensor
(2112x1568), while deactivating BLC everything works just fine.
So, something happens when BLC is off when the resolution is greater
than 4032x3024. (Sadly no part of the camera pipeline actually throws an
error)
Here is the debug log from cam:
```
[0:11:23.311130186] [4131] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
[0:11:23.311269310] [4131] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
[0:11:23.311451309] [4131] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
[0:11:23.311571766] [4131] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.311728682] [4131] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.311918555] [4131] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.312134679] [4131] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
[0:11:23.312276428] [4131] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8
[0:11:23.313918792] [4131] INFO IPARkISP1 rkisp1.cpp:120 Exposure: 4-3324 Gain: 16-248
[0:11:23.314731370] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Analogue Gain to 16 at index 1
[0:11:23.314913660] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Exposure to 4 at index 1
[0:11:23.336376391] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
[0:11:23.337981130] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 0 buffers requested.
[0:11:23.338467627] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339042498] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339212247] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339364496] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339509453] [4130] DEBUG Camera camera.cpp:1029 Starting capture
[0:11:23.340191657] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video3[15:out]: 4 buffers requested.
[0:11:23.341328566] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video2[14:cap]: 4 buffers requested.
[0:11:23.344065548] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
[0:11:23.344281088] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1351 /dev/video0[17:cap]: Prepared to import 4 buffers
Capture 5 frames
[0:11:23.346221533] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 0
[0:11:23.346598364] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 0
[0:11:23.346800779] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 0
[0:11:23.347141443] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 1
[0:11:23.347319651] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 1
[0:11:23.347473941] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 1
[0:11:23.347759189] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 2
[0:11:23.347921938] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 2
[0:11:23.348139520] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 2
[0:11:23.404159310] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 3
[0:11:23.404329934] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 3
[0:11:23.404401100] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 3
[0:11:23.404646390] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[15:out]: Dequeuing buffer 0
^CExiting
```
Conclusion:
At first glance, I thought it might be fine to just increase the
resolution limits, but after seeing that this only works when the sensor
performs black level calibration, I think that it would be unwise to
perform this change.
I am now working on the initial idea to crop the sink pad of the ISP
down to 4032x3024, if and only if the input resolution is greater than
4032x3024 and smaller or equal to 4416x3312 (because I don't want to
create a crop to 4032x3024 when the input resolution is for example
2112x1568). That crop is automatically propagated to the source pad of
the ISP within `rkisp1_isp_set_sink_crop` and within
`rkisp1_isp_set_src_fmt` we use the crop resolution as format
resolution. I will post that patch soon to the mailing list.
>>
>>>>>>>Signed-off-by: Sebastian Fricke <sebastian.fricke at posteo.net>
>>>>>>>---
>>>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---
>>>>>>> 1 file changed, 31 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>>diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>>>>index 50eaa6a4..56a406c1 100644
>>>>>>>--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>>>>+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>>>>@@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>>>>> return Invalid;
>>>>>>> }
>>>>>>>- /* Select the sensor format. */
>>>>>>>- Size maxSize;
>>>>>>>+ /* Get the ISP resolution limits */
>>>>>>>+ V4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);
>>>>>>
>>>>>>Related to 1/2, note that you don't necessarily need to store the ISP
>>>>>>pointer in RkISP1CameraData. You can access the pipeline handler here:
>>>>>>
>>>>>> PipelineHandlerRkISP1 *pipe =
>>>>>> static_cast<PipelineHandlerRkISP1 *>(data_->pipe_);
>>>>>> V4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);
>>>>>>
>>>>>>>+ if (ispFormats.empty()) {
>>>>>>>+ LOG(RkISP1, Error) << "Unable to fetch ISP formats.";
>>>>>>>+ return Invalid;
>>>>>>>+ }
>>>>>>
>>>>>>Missing blank line.
>>>>>>
>>>>>>>+ /*
>>>>>>>+ * 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;
>>>>>>>+
>>>>>>>+ /*
>>>>>>>+ * Select the sensor format, use either the best fit to the configured
>>>>>>>+ * format or a specific sensor format, when getFormat would choose a
>>>>>>>+ * resolution that surpasses the ISP maximum.
>>>>>>>+ */
>>>>>>>+ Size maxSensorSize;
>>>>>>>+ for (const Size &size : sensor->sizes()) {
>>>>>>>+ if (size.width > ispMaximum.width ||
>>>>>>>+ size.height > ispMaximum.height)
>>>>>>>+ continue;
>>>>>>
>>>>>>This makes me think we need better comparison functions for the Size
>>>>>>class. Maybe a Size::contains() function ? It doesn't have to be part of
>>>>>>this series.
I would love to work on that right after this patch and the connected
kernel patch :).
>>>>>>
>>>>>>>+ maxSensorSize = std::max(maxSensorSize, size);
>>>>>>>+ }
>>>>>>
>>>>>>Missing blank line here too.
>>>>>>
>>>>>>Could we move all the code above to
>>>>>>PipelineHandlerRkISP1::createCamera() and store maxSensorSize in
>>>>>>RkISP1CameraData, to avoid doing the calculation every time validate()
>>>>>>is called ?
Yes, that is a good idea, and actually when I think about it the most
logical location. I hope that my use case will be handled by the kernel
patch, but I will still post this patch in case a camera sensor comes
along with a resolution greater than 4416x3312. I will test the
libcamera patch without the kernel patch to make sure that it works. But
it would be great if someone could test the patch in combination with
the kernel patch, with an appropriate sensor. (I would also be open to
any recommendations as I don't know if such a sensor exists for my
platform).
>>>>>>
>>>>>>
>>>>>>>+ Size maxConfigSize;
>>>>>>> for (const StreamConfiguration &cfg : config_)
>>>>>>>- maxSize = std::max(maxSize, cfg.size);
>>>>>>>+ maxConfigSize = std::max(maxConfigSize, cfg.size);
>>>>>>>+
>>>>>>>+ if (maxConfigSize.height <= maxSensorSize.height &&
>>>>>>>+ maxConfigSize.width <= maxSensorSize.width)
>>>>>>>+ maxSensorSize = maxConfigSize;
>>>>>>> sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
>>>>>
>>>>>I wonder if it won't be an easier solution to add another optional parameter
>>>>>to the getFormat method of the sensor that limits the size that the sensor
>>>>>should return. This might benefit other pipline-handlers as well where
>>>>>a sensor is connected to an entity that has a maximal size it can accept.
>>>>>In rkisp1 all the above code could be replaced by just adding
>>>>>Size(4032,3024) as another parameter to getFormat.
I could add that in another patch, but I believe that if I don't require
that for the rkisp1, I would first wait for an actual use case to
appear.
>>>>>
>>>>>>> MEDIA_BUS_FMT_SGBRG12_1X12,
>>>>>>>@@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>>>>>> MEDIA_BUS_FMT_SGBRG8_1X8,
>>>>>>> MEDIA_BUS_FMT_SGRBG8_1X8,
>>>>>>> MEDIA_BUS_FMT_SRGGB8_1X8 },
>>>>>>>- maxSize);
>>>>>>>+ maxSensorSize);
>>>>>>> if (sensorFormat_.size.isNull())
>>>>>>> sensorFormat_.size = sensor->resolution();
>>
Greetings,
Sebastian
More information about the libcamera-devel
mailing list