[libcamera-devel] Constraints on the sensor subdevice

[Jacopo Mondi]

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

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

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

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

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

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

libcamera :D ?

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

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.

Thanks
  j

>   Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200221/031f8146/attachment.sig>