Re: [Intel-gfx] [PATCH v3 1/7] drm/i915/gsc: Create GSC request submission mechanism

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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