| Message ID | 20230705213010.390849-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand |
On Wed, Jul 05, 2023 at 11:29:56PM +0200, Hans de Goede wrote: > After commit f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu > bridge and move out of ipu3") the ipu-bridge code is always built in > even if all consumers are build as module. > > Fix this by turning "config IPU_BRIDGE" into a pure library Kconfig > option (not user selectable, must be selected by consumers) and > re-introducing the CIO2_BRIDGE Kconfig bits in .../pci/intel/ipu3/Kconfig > which were dropped to still allow building ipu3-cio2 without ipu-bridge > support. Reviewed-by: Andy Shevchenko <andy@kernel.org> > Fixes: f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/pci/intel/Kconfig | 18 ++---------------- > drivers/media/pci/intel/ipu-bridge.c | 4 ++++ > drivers/media/pci/intel/ipu3/Kconfig | 20 ++++++++++++++++++++ > 3 files changed, 26 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/pci/intel/Kconfig b/drivers/media/pci/intel/Kconfig > index 64a29b0b7033..3179184d7616 100644 > --- a/drivers/media/pci/intel/Kconfig > +++ b/drivers/media/pci/intel/Kconfig > @@ -1,21 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > config IPU_BRIDGE > - bool "Intel IPU Sensors Bridge" > - depends on VIDEO_IPU3_CIO2 && ACPI > + tristate > + depends on ACPI > depends on I2C > - help > - This extension provides an API for the Intel IPU driver to create > - connections to cameras that are hidden in the SSDB buffer in ACPI. > - It can be used to enable support for cameras in detachable / hybrid > - devices that ship with Windows. > - > - Say Y here if your device is a detachable / hybrid laptop that comes > - with Windows installed by the OEM, for example: > - > - - Microsoft Surface models (except Surface Pro 3) > - - The Lenovo Miix line (for example the 510, 520, 710 and 720) > - - Dell 7285 > - > - If in doubt, say N here. > > source "drivers/media/pci/intel/ipu3/Kconfig" > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c > index 1c88fd925a8b..97b544736af2 100644 > --- a/drivers/media/pci/intel/ipu-bridge.c > +++ b/drivers/media/pci/intel/ipu-bridge.c > @@ -497,3 +497,7 @@ int ipu_bridge_init(struct pci_dev *ipu) > return ret; > } > EXPORT_SYMBOL_NS_GPL(ipu_bridge_init, INTEL_IPU_BRIDGE); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>"); > +MODULE_DESCRIPTION("Intel IPU ACPI Sensors Bridge"); > diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig > index 9be06ee81ff0..0951545eab21 100644 > --- a/drivers/media/pci/intel/ipu3/Kconfig > +++ b/drivers/media/pci/intel/ipu3/Kconfig > @@ -8,6 +8,7 @@ config VIDEO_IPU3_CIO2 > select VIDEO_V4L2_SUBDEV_API > select V4L2_FWNODE > select VIDEOBUF2_DMA_SG > + select IPU_BRIDGE if CIO2_BRIDGE > > help > This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel > @@ -17,3 +18,22 @@ config VIDEO_IPU3_CIO2 > Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2 > connected camera. > The module will be called ipu3-cio2. > + > +config CIO2_BRIDGE > + bool "IPU3 CIO2 Sensors Bridge" > + depends on VIDEO_IPU3_CIO2 && ACPI > + depends on I2C > + help > + This extension provides an API for the ipu3-cio2 driver to create > + connections to cameras that are hidden in the SSDB buffer in ACPI. > + It can be used to enable support for cameras in detachable / hybrid > + devices that ship with Windows. > + > + Say Y here if your device is a detachable / hybrid laptop that comes > + with Windows installed by the OEM, for example: > + > + - Microsoft Surface models (except Surface Pro 3) > + - The Lenovo Miix line (for example the 510, 520, 710 and 720) > + - Dell 7285 > + > + If in doubt, say N here. > -- > 2.41.0 >
On Thu, Jul 06, 2023 at 02:29:35PM +0200, Hans de Goede wrote: > On 7/6/23 11:19, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: ... > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > Ah interesting, it does indeed look a lot like your version. > but it was developed independently. Very interesting! It's a second patch of me that collides with someone's else work. > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > changes in this series which rely on this are intended for 6.6-rc1 too. > > So we still need to figure out how to merge this. Standard way? I.e. ask Rafael for immutable tag/branch?
Hi Hans On 05/07/2023 22:29, Hans de Goede wrote: > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > sensor->adev is not set yet. > > So if either of the dev_warn() calls about unknown values are hit this > will lead to a NULL pointer deref. > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > on errors harder, to fix this. > > Fixes: 485aa3df0dff ("media: ipu3-cio2: Parse sensor orientation and rotation") > Cc: Fabian Wüthrich <me@fabwu.ch> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> And same for the corresponding 09/18 > drivers/media/pci/intel/ipu-bridge.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c > index 62daa8c1f6b1..f0927f80184d 100644 > --- a/drivers/media/pci/intel/ipu-bridge.c > +++ b/drivers/media/pci/intel/ipu-bridge.c > @@ -308,6 +308,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg, > } > > sensor = &bridge->sensors[bridge->n_sensors]; > + /* > + * Borrow our adev ref to the sensor for now, on success > + * acpi_dev_get(adev) is done further below. > + */ > + sensor->adev = adev; > > ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", > &sensor->ssdb,
On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 7/6/23 11:19, Andy Shevchenko wrote: > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: > >> Some ACPI glue code (1) may want to do an acpi_device_id match while > >> it only has a struct acpi_device available because the first physical > >> node may not have been instantiated yet. > >> > >> Add a new acpi_match_acpi_device() helper for this, which takes > >> a "struct acpi_device *" as argument rather then the "struct device *" > >> which acpi_match_device() takes. > >> > >> 1) E.g. code which parses ACPI tables to transforms them > >> into more standard kernel data structures like fwnodes > > > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > Ah interesting, it does indeed look a lot like your version. > but it was developed independently. > > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > changes in this series which rely on this are intended for 6.6-rc1 too. No, the material Andy is talking about will be pushed for 6.5-rc1 (probably even today), because it is part of a fix for systems that are broken in the field. > So we still need to figure out how to merge this. This shouldn't be a problem.
Hi, On 7/6/23 15:26, Rafael J. Wysocki wrote: > On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 7/6/23 11:19, Andy Shevchenko wrote: >>> On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: >>>> Some ACPI glue code (1) may want to do an acpi_device_id match while >>>> it only has a struct acpi_device available because the first physical >>>> node may not have been instantiated yet. >>>> >>>> Add a new acpi_match_acpi_device() helper for this, which takes >>>> a "struct acpi_device *" as argument rather then the "struct device *" >>>> which acpi_match_device() takes. >>>> >>>> 1) E.g. code which parses ACPI tables to transforms them >>>> into more standard kernel data structures like fwnodes >>> >>> Looks like it's v1 of my original patch, anyway this is now in Linux Next as >>> 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). >> >> Ah interesting, it does indeed look a lot like your version. >> but it was developed independently. >> >> Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp >> changes in this series which rely on this are intended for 6.6-rc1 too. > > No, the material Andy is talking about will be pushed for 6.5-rc1 > (probably even today), because it is part of a fix for systems that > are broken in the field. > >> So we still need to figure out how to merge this. > > This shouldn't be a problem. Great, thank you. Regards, Hans
On Thu, Jul 06, 2023 at 03:26:20PM +0200, Rafael J. Wysocki wrote: > On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On 7/6/23 11:19, Andy Shevchenko wrote: > > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote: ... > > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as > > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper"). > > > > Ah interesting, it does indeed look a lot like your version. > > but it was developed independently. > > > > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp > > changes in this series which rely on this are intended for 6.6-rc1 too. > > No, the material Andy is talking about will be pushed for 6.5-rc1 > (probably even today), because it is part of a fix for systems that > are broken in the field. Oh, totally forgot about that. > > So we still need to figure out how to merge this. > > This shouldn't be a problem. True, and thank you!
Hi All, Here is v3 of my patch-series to make the atomisp code share the ACPI -> sensor fwnode bridge code with the IPU3 (and IPU6 code). This series also rework VCM instantiation, which was my initial reason for unifying / sharing the code. Rafael, this v3 now includes a small ACPI patch: ACPI: bus: Introduce acpi_match_acpi_device() function It is probably easiest if you can give an ack for merging this through the media tree. May we have your ack for this? Sakari, I know that you have other pending patches depending on this, patches 1-14 can be merged independent of the new ACPI patch. Only the atomisp changes require that and those can also be merged later. Changes in v3: - New patches: media: i2c: Add driver for DW9719 VCM ACPI: bus: Introduce acpi_match_acpi_device() function media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation - media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init(): - Add a table with per sensor-HID atomisp_sensor_config settings for sensors which have lanes != 1 or which may have a VCM (VCM support is added in a follow-up patch) - Switch to acpi_handle_err() for logging errors - Set orientation based on CSI link/port This set now consists of the following parts: Patches 1-4 Bugfixes for recent changes Patches 5-12 Cleanup / preparation patches Patch 13 Rework how VCM client instantiation is done so that a device-link can be added from VCM to sensor to fix issues with the VCM power-state being tied to the sensor power state Patch 14 Resubmit DW9719 VCM driver upstream now that Vsio hack is gone Patch 15 New ACPI helper needed by atomisp bridge code Patch 16 Drop ipu-bridge code copy from atomisp, switching to the shared ipu-bridge module Patch 17-18 Further atomisp bridge code improvements Changes in v2: - Rebase on top of f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3") (rebase on top of sailus/media_tree.git/for-6.6-1.4-signed) - Share the ipu_supported_sensors[] array between atomisp and IPU3/IPU6 (leave it in ipu-bridge.c instead of giving each consumer its own copy) - 2 new bugfixes: media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings media: ipu-bridge: Allow building as module Original cover-letter: While working on adding (proper) VCM support to the atomisp code I found myself copying yet more code from drivers/media/pci/intel/ipu3/cio2-bridge.c into the atomisp code. So I decided that it really was time to factor out the common code (most of the code) from intel/ipu3/cio2-bridge.c into its own helper library and then share it between the atomisp and IPU3 code. This will hopefully also be useful for the ongoing work to upstream IPU6 input system support which also needs this functionality and currently contains a 3th copy of this code in the out of tree driver. Regards, Hans Daniel Scally (1): media: i2c: Add driver for DW9719 VCM Hans de Goede (17): media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings media: ipu-bridge: Do not use on stack memory for software_node.name field media: ipu-bridge: Move initialization of node_names.vcm to ipu_bridge_init_swnode_names() media: ipu-bridge: Allow building as module media: ipu-bridge: Make ipu_bridge_init() take a regular struct device as argument media: ipu-bridge: Store dev pointer in struct ipu_bridge media: ipu-bridge: Only keep PLD around while parsing media: ipu-bridge: Add a ipu_bridge_parse_ssdb() helper function media: ipu-bridge: Drop early setting of sensor->adev media: ipu-bridge: Add a parse_sensor_fwnode callback to ipu_bridge_init() media: ipu-bridge: Move ipu-bridge.h to include/media/ media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor ACPI: bus: Introduce acpi_match_acpi_device() function media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init() media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation MAINTAINERS | 7 + drivers/acpi/bus.c | 22 +- drivers/media/i2c/Kconfig | 11 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/dw9719.c | 427 ++++++++++++++++++ drivers/media/pci/intel/Kconfig | 18 +- drivers/media/pci/intel/ipu-bridge.c | 333 ++++++++------ drivers/media/pci/intel/ipu3/Kconfig | 20 + drivers/media/pci/intel/ipu3/ipu3-cio2.c | 10 +- drivers/staging/media/atomisp/Kconfig | 3 + .../staging/media/atomisp/pci/atomisp_csi2.h | 67 --- .../media/atomisp/pci/atomisp_csi2_bridge.c | 424 ++++++----------- .../staging/media/atomisp/pci/atomisp_v4l2.c | 1 + include/acpi/acpi_bus.h | 2 + .../pci/intel => include/media}/ipu-bridge.h | 27 +- 15 files changed, 851 insertions(+), 522 deletions(-) create mode 100644 drivers/media/i2c/dw9719.c rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (80%)