Message-ID: <ae28faf8-c8a4-3f75-08d0-8e5233f2fa5d@redhat.com>
Date:   Wed, 1 Mar 2023 11:41:43 +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:     Sakari Ailus <sakari.ailus@linux.intel.com>,
        Wentong Wu <wentong.wu@intel.com>
Cc:     mchehab@kernel.org, linux-media@vger.kernel.org,
        srinivas.pandruvada@intel.com,
        pierre-louis.bossart@linux.intel.com, zhifeng.wang@intel.com,
        xiang.ye@intel.com, tian.shu.qiu@intel.com, bingbu.cao@intel.com,
        linux-kernel@vger.kernel.org
References: <20230213022347.2480307-1-wentong.wu@intel.com>
 <Y/8qJzScTfFucpP9@kekkonen.localdomain>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <Y/8qJzScTfFucpP9@kekkonen.localdomain>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

Regards,

Hans


> 
> - The CSI-2 configuration should take place when streaming starts on the
>   sensor (as well as IVSC).
> 
> - Device links need to be set up via IPU bridge which now is called  CIO2
>   bridge (cio2-bridge.c).
> 
> Any questions, comments?
>