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