[v2,3/3] media: pci: intel: ivsc: Add acquire/release API for ivsc

  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.

Signed-off-by: Wentong Wu <wentong.wu@intel.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
  

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.

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@intel.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@intel.com>");
> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.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
  

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@intel.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@intel.com>");
>> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.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
>
  

On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> 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.

Agreed.

> > 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@intel.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@intel.com>");
> >> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.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
  

Hi, 

Thanks for your review

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, February 15, 2023 5:46 PM
> 
> On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > 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.
> 
> Agreed.

Thanks for your quick review and conclusion, I'm fresh to media sub-system, is there any convention that I should follow to upstream this kind of v4l2 sub-device driver so that not too much back and forth?

> 
> > > 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@intel.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@intel.com>");
> > >> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.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
> 
> --
> Regards,
> 
> Laurent Pinchart
  

Hi Wentong,

On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > 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.
> > 
> > Agreed.
> 
> Thanks for your quick review and conclusion, I'm fresh to media
> sub-system, is there any convention that I should follow to upstream
> this kind of v4l2 sub-device driver so that not too much back and
> forth?

This is a tentative proposal, as I'm not very familiar with the hardware
architecture:

- The subdev should have two pads, a sink pad connected to the camera
  sensor, and a source pad connected to the CSI-2 receiver in the IPU.

- As for any new driver, the subdev driver should use the active state
  managed by the v4l2-subdev core. This requires calling
  v4l2_subdev_init_finalize() at probe time. See
  https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
  example of a subdev driver converted to this mechanism.

- As we're talking about CSI-2, the subdev driver should use the streams
  API that was recently merged, and in particular support the
  .get_frame_desc(), .set_routing(), .enable_streams() and
  .disable_streams() operations.

- I don't see a need to support V4L2 controls in the subdev driver, but
  I may be missing something.

- The driver should be validated with v4l2-compliance, part of
  v4l-utils.

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

[snip]
  

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Friday, February 17, 2023 7:19 PM
> 
> Hi Wentong,
> 
> On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> > On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > > 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.
> > >
> > > Agreed.
> >
> > Thanks for your quick review and conclusion, I'm fresh to media
> > sub-system, is there any convention that I should follow to upstream
> > this kind of v4l2 sub-device driver so that not too much back and
> > forth?
> 
> This is a tentative proposal, as I'm not very familiar with the hardware
> architecture:
> 
> - The subdev should have two pads, a sink pad connected to the camera
>   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> 
> - As for any new driver, the subdev driver should use the active state
>   managed by the v4l2-subdev core. This requires calling
>   v4l2_subdev_init_finalize() at probe time. See
>   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
>   example of a subdev driver converted to this mechanism.
> 
> - As we're talking about CSI-2, the subdev driver should use the streams
>   API that was recently merged, and in particular support the
>   .get_frame_desc(), .set_routing(), .enable_streams() and
>   .disable_streams() operations.
> 
> - I don't see a need to support V4L2 controls in the subdev driver, but
>   I may be missing something.
> 
> - The driver should be validated with v4l2-compliance, part of
>   v4l-utils.
> 

Thanks for the detail, but I have one more question, during probe of sensor(v4l2-sudev) driver, it will configure camera sensor connected to IVSC via I2C, but before that it should acquire camera sensor's ownership from IVSC, how v4l2 framework guarantee this?

Thanks,
Wentong

> > > > > 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@intel.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
> 
> [snip]
> 
> --
> Regards,
> 
> Laurent Pinchart
  

Hi Wentong,

On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
> 
> 
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Sent: Friday, February 17, 2023 7:19 PM
> > 
> > Hi Wentong,
> > 
> > On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> > > On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > > > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > > > 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.
> > > >
> > > > Agreed.
> > >
> > > Thanks for your quick review and conclusion, I'm fresh to media
> > > sub-system, is there any convention that I should follow to upstream
> > > this kind of v4l2 sub-device driver so that not too much back and
> > > forth?
> > 
> > This is a tentative proposal, as I'm not very familiar with the hardware
> > architecture:
> > 
> > - The subdev should have two pads, a sink pad connected to the camera
> >   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> > 
> > - As for any new driver, the subdev driver should use the active state
> >   managed by the v4l2-subdev core. This requires calling
> >   v4l2_subdev_init_finalize() at probe time. See
> >   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
> >   example of a subdev driver converted to this mechanism.
> > 
> > - As we're talking about CSI-2, the subdev driver should use the streams
> >   API that was recently merged, and in particular support the
> >   .get_frame_desc(), .set_routing(), .enable_streams() and
> >   .disable_streams() operations.
> > 
> > - I don't see a need to support V4L2 controls in the subdev driver, but
> >   I may be missing something.
> > 
> > - The driver should be validated with v4l2-compliance, part of
> >   v4l-utils.
> > 
> 
> Thanks for the detail, but I have one more question, during probe of
> sensor(v4l2-sudev) driver, it will configure camera sensor connected to
> IVSC via I2C, but before that it should acquire camera sensor's ownership
> from IVSC, how v4l2 framework guarantee this?

Please wrap the lines at 74 characters or so when replying.

Do you mean accessing the sensor via I²C also requires acquiring the sensor
from IVSC?
  

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Monday, February 20, 2023 4:37 PM
> 
> Hi Wentong,
> 
> On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
> >
> >
> > > -----Original Message-----
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: Friday, February 17, 2023 7:19 PM
> > >
> > > Hi Wentong,
> > >
> > > On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> > > > On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > > > > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > > > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > > > > 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.
> > > > >
> > > > > Agreed.
> > > >
> > > > Thanks for your quick review and conclusion, I'm fresh to media
> > > > sub-system, is there any convention that I should follow to
> > > > upstream this kind of v4l2 sub-device driver so that not too much
> > > > back and forth?
> > >
> > > This is a tentative proposal, as I'm not very familiar with the
> > > hardware
> > > architecture:
> > >
> > > - The subdev should have two pads, a sink pad connected to the camera
> > >   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> > >
> > > - As for any new driver, the subdev driver should use the active state
> > >   managed by the v4l2-subdev core. This requires calling
> > >   v4l2_subdev_init_finalize() at probe time. See
> > >   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
> > >   example of a subdev driver converted to this mechanism.
> > >
> > > - As we're talking about CSI-2, the subdev driver should use the streams
> > >   API that was recently merged, and in particular support the
> > >   .get_frame_desc(), .set_routing(), .enable_streams() and
> > >   .disable_streams() operations.
> > >
> > > - I don't see a need to support V4L2 controls in the subdev driver, but
> > >   I may be missing something.
> > >
> > > - The driver should be validated with v4l2-compliance, part of
> > >   v4l-utils.
> > >
> >
> > Thanks for the detail, but I have one more question, during probe of
> > sensor(v4l2-sudev) driver, it will configure camera sensor connected
> > to IVSC via I2C, but before that it should acquire camera sensor's
> > ownership from IVSC, how v4l2 framework guarantee this?
> 
> Please wrap the lines at 74 characters or so when replying.

Thanks, I will.

> 
> Do you mean accessing the sensor via I²C also requires acquiring the sensor
> from IVSC?

Yes

> 
> --
> Regards,
> 
> Sakari Ailus
  

Hi,

On 2/20/23 09:57, Wu, Wentong wrote:
> 
> 
>> -----Original Message-----
>> From: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Sent: Monday, February 20, 2023 4:37 PM
>>
>> Hi Wentong,
>>
>> On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>> Sent: Friday, February 17, 2023 7:19 PM
>>>>
>>>> Hi Wentong,
>>>>
>>>> On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
>>>>> On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
>>>>>> On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
>>>>>>> On 2/14/23 17:06, Sakari Ailus wrote:
>>>>>>>> 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.
>>>>>>
>>>>>> Agreed.
>>>>>
>>>>> Thanks for your quick review and conclusion, I'm fresh to media
>>>>> sub-system, is there any convention that I should follow to
>>>>> upstream this kind of v4l2 sub-device driver so that not too much
>>>>> back and forth?
>>>>
>>>> This is a tentative proposal, as I'm not very familiar with the
>>>> hardware
>>>> architecture:
>>>>
>>>> - The subdev should have two pads, a sink pad connected to the camera
>>>>   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
>>>>
>>>> - As for any new driver, the subdev driver should use the active state
>>>>   managed by the v4l2-subdev core. This requires calling
>>>>   v4l2_subdev_init_finalize() at probe time. See
>>>>   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
>>>>   example of a subdev driver converted to this mechanism.
>>>>
>>>> - As we're talking about CSI-2, the subdev driver should use the streams
>>>>   API that was recently merged, and in particular support the
>>>>   .get_frame_desc(), .set_routing(), .enable_streams() and
>>>>   .disable_streams() operations.
>>>>
>>>> - I don't see a need to support V4L2 controls in the subdev driver, but
>>>>   I may be missing something.
>>>>
>>>> - The driver should be validated with v4l2-compliance, part of
>>>>   v4l-utils.
>>>>
>>>
>>> Thanks for the detail, but I have one more question, during probe of
>>> sensor(v4l2-sudev) driver, it will configure camera sensor connected
>>> to IVSC via I2C, but before that it should acquire camera sensor's
>>> ownership from IVSC, how v4l2 framework guarantee this?
>>
>> Please wrap the lines at 74 characters or so when replying.
> 
> Thanks, I will.
> 
>>
>> Do you mean accessing the sensor via I²C also requires acquiring the sensor
>> from IVSC?
> 
> Yes

Hmm, that is going to be a problem since  we really want to have
independent driver for the 2 which are not aware of each other.

I think that maybe we can model this part of the ivsc functionality
as an i2c-mux. But then we will need to somehow change the parent
of the i2c device for the sensor to the output of this mux ...

I guess this means adding some code (some hack likely) to
drivers/platform/x86/intel/int3472/discrete.c which' proe function 
is already guaranteed to run before the sensor's i2c-client gets
instantiated, because of the ACPI _DEP on the INT3472 ACPI device
in the DSDT.

This is going to be a tricky problem to tackle.

Regards,

Hans
  

Hi Sakari,

few questions as switching to v4l2 sub-dev framework.

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Wednesday, February 15, 2023 12:06 AM
> 
> 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.
> 
> 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.

This should be a control or event? If control, how user-space handle privacy stuff?

For the required link freq and lane number, is v4l2 control the correct way to configure
them? If yes, seems there is no CID value for them so that we should custom some
CID value(link freqm, lane number, and privacy) for ivsc in linux/v4l2-controls.h, is this
acceptable?

Thanks,
Wentong
> 
> Also cc Hans.
> 
> >
  

Hi Laurent,


> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Friday, February 17, 2023 7:19 PM
> 
> Hi Wentong,
> 
> On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> > On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> > > On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> > > > On 2/14/23 17:06, Sakari Ailus wrote:
> > > > > 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.
> > >
> > > Agreed.
> >
> > Thanks for your quick review and conclusion, I'm fresh to media
> > sub-system, is there any convention that I should follow to upstream
> > this kind of v4l2 sub-device driver so that not too much back and
> > forth?
> 
> This is a tentative proposal, as I'm not very familiar with the hardware
> architecture:
> 
> - The subdev should have two pads, a sink pad connected to the camera
>   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> 
> - As for any new driver, the subdev driver should use the active state
>   managed by the v4l2-subdev core. This requires calling
>   v4l2_subdev_init_finalize() at probe time. See
>   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
>   example of a subdev driver converted to this mechanism.
> 
> - As we're talking about CSI-2, the subdev driver should use the streams
>   API that was recently merged, and in particular support the
>   .get_frame_desc(), .set_routing(), .enable_streams() and
>   .disable_streams() operations.
> 
> - I don't see a need to support V4L2 controls in the subdev driver, but
>   I may be missing something.
> 
> - The driver should be validated with v4l2-compliance, part of
>   v4l-utils.

Thanks, one more question, because ivsc just routes the received data from
sensor and no format convert, how we handle the get_fmt and set_fmt callback
for ivsc?

Thanks
Wentong

> 
> > > > > 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@intel.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
> 
> [snip]
> 
> --
> Regards,
> 
> Laurent Pinchart
  

Hi Wentong,

On Tue, Feb 28, 2023 at 06:35:41AM +0000, Wu, Wentong wrote:
> Hi Sakari,
> 
> few questions as switching to v4l2 sub-dev framework.
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Wednesday, February 15, 2023 12:06 AM
> > 
> > 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.
> > 
> > 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.
> 
> This should be a control or event? If control, how user-space handle
> privacy stuff?

Changing control events generates events for the user space.

<URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/dev-event.html>

> 
> For the required link freq and lane number, is v4l2 control the correct
> way to configure them? If yes, seems there is no CID value for them so
> that we should custom some CID value(link freqm, lane number, and
> privacy) for ivsc in linux/v4l2-controls.h, is this acceptable?

You should obtain these using the V4L2 fwnode interface. Please see e.g.
drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c how that driver
registers a V4L2 async sub-device and a V4L2 async notifier.
  

On Mon, Feb 20, 2023 at 10:03:08AM +0100, Hans de Goede wrote:
> On 2/20/23 09:57, Wu, Wentong wrote:
> >> -----Original Message-----
> >> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Sent: Monday, February 20, 2023 4:37 PM
> >>
> >> Hi Wentong,
> >>
> >> On Mon, Feb 20, 2023 at 03:50:55AM +0000, Wu, Wentong wrote:
> >>>> -----Original Message-----
> >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>> Sent: Friday, February 17, 2023 7:19 PM
> >>>>
> >>>> Hi Wentong,
> >>>>
> >>>> On Fri, Feb 17, 2023 at 06:10:22AM +0000, Wu, Wentong wrote:
> >>>>> On Sent: Wednesday, February 15, 2023 5:46 PM, Laurent Pinchart wrote:
> >>>>>> On Wed, Feb 15, 2023 at 10:03:29AM +0100, Hans de Goede wrote:
> >>>>>>> On 2/14/23 17:06, Sakari Ailus wrote:
> >>>>>>>> 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.
> >>>>>>
> >>>>>> Agreed.
> >>>>>
> >>>>> Thanks for your quick review and conclusion, I'm fresh to media
> >>>>> sub-system, is there any convention that I should follow to
> >>>>> upstream this kind of v4l2 sub-device driver so that not too much
> >>>>> back and forth?
> >>>>
> >>>> This is a tentative proposal, as I'm not very familiar with the
> >>>> hardware
> >>>> architecture:
> >>>>
> >>>> - The subdev should have two pads, a sink pad connected to the camera
> >>>>   sensor, and a source pad connected to the CSI-2 receiver in the IPU.
> >>>>
> >>>> - As for any new driver, the subdev driver should use the active state
> >>>>   managed by the v4l2-subdev core. This requires calling
> >>>>   v4l2_subdev_init_finalize() at probe time. See
> >>>>   https://git.linuxtv.org/media_tree.git/commit/?id=a2514b9a634a for an
> >>>>   example of a subdev driver converted to this mechanism.
> >>>>
> >>>> - As we're talking about CSI-2, the subdev driver should use the streams
> >>>>   API that was recently merged, and in particular support the
> >>>>   .get_frame_desc(), .set_routing(), .enable_streams() and
> >>>>   .disable_streams() operations.
> >>>>
> >>>> - I don't see a need to support V4L2 controls in the subdev driver, but
> >>>>   I may be missing something.
> >>>>
> >>>> - The driver should be validated with v4l2-compliance, part of
> >>>>   v4l-utils.
> >>>>
> >>>
> >>> Thanks for the detail, but I have one more question, during probe of
> >>> sensor(v4l2-sudev) driver, it will configure camera sensor connected
> >>> to IVSC via I2C, but before that it should acquire camera sensor's
> >>> ownership from IVSC, how v4l2 framework guarantee this?
> >>
> >> Please wrap the lines at 74 characters or so when replying.
> > 
> > Thanks, I will.
> > 
> >> Do you mean accessing the sensor via I²C also requires acquiring the sensor
> >> from IVSC?
> > 
> > Yes
> 
> Hmm, that is going to be a problem since  we really want to have
> independent driver for the 2 which are not aware of each other.
> 
> I think that maybe we can model this part of the ivsc functionality
> as an i2c-mux. But then we will need to somehow change the parent
> of the i2c device for the sensor to the output of this mux ...
> 
> I guess this means adding some code (some hack likely) to
> drivers/platform/x86/intel/int3472/discrete.c which' proe function 
> is already guaranteed to run before the sensor's i2c-client gets
> instantiated, because of the ACPI _DEP on the INT3472 ACPI device
> in the DSDT.
> 
> This is going to be a tricky problem to tackle.

From a framework point of view I would also recommend i2c-mux, but when
it comes to the ACPI integration... I can feel your pain and I'm sorry
about that :-(
  

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Tuesday, February 28, 2023 4:24 PM
> 
> Hi Wentong,
> 
> On Tue, Feb 28, 2023 at 06:35:41AM +0000, Wu, Wentong wrote:
> > Hi Sakari,
> >
> > few questions as switching to v4l2 sub-dev framework.
> >
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Sent: Wednesday, February 15, 2023 12:06 AM
> > >
> > > 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.
> > >
> > > 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.
> >
> > This should be a control or event? If control, how user-space handle
> > privacy stuff?
> 
> Changing control events generates events for the user space.
> 
> <URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/dev-event.html>

Ok, V4L2_EVENT_CTRL event which reports 'struct v4l2_event_ctrl' data to user space

> 
> >
> > For the required link freq and lane number, is v4l2 control the
> > correct way to configure them? If yes, seems there is no CID value for
> > them so that we should custom some CID value(link freqm, lane number,
> > and
> > privacy) for ivsc in linux/v4l2-controls.h, is this acceptable?
> 
> You should obtain these using the V4L2 fwnode interface. Please see e.g.
> drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c how that driver registers
> a V4L2 async sub-device and a V4L2 async notifier.

Ok, something like v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_LINK_FREQ); is to get sensor's link
frequency, and the code like 'v4l2_subdev_call(source, pad, get_mbus_config, source, &mbus_config);
num_of_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;' is to get sensor's lane number.

BR,
Wentong
> 
> --
> Kind regards,
> 
> Sakari Ailus
  

Hi Wentong,

On Wed, Mar 01, 2023 at 07:26:40AM +0000, Wu, Wentong wrote:
> 
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Tuesday, February 28, 2023 4:24 PM
> > 
> > Hi Wentong,
> > 
> > On Tue, Feb 28, 2023 at 06:35:41AM +0000, Wu, Wentong wrote:
> > > Hi Sakari,
> > >
> > > few questions as switching to v4l2 sub-dev framework.
> > >
> > > > -----Original Message-----
> > > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Sent: Wednesday, February 15, 2023 12:06 AM
> > > >
> > > > 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.
> > > >
> > > > 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.
> > >
> > > This should be a control or event? If control, how user-space handle
> > > privacy stuff?
> > 
> > Changing control events generates events for the user space.
> > 
> > <URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/dev-event.html>
> 
> Ok, V4L2_EVENT_CTRL event which reports 'struct v4l2_event_ctrl' data to user space
> 
> > 
> > >
> > > For the required link freq and lane number, is v4l2 control the
> > > correct way to configure them? If yes, seems there is no CID value for
> > > them so that we should custom some CID value(link freqm, lane number,
> > > and
> > > privacy) for ivsc in linux/v4l2-controls.h, is this acceptable?
> > 
> > You should obtain these using the V4L2 fwnode interface. Please see e.g.
> > drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c how that driver registers
> > a V4L2 async sub-device and a V4L2 async notifier.
> 
> Ok, something like v4l2_ctrl_find(source->ctrl_handler,
> V4L2_CID_LINK_FREQ); is to get sensor's link frequency, and the code like
> 'v4l2_subdev_call(source, pad, get_mbus_config, source, &mbus_config);
> num_of_lanes = mbus_config.bus.mipi_csi2.num_data_lanes;' is to get
> sensor's lane number.

Please use the V4L2 fwnode interface instead.

The privacy control should only be needed in the user space.