[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler
Andrey Konovalov
andrey.konovalov at linaro.org
Tue Apr 21 22:42:11 CEST 2020
Hi Laurent,
On 08.04.2020 20:40, Andrey Konovalov wrote:
> Hi Laurent,
>
> On 04.04.2020 03:12, Laurent Pinchart wrote:
>> Hi Andrey,
>>
>> On Fri, Apr 03, 2020 at 04:13:35PM +0300, Andrey Konovalov wrote:
>>> On 31.03.2020 14:45, Kieran Bingham wrote:
>>>> On 20/03/2020 01:48, Laurent Pinchart wrote:
>>>>> From: Martijn Braam <martijn at brixit.nl>
>>>>>
>>>>> This new pipeline handler aims at supporting any simple device without
>>>>> requiring any device-specific code. Simple devices are currently defined
>>>>> as a graph made of one or multiple camera sensors and a single video
>>>>> node, with each sensor connected to the video node through a linear
>>>>> pipeline.
>>>>>
>>>>> The simple pipeline handler will automatically parse the media graph,
>>>>> enumerate sensors, build supported stream configurations, and configure
>>>>> the pipeline, without any device-specific knowledge. It doesn't support
>>>>> configuration of any processing in the pipeline at the moment, but may
>>>>> be extended to support simple processing such as format conversion or
>>>>> scaling in the future.
>>>>>
>>>>> The only device-specific information in the pipeline handler is the list
>>>>> of supported drivers, required for device matching. We may be able to
>>>>> remove this in the future by matching with the simple pipeline handler
>>>>> as a last resort option, after all other pipeline handlers have been
>>>>> tried.
>>>>>
>>>>> Signed-off-by: Martijn Braam <martijn at brixit.nl>
>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>>>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>>>
>>>> Some comments and discussion points below, but I think we should work
>>>> towards trying to get this integrated so it can actually be used (which
>>>> is where we'll find the nuances and corner cases as people test it)
>>>>
>>>>> ---
>>>>> Changes since v2:
>>>>>
>>>>> - Log an error when setupFormats() fail
>>>>> - Propagate getFormat() and setFormat() errors to the caller of
>>>>> setupFormats()
>>>>> - Reorder variable declarations in validate()
>>>>> - Add \todo comment related to the selection of the default format
>>>>> - Use log Error instead of Info if pipeline isn't valid
>>>>> - Rebase on top of V4L2PixelFormat
>>>>>
>>>>> Changes since v1:
>>>>>
>>>>> - Rebase on top of buffer API rework
>>>>> - Expose stream formats
>>>>> - Rework camera data config
>>>>> ---
>>>>> src/libcamera/pipeline/meson.build | 1 +
>>>>> src/libcamera/pipeline/simple/meson.build | 3 +
>>>>> src/libcamera/pipeline/simple/simple.cpp | 710 ++++++++++++++++++++++
>>>>> 3 files changed, 714 insertions(+)
>>>>> create mode 100644 src/libcamera/pipeline/simple/meson.build
>>>>> create mode 100644 src/libcamera/pipeline/simple/simple.cpp
>>>>>
>>>>> diff --git a/src/libcamera/pipeline/meson.build b/src/libcamera/pipeline/meson.build
>>>>> index 0d466225a72e..606ba31a0319 100644
>>>>> --- a/src/libcamera/pipeline/meson.build
>>>>> +++ b/src/libcamera/pipeline/meson.build
>>>>> @@ -5,3 +5,4 @@ libcamera_sources += files([
>>>>> subdir('ipu3')
>>>>> subdir('rkisp1')
>>>>> +subdir('simple')
>>>>> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build
>>>>> new file mode 100644
>>>>> index 000000000000..4945a3e173cf
>>>>> --- /dev/null
>>>>> +++ b/src/libcamera/pipeline/simple/meson.build
>>>>> @@ -0,0 +1,3 @@
>>>>> +libcamera_sources += files([
>>>>> + 'simple.cpp',
>>>>> +])
>>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>>>> new file mode 100644
>>>
>>> <snip>
>>>
>>>>> +int SimpleCameraData::init()
>>>>> +{
>>>>> + SimplePipelineHandler *pipe = static_cast<SimplePipelineHandler *>(pipe_);
>>>>> + V4L2VideoDevice *video = pipe->video();
>>>>> + int ret;
>>>>> +
>>>>> + /*
>>>>> + * Enumerate the possible pipeline configurations. For each media bus
>>>>> + * format supported by the sensor, propagate the formats through the
>>>>> + * pipeline, and enumerate the corresponding possible V4L2 pixel
>>>>> + * formats on the video node.
>>>>> + */
>>>>> + for (unsigned int code : sensor_->mbusCodes()) {
>>>>> + V4L2SubdeviceFormat format{ code, sensor_->resolution() };
>>>>> +
>>>>> + /*
>>>>> + * Setup links first as some subdev drivers take active links
>>>>> + * into account to propaget TRY formats. So is life :-(
>>>>
>>>> /propaget/propagate/
>>>>
>>>> /So is life/Such is life/
>>>>
>>>>
>>>>> + */
>>>>> + ret = setupLinks();
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + ret = setupFormats(&format, V4L2Subdevice::TryFormat);
>>>>> + if (ret < 0) {
>>>>> + LOG(SimplePipeline, Error)
>>>>> + << "Failed to setup pipeline for media bus code "
>>>>> + << utils::hex(code, 4);
>>>>
>>>> Oh dear, seems we need mbusCode.toString() too! hehe
>>>>
>>>>> + return ret;
>>>>
>>>> Andrey states that this return prevents iterating potentially successful
>>>> mbus-codes, and should instead 'continue'...
>>>
>>> I've taken a deeper look at the ov5640 + db410c logs I've received, and the above
>>> return *could* be a potential problem, but definitely not in this (ov5640 + db410c)
>>> case.
>>>
>>> We only saw SimpleCameraData::setupFormats() returning error due to failing
>>> sensor_->setFormat(format) call.
>>> This happened if before the cam is started (and the stream cfg isn't set to a
>>> particular width*height) the sensor is configured for the frame rate greater than 15 fps.
>>> In particular, in the ov5640's driver probe function the initial mode is set to
>>> "YUV422 UYVY VGA at 30fps".
>>> ov5640_enum_frame_size() doesn't consider the current fps at all, so the maximum
>>> frame size reported is 2592x1944. But it only works at 15fps max, and an attempt
>>> to set the sensor format to a one with the maximum possible frame size of 2592x1944
>>> results in -EINVAL returned by SUBDEV_S_FMT.
>>> (So in our ov5640 + db410c case replacing the "return" with "continue" does let the pipeline
>>> handler to try all the other media bus codes if the first one fails, but they all fail the
>>> same way as the first one does.)
>>>
>>> If this issue with the frame rate is worked around in some way, the enumeration
>>> of the possible pipeline configurations completes kind of OK: a good configuration
>>> is selected in the end. But during the enumeration all the configurations, the
>>> broken ones included are considered as the working ones:
>>>
>>> The links are set up this way ("entity name":pad_number]):
>>>
>>> "ov5640 4-0076":0 -> "msm_csiphy0":0,"msm_csiphy0":1 -> "msm_csid0":0,"msm_csid0":1 ->
>>> -> "msm_ispif0":0,"msm_ispif0":1 -> "msm_vfe0_rdi0":0
>>>
>>>
>>> SimpleCameraData::setupFormats() creates a broken pipeline when it tries the sensor format
>>> not supported by the rest of the entities. This results in e.g. the following:
>>>
>>> "ov5640 4-0076":0[MEDIA_BUS_FMT_RGB565_2X8_BE] -> [MEDIA_BUS_FMT_UYVY8_2X8]"msm_csiphy0":0,
>>> "msm_csiphy0":1[MEDIA_BUS_FMT_UYVY8_2X8] -> [MEDIA_BUS_FMT_UYVY8_2X8]"msm_csid0":0, etc
>>>
>>> I.e. an attempt to set fmt on "msm_csiphy0":0 to MEDIA_BUS_FMT_RGB565_2X8_BE results in the
>>> format set to MEDIA_BUS_FMT_UYVY8_2X8 and no error is returned here (and down the pipeline
>>> as MEDIA_BUS_FMT_UYVY8_2X8 is set OK for all the other pads). So we get the first link misconfigured,
>>> but the result is considered as a good pipeline as no error was returned while the format
>>> from the sensor output was propagated from the sensor output down to "msm_vfe0_rdi0":0.
>>> And the broken configuration is stored in the formats_ map (in the end, it works in our
>>> ov5640+db410c case as the broken configurations are rewritten with the valid ones later).
>>>
>>> Who/what is to ensure that for each of the links, the source and the sink pads of the same link
>>> are configured to the same format?
>>
>> The kernel is responsible for validating that at stream on time, but I
>> think we should also verify it in setupFormats(). Would you like to send
>> a patch on top of this series ?
I've just posted the two patches to address that.
Thanks,
Andrey
> I've got something which gets to the point when 'cam' starts streaming, but no buffer comes from the
> video device.
>
> That is now SimpleCameraData::init() enumerates the possible pipeline configurations correctly (skips
> the broken ones). But with the patch the StreamConfiguration now uses 'BA81' format which doesn't
> work (though looks valid for the pipeline) instead of 'YUYV' before which worked OK. (The format is
> selected in SimplePipelineHandler::CreateConfiguration() by choosing the first entry in the formats
> map.)
>
> I am looking into this now. But "debugging by emails" isn't the fastest process unfortunately (I don't
> have ov5640 for db410c locally).
>
>
> Thanks,
> Andrey
>
>>>>> + }
>>>>> +
>>>>> + std::map<V4L2PixelFormat, std::vector<SizeRange>> videoFormats =
>>>>> + video->formats(format.mbus_code);
>>>>> +
>>>>> + LOG(SimplePipeline, Debug)
>>>>> + << "Adding configuration for " << format.size.toString()
>>>>> + << " in pixel formats [ "
>>>>> + << utils::join(videoFormats, ", ",
>>>>> + [](const auto &f) {
>>>>> + return f.first.toString();
>>>>> + })
>>>>
>>>> Aha, I kinda like the utils::join!
>>>>
>>>>> + << " ]";
>>>>> +
>>>>> + /*
>>>>> + * Store the configuration in the formats_ map, mapping
>>>>> + * PixelFormat to configuration. Any previously stored value is
>>>>
>>>> /to configuration/to the current configuration/ ?
>>>>
>>>>> + * overwritten, as the pipeline handler currently doesn't care
>>>>> + * about how a particular PixelFormat is achieved.
>>>>
>>>> As long as there's an output, I agree. If devices have specific
>>>> constraints such as better performance or such with a particular
>>>> mbus-code, then they need to create a platform specific pipeline handler
>>>> to deal with the nuances.
>>>>
>>>> Maybe there might be tuning hints or something later, but certainly not now.
>>>>
>>>>
>>>>> + */
>>>>> + for (const auto &videoFormat : videoFormats) {
>>>>> + PixelFormat pixelFormat = video->toPixelFormat(videoFormat.first);
>>>>> + if (!pixelFormat)
>>>>> + continue;
>>>>> +
>>>>> + Configuration config;
>>>>> + config.code = code;
>>>>> + config.pixelFormat = pixelFormat;
>>>>> + config.size = format.size;
>>>>> +
>>>>> + formats_[pixelFormat] = config;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (formats_.empty()) {
>>>>> + LOG(SimplePipeline, Error) << "No valid configuration found";
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>
>> [snip]
>>
More information about the libcamera-devel
mailing list