On Mon, 2022-11-14 at 20:00 -0800, Ceraolo Spurio, Daniele wrote:
>
> On 10/21/2022 10:39 AM, Alan Previn wrote:
> > In preparation for future MTL-PXP feature support, PXP control
> > @@ -142,22 +166,21 @@ void intel_pxp_init(struct intel_pxp *pxp)
> > {
> > struct intel_gt *gt = pxp_to_gt(pxp);
> >
> > - /* we rely on the mei PXP module */
> > - if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP))
> > - return;
> > -
> > /*
> > * If HuC is loaded by GSC but PXP is disabled, we can skip the init of
> > * the full PXP session/object management and just init the tee channel.
> > */
> > - if (HAS_PXP(gt->i915))
> > + if (_gt_supports_pxp(gt))
> > pxp_init_full(pxp);
> > - else if (intel_huc_is_loaded_by_gsc(>->uc.huc) &&
> > intel_uc_uses_huc(>->uc))
> > + else if (_gt_needs_teelink(gt))
> > intel_pxp_tee_component_init(pxp);
> > }
> >
> > void intel_pxp_fini(struct intel_pxp *pxp)
> > {
> > + if (!intel_gtpxp_is_supported(pxp))
> > + return;
>
> Why do you need this? the fini below should already be smart enough to
> only cleanup when needed.
Eventually i plan to create a backend abstraction for tee based vs mtl's gscccs
based and rather keep as much of the
checking on the front end to keep it cleaner.
>
> > +
> > pxp->arb_is_valid = false;
> >
> > intel_pxp_tee_component_fini(pxp);
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > index 2da309088c6d..c12e4d419c78 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> > @@ -13,6 +13,8 @@ struct intel_pxp;
> > struct drm_i915_gem_object;
> >
> > struct intel_gt *pxp_to_gt(const struct intel_pxp *pxp);
> > +bool intel_gtpxp_is_supported(struct intel_pxp *pxp);
> > +
> > bool intel_pxp_is_enabled(const struct intel_pxp *pxp);
> > bool intel_pxp_is_active(const struct intel_pxp *pxp);
> >
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 4359e8be4101..124663cf0047 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -70,7 +70,7 @@ void intel_pxp_debugfs_register(struct intel_pxp *pxp,
> > struct dentry *gt_root)
> > if (!gt_root)
> > return;
> >
> > - if (!HAS_PXP((pxp_to_gt(pxp)->i915)))
> > + if (!intel_gtpxp_is_supported(pxp))
> > return;
> >
>
> This now returns true for DG2, but we don't want to register the PXP
> debugfs there as we don't support PXP aside from HuC loading.
>
yeah - ok.
> IMO a
> better approach would be to have intel_gtpxp_is_supported be what you
> currently have as _gt_supports_pxp().
>
Okay, let me take a look at that since i recall that future patches would rely
on intel_gtpxp_is_supported for the case
where PXP is not supported but we just want to know if GT has backend tee for
component binding or something - but i
guess that could get a separate function as opposed to reusing
intel_gtpxp_is_supported.
> Also, intel_gtpxp_is_supported is a bit confusing because of the new
> "gtpxp" prefix. Why not use just intel_pxp_is_supported? We already have
> per-gt checkers that refer only to the subcomponent, like
> intel_huc_is_supported(), which for MTL is false on the primary GT and
> true on the media one. I don't see why we can't do the same for PXP.
I think that existing method isn't a good way - i rather use this opportunity
to set a precendence for pxp we can have a
more standardized naming convention based on the global-vs-per-gt level
checking (i also wish i had time to look at
"intra-vs-inter function naming). So when something is called with _pxp_ its
meant to be called as a global check
(passing in i915 as its param) and if its using _gtpxp_, then its meant to be
called as gt-level checker. And the
similar function name should be okay if the check is similar (just at different
hierarchy level). I prefer my way
because it allows that understanding purely from the function name as opposed
to having to look at the full definition
before knowing if its a global check vs a gt level check. I think we really
ought to have a more concise naming
convention as opposed to "we do it like this, so why not just follow". An
alternative would be instead of
"intel_gtpxp_is_supported" then "intel_gt_supports_pxp".
>
> Daniele
>
> > root = debugfs_create_dir("pxp", gt_root);
>
