Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver
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
> > >
> > > 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.shevche...@linux.intel.com/
alan: sounds good- thanks for the URL.
Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver
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 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
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
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 ..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
+#include
+#include
+#include
+#include
alan: [nit?] i thought we need to have the headers alphabetically ordered?
(below too)
Will fix
+#include
+#include
+#include
+#include
+
+/**
+ * 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.shevche...@linux.intel.com/
Daniele
alan:snip
Re: [Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver
On Wed, 2023-03-29 at 09:56 -0700, Ceraolo Spurio, Daniele wrote:
> From: Alexander Usyskin
>
> 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 ..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
> +#include
> +#include
> +#include
> +#include
alan: [nit?] i thought we need to have the headers alphabetically ordered?
(below too)
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * 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
[Intel-gfx] [PATCH 2/4] mei: gsc_proxy: add gsc proxy driver
From: Alexander Usyskin
Add GSC proxy driver. It to allows messaging between GSC component
on Intel on board graphics card and CSE device.
Cc: Daniele Ceraolo Spurio
Cc: Alan Previn
Signed-off-by: Alexander Usyskin
Signed-off-by: Tomas Winkler
Acked-by: Greg Kroah-Hartman
---
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 ..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 ..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 ..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * 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_unbi
