[libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor ISP format mismatch
Sebastian Fricke
sebastian.fricke at posteo.net
Fri Mar 19 08:19:01 CET 2021
Hello Laurent, Dafna and Helen,
I have investigated this issue and have written some thoughts below.
On 01.03.2021 08:48, Dafna Hirschfeld wrote:
>Hi
>
>On 28.02.21 16:49, Laurent Pinchart wrote:
>>Hi Sebastian,
>>
>>Thank you for the patch.
>>
>>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 have looked into the code and I noticed that the cropping ability of
the ISP sink pad is more like a dummy functionality. You are able to
set a crop that is smaller than the format for the sink pad, but there
is a chain of events that make this condition invalid. When
`rkisp1_isp_set_sink_fmt` is called it automatically calls
`rkisp1_isp_set_sink_crop` with the last used crop for the pad
(0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to
the `sink_fmt` so that it is within the bounds of the new format. Then
it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on
the source pad, it maps the `src_crop` into the `sink_crop`. And finally
`rkisp1_isp_set_src_fmt` is called, which sets the width and height of
the format with the width and height of the crop.
We validate the links with `v4l2_subdev_link_validate_default`, which
checks if the formats of the pads are equal.
Therefore I can state the following:
- the sink pad crop is bounded by the sink pad format
- the source pad crop is bounded by the sink pad crop
- the source pad format is bounded by the source pad format
Meaning: when the sink pad format != sink pad crop, then the sink pad format
and source pad format can never be equal. And therefore can never be
validated.
The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),
mentions the following statement for the sink pad of the ISP:
"the size should be equal/less than sensor input size."
and for the source pad of the ISP:
"The size should be equal/less than sink pad size."
This means from the ISP's point of view, the following pad configuration
should probably work:
```
- entity 1: rkisp1_isp (4 pads, 5 links)
...
pad0: Sink
[fmt:SBGGR10_1X10/4224x3136 field:none
crop.bounds:(0,0)/4224x3136
crop:(96,72)/4032x2992]
<- "ov13850 1-0010":0 [ENABLED]
...
pad2: Source
[fmt:YUYV8_2X8/4032x2992 field:none
crop.bounds:(0,0)/4032x2992
crop:(0,0)/4032x2992]
-> "rkisp1_resizer_mainpath":0 [ENABLED]
-> "rkisp1_resizer_selfpath":0 []
...
- entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)
...
pad0: Sink
[fmt:YUYV8_2X8/4032x2992 field:none
crop.bounds:(0,0)/4032x2992
crop:(0,0)/4032x2992]
<- "rkisp1_isp":2 [ENABLED]
pad1: Source
[fmt:YUYV8_2X8/1920x1920 field:none]
-> "rkisp1_mainpath":0 [ENABLED,IMMUTABLE]
- entity 28: ov13850 1-0010 (1 pad, 1 link)
...
pad0: Source
[fmt:SBGGR10_1X10/4224x3136 at 20000/150000 field:none
crop.bounds:(16,8)/4224x3136
crop:(16,8)/4224x3136]
-> "rkisp1_isp":0 [ENABLED]
```
But the problem is that this not how the link validation currently works,
when the format sizes are not equal `v4l2_subdev_link_validate_default`,
will definitely fail.
Now, I have 3 ideas in mind for how to continue with this issue.
1. We could modify the callback to validate media links, that would
explicitly handle ISP sink and source differently, and check if the crops
align when the sink format size is greater than 4032x3024. I think that
this option is quite ugly and too specific.
2. We could add a new function similar to `v4l2_subdev_link_validate_default`,
that performs the same checks but additionally also checks if the crop
of a sink pad is equal to the format of a source pad when the format
size validation failed. I am not too sure about this either.
3. We don't touch this part of the code and I fix the issue only in
libcamera and the 4224x3136 mode of my camera cannot be used :(.
I am not too happy with any of the solutions, that I have proposed, I
would love to hear your thoughts and ideas.
Greetings,
Sebastian
>
>>
>>>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.
>>
>>>+ 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 ?
>>
>>
>>>+ 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.
>
>Thanks,
>Dafna
>
>>> 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();
>>
More information about the libcamera-devel
mailing list