On Wed, 2022-12-14 at 14:37 +0530, Kandpal, Suraj wrote:
> HDCP and PXP will require a common function to allow it to
> submit commands to the gsc cs. Also adding the gsc mtl header
> that needs to be added on to the existing payloads of HDCP
> and PXP.
>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Alan Previn <alan.previn.teres.alexis@xxxxxxxxx>
> Signed-off-by: Suraj Kandpal<suraj.kandpal@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 2 +
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 62 +++++++++++++++++++-
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 3 +
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fwif.h | 41 +++++++++++++
> 4 files changed, 105 insertions(+), 3 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fwif.h
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> index 2af1ae3831df..454179884801 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> @@ -439,6 +439,8 @@
> #define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
> #define HECI1_FW_LIMIT_VALID (1 << 31)
>
> +#define GSC_HECI_CMD_PKT GSC_INSTR(0, 0, 6)
> +
> /*
> * Used to convert any address to canonical form.
> * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> index e73d4440c5e8..f00e88fdb5d2 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -30,6 +30,35 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
> return fw_status & GSC_FW_INIT_COMPLETE_BIT;
> }
>
Alan:[snip]
> @@ -49,7 +78,12 @@ static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
> return 0;
> }
>
> -static int gsc_fw_load(struct intel_gsc_uc *gsc)
> +/*
> + * Our submissions to GSC are going to be either a FW load or an heci pkt, but
> + * all the request emission logic is the same so we can use a common func and
> + * just add the correct cmd
> + */
> +static int submit_to_gsc_fw(struct intel_gsc_uc *gsc, struct gsc_heci_pkt *pkt)
> {
> struct intel_context *ce = gsc->ce;
> struct i915_request *rq;
> @@ -68,7 +102,11 @@ static int gsc_fw_load(struct intel_gsc_uc *gsc)
> goto out_rq;
> }
>
> - err = emit_gsc_fw_load(rq, gsc);
> + if (pkt)
> + err = emit_gsc_heci_pkt(rq, pkt);
> + else
> + err = emit_gsc_fw_load(rq, gsc);
> +
Alan: To be honest, code function names + responsibilities lack proper hierarchy - doens't look quite right from my perspective for readability / scalability.
In my opinion, we create a separate functions for load_fw vs heci_packet. But have a common utility function for the actual sending to HW (engine->emit_flush)
and waiting with a timeout (i915_request_wait). We know heci_packet will in future be used by PXP and potentially across both concurrently.
Then we mirror the same thing for general heci load (thus also allowing differentiated debug messages):
intel_gsc_engine_send_loadfw
| (allocate the request, use the gsc-ce).
|---> emit_gsc_heci_pkt (fill up the send-heci-pkt cmd)
|---> submit_to_gsc_fw(req, ... timeout)
intel_gsc_engine_send_hecipkt
| (allocate the request, use the gsc-ce).
| (we could even potentially create the MTL CS HEADER here itself
| since the GSC-CS memory header isnt an entity of the caller
| subsystem such as hdcp or pxp, but rather is the entity of the
| (GSC) command-streamer param, so bring it into intel_gsc_fw file)
|---> emit_gsc_fw_load (fill up the fw load cmd)
|---> submit_to_gsc_fw(req, ... timeout)
* intel_gsc_engine_send_hecipkt common to >1 caller-subsystems
Additionally, one last thing might be to move only sets of functions into separate files with common helpers:
intel_gsc_fw.c : all the firmware loading related functions
intel_gsc_heci.c : all the heci command packet sending related functions (here is where we can add the GSC-CS memory header population and in future, the host-session-id
allocation for PXP).
intel_gsc_cs_helper : for the submit_to_gsc_fw and other common functions to both fw-loading and heci-packet sending.
Alan:[snip]
> + u8 gsc_address;
> +#define HECI_MEADDRESS_PXP 17
> +#define HECI_MEADDRESS_HDCP 18
> +
> + u8 reserved1;
> +
> + u16 header_version;
> +#define MTL_GSC_HEADER_VERSION 1
> +
> + u64 host_session_handle;
> + u64 gsc_message_handle;
> +
> + u32 message_size; /* lower 20 bits only, upper 12 are reserved */
> +
> + /*
> + * Flags mask:
> + * Bit 0: Pending
> + * Bit 1: Session Cleanup;
> + * Bits 2-15: Flags
> + * Bits 16-31: Extension Size
> + */
> + u32 flags;
> +
> + u32 status;
> +} __packed;
> +
> +#endif
> --
> 2.25.1
>
