[libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor ISP format mismatch
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 24 02:31:51 CET 2021
Hi Sebastian and Dafna,
Sorry for the late reply.
On Wed, Mar 17, 2021 at 08:07:16AM +0100, Sebastian Fricke wrote:
> 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:
> >> 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?
That's right.
> > So userspace should adjust the cropping according to the sensor.
It depends a bit on the sensor. Most sensors will not by default send
the optical black lines, so the ISP just doesn't receive them, and
there's nothing to crop. We can usually instruct the sensor to send
those lines (V4L2 is missing an API to do so properly), in which case
they can be send as a fixed number of lines before and/or after the
image that we would then need to instruct the ISP to crop (or to handle
in a special way, depending on the ISP). For CSI-2 sensors, the optical
black lines can be tagged with a different data type, and the CSI-2
receiver can then (if it supports this feature) capture them in a
separate buffer. There are thus lots of options, depending on the
hardware capabilities.
In this specific case, I don't think the sensor sends us any optical
black lines (at least not with the main image data type).
> > 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?
As I understand it, the ISP can't process images larger than 4032x3024
(due to limitations of the processing blocks inside the ISP). It however
can receive large frames at its input and crop them before any further
processing, so a sensor that would send a size larger than 4032x3024
could be supported.
This is based on the information I have so far, I haven't studied the
TRM in depth, so details may not be correct, especially the exact limits
on the resolution (information from different documents seems to
disagree, based on this e-mail discussion).
> >>> 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)
That sounds very fishy. BLC is the process of subtracting the black
level from all pixels. It shouldn't affect anything that the rkisp1
would care about. It may be an issue internal to the sensor.
This being said, maybe there's a side effect of the sensor sending the
black lines out and thus making the image size larger when BLC is
disabled (unlikely, but OmniVision...). This could affect the ISP. You
could try to play with cropping on the sensor side to reduce the height
a little bit and see if it fixes things.
> 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.
Can't we always enable BLC on the sensor ?
> 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.
>
> >>>>>
> >>>>>>> @@ -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();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list