> -----Original Message----- > From: Hans de Goede <hdegoede@xxxxxxxxxx> > Sent: Thursday, March 9, 2023 11:24 PM > > <re-added the previous Cc list, which I dropped because of the large > attachments> > > Hi Wentong, > > On 3/9/23 15:29, Wu, Wentong wrote: > > Hi Hans, > > > > Thanks > > > > And AFAICT, there is no IVSC device on your Dell Latitude 9420 where the > platform is based on TGL instead of ADL, and I have never heard IVSC runs on > TGL, if no IVSC, INT3472 will control sensor's power. > > And I will double confirm with people who know dell product well tomorrow. > > Ah, I was under the impression that there was an IVSC there because: > > 1. The sensor driver for the used sensor (tries to) poke the IVSC 2. Things did not > work without building the IVSC drivers, but that might > be due to a dependency on the LCJA GPIO expander instead > > But you might very well be right, that would also explain the "intel vsc not ready" > messages in dmesg. > > If with the IVSC case the IVSC controls the power to the sensor too, then > another option might be to model the I2C-switch + the power-control as a > powerdown GPIO for the sensor, which most sensor drivers already try to use. > The advantage of doing this would be that GPIO lookups can reference the GPIO > provider + consumer by device-name so then we don't need to have both > devices instantiated at the time of > adding the GPIO lookup. And in that case we could e.g. add the lookup > before registering the I2C controller. Thanks, So the drivers of sensors connected to IVSC have to add power up/down code. BR, Wentong > > Sakari, what do you think of instead of using runtime-pm + devlinks having the > IVSC code export a powerdown GPIO to the sensor ? > > This also decouples the ivsc powerstate from the sensor power-state which > might be useful if we ever want to use some of the more advanced ivsc features, > where AFAICT the ivsc fully controls the sensor. > > Regards, > > Hans > > > > > >> -----Original Message----- > >> From: Hans de Goede <hdegoede@xxxxxxxxxx> > >> Sent: Thursday, March 9, 2023 9:30 PM > >> To: Wu, Wentong <wentong.wu@xxxxxxxxx> > >> Subject: Re: [PATCH v2 0/3] media: pci: intel: ivsc: Add driver of > >> Intel Visual Sensing Controller(IVSC) > >> > >> Hi Wentong, > >> > >> Attached are the requested dmesg + acpidump for the Dell Latitude 9420. > >> > >> Regards, > >> > >> Hans > >> > >> > >> > >> > >> On 3/9/23 14:21, Wu, Wentong wrote: > >>> Hi > >>> > >>>> -----Original Message----- > >>>> From: Hans de Goede <hdegoede@xxxxxxxxxx> > >>>> Sent: Thursday, March 9, 2023 5:28 PM > >>>> > >>>> Hi, > >>>> > >>>> On 3/9/23 02:08, Wu, Wentong wrote: > >>>>> > >>>>> > >>>>>> -----Original Message----- > >>>>>> From: Hans de Goede <hdegoede@xxxxxxxxxx> > >>>>>> Sent: Tuesday, March 7, 2023 5:10 PM > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> On 3/7/23 09:40, Wu, Wentong wrote: > >>>>>>> > >>>>>>> > >>>>>>>> -----Original Message----- > >>>>>>>> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > >>>>>>>> Sent: Tuesday, March 7, 2023 4:30 PM > >>>>>>>> > >>>>>>>> Hi Wentong, > >>>>>>>> > >>>>>>>> On Tue, Mar 07, 2023 at 08:17:04AM +0000, Wu, Wentong wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> -----Original Message----- > >>>>>>>>>> From: Hans de Goede <hdegoede@xxxxxxxxxx> > >>>>>>>>>> Sent: Wednesday, March 1, 2023 6:42 PM > >>>>>>>>>> > >>>>>>>>>> Hi, > >>>>>>>>>> > >>>>>>>>>> On 3/1/23 11:34, Sakari Ailus wrote: > >>>>>>>>>>> Hi Wentong, > >>>>>>>>>>> > >>>>>>>>>>> On Mon, Feb 13, 2023 at 10:23:44AM +0800, Wentong Wu wrote: > >>>>>>>>>>>> Intel Visual Sensing Controller (IVSC), codenamed "Clover > >>>>>>>>>>>> Falls", is a companion chip designed to provide secure and > >>>>>>>>>>>> low power vision capability to IA platforms. IVSC is > >>>>>>>>>>>> available in existing commercial platforms from multiple OEMs. > >>>>>>>>>>>> > >>>>>>>>>>>> The primary use case of IVSC is to bring in context awareness. > >>>>>>>>>>>> IVSC interfaces directly with the platform main camera > >>>>>>>>>>>> sensor via a CSI-2 link and processes the image data with > >>>>>>>>>>>> the embedded AI engine. The detected events are sent over > >>>>>>>>>>>> I2C to ISH (Intel Sensor Hub) for additional data fusion > >>>>>>>>>>>> from multiple > >> sensors. > >>>>>>>>>>>> The fusion results are used to implement advanced use cases like: > >>>>>>>>>>>> - Face detection to unlock screen > >>>>>>>>>>>> - Detect user presence to manage backlight setting or > >>>>>>>>>>>> waking up system > >>>>>>>>>>>> > >>>>>>>>>>>> Since the Image Processing Unit(IPU) used on the host > >>>>>>>>>>>> processor needs to configure the CSI-2 link in normal > >>>>>>>>>>>> camera usages, the > >>>>>>>>>>>> CSI-2 link and camera sensor can only be used in > >>>>>>>>>>>> mutually-exclusive ways by host IPU and IVSC. By default > >>>>>>>>>>>> the IVSC owns the CSI-2 link and camera sensor. The IPU > >>>>>>>>>>>> driver can take ownership of the CSI-2 link and camera > >>>>>>>>>>>> sensor using interfaces provided > >>>>>>>> by this IVSC driver. > >>>>>>>>>>>> > >>>>>>>>>>>> Switching ownership requires an interface with two > >>>>>>>>>>>> different hardware modules inside IVSC. The software > >>>>>>>>>>>> interface to these modules is via Intel MEI (The Intel > Management Engine) commands. > >>>>>>>>>>>> These two hardware modules have two different MEI UUIDs to > >>>>>>>>>>>> enumerate. These hardware > >>>>>>>>>> modules are: > >>>>>>>>>>>> - ACE (Algorithm Context Engine): This module is for > >>>>>>>>>>>> algorithm computing when IVSC owns camera sensor. Also ACE > >>>>>>>>>>>> module controls camera sensor's ownership. This hardware > >>>>>>>>>>>> module is used to set ownership > >>>>>>>>>> of camera sensor. > >>>>>>>>>>>> - CSI (Camera Serial Interface): This module is used to > >>>>>>>>>>>> route camera sensor data either to IVSC or to host for IPU > >>>>>>>>>>>> driver and > >>>>>>>> application. > >>>>>>>>>>>> > >>>>>>>>>>>> IVSC also provides a privacy mode. When privacy mode is > >>>>>>>>>>>> turned on, camera sensor can't be used. This means that > >>>>>>>>>>>> both ACE and host IPU can't get image data. And when this > >>>>>>>>>>>> mode is turned on, host IPU driver is informed via a > >>>>>>>>>>>> registered callback, so that user can be > >>>>>>>> notified. > >>>>>>>>>>>> > >>>>>>>>>>>> In summary, to acquire ownership of camera by IPU driver, > >>>>>>>>>>>> first ACE module needs to be informed of ownership and then > >>>>>>>>>>>> to setup MIPI CSI-2 link for the camera sensor and IPU. > >>>>>>>>>>> > >>>>>>>>>>> I thought this for a while and did some research, and I can > >>>>>>>>>>> suggest the > >>>>>>>>>>> following: > >>>>>>>>>>> > >>>>>>>>>>> - The IVSC sub-device implements a control for privacy > >>>>>> (V4L2_CID_PRIVACY > >>>>>>>>>>> is a good fit). > >>>>>>>>>>> > >>>>>>>>>>> - Camera sensor access needs to be requested from IVSC > >>>>>>>>>>> before accessing > >>>>>>>> the > >>>>>>>>>>> sensor via I²C. The IVSC ownership control needs to be in the > right > >>>>>>>>>>> setting for this to work, and device links can be used for > >>>>>>>>>>> that > >> purpose > >>>>>>>>>>> (see device_link_add()). With DL_FLAG_PM_RUNTIME and > >>>>>>>>>> DL_FLAG_RPM_ACTIVE, > >>>>>>>>>>> the supplier devices will be PM runtime resumed before the > >> consumer > >>>>>>>>>>> (camera sensor). As these devices are purely virtual on > >>>>>>>>>>> host side and > >>>> has > >>>>>>>>>>> no power state as such, you can use runtime PM callbacks > >>>>>>>>>>> to transfer > >>>>>> the > >>>>>>>>>>> ownership. > >>>>>>>>>> > >>>>>>>>>> Interesting proposal to use device-links + runtime-pm for > >>>>>>>>>> this instead of modelling this as an i2c-mux. FWIW I'm fine > >>>>>>>>>> with going this route instead of using an i2c-mux approach. > >>>>>>>>>> > >>>>>>>>>> I have been thinking about the i2c-mux approach a bit and the > >>>>>>>>>> problem is that we are not really muxing but want to turn > >>>>>>>>>> on/off control and AFAIK the i2c-mux framework simply leaves > >>>>>>>>>> the mux muxed to the last used i2c-chain, so control will > >>>>>>>>>> never be released when the i2c > >>>>>>>> transfers are done. > >>>>>>>>>> > >>>>>>>>>> And if were to somehow modify things (or maybe there already > >>>>>>>>>> is some release > >>>>>>>>>> callback) then the downside becomes that the i2c-mux core > >>>>>>>>>> code operates at the i2c transfer level. So each i2c > >>>>>>>>>> read/write would then enable + > >>>>>>>> disavle control. > >>>>>>>>>> > >>>>>>>>>> Modelling this using something like runtime pm as such is a > >>>>>>>>>> much better fit because then we request control once on probe > >>>>>>>>>> / stream-on and release it once we are fully done, rather > >>>>>>>>>> then requesting + releasing control once per i2c- transfer. > >>>>>>>>> > >>>>>>>>> Seems runtime pm can't fix the problem of initial i2c transfer > >>>>>>>>> during sensor driver probe, probably we have to switch to > >>>>>>>>> i2c-mux modeling > >>>>>> way. > >>>>>>>> > >>>>>>>> What do you mean? The supplier devices are resumed before the > >>>>>>>> driver's probe is called. > >>>>>>> > >>>>>>> But we setup the link with device_link_add during IVSC driver's > >>>>>>> probe, we can't guarantee driver probe's sequence. > >>>>>> > >>>>>> Then maybe we need to do the device_link_add somewhere else. > >>>>> > >>>>> sensor's parent is the LJCA I2C device whose driver is being > >>>>> upstream https://www.spinics.net/lists/kernel/msg4702552.htmland > >>>>> and sensor's power is controlled by IVSC instead of INT3472 if IVSC > enabled. > >>>> > >>>> I believe that the INT3472 code is still involved at least on a > >>>> Dell Latitude 9420 the INT3472 code still needs to set the > >>>> clock-enable and the privacy-LED GPIOs otherwise the main camera won't > work. > >>>> > >>>> So I'm not sure what you mean with "sensor's power is controlled by > >>>> IVSC instead of INT3472" ? > >>> > >>> Could you please share your kernel log and DSDT? Thanks > >>> > >>> BR, > >>> Wentong > >>>> > >>>> > >>>>> struct device_link *device_link_add(struct device *consumer, > >>>>> struct device *supplier, u32 > >>>>> flags) > >>>>> > >>>>> So probably we have to add above device_link_add in LJCA I2C's > >>>>> driver, and we can find the consumer(camera sensor) with ACPI API, > >>>>> but the supplier, mei_ace, is mei client device under mei > >>>>> framework and it's dynamically allocated device instead of ACPI > >>>>> device, probably I can find its parent with some ACPI lookup from > >>>>> this LJCA I2C device, but unfortunately mei framework doesn't > >>>>> export the API to find mei client device with its parent bus device(struct > mei_device). > >>>>> > >>>>> I'm not sure if modeling this mei_ace as LJCA I2C's runtime power > >>>>> control is acceptable, if yes, probably this mei_ace driver have > >>>>> to go with LJCA I2C device driver. > >>>> > >>>> Looking at the ACPI table the sensor ACPI device has 2 _DEP-s > >>>> listed the I2C controller and the INT3472 device. Since we are > >>>> already doing similar setup in the INT3472 device that seems like a > >>>> good place to add the device_link()-s (it can return -EPROBE_DEFER > >>>> to wait for the mei_ace > >> to show up). > >>>> > >>>> But when the INT3472 code runs, the consumer device does not exist > >>>> yet and AFAICT the same is true when the LCJA i2c-controller driver > >>>> is getting > >> registered. > >>>> The consumer only exists when the i2c_client is instantiated and at > >>>> that point the sensor drivers probe() method can run immediately > >>>> and we are too late to add the device_link. > >>>> > >>>> As a hobby project I have been working on atomisp2 support and I > >>>> have a similar issue there. There is no INT3472 device there, but > >>>> there is a _DSM method which needs to be used to figure out which > >>>> ACPI GPIO resource is reset / powerdown and if the GPIOs are active-low > or active high. > >>>> > >>>> I have written a little helper function to call the _DSM and to > >>>> then turn this into lookups and call devm_acpi_dev_add_driver_gpios(). > >>>> > >>>> Since on atomisp2 we cannot use the INT3472 driver to delay the > >>>> sensor-driver probe and have the INT3472 driver setup the GPIO > >>>> lookup, at least for the sensor drivers used with > >>>> atomisp2 there is going to be a need to add a single line to probe() like this: > >>>> > >>>> v4l2_get_acpi_sensor_info(&i2c_client->dev, NULL); > >>>> > >>>> To me it sounds like we need to do something similar here and > >>>> extend the helper function which I have written (but not yet submitted > upstream) : > >>>> > >>>> https://github.com/jwrdegoede/linux- > >>>> sunxi/commit/e2287979db43d46fa7d354c1bde92eb6219b613d > >>>> > >>>> To also setup the device-links needed for the runtime-pm solution > >>>> to getting the i2c passed through to the sensor. > >>>> > >>>> Ideally v4l2_get_acpi_sensor_info() should return void (easier to > >>>> use in the sensor drivers) but I think it should return an int, so > >>>> that it can e.g. return - EPROBE_DEFER to wait for the mei_ace. > >>>> > >>>> Regards, > >>>> > >>>> Hans > >>>> > >>>> > >>>> > >>>> > >>>>>> The mainline kernel delays probing of camera sensors on Intel > >>>>>> platforms until the INT3472 driver has probed the INT3472 device > >>>>>> on which the sensors have an ACPI _DEP. > >>>>>> > >>>>>> This is already used to make sure that clock lookups and > >>>>>> regulator info is in place before the sensor's probe() function runs. > >>>>>> > >>>>>> So that when the driver does clk_get() it succeeds and so that > >>>>>> regulator_get() does not end up returning a dummy regulator. > >>>>>> > >>>>>> So I think the code adding the device_link-s for the IVSC should > >>>>>> be added > >>>>>> to: drivers/platform/x86/intel/int3472/discrete.c and then the > >>>>>> runtime-resume will happen before the sensor's probe() function runs. > >>>>>> > >>>>>> Likewise drivers/platform/x86/intel/int3472/discrete.c should > >>>>>> also ensure that the ivsc driver's probe() has run before it > >>>>>> calls > >>>> acpi_dev_clear_dependencies(). > >>>>>> > >>>>>> The acpi_dev_clear_dependencies() call in discrete.c tells the > >>>>>> ACPI subsystem to go ahead and create the i2c-clients for the > >>>>>> sensors and allow the sensor drivers to get loaded and probe the sensor. > >>>>>> > >>>>>> Regards, > >>>>>> > >>>>>> Hans > >>>>> > >>>
