Message-ID: <0457831e-a347-8278-01fe-52c011759d90@redhat.com>
Date:   Wed, 15 Feb 2023 10:03:29 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Wentong Wu <wentong.wu@...el.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     mchehab@...nel.org, linux-media@...r.kernel.org,
        srinivas.pandruvada@...el.com,
        pierre-louis.bossart@...ux.intel.com, zhifeng.wang@...el.com,
        xiang.ye@...el.com, tian.shu.qiu@...el.com, bingbu.cao@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] media: pci: intel: ivsc: Add acquire/release API
 for ivsc

Hi Sakari,

On 2/14/23 17:06, Sakari Ailus wrote:
> Hi Wentong,
> 
> Thanks for the patchset.
> 
> On Mon, Feb 13, 2023 at 10:23:47AM +0800, Wentong Wu wrote:
>> IVSC directly connects to camera sensor on source side, and on
>> output side it not only connects ISH via I2C, but also exposes
>> MIPI CSI-2 interface to output camera sensor data. IVSC can use
>> the camera sensor data to do AI algorithm, and send the results
>> to ISH. On the other end, IVSC can share camera sensor to host
>> by routing the raw camera sensor data to the exposed MIPI CSI-2
>> interface. But they can not work at the same time, so software
>> APIs are defined to sync the ownership.
>>
>> This commit defines the interfaces between IVSC and camera sensor
>> driver in include/linux/ivsc.h. The camera driver controls
>> ownership of the CSI-2 link and sensor with the acquire/release
>> APIs. When acquiring camera, lane number and link freq are also
>> required by IVSC frame router.
> 
> The more I learn about this system, the more I'm inclined to think this
> functionality should be exposed as a V4L2 sub-device. IVSC doesn't really
> do anything to the data (as long as it directs it towards the CSI-2
> receiver in the SoC), but it is definitely part of the image pipeline.

Yes I happened to discuss this exact same thing with Laurent at FOSDEM
and we also came to the conclusion that the IVSC chip should be modeled
as a V4L2 sub-device.

Regards,

Hans



> 
> I suppose the intended use cases assume a single instance of IVSC (as well
> as MEI) but there can, and often are, be multiple camera sensors in the
> system. The decision whether to request pass-through from IVCS can't be
> done in the camera sensor driver, and should not be visible to the camera
> sensor driver. Exposing IVSC as a V4L2 sub-device makes this trivial to
> address, as the IVSC driver's V4L2 sub-device video s_stream() operation
> gets called before streaming is started.
> 
> The information whether IVSC is found between the camera sensor and the
> host's CSI-2 receiver (IPU in this case) should come from system firmware
> and accessed most probably by what is called cio2-bridge at the moment.
> 
> The privacy status can be a V4L2 control.
> 
> Also cc Hans.
> 
>>
>> Signed-off-by: Wentong Wu <wentong.wu@...el.com>
>> ---
>>  drivers/media/pci/intel/ivsc/Makefile |  1 +
>>  drivers/media/pci/intel/ivsc/ivsc.c   | 84 +++++++++++++++++++++++++++
>>  include/linux/ivsc.h                  | 55 ++++++++++++++++++
>>  3 files changed, 140 insertions(+)
>>  create mode 100644 drivers/media/pci/intel/ivsc/ivsc.c
>>
>> diff --git a/drivers/media/pci/intel/ivsc/Makefile b/drivers/media/pci/intel/ivsc/Makefile
>> index de0a425c22c2..b8b6fc1083be 100644
>> --- a/drivers/media/pci/intel/ivsc/Makefile
>> +++ b/drivers/media/pci/intel/ivsc/Makefile
>> @@ -4,3 +4,4 @@
>>  
>>  obj-$(CONFIG_INTEL_VSC) += mei_csi.o
>>  obj-$(CONFIG_INTEL_VSC) += mei_ace.o
>> +obj-$(CONFIG_INTEL_VSC) += ivsc.o
>> diff --git a/drivers/media/pci/intel/ivsc/ivsc.c b/drivers/media/pci/intel/ivsc/ivsc.c
>> new file mode 100644
>> index 000000000000..12996b587639
>> --- /dev/null
>> +++ b/drivers/media/pci/intel/ivsc/ivsc.c
>> @@ -0,0 +1,84 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Intel Corporation. All rights reserved.
>> + * Intel Visual Sensing Controller interface
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/ivsc.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +
>> +#include "mei_ace.h"
>> +#include "mei_csi.h"
>> +
>> +/* lock for ivsc APIs */
>> +static DEFINE_MUTEX(ivsc_mutex);
>> +
>> +int ivsc_acquire_camera(u32 nr_of_lanes, u64 link_freq,
>> +			void (*callback)(void *, enum ivsc_privacy_status),
>> +			void *context)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&ivsc_mutex);
>> +
>> +	/* switch camera sensor ownership to host */
>> +	ret = ace_set_camera_owner(ACE_CAMERA_HOST);
>> +	if (ret)
>> +		goto error;
>> +
>> +	/* switch CSI-2 link to host */
>> +	ret = csi_set_link_owner(CSI_LINK_HOST, callback, context);
>> +	if (ret)
>> +		goto release_camera;
>> +
>> +	/* configure CSI-2 link */
>> +	ret = csi_set_link_cfg(nr_of_lanes, link_freq);
>> +	if (ret)
>> +		goto release_csi;
>> +
>> +	mutex_unlock(&ivsc_mutex);
>> +
>> +	return 0;
>> +
>> +release_csi:
>> +	csi_set_link_owner(CSI_LINK_IVSC, NULL, NULL);
>> +
>> +release_camera:
>> +	ace_set_camera_owner(ACE_CAMERA_IVSC);
>> +
>> +error:
>> +	mutex_unlock(&ivsc_mutex);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(ivsc_acquire_camera);
>> +
>> +int ivsc_release_camera(void)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&ivsc_mutex);
>> +
>> +	/* switch CSI-2 link to IVSC */
>> +	ret = csi_set_link_owner(CSI_LINK_IVSC, NULL, NULL);
>> +	if (ret)
>> +		goto error;
>> +
>> +	/* switch camera sensor ownership to IVSC */
>> +	ret = ace_set_camera_owner(ACE_CAMERA_IVSC);
>> +
>> +error:
>> +	mutex_unlock(&ivsc_mutex);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(ivsc_release_camera);
>> +
>> +MODULE_AUTHOR("Wentong Wu <wentong.wu@...el.com>");
>> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@...el.com>");
>> +MODULE_SOFTDEP("pre: mei_csi mei_ace");
>> +MODULE_DESCRIPTION("IVSC interface");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS(IVSC);
>> diff --git a/include/linux/ivsc.h b/include/linux/ivsc.h
>> index 6572ca4f340c..bc9006cd6efc 100644
>> --- a/include/linux/ivsc.h
>> +++ b/include/linux/ivsc.h
>> @@ -16,4 +16,59 @@ enum ivsc_privacy_status {
>>  	IVSC_PRIVACY_MAX,
>>  };
>>  
>> +#if IS_ENABLED(CONFIG_INTEL_VSC)
>> +/*
>> + * @brief Acquire camera sensor ownership to host and setup
>> + * the CSI-2 link between host and IVSC
>> + *
>> + * IVSC provides a privacy mode. When the privacy mode is turned
>> + * on, camera sensor can't be used. This means that both IVSC and
>> + * host Image Processing Unit(IPU) can't get image data. And when
>> + * this mode is turned on, host Image Processing Unit(IPU) driver
>> + * is informed via the registered callback, so that user can be
>> + * notified.
>> + *
>> + * @param nr_of_lanes Number of data lanes used on the CSI-2 link
>> + * @param link_freq Frequency of the CSI-2 link
>> + * @param callback The pointer of privacy callback function
>> + * @param context Privacy callback function runtime context
>> + *
>> + * @retval 0 If success
>> + * @retval -EIO IO error
>> + * @retval -EINVAL Invalid argument
>> + * @retval -EAGAIN IVSC device not ready
>> + * @retval negative values for other errors
>> + */
>> +int ivsc_acquire_camera(u32 nr_of_lanes, u64 link_freq,
>> +			void (*callback)(void *, enum ivsc_privacy_status),
>> +			void *context);
>> +
>> +/*
>> + * @brief Release camera sensor ownership and stop the CSI-2
>> + * link between host and IVSC
>> + *
>> + * @retval 0 If success
>> + * @retval -EIO IO error
>> + * @retval -EINVAL Invalid argument
>> + * @retval -EAGAIN IVSC device not ready
>> + * @retval negative values for other errors
>> + */
>> +int ivsc_release_camera(void);
>> +
>> +#else
>> +static inline
>> +int ivsc_acquire_camera(u32 nr_of_lanes, u64 link_freq,
>> +			void (*callback)(void *, enum ivsc_privacy_status),
>> +			void *context)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int ivsc_release_camera(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +#endif
>> +
>>  #endif
> 

Powered by blists - more mailing lists