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

[Kandpal, Suraj]

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

Wouldn’t that be a lot of repeated code just to have a little more readability.
Maybe just add some comments as to what the functions do that should avoid
The confusion.

> 
> 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
> 

Idk if filling the header in this function itself would be the best, we can have a common function defined here
If required to fill the header but I would like to give both hdcp pxp a little freedom as to when they fill
this up.

> 
> 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
> >