[libcamera-devel] Constraints on the sensor subdevice

[Dave Stevenson]

Hi Jacopo

I'll preface this with please don't take it as a personal attack!
It's not meant as one, and most uses of the word "you" are more the
V4L2 spec/behaviour than any individual.

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.

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

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?!

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?

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.

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.

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

[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.

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

  Dave