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