[libcamera-devel] Constraints on the sensor subdevice

[Dave Stevenson]

Hi Jacopo.

On Fri, 21 Feb 2020 at 15:42, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Dave
>
> On Fri, Feb 21, 2020 at 02:48:34PM +0000, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > I'll preface this with please don't take it as a personal attack!
>
> Absolutely :) I'm sorry if anything I've said made you feel like you
> have to specify this.

It was more in case my words could be misconstrued, or were typed in
haste. It's either that or I end up redrafting the email a dozen
times, and still sending out something that can be misinterpreted!

> > It's not meant as one, and most uses of the word "you" are more the
> > V4L2 spec/behaviour than any individual.
>
> I don't take any blame for the V4L2 specs :D

I wish someone would :-D

> >
> > On Fri, 21 Feb 2020 at 12:04, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Hi Dave,
> > >
> > > On Fri, Feb 21, 2020 at 11:25:05AM +0000, Dave Stevenson wrote:
> > > > Hi Jacopo
> > > >
> > >
> > > [snip]
> > >
> > > > > > If I got you right here, on your platform the image format selected on
> > > > > > the CSI-2 receiver gets propagated to the sensor subdevice by
> > > > > > performing the 4cc->mbus translation logic in kernel space. This
> > > > > > implies the sensor subdevice and the CSI-2 receiver drivers have to be
> > > > > > tightly coupled, unless complex mbus selection heuristics are
> > > > > > implemented in the CSI-2 receiver driver to support a wide range of
> > > > > > sensor drivers.
> > > > >
> > > > > Yes, this is correct.  The CSI-2 Rx and sensor subdevice are
> > > > > inherently coupled on our platform.
> > > > >
> > > > > There is a distinction to make here though.  A CSI-2 Rx could either
> > > > > be directly connected to an ISP pipeline, or dump out image data into
> > > > > memory to be read back by an ISP.  For the latter case, it might also
> > > > > be possible for the CSI-2 block to do some simple format conversion,
> > > > > or even rescaling.  In our platform, we only allow our CSI-2 RX to
> > > > > optionally do a data unpack operation (so a format conversion).  So,
> > > > > during probe, the kernel driver enumerates the subdevice formats, and
> > > > > generates a list of V4L formats to advertise - and these formats may
> > > > > be duplicated to represent packed and unpacked options.
> > > > >
> > > > > > This seems not standard behaviour to me, but I can't tell if upstream
> > > > > > would be fine with it or not. From a libcamera perspective, you won't
> > > > > > be able to use CameraSensor::getFormat() but you will have to select
> > > > > > which format to set on the CSI-2 receiver taking into account the mbus
> > > > > > selection logic performed by the same driver.
> > > > > >
> > > > > > Can I ask if you have any requirement that prevents you from decoupling that
> > > > > > logic in kernel space, and leave the mbus code selection on the sensor
> > > > > > to userspace ? It seems a lot of complication in the driver that
> > > > > > should instead live in your pipeline handler...
> > > > > >
> > > > >
> > > > > With our approach described above, there is no need to apply a MBUS
> > > > > format on the sensor subdevice as well as a V4L format on the CSI-2 Rx
> > > > > device as they are inherently the same operation, if that makes sense
> > > > > :)
> > > > >
> > > > > For more complicated topologies (where the CSI-2 Rx could rescale for
> > > > > example), I understand that we may want to decouple this, but for now,
> > > > > MBUS codes are essentially hidden from our pipeline handler.
> > > >
> > > > I originally wrote this driver based on the TI VPFE [1]. That's a
> > > > non-MC device, and suited our requirements nicely.
> > > > From an application perspective we want the simple use cases, so you
> > > > set the format and that is what you get. Beyond repacking from eg
> > > > V4L2_PIX_FMT_SBGGR10P to V4L2_PIX_FMT_SBGGR10 there is no image
> > > > processing going on.
> > > >
> > > > The change to use media controller was actually your libcamera
> > > > requirement for enumeration of devices. We have no need for it, so
> > > > would sooner rip it back out.
> > > > Complicating userspace apps for such a simple hardware block is nuts.
> > >
> > > I'm sorry I really don't agree here, but I don't think this comes
> > > unexpected :)
> > >
> > > Implementing heuristics for 4cc->mbus code translation in kernel space
> > > requires the csi-2 receiver and the sensor subdevice driver to be
> > > somehow coupled,
> >
> > Please define what you mean by coupling?
> > They are coupled in that they are given handles to each other via
> > device tree. They have to be coupled to start/stop streaming if
> > nothing else. V4L2 subdev API calls are made between the two. The V4L2
> > subdev API also defines pad operations for kernel drivers to set and
> > get formats.
> >
>
> Ok, let me take a step back and make this email thread a nice exchange of
> personal views, as that's what I can provide :)
>
> In the ideal world where I would like to live, all new code that gets
> upstreamd is MC centric. The number of subdev ops calls between driver
> is minimized and most of the configuration is performed from
> userspace, in order to finally end the 'single video device node'
> abstraction, which is limiting what applications can do on each single
> piece of the pipeline, is limiting the number of feature each block
> can expose, is blocking applications from implementing advanced use
> cases and overall is limiting V4L2 from being suitable to support most
> of modern and complex camera usages out there (I know, this is a
> simplification, but please bear with me, I know you are not in love
> with the V4L2 API too :)
>
> I myself have upstreamed a driver for a legacy platform which does
> exactly what you describe, so I'm not saying "this is wrong" by
> itself. What I'm saying is that, as you're now looking into libcamera,
> you have a space for implementing that logic in uspace, freeing your
> driver from any heuristic and let them just implement mechanisms to
> apply what user requires to HW, and let drivers focus on exposing
> 'features' to application instead of implementing 'logic'
>
> I know this is theory, I know it's nice on paper, I know when you have
> customers the world outside there looks very different, but a man can
> dream, can't he ?

I can hear the snoring as you dream away ;-)

If MC had really gained any traction in userspace then that theory
would be great, but it never has, so we're slightly stuck.

> > > or better, it's very hard to guarantee that your csi-2
> > > driver format selection logic work with all sensor drivers out there.
> > > Am I wrong ?
> >
> > Looking in the mainline tree I can see:
> > - TI am437x
> > - Atmel ISC/ISI
> > - Exynos4-is
> > - possibly via-camera
> > - (TI) cal
> > all of which have a table of V4L2_PIX_FMT_ to MEDIA_BUS_FMT_ format conversions.
>
> None of them is exactly a new implementation. I think they all come
> from the 'single video device node' era, and have been ported to MC
> later, but I get your point: you're not doing anything particularly
> nasty or not in line with existing code in v4l2, I agree.

A quick check says none of them use MC except Exynos4-is, probably
because of this enforced userspace behaviour change if you do enable
it :-/

> > Yes there is the potential for missing one, but we've already worked
> > around the slight craziness of there being both
> > MEDIA_BUS_FMT_YUYV8_1X8 and MEDIA_BUS_FMT_YUYV8_1X16 and similar (CSI
> > is really 16X1 as well seeing as it's serial!).
> > Where is the *obligation* to update libcamera format selection logic
> > when one adds a new MBUS format to the kernel?
> >
> > > Delegating that to userspace would allow you to de-couple the two drivers
> > > completely, but I understand libcamera was not there when you first
> > > designed that. Now that you have a place in userspace where to
> > > implement those heuristics (your pipeline handler) I really would not
> > > push them to kernel space.
> >
> > Where's the obligation to use libcamera in all situations?!
> >
>
> Ah well, we aim to conquer the world, you should know that already :)
>
> > Forget the complex camera use case, we have a trivially simple
> > hardware pipeline. CSI2 data is sent from a source and is written to
> > memory. Why should userspace be forced to worry about media controller
> > and MBUS formats at all?
> >
>
> Ah, this question have been asked multiple times from many vendors,
> and I really get your point: Why should I do a complex thing and
> complicate my users lives if I can get away with a simpler solution.
>
> My perspective might be mistaken because I thought we where talking
> about a new design. Nobody is asking you to trash whatever you have
> and I will always be on the "better to have good code upstream than
> perfect code out-of-tree" boat.
>
> > We have existing pipelines working with bcm2835-unicam using the
> > Toshiba TC358743 HDMI to CSI2 chip, and Analog Devices ADV728xM
> > analogue video to CSI chip that should only be broken with very good
> > reason. In both cases you have to set the timings/standard first, but
> > then it streams data on request.
> > GStreamer's v4l2src works very nicely with it, and nothing worries
> > about media controller, and why should it. GStreamer can switch
> > merrily between the UYVY or BGR3 output formats of TC358743 via
> > V4L2_PIX_FMT_xxx as they get forwarded to the TC358743 driver (as
> > MEDIA_BUS_FMT_xxx) to action.
> >
> > With the behaviour of media controller you're now proposing,
> > GStreamer, and FFmpeg, and ALL other clients every have to talk to
> > media controller to set up that format change. I don't see code
> > supporting that in v4l2src, so I have an issue that has to be
> > resolved.
>
> v4l2src does not know anything about MC and we have a libcamera gstreamer
> element on the list, but yes, it is still a new thing.

I know libcamera is coming, and with GStreamer support, but forcing
changes on users gets serious pushback.

> > Paragraph 8 of [1] does state
> > "Formats are not propagated across links, as that would involve
> > propagating them from one sub-device file handle to another.
> > Applications must then take care to configure both ends of every link
> > explicitly with compatible formats. Identical formats on the two ends
> > of a link are guaranteed to be compatible. Drivers are free to accept
> > different formats matching device requirements as being compatible."
> > which to my mind is barking mad in the simple case, and puts a
> > significant burden on all apps if the pipeline is simple.
> >
>
> We might get into philosophy here, but if you are considering a new
> design, defining it with the "simple" use case only in mind is really
> tying your hand in regards to any future-proof use case.
>
> If instead you have a lot of existing code, a lot of established
> users, and you feel like you need to trash it away just to make us
> pesky libcamera people happy, that's not what I meant to say at all.

It's probably not a huge number of existing users, but they are there,
and people whinge like mad if you break their systems!

The unicam driver has been kicking around for a couple of years, and
keeps getting fixed as my understanding of V4L2 is frequently
corrected or Hans updates the compliance tests to expose yet another
wrong assumption by me!

> > How does one support the simple applications if the underlying device
> > insists on using media controller?
> >
>
> libcamera :D ?

**rolls eyes** ;-)

> > [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/dev-subdev.html#format-negotiation
> >
> >
> > A question in return. Other than enumeration, what does media
> > controller gain us in this situation?
> > We don't need the request API.
> > We don't need to configure/validate any links or set/get pad formats.
>
> I understand, and again, I thought we were talking about a new design,
> and I didn't get why you wanted so badly to force everything to go
> through a single device node.
>
> The benefit of MC are self evident to me in the perspective of
> designing and implementing new code that is able to support any future
> extension to allow more complex use cases to happen on my platform.
>
> I understand you have a huge user base which just care of the most
> simple use case, and I understand for them, and for you
> consequentially, the benefits are secondary compared to the
> complications.
>
> > > Again, my 2c only, the linux-media community will provide much more> > > interesting feedbacks then mine :)
> >
> > At the moment I'm seeing us pushing the driver without media
> > controller, and we'll have to come up with an alternative mechanism
> > for enumeration.
> > Media controller support can come later because it is going to create
> > regressions. Possibly make media controller usage optional? Switch
> > based on device tree, or perhaps based on whether /dev/mediaX is
> > opened or not. (but how do you tell if that is the same app that is
> > asking for /dev/videoX?!)
> > Perhaps we need to look at uvcvideo as that supports media controller
> > and yet allows setting resolution and seemingly it propagating to the
> > sensor device. Does it only get away with this as the sub entities are
> > not V4L2 subdevices? It seems to be a very similar situation.
> >
>
> I happly defer this question to linux-media, Laurent and Kieran.

Kieran added the MC support for us, so the format setting change
really stems from that. I wasn't aware of the required behaviour
change at that time :-(

It'd be nice to know Laurent and Kieran's take on this, particularly
Laurent's with his knowledge of UVC and how that avoids this.

One thought that we're just trying. If we don't call
v4l2_device_register_subdev_nodes to register the subdevices, then
media controller still exists but logically it still makes sense in
that the format is set via the video node. No subdevice nodes means
you can't set the format any other way.
All still a bit of a kludge, but may be a solution that is vaguely acceptable.

> Hope you get that all the above is my personal interpretation and it
> is not my intention to define any upstream acceptance criteria for
> your driver nor for libcamera.

Fully understood. I know I'm the outsider with my own preconceived views too :-)

  Dave