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 > >
