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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

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