| Message ID | 20250303-igc_mqprio_tx_mode-v3-1-0efce85e6ae0@linutronix.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-next,v3] igc: Change Tx mode for MQPRIO offloading | expand |
On 3/3/2025 5:16 pm, Kurt Kanzenbach wrote: > The current MQPRIO offload implementation uses the legacy TSN Tx mode. In > this mode the hardware uses four packet buffers and considers queue > priorities. > > In order to harmonize the TAPRIO implementation with MQPRIO, switch to the > regular TSN Tx mode. This mode also uses four packet buffers and considers > queue priorities. In addition to the legacy mode, transmission is always > coupled to Qbv. The driver already has mechanisms to use a dummy schedule > of 1 second with all gates open for ETF. Simply use this for MQPRIO too. > > This reduces code and makes it easier to add support for frame preemption > later. > > While at it limit the netdev_tc calls to MQPRIO only. > > Tested on i225 with real time application using high priority queue, iperf3 > using low priority queue and network TAP device. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > Changes in v3: > - Commit message (Paul) > - Link to v2: https://lore.kernel.org/r/20250224-igc_mqprio_tx_mode-v2-1-9666da13c8d8@linutronix.de > > Changes in v2: > - Add comma to commit message (Faizal) > - Simplify if condition (Faizal) > - Link to v1: https://lore.kernel.org/r/20250217-igc_mqprio_tx_mode-v1-1-3a402fe1f326@linutronix.de > --- > drivers/net/ethernet/intel/igc/igc.h | 4 +--- > drivers/net/ethernet/intel/igc/igc_main.c | 18 +++++++++++++- > drivers/net/ethernet/intel/igc/igc_tsn.c | 40 ++----------------------------- > 3 files changed, 20 insertions(+), 42 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index cd1d7b6c1782352094f6867a31b6958c929bbbf4..16d85bdf55a7e9c412c47acf727bca6bc7154c61 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -388,11 +388,9 @@ extern char igc_driver_name[]; > #define IGC_FLAG_RX_LEGACY BIT(16) > #define IGC_FLAG_TSN_QBV_ENABLED BIT(17) > #define IGC_FLAG_TSN_QAV_ENABLED BIT(18) > -#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19) > > #define IGC_FLAG_TSN_ANY_ENABLED \ > - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \ > - IGC_FLAG_TSN_LEGACY_ENABLED) > + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED) > > #define IGC_FLAG_RSS_FIELD_IPV4_UDP BIT(6) > #define IGC_FLAG_RSS_FIELD_IPV6_UDP BIT(7) > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 472f009630c98e60c7166ceb8d05cb094f6c837b..240b6075197fb1e61077a736ddf8f9e67c1ed5cd 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6679,13 +6679,14 @@ static int igc_tsn_enable_mqprio(struct igc_adapter *adapter, > struct tc_mqprio_qopt_offload *mqprio) > { > struct igc_hw *hw = &adapter->hw; > - int i; > + int err, i; > > if (hw->mac.type != igc_i225) > return -EOPNOTSUPP; > > if (!mqprio->qopt.num_tc) { > adapter->strict_priority_enable = false; > + netdev_reset_tc(adapter->netdev); > goto apply; > } > > @@ -6716,6 +6717,21 @@ static int igc_tsn_enable_mqprio(struct igc_adapter *adapter, > igc_save_mqprio_params(adapter, mqprio->qopt.num_tc, > mqprio->qopt.offset); > > + err = netdev_set_num_tc(adapter->netdev, adapter->num_tc); > + if (err) > + return err; > + > + for (i = 0; i < adapter->num_tc; i++) { > + err = netdev_set_tc_queue(adapter->netdev, i, 1, > + adapter->queue_per_tc[i]); > + if (err) > + return err; > + } > + > + /* In case the card is configured with less than four queues. */ > + for (; i < IGC_MAX_TX_QUEUES; i++) > + adapter->queue_per_tc[i] = i; > + > mqprio->qopt.hw = TC_MQPRIO_HW_OFFLOAD_TCS; > > apply: > diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c > index 1e44374ca1ffbb86e9893266c590f318984ef574..7c28f3e7bb576f0e6a21c883e934ede4d53096f4 100644 > --- a/drivers/net/ethernet/intel/igc/igc_tsn.c > +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c > @@ -37,18 +37,13 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter) > { > unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED; > > - if (adapter->taprio_offload_enable) > - new_flags |= IGC_FLAG_TSN_QBV_ENABLED; > - > - if (is_any_launchtime(adapter)) > + if (adapter->taprio_offload_enable || is_any_launchtime(adapter) || > + adapter->strict_priority_enable) > new_flags |= IGC_FLAG_TSN_QBV_ENABLED; > > if (is_cbs_enabled(adapter)) > new_flags |= IGC_FLAG_TSN_QAV_ENABLED; > > - if (adapter->strict_priority_enable) > - new_flags |= IGC_FLAG_TSN_LEGACY_ENABLED; > - > return new_flags; > } > > @@ -157,16 +152,12 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter) > wr32(IGC_QBVCYCLET_S, 0); > wr32(IGC_QBVCYCLET, NSEC_PER_SEC); > > - /* Reset mqprio TC configuration. */ > - netdev_reset_tc(adapter->netdev); > - > /* Restore the default Tx arbitration: Priority 0 has the highest > * priority and is assigned to queue 0 and so on and so forth. > */ > igc_tsn_tx_arb(adapter, queue_per_tc); > > adapter->flags &= ~IGC_FLAG_TSN_QBV_ENABLED; > - adapter->flags &= ~IGC_FLAG_TSN_LEGACY_ENABLED; > > return 0; > } > @@ -206,37 +197,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) > igc_tsn_set_retx_qbvfullthreshold(adapter); > > if (adapter->strict_priority_enable) { > - int err; > - > - err = netdev_set_num_tc(adapter->netdev, adapter->num_tc); > - if (err) > - return err; > - > - for (i = 0; i < adapter->num_tc; i++) { > - err = netdev_set_tc_queue(adapter->netdev, i, 1, > - adapter->queue_per_tc[i]); > - if (err) > - return err; > - } > - > - /* In case the card is configured with less than four queues. */ > - for (; i < IGC_MAX_TX_QUEUES; i++) > - adapter->queue_per_tc[i] = i; > - > /* Configure queue priorities according to the user provided > * mapping. > */ > igc_tsn_tx_arb(adapter, adapter->queue_per_tc); > - > - /* Enable legacy TSN mode which will do strict priority without > - * any other TSN features. > - */ > - tqavctrl = rd32(IGC_TQAVCTRL); > - tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN; > - tqavctrl &= ~IGC_TQAVCTRL_ENHANCED_QAV; > - wr32(IGC_TQAVCTRL, tqavctrl); > - > - return 0; > } > > for (i = 0; i < adapter->num_tx_queues; i++) { > > --- > base-commit: f77f12010f67259bd0e1ad18877ed27c721b627a > change-id: 20250214-igc_mqprio_tx_mode-22c19c564605 > > Best regards, Acked-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
On Mon, Mar 03, 2025 at 10:16:33AM +0100, Kurt Kanzenbach wrote: > The current MQPRIO offload implementation uses the legacy TSN Tx mode. In > this mode the hardware uses four packet buffers and considers queue > priorities. > > In order to harmonize the TAPRIO implementation with MQPRIO, switch to the > regular TSN Tx mode. This mode also uses four packet buffers and considers > queue priorities. In addition to the legacy mode, transmission is always > coupled to Qbv. The driver already has mechanisms to use a dummy schedule > of 1 second with all gates open for ETF. Simply use this for MQPRIO too. > > This reduces code and makes it easier to add support for frame preemption > later. > > While at it limit the netdev_tc calls to MQPRIO only. Hi Kurt, Can this part be broken out into a separate patch? It seems so to me, but perhaps I'm missing something. The reason that I ask is that this appears to be a good portion of the change, and doing so would make the code changes for main part of the patch, as per the description prior to the line above, clearer IMHO. > > Tested on i225 with real time application using high priority queue, iperf3 > using low priority queue and network TAP device. > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> ...
On Tue Mar 04 2025, Simon Horman wrote: > On Mon, Mar 03, 2025 at 10:16:33AM +0100, Kurt Kanzenbach wrote: >> The current MQPRIO offload implementation uses the legacy TSN Tx mode. In >> this mode the hardware uses four packet buffers and considers queue >> priorities. >> >> In order to harmonize the TAPRIO implementation with MQPRIO, switch to the >> regular TSN Tx mode. This mode also uses four packet buffers and considers >> queue priorities. In addition to the legacy mode, transmission is always >> coupled to Qbv. The driver already has mechanisms to use a dummy schedule >> of 1 second with all gates open for ETF. Simply use this for MQPRIO too. >> >> This reduces code and makes it easier to add support for frame preemption >> later. >> >> While at it limit the netdev_tc calls to MQPRIO only. > > Hi Kurt, > > Can this part be broken out into a separate patch? > It seems so to me, but perhaps I'm missing something. > > The reason that I ask is that this appears to be a good portion of the > change, and doing so would make the code changes for main part of the > patch, as per the description prior to the line above, clearer IMHO. Sure, i think it can be broken out into a dedicated patch. I'll see what I can come up with. Thanks, Kurt
On Mon, Mar 03, 2025 at 10:16:33AM +0100, Kurt Kanzenbach wrote: > diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h > index cd1d7b6c1782352094f6867a31b6958c929bbbf4..16d85bdf55a7e9c412c47acf727bca6bc7154c61 100644 > --- a/drivers/net/ethernet/intel/igc/igc.h > +++ b/drivers/net/ethernet/intel/igc/igc.h > @@ -388,11 +388,9 @@ extern char igc_driver_name[]; > #define IGC_FLAG_RX_LEGACY BIT(16) > #define IGC_FLAG_TSN_QBV_ENABLED BIT(17) > #define IGC_FLAG_TSN_QAV_ENABLED BIT(18) > -#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19) > > #define IGC_FLAG_TSN_ANY_ENABLED \ > - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \ > - IGC_FLAG_TSN_LEGACY_ENABLED) > + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED) How do you and Faizal plan to serialize your changes on these flags? You delete IGC_FLAG_TSN_LEGACY_ENABLED and he adds IGC_FLAG_TSN_PREEMPT_ENABLED.
On 7/3/2025 11:02 pm, Vladimir Oltean wrote: > On Mon, Mar 03, 2025 at 10:16:33AM +0100, Kurt Kanzenbach wrote: >> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h >> index cd1d7b6c1782352094f6867a31b6958c929bbbf4..16d85bdf55a7e9c412c47acf727bca6bc7154c61 100644 >> --- a/drivers/net/ethernet/intel/igc/igc.h >> +++ b/drivers/net/ethernet/intel/igc/igc.h >> @@ -388,11 +388,9 @@ extern char igc_driver_name[]; >> #define IGC_FLAG_RX_LEGACY BIT(16) >> #define IGC_FLAG_TSN_QBV_ENABLED BIT(17) >> #define IGC_FLAG_TSN_QAV_ENABLED BIT(18) >> -#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19) >> >> #define IGC_FLAG_TSN_ANY_ENABLED \ >> - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \ >> - IGC_FLAG_TSN_LEGACY_ENABLED) >> + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED) > > How do you and Faizal plan to serialize your changes on these flags? > You delete IGC_FLAG_TSN_LEGACY_ENABLED and he adds > IGC_FLAG_TSN_PREEMPT_ENABLED. From what I’ve experienced before, when there’s a conflict like this, the Intel maintainer handles it and gets both authors to review the resolution (this has happened to both of us before) before they proceed to submit the patch. But if one patch gets merged first, the other person can just rebase and submit a new version ?
On Tue, Mar 11, 2025 at 04:03:57PM +0800, Abdul Rahim, Faizal wrote: > On 7/3/2025 11:02 pm, Vladimir Oltean wrote: > > How do you and Faizal plan to serialize your changes on these flags? > > You delete IGC_FLAG_TSN_LEGACY_ENABLED and he adds > > IGC_FLAG_TSN_PREEMPT_ENABLED. > > From what I’ve experienced before, when there’s a conflict like this, the > Intel maintainer handles it and gets both authors to review the resolution > (this has happened to both of us before) before they proceed to submit the > patch. > > But if one patch gets merged first, the other person can just rebase and > submit a new version ? Yes, rebasing after the other's patch is merged works just fine. I was asking if you had decided which one should go in first just to avoid any ambiguity, but if the answer is 'any', that is also fine.
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h index cd1d7b6c1782352094f6867a31b6958c929bbbf4..16d85bdf55a7e9c412c47acf727bca6bc7154c61 100644 --- a/drivers/net/ethernet/intel/igc/igc.h +++ b/drivers/net/ethernet/intel/igc/igc.h @@ -388,11 +388,9 @@ extern char igc_driver_name[]; #define IGC_FLAG_RX_LEGACY BIT(16) #define IGC_FLAG_TSN_QBV_ENABLED BIT(17) #define IGC_FLAG_TSN_QAV_ENABLED BIT(18) -#define IGC_FLAG_TSN_LEGACY_ENABLED BIT(19) #define IGC_FLAG_TSN_ANY_ENABLED \ - (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED | \ - IGC_FLAG_TSN_LEGACY_ENABLED) + (IGC_FLAG_TSN_QBV_ENABLED | IGC_FLAG_TSN_QAV_ENABLED) #define IGC_FLAG_RSS_FIELD_IPV4_UDP BIT(6) #define IGC_FLAG_RSS_FIELD_IPV6_UDP BIT(7) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 472f009630c98e60c7166ceb8d05cb094f6c837b..240b6075197fb1e61077a736ddf8f9e67c1ed5cd 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6679,13 +6679,14 @@ static int igc_tsn_enable_mqprio(struct igc_adapter *adapter, struct tc_mqprio_qopt_offload *mqprio) { struct igc_hw *hw = &adapter->hw; - int i; + int err, i; if (hw->mac.type != igc_i225) return -EOPNOTSUPP; if (!mqprio->qopt.num_tc) { adapter->strict_priority_enable = false; + netdev_reset_tc(adapter->netdev); goto apply; } @@ -6716,6 +6717,21 @@ static int igc_tsn_enable_mqprio(struct igc_adapter *adapter, igc_save_mqprio_params(adapter, mqprio->qopt.num_tc, mqprio->qopt.offset); + err = netdev_set_num_tc(adapter->netdev, adapter->num_tc); + if (err) + return err; + + for (i = 0; i < adapter->num_tc; i++) { + err = netdev_set_tc_queue(adapter->netdev, i, 1, + adapter->queue_per_tc[i]); + if (err) + return err; + } + + /* In case the card is configured with less than four queues. */ + for (; i < IGC_MAX_TX_QUEUES; i++) + adapter->queue_per_tc[i] = i; + mqprio->qopt.hw = TC_MQPRIO_HW_OFFLOAD_TCS; apply: diff --git a/drivers/net/ethernet/intel/igc/igc_tsn.c b/drivers/net/ethernet/intel/igc/igc_tsn.c index 1e44374ca1ffbb86e9893266c590f318984ef574..7c28f3e7bb576f0e6a21c883e934ede4d53096f4 100644 --- a/drivers/net/ethernet/intel/igc/igc_tsn.c +++ b/drivers/net/ethernet/intel/igc/igc_tsn.c @@ -37,18 +37,13 @@ static unsigned int igc_tsn_new_flags(struct igc_adapter *adapter) { unsigned int new_flags = adapter->flags & ~IGC_FLAG_TSN_ANY_ENABLED; - if (adapter->taprio_offload_enable) - new_flags |= IGC_FLAG_TSN_QBV_ENABLED; - - if (is_any_launchtime(adapter)) + if (adapter->taprio_offload_enable || is_any_launchtime(adapter) || + adapter->strict_priority_enable) new_flags |= IGC_FLAG_TSN_QBV_ENABLED; if (is_cbs_enabled(adapter)) new_flags |= IGC_FLAG_TSN_QAV_ENABLED; - if (adapter->strict_priority_enable) - new_flags |= IGC_FLAG_TSN_LEGACY_ENABLED; - return new_flags; } @@ -157,16 +152,12 @@ static int igc_tsn_disable_offload(struct igc_adapter *adapter) wr32(IGC_QBVCYCLET_S, 0); wr32(IGC_QBVCYCLET, NSEC_PER_SEC); - /* Reset mqprio TC configuration. */ - netdev_reset_tc(adapter->netdev); - /* Restore the default Tx arbitration: Priority 0 has the highest * priority and is assigned to queue 0 and so on and so forth. */ igc_tsn_tx_arb(adapter, queue_per_tc); adapter->flags &= ~IGC_FLAG_TSN_QBV_ENABLED; - adapter->flags &= ~IGC_FLAG_TSN_LEGACY_ENABLED; return 0; } @@ -206,37 +197,10 @@ static int igc_tsn_enable_offload(struct igc_adapter *adapter) igc_tsn_set_retx_qbvfullthreshold(adapter); if (adapter->strict_priority_enable) { - int err; - - err = netdev_set_num_tc(adapter->netdev, adapter->num_tc); - if (err) - return err; - - for (i = 0; i < adapter->num_tc; i++) { - err = netdev_set_tc_queue(adapter->netdev, i, 1, - adapter->queue_per_tc[i]); - if (err) - return err; - } - - /* In case the card is configured with less than four queues. */ - for (; i < IGC_MAX_TX_QUEUES; i++) - adapter->queue_per_tc[i] = i; - /* Configure queue priorities according to the user provided * mapping. */ igc_tsn_tx_arb(adapter, adapter->queue_per_tc); - - /* Enable legacy TSN mode which will do strict priority without - * any other TSN features. - */ - tqavctrl = rd32(IGC_TQAVCTRL); - tqavctrl |= IGC_TQAVCTRL_TRANSMIT_MODE_TSN; - tqavctrl &= ~IGC_TQAVCTRL_ENHANCED_QAV; - wr32(IGC_TQAVCTRL, tqavctrl); - - return 0; } for (i = 0; i < adapter->num_tx_queues; i++) {
The current MQPRIO offload implementation uses the legacy TSN Tx mode. In this mode the hardware uses four packet buffers and considers queue priorities. In order to harmonize the TAPRIO implementation with MQPRIO, switch to the regular TSN Tx mode. This mode also uses four packet buffers and considers queue priorities. In addition to the legacy mode, transmission is always coupled to Qbv. The driver already has mechanisms to use a dummy schedule of 1 second with all gates open for ETF. Simply use this for MQPRIO too. This reduces code and makes it easier to add support for frame preemption later. While at it limit the netdev_tc calls to MQPRIO only. Tested on i225 with real time application using high priority queue, iperf3 using low priority queue and network TAP device. Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> --- Changes in v3: - Commit message (Paul) - Link to v2: https://lore.kernel.org/r/20250224-igc_mqprio_tx_mode-v2-1-9666da13c8d8@linutronix.de Changes in v2: - Add comma to commit message (Faizal) - Simplify if condition (Faizal) - Link to v1: https://lore.kernel.org/r/20250217-igc_mqprio_tx_mode-v1-1-3a402fe1f326@linutronix.de --- drivers/net/ethernet/intel/igc/igc.h | 4 +--- drivers/net/ethernet/intel/igc/igc_main.c | 18 +++++++++++++- drivers/net/ethernet/intel/igc/igc_tsn.c | 40 ++----------------------------- 3 files changed, 20 insertions(+), 42 deletions(-) --- base-commit: f77f12010f67259bd0e1ad18877ed27c721b627a change-id: 20250214-igc_mqprio_tx_mode-22c19c564605 Best regards,