[libcamera-devel] Constraints on the sensor subdevice

[Jacopo Mondi]

Hello Naush,

On Fri, Feb 07, 2020 at 04:01:10PM +0000, Naushir Patuck wrote:
> HI Jacopo,
>
>
> On Thu, 6 Feb 2020 at 19:03, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hello Naush,
> >
> > On Thu, Feb 06, 2020 at 10:21:06AM +0000, Naushir Patuck wrote:
> > > Hi all,
> > >
> > > I just wanted to query a couple of constraints on the sensor subdevice
> > > that we have come across (in camera._sensor.cpp -
> > > CameraSensor::init()):
> > >
> >
> > Thanks, both your points are very appropriate. Limitations where known
> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/camera_sensor.cpp#n37
> > but I think it's now the right time to address them \o/
> >
> > I will reply to only the first point here, but won't forget the
> > second, I pinky-promise that :)
>
> I will remember this :)
>
> >
> > > 1) Sensor subdevice must have only one pad.
> > > As you know, right now we are overloading the subdevice pad to
> > > advertise sensor embedded data is available.  Eventually, we intend to
> > > add a "stream" property to the subdevice that can be used instead, but
> > > that may be some time before that's ready.  Would it be possible to
> > > remove this constraint?
> >
> > yes indeed. I just submitted a series that introduces a factory of
> > camera sensor handler, as the idea here is that management of more
> > complex devices (hopefully all of them :) should be delegated to a
> > CameraSensor sub-class.
> >
> > Sensor-specific handlers are meant to precisely interface with
> > sensor and their associated drivers, including handling more complex
> > topologies exposed to userspace such as in the case you have here
> > described.
> >
> > The question I now would have is which one, among the available entites
> > exposed by a sensor driver, should be used to 'identify' the sensor
> > when asking the factory to create an handler (have a look at
> > CameraSensorFactory::createSensor(MediaEntity *entity) in my series).
> >
> > I presume it is fair to consider the sensor's subdevice most close to
> > the ISP the one that is used to identify the sensor. In most cases, if
> > not all, it will be the CSI-2 transmitter entity. Once the specialized
> > sensor handler class receives this at creation time, it can walk the
> > links directed to its sink pads backward, to discover all the other
> > subdevices incrementally.
> >
> > Would something like that work for you ?
> >
>
> I need to have a look at the patch-set to see what is available, but
> it sounds like this could replace the code that I was talking about
> earlier.  I'll look at this again early next week and let me get back
> to you then.

I have now sent to the ML v3 of said patch series in case you want to
have a look and comment there.

>
> Regards,
> Naush
>
>
> > Thanks
> >    j
> >
> > >
> > > 3) Sensor subdevices must advertise the same frame sizes for all MBUS codes.
> > > There are sensors that we currently use (e.g. ov5647) that advertise a
> > > number of 10-bit modes (preferred) as well as a single 8-bit more
> > > (legacy use).  Other sensors may require lower bit depth formats for
> > > fast frame rate modes as another example.  With these configurations,
> > > this constraint seems too restrictive.
> > >
> > > From the code in camera_sensor.cpp, I can see why this is needed
> > > (mbusCodes_ and sizes_ are associated std::vectors.  Switching to
> > > using something like class ImageFormats (std::map with MBUS code as a
> > > key) would seem to solve this problem.  What do you think?
> > >

I think you're right, that returning a plain ImageFormats where each
mbus code is associated with a set of available resolutions would
expose to CameraSensor class the whole set of possible combinations.

We decided not to do so as the idea was to provide a better API for
pipeline handlers to retrieve the right mbus/size combination,
abstracting away from what the v4l2 subdevice provides. This is
implemented in CameraSensor::getFormat() as you can see.
Right now getFormat() is limited to work on single set of resolutions
(CameraSensor::sizez_) which is assumed to be the same for all mbus
code, as you have noticed, at CameraSensor::init() time.

My personal feeling is that we should refrain from exposing the whole
ImageFormat provided by the v4l2 subdevice to pipeline handlers, and
still enforce the usage of getFormat() as the 'right' way to obtain the
desired mbus/resolution. That method should be modified to work on
multiple sets of sizes, one per reported per-mbus-code and that would
require changing the way resolutions are stored at init() time and
inspected at getFormat() time.

Do you think expanding getFormat() in the way described above would
work for your use case?

Thanks
   j


> > > Regards,
> > > Naush
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20200218/73d16cc0/attachment.sig>