On 12/5/2022 11:04 AM, Teres Alexis, Alan Previn wrote:
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.
Yup, can confirm. Might be worth adding a note to the commit message
that GT is now considered a soft-dependency of PXP and that the
suspend_late function calls have been re-ordered accordingly, but that
this re-order doesn't have any impact apart from code flow because at
that point in time there can't be any PXP events, so it doesn't really
matter when we disable the PXP HW (global irqs are off anyway, so even
if there was a bug that generated spurious event we wouldn't see it and
we would just clean it up on resume).
I also agree that moving calls between different suspend stages should
be left to a separate patch.
Daniele
i915_gem_suspend_late(dev_priv);
