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

[Vivi, Rodrigo]

On Fri, 2022-12-02 at 19:21 +0000, Teres Alexis, Alan Previn wrote:
> 
> 
> 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&r
> ev=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.

Oh! okay, let's move without changing this flow in this patch and work
in a follow up.

> 
> > > 
> > > 
> 
> 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.
>