[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler
Andrey Konovalov
andrey.konovalov at linaro.org
Wed Apr 8 19:40:08 CEST 2020
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 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