[Intel-gfx] [PATCH v7 1/1] drm/i915/pxp: Promote pxp subsystem to top-level of i915

[Teres Alexis, Alan Previn]

On Fri, 2022-12-02 at 11:22 -0500, Vivi, Rodrigo wrote:
> On Thu, Dec 01, 2022 at 05:14:07PM -0800, Alan Previn wrote:
> > Starting with MTL, there will be two GT-tiles, a render and media
> > tile. PXP as a service for supporting workloads with protected
> > contexts and protected buffers can be subscribed by process
> > workloads on any tile. However, depending on the platform,
> > only one of the tiles is used for control events pertaining to PXP
> > 
Alan: [snip]

> > @@ -1168,6 +1176,8 @@ static int i915_drm_prepare(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(dev);
> >  
> > +	intel_pxp_suspend_prepare(i915->pxp);
> > +
> >  	/*
> >  	 * NB intel_display_suspend() may issue new requests after we've
> >  	 * ostensibly marked the GPU as ready-to-sleep here. We need to
> > @@ -1248,6 +1258,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> >  
> >  	disable_rpm_wakeref_asserts(rpm);
> >  
> > +	intel_pxp_suspend(dev_priv->pxp);
> 
> is this really needed here in the suspend_late?
> why not in suspend()?
> 
> In general what is needed in suspend_late is resumed from the resume_early,
> what doesn't look the case here. So something looks off.
> 

Actually this patch is NOT changing the code flow of when these pxp pm functions are getting called from an i915-level
perspective - i am merely moving it from inside gt level to top level up:

Original --> i915_drm_suspend_late calls i915_gem_suspend_late calls intel_gt_suspend_late calls intel_pxp_suspend
Patch --> i915_drm_suspend_late calls intel_pxp_suspend (before calling i915_gem_suspend_late)

Putting that aside, i think the original code was designed to have the suspend-prepare do nearly everything except
disable the KCR registers which is postponed in order to handle a failed system-suspend-prepare (after pxp's suspend-
prepare). A failed system-suspend-prepare (after pxp's suspend-prepare) would be recoverable with no-op from pxp's
perspective as the UMD would be forced to recreate the pxp context that recreates arb session again and because the KCR
registers hadnt been disabled, nothing more is required. I'm not 100% sure so I'll ping Daniele jump in to correct me. 

That said, the better way, for code readibility, would be completely skip having an intel_pxp_suspend and just disable
the KCR in intel_pxp_suspend_prepare and instead add an i915 callback for resume_complete (which is the symmetrical
opposite of suspend_prepare and surprisingly non-existend in i915 codebase) in order to re-start kcr registers there for
the case of a failed-system-suspend-prepare or completion of resume. I have a separate series that is attempting to fix
some of this lack of symmetry here: https://patchwork.freedesktop.org/patch/513279/?series=111409&rev=1 but i hadn't
removed the intel_pxp_suspend because i am not sure if the i915-resume-complete callback would still be called if i915
itself was the reason for the failed suspend-prepare AND the pxp-suspend-prepare had occured. So i need to craft out a
way to test that.

Do you want to continue pursuing the final fixups for pxp's suspend-resume flows in this patch? Again, i am NOT changing
this flow - just moving it from inside-gt to outside gem-gt where for suspend we move it outside-and-before and for
resume we move it outside-and-after.

> > 
> > 

Alan: [snip]

> > +
> >  	i915_gem_suspend_late(dev_priv);
> >  
> >  	for_each_gt(gt, dev_priv, i)
> > +int intel_pxp_init(struct drm_i915_private *i915)
> > +{
> > +	struct intel_pxp *newpxp;
> > +
> > +	newpxp = kzalloc(sizeof(*newpxp), GFP_KERNEL);
> > +	if (!newpxp)
> > +		return -ENOMEM;
> > +
> > +	i915->pxp = newpxp;
> 
> i915->pxp is already an intel_pxp *, why can't we simply
> do 
> i915->pxp = kzalloc(sizeof(...
> 	  if (!i915->pxp)
> 	  return -ENOMEM;
> ?
> 
yes but i wanted to avoid using 'i915->pxp' for all the codes below and just use a local variable to keep it shorter.
But since that's what you prefer, I will respin accordingly.

> > +
> > +	newpxp->ctrl_gt = pxp_get_ctrl_gt(i915);
> > +	if (!newpxp->ctrl_gt)
> > +		return -ENODEV;
> >  
> >  	/*
> >  	 * 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))
> > -		pxp_init_full(pxp);
> > -	else if (intel_huc_is_loaded_by_gsc(&gt->uc.huc) && intel_uc_uses_huc(&gt->uc))
> > -		intel_pxp_tee_component_init(pxp);
> > +	if (intel_pxp_is_supported(newpxp))
> > +		pxp_init_full(newpxp);
> > +	else if (intel_huc_is_loaded_by_gsc(&newpxp->ctrl_gt->uc.huc) &&
> > +		 intel_uc_uses_huc(&newpxp->ctrl_gt->uc))
> > +		intel_pxp_tee_component_init(newpxp);
> > +
> > +	return 0;
> >  }
> > 

Alan: [snip]

> >  static inline struct intel_pxp *i915_dev_to_pxp(struct device *i915_kdev)
> >  {
> >  	struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> >  
> > -	return &to_gt(i915)->pxp;
> > +	return i915->pxp;
> 
> hmmm... now that we have pxp under i915, we should simply kill this function
> and move it to simply use i915->pxp.
> 
Alan: sure will remove this.