[libcamera-devel] Constraints on the sensor subdevice

[Jacopo Mondi]

Hi Naush,

let me reply one last time to clarify a few points, then I'll let
other reply with their views.

On Fri, Feb 21, 2020 at 01:58:27PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Fri, 21 Feb 2020 at 11:55, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Naush,
>
>  <Snip>
>
> > > Agreed, this is our thinking as well.  We do exactly this in our
> > > pipeline handler.  But does that mean that CameraSensor::getFormat()
> > > is somewhat redundant?
> > >
> >
> > Well, not really, as there is no guarantee on what the sensor can
> > actually produce. CameraSensor::getFormat() is designed to provide the
> > closest available approximation to the pipeline handler requested
> > format/resolution, and to avoid each pipeline handler to implement
> > that logic by themselves in a slightly different way.
> >
> > The alternative is for pipeline handlers to deal with that by
> > themselves and inspect all the possible format/sizes combination, but
> > we really would like to provide a better API than what the raw V4L2
> > API would give.
> >
> > Do you think you would need to implement that logic by yourselves in
> > the pipeline handler ?
>
> I still struggle to see how getFormat() could abstract out all the
> constraints of the hardware platform.  It seems to me the pipeline

It does not. Pipeline handlers know the contraints of the platform and
it's capabilities and from there decides that it wants images produced
by the sensor in format F and sizes (WxH) because that's the best
combination to provide to the ISP to produce whatever the application
requested.

The CameraSensor::getFormat() recevies "F, (WxH)" and returns the best
approximation the sensor can produce. This last part is the one I
don't want to see duplicated. How to get to "F, (WxH)" it's totally up
to pipeline handlers implementation.

> handler might be better suited to make this decision based on a list
> of all available sensor modes and formats, particularly if the CSI-2
> Rx is more capable of doing things like reformat and rescaling.
>

I think pipeline handlers should decide that, based on the ISP
constraints, ask whatever format they desire to
CameraSensor::getFormat() (which accepts a list of mbus codes in
preference order), get back from there best approximation of what they
asked, and work from there with that. They should not manually inspect
the matrix of mbus-sizes provided by the sensor. That's the raw V4L2
API we're trying to abstrat away from.

> > I would really try to make CameraSenso::getFormat()
> > aware of the fact that different mbus codes could be associated with
> > different resolutions (which was the issue where this thread actually
> > started from :D).
>
> Yes, that was what originally begun this discussion :)  There is a
> hard failure if this is true.
>
> >
> > Anyway, scratch that, seems like you have no camera sensor at all :)
> >
> >
> > > > The right place to do so is indeed the validation phase. I think the
> > > > IPU3 pipeline provides a good example in that regard. validate() is
> > > > not only intended to validate the application requested configuration,
> > > > but also serves to adjust it to a valid version and to prepare the
> > > > platform to support the requested use case by, as an example,
> > > > selecting which format/size to request to the camera sensor to produce
> > > > the requested streams configuration.
> > > >
> > >
> > > > >
> > > > > Another thing to keep in mind is that for our pipeline, we do a
> > > > > setFormat() on the CSI2 image node as a V4L2DeviceFormat and never on
> > > > > the subdevice directly.  The mode gets propagated by the CSI2 kernel
> > > > > driver to the subdevice to set the appropriate sensor mode.  This
> > > > > means that our pipeline handler never actually deals with MBUS codes,
> > > > > rather only V4L2 4CCs and the V4L2Subdevice::getFormat() call does not
> > > > > get used by us.  Now I'm not sure that this is the right or wrong
> > > > > thing to do :)  I seem to recall talking with @Dave Stevenson about
> > > > > this, and we concluded that this is probably ok?
> > > >
> > > > 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.
> > >
> >
> > Oh :(
> >
> > > 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,
> >
> > CSI-2 receivers could as well perform format conversion/scaling when
> > used with an inline ISP (CSI-2 rx feeding data directly to the
> > ISP without going through memory). To me this sounds like a separate
> > topic as where the ISP is fed from is not directly dependent on the
> > ability of the CSI-2 receiver to perform image manipulation.
> >
> > > 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 still sounds like userspace duty to me. I understand the list of
> > 4cc formats available on the receiver might be constructed by
> > inspecting the list of available subdevice mbus codes, and that's imo
> > a fair thing to do in kernel space. But format selection should be
> > delegated to uspace, and fail eventually at pipeline start time if the
> > mbus/4cc combination programmed on the (sensor source) -> (csi2 sink)
> > link is not supported.
> >
>
> The logic to select the format still lives in userland (in code very
> similar to CameraSensor::getFormat).  The only thing the CSI-2 kernel
> driver does is effectively translate MBUS code to V4L2 4CC.
>

That's because you are effectively performing the sensor subdevice format
selection procedure in kernel space. You don't need any format
selection in user space on the sensor like it happens, say, for IPU3:
https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n1385

You want to access the CSI-2 video device format lists and select from
there, there is no CameraSensor abstraction involved afaict.

> > Anyway, not something to discuss with me only as I can only provide my
> > personal interpretation of the issue :) I'm sure on your way to upstream
> > you'll have better feedback than this.
> >
>
> Yes, of course we do want to follow the community's guidelines, so if
> there is pushback, we will have to re-evaluate our approach here.
>
> > > > 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
> > > :)
> >
> > It does, if I got your driver architecture right.

Just to clarify: "It makes sense" meant "I get what you're saying".

I don't think it's sane though. Userspace should be able to configure
formats on all subdevices, not let kernel driver magic infer that, but
again, linux-media will tell you if that's ok or not.

> >
> > Back to our original topics:
> >
> > delegated to a specialized CameraSensor subclass that handles all the
> > required subdevices exposed by the sensor driver. As the interesting
> > part for you here is the additional embedded data subdevice, what
> > operation do you envision to happen on it ? Does format configuration
> > happen independently from the format configuration on the video data
> > subdevice or do you have any kernel space logic to handle that too ?
> > I'm thinking that you should probably better go with V4L2Subdevice
> > direct interfacing, even if that would require you to duplicate a lot
> > of what we do in CameraSensor class for properties handling in your
> > pipeline handler, and I would really like that not to happen :)
> >
>
> The CSI-2 Rx driver advertises two nodes, image and embedded data.  So
> format configuration for each of those does happen as independent
> operations via userland through the standard V4L2 API.
>

Again, I don't see any actual use for CameraSensor in your case if not
collecting and reporting properties (if you use them).

> > 2) For image format selection: for you it would be really enough to
> > access the raw ImageFormats even if, at this point, I'm not sure what
> > you would use them for, if all the mbus code selection logic is
> > deferred to kernel space. Is that a matter of just retrieving the
> > available resolution per-mbus code to correctly program
> > scaling/cropping on the CSI-2 Rx video device I assume.
> >
>
> Our pipeline hander does access the raw ImageFormats to choose the
> mode.  The CSI-2 Rx driver does not do any selection of modes, it
> merely advertises the available modes on the subdevice (but with MBUS
> codes translated to V4L2 4cc).  For our use cases, doing this mode
> selection on V4L2 4CC vs doing it on MBUS codes does not make much of
> a difference really.  Given that ImageFormats presented format 4CCs +
> resolutions, that is what I went with.

ImageFormats reports 4cc code as you're using the one reported from
the CSI-2 video device. Again, not something that should then go
through the CameraSensor class then imo.

> > For libcamera this last point is indeed something that should be
> > solved in the long run, in a way that supports other platforms making use of
> > CameraSensor::getFormat(), by expanding that operation to take into
> > account per-mbus code resolution lists, as suggested in some previous email.
> > Giving access to the whole list of ImageFormats should be possible too,
> > even if that would make it tempting for pipeline handler implementers
> > to shortcut the CameraSensor class format selection logic and
> > re-implement the same in their own custom way (and that would be just
> > more duplicated code to review/maintain), which is exactly the reason
> > why we refrained from doing that in first place :)
> >
> > Now that I hope we have nailed down a little better what the
> > requirements are, I would really like to know what others think as
> > well :)
>
> Yes, it would be useful to get other perspectives on this :)
>

Hope to get feedback as well :)

Thanks
  j
-------------- 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/1b120334/attachment.sig>