Hi,
Thanks for your review
> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> > >> ---
> > >> 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@xxxxxxxxx>");
> > >> +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@xxxxxxxxx>");
> > >> +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
