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

[Ceraolo Spurio, Daniele]

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