RE: [PATCH v2 0/3] media: pci: intel: ivsc: Add driver of Intel Visual Sensing Controller(IVSC)

[Wu, Wentong]

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