| Message ID | 20230329165658.2686549-3-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add support for MTL GSC SW Proxy | expand |
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
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
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
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.
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");