| Message ID | 20220405093810.76613-1-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 03038d84ace72678a9944524508f218a00377dc0 |
| Headers | show |
| Series | PCI: Quirk Intel DG2 ASPM L1 acceptable latency to be unlimited | expand |
On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 > ASPM latency to be < 1us even though the hardware actually supports > higher latencies (> 64 us) just fine. In order to allow the links to go > into L1 and save power, quirk the acceptable L1 ASPM latency for these > endpoints to be unlimited. > > Note this does not have any effect unless the user requested the kernel > to enable ASPM in the first place (by default we don't enable it). This > is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" command > line parameters. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pci/quirks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index da829274fc66..e97b5daa00eb 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1533, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect); > + > +#ifdef CONFIG_PCIEASPM > +/* > + * Intel DG2 graphics card has hard-coded acceptable L1 latency that is > + * too low which prevents ASPM to be enabled. It does support ASPM L1 > + * and tolerates higher latencies so quirk it to be unlimited. > + */ > +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev) > +{ > + if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) { > + u32 devcap = dev->devcap; > + > + dev->devcap |= 7 << 9; > + pci_info(dev, "quirking devcap for L1 accepted latency 0x%08x -> 0x%08x\n", > + devcap, dev->devcap); > + } > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, quirk_aspm_accepted_l1_latency); > +#endif This matches our expectations and IDs. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > -- > 2.35.1 >
On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 > ASPM latency to be < 1us even though the hardware actually supports > higher latencies (> 64 us) just fine. In order to allow the links to go > into L1 and save power, quirk the acceptable L1 ASPM latency for these > endpoints to be unlimited. Is there a plan to fix this in future DG2 hardware/firmware? Obviously the point of Dev Cap is to make the device self-describing so we can avoid updates like this every time new hardware comes out. > Note this does not have any effect unless the user requested the kernel > to enable ASPM in the first place (by default we don't enable it). I think this depends on the platform and kernel config, doesn't it? If CONFIG_PCIEASPM_POWERSAVE=y or CONFIG_PCIEASPM_POWER_SUPERSAVE=y I suspect we would enable ASPM L1 even without the parameters below. > This is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" > command line parameters. s/powersupsersave/powersupersave/ This should affect "powersave" as well as "powersupersave", right? Both enable L1. "powersupersave" enables the L1 substates. We *should* be able to enable/disable ASPM L1 using the sysfs "l1_aspm file, too. > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/pci/quirks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index da829274fc66..e97b5daa00eb 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1533, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect); > + > +#ifdef CONFIG_PCIEASPM > +/* > + * Intel DG2 graphics card has hard-coded acceptable L1 latency that is > + * too low which prevents ASPM to be enabled. It does support ASPM L1 > + * and tolerates higher latencies so quirk it to be unlimited. > + */ > +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev) > +{ > + if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) { > + u32 devcap = dev->devcap; > + > + dev->devcap |= 7 << 9; > + pci_info(dev, "quirking devcap for L1 accepted latency 0x%08x -> 0x%08x\n", > + devcap, dev->devcap); > + } > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, quirk_aspm_accepted_l1_latency); > +#endif > -- > 2.35.1 >
On Tue, 2022-04-05 at 11:01 -0500, Bjorn Helgaas wrote: > On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > > Intel DG2 discrete graphics PCIe endpoints hard-code their > > acceptable L1 > > ASPM latency to be < 1us even though the hardware actually supports > > higher latencies (> 64 us) just fine. In order to allow the links > > to go > > into L1 and save power, quirk the acceptable L1 ASPM latency for > > these > > endpoints to be unlimited. > > Is there a plan to fix this in future DG2 hardware/firmware? > Obviously the point of Dev Cap is to make the device self-describing > so we can avoid updates like this every time new hardware comes out. Unfortunately no fix possible in DG2 hw (pci id list below), but it will be fixed for other platforms coming after those. > > > Note this does not have any effect unless the user requested the > > kernel > > to enable ASPM in the first place (by default we don't enable it). > > I think this depends on the platform and kernel config, doesn't it? > If CONFIG_PCIEASPM_POWERSAVE=y or CONFIG_PCIEASPM_POWER_SUPERSAVE=y > I suspect we would enable ASPM L1 even without the parameters below. > > > This is done with "pcie_aspm=force > > pcie_aspm.policy=powersupsersave" > > command line parameters. > > s/powersupsersave/powersupersave/ > > This should affect "powersave" as well as "powersupersave", right? > Both enable L1. "powersupersave" enables the L1 substates. > > We *should* be able to enable/disable ASPM L1 using the sysfs > "l1_aspm > file, too. > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/pci/quirks.c | 44 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index da829274fc66..e97b5daa00eb 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, > > 0x1533, rom_bar_overlap_defect); > > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, > > rom_bar_overlap_defect); > > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, > > rom_bar_overlap_defect); > > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, > > rom_bar_overlap_defect); > > + > > +#ifdef CONFIG_PCIEASPM > > +/* > > + * Intel DG2 graphics card has hard-coded acceptable L1 latency > > that is > > + * too low which prevents ASPM to be enabled. It does support ASPM > > L1 > > + * and tolerates higher latencies so quirk it to be unlimited. > > + */ > > +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev) > > +{ > > + if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) { > > + u32 devcap = dev->devcap; > > + > > + dev->devcap |= 7 << 9; > > + pci_info(dev, "quirking devcap for L1 accepted > > latency 0x%08x -> 0x%08x\n", > > + devcap, dev->devcap); > > + } > > +} > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, > > quirk_aspm_accepted_l1_latency); > > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, > > quirk_aspm_accepted_l1_latency); > > +#endif > > -- > > 2.35.1 > >
Hi Bjorn, On Tue, Apr 05, 2022 at 11:01:51AM -0500, Bjorn Helgaas wrote: > On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > > Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 > > ASPM latency to be < 1us even though the hardware actually supports > > higher latencies (> 64 us) just fine. In order to allow the links to go > > into L1 and save power, quirk the acceptable L1 ASPM latency for these > > endpoints to be unlimited. > > Is there a plan to fix this in future DG2 hardware/firmware? > Obviously the point of Dev Cap is to make the device self-describing > so we can avoid updates like this every time new hardware comes out. Yes, I think that's the plan. > > Note this does not have any effect unless the user requested the kernel > > to enable ASPM in the first place (by default we don't enable it). > > I think this depends on the platform and kernel config, doesn't it? > If CONFIG_PCIEASPM_POWERSAVE=y or CONFIG_PCIEASPM_POWER_SUPERSAVE=y > I suspect we would enable ASPM L1 even without the parameters below. > > > This is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" > > command line parameters. > > s/powersupsersave/powersupersave/ > > This should affect "powersave" as well as "powersupersave", right? > Both enable L1. "powersupersave" enables the L1 substates. > > We *should* be able to enable/disable ASPM L1 using the sysfs "l1_aspm > file, too. Indeed you are right. I think we can drop that paragraph completely from the commit log. Do you want me to send v2 with that corrected or you will do that while applying? Thanks!
On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 > ASPM latency to be < 1us even though the hardware actually supports > higher latencies (> 64 us) just fine. In order to allow the links to go > into L1 and save power, quirk the acceptable L1 ASPM latency for these > endpoints to be unlimited. > > Note this does not have any effect unless the user requested the kernel > to enable ASPM in the first place (by default we don't enable it). This > is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" command > line parameters. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> I wordsmithed as below and applied to pci/aspm for v5.19, thanks! Please double-check that I didn't screw up the FIELD_GET/PREP usage. > --- > drivers/pci/quirks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index da829274fc66..e97b5daa00eb 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1533, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect); > DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect); > + > +#ifdef CONFIG_PCIEASPM > +/* > + * Intel DG2 graphics card has hard-coded acceptable L1 latency that is > + * too low which prevents ASPM to be enabled. It does support ASPM L1 > + * and tolerates higher latencies so quirk it to be unlimited. > + */ > +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev) > +{ > + if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) { > + u32 devcap = dev->devcap; > + > + dev->devcap |= 7 << 9; > + pci_info(dev, "quirking devcap for L1 accepted latency 0x%08x -> 0x%08x\n", > + devcap, dev->devcap); > + } > +} > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, quirk_aspm_accepted_l1_latency); > +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, quirk_aspm_accepted_l1_latency); > +#endif commit 03038d84ace7 ("PCI/ASPM: Make Intel DG2 L1 acceptable latency unlimited") Author: Mika Westerberg <mika.westerberg@linux.intel.com> Date: Tue Apr 5 12:38:10 2022 +0300 PCI/ASPM: Make Intel DG2 L1 acceptable latency unlimited Intel DG2 discrete graphics PCIe endpoints advertise L1 acceptable exit latency to be < 1us even though they can actually tolerate unlimited exit latencies just fine. Quirk the L1 acceptable exit latency for these endpoints to be unlimited so ASPM L1 can be enabled. [bhelgaas: use FIELD_GET/FIELD_PREP, wordsmith comment & commit log] Link: https://lore.kernel.org/r/20220405093810.76613-1-mika.westerberg@linux.intel.com Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index da829274fc66..41aeaa235132 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -12,6 +12,7 @@ * file, where their drivers can use them. */ +#include <linux/bitfield.h> #include <linux/types.h> #include <linux/kernel.h> #include <linux/export.h> @@ -5895,3 +5896,49 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1533, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect); + +#ifdef CONFIG_PCIEASPM +/* + * Several Intel DG2 graphics devices advertise that they can only tolerate + * 1us latency when transitioning from L1 to L0, which may prevent ASPM L1 + * from being enabled. But in fact these devices can tolerate unlimited + * latency. Override their Device Capabilities value to allow ASPM L1 to + * be enabled. + */ +static void aspm_l1_acceptable_latency(struct pci_dev *dev) +{ + u32 l1_lat = FIELD_GET(PCI_EXP_DEVCAP_L1, dev->devcap); + + if (l1_lat < 7) { + dev->devcap |= FIELD_PREP(PCI_EXP_DEVCAP_L1, 7); + pci_info(dev, "ASPM: overriding L1 acceptable latency from %#x to 0x7\n", + l1_lat); + } +} +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency); +#endif
On Thu, Apr 07, 2022 at 12:06:55PM -0500, Bjorn Helgaas wrote: > On Tue, Apr 05, 2022 at 12:38:10PM +0300, Mika Westerberg wrote: > > Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 > > ASPM latency to be < 1us even though the hardware actually supports > > higher latencies (> 64 us) just fine. In order to allow the links to go > > into L1 and save power, quirk the acceptable L1 ASPM latency for these > > endpoints to be unlimited. > > > > Note this does not have any effect unless the user requested the kernel > > to enable ASPM in the first place (by default we don't enable it). This > > is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" command > > line parameters. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > I wordsmithed as below and applied to pci/aspm for v5.19, thanks! Thanks! > Please double-check that I didn't screw up the FIELD_GET/PREP usage. Looks good to me. I did not even know we have such macros, thanks for pointing that out :)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index da829274fc66..e97b5daa00eb 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5895,3 +5895,47 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1533, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1536, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1537, rom_bar_overlap_defect); DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x1538, rom_bar_overlap_defect); + +#ifdef CONFIG_PCIEASPM +/* + * Intel DG2 graphics card has hard-coded acceptable L1 latency that is + * too low which prevents ASPM to be enabled. It does support ASPM L1 + * and tolerates higher latencies so quirk it to be unlimited. + */ +static void quirk_aspm_accepted_l1_latency(struct pci_dev *dev) +{ + if ((dev->devcap & PCI_EXP_DEVCAP_L1) >> 9 < 7) { + u32 devcap = dev->devcap; + + dev->devcap |= 7 << 9; + pci_info(dev, "quirking devcap for L1 accepted latency 0x%08x -> 0x%08x\n", + devcap, dev->devcap); + } +} +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f80, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f81, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f82, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f83, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f84, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f85, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f86, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f87, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x4f88, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5690, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5691, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5692, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5693, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5694, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x5695, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a0, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a1, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a2, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a3, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a4, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a5, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56a6, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b0, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, quirk_aspm_accepted_l1_latency); +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, quirk_aspm_accepted_l1_latency); +#endif
Intel DG2 discrete graphics PCIe endpoints hard-code their acceptable L1 ASPM latency to be < 1us even though the hardware actually supports higher latencies (> 64 us) just fine. In order to allow the links to go into L1 and save power, quirk the acceptable L1 ASPM latency for these endpoints to be unlimited. Note this does not have any effect unless the user requested the kernel to enable ASPM in the first place (by default we don't enable it). This is done with "pcie_aspm=force pcie_aspm.policy=powersupsersave" command line parameters. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/pci/quirks.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)