Received-SPF: pass (google.com: domain of linux-wireless+bounces-3547-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <62f4c9be-d7a9-45d3-bc29-9901d64ebafe@gmail.com>
Date: Tue, 13 Feb 2024 16:32:34 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
Subject: Re: [PATCH] wifi: rtlwifi: rtl8192cu: Fix TX aggregation
To: Ping-Ke Shih <pkshih@realtek.com>,
 "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc: "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>
References: <651dae37-1911-4dcd-870b-4b60f759ca82@gmail.com>
 <cd687092be44f7fe596f20aa7305c0e4618437ab.camel@realtek.com>
Content-Language: en-US
In-Reply-To: <cd687092be44f7fe596f20aa7305c0e4618437ab.camel@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 12/02/2024 14:50, Ping-Ke Shih wrote:
> On Thu, 2024-02-08 at 21:54 +0200, Bitterblue Smith wrote:
>>
>> rtl8192cu is checking rtl_mac.tids when deciding if it should enable
>> aggregation. This is wrong because rtl_mac.tids is not initialised
>> anywhere. Check rtl_sta_info.tids instead, which is initialised.
>>
>> Also, when enabling aggregation also enable RTS. The vendor driver does
>> this, my router does this. It seems like the thing to do.
>>
>> Also also, it seems right to set the AMPDU density only when enabling
>> aggregation.
>>
>> Also also also, delete the unused member rtl_mac.tids and the unused
>> macros RTL_AGG_ON and RTL_AGG_OFF.
>>
>> Naturally, with working AMPDU the download/upload speeds are better.
>> Before: 59/32 Mbps.
>> After:  68/46 Mbps.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>>  .../wireless/realtek/rtlwifi/rtl8192cu/trx.c  | 27 ++++++++++---------
>>  .../wireless/realtek/rtlwifi/rtl8192cu/trx.h  |  2 --
>>  drivers/net/wireless/realtek/rtlwifi/wifi.h   |  3 ---
>>  3 files changed, 15 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c
>> index e5c81c1c63c0..cbbd1dab8af0 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/trx.c
>> @@ -475,8 +475,9 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
>>         struct rtl_priv *rtlpriv = rtl_priv(hw);
>>         struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
>>         struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
>> -       u8 *qc = ieee80211_get_qos_ctl(hdr);
>> -       u8 tid = qc[0] & IEEE80211_QOS_CTL_TID_MASK;
>> +       struct rtl_sta_info *sta_entry;
>> +       u8 agg_state = RTL_AGG_STOP;
>> +       u8 ampdu_density = 0;
>>         u16 seq_number;
>>         __le16 fc = hdr->frame_control;
>>         u8 rate_flag = info->control.rates[0].flags;
>> @@ -498,10 +499,20 @@ void rtl92cu_tx_fill_desc(struct ieee80211_hw *hw,
>>         set_tx_desc_tx_rate(txdesc, tcb_desc->hw_rate);
>>         if (tcb_desc->use_shortgi || tcb_desc->use_shortpreamble)
>>                 set_tx_desc_data_shortgi(txdesc, 1);
>> -       if (mac->tids[tid].agg.agg_state == RTL_AGG_ON &&
>> -                   info->flags & IEEE80211_TX_CTL_AMPDU) {
>> +
>> +       if (sta) {
>> +               sta_entry = (struct rtl_sta_info *)sta->drv_priv;
> 
> nit: It would be better to be separated statement of ieee80211_get_tid(hdr)
> 
> tid = ieee80211_get_tid(hdr);
> agg_state = sta_entry->tids[tid].agg.agg_state;
> 

All right.

> >> +               agg_state = sta_entry->tids[ieee80211_get_tid(hdr)].agg.agg_state;
>> +               ampdu_density = sta->deflink.ht_cap.ampdu_density;
>> +       }
>> +
>> +       if (agg_state == RTL_AGG_OPERATIONAL &&
>> +           info->flags & IEEE80211_TX_CTL_AMPDU) {
>>                 set_tx_desc_agg_enable(txdesc, 1);
>>                 set_tx_desc_max_agg_num(txdesc, 0x14);
> 
> Have you tried larger number of this? 
> As IEEE80211_MAX_AMPDU_BUF_HT is 0x40, set 0x3f here to get higher TX
> throughput if USB speed can afford. 
> 

With this patch, the upload speed is 46 Mbps. But that's just
because my device is detected as 1T2R and it's not allowed to use
rates above MCS7. When I fix that problem the upload speed is 82 Mbps
in the same conditions, about 3 meters from the router.

If I then move the device just 60 cm away, the upload speed is 92
Mbps.

I tried 0x1f now (this field is only 5 bits). The upload speed doesn't
get better. Maybe it gets a bit worse.

> You can reference an equation of rtw89: 
> 
> 	ampdu_num = (u8)((rtwsta->ampdu_params[tid].agg_num ?
> 			  rtwsta->ampdu_params[tid].agg_num :
> 			  4 << sta->deflink.ht_cap.ampdu_factor) - 1);
> 
> This is new for rtlwifi, so we can have this later. 
> 
> 
> [...]
> 
> 
>