[libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor: Add Sony IMX708 sensor properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 23 13:21:03 CET 2023
Hi Dave,
(CC'ing Sakari)
On Mon, Jan 23, 2023 at 11:47:25AM +0000, Dave Stevenson wrote:
> On Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel wrote:
> > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart wrote:
> >> 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.
Sakari, do you have any insight on how this is handled in ACPI (if at
all) ?
> 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.
Then let's have that discussion.
I'm afraid that support for the whole camera ecosystem can't rest on the
shoulders of two individuals only, with a whole industry working in a
fire-and-forget mode, and Sakari and I left to fix everything. It can't
scale, and it just wouldn't be fair.
> >> 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 :(
> >
> >> > > > 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
More information about the libcamera-devel
mailing list