> -----Original Message----- > From: Hans de Goede <hdegoede@xxxxxxxxxx> > Sent: Thursday, March 16, 2023 5:04 PM > > Hi, > > On 3/16/23 03:58, Wu, Wentong wrote: > > > > > >> -----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 > > > > Below is your dmesg log, the required SPI controller for IVSC isn't here. > > > > [ 35.538114] ljca 2-6:1.0: acked sem wait timed out ret:0 timeout:20 ack:0 > > [ 35.538129] ljca 2-6:1.0: MNG_ENUM_SPI failed ret:-110 len:7 num:0 > > [ 35.538621] ljca 2-6:1.0: LJCA USB device init success > > [ 35.538776] usbcore: registered new interface driver ljca > > > > Also I checked your SSDT, there is no IVSC device and the sensor > > device depends on > > INT3472 instead of IVSC device as on my setup. > > Ack. > > >> 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. > > > > Can we add IVSC device to acpi_honor_dep_ids, so that when everything > > is done during mei_ace probe, acpi_dev_clear_dependencies can make sensor > start probe? > > Does the sensor ACPI device node have an ACPI _DEP on the IVSC device ? Yes, > > If yes, then yes we can add the IVSC device to acpi_honor_dep_id and make > mei_ace probe call acpi_dev_clear_dependencies(). But I prefer the powerdown gpio model, because we have to follow the commands sequences as below which is required by firmware, runtime pm is hard to achieve this. + /* switch camera sensor ownership to host */ + ret = ace_set_camera_owner(ACE_CAMERA_HOST); + if (ret) + goto error; + + /* switch CSI-2 link to host */ + ret = csi_set_link_owner(CSI_LINK_HOST, callback, context); + if (ret) + goto release_camera; + + /* configure CSI-2 link */ + ret = csi_set_link_cfg(nr_of_lanes, link_freq); + if (ret) + goto release_csi; BR, Wentong > > Regards, > > Hans > > > > >> 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 > >>>>>>> > >>>>> > >
