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

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

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);
>