Re: [PATCH v7 2/2] PCI: Don't put non-power manageable PCIe root ports into D3

[Mario Limonciello]

On 8/2/23 09:31, Mika Westerberg wrote:
> Hi,
>
> On Wed, Aug 02, 2023 at 09:10:38AM -0500, Mario Limonciello wrote:
> > On 8/2/23 00:26, Mika Westerberg wrote:
> > > Hi Mario,
> > >
> > > On Tue, Aug 01, 2023 at 10:17:11PM -0500, Mario Limonciello wrote:
> > > > > Consequently, platform_pci_bridge_d3() will return false and the only
> > > > > thing that may allow the port to go into D0 is the dmi_get_bios_year()
> > > > > check at the end of pci_bridge_d3_possible().
> > > > >
> > > > > However, that was added, because there are Intel platforms on which
> > > > > Root Ports need to be programmed into D3hot on suspend (which allows
> > > > > the whole platform to reduce power significantly) and there are no
> > > > > ACPI device power management objects associated with them (Mika should
> > > > > know the gory details related to this).  It looks like under Windows
> > > > > the additional power reduction would not be possible on those systems,
> > > > > but that would be a problem, wouldn't it?
> > > >
> > > > I've been thinking on this today, and I at least have a hypothesis about
> > > > this behavior.  Perhaps Windows is actually utilizing enabled PEP
> > > > constraints to enforce what state device should be put into over Modern
> > > > Standby cycles in the absence of ACPI objects.
> > > >
> > > > In the case of one of my problematic system the PEP constraints for the root
> > > > port are:
> > > >
> > > > Package (0x04)
> > > > {
> > > > 	0x00,
> > > > 	"\\_SB.PCI0.GP17",
> > > > 	0x00,
> > > > 	0x00
> > > > },
> > > >
> > > > That first 0x00 means the constraint isn't actually enabled for the root
> > > > port.
> > > >
> > > > Mika,
> > > >
> > > > Could you get an acpidump from one of these problematic Intel systems so we
> > > > can check the PEP constraints to see if this theory works? Or maybe you have
> > > > some other ideas why this is different?
> > >
> > > The patch adding this was merged in 2016 and unfortunately I don't have
> > > any of the ACPI dumps from them available anymore (and do not recall the
> > > details either). I think these were Apollo Lake-P based systems with the
> > > initial runtime D3cold and S0ix support at the time.
> >
> >
> > I scoured the web looking for acpidumps a bit an Apollo Lake system and came
> > across this random bug report:
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1591307
> >
> > "Intel(R) Celeron(R) CPU N3450 @ 1.10GHz (family: 0x6, model: 0x5c,
> > stepping: 0x9)"
> >
> > I looked at the acpidump, and I notice:
> >
> > Low Power S0 Idle (V5) : 0
> >
> > That means that Windows wouldn't actually be putting it into Modern Standby
> > at suspend but would rather use S3.
>
> Same goes for Linux AFAICT. The ones needed this actually used S0ix so
> the bit should definitely be set.

OK.

> > Considering that result, could we perhaps adjust the check to:
> >
> > if ((c->x86_vendor == X86_VENDOR_INTEL) && !(acpi_gbl_FADT.flags &
> > ACPI_FADT_LOW_POWER_S0))
> >
> > Or could we quirk the PCI root ports from Apollo Lake to opt into D3?
>
> It is not just Apollo Lake, but all "modern" systems as well (sorry if
> this was unclear). Apollo Lake just was the first one that needed this.
> We also have the Low Power S0 Idle bit set in recent systems too.

Ah got it; I misunderstood it as Apollo Lake was the only one that 
needed it.

So modern systems that set the bit in the FADT, do they also lack _S0W 
and _S0D on the root ports?

Does my PEP constraints theory hold steam at all?