| Message ID | 20220511122806.2146847-2-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] igb: Remove duplicate defines | expand |
Dear Kai-Hang, Thank you for the patch. Am 11.05.22 um 14:28 schrieb Kai-Heng Feng: > We found Intel I210 can only achieve ~750Mbps Tx speed on some > platforms. The RR2DCDELAY shows around 0x2xxx DMA delay, which will be Please give an example platform, where it works and where it does not. How did you test transfer speed? > significantly lower when 1) ASPM is disabled or 2) SoC package c-state > stays above PC3. When the RR2DCDELAY is around 0x1xxx the Tx speed can > reach to ~950Mbps. > > According to the I210 datasheet "8.26.1 PCIe Misc. Register - PCIEMISC", > "DMA Idle Indication" doesn't seem to tie to DMA coalesce anymore, so > set it to 1b for "DMA is considered idle when there is no Rx or Tx AND > when there are no TLPs indicating that CPU is active detected on the > PCIe link (such as the host executes CSR or Configuration register read > or write operation)" and performing Tx should also fall under "active > CPU on PCIe link" case. > > In addition to that, commit b6e0c419f040 ("igb: Move DMA Coalescing init > code to separate function.") seems to wrongly changed from enabling > E1000_PCIEMISC_LX_DECISION to disabling it, also fix that. Please split this into a separate commit with Fixes tag, and maybe the commit author in Cc. Kind regards, Paul > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index 34b33b21e0dcd..eca797dded429 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -9897,11 +9897,10 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > struct e1000_hw *hw = &adapter->hw; > u32 dmac_thr; > u16 hwm; > + u32 reg; > > if (hw->mac.type > e1000_82580) { > if (adapter->flags & IGB_FLAG_DMAC) { > - u32 reg; > - > /* force threshold to 0. */ > wr32(E1000_DMCTXTH, 0); > > @@ -9934,7 +9933,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > /* Disable BMC-to-OS Watchdog Enable */ > if (hw->mac.type != e1000_i354) > reg &= ~E1000_DMACR_DC_BMC2OSW_EN; > - > wr32(E1000_DMACR, reg); > > /* no lower threshold to disable > @@ -9951,12 +9949,12 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > */ > wr32(E1000_DMCTXTH, (IGB_MIN_TXPBSIZE - > (IGB_TX_BUF_4096 + adapter->max_frame_size)) >> 6); > + } > > - /* make low power state decision controlled > - * by DMA coal > - */ > + if (hw->mac.type >= e1000_i210 || > + (adapter->flags & IGB_FLAG_DMAC)) { > reg = rd32(E1000_PCIEMISC); > - reg &= ~E1000_PCIEMISC_LX_DECISION; > + reg |= E1000_PCIEMISC_LX_DECISION; > wr32(E1000_PCIEMISC, reg); > } /* endif adapter->dmac is not disabled */ > } else if (hw->mac.type == e1000_82580) {
Hi Paul, On Wed, May 11, 2022 at 8:49 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Kai-Hang, > > > Thank you for the patch. > > > Am 11.05.22 um 14:28 schrieb Kai-Heng Feng: > > We found Intel I210 can only achieve ~750Mbps Tx speed on some > > platforms. The RR2DCDELAY shows around 0x2xxx DMA delay, which will be > > Please give an example platform, where it works and where it does not. The platform is about but not yet hit the market yet, so I can't disclose it. They are Intel Alder Lake based. > > How did you test transfer speed? Iperf. > > > significantly lower when 1) ASPM is disabled or 2) SoC package c-state > > stays above PC3. When the RR2DCDELAY is around 0x1xxx the Tx speed can > > reach to ~950Mbps. > > > > According to the I210 datasheet "8.26.1 PCIe Misc. Register - PCIEMISC", > > "DMA Idle Indication" doesn't seem to tie to DMA coalesce anymore, so > > set it to 1b for "DMA is considered idle when there is no Rx or Tx AND > > when there are no TLPs indicating that CPU is active detected on the > > PCIe link (such as the host executes CSR or Configuration register read > > or write operation)" and performing Tx should also fall under "active > > CPU on PCIe link" case. > > > > In addition to that, commit b6e0c419f040 ("igb: Move DMA Coalescing init > > code to separate function.") seems to wrongly changed from enabling > > E1000_PCIEMISC_LX_DECISION to disabling it, also fix that. > > Please split this into a separate commit with Fixes tag, and maybe the > commit author in Cc. I don't see the need to split to separate commit as both require the same change. I will add the "Fixes" tag once the igb maintainers reviewed the patch. Kai-Heng > > > Kind regards, > > Paul > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > index 34b33b21e0dcd..eca797dded429 100644 > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > @@ -9897,11 +9897,10 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > struct e1000_hw *hw = &adapter->hw; > > u32 dmac_thr; > > u16 hwm; > > + u32 reg; > > > > if (hw->mac.type > e1000_82580) { > > if (adapter->flags & IGB_FLAG_DMAC) { > > - u32 reg; > > - > > /* force threshold to 0. */ > > wr32(E1000_DMCTXTH, 0); > > > > @@ -9934,7 +9933,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > /* Disable BMC-to-OS Watchdog Enable */ > > if (hw->mac.type != e1000_i354) > > reg &= ~E1000_DMACR_DC_BMC2OSW_EN; > > - > > wr32(E1000_DMACR, reg); > > > > /* no lower threshold to disable > > @@ -9951,12 +9949,12 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > */ > > wr32(E1000_DMCTXTH, (IGB_MIN_TXPBSIZE - > > (IGB_TX_BUF_4096 + adapter->max_frame_size)) >> 6); > > + } > > > > - /* make low power state decision controlled > > - * by DMA coal > > - */ > > + if (hw->mac.type >= e1000_i210 || > > + (adapter->flags & IGB_FLAG_DMAC)) { > > reg = rd32(E1000_PCIEMISC); > > - reg &= ~E1000_PCIEMISC_LX_DECISION; > > + reg |= E1000_PCIEMISC_LX_DECISION; > > wr32(E1000_PCIEMISC, reg); > > } /* endif adapter->dmac is not disabled */ > > } else if (hw->mac.type == e1000_82580) {
On Thu, May 12, 2022 at 10:55 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: > > Hi Paul, > > On Wed, May 11, 2022 at 8:49 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > > > Dear Kai-Hang, > > > > > > Thank you for the patch. > > > > > > Am 11.05.22 um 14:28 schrieb Kai-Heng Feng: > > > We found Intel I210 can only achieve ~750Mbps Tx speed on some > > > platforms. The RR2DCDELAY shows around 0x2xxx DMA delay, which will be > > > > Please give an example platform, where it works and where it does not. > > The platform is about but not yet hit the market yet, so I can't disclose it. > They are Intel Alder Lake based. > > > > > How did you test transfer speed? > > Iperf. > > > > > > significantly lower when 1) ASPM is disabled or 2) SoC package c-state > > > stays above PC3. When the RR2DCDELAY is around 0x1xxx the Tx speed can > > > reach to ~950Mbps. > > > > > > According to the I210 datasheet "8.26.1 PCIe Misc. Register - PCIEMISC", > > > "DMA Idle Indication" doesn't seem to tie to DMA coalesce anymore, so > > > set it to 1b for "DMA is considered idle when there is no Rx or Tx AND > > > when there are no TLPs indicating that CPU is active detected on the > > > PCIe link (such as the host executes CSR or Configuration register read > > > or write operation)" and performing Tx should also fall under "active > > > CPU on PCIe link" case. > > > > > > In addition to that, commit b6e0c419f040 ("igb: Move DMA Coalescing init > > > code to separate function.") seems to wrongly changed from enabling > > > E1000_PCIEMISC_LX_DECISION to disabling it, also fix that. > > > > Please split this into a separate commit with Fixes tag, and maybe the > > commit author in Cc. > > I don't see the need to split to separate commit as both require the > same change. > > I will add the "Fixes" tag once the igb maintainers reviewed the patch. A gentle ping... Please let me know if this is a proper fix so I can send v2. Kai-Heng > > Kai-Heng > > > > > > > Kind regards, > > > > Paul > > > > > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > --- > > > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- > > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > > > index 34b33b21e0dcd..eca797dded429 100644 > > > --- a/drivers/net/ethernet/intel/igb/igb_main.c > > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > > > @@ -9897,11 +9897,10 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > > struct e1000_hw *hw = &adapter->hw; > > > u32 dmac_thr; > > > u16 hwm; > > > + u32 reg; > > > > > > if (hw->mac.type > e1000_82580) { > > > if (adapter->flags & IGB_FLAG_DMAC) { > > > - u32 reg; > > > - > > > /* force threshold to 0. */ > > > wr32(E1000_DMCTXTH, 0); > > > > > > @@ -9934,7 +9933,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > > /* Disable BMC-to-OS Watchdog Enable */ > > > if (hw->mac.type != e1000_i354) > > > reg &= ~E1000_DMACR_DC_BMC2OSW_EN; > > > - > > > wr32(E1000_DMACR, reg); > > > > > > /* no lower threshold to disable > > > @@ -9951,12 +9949,12 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) > > > */ > > > wr32(E1000_DMCTXTH, (IGB_MIN_TXPBSIZE - > > > (IGB_TX_BUF_4096 + adapter->max_frame_size)) >> 6); > > > + } > > > > > > - /* make low power state decision controlled > > > - * by DMA coal > > > - */ > > > + if (hw->mac.type >= e1000_i210 || > > > + (adapter->flags & IGB_FLAG_DMAC)) { > > > reg = rd32(E1000_PCIEMISC); > > > - reg &= ~E1000_PCIEMISC_LX_DECISION; > > > + reg |= E1000_PCIEMISC_LX_DECISION; > > > wr32(E1000_PCIEMISC, reg); > > > } /* endif adapter->dmac is not disabled */ > > > } else if (hw->mac.type == e1000_82580) {
> >On Thu, May 12, 2022 at 10:55 AM Kai-Heng Feng <kai.heng.feng@canonical.com> wrote: >> >> Hi Paul, >> >> On Wed, May 11, 2022 at 8:49 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: >> > >> > Dear Kai-Hang, >> > >> > >> > Thank you for the patch. >> > >> > >> > Am 11.05.22 um 14:28 schrieb Kai-Heng Feng: >> > > We found Intel I210 can only achieve ~750Mbps Tx speed on some >> > > platforms. The RR2DCDELAY shows around 0x2xxx DMA delay, which >> > > will be >> > >> > Please give an example platform, where it works and where it does not. >> >> The platform is about but not yet hit the market yet, so I can't disclose it. >> They are Intel Alder Lake based. >> >> > >> > How did you test transfer speed? >> >> Iperf. >> >> > >> > > significantly lower when 1) ASPM is disabled or 2) SoC package >> > > c-state stays above PC3. When the RR2DCDELAY is around 0x1xxx the >> > > Tx speed can reach to ~950Mbps. >> > > >> > > According to the I210 datasheet "8.26.1 PCIe Misc. Register - >> > > PCIEMISC", "DMA Idle Indication" doesn't seem to tie to DMA >> > > coalesce anymore, so set it to 1b for "DMA is considered idle when >> > > there is no Rx or Tx AND when there are no TLPs indicating that >> > > CPU is active detected on the PCIe link (such as the host executes >> > > CSR or Configuration register read or write operation)" and >> > > performing Tx should also fall under "active CPU on PCIe link" case. >> > > >> > > In addition to that, commit b6e0c419f040 ("igb: Move DMA >> > > Coalescing init code to separate function.") seems to wrongly >> > > changed from enabling E1000_PCIEMISC_LX_DECISION to disabling it, also fix that. >> > >> > Please split this into a separate commit with Fixes tag, and maybe >> > the commit author in Cc. >> >> I don't see the need to split to separate commit as both require the >> same change. >> >> I will add the "Fixes" tag once the igb maintainers reviewed the patch. > >A gentle ping... > >Please let me know if this is a proper fix so I can send v2. > >Kai-Heng > Hi, Looking good to me. Waiting for v2 version with fixes tag and then this will be fully accepted. Regards, Mateusz >> >> Kai-Heng >> >> > >> > >> > Kind regards, >> > >> > Paul >> > >> > >> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> > > --- >> > > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- >> > > 1 file changed, 5 insertions(+), 7 deletions(-) >> > > >> > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c >> > > b/drivers/net/ethernet/intel/igb/igb_main.c >> > > index 34b33b21e0dcd..eca797dded429 100644 >> > > --- a/drivers/net/ethernet/intel/igb/igb_main.c >> > > +++ b/drivers/net/ethernet/intel/igb/igb_main.c >> > > @@ -9897,11 +9897,10 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) >> > > struct e1000_hw *hw = &adapter->hw; >> > > u32 dmac_thr; >> > > u16 hwm; >> > > + u32 reg; >> > > >> > > if (hw->mac.type > e1000_82580) { >> > > if (adapter->flags & IGB_FLAG_DMAC) { >> > > - u32 reg; >> > > - >> > > /* force threshold to 0. */ >> > > wr32(E1000_DMCTXTH, 0); >> > > >> > > @@ -9934,7 +9933,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) >> > > /* Disable BMC-to-OS Watchdog Enable */ >> > > if (hw->mac.type != e1000_i354) >> > > reg &= ~E1000_DMACR_DC_BMC2OSW_EN; >> > > - >> > > wr32(E1000_DMACR, reg); >> > > >> > > /* no lower threshold to disable @@ -9951,12 >> > > +9949,12 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) >> > > */ >> > > wr32(E1000_DMCTXTH, (IGB_MIN_TXPBSIZE - >> > > (IGB_TX_BUF_4096 + >> > > adapter->max_frame_size)) >> 6); >> > > + } >> > > >> > > - /* make low power state decision controlled >> > > - * by DMA coal >> > > - */ >> > > + if (hw->mac.type >= e1000_i210 || >> > > + (adapter->flags & IGB_FLAG_DMAC)) { >> > > reg = rd32(E1000_PCIEMISC); >> > > - reg &= ~E1000_PCIEMISC_LX_DECISION; >> > > + reg |= E1000_PCIEMISC_LX_DECISION; >> > > wr32(E1000_PCIEMISC, reg); >> > > } /* endif adapter->dmac is not disabled */ >> > > } else if (hw->mac.type == e1000_82580) { > >
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Palczewski, Mateusz > Sent: Wednesday, May 25, 2022 1:43 PM > To: Kai-Heng Feng <kai.heng.feng@canonical.com>; Paul Menzel > <pmenzel@molgen.mpg.de>; intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni > <pabeni@redhat.com>; David S. Miller <davem@davemloft.net> > Subject: Re: [Intel-wired-lan] [PATCH 2/2] igb: Make DMA faster when CPU is > active on the PCIe link > > drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > Tested-by: Gurucharan <gurucharanx.g@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 34b33b21e0dcd..eca797dded429 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -9897,11 +9897,10 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) struct e1000_hw *hw = &adapter->hw; u32 dmac_thr; u16 hwm; + u32 reg; if (hw->mac.type > e1000_82580) { if (adapter->flags & IGB_FLAG_DMAC) { - u32 reg; - /* force threshold to 0. */ wr32(E1000_DMCTXTH, 0); @@ -9934,7 +9933,6 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) /* Disable BMC-to-OS Watchdog Enable */ if (hw->mac.type != e1000_i354) reg &= ~E1000_DMACR_DC_BMC2OSW_EN; - wr32(E1000_DMACR, reg); /* no lower threshold to disable @@ -9951,12 +9949,12 @@ static void igb_init_dmac(struct igb_adapter *adapter, u32 pba) */ wr32(E1000_DMCTXTH, (IGB_MIN_TXPBSIZE - (IGB_TX_BUF_4096 + adapter->max_frame_size)) >> 6); + } - /* make low power state decision controlled - * by DMA coal - */ + if (hw->mac.type >= e1000_i210 || + (adapter->flags & IGB_FLAG_DMAC)) { reg = rd32(E1000_PCIEMISC); - reg &= ~E1000_PCIEMISC_LX_DECISION; + reg |= E1000_PCIEMISC_LX_DECISION; wr32(E1000_PCIEMISC, reg); } /* endif adapter->dmac is not disabled */ } else if (hw->mac.type == e1000_82580) {
We found Intel I210 can only achieve ~750Mbps Tx speed on some platforms. The RR2DCDELAY shows around 0x2xxx DMA delay, which will be significantly lower when 1) ASPM is disabled or 2) SoC package c-state stays above PC3. When the RR2DCDELAY is around 0x1xxx the Tx speed can reach to ~950Mbps. According to the I210 datasheet "8.26.1 PCIe Misc. Register - PCIEMISC", "DMA Idle Indication" doesn't seem to tie to DMA coalesce anymore, so set it to 1b for "DMA is considered idle when there is no Rx or Tx AND when there are no TLPs indicating that CPU is active detected on the PCIe link (such as the host executes CSR or Configuration register read or write operation)" and performing Tx should also fall under "active CPU on PCIe link" case. In addition to that, commit b6e0c419f040 ("igb: Move DMA Coalescing init code to separate function.") seems to wrongly changed from enabling E1000_PCIEMISC_LX_DECISION to disabling it, also fix that. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)