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