Message-ID: <2e27fcb6-e9d8-b9d1-3466-abac9e8bf66e@redhat.com>
Date:   Thu, 16 Mar 2023 09:37:53 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 0/3] media: pci: intel: ivsc: Add driver of Intel
 Visual Sensing Controller(IVSC)
Content-Language: en-US, nl
To:     "Wu, Wentong" <wentong.wu@intel.com>,
        Sakari Ailus <sakari.ailus@linux.intel.com>
Cc:     "mchehab@kernel.org" <mchehab@kernel.org>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>,
        "pierre-louis.bossart@linux.intel.com" 
        <pierre-louis.bossart@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "Ye, Xiang" <xiang.ye@intel.com>,
        "Qiu, Tian Shu" <tian.shu.qiu@intel.com>,
        "Cao, Bingbu" <bingbu.cao@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230213022347.2480307-1-wentong.wu@intel.com>
 <Y/8qJzScTfFucpP9@kekkonen.localdomain>
 <ae28faf8-c8a4-3f75-08d0-8e5233f2fa5d@redhat.com>
 <DM6PR11MB4316F4B72F98ADBF577412378DB79@DM6PR11MB4316.namprd11.prod.outlook.com>
 <ZAb2G7kqsEvrBhpG@kekkonen.localdomain>
 <DM6PR11MB4316B4F865472CA998E696FC8DB79@DM6PR11MB4316.namprd11.prod.outlook.com>
 <4c3ba301-6241-f2f4-f139-b4f4a0cd6223@redhat.com>
 <DM6PR11MB43166ADFA7D0775BA2C223C78DB59@DM6PR11MB4316.namprd11.prod.outlook.com>
 <e12fe65e-0b1f-a058-75e6-fa3e0a292c5b@redhat.com>
 <DM6PR11MB4316B02E112305F411B1A5158DB59@DM6PR11MB4316.namprd11.prod.outlook.com>
 <bb608934-23a6-213b-ab28-5dd66afd88e5@redhat.com>
 <DM6PR11MB43169899E53424334CA08E568DBC9@DM6PR11MB4316.namprd11.prod.outlook.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <DM6PR11MB43169899E53424334CA08E568DBC9@DM6PR11MB4316.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 3/16/23 09:11, Wu, Wentong wrote:
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> 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
>>
>> 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.
> 
> Thanks,
> 
> So the drivers of sensors connected to IVSC have to add power up/down code.

If we go the model it as a powerdown GPIO route, yes. Note most sensor drivers already have support for this since this is used in the INT3472 case already.

Regards,

Hans


> 
> BR,
> Wentong
>>
>> 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@redhat.com>
>>>> Sent: Thursday, March 9, 2023 9:30 PM
>>>> To: Wu, Wentong <wentong.wu@intel.com>
>>>> 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@redhat.com>
>>>>>> 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@redhat.com>
>>>>>>>> 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@linux.intel.com>
>>>>>>>>>> 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@redhat.com>
>>>>>>>>>>>> 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
>>>>>>>
>>>>>
>