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

From: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>
To: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Cc: "justonli@chromium.org" <justonli@chromium.org>,
	"Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
Date: Thu, 9 Feb 2023 23:23:38 +0000	[thread overview]
Message-ID: <6850f599a093c651afe1869f986fe4c13cad83af.camel@intel.com> (raw)
In-Reply-To: <20230125080651.100223-5-alan.previn.teres.alexis@intel.com>

missed somethings on host-session-handle - for next rev.

On Wed, 2023-01-25 at 00:06 -0800, Teres Alexis, 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
> and 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.
> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>  .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |   4 +
>  drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 174 +++++++++++++++++-
>  drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |   6 +
>  3 files changed, 183 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> index ad67e3f49c20..b2523d6918c7 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> @@ -12,6 +12,10 @@
>  /* PXP-Cmd-Op definitions */
>  #define PXP43_CMDID_START_HUC_AUTH 0x0000003A
>  
> +/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
> +#define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
> +#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K)
> +
>  /* PXP-Input-Packet: HUC-Authentication */
>  struct pxp43_start_huc_auth_in {
>  	struct pxp_cmd_header header;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index b304864902c8..35b6bfa55dfc 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -6,12 +6,154 @@
>  #include "gem/i915_gem_internal.h"
>  
>  #include "gt/intel_context.h"
> +#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
>  
>  #include "i915_drv.h"
>  #include "intel_pxp_cmd_interface_43.h"
>  #include "intel_pxp_gsccs.h"
>  #include "intel_pxp_types.h"
>  
> 
alan:snip

>  static void
>  gsccs_destroy_execution_resource(struct intel_pxp *pxp)
>  {
> @@ -19,6 +161,10 @@ gsccs_destroy_execution_resource(struct intel_pxp *pxp)
> 
we should send a message to firmware to cleanup all resources associated with i915's host-session-handle here
>  
>  	if (strm_res->ce)
>  		intel_context_put(strm_res->ce);
> +	if (strm_res->bb_vma)
> +		i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP);
> +	if (strm_res->pkt_vma)
> +		i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP);
>  
>  	memset(strm_res, 0, sizeof(*strm_res));
>  }
> 

alan:snip

>  	 * First, ensure the GSC engine is present.
> @@ -40,11 +187,28 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
>  
>  	mutex_init(&pxp->tee_mutex);
>  
> +	/*
> +	 * 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",
> +				  PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE,
> +				  &strm_res->pkt_vma, &strm_res->pkt_vaddr);
> +	if (err)
> +		return err;
> +
> +	err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE,
> +				  &strm_res->bb_vma, &strm_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);
> @@ -52,6 +216,14 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
>  	strm_res->ce = ce;
we should be allocatting the host-session-handle here 
(the same one should be used throughout i915 life-cycle until unload)
so we can remove that from the arb-session-creation on next patch

 alan:snip