[libcamera-devel] [PATCH v1 13/14] libcamera: camera_sensor: Add Sony IMX708 sensor properties
Sakari Ailus
sakari.ailus at iki.fi
Mon Jan 23 15:09:02 CET 2023
Hi Laurent, Dave,
On Mon, Jan 23, 2023 at 02:21:03PM +0200, Laurent Pinchart wrote:
> 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) ?
Not at all I'm afraid.
With more generic user space, I expect there to be more and more needs to
convey information from the system firmware to the user space. I would use
controls, especially for passing individual parameters that we're mostly
dealing with.
The difficulty comes from the fact that these parameters are not often
known so the user space can't really rely on them being there. Still the
only way to address this is to start passing this information to the user
space.
In DT (nor ACPI) we haven't had even a concept of the module as it's been
viewed as a "box" which you just throw things into. Perhaps it's time to
start changing that. I'd begin with DT.
The camera sensor is central to the model as you always have one, so we
could continue to rely on that I think. There are cases where a single
module contains multiple sensors, perhaps we could have the module as a
parent node in that case.
This also overlaps with power on / off sequences that are module specific,
but there are very large numbers of different modules...
>
> > 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
--
Kind regards,
Sakari Ailus
More information about the libcamera-devel
mailing list