| Message ID | 20250403-uvc-orientation-v1-3-1a0cc595a62d@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Add support for V4L2_CID_CAMERA_SENSOR_ORIENTATION | expand |
Hi Ricardo, Thanks for the patch. On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote: > This patch modifies v4l2_fwnode_device_parse() to support ACPI devices. > > We initially add support only for orientation via the ACPI _PLD method. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++---- > 1 file changed, 52 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644 > --- a/drivers/media/v4l2-core/v4l2-fwnode.c > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > @@ -15,6 +15,7 @@ > * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > */ > #include <linux/acpi.h> > +#include <acpi/acpi_bus.h> > #include <linux/kernel.h> > #include <linux/mm.h> > #include <linux/module.h> > @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode, > } > EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link); > > -int v4l2_fwnode_device_parse(struct device *dev, > - struct v4l2_fwnode_device_properties *props) > +static int v4l2_fwnode_device_parse_acpi(struct device *dev, > + struct v4l2_fwnode_device_properties *props) > +{ > + struct acpi_pld_info *pld; > + int ret = 0; > + > + if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) { > + dev_dbg(dev, "acpi _PLD call failed\n"); > + return 0; > + } You could have software nodes in an ACPI system as well as DT-aligned properties. They're not the primary means to convey this information still. How about returning e.g. -ENODATA here if _PLD doesn't exist for the device and then proceeding to parse properties as in DT? > + > + switch (pld->panel) { > + case ACPI_PLD_PANEL_FRONT: > + props->orientation = V4L2_FWNODE_ORIENTATION_FRONT; > + break; > + case ACPI_PLD_PANEL_BACK: > + props->orientation = V4L2_FWNODE_ORIENTATION_BACK; > + break; > + case ACPI_PLD_PANEL_TOP: > + case ACPI_PLD_PANEL_LEFT: > + case ACPI_PLD_PANEL_RIGHT: > + case ACPI_PLD_PANEL_UNKNOWN: > + props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL; > + break; How about the rotation in _PLD? > + default: > + dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel); > + ret = -EINVAL; > + break; > + } > + > + ACPI_FREE(pld); > + return ret; > +} > + > +static int v4l2_fwnode_device_parse_dt(struct device *dev, > + struct v4l2_fwnode_device_properties *props) > { > struct fwnode_handle *fwnode = dev_fwnode(dev); > u32 val; > int ret; > > - memset(props, 0, sizeof(*props)); > - > - props->orientation = V4L2_FWNODE_PROPERTY_UNSET; > ret = fwnode_property_read_u32(fwnode, "orientation", &val); > if (!ret) { > switch (val) { > @@ -833,7 +865,6 @@ int v4l2_fwnode_device_parse(struct device *dev, > dev_dbg(dev, "device orientation: %u\n", val); > } > > - props->rotation = V4L2_FWNODE_PROPERTY_UNSET; > ret = fwnode_property_read_u32(fwnode, "rotation", &val); > if (!ret) { > if (val >= 360) { > @@ -847,6 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev, > > return 0; > } > + > +int v4l2_fwnode_device_parse(struct device *dev, > + struct v4l2_fwnode_device_properties *props) > +{ > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + > + memset(props, 0, sizeof(*props)); > + > + props->orientation = V4L2_FWNODE_PROPERTY_UNSET; > + props->rotation = V4L2_FWNODE_PROPERTY_UNSET; > + > + if (is_acpi_device_node(fwnode)) > + return v4l2_fwnode_device_parse_acpi(dev, props); > + return v4l2_fwnode_device_parse_dt(dev, props); > +} > EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse); > > /* >
Hi Ricardo, On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote: > Hi Sakari > > On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote: >> >> Hi Ricardo, >> >> Thanks for the patch. >> >> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote: >>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices. >>> >>> We initially add support only for orientation via the ACPI _PLD method. >>> >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >>> --- >>> drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++---- >>> 1 file changed, 52 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c >>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644 >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c >>> @@ -15,6 +15,7 @@ >>> * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> >>> */ >>> #include <linux/acpi.h> >>> +#include <acpi/acpi_bus.h> >>> #include <linux/kernel.h> >>> #include <linux/mm.h> >>> #include <linux/module.h> >>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode, >>> } >>> EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link); >>> >>> -int v4l2_fwnode_device_parse(struct device *dev, >>> - struct v4l2_fwnode_device_properties *props) >>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev, >>> + struct v4l2_fwnode_device_properties *props) >>> +{ >>> + struct acpi_pld_info *pld; >>> + int ret = 0; >>> + >>> + if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) { >>> + dev_dbg(dev, "acpi _PLD call failed\n"); >>> + return 0; >>> + } >> >> You could have software nodes in an ACPI system as well as DT-aligned >> properties. They're not the primary means to convey this information still. >> >> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device >> and then proceeding to parse properties as in DT? > > Do you mean that there can be devices with ACPI handles that can also > have DT properties? Yes it is possible to embed DT properties in ACPI, but I don't think that is really applicable here. But we also have secondary software-fwnodes which are used extensively on x86 to set device-properties on devices by platform code to deal with ACPI tables sometimes having incomplete information. For example atm _PLD is already being parsed in: drivers/media/pci/intel/ipu-bridge.c and that is then used to add a standard "orientation" device-property on the sensor device. This is actually something which I guess we can drop once your patches are in, since those should then do the same in a more generic manner. > What shall we do if _PLD contradicts the DT property? What takes precedence? As for priorities, at east for rotation it seems that we are going to need some quirks, I already have a few Dell laptops where it seems that the sensor is upside down and parsing the rotation field in the IPU6 specific SSDB ACPI package does not yield a 180° rotation, so we are going to need some quirks. I expect these quirks to live in the bridge code, while your helper will be called from sensor drivers, so in order to allow quirks to override things, I think that first the "orientation" device-property should be checked (which the ACPI glue code we have can set before the sensor driver binds) and only then should _PLD be checked. IOW _PLD should be seen as the fallback, because ACPI tables are often a copy and paste job so it can very well contain wrong info copy-pasted from some example ACPI code or from another hw model. Regards, Hans
Hi Sakari On Tue, 22 Apr 2025 at 11:41, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > Hi Hans, Ricardo, > > On Tue, Apr 22, 2025 at 10:44:41AM +0200, Hans de Goede wrote: > > Hi Ricardo, > > > > On 22-Apr-25 2:23 AM, Ricardo Ribalda wrote: > > > Hi Sakari > > > > > > On Sun, 13 Apr 2025 at 17:50, Sakari Ailus <sakari.ailus@iki.fi> wrote: > > >> > > >> Hi Ricardo, > > >> > > >> Thanks for the patch. > > >> > > >> On Thu, Apr 03, 2025 at 07:16:14PM +0000, Ricardo Ribalda wrote: > > >>> This patch modifies v4l2_fwnode_device_parse() to support ACPI devices. > > >>> > > >>> We initially add support only for orientation via the ACPI _PLD method. > > >>> > > >>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > >>> --- > > >>> drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++---- > > >>> 1 file changed, 52 insertions(+), 6 deletions(-) > > >>> > > >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c > > >>> index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644 > > >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c > > >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c > > >>> @@ -15,6 +15,7 @@ > > >>> * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> > > >>> */ > > >>> #include <linux/acpi.h> > > >>> +#include <acpi/acpi_bus.h> > > >>> #include <linux/kernel.h> > > >>> #include <linux/mm.h> > > >>> #include <linux/module.h> > > >>> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode, > > >>> } > > >>> EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link); > > >>> > > >>> -int v4l2_fwnode_device_parse(struct device *dev, > > >>> - struct v4l2_fwnode_device_properties *props) > > >>> +static int v4l2_fwnode_device_parse_acpi(struct device *dev, > > >>> + struct v4l2_fwnode_device_properties *props) > > >>> +{ > > >>> + struct acpi_pld_info *pld; > > >>> + int ret = 0; > > >>> + > > >>> + if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) { > > >>> + dev_dbg(dev, "acpi _PLD call failed\n"); > > >>> + return 0; > > >>> + } > > >> > > >> You could have software nodes in an ACPI system as well as DT-aligned > > >> properties. They're not the primary means to convey this information still. > > >> > > >> How about returning e.g. -ENODATA here if _PLD doesn't exist for the device > > >> and then proceeding to parse properties as in DT? > > > > > > Do you mean that there can be devices with ACPI handles that can also > > > have DT properties? > > > > Yes it is possible to embed DT properties in ACPI, but I don't > > think that is really applicable here. > > This is determined by > Documentation/firmware-guide/acpi/DSD-properties-rules.rst . So rotation > and orientation shouldn't come from _DSD properties on ACPI systems. Doesn't this contradict what DisCo does? if (!fwnode_property_present(adev_fwnode, "rotation")) { struct acpi_pld_info *pld; if (acpi_get_physical_device_location(handle, &pld)) { swnodes->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] = PROPERTY_ENTRY_U32("rotation", pld->rotation * 45U); kfree(pld); } } It seems to first check for the rotation property, and then check _DSD. If I send a v2, shall I also replace DisCo even if that means reverting its logic? > > > > > But we also have secondary software-fwnodes which are used > > extensively on x86 to set device-properties on devices by > > platform code to deal with ACPI tables sometimes having > > incomplete information. > > > > For example atm _PLD is already being parsed in: > > > > drivers/media/pci/intel/ipu-bridge.c and that is then used to add > > a standard "orientation" device-property on the sensor device. > > > > This is actually something which I guess we can drop once your > > patches are in, since those should then do the same in a more > > generic manner. > > DisCo for Imaging support currently also digs this information from _PDL > (see init_crs_csi2_swnodes() in drivers/acpi/mipi-disco-img.c), but this > is only done if a _CRS CSI-2 descriptor is present. It could also be done > for devices with the IPU Windows specific ACPI objects and it would be a > natural location for handing quirks -- there are some > unrelated Dell DSDT quirks already. > > > > > > What shall we do if _PLD contradicts the DT property? What takes precedence? > > > > As for priorities, at east for rotation it seems that we are going > > to need some quirks, I already have a few Dell laptops where it seems > > that the sensor is upside down and parsing the rotation field in > > the IPU6 specific SSDB ACPI package does not yield a 180° rotation, > > so we are going to need some quirks. > > > > I expect these quirks to live in the bridge code, while your helper > > will be called from sensor drivers, so in order to allow quirks to > > override things, I think that first the "orientation" device-property > > should be checked (which the ACPI glue code we have can set before > > the sensor driver binds) and only then should _PLD be checked. > > > > IOW _PLD should be seen as the fallback, because ACPI tables are > > often a copy and paste job so it can very well contain wrong info > > copy-pasted from some example ACPI code or from another hw model. > > Unfortunately that does happen. :-( > > -- > Regards, > > Sakari Ailus
diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c index cb153ce42c45d69600a3ec4e59a5584d7e791a2a..81563c36b6436bb61e1c96f2a5ede3fa9d64dab3 100644 --- a/drivers/media/v4l2-core/v4l2-fwnode.c +++ b/drivers/media/v4l2-core/v4l2-fwnode.c @@ -15,6 +15,7 @@ * Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de> */ #include <linux/acpi.h> +#include <acpi/acpi_bus.h> #include <linux/kernel.h> #include <linux/mm.h> #include <linux/module.h> @@ -807,16 +808,47 @@ int v4l2_fwnode_connector_add_link(struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(v4l2_fwnode_connector_add_link); -int v4l2_fwnode_device_parse(struct device *dev, - struct v4l2_fwnode_device_properties *props) +static int v4l2_fwnode_device_parse_acpi(struct device *dev, + struct v4l2_fwnode_device_properties *props) +{ + struct acpi_pld_info *pld; + int ret = 0; + + if (!acpi_get_physical_device_location(ACPI_HANDLE(dev), &pld)) { + dev_dbg(dev, "acpi _PLD call failed\n"); + return 0; + } + + switch (pld->panel) { + case ACPI_PLD_PANEL_FRONT: + props->orientation = V4L2_FWNODE_ORIENTATION_FRONT; + break; + case ACPI_PLD_PANEL_BACK: + props->orientation = V4L2_FWNODE_ORIENTATION_BACK; + break; + case ACPI_PLD_PANEL_TOP: + case ACPI_PLD_PANEL_LEFT: + case ACPI_PLD_PANEL_RIGHT: + case ACPI_PLD_PANEL_UNKNOWN: + props->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL; + break; + default: + dev_dbg(dev, "Unknown _PLD panel val %d\n", pld->panel); + ret = -EINVAL; + break; + } + + ACPI_FREE(pld); + return ret; +} + +static int v4l2_fwnode_device_parse_dt(struct device *dev, + struct v4l2_fwnode_device_properties *props) { struct fwnode_handle *fwnode = dev_fwnode(dev); u32 val; int ret; - memset(props, 0, sizeof(*props)); - - props->orientation = V4L2_FWNODE_PROPERTY_UNSET; ret = fwnode_property_read_u32(fwnode, "orientation", &val); if (!ret) { switch (val) { @@ -833,7 +865,6 @@ int v4l2_fwnode_device_parse(struct device *dev, dev_dbg(dev, "device orientation: %u\n", val); } - props->rotation = V4L2_FWNODE_PROPERTY_UNSET; ret = fwnode_property_read_u32(fwnode, "rotation", &val); if (!ret) { if (val >= 360) { @@ -847,6 +878,21 @@ int v4l2_fwnode_device_parse(struct device *dev, return 0; } + +int v4l2_fwnode_device_parse(struct device *dev, + struct v4l2_fwnode_device_properties *props) +{ + struct fwnode_handle *fwnode = dev_fwnode(dev); + + memset(props, 0, sizeof(*props)); + + props->orientation = V4L2_FWNODE_PROPERTY_UNSET; + props->rotation = V4L2_FWNODE_PROPERTY_UNSET; + + if (is_acpi_device_node(fwnode)) + return v4l2_fwnode_device_parse_acpi(dev, props); + return v4l2_fwnode_device_parse_dt(dev, props); +} EXPORT_SYMBOL_GPL(v4l2_fwnode_device_parse); /*
This patch modifies v4l2_fwnode_device_parse() to support ACPI devices. We initially add support only for orientation via the ACPI _PLD method. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/v4l2-core/v4l2-fwnode.c | 58 +++++++++++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 6 deletions(-)