[libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor ISP format mismatch
Dafna Hirschfeld
dafna.hirschfeld at collabora.com
Tue Mar 23 12:56:11 CET 2021
On 19.03.21 08:19, Sebastian Fricke wrote:
> 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.
Hi, note that the function v4l2_subdev_link_validate_default compares the pads of a link, that is, the two pads belong
to different entities in both side of the link. So inside the isp entity it is valid to have "sink_fmt > sink_crop > src_fmt > src_crop"
since the src_* and sink_* values belong to different pads of the same entity and not to pads of the same link.
>
> 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]
This configuration seems valid to me.
Do you get EPIPE when streaming with this configuration?
Thanks,
Dafna
> ```
>
> 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