[Intel-xe] [PATCH 09/12] drm/xe/gsc: Add an interface for GSC packet submissions

[John Harrison]

On 11/13/2023 13:19, Daniele Ceraolo Spurio wrote:
> On 11/13/2023 11:59 AM, John Harrison wrote:
>> On 10/27/2023 15:29, Daniele Ceraolo Spurio wrote:
>>> Communication with the GSC FW is done via input/output buffers, whose
>>> addresses are provided via a GSCCS command. The buffers contain a
>>> generic header and a client-specific packet (e.g. PXP, HDCP); the
>>> clients don't care about the header format and/or the GSCCS command in
>>> the batch, they only care about their client-specific header. This 
>>> patch
>>> therefore introduces helpers that allow the callers to automatically
>>> fill in the input header, submit the GSCCS job and decode the output
>>> header, to make it so that the caller only needs to worry about their
>>> client-specific input and output messages.
>>>
>>> NOTE: this patch by itself only adds the interface so it does nothing,
>>> I've kept it separate for review but the plan is to squash it with the
>>> follow up patch before merge, so that the interface and the user are
>>> introduced at the same time.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>>> Cc: Suraj Kandpal <suraj.kandpal at intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/Makefile                   |   1 +
>>>   .../gpu/drm/xe/abi/gsc_command_header_abi.h   |  46 +++++
>>>   .../gpu/drm/xe/instructions/xe_gsc_commands.h |   2 +
>>>   drivers/gpu/drm/xe/xe_gsc_submit.c            | 170 
>>> ++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_gsc_submit.h            |  30 ++++
>>>   5 files changed, 249 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
>>>   create mode 100644 drivers/gpu/drm/xe/xe_gsc_submit.c
>>>   create mode 100644 drivers/gpu/drm/xe/xe_gsc_submit.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 4a442dcf4d79..876c122ad63c 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -58,6 +58,7 @@ xe-y += xe_bb.o \
>>>       xe_force_wake.o \
>>>       xe_ggtt.o \
>>>       xe_gsc.o \
>>> +    xe_gsc_submit.o \
>>>       xe_gt.o \
>>>       xe_gt_clock.o \
>>>       xe_gt_debugfs.o \
>>> diff --git a/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h 
>>> b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
>>> new file mode 100644
>>> index 000000000000..a4c2646803b5
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/abi/gsc_command_header_abi.h
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2023 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _ABI_GSC_COMMAND_HEADER_ABI_H
>>> +#define _ABI_GSC_COMMAND_HEADER_ABI_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +struct intel_gsc_mtl_header {
>> Is this all really MTL platform specific? Or is it GSC version specific?
>>
>> Given that the reset of the driver is moving to be IP version based 
>> instead of platform name based, it seems like this should also be 
>> based in some kind of version rather than platform name. Or are the 
>> GSC versions all just relative to the platform anyway?
>
> GSC versioning is relative to the platform. As far as I'm aware the 
> expectation so far is that this header will stay the same for LNL, but 
> that's not a guarantee. Some of the binary headers for example are 
> slightly different in the LNL blob (but the differences don't impact 
> any of the areas we actually look at, so we can re-use the MTL code 
> there as well). If you want to avoid the platform name I can call it 
> intel_gsc_cmd_header for now and we can think about per-platform 
> naming if we ever get different versions. Would that work?
If the GSC team are saying that this is a platform specific interface 
then naming it by platform is fine by me. And keeping the name for 
future platforms doesn't seem like an issue. That's how everything else 
works - named by the device/platform/gen that introduced the feature and 
still used for future devices/platforms/gens until the next version 
comes along with a new name.

Reviewed-by: John Harrison <John.C.Harrison at Intel.Com>


>
> Daniele
>
>>
>> Apart from that, it all looks good to me.
>>
>> John.
>>
>