archive mirror
 help / color / mirror / Atom feed
From: Hugues FRUCHET <>
To: Laurent Pinchart <>,
	Sakari Ailus <>,
	Hans Verkuil <>
Cc: Alexandre TORGUE <>,
	Mauro Carvalho Chehab <>,
	"" <>,
	"" <>,
	Benjamin Gaignard <>,
	Yannick FERTRE <>,
	Philippe CORNU <>,
	Mickael GUENE <>
Subject: Re: [PATCH v2 0/3] DCMI bridge support
Date: Thu, 27 Jun 2019 12:38:40 +0000	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Hi Laurent,

Thanks for reviewing,

On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> Hi Hugues,
> On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>   > - Where's the sub-device representing the bridge itself?
>> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
>>   > - As the driver becomes MC-centric, crop configuration takes place
>> through
>>   >   V4L2 sub-device interface, not through the video device node.
>>   > - Same goes for accessing sensor configuration: it does not take place
>>   >   through video node but through the sub-device nodes.
>> Our objective is to be able to support either a simple parallel sensor
>> or a CSI-2 sensor connected through a bridge without any changes on
>> userspace side because no additional processing or conversion involved,
>> only deserialisation is m.
>> With the proposed set of patches, we succeeded to do so, the same
>> non-regression tests campaign is passed with OV5640 parallel sensor
>> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with
>> D3 mezzanine board).
>> We don't want driver to be MC-centric, media controller support was
>> required only to get access to the set of functions needed to link and
>> walk trough subdevices: media_create_pad_link(),
>> media_entity_remote_pad(), etc...
>> We did a try with the v1 version of this patchset, delegating subdevices
>> handling to userspace, by using media-controller, but this require to
>> configure first the pipeline for each single change of resolution and
>> format before making any capture using v4l2-ctl or GStreamer, quite
>> heavy in fact.
>> Benjamin did another try using new libcamera codebase, but even for a
>> basic capture use-case, negotiation code is quite tricky in order to
>> match the right subdevices bus format to the required V4L2 format.
> Why would it be trickier in userspace than in the kernel ? The V4L2
> subdev operations are more or less expose verbatim through the subdev
> userspace API.
>> Moreover, it was not clear how to call libcamera library prior to any
>> v4l2-ctl or GStreamer calls.
> libcamera isn't meant to be called before v4l2-ctl or GStreamer.
> Applications are supposed to be based directly on libcamera, or, for
> existing userspace APIs such as V4L2 or GStreamer, compatibility layers
> are supposed to be developed. For V4L2 it will take the form of a
> LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
> V4L2 applications work with libcamera unmodified (I said most as 100%
> compatibility will likely not be achievable). For GStreamer it will take
> the form of a GStreamer libcamera element that will replace the V4L2
> source element.
>> Adding 100 lines of code into DCMI to well configure resolution and
>> formats fixes the point and allows us to keep backward compatibility
>> as per our objective, so it seems far more reasonable to us to do so
>> even if DCMI controls more than the subdevice it is connected to.
>> Moreover we found similar code in other video interfaces code like
>> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole
>> pipeline, so it seems to us quite natural to go this way.
> I can't comment on the qcom-camss driver as I'm not aware of its
> internals, but where have you found such code in the Xilinx V4L2 drivers
> ?
For ex. in xilinx/xilinx-dma.c, stream on/off is propagated to all 
subdevices within pipeline:
  * Walk the entities chain starting at the pipeline output video node 
static int xvip_pipeline_start_stop(struct xvip_pipeline *pipe, bool start)

Same for qcom/camss/camss-video.c:
static int video_start_streaming(struct vb2_queue *q, unsigned int count)

For resolution/format, in exynos4-is/fimc-capture.c:
static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
	while (1) {
		/* set format on all pipeline subdevs */
		while (me != &fimc->vid_cap.subdev.entity) {
			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);

>> To summarize, if we cannot do the negotiation within kernel, delegating
>> this to userspace implies far more complexity and breaks compatibility
>> with existing applications without adding new functionalities.
>> Having all that in mind, what should be reconsidered in your opinion
>> Sakari ? Do you have some alternatives ?
> First of all, let's note that your patch series performs to related but
> still independent changes: it enables MC support, *and* enables the V4L2
> subdev userspace API. The former is clearly needed and will allow you to
> use the MC API internally in the kernel, simplifying pipeline traversal.
> The latter then enables the V4L2 subdev userspace API, moving the
> pipeline configuration responsibility to userspace.
> You could in theory move to the MC API inside the kernel, without
> enabling support for the V4L2 subdev userspace API. Configuring the
> pipeline and propagating the formats would then be the responsibility of
> the kernel driver.

Yes this is exactly what we want to do.
If I understand well, to disable the V4L2 subdev userspace API, I just 
have to remove the media device registry:
-	/* Register the media device */
-	ret = media_device_register(&dcmi->mdev);
-	if (ret) {
-		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
-			ret);
-		goto err_media_device_cleanup;
-	}
Do you see any additional things to do ?

> However, this will limit your driver to the
> following:
> - Fully linear pipelines only (single sensor)
> - No support for controls implemented by multiple entities in the
>    pipeline (for instance controls that would exist in both the sensor
>    and the bridge, such as gains)
> - No proper support for scaling configuration if multiple components in
>    the pipeline can scale
> Are you willing to set those limitations in stone and give up on
> supporting those features ?

The involved hardware do not have those features, no need of extra 
functionalities to be exposed to userspace, so this is fine.

I'll push a v3 with this change and the other fixes related to Sakari 
and Hans comments.

Please Sakari & Hans, also comment on that change that we can converge 
on v3.

Best regards,

  reply	other threads:[~2019-06-27 12:39 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-11  8:48 [PATCH v2 0/3] DCMI bridge support Hugues Fruchet
2019-06-11  8:48 ` [PATCH v2 1/3] media: stm32-dcmi: improve sensor subdev naming Hugues Fruchet
2019-06-20 15:26   ` Sakari Ailus
2019-07-02 15:21     ` Hugues FRUCHET
2019-06-11  8:48 ` [PATCH v2 2/3] media: stm32-dcmi: add media controller support Hugues Fruchet
2019-06-20 12:01   ` Hans Verkuil
2019-07-02 15:18     ` Hugues FRUCHET
2019-06-20 16:13   ` Sakari Ailus
2019-07-02 15:29     ` Hugues FRUCHET
2019-06-11  8:48 ` [PATCH v2 3/3] media: stm32-dcmi: add support of several sub-devices Hugues Fruchet
2019-06-20 15:54   ` Sakari Ailus
2019-07-02 15:26     ` Hugues FRUCHET
2019-06-20 16:17 ` [PATCH v2 0/3] DCMI bridge support Sakari Ailus
2019-06-24 10:10   ` Hugues FRUCHET
2019-06-26 17:25     ` Laurent Pinchart
2019-06-27 12:38       ` Hugues FRUCHET [this message]
2019-06-27 13:38         ` Laurent Pinchart
2019-07-05  7:55           ` Sakari Ailus
2019-07-05  8:04             ` Laurent Pinchart
2019-07-05  9:16               ` Sakari Ailus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).