| Message ID | 1681096465-17287-1-git-send-email-wentong.wu@intel.com |
|---|---|
| Headers | show |
| Series | media: pci: intel: ivsc: Add driver of Intel Visual Sensing Controller(IVSC) | expand |
Hi, Please help review this V5 patch set, thanks a lot! BR, Wentong > -----Original Message----- > From: Wu, Wentong <wentong.wu@intel.com> > Sent: Monday, April 10, 2023 11:14 AM > To: sakari.ailus@linux.intel.com; hdegoede@redhat.com; djrscally@gmail.com; > > Intel Visual Sensing Controller (IVSC), codenamed "Clover Falls", is a companion > chip designed to provide secure and low power vision capability to IA platforms. > IVSC is available in existing commercial platforms from multiple OEMs. > > The primary use case of IVSC is to bring in context awareness. IVSC interfaces > directly with the platform main camera sensor via a CSI-2 link and processes the > image data with the embedded AI engine. The detected events are sent over I2C > to ISH (Intel Sensor Hub) for additional data fusion from multiple sensors. The > fusion results are used to implement advanced use cases like: > - Face detection to unlock screen > - Detect user presence to manage backlight setting or waking up system > > Since the Image Processing Unit(IPU) used on the host processor needs to > configure the CSI-2 link in normal camera usages, the CSI-2 link and camera > sensor can only be used in mutually-exclusive ways by host IPU and IVSC. By > default the IVSC owns the CSI-2 link and camera sensor. The IPU driver can take > ownership of the CSI-2 link and camera sensor using the interfaces exported via > v4l2 sub-device. > > Switching ownership requires an interface with two different hardware modules > inside IVSC. The software interface to these modules is via Intel MEI (The Intel > Management Engine) commands. These two hardware modules have two > different MEI UUIDs to enumerate. These hardware modules are: > - ACE (Algorithm Context Engine): This module is for algorithm computing when > IVSC owns camera sensor. Also ACE module controls camera sensor's ownership. > This hardware module is used to set ownership of camera sensor. > - CSI (Camera Serial Interface): This module is used to route camera sensor data > either to IVSC or to host for IPU driver and application. > > IVSC also provides a privacy mode. When privacy mode is turned on, camera > sensor can't be used. This means that both ACE and host IPU can't get image > data. And when this mode is turned on, users are informed via v4l2 control API. > > In summary, to acquire ownership of camera by IPU driver, first ACE module > needs to be informed of ownership and then to setup MIPI CSI-2 link for the > camera sensor and IPU. > > Implementation: > There are two different drivers to handle ACE and CSI hardware modules inside > IVSC. > - ivsc_csi: MEI client driver to send commands and receive notifications from > CSI module. > - ivsc_ace: MEI client driver to send commands and get status from ACE module. > Interface is exposed via v4l2 sub-devcie APIs to acquire and release camera > sensor and CSI-2 link. > > Below diagram shows connections of IVSC/ISH/IPU/Camera sensor. > ----------------------------------------------------------------------------- > | Host Processor | > | | > | ----------------- ----------------- --------------- | > | | | | | | | I2C | > | | IPU | | ISH | |camera driver|--| | > | | | | | | | | | > | ----------------- ----------------- --------------- | | > | | | | | | > | | | --------------- | | > | | | | | | | > | | | | IVSC driver | | | > | | | | | | | > | | | --------------- | | > | | | | | | > ----------------|-----------------------|----------------------|---------|--- > | CSI | I2C |SPI | > | | | | > ----------------|-----------------------|---------------- | | > | IVSC | | | | > | | | | | > | ----------------- ----------------- | | | > | | | | | | | | > | | CSI | | ACE | |------| | > | | | | | | | > | ----------------- ----------------- | | > | | | I2C | | > ----------------|-----------------------|---------------- | > | CSI | | > | | | > -------------------------------- | > | | I2C | > | camera sensor |-----------------------------| > | | > -------------------------------- > > --- > v5: > - probe mei_csi only after software node has been setup > > v4: > - call v4l2_ctrl_handler_free() if setting up the handler failed > - set V4L2_CTRL_FLAG_READ_ONLY for privacy_ctrl > - add dev_warn if failed to switch CSI-2 link to IVSC > - use v4l2_fwnode_endpoint_alloc_parse to get num_data_lanes > - add document about how sensor connected to IVSC is powered > - move lock to mei_ace_send > - check return value for device_link_add > > Wentong Wu (3): > media: pci: intel: ivsc: Add CSI submodule > media: pci: intel: ivsc: Add ACE submodule > ACPI: delay enumeration of devices with a _DEP pointing to IVSC device > > drivers/acpi/scan.c | 2 + > drivers/media/pci/Kconfig | 1 + > drivers/media/pci/intel/Makefile | 2 + > drivers/media/pci/intel/ivsc/Kconfig | 12 + > drivers/media/pci/intel/ivsc/Makefile | 9 + > drivers/media/pci/intel/ivsc/mei_ace.c | 560 ++++++++++++++++++++++++ > drivers/media/pci/intel/ivsc/mei_csi.c | 772 > +++++++++++++++++++++++++++++++++ > 7 files changed, 1358 insertions(+) > create mode 100644 drivers/media/pci/intel/ivsc/Kconfig > create mode 100644 drivers/media/pci/intel/ivsc/Makefile > create mode 100644 drivers/media/pci/intel/ivsc/mei_ace.c > create mode 100644 drivers/media/pci/intel/ivsc/mei_csi.c > > -- > 2.7.4
On Mon, Apr 10, 2023 at 11:14:23AM +0800, Wentong Wu wrote: > diff --git a/drivers/media/pci/intel/ivsc/Kconfig b/drivers/media/pci/intel/ivsc/Kconfig > new file mode 100644 > index 0000000..9535ac1 > --- /dev/null > +++ b/drivers/media/pci/intel/ivsc/Kconfig > @@ -0,0 +1,12 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# Copyright (C) 2023, Intel Corporation. All rights reserved. > + > +config INTEL_VSC > + tristate "Intel Visual Sensing Controller" > + depends on INTEL_MEI I think I requested to depend on PM in an earlier review, didn't I?
Hi Wentong, On Mon, Apr 10, 2023 at 11:14:24AM +0800, Wentong Wu wrote: > ACE is a submodule of IVSC which controls camera sensor's > ownership, belonging to host or IVSC. When IVSC owns camera > sensor, it is for algorithm computing. When host wants to > control camera sensor, ACE module needs to be informed of > ownership with defined interface. > > The interface is via MEI. There is a separate MEI UUID, which > this driver uses to enumerate. > > To switch ownership of camera sensor between IVSC and host, > the caller specifies the defined ownership information which > will be sent to firmware by sending MEI command. > > Device link(device_link_add) is used to set the right camera > sensor ownership before accessing the sensor via I2C. With > DL_FLAG_PM_RUNTIME and DL_FLAG_RPM_ACTIVE, the supplier device > will be PM runtime resumed before the consumer(camera sensor). > So use runtime PM callbacks to transfer the ownership between > host and IVSC. > > Signed-off-by: Wentong Wu <wentong.wu@intel.com> > --- > drivers/media/pci/intel/ivsc/Makefile | 3 + > drivers/media/pci/intel/ivsc/mei_ace.c | 560 +++++++++++++++++++++++++++++++++ > 2 files changed, 563 insertions(+) > create mode 100644 drivers/media/pci/intel/ivsc/mei_ace.c > > diff --git a/drivers/media/pci/intel/ivsc/Makefile b/drivers/media/pci/intel/ivsc/Makefile > index cbd194a..00fad29 100644 > --- a/drivers/media/pci/intel/ivsc/Makefile > +++ b/drivers/media/pci/intel/ivsc/Makefile > @@ -4,3 +4,6 @@ > > obj-$(CONFIG_INTEL_VSC) += ivsc-csi.o > ivsc-csi-y += mei_csi.o > + > +obj-$(CONFIG_INTEL_VSC) += ivsc-ace.o > +ivsc-ace-y += mei_ace.o > diff --git a/drivers/media/pci/intel/ivsc/mei_ace.c b/drivers/media/pci/intel/ivsc/mei_ace.c > new file mode 100644 > index 0000000..e76a871 > --- /dev/null > +++ b/drivers/media/pci/intel/ivsc/mei_ace.c > @@ -0,0 +1,560 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 Intel Corporation. All rights reserved. > + * Intel Visual Sensing Controller ACE Linux driver > + */ > + > +/* > + * To set ownership of camera sensor, there is specific command, which > + * is sent via MEI protocol. That's a two-step scheme where the firmware > + * first acks receipt of the command and later responses the command was > + * executed. The command sending function uses "completion" as the > + * synchronization mechanism. The notification for command is received > + * via a mei callback which wakes up the caller. There can be only one > + * outstanding command at a time. > + * > + * The power line of camera sensor is directly connected to IVSC instead > + * of host, when camera sensor ownership is switched to host, sensor is > + * already powered up by firmware. > + */ > + > +#include <linux/acpi.h> > +#include <linux/completion.h> > +#include <linux/delay.h> > +#include <linux/kernel.h> > +#include <linux/mei_cl_bus.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/pm_runtime.h> > +#include <linux/slab.h> > +#include <linux/uuid.h> > + > +#define MEI_ACE_DRIVER_NAME "ivsc_ace" > + > +/* indicating driver message */ > +#define ACE_DRV_MSG 1 > +/* indicating set command */ > +#define ACE_CMD_SET 4 > +/* command timeout determined experimentally */ > +#define ACE_CMD_TIMEOUT (5 * HZ) > +/* indicating the first command block */ > +#define ACE_CMD_INIT_BLOCK 1 > +/* indicating the last command block */ > +#define ACE_CMD_FINAL_BLOCK 1 > +/* size of camera status notification content */ > +#define ACE_CAMERA_STATUS_SIZE 5 > + > +/* UUID used to get firmware id */ > +#define ACE_GET_FW_ID_UUID UUID_LE(0x6167DCFB, 0x72F1, 0x4584, 0xBF, \ > + 0xE3, 0x84, 0x17, 0x71, 0xAA, 0x79, 0x0B) > + > +/* UUID used to get csi device */ > +#define MEI_CSI_UUID UUID_LE(0x92335FCF, 0x3203, 0x4472, \ > + 0xAF, 0x93, 0x7b, 0x44, 0x53, 0xAC, 0x29, 0xDA) > + > +/* identify firmware event type */ > +enum ace_event_type { > + /* firmware ready */ > + ACE_FW_READY = 0x8, > + > + /* command response */ > + ACE_CMD_RESPONSE = 0x10, > +}; > + > +/* identify camera sensor ownership */ > +enum ace_camera_owner { > + ACE_CAMERA_IVSC, > + ACE_CAMERA_HOST, > +}; > + > +/* identify the command id supported by firmware IPC */ > +enum ace_cmd_id { > + /* used to switch camera sensor to host */ > + ACE_SWITCH_CAMERA_TO_HOST = 0x13, > + > + /* used to switch camera sensor to IVSC */ > + ACE_SWITCH_CAMERA_TO_IVSC = 0x14, > + > + /* used to get firmware id */ > + ACE_GET_FW_ID = 0x1A, > +}; > + > +/* ACE command header structure */ > +struct ace_cmd_hdr { > + u32 firmware_id : 16; > + u32 instance_id : 8; > + u32 type : 5; > + u32 rsp : 1; > + u32 msg_tgt : 1; > + u32 _hw_rsvd_1 : 1; > + u32 param_size : 20; > + u32 cmd_id : 8; > + u32 final_block : 1; > + u32 init_block : 1; > + u32 _hw_rsvd_2 : 2; > +} __packed; > + > +/* ACE command parameter structure */ > +union ace_cmd_param { > + uuid_le uuid; > + u32 param; > +}; > + > +/* ACE command structure */ > +struct ace_cmd { > + struct ace_cmd_hdr hdr; > + union ace_cmd_param param; > +} __packed; > + > +/* ACE notification header */ > +union ace_notif_hdr { > + struct _confirm { > + u32 status : 24; > + u32 type : 5; > + u32 rsp : 1; > + u32 msg_tgt : 1; > + u32 _hw_rsvd_1 : 1; > + u32 param_size : 20; > + u32 cmd_id : 8; > + u32 final_block : 1; > + u32 init_block : 1; > + u32 _hw_rsvd_2 : 2; > + } __packed ack; > + > + struct _event { > + u32 rsvd1 : 16; > + u32 event_type : 8; > + u32 type : 5; > + u32 ack : 1; > + u32 msg_tgt : 1; > + u32 _hw_rsvd_1 : 1; > + u32 rsvd2 : 30; > + u32 _hw_rsvd_2 : 2; > + } __packed event; > + > + struct _response { > + u32 event_id : 16; > + u32 notif_type : 8; > + u32 type : 5; > + u32 rsp : 1; > + u32 msg_tgt : 1; > + u32 _hw_rsvd_1 : 1; > + u32 event_data_size : 16; > + u32 request_target : 1; > + u32 request_type : 5; > + u32 cmd_id : 8; > + u32 _hw_rsvd_2 : 2; > + } __packed response; > +}; > + > +/* ACE notification content */ > +union ace_notif_cont { > + u16 firmware_id; > + u8 state_notif; > + u8 camera_status[ACE_CAMERA_STATUS_SIZE]; > +}; > + > +/* ACE notification structure */ > +struct ace_notif { > + union ace_notif_hdr hdr; > + union ace_notif_cont cont; > +} __packed; > + > +struct mei_ace { > + struct mei_cl_device *cldev; > + > + /* command ack */ > + struct ace_notif cmd_ack; > + /* command response */ > + struct ace_notif cmd_response; > + /* used to wait for command ack and response */ > + struct completion cmd_completion; > + /* lock used to prevent multiple call to send command */ > + struct mutex lock; > + > + /* used to construct command */ > + u16 firmware_id; > + > + /* runtime PM link from ace to csi */ > + struct device_link *csi_link; > + /* runtime PM link from ace to sensor */ > + struct device_link *sensor_link; > +}; > + > +static inline void init_cmd_hdr(struct ace_cmd_hdr *hdr) > +{ > + memset(hdr, 0, sizeof(struct ace_cmd_hdr)); > + > + hdr->type = ACE_CMD_SET; > + hdr->msg_tgt = ACE_DRV_MSG; > + hdr->init_block = ACE_CMD_INIT_BLOCK; > + hdr->final_block = ACE_CMD_FINAL_BLOCK; > +} > + > +static int construct_command(struct mei_ace *ace, struct ace_cmd *cmd, > + enum ace_cmd_id cmd_id) > +{ > + union ace_cmd_param *param = &cmd->param; > + struct ace_cmd_hdr *hdr = &cmd->hdr; > + > + init_cmd_hdr(hdr); > + > + hdr->cmd_id = cmd_id; > + switch (cmd_id) { > + case ACE_GET_FW_ID: > + param->uuid = ACE_GET_FW_ID_UUID; > + hdr->param_size = sizeof(param->uuid); > + break; > + case ACE_SWITCH_CAMERA_TO_IVSC: > + param->param = 0; > + hdr->firmware_id = ace->firmware_id; > + hdr->param_size = sizeof(param->param); > + break; > + case ACE_SWITCH_CAMERA_TO_HOST: > + hdr->firmware_id = ace->firmware_id; > + break; > + default: > + return -EINVAL; > + } > + > + return hdr->param_size + sizeof(cmd->hdr); > +} > + > +/* send command to firmware */ > +static int mei_ace_send(struct mei_ace *ace, struct ace_cmd *cmd, > + size_t len, bool only_ack) > +{ > + union ace_notif_hdr *resp_hdr = &ace->cmd_response.hdr; > + union ace_notif_hdr *ack_hdr = &ace->cmd_ack.hdr; > + struct ace_cmd_hdr *cmd_hdr = &cmd->hdr; > + int ret; > + > + mutex_lock(&ace->lock); > + > + reinit_completion(&ace->cmd_completion); > + > + ret = mei_cldev_send(ace->cldev, (u8 *)cmd, len); > + if (ret < 0) > + goto out; > + > + ret = wait_for_completion_killable_timeout(&ace->cmd_completion, > + ACE_CMD_TIMEOUT); > + if (ret < 0) { > + goto out; > + } else if (!ret) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + if (ack_hdr->ack.cmd_id != cmd_hdr->cmd_id) { > + ret = -EINVAL; > + goto out; > + } > + > + /* command ack status */ > + ret = ack_hdr->ack.status; > + if (ret) { > + ret = -EIO; > + goto out; > + } > + > + if (only_ack) > + goto out; > + > + ret = wait_for_completion_killable_timeout(&ace->cmd_completion, > + ACE_CMD_TIMEOUT); > + if (ret < 0) { > + goto out; > + } else if (!ret) { > + ret = -ETIMEDOUT; > + goto out; > + } > + > + if (resp_hdr->response.cmd_id != cmd_hdr->cmd_id) > + ret = -EINVAL; > + > +out: > + mutex_unlock(&ace->lock); > + > + return ret; > +} > + > +static int ace_set_camera_owner(struct mei_ace *ace, > + enum ace_camera_owner owner) > +{ > + enum ace_cmd_id cmd_id; > + struct ace_cmd cmd; > + int cmd_size; > + int ret; > + > + if (owner == ACE_CAMERA_IVSC) > + cmd_id = ACE_SWITCH_CAMERA_TO_IVSC; > + else > + cmd_id = ACE_SWITCH_CAMERA_TO_HOST; > + > + cmd_size = construct_command(ace, &cmd, cmd_id); > + if (cmd_size >= 0) > + ret = mei_ace_send(ace, &cmd, cmd_size, false); > + else > + ret = cmd_size; > + > + return ret; > +} > + > +/* the first command downloaded to firmware */ > +static inline int ace_get_firmware_id(struct mei_ace *ace) > +{ > + struct ace_cmd cmd; > + int cmd_size; > + int ret; > + > + cmd_size = construct_command(ace, &cmd, ACE_GET_FW_ID); > + if (cmd_size >= 0) > + ret = mei_ace_send(ace, &cmd, cmd_size, true); > + else > + ret = cmd_size; > + > + return ret; > +} > + > +static void handle_command_response(struct mei_ace *ace, > + struct ace_notif *resp, int len) > +{ > + union ace_notif_hdr *hdr = &resp->hdr; > + > + switch (hdr->response.cmd_id) { > + case ACE_SWITCH_CAMERA_TO_IVSC: > + case ACE_SWITCH_CAMERA_TO_HOST: > + memcpy(&ace->cmd_response, resp, len); > + complete(&ace->cmd_completion); > + break; > + case ACE_GET_FW_ID: > + break; > + default: > + break; > + } > +} > + > +static void handle_command_ack(struct mei_ace *ace, > + struct ace_notif *ack, int len) > +{ > + union ace_notif_hdr *hdr = &ack->hdr; > + > + switch (hdr->ack.cmd_id) { > + case ACE_GET_FW_ID: > + ace->firmware_id = ack->cont.firmware_id; > + fallthrough; > + case ACE_SWITCH_CAMERA_TO_IVSC: > + case ACE_SWITCH_CAMERA_TO_HOST: > + memcpy(&ace->cmd_ack, ack, len); > + complete(&ace->cmd_completion); > + break; > + default: > + break; > + } > +} > + > +/* callback for receive */ > +static void mei_ace_rx(struct mei_cl_device *cldev) > +{ > + struct mei_ace *ace = mei_cldev_get_drvdata(cldev); > + struct ace_notif event; > + union ace_notif_hdr *hdr = &event.hdr; > + int ret; > + > + ret = mei_cldev_recv(cldev, (u8 *)&event, sizeof(event)); > + if (ret < 0) { > + dev_err(&cldev->dev, "recv error: %d\n", ret); > + return; > + } > + > + if (hdr->event.ack) { > + handle_command_ack(ace, &event, ret); > + return; > + } > + > + switch (hdr->event.event_type) { > + case ACE_CMD_RESPONSE: > + handle_command_response(ace, &event, ret); > + break; > + case ACE_FW_READY: > + /* > + * firmware ready notification sent to driver > + * after HECI client connected with firmware. > + */ > + dev_dbg(&cldev->dev, "firmware ready\n"); > + break; > + default: > + break; > + } > +} > + > +static int mei_ace_setup_dev_link(struct mei_ace *ace) > +{ > + struct device *dev = &ace->cldev->dev; > + uuid_le uuid = MEI_CSI_UUID; > + struct acpi_device *adev; > + struct device *csi_dev; > + char name[64]; > + > + snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev->parent), &uuid); > + > + csi_dev = device_find_child_by_name(dev->parent, name); > + if (!csi_dev) Note that you'll need to put csi_dev if probe fails. Same for driver's remove. > + return -EPROBE_DEFER; > + > + /* sensor's ACPI _DEP is mei bus device */ > + adev = acpi_dev_get_next_consumer_dev(ACPI_COMPANION(dev->parent), > + NULL); > + if (!adev) > + return -EPROBE_DEFER; Good, the devices can be found here. > + > + /* setup link between mei_ace and mei_csi */ > + ace->csi_link = device_link_add(csi_dev, dev, DL_FLAG_PM_RUNTIME | > + DL_FLAG_RPM_ACTIVE); > + if (!ace->csi_link) { > + dev_err(dev, "failed to link to %s\n", dev_name(csi_dev)); > + return -EINVAL; > + } > + > + /* setup link between mei_ace and sensor */ > + ace->sensor_link = device_link_add(&adev->dev, dev, DL_FLAG_PM_RUNTIME | > + DL_FLAG_RPM_ACTIVE); > + if (!ace->sensor_link) { > + dev_err(dev, "failed to link to %s\n", dev_name(&adev->dev)); Please add error handling --- remove created links if probe fails. > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int mei_ace_probe(struct mei_cl_device *cldev, > + const struct mei_cl_device_id *id) > +{ > + struct device *dev = &cldev->dev; > + struct mei_ace *ace; > + int ret; > + > + ace = devm_kzalloc(dev, sizeof(struct mei_ace), GFP_KERNEL); > + if (!ace) > + return -ENOMEM; > + > + ace->cldev = cldev; > + > + ret = mei_ace_setup_dev_link(ace); > + if (ret) > + return ret; > + > + mutex_init(&ace->lock); > + init_completion(&ace->cmd_completion); > + > + mei_cldev_set_drvdata(cldev, ace); > + > + ret = mei_cldev_enable(cldev); > + if (ret < 0) { > + dev_err(dev, "mei_cldev_enable failed: %d\n", ret); > + goto destroy_mutex; > + } > + > + ret = mei_cldev_register_rx_cb(cldev, mei_ace_rx); > + if (ret) { > + dev_err(dev, "event cb registration failed: %d\n", ret); > + goto err_disable; > + } > + > + ret = ace_get_firmware_id(ace); > + if (ret) { > + dev_err(dev, "get firmware id failed: %d\n", ret); > + goto err_disable; > + } > + > + if (IS_ENABLED(CONFIG_PM)) { > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + } else { > + ret = ace_set_camera_owner(ace, ACE_SWITCH_CAMERA_TO_HOST); > + if (ret) { > + dev_err(dev, "switch camera to host failed: %d\n", ret); > + goto err_disable; > + } > + } > + > + acpi_dev_clear_dependencies(ACPI_COMPANION(dev->parent)); > + > + return 0; > + > +err_disable: > + mei_cldev_disable(cldev); > + > +destroy_mutex: > + mutex_destroy(&ace->lock); > + > + device_link_del(ace->csi_link); > + device_link_del(ace->sensor_link); > + > + return ret; > +} > + > +static void mei_ace_remove(struct mei_cl_device *cldev) > +{ > + struct mei_ace *ace = mei_cldev_get_drvdata(cldev); > + > + device_link_del(ace->csi_link); > + device_link_del(ace->sensor_link); > + > + if (IS_ENABLED(CONFIG_PM)) { > + pm_runtime_disable(&cldev->dev); > + pm_runtime_set_suspended(&cldev->dev); > + } else { > + ace_set_camera_owner(ace, ACE_SWITCH_CAMERA_TO_IVSC); > + } > + > + mutex_destroy(&ace->lock); > +} > + > +static int __maybe_unused mei_ace_runtime_suspend(struct device *dev) > +{ > + struct mei_ace *ace = dev_get_drvdata(dev); > + > + return ace_set_camera_owner(ace, ACE_SWITCH_CAMERA_TO_IVSC); > +} > + > +static int __maybe_unused mei_ace_runtime_resume(struct device *dev) > +{ > + struct mei_ace *ace = dev_get_drvdata(dev); > + > + return ace_set_camera_owner(ace, ACE_SWITCH_CAMERA_TO_HOST); > +} > + > +static const struct dev_pm_ops mei_ace_pm_ops = { > + SET_RUNTIME_PM_OPS(mei_ace_runtime_suspend, > + mei_ace_runtime_resume, NULL) > +}; > + > +#define MEI_ACE_UUID UUID_LE(0x5DB76CF6, 0x0A68, 0x4ED6, \ > + 0x9B, 0x78, 0x03, 0x61, 0x63, 0x5E, 0x24, 0x47) > + > +static const struct mei_cl_device_id mei_ace_tbl[] = { > + { MEI_ACE_DRIVER_NAME, MEI_ACE_UUID, MEI_CL_VERSION_ANY }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(mei, mei_ace_tbl); > + > +static struct mei_cl_driver mei_ace_driver = { > + .id_table = mei_ace_tbl, > + .name = MEI_ACE_DRIVER_NAME, > + > + .probe = mei_ace_probe, > + .remove = mei_ace_remove, > + > + .driver = { > + .pm = &mei_ace_pm_ops, > + }, > +}; > + > +module_mei_cl_driver(mei_ace_driver); > + > +MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>"); > +MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>"); > +MODULE_DESCRIPTION("Device driver for IVSC ACE"); > +MODULE_LICENSE("GPL");
Hi Sakari, Thanks for your review > -----Original Message----- > From: Sakari Ailus <sakari.ailus@linux.intel.com> > Sent: Monday, April 24, 2023 3:25 AM > To: Wu, Wentong <wentong.wu@intel.com> > > > + > > +static int mei_ace_setup_dev_link(struct mei_ace *ace) { > > + struct device *dev = &ace->cldev->dev; > > + uuid_le uuid = MEI_CSI_UUID; > > + struct acpi_device *adev; > > + struct device *csi_dev; > > + char name[64]; > > + > > + snprintf(name, sizeof(name), "%s-%pUl", dev_name(dev->parent), > > +&uuid); > > + > > + csi_dev = device_find_child_by_name(dev->parent, name); > > + if (!csi_dev) > > Note that you'll need to put csi_dev if probe fails. Same for driver's remove. Thanks > > > + return -EPROBE_DEFER; > > + > > + /* sensor's ACPI _DEP is mei bus device */ > > + adev = acpi_dev_get_next_consumer_dev(ACPI_COMPANION(dev- > >parent), > > + NULL); > > + if (!adev) > > + return -EPROBE_DEFER; > > Good, the devices can be found here. > > > + > > + /* setup link between mei_ace and mei_csi */ > > + ace->csi_link = device_link_add(csi_dev, dev, DL_FLAG_PM_RUNTIME | > > + DL_FLAG_RPM_ACTIVE); > > + if (!ace->csi_link) { > > + dev_err(dev, "failed to link to %s\n", dev_name(csi_dev)); > > + return -EINVAL; > > + } > > + > > + /* setup link between mei_ace and sensor */ > > + ace->sensor_link = device_link_add(&adev->dev, dev, > DL_FLAG_PM_RUNTIME | > > + DL_FLAG_RPM_ACTIVE); > > + if (!ace->sensor_link) { > > + dev_err(dev, "failed to link to %s\n", dev_name(&adev->dev)); > > Please add error handling --- remove created links if probe fails. ack, thanks > > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > -- > Regards, > > Sakari Ailus
Hi Sakari, > -----Original Message----- > From: Sakari Ailus <sakari.ailus@linux.intel.com> > Sent: Monday, April 24, 2023 2:57 PM > To: Wu, Wentong <wentong.wu@intel.com> > > > > > + > > > > +/* configure CSI-2 link between host and IVSC */ static int > > > > +csi_set_link_cfg(struct mei_csi *csi) { > > > > + struct csi_cmd cmd = { 0 }; > > > > + size_t cmd_size; > > > > + int ret; > > > > + > > > > + cmd.cmd_id = CSI_SET_CONF; > > > > + cmd.param.conf.nr_of_lanes = csi->nr_of_lanes; > > > > + cmd.param.conf.link_freq = CSI_LINK_FREQ(csi->link_freq); > > > > + cmd_size = sizeof(cmd.cmd_id) + sizeof(cmd.param.conf); > > > > > > Could you initialise this in variable declaration as I requested earlier? > > > The same for other similar cases. > > > > I believe we have discussed this on v3 patch set as below: > > > > > > > In some cases you're using memset and in others not. If you don't > need memset, > > > > > I'd prefer assigning the fields in variable declaration instead. > > > > > > > > The declaration will be like below, but it will break reverse x-mas > tree style. > > > > > > > > struct csi_cmd cmd = { 0 }; > > > > size_t cmd_size = sizeof(cmd.cmd_id) + sizeof(cmd.param.param); > > > > int ret; > > > > > It's not a problem if you have a dependency. > > Yes, I meant the non-Christmas tree (reverse or not) ordering is not an issue. > Dependencies of course are of higher priority. Thanks, > > > > > > > > > > + > > > > + mutex_lock(&csi->lock); > > > > + > > > > + ret = mei_csi_send(csi, (u8 *)&cmd, cmd_size); > > > > + /* > > > > + * wait configuration ready if download success. placing > > > > + * delay under mutex is to make sure current command flow > > > > + * completed before starting a possible new one. > > > > + */ > > > > + if (!ret) > > > > + msleep(CSI_FW_READY_DELAY_MS); > > > > + > > > > + mutex_unlock(&csi->lock); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int mei_csi_pre_streamon(struct v4l2_subdev *sd, u32 flags) { > > > > + struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ, }; > > > > + struct mei_csi *csi = sd_to_csi(sd); > > > > + struct v4l2_ctrl *ctrl; > > > > + int ret = 0; > > > > > > No need to initialise this, it is always set. > > > > ack, thanks > > > > > > > > > + > > > > + if (!csi->remote) > > > > + return -ENODEV; > > > > + > > > > + ctrl = v4l2_ctrl_find(csi->remote->ctrl_handler, V4L2_CID_LINK_FREQ); > > > > + if (!ctrl) > > > > + return -EINVAL; > > > > + qm.index = v4l2_ctrl_g_ctrl(ctrl); > > > > + > > > > + ret = v4l2_querymenu(csi->remote->ctrl_handler, &qm); > > > > + if (ret) > > > > + return ret; > > > > > > Please use v4l2_get_link_freq() as already discussed. > > > > ack, thanks, > > > > > > > > Didn't we also discuss that you could do this in the s_stream() callback? > > > > We don't need configure CSI2 link if call s_stream with enable = 0, > > > > But I'm ok to get this in s_stream, thanks > > Yes, you should only query this if streaming is being enabled. Can we just query the link freq in mei_csi_notify_bound and record it? > > > > > > > > > > + > > > > + csi->link_freq = qm.value; > > > > + > > > > + return ret; > > > > +} > > > > + > > > > + > > > > +static const struct v4l2_async_notifier_operations mei_csi_notify_ops = { > > > > + .bound = mei_csi_notify_bound, > > > > + .unbind = mei_csi_notify_unbind, }; > > > > + > > > > +static int mei_csi_init_control(struct mei_csi *csi) { > > > > + v4l2_ctrl_handler_init(&csi->ctrl_handler, 1); > > > > + csi->ctrl_handler.lock = &csi->lock; > > > > + > > > > + csi->privacy_ctrl = v4l2_ctrl_new_std(&csi->ctrl_handler, NULL, > > > > + V4L2_CID_PRIVACY, 0, 1, 1, 0); > > > > + if (csi->ctrl_handler.error) > > > > + return csi->ctrl_handler.error; > > > > + csi->privacy_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > > > > > You should also add the LINK_FREQUENCY control. Make it U64 and and > > > VOLATILE, too. This way the driver's g_volatile_ctrl() gets called > > > when the control value is read. > > > > ack, thanks > > > > > > > > > + > > > > + csi->subdev.ctrl_handler = &csi->ctrl_handler; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mei_csi_parse_firmware(struct mei_csi *csi) { > > > > + struct v4l2_fwnode_endpoint v4l2_ep = { > > > > + .bus_type = V4L2_MBUS_CSI2_DPHY, > > > > + }; > > > > + struct device *dev = &csi->cldev->dev; > > > > + struct v4l2_async_subdev *asd; > > > > + struct fwnode_handle *fwnode; > > > > + struct fwnode_handle *ep; > > > > + int ret; > > > > + > > > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > > > > + if (!ep) { > > > > + dev_err(dev, "not connected to subdevice\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep); > > > > + if (ret) { > > > > + dev_err(dev, "could not parse v4l2 endpoint\n"); > > > > + fwnode_handle_put(ep); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + fwnode = fwnode_graph_get_remote_endpoint(ep); > > > > + fwnode_handle_put(ep); > > > > + > > > > + v4l2_async_nf_init(&csi->notifier); > > > > + csi->notifier.ops = &mei_csi_notify_ops; > > > > + > > > > + asd = v4l2_async_nf_add_fwnode(&csi->notifier, fwnode, > > > > + struct v4l2_async_subdev); > > > > + if (IS_ERR(asd)) { > > > > + fwnode_handle_put(fwnode); > > > > + return PTR_ERR(asd); > > > > + } > > > > + > > > > + ret = v4l2_fwnode_endpoint_alloc_parse(fwnode, &v4l2_ep); > > > > > > It seems you're parsing the remote endpoint properties here. This > > > shouldn't be needed for any reason. > > > > We need sensor's lane number to configure IVSC's CSI2 > > > > > > > > > + fwnode_handle_put(fwnode); > > > > + if (ret) > > > > + return ret; > > > > + csi->nr_of_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; > > > > > > Instead the number of lanes comes from the local endpoint. > > > > The lane number of IVSC follows sensor's lane number, why local endpoint? > > Because you shouldn't access other devices' endpoint properties in drivers. > They are outside of the scope of the device's bindings. Ok, in v3 patch set, it was trying to get the lane number as below: + ret = v4l2_subdev_call(csi->remote, pad, get_mbus_config, + csi->remote_pad, &mbus_config); + if (ret) + return ret; + + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) + return -EINVAL; + + csi->nr_of_lanes = mbus_config.bus.mipi_csi2.num_data_lanes; Can we use above subdev_call in mei_csi_notify_bound to get lane number? BR, Wentong > > > > > > > > > > + > > > > + ret = v4l2_async_subdev_nf_register(&csi->subdev, &csi->notifier); > > > > + if (ret) > > > > + v4l2_async_nf_cleanup(&csi->notifier); > > > > + > > > > + v4l2_fwnode_endpoint_free(&v4l2_ep); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int mei_csi_probe(struct mei_cl_device *cldev, > > > > + const struct mei_cl_device_id *id) { > > > > + struct device *dev = &cldev->dev; > > > > + struct mei_csi *csi; > > > > + int ret; > > > > + > > > > + if (!dev_fwnode(dev)) > > > > + return -EPROBE_DEFER; > > > > + > > > > + csi = devm_kzalloc(dev, sizeof(struct mei_csi), GFP_KERNEL); > > > > + if (!csi) > > > > + return -ENOMEM; > > > > + > > > > + csi->cldev = cldev; > > > > + mutex_init(&csi->lock); > > > > + init_completion(&csi->cmd_completion); > > > > + > > > > + mei_cldev_set_drvdata(cldev, csi); > > > > + > > > > + ret = mei_cldev_enable(cldev); > > > > + if (ret < 0) { > > > > + dev_err(dev, "mei_cldev_enable failed: %d\n", ret); > > > > + goto destroy_mutex; > > > > + } > > > > + > > > > + ret = mei_cldev_register_rx_cb(cldev, mei_csi_rx); > > > > + if (ret) { > > > > + dev_err(dev, "event cb registration failed: %d\n", ret); > > > > + goto err_disable; > > > > + } > > > > + > > > > + ret = mei_csi_parse_firmware(csi); > > > > + if (ret) > > > > + goto err_disable; > > > > + > > > > + csi->subdev.dev = &cldev->dev; > > > > + v4l2_subdev_init(&csi->subdev, &mei_csi_subdev_ops); > > > > + v4l2_set_subdevdata(&csi->subdev, csi); > > > > + csi->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE; > > > > > > Please add V4L2_SUBDEV_FL_HAS_EVENTS for control events. > > > > ack, thanks > > -- > Kind regards, > > Sakari Ailus
Hi Wentong, On Mon, Apr 24, 2023 at 07:09:34AM +0000, Wu, Wentong wrote: > > > > > + if (!csi->remote) > > > > > + return -ENODEV; > > > > > + > > > > > + ctrl = v4l2_ctrl_find(csi->remote->ctrl_handler, V4L2_CID_LINK_FREQ); > > > > > + if (!ctrl) > > > > > + return -EINVAL; > > > > > + qm.index = v4l2_ctrl_g_ctrl(ctrl); > > > > > + > > > > > + ret = v4l2_querymenu(csi->remote->ctrl_handler, &qm); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Please use v4l2_get_link_freq() as already discussed. > > > > > > ack, thanks, > > > > > > > > > > > Didn't we also discuss that you could do this in the s_stream() callback? > > > > > > We don't need configure CSI2 link if call s_stream with enable = 0, > > > > > > But I'm ok to get this in s_stream, thanks > > > > Yes, you should only query this if streaming is being enabled. > > Can we just query the link freq in mei_csi_notify_bound and record it? Yes. It can't change during streaming in any case. > > > > > +static int mei_csi_parse_firmware(struct mei_csi *csi) { > > > > > + struct v4l2_fwnode_endpoint v4l2_ep = { > > > > > + .bus_type = V4L2_MBUS_CSI2_DPHY, > > > > > + }; > > > > > + struct device *dev = &csi->cldev->dev; > > > > > + struct v4l2_async_subdev *asd; > > > > > + struct fwnode_handle *fwnode; > > > > > + struct fwnode_handle *ep; > > > > > + int ret; > > > > > + > > > > > + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0); > > > > > + if (!ep) { > > > > > + dev_err(dev, "not connected to subdevice\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + ret = v4l2_fwnode_endpoint_parse(ep, &v4l2_ep); > > > > > + if (ret) { > > > > > + dev_err(dev, "could not parse v4l2 endpoint\n"); > > > > > + fwnode_handle_put(ep); > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + fwnode = fwnode_graph_get_remote_endpoint(ep); > > > > > + fwnode_handle_put(ep); > > > > > + > > > > > + v4l2_async_nf_init(&csi->notifier); > > > > > + csi->notifier.ops = &mei_csi_notify_ops; > > > > > + > > > > > + asd = v4l2_async_nf_add_fwnode(&csi->notifier, fwnode, > > > > > + struct v4l2_async_subdev); > > > > > + if (IS_ERR(asd)) { > > > > > + fwnode_handle_put(fwnode); > > > > > + return PTR_ERR(asd); > > > > > + } > > > > > + > > > > > + ret = v4l2_fwnode_endpoint_alloc_parse(fwnode, &v4l2_ep); > > > > > > > > It seems you're parsing the remote endpoint properties here. This > > > > shouldn't be needed for any reason. > > > > > > We need sensor's lane number to configure IVSC's CSI2 > > > > > > > > > > > > + fwnode_handle_put(fwnode); > > > > > + if (ret) > > > > > + return ret; > > > > > + csi->nr_of_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes; > > > > > > > > Instead the number of lanes comes from the local endpoint. > > > > > > The lane number of IVSC follows sensor's lane number, why local endpoint? > > > > Because you shouldn't access other devices' endpoint properties in drivers. > > They are outside of the scope of the device's bindings. > > Ok, in v3 patch set, it was trying to get the lane number as below: > > + ret = v4l2_subdev_call(csi->remote, pad, get_mbus_config, > + csi->remote_pad, &mbus_config); > + if (ret) > + return ret; > + > + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) > + return -EINVAL; > + > + csi->nr_of_lanes = mbus_config.bus.mipi_csi2.num_data_lanes; > > Can we use above subdev_call in mei_csi_notify_bound to get lane number? My point is that you're not supposed to find this from an external device or its firmware node. This information should instead be present in the device's own firmware nodes. It's the IPU bridge's job to put it there. Please see Documentation/firmware-guide/acpi/dsd/graph.rst and Documentation/devicetree/bindings/media/video-interfaces.yaml .
Intel Visual Sensing Controller (IVSC), codenamed "Clover Falls", is a companion chip designed to provide secure and low power vision capability to IA platforms. IVSC is available in existing commercial platforms from multiple OEMs. The primary use case of IVSC is to bring in context awareness. IVSC interfaces directly with the platform main camera sensor via a CSI-2 link and processes the image data with the embedded AI engine. The detected events are sent over I2C to ISH (Intel Sensor Hub) for additional data fusion from multiple sensors. The fusion results are used to implement advanced use cases like: - Face detection to unlock screen - Detect user presence to manage backlight setting or waking up system Since the Image Processing Unit(IPU) used on the host processor needs to configure the CSI-2 link in normal camera usages, the CSI-2 link and camera sensor can only be used in mutually-exclusive ways by host IPU and IVSC. By default the IVSC owns the CSI-2 link and camera sensor. The IPU driver can take ownership of the CSI-2 link and camera sensor using the interfaces exported via v4l2 sub-device. Switching ownership requires an interface with two different hardware modules inside IVSC. The software interface to these modules is via Intel MEI (The Intel Management Engine) commands. These two hardware modules have two different MEI UUIDs to enumerate. These hardware modules are: - ACE (Algorithm Context Engine): This module is for algorithm computing when IVSC owns camera sensor. Also ACE module controls camera sensor's ownership. This hardware module is used to set ownership of camera sensor. - CSI (Camera Serial Interface): This module is used to route camera sensor data either to IVSC or to host for IPU driver and application. IVSC also provides a privacy mode. When privacy mode is turned on, camera sensor can't be used. This means that both ACE and host IPU can't get image data. And when this mode is turned on, users are informed via v4l2 control API. In summary, to acquire ownership of camera by IPU driver, first ACE module needs to be informed of ownership and then to setup MIPI CSI-2 link for the camera sensor and IPU. Implementation: There are two different drivers to handle ACE and CSI hardware modules inside IVSC. - ivsc_csi: MEI client driver to send commands and receive notifications from CSI module. - ivsc_ace: MEI client driver to send commands and get status from ACE module. Interface is exposed via v4l2 sub-devcie APIs to acquire and release camera sensor and CSI-2 link. Below diagram shows connections of IVSC/ISH/IPU/Camera sensor. ----------------------------------------------------------------------------- | Host Processor | | | | ----------------- ----------------- --------------- | | | | | | | | I2C | | | IPU | | ISH | |camera driver|--| | | | | | | | | | | | ----------------- ----------------- --------------- | | | | | | | | | | | --------------- | | | | | | | | | | | | | IVSC driver | | | | | | | | | | | | | --------------- | | | | | | | | ----------------|-----------------------|----------------------|---------|--- | CSI | I2C |SPI | | | | | ----------------|-----------------------|---------------- | | | IVSC | | | | | | | | | | ----------------- ----------------- | | | | | | | | | | | | | CSI | | ACE | |------| | | | | | | | | | ----------------- ----------------- | | | | | I2C | | ----------------|-----------------------|---------------- | | CSI | | | | | -------------------------------- | | | I2C | | camera sensor |-----------------------------| | | -------------------------------- --- v5: - probe mei_csi only after software node has been setup v4: - call v4l2_ctrl_handler_free() if setting up the handler failed - set V4L2_CTRL_FLAG_READ_ONLY for privacy_ctrl - add dev_warn if failed to switch CSI-2 link to IVSC - use v4l2_fwnode_endpoint_alloc_parse to get num_data_lanes - add document about how sensor connected to IVSC is powered - move lock to mei_ace_send - check return value for device_link_add Wentong Wu (3): media: pci: intel: ivsc: Add CSI submodule media: pci: intel: ivsc: Add ACE submodule ACPI: delay enumeration of devices with a _DEP pointing to IVSC device drivers/acpi/scan.c | 2 + drivers/media/pci/Kconfig | 1 + drivers/media/pci/intel/Makefile | 2 + drivers/media/pci/intel/ivsc/Kconfig | 12 + drivers/media/pci/intel/ivsc/Makefile | 9 + drivers/media/pci/intel/ivsc/mei_ace.c | 560 ++++++++++++++++++++++++ drivers/media/pci/intel/ivsc/mei_csi.c | 772 +++++++++++++++++++++++++++++++++ 7 files changed, 1358 insertions(+) create mode 100644 drivers/media/pci/intel/ivsc/Kconfig create mode 100644 drivers/media/pci/intel/ivsc/Makefile create mode 100644 drivers/media/pci/intel/ivsc/mei_ace.c create mode 100644 drivers/media/pci/intel/ivsc/mei_csi.c