Re: [Intel-gfx] [PATCH v7 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

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

On 4/6/2023 10:44 AM, Alan Previn wrote:
> Add GSC engine based method for sending PXP firmware packets
> to the GSC firmware for MTL (and future) products.
>
> Use the newly added helpers to populate the GSC-CS memory
> header and send the message packet to the FW by dispatching
> the GSC_HECI_CMD_PKT instruction on the GSC engine.
>
> We use non-priveleged batches for submission to GSC engine
> which require two buffers for the request:
>       - a buffer for the HECI packet that contains PXP FW commands
>       - a batch-buffer that contains the engine instruction for
>         sending the HECI packet to the GSC firmware.
>
> Thus, add the allocation and freeing of these buffers in gsccs
> init and fini.
>
> The GSC-fw may reply to commands with a SUCCESS but with an
> additional pending-bit set in the reply packet. This bit
> means the GSC-FW is currently busy and the caller needs to
> try again with the gsc_message_handle the fw returned. Thus,
> add a wrapper to continuously retry send_message while
> replaying the gsc_message_handle. Retries need to follow the
> arch-spec count and delay until GSC-FW replies with the real
> SUCCESS or timeout after that spec'd delay.
>
> The GSC-fw requires a non-zero host_session_handle provided
> by the caller to enable gsc_message_handle tracking. Thus,
> allocate the host_session_handle at init and destroy it
> at fini (the latter requiring an FYI to the gsc-firmware).
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
>   .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |   3 +
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 240 +++++++++++++++++-
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |   4 +
>   drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |   6 +
>   5 files changed, 253 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> index 3addce861854..e4d6662339e8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> @@ -51,6 +51,7 @@ struct intel_gsc_mtl_header {
>   	 */
>   	u32 flags;
>   #define GSC_OUTFLAG_MSG_PENDING 1
> +#define GSC_INFLAG_MSG_CLEANUP BIT(1)

For consistency those should all be BIT() defines

>   
>   	u32 status;
>   } __packed;

<snip>

> @@ -38,18 +248,46 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
>   	if (!engine)
>   		return -ENODEV;
>   
> +	/*
> +	 * Now, allocate, pin and map two objects, one for the heci message packet
> +	 * and another for the batch buffer we submit into GSC engine (that includes the packet).
> +	 * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem.
> +	 */
> +	err = gsccs_create_buffer(pxp->ctrl_gt, "Heci Packet",
> +				  2 * PXP43_MAX_HECI_INOUT_SIZE,
> +				  &exec_res->pkt_vma, &exec_res->pkt_vaddr);
> +	if (err)
> +		return err;
> +
> +	err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE,
> +				  &exec_res->bb_vma, &exec_res->bb_vaddr);
> +	if (err)
> +		goto free_pkt;
> +
>   	/* Finally, create an intel_context to be used during the submission */
>   	ce = intel_context_create(engine);
>   	if (IS_ERR(ce)) {
>   		drm_err(&gt->i915->drm, "Failed creating gsccs backend ctx\n");
> -		return PTR_ERR(ce);
> +		err = PTR_ERR(ce);
> +		goto free_batch;
>   	}
>   
>   	i915_vm_put(ce->vm);
>   	ce->vm = i915_vm_get(pxp->ctrl_gt->vm);
>   	exec_res->ce = ce;
>   
> +	/* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */
> +	get_random_bytes(&exec_res->host_session_handle, sizeof(exec_res->host_session_handle));

Thinking back at this, maybe a possible solution to avoid randomly 
generated clashing values is to check if any of the existing exec_res 
already uses the generated value. Not a blocker because we only have 1 
exec_res for now, so no chance of clashing.

With the define fixed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> +
>   	return 0;
> +
> +free_batch:
> +	i915_vma_unpin_and_release(&exec_res->bb_vma, I915_VMA_RELEASE_MAP);
> +free_pkt:
> +	i915_vma_unpin_and_release(&exec_res->pkt_vma, I915_VMA_RELEASE_MAP);
> +	memset(exec_res, 0, sizeof(*exec_res));
> +
> +	return err;
>   }
>   
>   void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> index 354ea9a8f940..bd1c028bc80f 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> @@ -10,6 +10,10 @@
>   
>   struct intel_pxp;
>   
> +#define GSC_REPLY_LATENCY_MS 200
> +#define GSC_PENDING_RETRY_MAXCOUNT 40
> +#define GSC_PENDING_RETRY_PAUSE_MS 50
> +
>   #ifdef CONFIG_DRM_I915_PXP
>   void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
>   int intel_pxp_gsccs_init(struct intel_pxp *pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index fdd98911968d..73392fbab7ee 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -38,6 +38,12 @@ struct intel_pxp {
>   	struct gsccs_session_resources {
>   		u64 host_session_handle; /* used by firmware to link commands to sessions */
>   		struct intel_context *ce; /* context for gsc command submission */
> +
> +		struct i915_vma *pkt_vma; /* GSC FW cmd packet vma */
> +		void *pkt_vaddr;  /* GSC FW cmd packet virt pointer */
> +
> +		struct i915_vma *bb_vma; /* HECI_PKT batch buffer vma */
> +		void *bb_vaddr; /* HECI_PKT batch buffer virt pointer */
>   	} gsccs_res;
>   
>   	/**