| Message ID | 20230111214226.907536-3-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add MTL PXP Support | expand |
On 1/11/2023 1:42 PM, Alan Previn wrote: > For MTL, PXP transport back-end uses the GSC engine to submit > HECI packets for PXP arb session management. The command submission > that uses non-priveleged mode requires us to allocate (or free) > a set of execution submission resources (buffer-object, batch-buffer > and context). Thus, do this one time allocation of resources in > GSC-CS init and clean them up in fini. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 6 +- > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 216 +++++++++++++++++- > drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 5 + > 3 files changed, 225 insertions(+), 2 deletions(-) > > 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..52b9a61bcdd4 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 > @@ -10,7 +10,11 @@ > #include "intel_pxp_cmd_interface_cmn.h" > > /* PXP-Cmd-Op definitions */ > -#define PXP43_CMDID_START_HUC_AUTH 0x0000003A > +#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 { > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > index 21400650fc86..97ca187e6fde 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -3,9 +3,41 @@ > * Copyright(c) 2023 Intel Corporation. > */ > > +#include "gem/i915_gem_internal.h" > + > +#include "gt/intel_context.h" > + > #include "i915_drv.h" > -#include "intel_pxp_types.h" > +#include "intel_pxp_cmd_interface_43.h" > #include "intel_pxp_gsccs.h" > +#include "intel_pxp_types.h" > + > +struct gsccs_session_resources { > + struct mutex cmd_mutex; /* Protects submission for arb session */ > + u64 host_session_handle; /* used by firmware to link commands to sessions */ > + > + struct intel_context *ce; /* context for gsc command submission */ > + struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */ The arb session creation is a kernel submission, so can't you just use the default kernel ppgtt (i.e., gt->vm)? > + > + struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */ > + struct i915_vma *pkt_vma; /* PXP HECI message packet vma */ > + void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */ > + > + /* Buffer info for GSC engine batch buffer: */ > + struct drm_i915_gem_object *bb_obj; /* batch buffer object */ > + struct i915_vma *bb_vma; /* batch buffer vma */ > + void *bb_vaddr; /* batch buffer virtual memory pointer */ You aren't actually making use of any of the variables in this struct in this patch, apart from initialization. Some of those are pretty clear on what they will be used for (e.g. the context), bu others are a bit more vague(e.g. the vaddrs). It'll probably be cleaner to reorder things so the more implementation-specific variables are added when they're used. I'll try to add more comment in follow-up patches. > +}; > + > +struct gsccs_teelink_priv { > + /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */ > + struct gsccs_session_resources arb_exec_res; > +}; > + > +static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp) > +{ > + return (struct gsccs_teelink_priv *)pxp->gsccs_priv; > +} Why do we need this layer of obfuscation with the void gsccs_priv? it's not like that can be assigned to anything else but gsccs_session_resources, so why not just have a pointer to that? If you want to have different priv based on the backend (mei vs gsccs) than it should be a more generic priv and be used in both cases. > > int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > int arb_session_id) > @@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > return -ENODEV; > } > > +static void > +gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma, > + struct drm_i915_gem_object *obj) > +{ > + int err; > + > + i915_vma_unpin(vma); > + err = i915_vma_unbind(vma); > + if (err) > + drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err); I don't think you need to explicitly call unbind here, it should be automatically covered by the object cleanup as long as it is unpinned > + > + i915_gem_object_unpin_map(obj); > + i915_gem_object_unpin_pages(obj); > + i915_gem_object_put(obj); If you don't explicitly pin the pages (which you don't need to, see comment below) the whole cleanup in this function can be done with just: i915_vma_unpin_and_release(vma, I915_VMA_RELEASE_MAP); > +} > + > +static int > +gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname, > + size_t size, struct i915_ppgtt *ppgtt, personal preference: IMO better to pass a generic "i915_address_space *vm" to this function. > + struct drm_i915_gem_object **obj, > + struct i915_vma **vma, void **map) > +{ > + int err = 0; > + > + *obj = i915_gem_object_create_internal(i915, size); > + if (IS_ERR(*obj)) { > + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname); > + err = PTR_ERR(*obj); > + goto out_none; > + } > + > + *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL); > + if (IS_ERR(*vma)) { > + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname); > + err = PTR_ERR(*vma); > + goto out_put; > + } > + > + err = i915_gem_object_pin_pages_unlocked(*obj); > + if (err) { > + drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname); > + goto out_put; > + } You're doing a vma_pin below, so no need to explicitly pin the pages here as the vma_pin will cover it. Also, do you need the object to be returned by the function? As mentioned above, the cleanup can be done with just the vma pointer and from a quick look I don't see the object being used in follow up patches. > + > + /* map to the virtual memory pointer */ > + *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true)); > + if (IS_ERR(*map)) { > + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname); > + err = PTR_ERR(*map); > + goto out_unpin; > + } > + > + /* all PXP sessions commands are treated as non-priveleged */ > + err = i915_vma_pin(*vma, 0, 0, PIN_USER); > + if (err) { > + drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname); > + goto out_unmap; > + } > + > + return 0; > + > +out_unmap: > + i915_gem_object_unpin_map(*obj); > +out_unpin: > + i915_gem_object_unpin_pages(*obj); > +out_put: > + i915_gem_object_put(*obj); > +out_none: > + *obj = NULL; > + *vma = NULL; > + *map = NULL; > + > + return err; > +} > + > +static void > +gsccs_destroy_execution_resource(struct intel_pxp *pxp, > + struct gsccs_session_resources *strm_res) > +{ > + if (strm_res->ce) > + intel_context_put(strm_res->ce); > + if (strm_res->bb_obj) > + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->bb_vma, strm_res->bb_obj); > + if (strm_res->pkt_obj) > + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->pkt_vma, strm_res->pkt_obj); > + if (strm_res->ppgtt) > + i915_vm_put(&strm_res->ppgtt->vm); > + > + memset(strm_res, 0, sizeof(*strm_res)); > +} > + > +static int > +gsccs_allocate_execution_resource(struct intel_pxp *pxp, > + struct gsccs_session_resources *strm_res) > +{ > + struct intel_gt *gt = pxp->ctrl_gt; > + struct intel_engine_cs *engine = gt->engine[GSC0]; > + struct i915_ppgtt *ppgtt; > + struct intel_context *ce; > + int err = 0; > + > + /* > + * First, ensure the GSC engine is present. > + * NOTE: Backend should would only be called with the correct gt. typo: should would > + */ > + if (!engine) > + return -ENODEV; > + > + mutex_init(&strm_res->cmd_mutex); > + > + ppgtt = i915_ppgtt_create(gt, 0); > + if (IS_ERR(ppgtt)) > + return PTR_ERR(ppgtt); > + > + strm_res->ppgtt = ppgtt; > + > + /* > + * 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->i915, "Heci Packet", > + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE, > + strm_res->ppgtt, > + &strm_res->pkt_obj, &strm_res->pkt_vma, > + &strm_res->pkt_vaddr); > + if (err) { > + gsccs_destroy_execution_resource(pxp, strm_res); > + return err; > + } > + > + err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Batch Buffer", > + PAGE_SIZE, strm_res->ppgtt, > + &strm_res->bb_obj, &strm_res->bb_vma, > + &strm_res->bb_vaddr); > + if (err) { > + gsccs_destroy_execution_resource(pxp, strm_res); > + return err; > + } > + /* > + * TODO: Consider optimization of pre-populating batch buffer > + * with the send-HECI instruction now at init and reuse through its life. > + */ This looks like a nice optimization, and it would also allow us to drop the bb_vaddr variable from the struct. If the only heci message we're ever going to send with this object is the session creation, we can probably also fill the "send" section of the pkt. > + > + /* Finally, create an intel_context to be used during the submission */ > + ce = intel_context_create(engine); > + if (IS_ERR(ce)) { > + drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); > + gsccs_destroy_execution_resource(pxp, strm_res); we usually prefer onion unwind to calling the full destroy function from the create one. Not a blocker. Daniele > + return PTR_ERR(ce); > + } > + i915_vm_put(ce->vm); > + ce->vm = i915_vm_get(&ppgtt->vm); > + > + strm_res->ce = ce; > + > + return 0; > +} > + > void intel_pxp_gsccs_fini(struct intel_pxp *pxp) > { > + struct gsccs_teelink_priv *gsccs = pxp_to_gsccs_priv(pxp); > + > + if (!gsccs) > + return; > + > + gsccs_destroy_execution_resource(pxp, &gsccs->arb_exec_res); > + kfree(gsccs); > + pxp->gsccs_priv = NULL; > } > > int intel_pxp_gsccs_init(struct intel_pxp *pxp) > { > + struct gsccs_teelink_priv *gsccs; > + int ret; > + > + gsccs = kzalloc(sizeof(*gsccs), GFP_KERNEL); > + if (!gsccs) > + return -ENOMEM; > + > + ret = gsccs_allocate_execution_resource(pxp, &gsccs->arb_exec_res); > + if (ret) { > + kfree(gsccs); > + return ret; > + } > + > + pxp->gsccs_priv = gsccs; > + > return 0; > } > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index 43aa61c26de5..fdcb9a66f691 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -32,6 +32,11 @@ struct intel_pxp { > */ > bool uses_gsccs; > > + /** > + * @gsccs_priv: GSC-CS based tee-link private context. > + */ > + void *gsccs_priv; > + > /** > * @pxp_component: i915_pxp_component struct of the bound mei_pxp > * module. Only set and cleared inside component bind/unbind functions,
Thanks for reviewing, Daniele. Responses are inline below. On Wed, 2023-01-18 at 15:55 -0800, Ceraolo Spurio, Daniele wrote: > > > > > > On 1/11/2023 1:42 PM, Alan Previn wrote: > > > > For MTL, PXP transport back-end uses the GSC engine to submit > > > > HECI packets for PXP arb session management. The command submission > > > > that uses non-priveleged mode requires us to allocate (or free) > > > > a set of execution submission resources (buffer-object, batch-buffer > > > > and context). Thus, do this one time allocation of resources in > > > > GSC-CS init and clean them up in fini. > > > > > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > > > --- > > > > Alan: [snip] > > > > +struct gsccs_session_resources { > > > > + struct mutex cmd_mutex; /* Protects submission for arb session */ > > > > + u64 host_session_handle; /* used by firmware to link commands to sessions */ > > > > + > > > > + struct intel_context *ce; /* context for gsc command submission */ > > > > + struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */ > > > > The arb session creation is a kernel submission, so can't you just use > > the default kernel ppgtt (i.e., gt->vm)? Alan: yes i can - will remove 'ppgtt' > > > > > > + > > > > + struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */ > > > > + struct i915_vma *pkt_vma; /* PXP HECI message packet vma */ > > > > + void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */ > > > > + > > > > + /* Buffer info for GSC engine batch buffer: */ > > > > + struct drm_i915_gem_object *bb_obj; /* batch buffer object */ > > > > + struct i915_vma *bb_vma; /* batch buffer vma */ > > > > + void *bb_vaddr; /* batch buffer virtual memory pointer */ > > > > You aren't actually making use of any of the variables in this struct in > > this patch, apart from initialization. Some of those are pretty clear on > > what they will be used for (e.g. the context), bu others are a bit more > > vague(e.g. the vaddrs). It'll probably be cleaner to reorder things so > > the more implementation-specific variables are added when they're used. > > I'll try to add more comment in follow-up patches. > > Alan: okay - will keep context and introduce the other members of this structure in the patches they get introduced in (..or not, depending on the subsequent review comments). > > > > +}; > > > > + > > > > +struct gsccs_teelink_priv { > > > > + /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */ > > > > + struct gsccs_session_resources arb_exec_res; > > > > +}; > > > > + > > > > +static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp) > > > > +{ > > > > + return (struct gsccs_teelink_priv *)pxp->gsccs_priv; > > > > +} > > > > Why do we need this layer of obfuscation with the void gsccs_priv? it's > > not like that can be assigned to anything else but > > gsccs_session_resources, so why not just have a pointer to that? > > If you want to have different priv based on the backend (mei vs gsccs) > > than it should be a more generic priv and be used in both cases. > > Alan: Agreed. I will just include that into the pxp structure. My original intention was to prepare for that abstraction of backends (like you mentioned, as per the original RFC) as well as to limit the build time impact on other pxp files when changing this structure. However given that the former would be a much bigger effort that ought to begin only after mtl-pxp is fully completed (as per offline discussions) and merged (for a proper "two-steps-back and absract"), and this structure really shouldnt change much at all moving forward after this series, i will re-rev as per your recommendation. > > > > > > > > int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > > > > int arb_session_id) > > > > @@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > > > > return -ENODEV; > > > > } > > > > > > > > +static void > > > > +gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma, > > > > + struct drm_i915_gem_object *obj) > > > > +{ > > > > + int err; > > > > + > > > > + i915_vma_unpin(vma); > > > > + err = i915_vma_unbind(vma); > > > > + if (err) > > > > + drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err); > > > > I don't think you need to explicitly call unbind here, it should be > > automatically covered by the object cleanup as long as it is unpinned Alan: Thanks - I will fix accordingly > > > > > > + > > > > + i915_gem_object_unpin_map(obj); > > > > + i915_gem_object_unpin_pages(obj); > > > > + i915_gem_object_put(obj); > > > > If you don't explicitly pin the pages (which you don't need to, see > > comment below) the whole cleanup in this function can be done with just: > > > > i915_vma_unpin_and_release(vma, I915_VMA_RELEASE_MAP); > > Alan: good catch - i missed that (my code was based off some selftest that had different goals). Will optimize this accordingly. > > > > +} > > > > + > > > > +static int > > > > +gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname, > > > > + size_t size, struct i915_ppgtt *ppgtt, > > > > personal preference: IMO better to pass a generic "i915_address_space > > *vm" to this function. Alan: sounds good - no prob. > > > > > > + struct drm_i915_gem_object **obj, > > > > + struct i915_vma **vma, void **map) > > > > +{ > > > > + int err = 0; > > > > + > > > > + *obj = i915_gem_object_create_internal(i915, size); > > > > + if (IS_ERR(*obj)) { > > > > + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname); > > > > + err = PTR_ERR(*obj); > > > > + goto out_none; > > > > + } > > > > + > > > > + *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL); > > > > + if (IS_ERR(*vma)) { > > > > + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname); > > > > + err = PTR_ERR(*vma); > > > > + goto out_put; > > > > + } > > > > + > > > > + err = i915_gem_object_pin_pages_unlocked(*obj); > > > > + if (err) { > > > > + drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname); > > > > + goto out_put; > > > > + } > > > > You're doing a vma_pin below, so no need to explicitly pin the pages > > here as the vma_pin will cover it. > > Also, do you need the object to be returned by the function? As > > mentioned above, the cleanup can be done with just the vma pointer and > > from a quick look I don't see the object being used in follow up patches. > > Alan: Yup - got it. Yes we don't reference object elsewhere. > > > > + > > > > + /* map to the virtual memory pointer */ > > > > + *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true)); > > > > + if (IS_ERR(*map)) { > > > > + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname); > > > > + err = PTR_ERR(*map); > > > > + goto out_unpin; > > > > + } > > > > Alan: [snip] > > > > +static int > > > > +gsccs_allocate_execution_resource(struct intel_pxp *pxp, > > > > + struct gsccs_session_resources *strm_res) > > > > +{ > > > > + struct intel_gt *gt = pxp->ctrl_gt; > > > > + struct intel_engine_cs *engine = gt->engine[GSC0]; > > > > + struct i915_ppgtt *ppgtt; > > > > + struct intel_context *ce; > > > > + int err = 0; > > > > + > > > > + /* > > > > + * First, ensure the GSC engine is present. > > > > + * NOTE: Backend should would only be called with the correct gt. > > > > typo: should would Alan: yup > > > > > > Alan: [snip] > > > > + /* > > > > + * TODO: Consider optimization of pre-populating batch buffer > > > > + * with the send-HECI instruction now at init and reuse through its life. > > > > + */ > > > > This looks like a nice optimization, and it would also allow us to drop > > the bb_vaddr variable from the struct. If the only heci message we're > > ever going to send with this object is the session creation, we can > > probably also fill the "send" section of the pkt. > > Alan: unfortunately, i have to remove that comment because we will be seeing the session-teardown coming up. As per the other series we are reviewing for ADL, i'll have to implement the same for MTL - but i want that to get reviewed and get the RB's first before i update this series to include it. > > > > + > > > > + /* Finally, create an intel_context to be used during the submission */ > > > > + ce = intel_context_create(engine); > > > > + if (IS_ERR(ce)) { > > > > + drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); > > > > + gsccs_destroy_execution_resource(pxp, strm_res); > > > > we usually prefer onion unwind to calling the full destroy function from > > the create one. Not a blocker. > > > > Daniele > > >
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..52b9a61bcdd4 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 @@ -10,7 +10,11 @@ #include "intel_pxp_cmd_interface_cmn.h" /* PXP-Cmd-Op definitions */ -#define PXP43_CMDID_START_HUC_AUTH 0x0000003A +#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 { diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c index 21400650fc86..97ca187e6fde 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -3,9 +3,41 @@ * Copyright(c) 2023 Intel Corporation. */ +#include "gem/i915_gem_internal.h" + +#include "gt/intel_context.h" + #include "i915_drv.h" -#include "intel_pxp_types.h" +#include "intel_pxp_cmd_interface_43.h" #include "intel_pxp_gsccs.h" +#include "intel_pxp_types.h" + +struct gsccs_session_resources { + struct mutex cmd_mutex; /* Protects submission for arb session */ + u64 host_session_handle; /* used by firmware to link commands to sessions */ + + struct intel_context *ce; /* context for gsc command submission */ + struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */ + + struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */ + struct i915_vma *pkt_vma; /* PXP HECI message packet vma */ + void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */ + + /* Buffer info for GSC engine batch buffer: */ + struct drm_i915_gem_object *bb_obj; /* batch buffer object */ + struct i915_vma *bb_vma; /* batch buffer vma */ + void *bb_vaddr; /* batch buffer virtual memory pointer */ +}; + +struct gsccs_teelink_priv { + /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */ + struct gsccs_session_resources arb_exec_res; +}; + +static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp) +{ + return (struct gsccs_teelink_priv *)pxp->gsccs_priv; +} int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id) @@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, return -ENODEV; } +static void +gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma, + struct drm_i915_gem_object *obj) +{ + int err; + + i915_vma_unpin(vma); + err = i915_vma_unbind(vma); + if (err) + drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err); + + i915_gem_object_unpin_map(obj); + i915_gem_object_unpin_pages(obj); + i915_gem_object_put(obj); +} + +static int +gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname, + size_t size, struct i915_ppgtt *ppgtt, + struct drm_i915_gem_object **obj, + struct i915_vma **vma, void **map) +{ + int err = 0; + + *obj = i915_gem_object_create_internal(i915, size); + if (IS_ERR(*obj)) { + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname); + err = PTR_ERR(*obj); + goto out_none; + } + + *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL); + if (IS_ERR(*vma)) { + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname); + err = PTR_ERR(*vma); + goto out_put; + } + + err = i915_gem_object_pin_pages_unlocked(*obj); + if (err) { + drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname); + goto out_put; + } + + /* map to the virtual memory pointer */ + *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true)); + if (IS_ERR(*map)) { + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname); + err = PTR_ERR(*map); + goto out_unpin; + } + + /* all PXP sessions commands are treated as non-priveleged */ + err = i915_vma_pin(*vma, 0, 0, PIN_USER); + if (err) { + drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname); + goto out_unmap; + } + + return 0; + +out_unmap: + i915_gem_object_unpin_map(*obj); +out_unpin: + i915_gem_object_unpin_pages(*obj); +out_put: + i915_gem_object_put(*obj); +out_none: + *obj = NULL; + *vma = NULL; + *map = NULL; + + return err; +} + +static void +gsccs_destroy_execution_resource(struct intel_pxp *pxp, + struct gsccs_session_resources *strm_res) +{ + if (strm_res->ce) + intel_context_put(strm_res->ce); + if (strm_res->bb_obj) + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->bb_vma, strm_res->bb_obj); + if (strm_res->pkt_obj) + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->pkt_vma, strm_res->pkt_obj); + if (strm_res->ppgtt) + i915_vm_put(&strm_res->ppgtt->vm); + + memset(strm_res, 0, sizeof(*strm_res)); +} + +static int +gsccs_allocate_execution_resource(struct intel_pxp *pxp, + struct gsccs_session_resources *strm_res) +{ + struct intel_gt *gt = pxp->ctrl_gt; + struct intel_engine_cs *engine = gt->engine[GSC0]; + struct i915_ppgtt *ppgtt; + struct intel_context *ce; + int err = 0; + + /* + * First, ensure the GSC engine is present. + * NOTE: Backend should would only be called with the correct gt. + */ + if (!engine) + return -ENODEV; + + mutex_init(&strm_res->cmd_mutex); + + ppgtt = i915_ppgtt_create(gt, 0); + if (IS_ERR(ppgtt)) + return PTR_ERR(ppgtt); + + strm_res->ppgtt = ppgtt; + + /* + * 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->i915, "Heci Packet", + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE, + strm_res->ppgtt, + &strm_res->pkt_obj, &strm_res->pkt_vma, + &strm_res->pkt_vaddr); + if (err) { + gsccs_destroy_execution_resource(pxp, strm_res); + return err; + } + + err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Batch Buffer", + PAGE_SIZE, strm_res->ppgtt, + &strm_res->bb_obj, &strm_res->bb_vma, + &strm_res->bb_vaddr); + if (err) { + gsccs_destroy_execution_resource(pxp, strm_res); + return err; + } + /* + * TODO: Consider optimization of pre-populating batch buffer + * with the send-HECI instruction now at init and reuse through its life. + */ + + /* Finally, create an intel_context to be used during the submission */ + ce = intel_context_create(engine); + if (IS_ERR(ce)) { + drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); + gsccs_destroy_execution_resource(pxp, strm_res); + return PTR_ERR(ce); + } + i915_vm_put(ce->vm); + ce->vm = i915_vm_get(&ppgtt->vm); + + strm_res->ce = ce; + + return 0; +} + void intel_pxp_gsccs_fini(struct intel_pxp *pxp) { + struct gsccs_teelink_priv *gsccs = pxp_to_gsccs_priv(pxp); + + if (!gsccs) + return; + + gsccs_destroy_execution_resource(pxp, &gsccs->arb_exec_res); + kfree(gsccs); + pxp->gsccs_priv = NULL; } int intel_pxp_gsccs_init(struct intel_pxp *pxp) { + struct gsccs_teelink_priv *gsccs; + int ret; + + gsccs = kzalloc(sizeof(*gsccs), GFP_KERNEL); + if (!gsccs) + return -ENOMEM; + + ret = gsccs_allocate_execution_resource(pxp, &gsccs->arb_exec_res); + if (ret) { + kfree(gsccs); + return ret; + } + + pxp->gsccs_priv = gsccs; + return 0; } diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h index 43aa61c26de5..fdcb9a66f691 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h @@ -32,6 +32,11 @@ struct intel_pxp { */ bool uses_gsccs; + /** + * @gsccs_priv: GSC-CS based tee-link private context. + */ + void *gsccs_priv; + /** * @pxp_component: i915_pxp_component struct of the bound mei_pxp * module. Only set and cleared inside component bind/unbind functions,
For MTL, PXP transport back-end uses the GSC engine to submit HECI packets for PXP arb session management. The command submission that uses non-priveleged mode requires us to allocate (or free) a set of execution submission resources (buffer-object, batch-buffer and context). Thus, do this one time allocation of resources in GSC-CS init and clean them up in fini. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 6 +- drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 216 +++++++++++++++++- drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 5 + 3 files changed, 225 insertions(+), 2 deletions(-)