[PATCH 0/3] Fixes to the AE4DMA

[PATCH 0/3] Fixes to the AE4DMA

[Basavaraj Natikar]

This patch series include changes for:
- Removing unused PCI IDs.
- Use of proper MSI functions.
- Enhancing and fixing the AE4DMA engine's multi-queue functionality.


Basavaraj Natikar (3):
  dmaengine: ae4dma: Remove deprecated PCI IDs
  dmaengine: ae4dma: Use the MSI count and its corresponding IRQ number
  dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue
    functionality

 drivers/dma/amd/ae4dma/ae4dma-pci.c     |  6 +-
 drivers/dma/amd/ae4dma/ae4dma.h         |  2 +
 drivers/dma/amd/ptdma/ptdma-dmaengine.c | 90 ++++++++++++++++++++++++-
 3 files changed, 91 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] dmaengine: ae4dma: Remove deprecated PCI IDs

[Basavaraj Natikar]

Two previously used PCI IDs are deprecated and should not be used for
AE4DMA. Hence, remove as they are unsupported for AE4DMA.

Fixes: 90a30e268d9b ("dmaengine: ae4dma: Add AMD ae4dma controller driver")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/dma/amd/ae4dma/ae4dma-pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/amd/ae4dma/ae4dma-pci.c b/drivers/dma/amd/ae4dma/ae4dma-pci.c
index aad0dc4294a3..7f96843f5215 100644
--- a/drivers/dma/amd/ae4dma/ae4dma-pci.c
+++ b/drivers/dma/amd/ae4dma/ae4dma-pci.c
@@ -137,8 +137,6 @@ static void ae4_pci_remove(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id ae4_pci_table[] = {
-	{ PCI_VDEVICE(AMD, 0x14C8), },
-	{ PCI_VDEVICE(AMD, 0x14DC), },
 	{ PCI_VDEVICE(AMD, 0x149B), },
 	/* Last entry must be zero */
 	{ 0, }
-- 
2.25.1

Re: [PATCH 1/3] dmaengine: ae4dma: Remove deprecated PCI IDs

[Vinod Koul]

On 03-02-25, 21:55, Basavaraj Natikar wrote:
> Two previously used PCI IDs are deprecated and should not be used for
> AE4DMA. Hence, remove as they are unsupported for AE4DMA.
> 
> Fixes: 90a30e268d9b ("dmaengine: ae4dma: Add AMD ae4dma controller driver")

Why is this a fix?

> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/dma/amd/ae4dma/ae4dma-pci.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/dma/amd/ae4dma/ae4dma-pci.c b/drivers/dma/amd/ae4dma/ae4dma-pci.c
> index aad0dc4294a3..7f96843f5215 100644
> --- a/drivers/dma/amd/ae4dma/ae4dma-pci.c
> +++ b/drivers/dma/amd/ae4dma/ae4dma-pci.c
> @@ -137,8 +137,6 @@ static void ae4_pci_remove(struct pci_dev *pdev)
>  }
>  
>  static const struct pci_device_id ae4_pci_table[] = {
> -	{ PCI_VDEVICE(AMD, 0x14C8), },
> -	{ PCI_VDEVICE(AMD, 0x14DC), },
>  	{ PCI_VDEVICE(AMD, 0x149B), },
>  	/* Last entry must be zero */
>  	{ 0, }
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH 1/3] dmaengine: ae4dma: Remove deprecated PCI IDs

[Basavaraj Natikar]

On 2/10/2025 7:29 PM, Vinod Koul wrote:
> On 03-02-25, 21:55, Basavaraj Natikar wrote:
>> Two previously used PCI IDs are deprecated and should not be used for
>> AE4DMA. Hence, remove as they are unsupported for AE4DMA.
>>
>> Fixes: 90a30e268d9b ("dmaengine: ae4dma: Add AMD ae4dma controller driver")
> Why is this a fix?

Yes, as these changes are needed to work only on supported devices.
If it loads on those PCI IDs now, since they are unsupported,
the probe will fail.

Thanks,
--
Basavaraj

>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>>   drivers/dma/amd/ae4dma/ae4dma-pci.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/dma/amd/ae4dma/ae4dma-pci.c b/drivers/dma/amd/ae4dma/ae4dma-pci.c
>> index aad0dc4294a3..7f96843f5215 100644
>> --- a/drivers/dma/amd/ae4dma/ae4dma-pci.c
>> +++ b/drivers/dma/amd/ae4dma/ae4dma-pci.c
>> @@ -137,8 +137,6 @@ static void ae4_pci_remove(struct pci_dev *pdev)
>>   }
>>   
>>   static const struct pci_device_id ae4_pci_table[] = {
>> -	{ PCI_VDEVICE(AMD, 0x14C8), },
>> -	{ PCI_VDEVICE(AMD, 0x14DC), },
>>   	{ PCI_VDEVICE(AMD, 0x149B), },
>>   	/* Last entry must be zero */
>>   	{ 0, }
>> -- 
>> 2.25.1

[PATCH 2/3] dmaengine: ae4dma: Use the MSI count and its corresponding IRQ number

[Basavaraj Natikar]

Instead of using the defined maximum hardware queue, which can lead to
incorrect values if the counts mismatch, use the exact supported MSI
count and its corresponding IRQ number.

Fixes: 90a30e268d9b ("dmaengine: ae4dma: Add AMD ae4dma controller driver")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/dma/amd/ae4dma/ae4dma-pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/amd/ae4dma/ae4dma-pci.c b/drivers/dma/amd/ae4dma/ae4dma-pci.c
index 7f96843f5215..2c63907db228 100644
--- a/drivers/dma/amd/ae4dma/ae4dma-pci.c
+++ b/drivers/dma/amd/ae4dma/ae4dma-pci.c
@@ -46,8 +46,8 @@ static int ae4_get_irqs(struct ae4_device *ae4)
 
 	} else {
 		ae4_msix->msix_count = ret;
-		for (i = 0; i < MAX_AE4_HW_QUEUES; i++)
-			ae4->ae4_irq[i] = ae4_msix->msix_entry[i].vector;
+		for (i = 0; i < ae4_msix->msix_count; i++)
+			ae4->ae4_irq[i] = pci_irq_vector(pdev, i);
 	}
 
 	return ret;
-- 
2.25.1

[PATCH 3/3] dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue functionality

[Basavaraj Natikar]

As AE4DMA offers multi-channel functionality compared to PTDMA’s single
queue, utilize multi-queue, which supports higher speeds than PTDMA, to
achieve higher performance using the AE4DMA workqueue based mechanism.

Fixes: 69a47b16a51b ("dmaengine: ptdma: Extend ptdma to support multi-channel and version")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/dma/amd/ae4dma/ae4dma.h         |  2 +
 drivers/dma/amd/ptdma/ptdma-dmaengine.c | 90 ++++++++++++++++++++++++-
 2 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/amd/ae4dma/ae4dma.h b/drivers/dma/amd/ae4dma/ae4dma.h
index 265c5d436008..57f6048726bb 100644
--- a/drivers/dma/amd/ae4dma/ae4dma.h
+++ b/drivers/dma/amd/ae4dma/ae4dma.h
@@ -37,6 +37,8 @@
 #define AE4_DMA_VERSION			4
 #define CMD_AE4_DESC_DW0_VAL		2
 
+#define AE4_TIME_OUT			5000
+
 struct ae4_msix {
 	int msix_count;
 	struct msix_entry msix_entry[MAX_AE4_HW_QUEUES];
diff --git a/drivers/dma/amd/ptdma/ptdma-dmaengine.c b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
index 35c84ec9608b..715ac3ae067b 100644
--- a/drivers/dma/amd/ptdma/ptdma-dmaengine.c
+++ b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
@@ -198,8 +198,10 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
 {
 	struct dma_async_tx_descriptor *tx_desc;
 	struct virt_dma_desc *vd;
+	struct pt_device *pt;
 	unsigned long flags;
 
+	pt = chan->pt;
 	/* Loop over descriptors until one is found with commands */
 	do {
 		if (desc) {
@@ -217,7 +219,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
 
 		spin_lock_irqsave(&chan->vc.lock, flags);
 
-		if (desc) {
+		if (pt->ver != AE4_DMA_VERSION && desc) {
 			if (desc->status != DMA_COMPLETE) {
 				if (desc->status != DMA_ERROR)
 					desc->status = DMA_COMPLETE;
@@ -235,7 +237,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
 
 		spin_unlock_irqrestore(&chan->vc.lock, flags);
 
-		if (tx_desc) {
+		if (pt->ver != AE4_DMA_VERSION && tx_desc) {
 			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
 			dma_run_dependencies(tx_desc);
 			vchan_vdesc_fini(vd);
@@ -245,11 +247,25 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
 	return NULL;
 }
 
+static inline bool ae4_core_queue_full(struct pt_cmd_queue *cmd_q)
+{
+	u32 front_wi = readl(cmd_q->reg_control + AE4_WR_IDX_OFF);
+	u32 rear_ri = readl(cmd_q->reg_control + AE4_RD_IDX_OFF);
+
+	if (((MAX_CMD_QLEN + front_wi - rear_ri) % MAX_CMD_QLEN)  >= (MAX_CMD_QLEN - 1))
+		return true;
+
+	return false;
+}
+
 static void pt_cmd_callback(void *data, int err)
 {
 	struct pt_dma_desc *desc = data;
+	struct ae4_cmd_queue *ae4cmd_q;
 	struct dma_chan *dma_chan;
 	struct pt_dma_chan *chan;
+	struct ae4_device *ae4;
+	struct pt_device *pt;
 	int ret;
 
 	if (err == -EINPROGRESS)
@@ -257,11 +273,32 @@ static void pt_cmd_callback(void *data, int err)
 
 	dma_chan = desc->vd.tx.chan;
 	chan = to_pt_chan(dma_chan);
+	pt = chan->pt;
 
 	if (err)
 		desc->status = DMA_ERROR;
 
 	while (true) {
+		if (pt->ver == AE4_DMA_VERSION) {
+			ae4 = container_of(pt, struct ae4_device, pt);
+			ae4cmd_q = &ae4->ae4cmd_q[chan->id];
+
+			if (ae4cmd_q->q_cmd_count >= (CMD_Q_LEN - 1) ||
+			    ae4_core_queue_full(&ae4cmd_q->cmd_q)) {
+				wake_up(&ae4cmd_q->q_w);
+
+				if (wait_for_completion_timeout(&ae4cmd_q->cmp,
+								msecs_to_jiffies(AE4_TIME_OUT))
+								== 0) {
+					dev_err(pt->dev, "TIMEOUT %d:\n", ae4cmd_q->id);
+					break;
+				}
+
+				reinit_completion(&ae4cmd_q->cmp);
+				continue;
+			}
+		}
+
 		/* Check for DMA descriptor completion */
 		desc = pt_handle_active_desc(chan, desc);
 
@@ -296,6 +333,49 @@ static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
 	return desc;
 }
 
+static void pt_cmd_callback_work(void *data, int err)
+{
+	struct dma_async_tx_descriptor *tx_desc;
+	struct pt_dma_desc *desc = data;
+	struct dma_chan *dma_chan;
+	struct virt_dma_desc *vd;
+	struct pt_dma_chan *chan;
+	unsigned long flags;
+
+	dma_chan = desc->vd.tx.chan;
+	chan = to_pt_chan(dma_chan);
+
+	if (err == -EINPROGRESS)
+		return;
+
+	tx_desc = &desc->vd.tx;
+	vd = &desc->vd;
+
+	if (err)
+		desc->status = DMA_ERROR;
+
+	spin_lock_irqsave(&chan->vc.lock, flags);
+	if (desc) {
+		if (desc->status != DMA_COMPLETE) {
+			if (desc->status != DMA_ERROR)
+				desc->status = DMA_COMPLETE;
+
+			dma_cookie_complete(tx_desc);
+			dma_descriptor_unmap(tx_desc);
+		} else {
+			tx_desc = NULL;
+		}
+	}
+	spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+	if (tx_desc) {
+		dmaengine_desc_get_callback_invoke(tx_desc, NULL);
+		dma_run_dependencies(tx_desc);
+		list_del(&desc->vd.node);
+		vchan_vdesc_fini(vd);
+	}
+}
+
 static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
 					  dma_addr_t dst,
 					  dma_addr_t src,
@@ -327,6 +407,7 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
 	desc->len = len;
 
 	if (pt->ver == AE4_DMA_VERSION) {
+		pt_cmd->pt_cmd_callback = pt_cmd_callback_work;
 		ae4 = container_of(pt, struct ae4_device, pt);
 		ae4cmd_q = &ae4->ae4cmd_q[chan->id];
 		mutex_lock(&ae4cmd_q->cmd_lock);
@@ -367,13 +448,16 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
 {
 	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
 	struct pt_dma_desc *desc;
+	struct pt_device *pt;
 	unsigned long flags;
 	bool engine_is_idle = true;
 
+	pt = chan->pt;
+
 	spin_lock_irqsave(&chan->vc.lock, flags);
 
 	desc = pt_next_dma_desc(chan);
-	if (desc)
+	if (desc && pt->ver != AE4_DMA_VERSION)
 		engine_is_idle = false;
 
 	vchan_issue_pending(&chan->vc);
-- 
2.25.1

Re: [PATCH 3/3] dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue functionality

[Vinod Koul]

On 03-02-25, 21:55, Basavaraj Natikar wrote:
> As AE4DMA offers multi-channel functionality compared to PTDMA’s single
> queue, utilize multi-queue, which supports higher speeds than PTDMA, to
> achieve higher performance using the AE4DMA workqueue based mechanism.
> 
> Fixes: 69a47b16a51b ("dmaengine: ptdma: Extend ptdma to support multi-channel and version")

Why is this a fix, again!

> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/dma/amd/ae4dma/ae4dma.h         |  2 +
>  drivers/dma/amd/ptdma/ptdma-dmaengine.c | 90 ++++++++++++++++++++++++-
>  2 files changed, 89 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/amd/ae4dma/ae4dma.h b/drivers/dma/amd/ae4dma/ae4dma.h
> index 265c5d436008..57f6048726bb 100644
> --- a/drivers/dma/amd/ae4dma/ae4dma.h
> +++ b/drivers/dma/amd/ae4dma/ae4dma.h
> @@ -37,6 +37,8 @@
>  #define AE4_DMA_VERSION			4
>  #define CMD_AE4_DESC_DW0_VAL		2
>  
> +#define AE4_TIME_OUT			5000
> +
>  struct ae4_msix {
>  	int msix_count;
>  	struct msix_entry msix_entry[MAX_AE4_HW_QUEUES];
> diff --git a/drivers/dma/amd/ptdma/ptdma-dmaengine.c b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
> index 35c84ec9608b..715ac3ae067b 100644
> --- a/drivers/dma/amd/ptdma/ptdma-dmaengine.c
> +++ b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
> @@ -198,8 +198,10 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>  {
>  	struct dma_async_tx_descriptor *tx_desc;
>  	struct virt_dma_desc *vd;
> +	struct pt_device *pt;
>  	unsigned long flags;
>  
> +	pt = chan->pt;
>  	/* Loop over descriptors until one is found with commands */
>  	do {
>  		if (desc) {
> @@ -217,7 +219,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>  
>  		spin_lock_irqsave(&chan->vc.lock, flags);
>  
> -		if (desc) {
> +		if (pt->ver != AE4_DMA_VERSION && desc) {
>  			if (desc->status != DMA_COMPLETE) {
>  				if (desc->status != DMA_ERROR)
>  					desc->status = DMA_COMPLETE;
> @@ -235,7 +237,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>  
>  		spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
> -		if (tx_desc) {
> +		if (pt->ver != AE4_DMA_VERSION && tx_desc) {

Why should this handling be different for DMA_VERSION?

>  			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
>  			dma_run_dependencies(tx_desc);
>  			vchan_vdesc_fini(vd);
> @@ -245,11 +247,25 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>  	return NULL;
>  }
>  
> +static inline bool ae4_core_queue_full(struct pt_cmd_queue *cmd_q)
> +{
> +	u32 front_wi = readl(cmd_q->reg_control + AE4_WR_IDX_OFF);
> +	u32 rear_ri = readl(cmd_q->reg_control + AE4_RD_IDX_OFF);
> +
> +	if (((MAX_CMD_QLEN + front_wi - rear_ri) % MAX_CMD_QLEN)  >= (MAX_CMD_QLEN - 1))
> +		return true;
> +
> +	return false;
> +}
> +
>  static void pt_cmd_callback(void *data, int err)
>  {
>  	struct pt_dma_desc *desc = data;
> +	struct ae4_cmd_queue *ae4cmd_q;
>  	struct dma_chan *dma_chan;
>  	struct pt_dma_chan *chan;
> +	struct ae4_device *ae4;
> +	struct pt_device *pt;
>  	int ret;
>  
>  	if (err == -EINPROGRESS)
> @@ -257,11 +273,32 @@ static void pt_cmd_callback(void *data, int err)
>  
>  	dma_chan = desc->vd.tx.chan;
>  	chan = to_pt_chan(dma_chan);
> +	pt = chan->pt;
>  
>  	if (err)
>  		desc->status = DMA_ERROR;
>  
>  	while (true) {
> +		if (pt->ver == AE4_DMA_VERSION) {
> +			ae4 = container_of(pt, struct ae4_device, pt);
> +			ae4cmd_q = &ae4->ae4cmd_q[chan->id];
> +
> +			if (ae4cmd_q->q_cmd_count >= (CMD_Q_LEN - 1) ||
> +			    ae4_core_queue_full(&ae4cmd_q->cmd_q)) {
> +				wake_up(&ae4cmd_q->q_w);
> +
> +				if (wait_for_completion_timeout(&ae4cmd_q->cmp,
> +								msecs_to_jiffies(AE4_TIME_OUT))
> +								== 0) {
> +					dev_err(pt->dev, "TIMEOUT %d:\n", ae4cmd_q->id);
> +					break;
> +				}
> +
> +				reinit_completion(&ae4cmd_q->cmp);
> +				continue;
> +			}
> +		}
> +
>  		/* Check for DMA descriptor completion */
>  		desc = pt_handle_active_desc(chan, desc);
>  
> @@ -296,6 +333,49 @@ static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>  	return desc;
>  } 
>  
> +static void pt_cmd_callback_work(void *data, int err)
> +{
> +	struct dma_async_tx_descriptor *tx_desc;
> +	struct pt_dma_desc *desc = data;
> +	struct dma_chan *dma_chan;
> +	struct virt_dma_desc *vd;
> +	struct pt_dma_chan *chan;
> +	unsigned long flags;
> +
> +	dma_chan = desc->vd.tx.chan;
> +	chan = to_pt_chan(dma_chan);
> +
> +	if (err == -EINPROGRESS)
> +		return;
> +
> +	tx_desc = &desc->vd.tx;
> +	vd = &desc->vd;
> +
> +	if (err)
> +		desc->status = DMA_ERROR;
> +
> +	spin_lock_irqsave(&chan->vc.lock, flags);
> +	if (desc) {
> +		if (desc->status != DMA_COMPLETE) {
> +			if (desc->status != DMA_ERROR)
> +				desc->status = DMA_COMPLETE;
> +
> +			dma_cookie_complete(tx_desc);
> +			dma_descriptor_unmap(tx_desc);
> +		} else {
> +			tx_desc = NULL;
> +		}
> +	}
> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> +	if (tx_desc) {
> +		dmaengine_desc_get_callback_invoke(tx_desc, NULL);
> +		dma_run_dependencies(tx_desc);
> +		list_del(&desc->vd.node);
> +		vchan_vdesc_fini(vd);
> +	}
> +}

Why do we have callback in driver...?

> +
>  static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>  					  dma_addr_t dst,
>  					  dma_addr_t src,
> @@ -327,6 +407,7 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>  	desc->len = len;
>  
>  	if (pt->ver == AE4_DMA_VERSION) {
> +		pt_cmd->pt_cmd_callback = pt_cmd_callback_work;
>  		ae4 = container_of(pt, struct ae4_device, pt);
>  		ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>  		mutex_lock(&ae4cmd_q->cmd_lock);
> @@ -367,13 +448,16 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>  {
>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>  	struct pt_dma_desc *desc;
> +	struct pt_device *pt;
>  	unsigned long flags;
>  	bool engine_is_idle = true;
>  
> +	pt = chan->pt;
> +
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  
>  	desc = pt_next_dma_desc(chan);
> -	if (desc)
> +	if (desc && pt->ver != AE4_DMA_VERSION)
>  		engine_is_idle = false;
>  
>  	vchan_issue_pending(&chan->vc);
> -- 
> 2.25.1

-- 
~Vinod

Re: [PATCH 3/3] dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue functionality

[Basavaraj Natikar]

On 2/10/2025 7:48 PM, Vinod Koul wrote:
> On 03-02-25, 21:55, Basavaraj Natikar wrote:
>> As AE4DMA offers multi-channel functionality compared to PTDMA’s single
>> queue, utilize multi-queue, which supports higher speeds than PTDMA, to
>> achieve higher performance using the AE4DMA workqueue based mechanism.
>>
>> Fixes: 69a47b16a51b ("dmaengine: ptdma: Extend ptdma to support multi-channel and version")
> Why is this a fix, again!

Yes, as AE4DMA is much faster with multi-queue. However, sometimes due to multi-queue
processing, it takes longer and introduces a timeout for the synchronization of
consumers and producers, which helps avoid long waits that could eventually cause a hang.

Thanks,
--
Basavaraj

>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>>   drivers/dma/amd/ae4dma/ae4dma.h         |  2 +
>>   drivers/dma/amd/ptdma/ptdma-dmaengine.c | 90 ++++++++++++++++++++++++-
>>   2 files changed, 89 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/amd/ae4dma/ae4dma.h b/drivers/dma/amd/ae4dma/ae4dma.h
>> index 265c5d436008..57f6048726bb 100644
>> --- a/drivers/dma/amd/ae4dma/ae4dma.h
>> +++ b/drivers/dma/amd/ae4dma/ae4dma.h
>> @@ -37,6 +37,8 @@
>>   #define AE4_DMA_VERSION			4
>>   #define CMD_AE4_DESC_DW0_VAL		2
>>   
>> +#define AE4_TIME_OUT			5000
>> +
>>   struct ae4_msix {
>>   	int msix_count;
>>   	struct msix_entry msix_entry[MAX_AE4_HW_QUEUES];
>> diff --git a/drivers/dma/amd/ptdma/ptdma-dmaengine.c b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> index 35c84ec9608b..715ac3ae067b 100644
>> --- a/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> +++ b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> @@ -198,8 +198,10 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>   {
>>   	struct dma_async_tx_descriptor *tx_desc;
>>   	struct virt_dma_desc *vd;
>> +	struct pt_device *pt;
>>   	unsigned long flags;
>>   
>> +	pt = chan->pt;
>>   	/* Loop over descriptors until one is found with commands */
>>   	do {
>>   		if (desc) {
>> @@ -217,7 +219,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>   
>>   		spin_lock_irqsave(&chan->vc.lock, flags);
>>   
>> -		if (desc) {
>> +		if (pt->ver != AE4_DMA_VERSION && desc) {
>>   			if (desc->status != DMA_COMPLETE) {
>>   				if (desc->status != DMA_ERROR)
>>   					desc->status = DMA_COMPLETE;
>> @@ -235,7 +237,7 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>   
>>   		spin_unlock_irqrestore(&chan->vc.lock, flags);
>>   
>> -		if (tx_desc) {
>> +		if (pt->ver != AE4_DMA_VERSION && tx_desc) {
> Why should this handling be different for DMA_VERSION?
>
>>   			dmaengine_desc_get_callback_invoke(tx_desc, NULL);
>>   			dma_run_dependencies(tx_desc);
>>   			vchan_vdesc_fini(vd);
>> @@ -245,11 +247,25 @@ static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>>   	return NULL;
>>   }
>>   
>> +static inline bool ae4_core_queue_full(struct pt_cmd_queue *cmd_q)
>> +{
>> +	u32 front_wi = readl(cmd_q->reg_control + AE4_WR_IDX_OFF);
>> +	u32 rear_ri = readl(cmd_q->reg_control + AE4_RD_IDX_OFF);
>> +
>> +	if (((MAX_CMD_QLEN + front_wi - rear_ri) % MAX_CMD_QLEN)  >= (MAX_CMD_QLEN - 1))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>>   static void pt_cmd_callback(void *data, int err)
>>   {
>>   	struct pt_dma_desc *desc = data;
>> +	struct ae4_cmd_queue *ae4cmd_q;
>>   	struct dma_chan *dma_chan;
>>   	struct pt_dma_chan *chan;
>> +	struct ae4_device *ae4;
>> +	struct pt_device *pt;
>>   	int ret;
>>   
>>   	if (err == -EINPROGRESS)
>> @@ -257,11 +273,32 @@ static void pt_cmd_callback(void *data, int err)
>>   
>>   	dma_chan = desc->vd.tx.chan;
>>   	chan = to_pt_chan(dma_chan);
>> +	pt = chan->pt;
>>   
>>   	if (err)
>>   		desc->status = DMA_ERROR;
>>   
>>   	while (true) {
>> +		if (pt->ver == AE4_DMA_VERSION) {
>> +			ae4 = container_of(pt, struct ae4_device, pt);
>> +			ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>> +
>> +			if (ae4cmd_q->q_cmd_count >= (CMD_Q_LEN - 1) ||
>> +			    ae4_core_queue_full(&ae4cmd_q->cmd_q)) {
>> +				wake_up(&ae4cmd_q->q_w);
>> +
>> +				if (wait_for_completion_timeout(&ae4cmd_q->cmp,
>> +								msecs_to_jiffies(AE4_TIME_OUT))
>> +								== 0) {
>> +					dev_err(pt->dev, "TIMEOUT %d:\n", ae4cmd_q->id);
>> +					break;
>> +				}
>> +
>> +				reinit_completion(&ae4cmd_q->cmp);
>> +				continue;
>> +			}
>> +		}
>> +
>>   		/* Check for DMA descriptor completion */
>>   		desc = pt_handle_active_desc(chan, desc);
>>   
>> @@ -296,6 +333,49 @@ static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>>   	return desc;
>>   }
>>   
>> +static void pt_cmd_callback_work(void *data, int err)
>> +{
>> +	struct dma_async_tx_descriptor *tx_desc;
>> +	struct pt_dma_desc *desc = data;
>> +	struct dma_chan *dma_chan;
>> +	struct virt_dma_desc *vd;
>> +	struct pt_dma_chan *chan;
>> +	unsigned long flags;
>> +
>> +	dma_chan = desc->vd.tx.chan;
>> +	chan = to_pt_chan(dma_chan);
>> +
>> +	if (err == -EINPROGRESS)
>> +		return;
>> +
>> +	tx_desc = &desc->vd.tx;
>> +	vd = &desc->vd;
>> +
>> +	if (err)
>> +		desc->status = DMA_ERROR;
>> +
>> +	spin_lock_irqsave(&chan->vc.lock, flags);
>> +	if (desc) {
>> +		if (desc->status != DMA_COMPLETE) {
>> +			if (desc->status != DMA_ERROR)
>> +				desc->status = DMA_COMPLETE;
>> +
>> +			dma_cookie_complete(tx_desc);
>> +			dma_descriptor_unmap(tx_desc);
>> +		} else {
>> +			tx_desc = NULL;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
>> +
>> +	if (tx_desc) {
>> +		dmaengine_desc_get_callback_invoke(tx_desc, NULL);
>> +		dma_run_dependencies(tx_desc);
>> +		list_del(&desc->vd.node);
>> +		vchan_vdesc_fini(vd);
>> +	}
>> +}
> Why do we have callback in driver...?
>
>> +
>>   static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>   					  dma_addr_t dst,
>>   					  dma_addr_t src,
>> @@ -327,6 +407,7 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>   	desc->len = len;
>>   
>>   	if (pt->ver == AE4_DMA_VERSION) {
>> +		pt_cmd->pt_cmd_callback = pt_cmd_callback_work;
>>   		ae4 = container_of(pt, struct ae4_device, pt);
>>   		ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>>   		mutex_lock(&ae4cmd_q->cmd_lock);
>> @@ -367,13 +448,16 @@ static void pt_issue_pending(struct dma_chan *dma_chan)
>>   {
>>   	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>   	struct pt_dma_desc *desc;
>> +	struct pt_device *pt;
>>   	unsigned long flags;
>>   	bool engine_is_idle = true;
>>   
>> +	pt = chan->pt;
>> +
>>   	spin_lock_irqsave(&chan->vc.lock, flags);
>>   
>>   	desc = pt_next_dma_desc(chan);
>> -	if (desc)
>> +	if (desc && pt->ver != AE4_DMA_VERSION)
>>   		engine_is_idle = false;
>>   
>>   	vchan_issue_pending(&chan->vc);
>> -- 
>> 2.25.1

Re: [PATCH 3/3] dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue functionality

[Basavaraj Natikar]

On 2/10/2025 7:56 PM, Basavaraj Natikar wrote:
>
>
> On 2/10/2025 7:48 PM, Vinod Koul wrote:
>> On 03-02-25, 21:55, Basavaraj Natikar wrote:
>>> As AE4DMA offers multi-channel functionality compared to PTDMA’s single
>>> queue, utilize multi-queue, which supports higher speeds than PTDMA, to
>>> achieve higher performance using the AE4DMA workqueue based mechanism.
>>>
>>> Fixes: 69a47b16a51b ("dmaengine: ptdma: Extend ptdma to support 
>>> multi-channel and version")
>> Why is this a fix, again!
>
> Yes, as AE4DMA is much faster with multi-queue. However, sometimes due 
> to multi-queue
> processing, it takes longer and introduces a timeout for the 
> synchronization of
> consumers and producers, which helps avoid long waits that could 
> eventually cause a hang.
>
> Thanks,
> -- 
> Basavaraj
>
>>
>>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>> ---
>>>   drivers/dma/amd/ae4dma/ae4dma.h         |  2 +
>>>   drivers/dma/amd/ptdma/ptdma-dmaengine.c | 90 
>>> ++++++++++++++++++++++++-
>>>   2 files changed, 89 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma/amd/ae4dma/ae4dma.h 
>>> b/drivers/dma/amd/ae4dma/ae4dma.h
>>> index 265c5d436008..57f6048726bb 100644
>>> --- a/drivers/dma/amd/ae4dma/ae4dma.h
>>> +++ b/drivers/dma/amd/ae4dma/ae4dma.h
>>> @@ -37,6 +37,8 @@
>>>   #define AE4_DMA_VERSION            4
>>>   #define CMD_AE4_DESC_DW0_VAL        2
>>>   +#define AE4_TIME_OUT            5000
>>> +
>>>   struct ae4_msix {
>>>       int msix_count;
>>>       struct msix_entry msix_entry[MAX_AE4_HW_QUEUES];
>>> diff --git a/drivers/dma/amd/ptdma/ptdma-dmaengine.c 
>>> b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>>> index 35c84ec9608b..715ac3ae067b 100644
>>> --- a/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>>> +++ b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>>> @@ -198,8 +198,10 @@ static struct pt_dma_desc 
>>> *pt_handle_active_desc(struct pt_dma_chan *chan,
>>>   {
>>>       struct dma_async_tx_descriptor *tx_desc;
>>>       struct virt_dma_desc *vd;
>>> +    struct pt_device *pt;
>>>       unsigned long flags;
>>>   +    pt = chan->pt;
>>>       /* Loop over descriptors until one is found with commands */
>>>       do {
>>>           if (desc) {
>>> @@ -217,7 +219,7 @@ static struct pt_dma_desc 
>>> *pt_handle_active_desc(struct pt_dma_chan *chan,
>>>             spin_lock_irqsave(&chan->vc.lock, flags);
>>>   -        if (desc) {
>>> +        if (pt->ver != AE4_DMA_VERSION && desc) {
>>>               if (desc->status != DMA_COMPLETE) {
>>>                   if (desc->status != DMA_ERROR)
>>>                       desc->status = DMA_COMPLETE;
>>> @@ -235,7 +237,7 @@ static struct pt_dma_desc 
>>> *pt_handle_active_desc(struct pt_dma_chan *chan,
>>>             spin_unlock_irqrestore(&chan->vc.lock, flags);
>>>   -        if (tx_desc) {
>>> +        if (pt->ver != AE4_DMA_VERSION && tx_desc) {
>> Why should this handling be different for DMA_VERSION?

PTDMA always handles per command based interrupt and it is single queue while
AE4DMA we can submit multiple command each time with multiple as AE4DMA is
much faster with multi-queue.
However, sometimes due to multi-queue processing, it takes longer and introduces a
timeout for the synchronization of consumers and producers,
which helps avoid long waits that could eventually cause a hang.

>>
>>> dmaengine_desc_get_callback_invoke(tx_desc, NULL);
>>>               dma_run_dependencies(tx_desc);
>>>               vchan_vdesc_fini(vd);
>>> @@ -245,11 +247,25 @@ static struct pt_dma_desc 
>>> *pt_handle_active_desc(struct pt_dma_chan *chan,
>>>       return NULL;
>>>   }
>>>   +static inline bool ae4_core_queue_full(struct pt_cmd_queue *cmd_q)
>>> +{
>>> +    u32 front_wi = readl(cmd_q->reg_control + AE4_WR_IDX_OFF);
>>> +    u32 rear_ri = readl(cmd_q->reg_control + AE4_RD_IDX_OFF);
>>> +
>>> +    if (((MAX_CMD_QLEN + front_wi - rear_ri) % MAX_CMD_QLEN) >= 
>>> (MAX_CMD_QLEN - 1))
>>> +        return true;
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   static void pt_cmd_callback(void *data, int err)
>>>   {
>>>       struct pt_dma_desc *desc = data;
>>> +    struct ae4_cmd_queue *ae4cmd_q;
>>>       struct dma_chan *dma_chan;
>>>       struct pt_dma_chan *chan;
>>> +    struct ae4_device *ae4;
>>> +    struct pt_device *pt;
>>>       int ret;
>>>         if (err == -EINPROGRESS)
>>> @@ -257,11 +273,32 @@ static void pt_cmd_callback(void *data, int err)
>>>         dma_chan = desc->vd.tx.chan;
>>>       chan = to_pt_chan(dma_chan);
>>> +    pt = chan->pt;
>>>         if (err)
>>>           desc->status = DMA_ERROR;
>>>         while (true) {
>>> +        if (pt->ver == AE4_DMA_VERSION) {
>>> +            ae4 = container_of(pt, struct ae4_device, pt);
>>> +            ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>>> +
>>> +            if (ae4cmd_q->q_cmd_count >= (CMD_Q_LEN - 1) ||
>>> +                ae4_core_queue_full(&ae4cmd_q->cmd_q)) {
>>> +                wake_up(&ae4cmd_q->q_w);
>>> +
>>> +                if (wait_for_completion_timeout(&ae4cmd_q->cmp,
>>> + msecs_to_jiffies(AE4_TIME_OUT))
>>> +                                == 0) {
>>> +                    dev_err(pt->dev, "TIMEOUT %d:\n", ae4cmd_q->id);
>>> +                    break;
>>> +                }
>>> +
>>> +                reinit_completion(&ae4cmd_q->cmp);
>>> +                continue;
>>> +            }
>>> +        }
>>> +
>>>           /* Check for DMA descriptor completion */
>>>           desc = pt_handle_active_desc(chan, desc);
>>>   @@ -296,6 +333,49 @@ static struct pt_dma_desc 
>>> *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>>>       return desc;
>>>   }
>>>   +static void pt_cmd_callback_work(void *data, int err)
>>> +{
>>> +    struct dma_async_tx_descriptor *tx_desc;
>>> +    struct pt_dma_desc *desc = data;
>>> +    struct dma_chan *dma_chan;
>>> +    struct virt_dma_desc *vd;
>>> +    struct pt_dma_chan *chan;
>>> +    unsigned long flags;
>>> +
>>> +    dma_chan = desc->vd.tx.chan;
>>> +    chan = to_pt_chan(dma_chan);
>>> +
>>> +    if (err == -EINPROGRESS)
>>> +        return;
>>> +
>>> +    tx_desc = &desc->vd.tx;
>>> +    vd = &desc->vd;
>>> +
>>> +    if (err)
>>> +        desc->status = DMA_ERROR;
>>> +
>>> +    spin_lock_irqsave(&chan->vc.lock, flags);
>>> +    if (desc) {
>>> +        if (desc->status != DMA_COMPLETE) {
>>> +            if (desc->status != DMA_ERROR)
>>> +                desc->status = DMA_COMPLETE;
>>> +
>>> +            dma_cookie_complete(tx_desc);
>>> +            dma_descriptor_unmap(tx_desc);
>>> +        } else {
>>> +            tx_desc = NULL;
>>> +        }
>>> +    }
>>> +    spin_unlock_irqrestore(&chan->vc.lock, flags);
>>> +
>>> +    if (tx_desc) {
>>> +        dmaengine_desc_get_callback_invoke(tx_desc, NULL);
>>> +        dma_run_dependencies(tx_desc);
>>> +        list_del(&desc->vd.node);
>>> +        vchan_vdesc_fini(vd);
>>> +    }
>>> +}
>> Why do we have callback in driver...?

PTDMA also has similar callback pt_cmd_callback
hence AE4DMA also has callback for the multi-queue command ,
once command is processed to signal upper layer that processing
done for that queue.Thanks,
--
Basavaraj

>>
>>> +
>>>   static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>>                         dma_addr_t dst,
>>>                         dma_addr_t src,
>>> @@ -327,6 +407,7 @@ static struct pt_dma_desc *pt_create_desc(struct 
>>> dma_chan *dma_chan,
>>>       desc->len = len;
>>>         if (pt->ver == AE4_DMA_VERSION) {
>>> +        pt_cmd->pt_cmd_callback = pt_cmd_callback_work;
>>>           ae4 = container_of(pt, struct ae4_device, pt);
>>>           ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>>>           mutex_lock(&ae4cmd_q->cmd_lock);
>>> @@ -367,13 +448,16 @@ static void pt_issue_pending(struct dma_chan 
>>> *dma_chan)
>>>   {
>>>       struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>>       struct pt_dma_desc *desc;
>>> +    struct pt_device *pt;
>>>       unsigned long flags;
>>>       bool engine_is_idle = true;
>>>   +    pt = chan->pt;
>>> +
>>>       spin_lock_irqsave(&chan->vc.lock, flags);
>>>         desc = pt_next_dma_desc(chan);
>>> -    if (desc)
>>> +    if (desc && pt->ver != AE4_DMA_VERSION)
>>>           engine_is_idle = false;
>>>         vchan_issue_pending(&chan->vc);
>>> -- 
>>> 2.25.1
>

Re: [PATCH 0/3] Fixes to the AE4DMA

[Vinod Koul]

On Mon, 03 Feb 2025 21:55:08 +0530, Basavaraj Natikar wrote:
> This patch series include changes for:
> - Removing unused PCI IDs.
> - Use of proper MSI functions.
> - Enhancing and fixing the AE4DMA engine's multi-queue functionality.
> 
> 
> Basavaraj Natikar (3):
>   dmaengine: ae4dma: Remove deprecated PCI IDs
>   dmaengine: ae4dma: Use the MSI count and its corresponding IRQ number
>   dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue
>     functionality
> 
> [...]

Applied, thanks!

[1/3] dmaengine: ae4dma: Remove deprecated PCI IDs
      commit: b87c29c007e80f4737a056b3c5c21b5b5106b7f7
[2/3] dmaengine: ae4dma: Use the MSI count and its corresponding IRQ number
      commit: feba04e6fdf4daccc83fc09d499a3e32c178edb4
[3/3] dmaengine: ptdma: Utilize the AE4DMA engine's multi-queue functionality
      commit: 6565439894570a07b00dba0b739729fe6b56fba4

Best regards,
-- 
~Vinod