[libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor: Add Sony IMX708 sensor properties

[Dave Stevenson]

Hi Laurent and Naush

On Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> Hi Laurent,
>
>
> On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:
>>
>> Hi Naush,
>>
>> On Mon, Jan 23, 2023 at 09:07:53AM +0000, Naushir Patuck wrote:
>> > On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart wrote:
>> > > On Thu, Jan 19, 2023 at 01:03:04PM +0000, Kieran Bingham via libcamera-devel wrote:
>> > > > Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43)
>> > > > > From: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>
>> > > > >
>> > > > > The IMX708 sensor driver advertises its module variants (narrow/wide angle lens,
>> > > > > IR block/pass) by modifying the media entity name string. So add duplicate
>> > > > > entries for each variant.
>> > > >
>> > > > The duplication here is a bit of a pain. Are there any sensor properties
>> > > > we might register that would change in each of these variants?
>> > > >
>> > > > Or should we make the CameraSensorProperties class more intelligent so
>> > > > it just does the lookup on the sensor only. Of course ideally we should
>> > > > be able to identify the sensor and the variant separately from the
>> > > > kernel - which might not be possible right now - in which case, given
>> > > > how little information is actually duplicated here I'd be fine with this
>> > > > for now.
>> > >
>> > > I'm concerned by this too. The kernel shouldn't report different entity
>> > > names for different lenses. There should thus be a single entry in the
>> > > camera sensors properties table, and lens information should be reported
>> > > separately.
>> >
>> > I agree this is not the tidiest thing to do.  Right now, we have no other way of
>> > getting the variant details from the kernel driver into userland.  I believe
>> > @Dave Stevenson raised this topic at the last Embedded Linux Conference, but no
>> > consensus was reached.  Once we have a mechanism in the kernel to report lens
>> > properties, we will remove this workaround.  Would a \todo suffice for now?
>>
>> If adding a \todo was enough to make a mechanism for this appear
>> upstream by magic, I'd say it's fine. Unfortunately, that doesn't match
>> my experience :-)
>
>
> If only! :)

I'll acknowledge that time was getting short at the media summit.

The initial proposal was made as I knew these modules were coming. I'd
suggested the addition of an optics descriptor in the media entitiy,
not unlike the connector on the end of bridges in DRM. That would
provide the basic properties of the optics, with the option of
calibration too.
The general response seemed to be along the lines "oh no it's far too
complicated to model, and needs to be done as a module description".
Seeing as a module description almost flies in the face of the current
kernel frameworks describing the generic sensor and VCM drivers, I
have no concept of the framework being anticipated and how it fits
into the current world. Add in the complication of ACPI as well as DT,
and all bets are off.

Yourself (Laurent) and Sakari are the two main gatekeepers of what is
merged to mainline, so it's down to what satisfies you. As you both
shot down the initial proposal with no greater context, it leaves 3rd
parties totally at sea as to the use cases that were considered as not
covered, or any indication of what would be acceptable. That's not a
great motivator to invest time developing a proposal.

There needs to be an initial discussion including both of you to
formulate a basic direction on the subject, otherwise it's wasted
effort. Until then you'll get vendors doing their own thing, and their
use of libcamera will inherently fragment.

  Dave

>>
>>
>> One middleground option would be to merge the first "variant" (the
>> "imx708") when the driver gets posted to the linux-media mailing list,
>> while the rest gets developed, but in any case it will require someone
>> making a proposal upstream.
>
>
> We will post a variant-less driver to the mailing list and chat with Dave about
> a proposal for module variant reporting.
>
> The reason we added the other variants in the CameraSensorProperties was to
> quiet the warning log message when a device is not listed. Would it be ok to
> keep all variants listed like this with a \todo to clean-up?  If not, we will have to
> make this change in our downstream tree which we are trying hard to deprecate :(
>
> Naush
>
>>
>>
>> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> > > >
>> > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst at raspberrypi.com>
>> > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
>> > > > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>> > > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>> > > > > ---
>> > > > >  src/libcamera/camera_sensor_properties.cpp | 16 ++++++++++++++++
>> > > > >  1 file changed, 16 insertions(+)
>> > > > >
>> > > > > diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp
>> > > > > index c3c2caced906..3afd500ea3be 100644
>> > > > > --- a/src/libcamera/camera_sensor_properties.cpp
>> > > > > +++ b/src/libcamera/camera_sensor_properties.cpp
>> > > > > @@ -123,6 +123,22 @@ const CameraSensorProperties *CameraSensorProperties::get(const std::string &sen
>> > > > >                                  */
>> > > > >                         },
>> > > > >                 } },
>> > > > > +               { "imx708", {
>> > > > > +                       .unitCellSize = { 1400, 1400 },
>> > > > > +                       .testPatternModes = {},
>> > > > > +               } },
>> > > > > +               { "imx708_noir", {
>> > > > > +                       .unitCellSize = { 1400, 1400 },
>> > > > > +                       .testPatternModes = {},
>> > > > > +               } },
>> > > > > +               { "imx708_wide", {
>> > > > > +                       .unitCellSize = { 1400, 1400 },
>> > > > > +                       .testPatternModes = {},
>> > > > > +               } },
>> > > > > +               { "imx708_wide_noir", {
>> > > > > +                       .unitCellSize = { 1400, 1400 },
>> > > > > +                       .testPatternModes = {},
>> > > > > +               } },
>> > > > >                 { "ov2740", {
>> > > > >                         .unitCellSize = { 1400, 1400 },
>> > > > >                         .testPatternModes = {
>>
>> --
>> Regards,
>>
>> Laurent Pinchart