[Intel-gfx] [PATCH v3 1/6] drm/i915/pxp: Make gt and pxp init/fini aware of PXP-owning-GT

[Teres Alexis, Alan Previn]

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(&gt->uc.huc) && intel_uc_uses_huc(&gt->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);
> > 
>