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

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

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@intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
>> Cc: Suraj Kandpal <suraj.kandpal@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?

Daniele

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