Re: [PATCH 2/3] wifi: mt76: mt792x: Add TX page pool support for IOMMU-enabled systems

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 15545 bytes --]

On Jul 09, Eason Lai wrote:
> From: Eason Lai <Eason.Lai@mediatek.com>
> 
> Add support for TX page pool allocation in mt792x devices when IOMMU is
> enabled. This optimization reduces DMA mapping overhead by
> pre-allocating and reusing page pool buffers for TX operations.

Can you please provide some test results here? In particular regarding the
resulting throughput.

> 
> Key changes:
> - Export DMA helper functions and ops for mt792x usage
>   - Add tx_prealloc_enabled flag to track page pool state
>     - Implement mt792x-specific DMA queue operations with TX page pool
>       - Create page pools per TX queue when IOMMU is detected
>         - Handle page pool buffer cleanup in both success and error
>           paths
> - Add proper skip_unmap flag handling for page pool buffers
> 
> The page pool path is used for linear skbs without fragments, falling
> back to standard DMA mapping for complex skb structures.
> 
> Signed-off-by: Eason Lai <Eason.Lai@mediatek.com>
> ---

[...]

>  
> -static void
> +void
>  mt76_dma_kick_queue(struct mt76_dev *dev, struct mt76_queue *q)
>  {
>  	wmb();
> @@ -409,6 +417,7 @@ mt76_dma_kick_queue(struct mt76_dev *dev, struct mt76_queue *q)
>  	else
>  		Q_WRITE(q, cpu_idx, q->head);
>  }
> +EXPORT_SYMBOL_GPL(mt76_dma_kick_queue);

this is already available in mt76_queue_ops struct.

>  
>  static void
>  mt76_dma_tx_cleanup(struct mt76_dev *dev, struct mt76_queue *q, bool flush)
> @@ -1152,7 +1161,7 @@ mt76_dma_init(struct mt76_dev *dev,
>  	return 0;
>  }
>  
> -static const struct mt76_queue_ops mt76_dma_ops = {
> +const struct mt76_queue_ops mt76_dma_ops = {
>  	.init = mt76_dma_init,
>  	.alloc = mt76_dma_alloc_queue,
>  	.reset_q = mt76_dma_queue_reset,
> @@ -1164,6 +1173,7 @@ static const struct mt76_queue_ops mt76_dma_ops = {
>  	.rx_reset = mt76_dma_rx_reset,
>  	.kick = mt76_dma_kick_queue,
>  };
> +EXPORT_SYMBOL_GPL(mt76_dma_ops);
>  
>  void mt76_dma_attach(struct mt76_dev *dev)
>  {
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 927c21536f4e..25e24fa36eca 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -957,6 +957,8 @@ struct mt76_dev {
>  	int tx_dma_idx[4];
>  	enum mt76_hwrro_mode hwrro_mode;
>  
> +	bool tx_prealloc_enabled;
> +
>  	struct mt76_worker tx_worker;
>  	struct napi_struct tx_napi;
>  
> @@ -1785,7 +1787,16 @@ mt76_tx_status_get_hw(struct mt76_dev *dev, struct sk_buff *skb)
>  
>  void mt76_put_txwi(struct mt76_dev *dev, struct mt76_txwi_cache *t);
>  void mt76_put_rxwi(struct mt76_dev *dev, struct mt76_txwi_cache *t);
> +struct mt76_txwi_cache *mt76_get_txwi(struct mt76_dev *dev);
>  struct mt76_txwi_cache *mt76_get_rxwi(struct mt76_dev *dev);
> +int mt76_dma_add_buf(struct mt76_dev *dev, struct mt76_queue *q,
> +		     struct mt76_queue_buf *buf, int nbufs, u32 info,
> +		     struct sk_buff *skb, void *txwi);
> +void mt76_dma_tx_cleanup_idx(struct mt76_dev *dev, struct mt76_queue *q, int idx,
> +			     struct mt76_queue_entry *prev_e);
> +void mt76_dma_kick_queue(struct mt76_dev *dev, struct mt76_queue *q);
> +void mt76_dma_sync_idx(struct mt76_dev *dev, struct mt76_queue *q);
> +extern const struct mt76_queue_ops mt76_dma_ops;
>  void mt76_free_pending_rxwi(struct mt76_dev *dev);
>  void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
>  		      struct napi_struct *napi);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c b/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
> index 0339e2e7ab60..d4b36b0832b8 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76_connac_mac.c
> @@ -250,6 +250,12 @@ void mt76_connac_txp_skb_unmap(struct mt76_dev *dev,
>  {
>  	struct mt76_connac_txp_common *txp;
>  
> +	if (t->page_pool_buf) {
> +		mt76_put_page_pool_buf(t->page_pool_buf, false);
> +		t->page_pool_buf = NULL;
> +		return;
> +	}
> +
>  	txp = mt76_connac_txwi_to_txp(dev, t);
>  	if (is_mt76_fw_txp(dev))
>  		mt76_connac_txp_skb_unmap_fw(dev, &txp->fw);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7925/pci.c b/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
> index c4161754c01d..3d69d8c67dea 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7925/pci.c
> @@ -214,7 +214,7 @@ static int mt7925_dma_init(struct mt792x_dev *dev)
>  {
>  	int ret;
>  
> -	mt76_dma_attach(&dev->mt76);
> +	mt792x_dma_attach(&dev->mt76);
>  
>  	ret = mt792x_dma_disable(dev, true);
>  	if (ret)
> @@ -263,6 +263,8 @@ static int mt7925_dma_init(struct mt792x_dev *dev)
>  			  mt792x_poll_tx);
>  	napi_enable(&dev->mt76.tx_napi);
>  
> +	mt792x_dma_tx_page_pool_init(dev);
> +
>  	return mt792x_dma_enable(dev);
>  }
>  
> diff --git a/drivers/net/wireless/mediatek/mt76/mt792x.h b/drivers/net/wireless/mediatek/mt76/mt792x.h
> index 4ff93f2cd624..7ac0318dc249 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt792x.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt792x.h
> @@ -418,8 +418,10 @@ void mt792x_sta_statistics(struct ieee80211_hw *hw,
>  			   struct station_info *sinfo);
>  void mt792x_set_coverage_class(struct ieee80211_hw *hw, int radio_idx,
>  			       s16 coverage_class);
> +void mt792x_dma_attach(struct mt76_dev *dev);
>  void mt792x_dma_cleanup(struct mt792x_dev *dev);
>  int mt792x_dma_enable(struct mt792x_dev *dev);
> +int mt792x_dma_tx_page_pool_init(struct mt792x_dev *dev);
>  int mt792x_wpdma_reset(struct mt792x_dev *dev, bool force);
>  int mt792x_wpdma_reinit_cond(struct mt792x_dev *dev);
>  int mt792x_dma_disable(struct mt792x_dev *dev, bool force);
> diff --git a/drivers/net/wireless/mediatek/mt76/mt792x_dma.c b/drivers/net/wireless/mediatek/mt76/mt792x_dma.c
> index 002aece857b2..b341f1cb3ce0 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt792x_dma.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt792x_dma.c
> @@ -3,6 +3,9 @@
>  
>  #include <linux/module.h>
>  #include <linux/firmware.h>
> +#include <linux/iommu.h>
> +#include <net/page_pool/helpers.h>
> +#include <linux/of.h>
>  
>  #include "mt792x.h"
>  #include "dma.h"
> @@ -266,6 +269,323 @@ int mt792x_wpdma_reinit_cond(struct mt792x_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(mt792x_wpdma_reinit_cond);
>  
> +static int mt792x_create_tx_page_pool(struct mt76_dev *dev, struct mt76_queue *q)
> +{
> +	struct page_pool_params pp_params = {
> +		.order = 0,
> +		.flags = 0,
> +		.nid = NUMA_NO_NODE,
> +		.dev = dev->dma_dev,
> +	};
> +
> +	if (!q || !dev->tx_prealloc_enabled)
> +		return 0;
> +
> +	if (!mt76_is_mmio(dev))
> +		return 0;

do you really need this check? you already check it in
mt792x_dma_tx_page_pool_init().

> +
> +	pp_params.pool_size = 256;
> +	pp_params.flags |= PP_FLAG_DMA_MAP;
> +	pp_params.dma_dir = DMA_BIDIRECTIONAL;
> +	pp_params.max_len = PAGE_SIZE;
> +	pp_params.offset = 0;
> +
> +	q->page_pool = page_pool_create(&pp_params);
> +	if (IS_ERR(q->page_pool)) {
> +		int err = PTR_ERR(q->page_pool);
> +
> +		q->page_pool = NULL;
> +		dev_warn(dev->dev, "Failed to create TX page pool for queue %d (err=%d)\n",
> +			 q->hw_idx, err);
> +		return 0;
> +	}
> +
> +	return 0;
> +}
> +
> +int mt792x_dma_tx_page_pool_init(struct mt792x_dev *dev)
> +{
> +	struct mt76_dev *mdev = &dev->mt76;
> +	int i, ret, pool_count = 0;
> +
> +	if (!iommu_get_domain_for_dev(mdev->dma_dev))
> +		return 0;
> +
> +	if (!mt76_is_mmio(mdev))
> +		return 0;
> +
> +	mdev->tx_prealloc_enabled = true;
> +
> +	for (i = 0; i < ARRAY_SIZE(mdev->phy.q_tx); i++) {
> +		struct mt76_queue *q = mdev->phy.q_tx[i];
> +
> +		if (!q)
> +			continue;
> +
> +		ret = mt792x_create_tx_page_pool(mdev, q);
> +		if (ret)
> +			return ret;
> +
> +		if (q->page_pool)
> +			pool_count++;
> +	}
> +
> +	if (pool_count > 0)
> +		dev_info(mdev->dev,
> +			 "IOMMU enabled, created %d TX page pools\n", pool_count);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mt792x_dma_tx_page_pool_init);
> +
> +static void mt792x_dma_tx_page_pool_cleanup(struct mt792x_dev *dev)
> +{
> +	struct mt76_dev *mdev = &dev->mt76;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(mdev->phy.q_tx); i++) {
> +		struct mt76_queue *q = mdev->phy.q_tx[i];
> +
> +		if (!q || !q->page_pool)
> +			continue;
> +
> +		page_pool_destroy(q->page_pool);
> +		q->page_pool = NULL;
> +	}
> +
> +	mdev->tx_prealloc_enabled = false;
> +}
> +
> +static void *
> +mt792x_dma_tx_alloc_page_pool_buf(struct mt76_dev *dev, struct mt76_queue *q,
> +				  struct sk_buff *skb, dma_addr_t *dma_addr,
> +				  int *buf_len)
> +{
> +	struct page *page;
> +	void *buf;
> +	int len;
> +	u32 offset;
> +
> +	if (!q->page_pool || !dev->tx_prealloc_enabled)
> +		return NULL;
> +
> +	len = skb_headlen(skb);
> +	if (len > PAGE_SIZE)
> +		return NULL;
> +
> +	buf = mt76_get_page_pool_buf(q, &offset, len);
> +	if (!buf)
> +		return NULL;
> +
> +	page = virt_to_head_page(buf);
> +	*dma_addr = page_pool_get_dma_addr(page) + offset;
> +	if (unlikely(!*dma_addr)) {
> +		dev_warn_ratelimited(dev->dev, "Page pool returned NULL DMA address\n");
> +		mt76_put_page_pool_buf(buf, false);
> +		return NULL;
> +	}
> +
> +	*buf_len = len;
> +
> +	dma_sync_single_for_cpu(dev->dma_dev, *dma_addr, len, DMA_TO_DEVICE);
> +	skb_copy_from_linear_data(skb, buf, len);

I would like to see some results here since usually it is not a good idea to
copy all the packets (even if it is just the linear part).

> +	dma_sync_single_for_device(dev->dma_dev, *dma_addr, len, DMA_TO_DEVICE);
> +
> +	return buf;
> +}
> +
> +static int
> +mt792x_dma_tx_queue_skb(struct mt76_phy *phy, struct mt76_queue *q,
> +			enum mt76_txq_id qid, struct sk_buff *skb,
> +			struct mt76_wcid *wcid, struct ieee80211_sta *sta)
> +{
> +	struct ieee80211_tx_status status = {
> +		.sta = sta,
> +	};
> +	struct mt76_tx_info tx_info = {
> +		.skb = skb,
> +	};
> +	struct mt76_dev *dev = phy->dev;
> +	struct ieee80211_hw *hw;
> +	int len, n = 0, ret = -ENOMEM;
> +	struct mt76_txwi_cache *t;
> +	struct sk_buff *iter;
> +	dma_addr_t addr;
> +	u8 *txwi;
> +
> +	if (test_bit(MT76_RESET, &phy->state))
> +		goto free_skb;
> +
> +	t = mt76_get_txwi(dev);
> +	if (!t)
> +		goto free_skb;
> +
> +	txwi = mt76_get_txwi_ptr(dev, t);
> +
> +	skb->prev = NULL;
> +	skb->next = NULL;
> +	if (dev->drv->drv_flags & MT_DRV_TX_ALIGNED4_SKBS)
> +		mt76_insert_hdr_pad(skb);
> +
> +	len = skb_headlen(skb);
> +
> +	if (dev->tx_prealloc_enabled && q->page_pool &&
> +	    !skb_has_frag_list(skb) && !skb_shinfo(skb)->nr_frags) {
> +		void *buf;
> +		int pp_len;
> +
> +		buf = mt792x_dma_tx_alloc_page_pool_buf(dev, q, skb, &addr, &pp_len);
> +		if (buf) {
> +			t->page_pool_buf = buf;
> +			len = pp_len;
> +
> +			tx_info.buf[n].addr = t->dma_addr;
> +			tx_info.buf[n++].len = dev->drv->txwi_size;
> +			tx_info.buf[n].addr = addr;
> +			tx_info.buf[n].len = len;
> +			tx_info.buf[n].skip_unmap = true;
> +			n++;
> +
> +			goto skip_dma_map;
> +		}
> +	}
> +
> +	addr = dma_map_single(dev->dma_dev, skb->data, len, DMA_TO_DEVICE);
> +	if (unlikely(dma_mapping_error(dev->dma_dev, addr)))
> +		goto free;
> +
> +	t->page_pool_buf = NULL;
> +
> +	tx_info.buf[n].addr = t->dma_addr;
> +	tx_info.buf[n++].len = dev->drv->txwi_size;
> +	tx_info.buf[n].addr = addr;
> +	tx_info.buf[n++].len = len;
> +
> +skip_dma_map:
> +	skb_walk_frags(skb, iter) {
> +		if (n == ARRAY_SIZE(tx_info.buf))
> +			goto unmap;
> +
> +		addr = dma_map_single(dev->dma_dev, iter->data, iter->len,
> +				      DMA_TO_DEVICE);
> +		if (unlikely(dma_mapping_error(dev->dma_dev, addr)))
> +			goto unmap;
> +
> +		tx_info.buf[n].addr = addr;
> +		tx_info.buf[n].skip_unmap = false;
> +		tx_info.buf[n++].len = iter->len;
> +	}
> +	tx_info.nbuf = n;
> +
> +	if (q->queued + (tx_info.nbuf + 1) / 2 >= q->ndesc - 1) {
> +		ret = -ENOMEM;
> +		goto unmap;
> +	}
> +
> +	dma_sync_single_for_cpu(dev->dma_dev, t->dma_addr, dev->drv->txwi_size,
> +				DMA_TO_DEVICE);
> +	ret = dev->drv->tx_prepare_skb(dev, txwi, qid, wcid, sta, &tx_info);
> +	dma_sync_single_for_device(dev->dma_dev, t->dma_addr, dev->drv->txwi_size,
> +				   DMA_TO_DEVICE);
> +	if (ret < 0)
> +		goto unmap;
> +
> +	return mt76_dma_add_buf(dev, q, tx_info.buf, tx_info.nbuf,
> +				tx_info.info, tx_info.skb, t);
> +
> +unmap:
> +	for (n--; n > 0; n--) {
> +		if (!tx_info.buf[n].skip_unmap)
> +			dma_unmap_single(dev->dma_dev, tx_info.buf[n].addr,
> +					 tx_info.buf[n].len, DMA_TO_DEVICE);
> +	}
> +
> +	if (t->page_pool_buf) {
> +		mt76_put_page_pool_buf(t->page_pool_buf, false);
> +		t->page_pool_buf = NULL;
> +	}
> +
> +free:
> +#ifdef CONFIG_NL80211_TESTMODE
> +	if (mt76_is_testmode_skb(dev, skb, &hw)) {
> +		struct mt76_phy *phy = hw->priv;
> +
> +		if (tx_info.skb == phy->test.tx_skb)
> +			phy->test.tx_done--;
> +	}
> +#endif
> +
> +	mt76_put_txwi(dev, t);
> +
> +free_skb:
> +	status.skb = tx_info.skb;
> +	hw = mt76_tx_status_get_hw(dev, tx_info.skb);
> +	spin_lock_bh(&dev->rx_lock);
> +	ieee80211_tx_status_ext(hw, &status);
> +	spin_unlock_bh(&dev->rx_lock);
> +
> +	return ret;
> +}
> +
> +static void
> +mt792x_dma_tx_cleanup(struct mt76_dev *dev, struct mt76_queue *q, bool flush)

this is almost a copy of mt76_dma_tx_cleanup(). Can we do something better
here?

> +{
> +	struct mt76_queue_entry entry;
> +	int last;
> +
> +	if (!q || !q->ndesc)
> +		return;
> +
> +	spin_lock_bh(&q->cleanup_lock);
> +	if (flush)
> +		last = -1;
> +	else
> +		last = Q_READ(q, dma_idx);
> +
> +	while (q->queued > 0 && q->tail != last) {
> +		mt76_dma_tx_cleanup_idx(dev, q, q->tail, &entry);
> +		mt76_npu_txdesc_cleanup(q, q->tail);
> +		mt76_queue_tx_complete(dev, q, &entry);
> +
> +		if (entry.txwi) {
> +			if (entry.skb && entry.txwi->page_pool_buf) {
> +				mt76_put_page_pool_buf(entry.txwi->page_pool_buf, false);
> +				entry.txwi->page_pool_buf = NULL;
> +			}
> +
> +			if (!(dev->drv->drv_flags & MT_DRV_TXWI_NO_FREE))
> +				mt76_put_txwi(dev, entry.txwi);
> +		}
> +
> +		if (!flush && q->tail == last)
> +			last = Q_READ(q, dma_idx);
> +	}
> +	spin_unlock_bh(&q->cleanup_lock);
> +
> +	if (flush) {
> +		spin_lock_bh(&q->lock);
> +		mt76_dma_sync_idx(dev, q);
> +		mt76_dma_kick_queue(dev, q);
> +		spin_unlock_bh(&q->lock);
> +	}
> +
> +	if (!q->queued)
> +		wake_up(&dev->tx_wait);
> +}
> +
> +static struct mt76_queue_ops mt792x_queue_ops;
> +
> +void mt792x_dma_attach(struct mt76_dev *dev)
> +{
> +	mt792x_queue_ops = mt76_dma_ops;

can you please take a look to mt7615_mmio_probe()?

Regards,
Lorenzo

> +
> +	mt792x_queue_ops.tx_queue_skb = mt792x_dma_tx_queue_skb;
> +	mt792x_queue_ops.tx_cleanup = mt792x_dma_tx_cleanup;
> +
> +	dev->queue_ops = &mt792x_queue_ops;
> +}
> +EXPORT_SYMBOL_GPL(mt792x_dma_attach);
> +
>  int mt792x_dma_disable(struct mt792x_dev *dev, bool force)
>  {
>  	/* disable WFDMA0 */
> @@ -326,6 +646,8 @@ void mt792x_dma_cleanup(struct mt792x_dev *dev)
>  		 MT_WFDMA0_RST_LOGIC_RST);
>  
>  	mt76_dma_cleanup(&dev->mt76);
> +
> +	mt792x_dma_tx_page_pool_cleanup(dev);
>  }
>  EXPORT_SYMBOL_GPL(mt792x_dma_cleanup);
>  
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]