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

[Hans de Goede]

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.

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