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

[Wu, Wentong]

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

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.

BR,
Wentong

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