On Mon, 2022-12-05 at 12:57 -0500, Vivi, Rodrigo wrote:
> On Fri, Dec 02, 2022 at 03:28:21PM -0800, Alan Previn wrote:
> >
> >
> >
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);
> > +
>
> Before this patch the pxp_suspend was done right after uc_suspend.
> Right now, the uc_suspend will happen couple lines below.
>
>
>
Okay - I see this 2nd level flow change but this has no functional change. I have gone through the codes and whether
intel_pxp_suspend came in after i915_gem_suspend_late or the middle of gt_suspend_late (after us_suspend), it does not
make any difference. intel_pxp_suspend only disables display-control-events on KCR register and disables CRYPTO-IRQ
registers. GuC or HuC is only ever doing any PXP related work if it receives workloads via exec-buf (kernel side PXP
workload subsmission is limited to arb-session creation today OR, in future, teardown-flows at suspend_prepare - this is
upcoming change in flight). That said, since the GT is already quiesced in suspend_prepare, therefore HuC or GuC should
be not doing anything inside of i915_suspend_late whether its before i915_gem_suspend_late or before uc_suspend.
> If this is okay, why can't we move already the pxp_suspend to the
> suspend function and need to keep this in the suspend late?
>
We can - but i dont see any difference in doing so functionally speaking - i do however prefer minimizing the code flow
changes to
> Or we don't change the flow at all, or we already fix the unbalanced
> thing.
> I believe the first option is better and changing the flow in a
> separated patch is better.
>
Actually, I rather fix the symmetry at the this level: As part of this feature to promote PXP - Gt becomes a dependency
of PXP. So at init, we ensure GT is init first and then we init PXP. Therefore we should do the opposite for fini -
ensure PXP fini is done first and the cleanup the GT. That why the move above. But as the global i915 level we are
keeping it within suspend_late - after everything was quiesced in suspend_prepare.
> Specially because I don't understand if huc has any dependency on
> the pxp. Maybe that was the original reason why that function end up there
> at first place.
>
> I believe Daniele is the right one to let us know that.
>
I spoke to Daniele and he confirmed what i explained above.
> > i915_gem_suspend_late(dev_priv);
> >
> >
