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,
Hugues.
