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