[Intel-gfx] [PATCH 0/4] drm/i915: Add support for MTL GSC SW Proxy

[Intel-gfx] [PATCH 0/4] drm/i915: Add support for MTL GSC SW Proxy

[Daniele Ceraolo Spurio]

On platforms where the GSC is part of GT, it needs to communicate with
CSME for some of its operations. However, there is no direct HW
communication channel, so the i915 and mei drivers must carry the
messages back and forth between the 2 units. The protocol is fully
described in the i915 patch that adds the initial support, but it
basically amounts to SW blindly moving messages back and forth until the
GSC tells us to stop.

Implementing this features requires a new mei component to handle
the mei side of things. The patches for this have already been
reviewed on the char-misc ML and we already have an ack from Greg to
merge them via the drm tree [1].

[1] https://lore.kernel.org/lkml/20230208142358.1401618-1-tomas.winkler@intel.com/t/

Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Cc: Alexander Usyskin <alexander.usyskin@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Alexander Usyskin (2):
  drm/i915/mtl: Define GSC Proxy component interface
  mei: gsc_proxy: add gsc proxy driver

Daniele Ceraolo Spurio (2):
  drm/i915/gsc: add initial support for GSC proxy
  drm/i915/gsc: add support for GSC proxy interrupt

 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/gt/intel_gt_irq.c        |  22 +-
 drivers/gpu/drm/i915/gt/intel_gt_regs.h       |   3 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c  | 424 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h  |  18 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c     |  66 ++-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h     |  17 +-
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
 drivers/misc/mei/Kconfig                      |   2 +-
 drivers/misc/mei/Makefile                     |   1 +
 drivers/misc/mei/gsc_proxy/Kconfig            |  14 +
 drivers/misc/mei/gsc_proxy/Makefile           |   7 +
 drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c    | 208 +++++++++
 include/drm/i915_component.h                  |   3 +-
 include/drm/i915_gsc_proxy_mei_interface.h    |  36 ++
 15 files changed, 813 insertions(+), 10 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
 create mode 100644 drivers/misc/mei/gsc_proxy/Kconfig
 create mode 100644 drivers/misc/mei/gsc_proxy/Makefile
 create mode 100644 drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
 create mode 100644 include/drm/i915_gsc_proxy_mei_interface.h

-- 
2.37.3

[Intel-gfx] [PATCH 1/4] drm/i915/mtl: Define GSC Proxy component interface

[Daniele Ceraolo Spurio]

From: Alexander Usyskin <alexander.usyskin@intel.com>

GSC Proxy component is used for communication between the
Intel graphics driver and MEI driver.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 include/drm/i915_component.h               |  3 +-
 include/drm/i915_gsc_proxy_mei_interface.h | 36 ++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 include/drm/i915_gsc_proxy_mei_interface.h

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c1e2a43d2d1e..56a84ee1c64c 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -29,7 +29,8 @@
 enum i915_component_type {
 	I915_COMPONENT_AUDIO = 1,
 	I915_COMPONENT_HDCP,
-	I915_COMPONENT_PXP
+	I915_COMPONENT_PXP,
+	I915_COMPONENT_GSC_PROXY,
 };
 
 /* MAX_PORT is the number of port
diff --git a/include/drm/i915_gsc_proxy_mei_interface.h b/include/drm/i915_gsc_proxy_mei_interface.h
new file mode 100644
index 000000000000..e817bb316d5c
--- /dev/null
+++ b/include/drm/i915_gsc_proxy_mei_interface.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright (c) 2022-2023 Intel Corporation
+ */
+
+#ifndef _I915_GSC_PROXY_MEI_INTERFACE_H_
+#define _I915_GSC_PROXY_MEI_INTERFACE_H_
+
+#include <linux/mutex.h>
+#include <linux/device.h>
+
+/**
+ * struct i915_gsc_proxy_component_ops - ops for GSC Proxy services.
+ * @owner: Module providing the ops
+ * @send: sends data through GSC proxy
+ * @recv: receives data through GSC proxy
+ */
+struct i915_gsc_proxy_component_ops {
+	struct module *owner;
+
+	int (*send)(struct device *dev, const void *buf, size_t size);
+	int (*recv)(struct device *dev, void *buf, size_t size);
+};
+
+/**
+ * struct i915_gsc_proxy_component - Used for communication between i915 and
+ * MEI drivers for GSC proxy services
+ * @mei_dev: device that provide the GSC proxy service.
+ * @ops: Ops implemented by GSC proxy driver, used by i915 driver.
+ */
+struct i915_gsc_proxy_component {
+	struct device *mei_dev;
+	const struct i915_gsc_proxy_component_ops *ops;
+};
+
+#endif /* _I915_GSC_PROXY_MEI_INTERFACE_H_ */
-- 
2.37.3

[Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

[Daniele Ceraolo Spurio]

From: Alexander Usyskin <alexander.usyskin@intel.com>

Add GSC proxy driver. It to allows messaging between GSC component
on Intel on board graphics card and CSE device.

Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/misc/mei/Kconfig                   |   2 +-
 drivers/misc/mei/Makefile                  |   1 +
 drivers/misc/mei/gsc_proxy/Kconfig         |  14 ++
 drivers/misc/mei/gsc_proxy/Makefile        |   7 +
 drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c | 208 +++++++++++++++++++++
 5 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 drivers/misc/mei/gsc_proxy/Kconfig
 create mode 100644 drivers/misc/mei/gsc_proxy/Makefile
 create mode 100644 drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c

diff --git a/drivers/misc/mei/Kconfig b/drivers/misc/mei/Kconfig
index d21486d69df2..37db142de413 100644
--- a/drivers/misc/mei/Kconfig
+++ b/drivers/misc/mei/Kconfig
@@ -62,4 +62,4 @@ config INTEL_MEI_GSC
 
 source "drivers/misc/mei/hdcp/Kconfig"
 source "drivers/misc/mei/pxp/Kconfig"
-
+source "drivers/misc/mei/gsc_proxy/Kconfig"
diff --git a/drivers/misc/mei/Makefile b/drivers/misc/mei/Makefile
index fb740d754900..14aee253ae48 100644
--- a/drivers/misc/mei/Makefile
+++ b/drivers/misc/mei/Makefile
@@ -30,3 +30,4 @@ CFLAGS_mei-trace.o = -I$(src)
 
 obj-$(CONFIG_INTEL_MEI_HDCP) += hdcp/
 obj-$(CONFIG_INTEL_MEI_PXP) += pxp/
+obj-$(CONFIG_INTEL_MEI_GSC_PROXY) += gsc_proxy/
diff --git a/drivers/misc/mei/gsc_proxy/Kconfig b/drivers/misc/mei/gsc_proxy/Kconfig
new file mode 100644
index 000000000000..fd45ce8c1df4
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022-2023, Intel Corporation. All rights reserved.
+#
+config INTEL_MEI_GSC_PROXY
+	tristate "Intel GSC Proxy services of ME Interface"
+	select INTEL_MEI_ME
+	depends on DRM_I915
+	help
+         MEI Support for GSC Proxy Services on Intel platforms.
+
+         MEI GSC proxy enables messaging between GSC service on
+         Intel graphics on-board card and services on CSE (MEI)
+         firmware residing SoC or PCH.
+
diff --git a/drivers/misc/mei/gsc_proxy/Makefile b/drivers/misc/mei/gsc_proxy/Makefile
new file mode 100644
index 000000000000..358847e9aaa9
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Copyright (c) 2022-2023, Intel Corporation. All rights reserved.
+#
+# Makefile - GSC Proxy client driver for Intel MEI Bus Driver.
+
+obj-$(CONFIG_INTEL_MEI_GSC_PROXY) += mei_gsc_proxy.o
diff --git a/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
new file mode 100644
index 000000000000..953eda1a16fb
--- /dev/null
+++ b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2022-2023 Intel Corporation
+ */
+
+/**
+ * DOC: MEI_GSC_PROXY Client Driver
+ *
+ * The mei_gsc_proxy driver acts as a translation layer between
+ * proxy user (I915) and ME FW by proxying messages to ME FW
+ */
+
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/uuid.h>
+#include <linux/mei_cl_bus.h>
+#include <linux/component.h>
+#include <drm/drm_connector.h>
+#include <drm/i915_component.h>
+#include <drm/i915_gsc_proxy_mei_interface.h>
+
+/**
+ * mei_gsc_proxy_send - Sends a proxy message to ME FW.
+ * @dev: device corresponding to the mei_cl_device
+ * @buf: a message buffer to send
+ * @size: size of the message
+ * Return: bytes sent on Success, <0 on Failure
+ */
+static int mei_gsc_proxy_send(struct device *dev, const void *buf, size_t size)
+{
+	ssize_t ret;
+
+	if (!dev || !buf)
+		return -EINVAL;
+
+	ret = mei_cldev_send(to_mei_cl_device(dev), buf, size);
+	if (ret < 0)
+		dev_dbg(dev, "mei_cldev_send failed. %zd\n", ret);
+
+	return ret;
+}
+
+/**
+ * mei_gsc_proxy_recv - Receives a proxy message from ME FW.
+ * @dev: device corresponding to the mei_cl_device
+ * @buf: a message buffer to contain the received message
+ * @size: size of the buffer
+ * Return: bytes received on Success, <0 on Failure
+ */
+static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
+{
+	ssize_t ret;
+
+	if (!dev || !buf)
+		return -EINVAL;
+
+	ret = mei_cldev_recv(to_mei_cl_device(dev), buf, size);
+	if (ret < 0)
+		dev_dbg(dev, "mei_cldev_recv failed. %zd\n", ret);
+
+	return ret;
+}
+
+static const struct i915_gsc_proxy_component_ops mei_gsc_proxy_ops = {
+	.owner = THIS_MODULE,
+	.send = mei_gsc_proxy_send,
+	.recv = mei_gsc_proxy_recv,
+};
+
+static int mei_component_master_bind(struct device *dev)
+{
+	struct mei_cl_device *cldev = to_mei_cl_device(dev);
+	struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+
+	comp_master->ops = &mei_gsc_proxy_ops;
+	comp_master->mei_dev = dev;
+	return component_bind_all(dev, comp_master);
+}
+
+static void mei_component_master_unbind(struct device *dev)
+{
+	struct mei_cl_device *cldev = to_mei_cl_device(dev);
+	struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+
+	component_unbind_all(dev, comp_master);
+}
+
+static const struct component_master_ops mei_component_master_ops = {
+	.bind = mei_component_master_bind,
+	.unbind = mei_component_master_unbind,
+};
+
+/**
+ * mei_gsc_proxy_component_match - compare function for matching mei.
+ *
+ *    The function checks if the device is pci device and
+ *    Intel VGA adapter, the subcomponent is SW Proxy
+ *    and the parent of MEI PCI and the parent of VGA are the same PCH device.
+ *
+ * @dev: master device
+ * @subcomponent: subcomponent to match (I915_COMPONENT_SWPROXY)
+ * @data: compare data (mei pci parent)
+ *
+ * Return:
+ * * 1 - if components match
+ * * 0 - otherwise
+ */
+static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent,
+					 void *data)
+{
+	struct pci_dev *pdev;
+
+	if (!dev_is_pci(dev))
+		return 0;
+
+	pdev = to_pci_dev(dev);
+
+	if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) ||
+	    pdev->vendor != PCI_VENDOR_ID_INTEL)
+		return 0;
+
+	if (subcomponent != I915_COMPONENT_GSC_PROXY)
+		return 0;
+
+	return component_compare_dev(dev->parent, ((struct device *)data)->parent);
+}
+
+static int mei_gsc_proxy_probe(struct mei_cl_device *cldev,
+			       const struct mei_cl_device_id *id)
+{
+	struct i915_gsc_proxy_component *comp_master;
+	struct component_match *master_match = NULL;
+	int ret;
+
+	ret = mei_cldev_enable(cldev);
+	if (ret < 0) {
+		dev_err(&cldev->dev, "mei_cldev_enable Failed. %d\n", ret);
+		goto enable_err_exit;
+	}
+
+	comp_master = kzalloc(sizeof(*comp_master), GFP_KERNEL);
+	if (!comp_master) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	component_match_add_typed(&cldev->dev, &master_match,
+				  mei_gsc_proxy_component_match, cldev->dev.parent);
+	if (IS_ERR_OR_NULL(master_match)) {
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	mei_cldev_set_drvdata(cldev, comp_master);
+	ret = component_master_add_with_match(&cldev->dev,
+					      &mei_component_master_ops,
+					      master_match);
+	if (ret < 0) {
+		dev_err(&cldev->dev, "Master comp add failed %d\n", ret);
+		goto err_exit;
+	}
+
+	return 0;
+
+err_exit:
+	mei_cldev_set_drvdata(cldev, NULL);
+	kfree(comp_master);
+	mei_cldev_disable(cldev);
+enable_err_exit:
+	return ret;
+}
+
+static void mei_gsc_proxy_remove(struct mei_cl_device *cldev)
+{
+	struct i915_gsc_proxy_component *comp_master = mei_cldev_get_drvdata(cldev);
+	int ret;
+
+	component_master_del(&cldev->dev, &mei_component_master_ops);
+	kfree(comp_master);
+	mei_cldev_set_drvdata(cldev, NULL);
+
+	ret = mei_cldev_disable(cldev);
+	if (ret)
+		dev_warn(&cldev->dev, "mei_cldev_disable() failed %d\n", ret);
+}
+
+#define MEI_UUID_GSC_PROXY UUID_LE(0xf73db04, 0x97ab, 0x4125, \
+				   0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)
+
+static struct mei_cl_device_id mei_gsc_proxy_tbl[] = {
+	{ .uuid = MEI_UUID_GSC_PROXY, .version = MEI_CL_VERSION_ANY },
+	{ }
+};
+MODULE_DEVICE_TABLE(mei, mei_gsc_proxy_tbl);
+
+static struct mei_cl_driver mei_gsc_proxy_driver = {
+	.id_table = mei_gsc_proxy_tbl,
+	.name = KBUILD_MODNAME,
+	.probe = mei_gsc_proxy_probe,
+	.remove	= mei_gsc_proxy_remove,
+};
+
+module_mei_cl_driver(mei_gsc_proxy_driver);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MEI GSC PROXY");
-- 
2.37.3

[Intel-gfx] [PATCH 3/4] drm/i915/gsc: add initial support for GSC proxy

[Daniele Ceraolo Spurio]

The GSC uC needs to communicate with the CSME to perform certain
operations. Since the GSC can't perform this communication directly
on platforms where it is integrated in GT, i915 needs to transfer the
messages from GSC to CSME and back.
The proxy flow is as follow:
1 - i915 submits a request to GSC asking for the message to CSME
2 - GSC replies with the proxy header + payload for CSME
3 - i915 sends the reply from GSC as-is to CSME via the mei proxy
    component
4 - CSME replies with the proxy header + payload for GSC
5 - i915 submits a request to GSC with the reply from CSME
6 - GSC replies either with a new header + payload (same as step 2,
    so we restart from there) or with an end message.

After GSC load, i915 is expected to start the first proxy message chain,
while all subsequent ones will be triggered by the GSC via interrupt.

To communicate with the CSME, we use a dedicated mei component, which
means that we need to wait for it to bind before we can initialize the
proxies. This usually happens quite fast, but given that there is a
chance that we'll have to wait a few seconds the GSC work has been moved
to a dedicated WQ to not stall other processes.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c  | 384 ++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h  |  17 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c     |  40 +-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h     |  14 +-
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
 6 files changed, 452 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 057ef22fa9c6..aae041137f93 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -194,6 +194,7 @@ i915-y += \
 # general-purpose microcontroller (GuC) support
 i915-y += \
 	  gt/uc/intel_gsc_fw.o \
+	  gt/uc/intel_gsc_proxy.o \
 	  gt/uc/intel_gsc_uc.o \
 	  gt/uc/intel_gsc_uc_heci_cmd_submit.o\
 	  gt/uc/intel_guc.o \
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
new file mode 100644
index 000000000000..ed8f68e78c26
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
@@ -0,0 +1,384 @@
+#include "intel_gsc_proxy.h"
+
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <linux/component.h>
+#include "drm/i915_gsc_proxy_mei_interface.h"
+#include "drm/i915_component.h"
+
+#include "gt/intel_gt.h"
+#include "gt/intel_gt_print.h"
+#include "intel_gsc_uc.h"
+#include "intel_gsc_uc_heci_cmd_submit.h"
+#include "i915_drv.h"
+
+/*
+ * GSC proxy:
+ * The GSC uC needs to communicate with the CSME to perform certain operations.
+ * Since the GSC can't perform this communication directly on platforms where it
+ * is integrated in GT, i915 needs to transfer the messages from GSC to CSME
+ * and back. i915 must manually start the proxy flow after the GSC is loaded to
+ * signal to GSC that we're ready to handle its messages and allow it to query
+ * its init data from CSME; GSC will then trigger an HECI2 interrupt if it needs
+ * to send messages to CSME again.
+ * The proxy flow is as follow:
+ * 1 - i915 submits a request to GSC asking for the message to CSME
+ * 2 - GSC replies with the proxy header + payload for CSME
+ * 3 - i915 sends the reply from GSC as-is to CSME via the mei proxy component
+ * 4 - CSME replies with the proxy header + payload for GSC
+ * 5 - i915 submits a request to GSC with the reply from CSME
+ * 6 - GSC replies either with a new header + payload (same as step 2, so we
+ *     restart from there) or with an end message.
+ */
+
+/*
+ * The component should load quite quickly in most cases, but it could take
+ * a bit. Using a very big timeout just to cover the worst case scenario
+ */
+#define GSC_PROXY_INIT_TIMEOUT_MS 20000
+
+/* the protocol supports up to 32K in each direction */
+#define GSC_PROXY_BUFFER_SIZE SZ_32K
+#define GSC_PROXY_CHANNEL_SIZE (GSC_PROXY_BUFFER_SIZE * 2)
+#define GSC_PROXY_MAX_MSG_SIZE (GSC_PROXY_BUFFER_SIZE - sizeof(struct intel_gsc_mtl_header))
+
+/* FW-defined proxy header */
+struct intel_gsc_proxy_header
+{
+	/*
+	 * hdr:
+	 * Bits 0-7: type of the proxy message (see enum intel_gsc_proxy_type)
+	 * Bits 8-15: rsvd
+	 * Bits 16-31: length in bytes of the payload following the proxy header
+	 */
+	u32 hdr;
+#define GSC_PROXY_TYPE		 GENMASK(7, 0)
+#define GSC_PROXY_PAYLOAD_LENGTH GENMASK(31, 16)
+
+	u32 source;		/* Source of the Proxy message */
+	u32 destination;	/* Destination of the Proxy message */
+#define GSC_PROXY_ADDRESSING_KMD  0x10000
+#define GSC_PROXY_ADDRESSING_GSC  0x20000
+#define GSC_PROXY_ADDRESSING_CSME 0x30000
+
+	u32 status;		/* Command status */
+} __packed;
+
+/* FW-defined proxy types */
+enum intel_gsc_proxy_type {
+	GSC_PROXY_MSG_TYPE_PROXY_INVALID = 0,
+	GSC_PROXY_MSG_TYPE_PROXY_QUERY = 1,
+	GSC_PROXY_MSG_TYPE_PROXY_PAYLOAD = 2,
+	GSC_PROXY_MSG_TYPE_PROXY_END = 3,
+	GSC_PROXY_MSG_TYPE_PROXY_NOTIFICATION = 4,
+};
+
+struct gsc_proxy_msg
+{
+	struct intel_gsc_mtl_header header;
+	struct intel_gsc_proxy_header proxy_header;
+} __packed;
+
+static int proxy_send_to_csme(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct i915_gsc_proxy_component *comp = gsc->proxy.component;
+	struct intel_gsc_mtl_header *hdr;
+	void *in = gsc->proxy.to_csme;
+	void *out = gsc->proxy.to_gsc;
+	u32 in_size;
+	int ret;
+
+	/* CSME msg only includes the proxy */
+	hdr = in;
+	in += sizeof(struct intel_gsc_mtl_header);
+	out += sizeof(struct intel_gsc_mtl_header);
+
+	in_size = hdr->message_size - sizeof(struct intel_gsc_mtl_header);
+
+	/* the message must contain at least the proxy header */
+	if (in_size < sizeof(struct intel_gsc_proxy_header) ||
+	    in_size > GSC_PROXY_MAX_MSG_SIZE) {
+		gt_err(gt, "Invalid CSME message size: %u\n", in_size);
+		return -EINVAL;
+	}
+
+	ret = comp->ops->send(comp->mei_dev, in, in_size);
+	if (ret < 0) {
+		gt_err(gt, "Failed to send CSME message\n");
+		return ret;
+	}
+
+	ret = comp->ops->recv(comp->mei_dev, out, GSC_PROXY_MAX_MSG_SIZE);
+	if (ret < 0) {
+		gt_err(gt, "Failed to receive CSME message\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int submit_gsc_proxy_request(struct intel_gsc_uc *gsc, u32 size)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	u32 *marker = gsc->proxy.to_csme; /* first dw of the reply header */
+	u64 addr_in = i915_ggtt_offset(gsc->proxy.vma);
+	u64 addr_out = addr_in + GSC_PROXY_BUFFER_SIZE;
+	int err;
+
+	/* the message must contain at least the gsc and proxy headers */
+	if (size < sizeof(struct gsc_proxy_msg) || size > GSC_PROXY_BUFFER_SIZE) {
+		gt_err(gt, "Invalid GSC proxy message size: %u\n", size);
+		return -EINVAL;
+	}
+
+	/* clear the message marker */
+	*marker = 0;
+	wmb();
+
+	/* send the request */
+	err = intel_gsc_uc_heci_cmd_submit_packet(gsc, addr_in, size,
+						  addr_out, GSC_PROXY_BUFFER_SIZE);
+
+	if (!err) {
+		/* wait for the reply to show up */
+		err = wait_for(*marker != 0, 300);
+		if (err)
+			gt_err(gt, "Failed to get a proxy reply from gsc\n");
+	}
+
+	return err;
+}
+
+static int validate_proxy_header(struct intel_gsc_proxy_header *header,
+				 u32 source, u32 dest)
+{
+	u32 type = FIELD_GET(GSC_PROXY_TYPE, header->hdr);
+	u32 length = FIELD_GET(GSC_PROXY_PAYLOAD_LENGTH, header->hdr);
+	int ret = 0;
+
+	if (header->destination != dest || header->source != source) {
+		ret = -ENOEXEC;
+		goto fail;
+	}
+
+	switch (type) {
+	case GSC_PROXY_MSG_TYPE_PROXY_PAYLOAD:
+		if (length > 0)
+			break;
+		fallthrough;
+	case GSC_PROXY_MSG_TYPE_PROXY_INVALID:
+		ret = -EIO;
+		goto fail;
+	default:
+		break;
+	}
+
+fail:
+	return ret;
+
+}
+
+static int proxy_query(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct gsc_proxy_msg *to_gsc = gsc->proxy.to_gsc;
+	struct gsc_proxy_msg *to_csme = gsc->proxy.to_csme;
+	int ret;
+
+	intel_gsc_uc_heci_cmd_emit_mtl_header(&to_gsc->header,
+					      HECI_MEADDRESS_PROXY,
+					      sizeof(struct gsc_proxy_msg),
+					      0);
+
+	to_gsc->proxy_header.hdr =
+		FIELD_PREP(GSC_PROXY_TYPE, GSC_PROXY_MSG_TYPE_PROXY_QUERY) |
+		FIELD_PREP(GSC_PROXY_PAYLOAD_LENGTH, 0);
+
+	to_gsc->proxy_header.source = GSC_PROXY_ADDRESSING_KMD;
+	to_gsc->proxy_header.destination = GSC_PROXY_ADDRESSING_GSC;
+	to_gsc->proxy_header.status = 0;
+
+	while (1) {
+		/* clear the GSC response header space */
+		memset(gsc->proxy.to_csme, 0, sizeof(struct gsc_proxy_msg));
+
+		/* send proxy message to GSC */
+		ret = submit_gsc_proxy_request(gsc, to_gsc->header.message_size);
+		if (ret) {
+			gt_err(gt, "failed to send proxy message to GSC! %d\n", ret);
+			goto proxy_error;
+		}
+
+		/* stop if this was the last message */
+		if (FIELD_GET(GSC_PROXY_TYPE, to_csme->proxy_header.hdr) ==
+				GSC_PROXY_MSG_TYPE_PROXY_END)
+			break;
+
+		/* make sure the GSC-to-CSME proxy header is sane */
+		ret = validate_proxy_header(&to_csme->proxy_header,
+					    GSC_PROXY_ADDRESSING_GSC,
+					    GSC_PROXY_ADDRESSING_CSME);
+		if (ret) {
+			gt_err(gt, "invalid GSC to CSME proxy header! %d\n", ret);
+			goto proxy_error;
+		}
+
+		/* send the GSC message to the CSME */
+		ret = proxy_send_to_csme(gsc);
+		if (ret < 0) {
+			gt_err(gt, "failed to send proxy message to CSME! %d\n", ret);
+			goto proxy_error;
+		}
+
+		/* update the GSC message size with the returned value from CSME */
+		to_gsc->header.message_size = ret + sizeof(struct intel_gsc_mtl_header);
+
+		/* make sure the CSME-to-GSC proxy header is sane */
+		ret = validate_proxy_header(&to_gsc->proxy_header,
+					    GSC_PROXY_ADDRESSING_CSME,
+					    GSC_PROXY_ADDRESSING_GSC);
+		if (ret) {
+			gt_err(gt, "invalid CSME to GSC proxy header! %d\n", ret);
+			goto proxy_error;
+		}
+	}
+
+proxy_error:
+	return ret < 0 ? ret : 0;
+}
+
+int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	int err;
+
+	if (!gsc->proxy.component_added)
+		return -ENODEV;
+
+	assert_rpm_wakelock_held(gt->uncore->rpm);
+
+	/* when GSC is loaded, we can queue this before the component is bound */
+	err = wait_for(gsc->proxy.component, GSC_PROXY_INIT_TIMEOUT_MS);
+	if (err) {
+		gt_err(gt, "GSC proxy component didn't bind within the expected timeout\n");
+		return -EIO;
+	}
+
+	mutex_lock(&gsc->proxy.mutex);
+	if (!gsc->proxy.component) {
+		gt_err(gt, "GSC proxy worker called without the component being bound!\n");
+		err = -EIO;
+	} else {
+		err = proxy_query(gsc);
+	}
+	mutex_unlock(&gsc->proxy.mutex);
+	return err;
+}
+
+static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
+					 struct device *tee_kdev, void *data)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
+	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
+
+	mutex_lock(&gsc->proxy.mutex);
+	gsc->proxy.component = data;
+	gsc->proxy.component->mei_dev = tee_kdev;
+	mutex_unlock(&gsc->proxy.mutex);
+
+	return 0;
+}
+
+static void i915_gsc_proxy_component_unbind(struct device *i915_kdev,
+					    struct device *tee_kdev, void *data)
+{
+	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
+	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
+
+	mutex_lock(&gsc->proxy.mutex);
+	gsc->proxy.component = NULL;
+	mutex_unlock(&gsc->proxy.mutex);
+}
+
+static const struct component_ops i915_gsc_proxy_component_ops = {
+	.bind   = i915_gsc_proxy_component_bind,
+	.unbind = i915_gsc_proxy_component_unbind,
+};
+
+static int proxy_channel_alloc(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct i915_vma *vma;
+	void *vaddr;
+	int err;
+
+	err = intel_guc_allocate_and_map_vma(&gt->uc.guc, GSC_PROXY_CHANNEL_SIZE,
+					     &vma, &vaddr);
+	if (err)
+		return err;
+
+	gsc->proxy.vma = vma;
+	gsc->proxy.to_gsc = vaddr;
+	gsc->proxy.to_csme = vaddr + GSC_PROXY_BUFFER_SIZE;
+
+	return 0;
+}
+
+static void proxy_channel_free(struct intel_gsc_uc *gsc)
+{
+	if (!gsc->proxy.vma)
+		return;
+
+	gsc->proxy.to_gsc = NULL;
+	gsc->proxy.to_csme = NULL;
+	i915_vma_unpin_and_release(&gsc->proxy.vma, I915_VMA_RELEASE_MAP);
+}
+
+void intel_gsc_proxy_fini(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+
+	if (fetch_and_zero(&gsc->proxy.component_added))
+		component_del(i915->drm.dev, &i915_gsc_proxy_component_ops);
+
+	proxy_channel_free(gsc);
+}
+
+int intel_gsc_proxy_init(struct intel_gsc_uc *gsc)
+{
+	int err;
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+
+	mutex_init(&gsc->proxy.mutex);
+
+	if (!IS_ENABLED(CONFIG_INTEL_MEI_GSC_PROXY)) {
+		gt_info(gt, "can't init GSC proxy due to missing mei component\n");
+		return -ENODEV;
+	}
+
+	err = proxy_channel_alloc(gsc);
+	if (err)
+		return err;
+
+	err = component_add_typed(i915->drm.dev, &i915_gsc_proxy_component_ops,
+				  I915_COMPONENT_GSC_PROXY);
+	if (err < 0) {
+		gt_err(gt, "Failed to add GSC_PROXY component (%d)\n", err);
+		goto out_free;
+	}
+
+	gsc->proxy.component_added = true;
+
+	return 0;
+
+out_free:
+	proxy_channel_free(gsc);
+	return err;
+}
+
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
new file mode 100644
index 000000000000..da3e9dd5d820
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_PROXY_H_
+#define _INTEL_GSC_PROXY_H_
+
+#include <linux/types.h>
+
+struct intel_gsc_uc;
+
+int intel_gsc_proxy_init(struct intel_gsc_uc *gsc);
+void intel_gsc_proxy_fini(struct intel_gsc_uc *gsc);
+int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc);
+
+#endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 2d5b70b3384c..b505b208f04b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -10,15 +10,30 @@
 #include "intel_gsc_uc.h"
 #include "intel_gsc_fw.h"
 #include "i915_drv.h"
+#include "intel_gsc_proxy.h"
 
 static void gsc_work(struct work_struct *work)
 {
 	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
 	struct intel_gt *gt = gsc_uc_to_gt(gsc);
 	intel_wakeref_t wakeref;
+	int ret;
 
-	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
-		intel_gsc_uc_fw_upload(gsc);
+	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
+
+	ret = intel_gsc_uc_fw_upload(gsc);
+	if (ret)
+		goto out_put;
+
+	ret = intel_gsc_proxy_request_handler(gsc);
+	if (ret)
+		goto out_put;
+
+	gt_dbg(gt, "GSC Proxy initialized\n");
+	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
+
+out_put:
+	intel_runtime_pm_put(gt->uncore->rpm, wakeref);
 }
 
 static bool gsc_engine_supported(struct intel_gt *gt)
@@ -43,6 +58,8 @@ static bool gsc_engine_supported(struct intel_gt *gt)
 
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 {
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+
 	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC);
 	INIT_WORK(&gsc->work, gsc_work);
 
@@ -50,10 +67,16 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 	 * GT with it being not fully setup hence check device info's
 	 * engine mask
 	 */
-	if (!gsc_engine_supported(gsc_uc_to_gt(gsc))) {
+	if (!gsc_engine_supported(gt)) {
 		intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_NOT_SUPPORTED);
 		return;
 	}
+
+	gsc->wq = alloc_ordered_workqueue("i915_gsc", 0);
+	if (!gsc->wq) {
+		gt_err(gt, "failed to allocate WQ for GSC, disabling FW\n");
+		intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_NOT_SUPPORTED);
+	}
 }
 
 int intel_gsc_uc_init(struct intel_gsc_uc *gsc)
@@ -88,6 +111,9 @@ int intel_gsc_uc_init(struct intel_gsc_uc *gsc)
 
 	gsc->ce = ce;
 
+	/* if we fail to init proxy we still want to load GSC for PM */
+	intel_gsc_proxy_init(gsc);
+
 	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOADABLE);
 
 	return 0;
@@ -107,6 +133,12 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
 		return;
 
 	flush_work(&gsc->work);
+	if (gsc->wq) {
+		destroy_workqueue(gsc->wq);
+		gsc->wq = NULL;
+	}
+
+	intel_gsc_proxy_fini(gsc);
 
 	if (gsc->ce)
 		intel_engine_destroy_pinned_context(fetch_and_zero(&gsc->ce));
@@ -151,5 +183,5 @@ void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc)
 	if (intel_gsc_uc_fw_init_done(gsc))
 		return;
 
-	queue_work(system_unbound_wq, &gsc->work);
+	queue_work(gsc->wq, &gsc->work);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
index 5f50fa1ff8b9..023bded10dde 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -10,6 +10,7 @@
 
 struct i915_vma;
 struct intel_context;
+struct i915_gsc_proxy_component;
 
 struct intel_gsc_uc {
 	/* Generic uC firmware management */
@@ -19,7 +20,18 @@ struct intel_gsc_uc {
 	struct i915_vma *local; /* private memory for GSC usage */
 	struct intel_context *ce; /* for submission to GSC FW via GSC engine */
 
-	struct work_struct work; /* for delayed load */
+	/* for delayed load and proxy handling */
+	struct workqueue_struct *wq;
+	struct work_struct work;
+
+	struct {
+		struct i915_gsc_proxy_component *component;
+		bool component_added;
+		struct i915_vma *vma;
+		void *to_gsc;
+		void *to_csme;
+		struct mutex mutex; /* protects the tee channel binding */
+	} proxy;
 };
 
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
index 3d56ae501991..8f199d5f963e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -14,6 +14,7 @@ struct intel_gsc_mtl_header {
 #define GSC_HECI_VALIDITY_MARKER 0xA578875A
 
 	u8 heci_client_id;
+#define HECI_MEADDRESS_PROXY 10
 #define HECI_MEADDRESS_PXP 17
 #define HECI_MEADDRESS_HDCP 18
 
-- 
2.37.3

[Intel-gfx] [PATCH 4/4] drm/i915/gsc: add support for GSC proxy interrupt

[Daniele Ceraolo Spurio]

The GSC notifies us of a proxy request via the HECI2 interrupt. The
interrupt must be enabled both in the HECI layer and in our usual gt irq
programming; for the latter, the interrupt is enabled via the same enable
register as the GSC CS, but it does have its own mask register. When the
interrupt is received, we also need to de-assert it in both layers.

The handling of the proxy request is deferred to the same worker that we
use for GSC load. New flags have been added to distinguish between the
init case and the proxy interrupt.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_irq.c       | 22 ++++++++--
 drivers/gpu/drm/i915/gt/intel_gt_regs.h      |  3 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c | 44 +++++++++++++++++++-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c    | 44 ++++++++++++++++----
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h    |  3 ++
 6 files changed, 103 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index 1b25a6039152..c433ffdb3380 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -15,6 +15,7 @@
 #include "intel_uncore.h"
 #include "intel_rps.h"
 #include "pxp/intel_pxp_irq.h"
+#include "uc/intel_gsc_proxy.h"
 
 static void guc_irq_handler(struct intel_guc *guc, u16 iir)
 {
@@ -81,6 +82,9 @@ gen11_other_irq_handler(struct intel_gt *gt, const u8 instance,
 	if (instance == OTHER_GSC_INSTANCE)
 		return intel_gsc_irq_handler(gt, iir);
 
+	if (instance == OTHER_GSC_HECI_2_INSTANCE)
+		return intel_gsc_proxy_irq_handler(&gt->uc.gsc, iir);
+
 	WARN_ONCE(1, "unhandled other interrupt instance=0x%x, iir=0x%x\n",
 		  instance, iir);
 }
@@ -100,6 +104,8 @@ static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance)
 	case VIDEO_ENHANCEMENT_CLASS:
 		return media_gt;
 	case OTHER_CLASS:
+		if (instance == OTHER_GSC_HECI_2_INSTANCE)
+			return media_gt;
 		if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0))
 			return media_gt;
 		fallthrough;
@@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
 	u32 irqs = GT_RENDER_USER_INTERRUPT;
 	u32 guc_mask = intel_uc_wants_guc(&gt->uc) ? GUC_INTR_GUC2HOST : 0;
 	u32 gsc_mask = 0;
+	u32 heci_mask = 0;
 	u32 dmask;
 	u32 smask;
 
@@ -267,10 +274,16 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
 	dmask = irqs << 16 | irqs;
 	smask = irqs << 16;
 
-	if (HAS_ENGINE(gt, GSC0))
+	if (HAS_ENGINE(gt, GSC0)) {
+		/*
+		 * the heci2 interrupt is enabled via the same register as the
+		 * GSC interrupt, but it has its own mask register.
+		 */
 		gsc_mask = irqs;
-	else if (HAS_HECI_GSC(gt->i915))
+		heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
+	} else if (HAS_HECI_GSC(gt->i915)) {
 		gsc_mask = GSC_IRQ_INTF(0) | GSC_IRQ_INTF(1);
+	}
 
 	BUILD_BUG_ON(irqs & 0xffff0000);
 
@@ -280,7 +293,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
 	if (CCS_MASK(gt))
 		intel_uncore_write(uncore, GEN12_CCS_RSVD_INTR_ENABLE, smask);
 	if (gsc_mask)
-		intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask);
+		intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_ENABLE, gsc_mask | heci_mask);
 
 	/* Unmask irqs on RCS, BCS, VCS and VECS engines. */
 	intel_uncore_write(uncore, GEN11_RCS0_RSVD_INTR_MASK, ~smask);
@@ -308,6 +321,9 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
 		intel_uncore_write(uncore, GEN12_CCS2_CCS3_INTR_MASK, ~dmask);
 	if (gsc_mask)
 		intel_uncore_write(uncore, GEN11_GUNIT_CSME_INTR_MASK, ~gsc_mask);
+	if (heci_mask)
+		intel_uncore_write(uncore, GEN11_HECI2_RSVD_INTR_MASK,
+				   ~REG_FIELD_PREP(ENGINE1_MASK, heci_mask));
 
 	if (guc_mask) {
 		/* the enable bit is common for both GTs but the masks are separate */
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
index 4aecb5a7b631..da11ce5ca99e 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1577,6 +1577,7 @@
 
 #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
 #define   GEN11_CSME				(31)
+#define   GEN12_HECI_2				(30)
 #define   GEN11_GUNIT				(28)
 #define   GEN11_GUC				(25)
 #define   MTL_MGUC				(24)
@@ -1618,6 +1619,7 @@
 /* irq instances for OTHER_CLASS */
 #define   OTHER_GUC_INSTANCE			0
 #define   OTHER_GTPM_INSTANCE			1
+#define   OTHER_GSC_HECI_2_INSTANCE		3
 #define   OTHER_KCR_INSTANCE			4
 #define   OTHER_GSC_INSTANCE			6
 #define   OTHER_MEDIA_GUC_INSTANCE		16
@@ -1633,6 +1635,7 @@
 #define GEN12_VCS6_VCS7_INTR_MASK		_MMIO(0x1900b4)
 #define GEN11_VECS0_VECS1_INTR_MASK		_MMIO(0x1900d0)
 #define GEN12_VECS2_VECS3_INTR_MASK		_MMIO(0x1900d4)
+#define GEN11_HECI2_RSVD_INTR_MASK		_MMIO(0x1900e4)
 #define GEN11_GUC_SG_INTR_MASK			_MMIO(0x1900e8)
 #define MTL_GUC_MGUC_INTR_MASK			_MMIO(0x1900e8) /* MTL+ */
 #define GEN11_GPM_WGBOXPERF_INTR_MASK		_MMIO(0x1900ec)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
index ed8f68e78c26..2889e0d39ff3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
@@ -14,6 +14,7 @@
 #include "intel_gsc_uc.h"
 #include "intel_gsc_uc_heci_cmd_submit.h"
 #include "i915_drv.h"
+#include "i915_reg.h"
 
 /*
  * GSC proxy:
@@ -273,17 +274,49 @@ int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc)
 		gt_err(gt, "GSC proxy worker called without the component being bound!\n");
 		err = -EIO;
 	} else {
+		/*
+		 * write the status bit to clear it and allow new proxy
+		 * interrupts to be generated while we handle the current
+		 * request, but be sure not to write the reset bit
+		 */
+		intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
+				 HECI_H_CSR_RST, HECI_H_CSR_IS);
 		err = proxy_query(gsc);
 	}
 	mutex_unlock(&gsc->proxy.mutex);
 	return err;
 }
 
+void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+
+	if (unlikely(!iir))
+		return;
+
+	lockdep_assert_held(gt->irq_lock);
+
+	if (!gsc->proxy.component) {
+		gt_err(gt, "GSC proxy irq received without the component being bound!\n");
+		return;
+	}
+
+	gsc->gsc_work_actions |= GSC_ACTION_SW_PROXY;
+	queue_work(gsc->wq, &gsc->work);
+}
+
 static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
 					 struct device *tee_kdev, void *data)
 {
 	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
-	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
+	struct intel_gt *gt = i915->media_gt;
+	struct intel_gsc_uc *gsc = &gt->uc.gsc;
+	intel_wakeref_t wakeref;
+
+	/* enable HECI2 IRQs */
+	with_intel_runtime_pm(&i915->runtime_pm, wakeref)
+		intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
+				 0, HECI_H_CSR_IE);
 
 	mutex_lock(&gsc->proxy.mutex);
 	gsc->proxy.component = data;
@@ -297,11 +330,18 @@ static void i915_gsc_proxy_component_unbind(struct device *i915_kdev,
 					    struct device *tee_kdev, void *data)
 {
 	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
-	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
+	struct intel_gt *gt = i915->media_gt;
+	struct intel_gsc_uc *gsc = &gt->uc.gsc;
+	intel_wakeref_t wakeref;
 
 	mutex_lock(&gsc->proxy.mutex);
 	gsc->proxy.component = NULL;
 	mutex_unlock(&gsc->proxy.mutex);
+
+	/* disable HECI2 IRQs */
+	with_intel_runtime_pm(&i915->runtime_pm, wakeref)
+		intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
+				 HECI_H_CSR_IE, 0);
 }
 
 static const struct component_ops i915_gsc_proxy_component_ops = {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
index da3e9dd5d820..c55bafcbaec4 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
@@ -13,5 +13,6 @@ struct intel_gsc_uc;
 int intel_gsc_proxy_init(struct intel_gsc_uc *gsc);
 void intel_gsc_proxy_fini(struct intel_gsc_uc *gsc);
 int intel_gsc_proxy_request_handler(struct intel_gsc_uc *gsc);
+void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir);
 
 #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index b505b208f04b..64bff01026e8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -17,20 +17,40 @@ static void gsc_work(struct work_struct *work)
 	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
 	struct intel_gt *gt = gsc_uc_to_gt(gsc);
 	intel_wakeref_t wakeref;
+	u32 actions;
 	int ret;
 
 	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
 
-	ret = intel_gsc_uc_fw_upload(gsc);
-	if (ret)
-		goto out_put;
-
-	ret = intel_gsc_proxy_request_handler(gsc);
-	if (ret)
-		goto out_put;
+	spin_lock_irq(gt->irq_lock);
+	actions = gsc->gsc_work_actions;
+	gsc->gsc_work_actions = 0;
+	spin_unlock_irq(gt->irq_lock);
+
+	if (actions & GSC_ACTION_FW_LOAD) {
+		ret = intel_gsc_uc_fw_upload(gsc);
+		if (ret == -EEXIST) /* skip proxy if not a new load */
+			actions &= ~GSC_ACTION_FW_LOAD;
+		else if (ret)
+			goto out_put;
+	}
 
-	gt_dbg(gt, "GSC Proxy initialized\n");
-	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
+	if (actions & (GSC_ACTION_FW_LOAD | GSC_ACTION_SW_PROXY)) {
+		if (!intel_gsc_uc_fw_init_done(gsc)) {
+			gt_err(gt, "Proxy request received with GSC not loaded!\n");
+			goto out_put;
+		}
+
+		ret = intel_gsc_proxy_request_handler(gsc);
+		if (ret)
+			goto out_put;
+
+		/* mark the GSC FW init as done the first time we run this */
+		if (actions & GSC_ACTION_FW_LOAD) {
+			gt_dbg(gt, "GSC Proxy initialized\n");
+			intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_RUNNING);
+		}
+	}
 
 out_put:
 	intel_runtime_pm_put(gt->uncore->rpm, wakeref);
@@ -177,11 +197,17 @@ void intel_gsc_uc_resume(struct intel_gsc_uc *gsc)
 
 void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc)
 {
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+
 	if (!intel_uc_fw_is_loadable(&gsc->fw))
 		return;
 
 	if (intel_gsc_uc_fw_init_done(gsc))
 		return;
 
+	spin_lock_irq(gt->irq_lock);
+	gsc->gsc_work_actions |= GSC_ACTION_FW_LOAD;
+	spin_unlock_irq(gt->irq_lock);
+
 	queue_work(gsc->wq, &gsc->work);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
index 023bded10dde..a2a0813b8a76 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -23,6 +23,9 @@ struct intel_gsc_uc {
 	/* for delayed load and proxy handling */
 	struct workqueue_struct *wq;
 	struct work_struct work;
+	u32 gsc_work_actions; /* protected by gt->irq_lock */
+#define GSC_ACTION_FW_LOAD BIT(0)
+#define GSC_ACTION_SW_PROXY BIT(1)
 
 	struct {
 		struct i915_gsc_proxy_component *component;
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for MTL GSC SW Proxy

[Patchwork]

== Series Details ==

Series: drm/i915: Add support for MTL GSC SW Proxy
URL   : https://patchwork.freedesktop.org/series/115806/
State : warning

== Summary ==

Error: dim checkpatch failed
cd5858341f03 drm/i915/mtl: Define GSC Proxy component interface
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:30: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#30: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 45 lines checked
9b6e1a1d6cf0 mei: gsc_proxy: add gsc proxy driver
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:35: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#35: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 238 lines checked
2429872a6959 drm/i915/gsc: add initial support for GSC proxy
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:45: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#45: 
new file mode 100644

-:50: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1
#50: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:1:
+#include "intel_gsc_proxy.h"

-:52: WARNING:SPDX_LICENSE_TAG: Misplaced SPDX-License-Identifier tag - use line 1 instead
#52: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:3:
+// SPDX-License-Identifier: MIT

-:99: ERROR:OPEN_BRACE: open brace '{' following struct go on the same line
#99: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:50:
+struct intel_gsc_proxy_header
+{

-:129: ERROR:OPEN_BRACE: open brace '{' following struct go on the same line
#129: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:80:
+struct gsc_proxy_msg
+{

-:189: WARNING:MEMORY_BARRIER: memory barrier without comment
#189: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:140:
+	wmb();

-:232: CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#232: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c:183:
+
+}

total: 2 errors, 4 warnings, 1 checks, 525 lines checked
c90211076ba2 drm/i915/gsc: add support for GSC proxy interrupt

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Add support for MTL GSC SW Proxy

[Patchwork]

== Series Details ==

Series: drm/i915: Add support for MTL GSC SW Proxy
URL   : https://patchwork.freedesktop.org/series/115806/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Add support for MTL GSC SW Proxy

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6547 bytes --]

== Series Details ==

Series: drm/i915: Add support for MTL GSC SW Proxy
URL   : https://patchwork.freedesktop.org/series/115806/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12936 -> Patchwork_115806v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/index.html

Participating hosts (37 -> 35)
------------------------------

  Missing    (2): fi-kbl-soraka fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_115806v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][1] -> [DMESG-FAIL][2] ([i915#5334])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         NOTRUN -> [DMESG-FAIL][3] ([i915#6367])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rpls-1/igt@i915_selftest@live@slpc.html
    - bat-adlp-9:         [PASS][4] -> [DMESG-FAIL][5] ([i915#6367])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-adlp-9/igt@i915_selftest@live@slpc.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-adlp-9/igt@i915_selftest@live@slpc.html

  * igt@i915_selftest@live@workarounds:
    - bat-dg2-11:         [PASS][6] -> [INCOMPLETE][7] ([i915#7913])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-dg2-11/igt@i915_selftest@live@workarounds.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-dg2-11/igt@i915_selftest@live@workarounds.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - fi-bsw-nick:        NOTRUN -> [SKIP][8] ([fdo#109271]) +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/fi-bsw-nick/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
    - bat-rpls-1:         NOTRUN -> [SKIP][9] ([i915#7828])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][10] ([i915#5354])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-adlp-9:         NOTRUN -> [SKIP][11] ([i915#3546]) +1 similar issue
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-rpls-1:         NOTRUN -> [SKIP][12] ([i915#1845])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         [ABORT][13] ([i915#6687] / [i915#7978]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [ABORT][15] ([i915#7911] / [i915#7913]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@mman:
    - bat-rpls-1:         [TIMEOUT][17] ([i915#6794]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-rpls-1/igt@i915_selftest@live@mman.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rpls-1/igt@i915_selftest@live@mman.html

  * igt@i915_selftest@live@slpc:
    - bat-rplp-1:         [DMESG-FAIL][19] ([i915#6367] / [i915#7913]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-rplp-1/igt@i915_selftest@live@slpc.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-rplp-1/igt@i915_selftest@live@slpc.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1:
    - bat-dg2-8:          [FAIL][21] ([i915#7932]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978


Build changes
-------------

  * Linux: CI_DRM_12936 -> Patchwork_115806v1

  CI-20190529: 20190529
  CI_DRM_12936: 906438caae695f109636f82e2d1845a258f57d8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7226: 41be8b4ab86f9e11388c10366dfd71e5032589c1 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115806v1: 906438caae695f109636f82e2d1845a258f57d8b @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

76c27cac191f drm/i915/gsc: add support for GSC proxy interrupt
8b3e9c4bf11d drm/i915/gsc: add initial support for GSC proxy
8367552d7b07 mei: gsc_proxy: add gsc proxy driver
5d7483278cc9 drm/i915/mtl: Define GSC Proxy component interface

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/index.html

[-- Attachment #2: Type: text/html, Size: 7770 bytes --]

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Add support for MTL GSC SW Proxy

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 14205 bytes --]

== Series Details ==

Series: drm/i915: Add support for MTL GSC SW Proxy
URL   : https://patchwork.freedesktop.org/series/115806/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12936_full -> Patchwork_115806v1_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_115806v1_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_115806v1_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_115806v1_full:

### IGT changes ###

#### Possible regressions ####

  * igt@kms_flip@2x-flip-vs-dpms-off-vs-modeset-interruptible:
    - shard-glk:          NOTRUN -> [TIMEOUT][1] +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk9/igt@kms_flip@2x-flip-vs-dpms-off-vs-modeset-interruptible.html

  
#### Warnings ####

  * igt@kms_big_fb@linear-16bpp-rotate-90:
    - shard-glk:          [SKIP][2] ([fdo#109271]) -> [TIMEOUT][3] +3 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk5/igt@kms_big_fb@linear-16bpp-rotate-90.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk9/igt@kms_big_fb@linear-16bpp-rotate-90.html

  * igt@kms_ccs@pipe-b-crc-primary-rotation-180-y_tiled_gen12_mc_ccs:
    - shard-glk:          [SKIP][4] ([fdo#109271] / [i915#3886]) -> [TIMEOUT][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk5/igt@kms_ccs@pipe-b-crc-primary-rotation-180-y_tiled_gen12_mc_ccs.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk9/igt@kms_ccs@pipe-b-crc-primary-rotation-180-y_tiled_gen12_mc_ccs.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@perf@enable-disable@0-rcs0}:
    - shard-glk:          [PASS][6] -> [TIMEOUT][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk5/igt@perf@enable-disable@0-rcs0.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk9/igt@perf@enable-disable@0-rcs0.html

  
Known issues
------------

  Here are the changes found in Patchwork_115806v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs:
    - shard-apl:          NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#3886])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl6/igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a1:
    - shard-glk:          [PASS][9] -> [FAIL][10] ([i915#79])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk6/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk9/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a1.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-mmap-gtt:
    - shard-apl:          NOTRUN -> [SKIP][11] ([fdo#109271]) +13 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl4/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-pri-indfb-draw-mmap-gtt.html

  * igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area:
    - shard-apl:          NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#658])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl6/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area.html

  
#### Possible fixes ####

  * {igt@gem_barrier_race@remote-request@rcs0}:
    - shard-apl:          [ABORT][13] ([i915#8211] / [i915#8234]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-apl7/igt@gem_barrier_race@remote-request@rcs0.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl4/igt@gem_barrier_race@remote-request@rcs0.html
    - {shard-tglu}:       [ABORT][15] ([i915#8211]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-tglu-2/igt@gem_barrier_race@remote-request@rcs0.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-tglu-5/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_exec_endless@dispatch@vecs0:
    - {shard-tglu}:       [TIMEOUT][17] ([i915#3778]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-tglu-10/igt@gem_exec_endless@dispatch@vecs0.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-tglu-3/igt@gem_exec_endless@dispatch@vecs0.html

  * igt@gem_exec_fair@basic-deadline:
    - shard-apl:          [FAIL][19] ([i915#2846]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-apl1/igt@gem_exec_fair@basic-deadline.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl7/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][21] ([i915#2842]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-apl7/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl1/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [FAIL][23] ([i915#2842]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk3/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk8/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@i915_pm_dc@dc6-dpms:
    - {shard-tglu}:       [FAIL][25] ([i915#3989] / [i915#454]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-tglu-8/igt@i915_pm_dc@dc6-dpms.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-tglu-2/igt@i915_pm_dc@dc6-dpms.html

  * igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1:
    - shard-apl:          [ABORT][27] ([i915#180]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-apl1/igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-apl6/igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1.html

  * {igt@kms_cursor_edge_walk@256x256-top-edge@pipe-a-hdmi-a-1}:
    - shard-glk:          [DMESG-FAIL][29] ([i915#118]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk7/igt@kms_cursor_edge_walk@256x256-top-edge@pipe-a-hdmi-a-1.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk7/igt@kms_cursor_edge_walk@256x256-top-edge@pipe-a-hdmi-a-1.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-glk:          [FAIL][31] ([i915#2346]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * {igt@kms_draw_crc@draw-method-blt@xrgb2101010-ytiled}:
    - shard-glk:          [DMESG-WARN][33] ([i915#7936]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-glk7/igt@kms_draw_crc@draw-method-blt@xrgb2101010-ytiled.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-glk3/igt@kms_draw_crc@draw-method-blt@xrgb2101010-ytiled.html

  * igt@kms_fbcon_fbt@fbc-suspend:
    - {shard-tglu}:       [FAIL][35] ([i915#4767]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12936/shard-tglu-10/igt@kms_fbcon_fbt@fbc-suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/shard-tglu-3/igt@kms_fbcon_fbt@fbc-suspend.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3778]: https://gitlab.freedesktop.org/drm/intel/issues/3778
  [i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
  [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7936]: https://gitlab.freedesktop.org/drm/intel/issues/7936
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8234]: https://gitlab.freedesktop.org/drm/intel/issues/8234
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292


Build changes
-------------

  * Linux: CI_DRM_12936 -> Patchwork_115806v1

  CI-20190529: 20190529
  CI_DRM_12936: 906438caae695f109636f82e2d1845a258f57d8b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7226: 41be8b4ab86f9e11388c10366dfd71e5032589c1 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115806v1: 906438caae695f109636f82e2d1845a258f57d8b @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115806v1/index.html

[-- Attachment #2: Type: text/html, Size: 11781 bytes --]

Re: [Intel-gfx] [PATCH 1/4] drm/i915/mtl: Define GSC Proxy component interface

[Teres Alexis, Alan Previn]

On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> GSC Proxy component is used for communication between the
> Intel graphics driver and MEI driver.



> diff --git a/include/drm/i915_gsc_proxy_mei_interface.h b/include/drm/i915_gsc_proxy_mei_interface.h
> new file mode 100644
> index 000000000000..e817bb316d5c
> --- /dev/null
> +++ b/include/drm/i915_gsc_proxy_mei_interface.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (c) 2022-2023 Intel Corporation
> + */
> +
> +#ifndef _I915_GSC_PROXY_MEI_INTERFACE_H_
> +#define _I915_GSC_PROXY_MEI_INTERFACE_H_
> +
> +#include <linux/mutex.h>

alan: i notice u have included mutex.h but don't have any mutex use in this header.
side note: looking at at least one other component interfaces (pxp), I see a mutex in the
component struct but don't see it being used at all - a topic for a different series.


> +#include <linux/device.h>
alan: any reason we should include "device.h"? as opposed to just define the ptr type
(since w only reference the ptrs). this could save us a little on build time

> +
> +/**
> + * struct i915_gsc_proxy_component_ops - ops for GSC Proxy services.
> + * @owner: Module providing the ops
> + * @send: sends data through GSC proxy
> + * @recv: receives data through GSC proxy
alan: nit: to be more specific "... from i915 through GSC proxy"

> + */
> +struct i915_gsc_proxy_component_ops {
> +	struct module *owner;
> +
> +	int (*send)(struct device *dev, const void *buf, size_t size);
> +	int (*recv)(struct device *dev, void *buf, size_t size);
> +};
alan: i believe we should have proper documentation on the possible list of
return values for the various error conditions (assuming 0 is success).

> +
> +/**
> + * struct i915_gsc_proxy_component - Used for communication between i915 and
> + * MEI drivers for GSC proxy services
> + * @mei_dev: device that provide the GSC proxy service.
> + * @ops: Ops implemented by GSC proxy driver, used by i915 driver.
> + */
> +struct i915_gsc_proxy_component {
> +	struct device *mei_dev;
> +	const struct i915_gsc_proxy_component_ops *ops;
> +};
> +
> +#endif /* _I915_GSC_PROXY_MEI_INTERFACE_H_ */

Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

[Teres Alexis, Alan Previn]

On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add GSC proxy driver. It to allows messaging between GSC component
> on Intel on board graphics card and CSE device.
alan:nit: isn't "Intel integrated GPU" clearer than "Intel on-board graphics card"?
Same thing for the Kconfig description later (or am i missing something else here).

alan:snip



> +         MEI GSC proxy enables messaging between GSC service on
> +         Intel graphics on-board card and services on CSE (MEI)
alan:nit: same as above




> diff --git a/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
> new file mode 100644
> index 000000000000..953eda1a16fb
> --- /dev/null
> +++ b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2022-2023 Intel Corporation
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/uuid.h>
> +#include <linux/mei_cl_bus.h>
alan: [nit?] i thought we need to have the headers alphabetically ordered? (below too)
> +#include <linux/component.h>
> +#include <drm/drm_connector.h>
> +#include <drm/i915_component.h>
> +#include <drm/i915_gsc_proxy_mei_interface.h>

> +
> +/**
> + * mei_gsc_proxy_send - Sends a proxy message to ME FW.
> + * @dev: device corresponding to the mei_cl_device
> + * @buf: a message buffer to send
> + * @size: size of the message
> + * Return: bytes sent on Success, <0 on Failure
> + */
> +static int mei_gsc_proxy_send(struct device *dev, const void *buf, size_t size)
> +{
> +	ssize_t ret;
> +
> +	if (!dev || !buf)
alan: nit: not sure if we should be checking for !size here - i do see that next patch
is checking for this from i915 side of the interface... but just wasnt sure which is the prefered style
(in terms of where to check for this condition). Either way, its a nit for me since i traced down
all the way to mei_cl_alloc_cb and it looks like mei bus can tolerate zero sized messages.
> +		return -EINVAL;
alan:snip

> +static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
> +{
> +	ssize_t ret;
> +
> +	if (!dev || !buf)
alan: nit: same as in the 'send' above,.. not sure if we should be checking for !size here...
or perhaps 0 sized recv is supported.

> +		return -EINVAL;
alan:snip

> +static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent,
> +					 void *data)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!dev_is_pci(dev))
> +		return 0;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) ||
> +	    pdev->vendor != PCI_VENDOR_ID_INTEL)
> +		return 0;
> +
> +	if (subcomponent != I915_COMPONENT_GSC_PROXY)
> +		return 0;
> +
> +	return component_compare_dev(dev->parent, ((struct device *)data)->parent);
alan: do we care if both these parents are non-null? i notice in other mei component
drivers match functions we do check that.

> +}
> +
alan:snip

> +#define MEI_UUID_GSC_PROXY UUID_LE(0xf73db04, 0x97ab, 0x4125, \
> +				   0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)

alan: apologies for the newbie question, but why are we using UUID for the gsc_proxy
as opposed to GUID like the other mei components? i am not sure if i read the right
archived patch review but it sounded like GUID is for internal to kernel only whereas
UUID is for external too? (in which case why are we not using GUID for gsc-proxy?)
Consider this a non-blocking inquiry since i assume mei folks have reviewed this
prior and this is an explicit design decision that I'm just not versed on.

alan:snip

Re: [Intel-gfx] [PATCH 3/4] drm/i915/gsc: add initial support for GSC proxy

[Teres Alexis, Alan Previn]

I have a number of comments but most are personal preferences and so i labelled them nits.
I did catch a few minor coding styling issues and am assuming those need to be enforced as per i915/kernel rules?
That said, since they are so minor (or maybe they are not strict), I'm providing a conditional RB to fix those 4 issues
(i.e. the header inclusion alphabetical ordering and struct '{' bracket position)

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> The GSC uC needs to communicate with the CSME to perform certain
> operations. Since the GSC can't perform this communication directly
> on platforms where it is integrated in GT, i915 needs to transfer the
> messages from GSC to CSME and back.
> The proxy flow is as follow:
> 1 - i915 submits a request to GSC asking for the message to CSME
> 2 - GSC replies with the proxy header + payload for CSME
> 3 - i915 sends the reply from GSC as-is to CSME via the mei proxy
>     component
> 4 - CSME replies with the proxy header + payload for GSC
> 5 - i915 submits a request to GSC with the reply from CSME
> 6 - GSC replies either with a new header + payload (same as step 2,
>     so we restart from there) or with an end message.
> 
> After GSC load, i915 is expected to start the first proxy message chain,
> while all subsequent ones will be triggered by the GSC via interrupt.
> 
> To communicate with the CSME, we use a dedicated mei component, which
> means that we need to wait for it to bind before we can initialize the
> proxies. This usually happens quite fast, but given that there is a
> chance that we'll have to wait a few seconds the GSC work has been moved
> to a dedicated WQ to not stall other processes.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c  | 384 ++++++++++++++++++
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h  |  17 +
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c     |  40 +-
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h     |  14 +-
>  .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
>  6 files changed, 452 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
>  create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.h
> 
alan:snip

> new file mode 100644
> index 000000000000..ed8f68e78c26
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_proxy.c
> @@ -0,0 +1,384 @@
> +#include "intel_gsc_proxy.h"
> +
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
alan: nit: 2022 - 2023?

> + */
> +
> +#include <linux/component.h>
> +#include "drm/i915_gsc_proxy_mei_interface.h"
alan: alphabetical
> +#include "drm/i915_component.h"
alan: snip

> +/*
> + * GSC proxy:
> + * The GSC uC needs to communicate with the CSME to perform certain operations.
> + * Since the GSC can't perform this communication directly on platforms where it
> + * is integrated in GT, i915 needs to transfer the messages from GSC to CSME
> + * and back. i915 must manually start the proxy flow after the GSC is loaded to
> + * signal to GSC that we're ready to handle its messages and allow it to query
> + * its init data from CSME; GSC will then trigger an HECI2 interrupt if it needs
> + * to send messages to CSME again.
> + * The proxy flow is as follow:
> + * 1 - i915 submits a request to GSC asking for the message to CSME
> + * 2 - GSC replies with the proxy header + payload for CSME
> + * 3 - i915 sends the reply from GSC as-is to CSME via the mei proxy component
> + * 4 - CSME replies with the proxy header + payload for GSC
> + * 5 - i915 submits a request to GSC with the reply from CSME
> + * 6 - GSC replies either with a new header + payload (same as step 2, so we
> + *     restart from there) or with an end message.
> + */
> +
> +/*
> + * The component should load quite quickly in most cases, but it could take
> + * a bit. Using a very big timeout just to cover the worst case scenario
> + */
> +#define GSC_PROXY_INIT_TIMEOUT_MS 20000
> +
> +/* the protocol supports up to 32K in each direction */
> +#define GSC_PROXY_BUFFER_SIZE SZ_32K
> +#define GSC_PROXY_CHANNEL_SIZE (GSC_PROXY_BUFFER_SIZE * 2)
> +#define GSC_PROXY_MAX_MSG_SIZE (GSC_PROXY_BUFFER_SIZE - sizeof(struct intel_gsc_mtl_header))
> +
> +/* FW-defined proxy header */
> +struct intel_gsc_proxy_header
> +{
alan: i thought we typically put the '{' on the same line as the struct name 
> 
alan:snip

> +struct gsc_proxy_msg
> +{
alan: shouldnt the '{' be above?
> +	struct intel_gsc_mtl_header header;
> +	struct intel_gsc_proxy_header proxy_header;
> +} __packed;
> +
> +static int proxy_send_to_csme(struct intel_gsc_uc *gsc)
> +{
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +	struct i915_gsc_proxy_component *comp = gsc->proxy.component;
> +	struct intel_gsc_mtl_header *hdr;
> +	void *in = gsc->proxy.to_csme;
> +	void *out = gsc->proxy.to_gsc;
> +	u32 in_size;
> +	int ret;
> +
> +	/* CSME msg only includes the proxy */
> +	hdr = in;
> +	in += sizeof(struct intel_gsc_mtl_header);
> +	out += sizeof(struct intel_gsc_mtl_header);
> +
> +	in_size = hdr->message_size - sizeof(struct intel_gsc_mtl_header);
> +
> +	/* the message must contain at least the proxy header */
> +	if (in_size < sizeof(struct intel_gsc_proxy_header) ||
> +	    in_size > GSC_PROXY_MAX_MSG_SIZE) {
> +		gt_err(gt, "Invalid CSME message size: %u\n", in_size);
> +		return -EINVAL;
> +	}
> +
> +	ret = comp->ops->send(comp->mei_dev, in, in_size);
alan: probably not something to do as part of this series but a future improvement series would be to
have a version of this ops->send with a timeout period as we've seen how these interfaces can
hang in rare corner cases (if we encounter a bug in the system). Since such a change would
be more intrusive and take more time, will leave that for a future follow up improvement.

> +	if (ret < 0) {
> +		gt_err(gt, "Failed to send CSME message\n");
> +		return ret;
> +	}
> +
> +	ret = comp->ops->recv(comp->mei_dev, out, GSC_PROXY_MAX_MSG_SIZE);
alan: same as above, a future-series follow up discussion, i believe, is that we need a version
of this interface with a timeout.
> +	if (ret < 0) {
> +		gt_err(gt, "Failed to receive CSME message\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int submit_gsc_proxy_request(struct intel_gsc_uc *gsc, u32 size)
alan: nit: the earlier half of the proxy operation function was called "proxy_send_to_csme"
wonder if, for no other reason that 'infered-duality', should this function be called "proxy_send_to_gsc"?
(or perhaps that should be called "proxy_comm_to_cse" and this called "proxy_comm_to_gsc".
Either way, since this is all internal to this one src file, treat as a nit.
> +{
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +	u32 *marker = gsc->proxy.to_csme; /* first dw of the reply header */
> +	u64 addr_in = i915_ggtt_offset(gsc->proxy.vma);
> +	u64 addr_out = addr_in + GSC_PROXY_BUFFER_SIZE;
> +	int err;
alan:snip






> +static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
> +					 struct device *tee_kdev, void *data)
alan: nit: do we still call this "tee_kdev" for the case of gsc_proxy? maybe should be "mei_gsc_proxy_kdev"?

alan:snip

> +static void i915_gsc_proxy_component_unbind(struct device *i915_kdev,
> +					    struct device *tee_kdev, void *data)
alan: nit: same as last one on "tee_kdev".

alan:snip


> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 2d5b70b3384c..b505b208f04b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -10,15 +10,30 @@
>  #include "intel_gsc_uc.h"
>  #include "intel_gsc_fw.h"
>  #include "i915_drv.h"
> +#include "intel_gsc_proxy.h"
alan:alphabetical 
alan:snip

Re: [Intel-gfx] [PATCH 1/4] drm/i915/mtl: Define GSC Proxy component interface

[Jani Nikula]

On Tue, 18 Apr 2023, "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com> wrote:
> On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>> 
>> GSC Proxy component is used for communication between the
>> Intel graphics driver and MEI driver.
>
>
>
>> diff --git a/include/drm/i915_gsc_proxy_mei_interface.h b/include/drm/i915_gsc_proxy_mei_interface.h
>> new file mode 100644
>> index 000000000000..e817bb316d5c
>> --- /dev/null
>> +++ b/include/drm/i915_gsc_proxy_mei_interface.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright (c) 2022-2023 Intel Corporation
>> + */
>> +
>> +#ifndef _I915_GSC_PROXY_MEI_INTERFACE_H_
>> +#define _I915_GSC_PROXY_MEI_INTERFACE_H_
>> +
>> +#include <linux/mutex.h>
>
> alan: i notice u have included mutex.h but don't have any mutex use in this header.
> side note: looking at at least one other component interfaces (pxp), I see a mutex in the
> component struct but don't see it being used at all - a topic for a different series.
>
>
>> +#include <linux/device.h>
> alan: any reason we should include "device.h"? as opposed to just define the ptr type
> (since w only reference the ptrs). this could save us a little on build time

The only thing required is <linux/types.h>. Everything else can be
forward declared.

BR,
Jani.

>
>> +
>> +/**
>> + * struct i915_gsc_proxy_component_ops - ops for GSC Proxy services.
>> + * @owner: Module providing the ops
>> + * @send: sends data through GSC proxy
>> + * @recv: receives data through GSC proxy
> alan: nit: to be more specific "... from i915 through GSC proxy"
>
>> + */
>> +struct i915_gsc_proxy_component_ops {
>> +	struct module *owner;
>> +
>> +	int (*send)(struct device *dev, const void *buf, size_t size);
>> +	int (*recv)(struct device *dev, void *buf, size_t size);
>> +};
> alan: i believe we should have proper documentation on the possible list of
> return values for the various error conditions (assuming 0 is success).
>
>> +
>> +/**
>> + * struct i915_gsc_proxy_component - Used for communication between i915 and
>> + * MEI drivers for GSC proxy services
>> + * @mei_dev: device that provide the GSC proxy service.
>> + * @ops: Ops implemented by GSC proxy driver, used by i915 driver.
>> + */
>> +struct i915_gsc_proxy_component {
>> +	struct device *mei_dev;
>> +	const struct i915_gsc_proxy_component_ops *ops;
>> +};
>> +
>> +#endif /* _I915_GSC_PROXY_MEI_INTERFACE_H_ */
>

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 4/4] drm/i915/gsc: add support for GSC proxy interrupt

[Teres Alexis, Alan Previn]

On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> The GSC notifies us of a proxy request via the HECI2 interrupt. The

alan:snip

> @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>  	u32 irqs = GT_RENDER_USER_INTERRUPT;
>  	u32 guc_mask = intel_uc_wants_guc(&gt->uc) ? GUC_INTR_GUC2HOST : 0;
>  	u32 gsc_mask = 0;
> +	u32 heci_mask = 0;
alan: nit: should we call this heci2_mask - uniquely identifiable from legacy.


alan:snip

> -	else if (HAS_HECI_GSC(gt->i915))
> +		heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2?
i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14?

alan:snip



> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> index 4aecb5a7b631..da11ce5ca99e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> @@ -1577,6 +1577,7 @@
>  
>  #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
>  #define   GEN11_CSME				(31)
> +#define   GEN12_HECI_2				(30)
alan: I dont see this being used anywhere - should remove this.

> +#define GEN11_HECI2_RSVD_INTR_MASK		_MMIO(0x1900e4)
alan: GEN11? don't you mean GEN12?





> +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
> +{
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +
> +	if (unlikely(!iir))
> +		return;
> +
> +	lockdep_assert_held(gt->irq_lock);
> +
> +	if (!gsc->proxy.component) {
> +		gt_err(gt, "GSC proxy irq received without the component being bound!\n");
alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we
know that proxy runtime irqs could happen anytime (depending on other gpu-security-related
system interactions). However, would the component driver be bound/unbound through a
suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request
came too early after a resume cycle. If yes, then should we not do somethign here to ensure that
when the component gets bound later, we know there is something waiting to be processed?
(in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger
the worker from there, i.e. the bind func?)

alan:snip

>  static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
>  					 struct device *tee_kdev, void *data)
>  {
>  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> -	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
> +	struct intel_gt *gt = i915->media_gt;
> +	struct intel_gsc_uc *gsc = &gt->uc.gsc;
> +	intel_wakeref_t wakeref;
> +
> +	/* enable HECI2 IRQs */
> +	with_intel_runtime_pm(&i915->runtime_pm, wakeref)
> +		intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
> +				 0, HECI_H_CSR_IE);
alan: i notice we don't seem to care about potentially re-writing a '1' into reset
if it was midst reset when we read. Shouldn't we also protect against that here?

alan:snip


> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> index 023bded10dde..a2a0813b8a76 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> @@ -23,6 +23,9 @@ struct intel_gsc_uc {
>  	/* for delayed load and proxy handling */
>  	struct workqueue_struct *wq;
>  	struct work_struct work;
> +	u32 gsc_work_actions; /* protected by gt->irq_lock */
> +#define GSC_ACTION_FW_LOAD BIT(0)
> +#define GSC_ACTION_SW_PROXY BIT(1
> 
alan: perhaps its time to have a substruct for "proxy_worker" and include
'wq' and 'work' in additional to actions?
> )
>  
>  	struct {
>  		struct i915_gsc_proxy_component *component;

Re: [Intel-gfx] [PATCH 4/4] drm/i915/gsc: add support for GSC proxy interrupt

[Ceraolo Spurio, Daniele]

On 4/20/2023 11:49 AM, Teres Alexis, Alan Previn wrote:
> On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
>> The GSC notifies us of a proxy request via the HECI2 interrupt. The
> alan:snip
>
>> @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
>>   	u32 irqs = GT_RENDER_USER_INTERRUPT;
>>   	u32 guc_mask = intel_uc_wants_guc(&gt->uc) ? GUC_INTR_GUC2HOST : 0;
>>   	u32 gsc_mask = 0;
>> +	u32 heci_mask = 0;
> alan: nit: should we call this heci2_mask - uniquely identifiable from legacy.

The mask is theoretically not just for HECI2, the bit we set in it is. 
If future platforms add back the HECI1 interrupt then it'd go in the 
same mask.

>
>
> alan:snip
>
>> -	else if (HAS_HECI_GSC(gt->i915))
>> +		heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
> alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2?
> i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14?

GSC_IRQ_INTF(1) is the HECI2 interrupt on DG2 and the bit has remained 
the same exactly to allow SW to re-use the same code/defines, so IMO we 
should do so.

>
> alan:snip
>
>
>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> index 4aecb5a7b631..da11ce5ca99e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
>> @@ -1577,6 +1577,7 @@
>>   
>>   #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
>>   #define   GEN11_CSME				(31)
>> +#define   GEN12_HECI_2				(30)
> alan: I dont see this being used anywhere - should remove this.

A few of the defines for this register here are not used. I've added 
this one in as a way of documenting where the bit was, but I can remove 
it if you think it's unneeded.

>
>> +#define GEN11_HECI2_RSVD_INTR_MASK		_MMIO(0x1900e4)
> alan: GEN11? don't you mean GEN12?
>

Yup, this should be GEN12

>
>
>
>> +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
>> +{
>> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
>> +
>> +	if (unlikely(!iir))
>> +		return;
>> +
>> +	lockdep_assert_held(gt->irq_lock);
>> +
>> +	if (!gsc->proxy.component) {
>> +		gt_err(gt, "GSC proxy irq received without the component being bound!\n");
> alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we
> know that proxy runtime irqs could happen anytime (depending on other gpu-security-related
> system interactions). However, would the component driver be bound/unbound through a
> suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request
> came too early after a resume cycle. If yes, then should we not do somethign here to ensure that
> when the component gets bound later, we know there is something waiting to be processed?
> (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger
> the worker from there, i.e. the bind func?)

A proxy request can only be triggered in response to a userspace ask, 
which in turn can only happen once we've completed the resume flow and 
the component is re-bound. Therefore, we should never have a situation 
where we get a proxy interrupt without the component being bound.

>
> alan:snip
>
>>   static int i915_gsc_proxy_component_bind(struct device *i915_kdev,
>>   					 struct device *tee_kdev, void *data)
>>   {
>>   	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
>> -	struct intel_gsc_uc *gsc = &i915->media_gt->uc.gsc;
>> +	struct intel_gt *gt = i915->media_gt;
>> +	struct intel_gsc_uc *gsc = &gt->uc.gsc;
>> +	intel_wakeref_t wakeref;
>> +
>> +	/* enable HECI2 IRQs */
>> +	with_intel_runtime_pm(&i915->runtime_pm, wakeref)
>> +		intel_uncore_rmw(gt->uncore, HECI_H_CSR(MTL_GSC_HECI2_BASE),
>> +				 0, HECI_H_CSR_IE);
> alan: i notice we don't seem to care about potentially re-writing a '1' into reset
> if it was midst reset when we read. Shouldn't we also protect against that here?

Yeah, I'll add that in

>
> alan:snip
>
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> index 023bded10dde..a2a0813b8a76 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> @@ -23,6 +23,9 @@ struct intel_gsc_uc {
>>   	/* for delayed load and proxy handling */
>>   	struct workqueue_struct *wq;
>>   	struct work_struct work;
>> +	u32 gsc_work_actions; /* protected by gt->irq_lock */
>> +#define GSC_ACTION_FW_LOAD BIT(0)
>> +#define GSC_ACTION_SW_PROXY BIT(1
>>
> alan: perhaps its time to have a substruct for "proxy_worker" and include
> 'wq' and 'work' in additional to actions?

The worker is not just for proxy, we use it for a GSC and HuC loading as 
well. It's the main way we handle the gsc_uc, so IMO it's better off 
staying in the main struct. However, if you still think a substructure 
will make things cleaner I can add it in, but please suggest a name 
because I have no idea what to call it (something like handler?).

Daniele

>> )
>>   
>>   	struct {
>>   		struct i915_gsc_proxy_component *component;

Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

[Ceraolo Spurio, Daniele]

On 4/18/2023 11:57 PM, Teres Alexis, Alan Previn wrote:
> On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>>
>> Add GSC proxy driver. It to allows messaging between GSC component
>> on Intel on board graphics card and CSE device.
> alan:nit: isn't "Intel integrated GPU" clearer than "Intel on-board graphics card"?
> Same thing for the Kconfig description later (or am i missing something else here).

Will change

>
> alan:snip
>
>
>
>> +         MEI GSC proxy enables messaging between GSC service on
>> +         Intel graphics on-board card and services on CSE (MEI)
> alan:nit: same as above
>
>
>
>
>> diff --git a/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
>> new file mode 100644
>> index 000000000000..953eda1a16fb
>> --- /dev/null
>> +++ b/drivers/misc/mei/gsc_proxy/mei_gsc_proxy.c
>> @@ -0,0 +1,208 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2022-2023 Intel Corporation
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/slab.h>
>> +#include <linux/uuid.h>
>> +#include <linux/mei_cl_bus.h>
> alan: [nit?] i thought we need to have the headers alphabetically ordered? (below too)

Will fix

>> +#include <linux/component.h>
>> +#include <drm/drm_connector.h>
>> +#include <drm/i915_component.h>
>> +#include <drm/i915_gsc_proxy_mei_interface.h>
>> +
>> +/**
>> + * mei_gsc_proxy_send - Sends a proxy message to ME FW.
>> + * @dev: device corresponding to the mei_cl_device
>> + * @buf: a message buffer to send
>> + * @size: size of the message
>> + * Return: bytes sent on Success, <0 on Failure
>> + */
>> +static int mei_gsc_proxy_send(struct device *dev, const void *buf, size_t size)
>> +{
>> +	ssize_t ret;
>> +
>> +	if (!dev || !buf)
> alan: nit: not sure if we should be checking for !size here - i do see that next patch
> is checking for this from i915 side of the interface... but just wasnt sure which is the prefered style
> (in terms of where to check for this condition). Either way, its a nit for me since i traced down
> all the way to mei_cl_alloc_cb and it looks like mei bus can tolerate zero sized messages.
>> +		return -EINVAL;
> alan:snip
>
>> +static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
>> +{
>> +	ssize_t ret;
>> +
>> +	if (!dev || !buf)
> alan: nit: same as in the 'send' above,.. not sure if we should be checking for !size here...
> or perhaps 0 sized recv is supported.

AFAICS the lower level of the mei code do allow for size 0 for both send 
and recv. Also, this is the same check as what we do for the PXP component.

>
>> +		return -EINVAL;
> alan:snip
>
>> +static int mei_gsc_proxy_component_match(struct device *dev, int subcomponent,
>> +					 void *data)
>> +{
>> +	struct pci_dev *pdev;
>> +
>> +	if (!dev_is_pci(dev))
>> +		return 0;
>> +
>> +	pdev = to_pci_dev(dev);
>> +
>> +	if (pdev->class != (PCI_CLASS_DISPLAY_VGA << 8) ||
>> +	    pdev->vendor != PCI_VENDOR_ID_INTEL)
>> +		return 0;
>> +
>> +	if (subcomponent != I915_COMPONENT_GSC_PROXY)
>> +		return 0;
>> +
>> +	return component_compare_dev(dev->parent, ((struct device *)data)->parent);
> alan: do we care if both these parents are non-null? i notice in other mei component
> drivers match functions we do check that.

Those should always both be non-NULL, since both the mei and the GFX 
device have the PCI bus as parent (and the previous check on pdev 
ensures those are the 2 devices we're handling at this point).

>
>> +}
>> +
> alan:snip
>
>> +#define MEI_UUID_GSC_PROXY UUID_LE(0xf73db04, 0x97ab, 0x4125, \
>> +				   0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)
> alan: apologies for the newbie question, but why are we using UUID for the gsc_proxy
> as opposed to GUID like the other mei components? i am not sure if i read the right
> archived patch review but it sounded like GUID is for internal to kernel only whereas
> UUID is for external too? (in which case why are we not using GUID for gsc-proxy?)
> Consider this a non-blocking inquiry since i assume mei folks have reviewed this
> prior and this is an explicit design decision that I'm just not versed on.

AFAICS all other mei components use UUID_LE as well. The code was 
updated from GUID to UUID_LE in:
https://lore.kernel.org/all/20221228160558.21311-1-andriy.shevchenko@linux.intel.com/

Daniele

>
> alan:snip

Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

[Ceraolo Spurio, Daniele]

On 4/20/2023 3:04 PM, Ceraolo Spurio, Daniele wrote:
>
>
> On 4/18/2023 11:57 PM, Teres Alexis, Alan Previn wrote:
>> On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
>>> From: Alexander Usyskin <alexander.usyskin@intel.com>
>>>
>>> Add GSC proxy driver. It to allows messaging between GSC component
>>> on Intel on board graphics card and CSE device.
>> alan:nit: isn't "Intel integrated GPU" clearer than "Intel on-board 
>> graphics card"?
>> Same thing for the Kconfig description later (or am i missing 
>> something else here).
>
> Will change

Thinking again, GSC proxy will be applicable to non-integrated GPUs as 
well, so I'm just going to change this to "Intel graphics card".

Daniele

Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver

[Teres Alexis, Alan Previn]

i guess we are settled with this patch...

On Thu, 2023-04-20 at 15:04 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/18/2023 11:57 PM, Teres Alexis, Alan Previn wrote:
> > On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> > > From: Alexander Usyskin <alexander.usyskin@intel.com>
> > > 
> > > Add GSC proxy driver. It to allows messaging between GSC component
> > > on Intel on board graphics card and CSE device.
> > alan:nit: isn't "Intel integrated GPU" clearer than "Intel on-board graphics card"?
> > Same thing for the Kconfig description later (or am i missing something else here).
> 
> Will change
> 
alan: saw your reply on better alternative for both 'i' and 'd'

alan:snip
> > > +static int mei_gsc_proxy_recv(struct device *dev, void *buf, size_t size)
> > > +{
> > > +	ssize_t ret;
> > > +
> > > +	if (!dev || !buf)
> > alan: nit: same as in the 'send' above,.. not sure if we should be checking for !size here...
> > or perhaps 0 sized recv is supported.
> 
> AFAICS the lower level of the mei code do allow for size 0 for both send 
> and recv. Also, this is the same check as what we do for the PXP component.
alan: agreed - thus the nit as per my earlier email.

> > 
alan:snip

> > > +	if (subcomponent != I915_COMPONENT_GSC_PROXY)
> > > +		return 0;
> > > +
> > > +	return component_compare_dev(dev->parent, ((struct device *)data)->parent);
> > alan: do we care if both these parents are non-null? i notice in other mei component
> > drivers match functions we do check that.
> 
> Those should always both be non-NULL, since both the mei and the GFX 
> device have the PCI bus as parent (and the previous check on pdev 
> ensures those are the 2 devices we're handling at this point).
alan: sounds good.



> > > +#define MEI_UUID_GSC_PROXY UUID_LE(0xf73db04, 0x97ab, 0x4125, \
> > > +				   0xb8, 0x93, 0xe9, 0x4, 0xad, 0xd, 0x54, 0x64)
> > alan: apologies for the newbie question, but why are we using UUID for the gsc_proxy
> > as opposed to GUID like the other mei components? i am not sure if i read the right
> > archived patch review but it sounded like GUID is for internal to kernel only whereas
> > UUID is for external too? 
[snip]
> AFAICS all other mei components use UUID_LE as well. The code was 
> updated from GUID to UUID_LE in:
> https://lore.kernel.org/all/20221228160558.21311-1-andriy.shevchenko@linux.intel.com/
alan: sounds good- thanks for the URL.

Re: [Intel-gfx] [PATCH 4/4] drm/i915/gsc: add support for GSC proxy interrupt

[Teres Alexis, Alan Previn]

I think we are also bottom-ing on the opens fo this patch too:


On Thu, 2023-04-20 at 13:21 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 11:49 AM, Teres Alexis, Alan Previn wrote:
> > On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> > > The GSC notifies us of a proxy request via the HECI2 interrupt. The
> > alan:snip
> > 
> > > @@ -256,6 +262,7 @@ void gen11_gt_irq_postinstall(struct intel_gt *gt)
> > >   	u32 irqs = GT_RENDER_USER_INTERRUPT;
> > >   	u32 guc_mask = intel_uc_wants_guc(&gt->uc) ? GUC_INTR_GUC2HOST : 0;
> > >   	u32 gsc_mask = 0;
> > > +	u32 heci_mask = 0;
> > alan: nit: should we call this heci2_mask - uniquely identifiable from legacy.
> 
> The mask is theoretically not just for HECI2, the bit we set in it is. 
> If future platforms add back the HECI1 interrupt then it'd go in the 
> same mask.
alan: yeah - im good with that - no change needed then - was a nit anyways.


> > alan:snip
> > 
> > > -	else if (HAS_HECI_GSC(gt->i915))
> > > +		heci_mask = GSC_IRQ_INTF(1); /* HECI2 IRQ for SW Proxy*/
> > alan: does this GSC_IRQ_INTF macro still make sense for this newer platforms with GSC-CS / HECI2?
> > i dont think i see other bit definitions for this mask register, so might else well just define it as BIT14?
> 
> GSC_IRQ_INTF(1) is the HECI2 interrupt on DG2 and the bit has remained 
> the same exactly to allow SW to re-use the same code/defines, so IMO we 
> should do so.
alan: okay sure - sounds good.


> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > index 4aecb5a7b631..da11ce5ca99e 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > @@ -1577,6 +1577,7 @@
> > >   
> > >   #define GEN11_GT_INTR_DW(x)			_MMIO(0x190018 + ((x) * 4))
> > >   #define   GEN11_CSME				(31)
> > > +#define   GEN12_HECI_2				(30)
> > alan: I dont see this being used anywhere - should remove this.
> 
> A few of the defines for this register here are not used. I've added 
> this one in as a way of documenting where the bit was, but I can remove 
> it if you think it's unneeded.
alan: Oh i actually didnt notice that earlier - in that case, please keep it
would be nice to add a comment for all of those such bits (consider this a nit).

> > 

> > > +void intel_gsc_proxy_irq_handler(struct intel_gsc_uc *gsc, u32 iir)
> > > +{
> > > +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> > > +
> > > +	if (unlikely(!iir))
> > > +		return;
> > > +
> > > +	lockdep_assert_held(gt->irq_lock);
> > > +
> > > +	if (!gsc->proxy.component) {
> > > +		gt_err(gt, "GSC proxy irq received without the component being bound!\n");
> > alan: So although proxy init is only a one-time thing (even surviving suspend-resume), we
> > know that proxy runtime irqs could happen anytime (depending on other gpu-security-related
> > system interactions). However, would the component driver be bound/unbound through a
> > suspend-resume cycle? If yes, then would this check fail if an IRQ for a proxy request
> > came too early after a resume cycle. If yes, then should we not do somethign here to ensure that
> > when the component gets bound later, we know there is something waiting to be processed?
> > (in bind, if proxy-init was done before, but we have outstanding IRQs, then we can trigger
> > the worker from there, i.e. the bind func?)
> 
> A proxy request can only be triggered in response to a userspace ask, 
> which in turn can only happen once we've completed the resume flow and 
> the component is re-bound. Therefore, we should never have a situation 
> where we get a proxy interrupt without the component being bound.
alan: thanks -my bad.
> > 


> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > @@ -23,6 +23,9 @@ struct intel_gsc_uc {
> > >   	/* for delayed load and proxy handling */
> > >   	struct workqueue_struct *wq;
> > >   	struct work_struct work;
> > > +	u32 gsc_work_actions; /* protected by gt->irq_lock */
> > > +#define GSC_ACTION_FW_LOAD BIT(0)
> > > +#define GSC_ACTION_SW_PROXY BIT(1
> > > 
> > alan: perhaps its time to have a substruct for "proxy_worker" and include
> > 'wq' and 'work' in additional to actions?
> 
> The worker is not just for proxy, we use it for a GSC and HuC loading as 
> well. It's the main way we handle the gsc_uc, so IMO it's better off 
> staying in the main struct. However, if you still think a substructure 
> will make things cleaner I can add it in, but please suggest a name 
> because I have no idea what to call it (something like handler?).
alan: i was thinking "task_handler" - but since its not specific to proxy,
then let's not change it.