Date:   Tue, 14 Feb 2023 18:06:05 +0200
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     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, Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v2 3/3] media: pci: intel: ivsc: Add acquire/release API
 for ivsc
Message-ID: <Y+uxbQi7seGf+adP@kekkonen.localdomain>
References: <20230213022347.2480307-1-wentong.wu@intel.com>
 <20230213022347.2480307-4-wentong.wu@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230213022347.2480307-4-wentong.wu@intel.com>
Precedence: bulk

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

-- 
Kind regards,

Sakari Ailus