just recapping offline conversation summary - we agreed on:
intel_pxp_enabled(i915)
intel_pxp_enabled_on_gt(pxp)
(where one is wrapper over the other, the action part of the function name stays the same).
...alan
On Mon, 2022-11-14 at 21:13 -0800, Alan Previn Teres Alexis wrote:
>
> 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);
> >
>
