[libcamera-devel] [PATCH v3 09/11] libcamera: pipeline: Add a simple pipeline handler
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Apr 4 02:12:02 CEST 2020
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 ?
> >> + }
> >> +
> >> + 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]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list