| Message ID | 20230119104544.9456-14-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck via libcamera-devel (2023-01-19 10:45:43) > From: Nick Hollinghurst <nick.hollinghurst@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. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@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 = { > -- > 2.25.1 >
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@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. > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@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 = {
Hi Laurent, On Sun, 22 Jan 2023 at 18:58, Laurent Pinchart < laurent.pinchart@ideasonboard.com> 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@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? Regards, Naush > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@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 >
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@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 :-) 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. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@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 = {
Hi Laurent, On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart < laurent.pinchart@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@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! :) > > 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@ideasonboard.com> > > > > > > > > > Signed-off-by: Nick Hollinghurst < > nick.hollinghurst@raspberrypi.com> > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > Reviewed-by: David Plowman <david.plowman@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 >
Hi Laurent and Naush On Mon, 23 Jan 2023 at 10:33, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Laurent, > > > On Mon, 23 Jan 2023 at 10:01, Laurent Pinchart <laurent.pinchart@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@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@ideasonboard.com> >> > > > >> > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> >> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> >> > > > > Reviewed-by: David Plowman <david.plowman@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
On Mon, Jan 23, 2023 at 10:33:35AM +0000, Naushir Patuck 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@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! :) > > > 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. You could also include variant support in the driver posted to linux-media, so the proposal could be discussed during reviews. Up to you. > 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 :( What concerns me is when that would be cleaned up. Unless there's a real effort to design and upstream a mechanism for this, it won't happen, so I want to see someone taking the lead and bringing it to completion. Merging support for a device in libcamera before the kernel driver is ready upstream is already a middleground. > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > Reviewed-by: David Plowman <david.plowman@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 = {
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@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@ideasonboard.com> > >> > > > > >> > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > >> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > >> > > > > Reviewed-by: David Plowman <david.plowman@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 = {
Hi Laurent, On Mon, 23 Jan 2023 at 12:12, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > On Mon, Jan 23, 2023 at 10:33:35AM +0000, Naushir Patuck 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@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! :) > > > > > 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. > > You could also include variant support in the driver posted to > linux-media, so the proposal could be discussed during reviews. Up to > you. > I think we will remove this to start with. I fear it will only muddy the waters for the bigger discussion. We can always refer back to the downstream driver to show examples of why it is required. > > > 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 :( > > What concerns me is when that would be cleaned up. Unless there's a real > effort to design and upstream a mechanism for this, it won't happen, so > I want to see someone taking the lead and bringing it to completion. > Merging support for a device in libcamera before the kernel driver is > ready upstream is already a middleground. > Got it, I'll remove the duplicate entries from the patch. Regards, Naush > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > > > > Signed-off-by: Nick Hollinghurst < > nick.hollinghurst@raspberrypi.com> > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > Reviewed-by: David Plowman <david.plowman@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 >
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@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@ideasonboard.com> > > >> > > > > > >> > > > > Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com> > > >> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > >> > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > >> > > > > Reviewed-by: David Plowman <david.plowman@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
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 = {