[libcamera-devel] [PATCH v5 2/2] pipeline: rkisp1: Fix sensor ISP format mismatch
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 24 02:41:39 CET 2021
Hi Dafna and Sebastian,
On Tue, Mar 23, 2021 at 12:56:11PM +0100, Dafna Hirschfeld wrote:
> On 19.03.21 08:19, Sebastian Fricke wrote:
> > On 01.03.2021 08:48, 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 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.
It's actually more than valid, it's fairly normal, given that cropping
can only produce a smaller (or equal) image :-)
> > 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."
Note that there are two things to consider: the hardware capabilities,
and the driver implementation. The latter could be incorrect in some
areas (which is the topic of this whole e-mail thread), so you can
challenge any driver implementation decision if they don't match the
hardware capabilities.
> > 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?
The configuration seems valid to me too.
> > ```
> >
> > 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.
I think failures need to be investigated, because, as Dafna explained,
link validation will compare formats on the two end of each link, not
formats on different pads of the same subdev. Validation of the
configuration of a given subdev (for instance the formats and crop
rectangles on the rkisp1_isp subdev) is something that is handled by the
subdev drivers themselves, at .set_format() and .set_selection() time,
not when starting the stream.
You can enable the debugging messages in
v4l2_subdev_link_validate_default() to get more information about what
fails exactly (and possibly add more debugging messages in other places
if the existing ones don't provide enough information to debug the
issue).
> > 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.
> >
> >>>> 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();
> >>>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list