All of lore.kernel.org
 help / color / mirror / Atom feed
* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-10-31 16:03 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2018-10-31 16:03 UTC (permalink / raw)
  To: dan.j.williams, vkoul; +Cc: ldewangan, dmaengine, linux-tegra, Ben Dooks

The tx_status callback does not report the state of the transfer
beyond complete segments. This causes problems with users such as
ALSA when applications want to know accurately how much data has
been moved.

This patch addes a function tegra_dma_update_residual() to query
the hardware and modify the residual information accordinly. It
takes into account any hardware issues when trying to read the
state, such as delays between finishing a buffer and signalling
the interrupt.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
 1 file changed, 87 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4f7d1e576d03..3fa3a1ac4f57 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	return 0;
 }
 
+static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
+					      struct tegra_dma_sg_req *sg_req,
+					      struct tegra_dma_desc *dma_desc,
+					      unsigned int residual)
+{
+	unsigned long status = 0x0;
+	unsigned long wcount;
+	unsigned long ahbptr;
+	unsigned long tmp = 0x0;
+	unsigned int result;
+	int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
+	int done;
+
+	/* if we're not the current request, then don't alter the residual */
+	if (sg_req != list_first_entry(&tdc->pending_sg_req,
+				       struct tegra_dma_sg_req, node)) {
+		result = residual;
+		ahbptr = 0xffffffff;
+		goto done;
+	}
+
+	/* loop until we have a reliable result for residual */
+	do {
+		ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
+		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+		tmp =  tdc_read(tdc, 0x08);	/* total count for debug */
+
+		/* check status, if channel isn't busy then skip */
+		if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
+			result = residual;
+			break;
+		}
+
+		/* if we've got an interrupt pending on the channel, don't
+		 * try and deal with the residue as the hardware has likely
+		 * moved on to the next buffer. return all data moved.
+		 */
+		if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
+			result = residual - sg_req->req_len;
+			break;
+		}
+
+		if (tdc->tdma->chip_data->support_separate_wcount_reg)
+			wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+		else
+			wcount = status;
+
+		/* If the request is at the full point, then there is a
+		 * chance that we have read the status register in the
+		 * middle of the hardware reloading the next buffer.
+		 *
+		 * The sequence seems to be at the end of the buffer, to
+		 * load the new word count before raising the EOC flag (or
+		 * changing the ping-pong flag which could have also been
+		 * used to determine a new buffer). This  means there is a
+		 * small window where we cannot determine zero-done for the
+		 * current buffer, or moved to next buffer.
+		 *
+		 * If done shows 0, then retry the load, as it may hit the
+		 * above hardware race. We will either get a new value which
+		 * is from the first buffer, or we get an EOC (new buffer)
+		 * or both a new value and an EOC...
+		 */
+		done = get_current_xferred_count(tdc, sg_req, wcount);
+		if (done != 0) {
+			result = residual - done;
+			break;
+		}
+
+		ndelay(100);
+	} while (--retries > 0);
+
+	if (retries <= 0) {
+		dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
+		result = residual;
+	}
+
+done:	
+	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
+		 sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
+
+	return result;
+}
+
 static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 	dma_cookie_t cookie, struct dma_tx_state *txstate)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 	struct tegra_dma_desc *dma_desc;
-	struct tegra_dma_sg_req *sg_req;
+	struct tegra_dma_sg_req *sg_req = NULL;
 	enum dma_status ret;
 	unsigned long flags;
 	unsigned int residual;
@@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 		residual = dma_desc->bytes_requested -
 			   (dma_desc->bytes_transferred %
 			    dma_desc->bytes_requested);
+		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
 		dma_set_residue(txstate, residual);
 	}
 
@@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
 		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
 	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
-	/*
-	 * XXX The hardware appears to support
-	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
-	 * only used by this driver during tegra_dma_terminate_all()
-	 */
-	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	tdma->dma_dev.device_config = tegra_dma_slave_config;
 	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
 	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-02-21  0:41 Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-02-21  0:41 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

31.10.2018 19:03, Ben Dooks пишет:
> The tx_status callback does not report the state of the transfer
> beyond complete segments. This causes problems with users such as
> ALSA when applications want to know accurately how much data has
> been moved.
> 
> This patch addes a function tegra_dma_update_residual() to query
> the hardware and modify the residual information accordinly. It
> takes into account any hardware issues when trying to read the
> state, such as delays between finishing a buffer and signalling
> the interrupt.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Hello Ben,

Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-02-21 10:06 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2019-02-21 10:06 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 21/02/2019 00:41, Dmitry Osipenko wrote:
> 31.10.2018 19:03, Ben Dooks пишет:
>> The tx_status callback does not report the state of the transfer
>> beyond complete segments. This causes problems with users such as
>> ALSA when applications want to know accurately how much data has
>> been moved.
>>
>> This patch addes a function tegra_dma_update_residual() to query
>> the hardware and modify the residual information accordinly. It
>> takes into account any hardware issues when trying to read the
>> state, such as delays between finishing a buffer and signalling
>> the interrupt.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 
> Hello Ben,
> 
> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.

Personally I think the original version was fine. It has been tested
and returns fairly quickly (I am not a fan of just adding more delay in)

My notes say the condition doesn't last for long and the loop tends
to terminate within 2 runs.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-02-21 13:02 Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-02-21 13:02 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

21.02.2019 13:06, Ben Dooks пишет:
> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>> 31.10.2018 19:03, Ben Dooks пишет:
>>> The tx_status callback does not report the state of the transfer
>>> beyond complete segments. This causes problems with users such as
>>> ALSA when applications want to know accurately how much data has
>>> been moved.
>>>
>>> This patch addes a function tegra_dma_update_residual() to query
>>> the hardware and modify the residual information accordinly. It
>>> takes into account any hardware issues when trying to read the
>>> state, such as delays between finishing a buffer and signalling
>>> the interrupt.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>
>> Hello Ben,
>>
>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
> 
> Personally I think the original version was fine. It has been tested
> and returns fairly quickly (I am not a fan of just adding more delay in)
> 
> My notes say the condition doesn't last for long and the loop tends
> to terminate within 2 runs.
> 

Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.

[0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-02-22 17:23 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2019-02-22 17:23 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 21/02/2019 13:02, Dmitry Osipenko wrote:
> 21.02.2019 13:06, Ben Dooks пишет:
>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>> The tx_status callback does not report the state of the transfer
>>>> beyond complete segments. This causes problems with users such as
>>>> ALSA when applications want to know accurately how much data has
>>>> been moved.
>>>>
>>>> This patch addes a function tegra_dma_update_residual() to query
>>>> the hardware and modify the residual information accordinly. It
>>>> takes into account any hardware issues when trying to read the
>>>> state, such as delays between finishing a buffer and signalling
>>>> the interrupt.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>
>>> Hello Ben,
>>>
>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>
>> Personally I think the original version was fine. It has been tested
>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>
>> My notes say the condition doesn't last for long and the loop tends
>> to terminate within 2 runs.
>>
> 
> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
> 
> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
> 

I can try rebasing, but I have not got a lot of time to do any testing
at the moment. I agree I should have remembered to chase this stuff up
earlier.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-02-22 18:10 Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-02-22 18:10 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

22.02.2019 20:23, Ben Dooks пишет:
> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>> 21.02.2019 13:06, Ben Dooks пишет:
>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>> The tx_status callback does not report the state of the transfer
>>>>> beyond complete segments. This causes problems with users such as
>>>>> ALSA when applications want to know accurately how much data has
>>>>> been moved.
>>>>>
>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>> the hardware and modify the residual information accordinly. It
>>>>> takes into account any hardware issues when trying to read the
>>>>> state, such as delays between finishing a buffer and signalling
>>>>> the interrupt.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>
>>>> Hello Ben,
>>>>
>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>
>>> Personally I think the original version was fine. It has been tested
>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>
>>> My notes say the condition doesn't last for long and the loop tends
>>> to terminate within 2 runs.
>>>
>>
>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>
>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>
> 
> I can try rebasing, but I have not got a lot of time to do any testing
> at the moment. I agree I should have remembered to chase this stuff up
> earlier.

No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-14 15:20 ` Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-04-14 15:20 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

22.02.2019 21:10, Dmitry Osipenko пишет:
> 22.02.2019 20:23, Ben Dooks пишет:
>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>> The tx_status callback does not report the state of the transfer
>>>>>> beyond complete segments. This causes problems with users such as
>>>>>> ALSA when applications want to know accurately how much data has
>>>>>> been moved.
>>>>>>
>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>> the hardware and modify the residual information accordinly. It
>>>>>> takes into account any hardware issues when trying to read the
>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>> the interrupt.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>
>>>>> Hello Ben,
>>>>>
>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>
>>>> Personally I think the original version was fine. It has been tested
>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>
>>>> My notes say the condition doesn't last for long and the loop tends
>>>> to terminate within 2 runs.
>>>>
>>>
>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>
>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>
>>
>> I can try rebasing, but I have not got a lot of time to do any testing
>> at the moment. I agree I should have remembered to chase this stuff up
>> earlier.
> 
> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
> 

Hello Ben,

Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-14 15:20 ` Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-04-14 15:20 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

22.02.2019 21:10, Dmitry Osipenko пишет:
> 22.02.2019 20:23, Ben Dooks пишет:
>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>> The tx_status callback does not report the state of the transfer
>>>>>> beyond complete segments. This causes problems with users such as
>>>>>> ALSA when applications want to know accurately how much data has
>>>>>> been moved.
>>>>>>
>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>> the hardware and modify the residual information accordinly. It
>>>>>> takes into account any hardware issues when trying to read the
>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>> the interrupt.
>>>>>>
>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>
>>>>> Hello Ben,
>>>>>
>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>
>>>> Personally I think the original version was fine. It has been tested
>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>
>>>> My notes say the condition doesn't last for long and the loop tends
>>>> to terminate within 2 runs.
>>>>
>>>
>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>
>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>
>>
>> I can try rebasing, but I have not got a lot of time to do any testing
>> at the moment. I agree I should have remembered to chase this stuff up
>> earlier.
> 
> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
> 

Hello Ben,

Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-15 15:01 ` Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2019-04-15 15:01 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 14/04/2019 16:20, Dmitry Osipenko wrote:
> 22.02.2019 21:10, Dmitry Osipenko пишет:
>> 22.02.2019 20:23, Ben Dooks пишет:
>>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>>> The tx_status callback does not report the state of the transfer
>>>>>>> beyond complete segments. This causes problems with users such as
>>>>>>> ALSA when applications want to know accurately how much data has
>>>>>>> been moved.
>>>>>>>
>>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>>> the hardware and modify the residual information accordinly. It
>>>>>>> takes into account any hardware issues when trying to read the
>>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>>> the interrupt.
>>>>>>>
>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>
>>>>>> Hello Ben,
>>>>>>
>>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>>
>>>>> Personally I think the original version was fine. It has been tested
>>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>>
>>>>> My notes say the condition doesn't last for long and the loop tends
>>>>> to terminate within 2 runs.
>>>>>
>>>>
>>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>>
>>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>>
>>>
>>> I can try rebasing, but I have not got a lot of time to do any testing
>>> at the moment. I agree I should have remembered to chase this stuff up
>>> earlier.
>>
>> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
>>
> 
> Hello Ben,
> 
> Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.

I am currently re-looking at the updates for TDM audio and slave mode
audio for the tegra, I should be able to re-test the original patch
tomorrow.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-15 15:01 ` Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2019-04-15 15:01 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 14/04/2019 16:20, Dmitry Osipenko wrote:
> 22.02.2019 21:10, Dmitry Osipenko пишет:
>> 22.02.2019 20:23, Ben Dooks пишет:
>>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>>> The tx_status callback does not report the state of the transfer
>>>>>>> beyond complete segments. This causes problems with users such as
>>>>>>> ALSA when applications want to know accurately how much data has
>>>>>>> been moved.
>>>>>>>
>>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>>> the hardware and modify the residual information accordinly. It
>>>>>>> takes into account any hardware issues when trying to read the
>>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>>> the interrupt.
>>>>>>>
>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>
>>>>>> Hello Ben,
>>>>>>
>>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>>
>>>>> Personally I think the original version was fine. It has been tested
>>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>>
>>>>> My notes say the condition doesn't last for long and the loop tends
>>>>> to terminate within 2 runs.
>>>>>
>>>>
>>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>>
>>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>>
>>>
>>> I can try rebasing, but I have not got a lot of time to do any testing
>>> at the moment. I agree I should have remembered to chase this stuff up
>>> earlier.
>>
>> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
>>
> 
> Hello Ben,
> 
> Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.

I am currently re-looking at the updates for TDM audio and slave mode
audio for the tegra, I should be able to re-test the original patch
tomorrow.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-15 16:56 ` Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-04-15 16:56 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

15.04.2019 18:01, Ben Dooks пишет:
> On 14/04/2019 16:20, Dmitry Osipenko wrote:
>> 22.02.2019 21:10, Dmitry Osipenko пишет:
>>> 22.02.2019 20:23, Ben Dooks пишет:
>>>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>>>> The tx_status callback does not report the state of the transfer
>>>>>>>> beyond complete segments. This causes problems with users such as
>>>>>>>> ALSA when applications want to know accurately how much data has
>>>>>>>> been moved.
>>>>>>>>
>>>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>>>> the hardware and modify the residual information accordinly. It
>>>>>>>> takes into account any hardware issues when trying to read the
>>>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>>>> the interrupt.
>>>>>>>>
>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>
>>>>>>> Hello Ben,
>>>>>>>
>>>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>>>
>>>>>> Personally I think the original version was fine. It has been tested
>>>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>>>
>>>>>> My notes say the condition doesn't last for long and the loop tends
>>>>>> to terminate within 2 runs.
>>>>>>
>>>>>
>>>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>>>
>>>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>>>
>>>>
>>>> I can try rebasing, but I have not got a lot of time to do any testing
>>>> at the moment. I agree I should have remembered to chase this stuff up
>>>> earlier.
>>>
>>> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
>>>
>>
>> Hello Ben,
>>
>> Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.
> 
> I am currently re-looking at the updates for TDM audio and slave mode
> audio for the tegra, I should be able to re-test the original patch
> tomorrow.

Sounds great, thank you very much.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 4/6] dma: tegra: add accurate reporting of dma state
@ 2019-04-15 16:56 ` Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2019-04-15 16:56 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

15.04.2019 18:01, Ben Dooks пишет:
> On 14/04/2019 16:20, Dmitry Osipenko wrote:
>> 22.02.2019 21:10, Dmitry Osipenko пишет:
>>> 22.02.2019 20:23, Ben Dooks пишет:
>>>> On 21/02/2019 13:02, Dmitry Osipenko wrote:
>>>>> 21.02.2019 13:06, Ben Dooks пишет:
>>>>>> On 21/02/2019 00:41, Dmitry Osipenko wrote:
>>>>>>> 31.10.2018 19:03, Ben Dooks пишет:
>>>>>>>> The tx_status callback does not report the state of the transfer
>>>>>>>> beyond complete segments. This causes problems with users such as
>>>>>>>> ALSA when applications want to know accurately how much data has
>>>>>>>> been moved.
>>>>>>>>
>>>>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>>>>> the hardware and modify the residual information accordinly. It
>>>>>>>> takes into account any hardware issues when trying to read the
>>>>>>>> state, such as delays between finishing a buffer and signalling
>>>>>>>> the interrupt.
>>>>>>>>
>>>>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>>>>
>>>>>>> Hello Ben,
>>>>>>>
>>>>>>> Do you have any plans to submit a new version of this patch? It's really useful and fixes a real problem with the audio playback. I could help with finalizing the patch and could submit it for you if you happened to lost the interest.
>>>>>>
>>>>>> Personally I think the original version was fine. It has been tested
>>>>>> and returns fairly quickly (I am not a fan of just adding more delay in)
>>>>>>
>>>>>> My notes say the condition doesn't last for long and the loop tends
>>>>>> to terminate within 2 runs.
>>>>>>
>>>>>
>>>>> Okay, so are you going to re-send the patch? We can back to the review after, you need at least to re-send because this series has been outdated. Also please take a look and feel free to use as-is the reduced variant of yours patch that I was carrying and testing for months now [0], it works great.
>>>>>
>>>>> [0] https://github.com/grate-driver/linux/commit/ab8a67a6f47185f265f16749b55df214aaaefad4
>>>>>
>>>>
>>>> I can try rebasing, but I have not got a lot of time to do any testing
>>>> at the moment. I agree I should have remembered to chase this stuff up
>>>> earlier.
>>>
>>> No problems, thank you. I'll help with the testing. And I could rebase the patch and send it out for you if will be needed, please just let me know if you're okay with it.
>>>
>>
>> Hello Ben,
>>
>> Do you have any status update on the state of the patch? Again, I could help with sending it out for you if you're too busy or something else. Please let me know how we could proceed with getting the fix upstreamed and sorry for disturbing you again with this.
> 
> I am currently re-looking at the updates for TDM audio and slave mode
> audio for the tegra, I should be able to re-test the original patch
> tomorrow.

Sounds great, thank you very much.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-05 14:05 Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2018-11-05 14:05 UTC (permalink / raw)
  To: Ben Dooks; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 05.11.2018 12:03, Ben Dooks wrote:
> 
> 
> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>> On 31.10.2018 19:03, Ben Dooks wrote:
>>> The tx_status callback does not report the state of the transfer
>>> beyond complete segments. This causes problems with users such as
>>> ALSA when applications want to know accurately how much data has
>>> been moved.
>>>
>>> This patch addes a function tegra_dma_update_residual() to query
>>> the hardware and modify the residual information accordinly. It
>>> takes into account any hardware issues when trying to read the
>>> state, such as delays between finishing a buffer and signalling
>>> the interrupt.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>
>> Hello Ben,
>>
>> Thank you very much for the patch! It fixes "farting sound" for
>> chromium-browser and applications that use chromium-engine (tested on
>> Tegra20) because apparently it tries to use low latency for everything
>> and audio buffer is constantly underflowing without more detailed
>> DMA-progress reporting. See couple more comments below.
>>
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
>>>  1 file changed, 87 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index 4f7d1e576d03..3fa3a1ac4f57 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>>>      return 0;
>>>  }
>>>
>>> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
>>> +                          struct tegra_dma_sg_req *sg_req,
>>> +                          struct tegra_dma_desc *dma_desc,
>>> +                          unsigned int residual)
>>> +{
>>> +    unsigned long status = 0x0;
>>
>> There is no need to initialize "status" variable.
> 
> ok, will check this.
> 
>>> +    unsigned long wcount;
>>> +    unsigned long ahbptr;
>>> +    unsigned long tmp = 0x0;
>>> +    unsigned int result;
>>> +    int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
>>> +    int done;
>>> +
>>> +    /* if we're not the current request, then don't alter the residual */
>>> +    if (sg_req != list_first_entry(&tdc->pending_sg_req,
>>> +                       struct tegra_dma_sg_req, node)) {
>>> +        result = residual;
>>> +        ahbptr = 0xffffffff;
>>> +        goto done;
>>> +    }
>>> +
>>> +    /* loop until we have a reliable result for residual */
>>> +    do {
>>> +        ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
>>> +        status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
>>
>>> +        tmp =  tdc_read(tdc, 0x08);    /* total count for debug */
>>
>> Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp"
>> isn't used anywhere in the code. Please remove it entirely.
> 
> ok, fixed
> 
>>> +
>>> +        /* check status, if channel isn't busy then skip */
>>> +        if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>>> +            result = residual;
>>> +            break;
>>> +        }
>>
>> If "BUSY" is unset, doesn't this mean that transaction could be
>> completed already? I don't quite understand why you want to skip here.
> 
> I can't remember what the reasoning behind this was, this work was
> originally done over a year ago and I am not sure if I can find any
> of the notes from this.
> 

Looks like it should be safe to remove this hunk. Please consider the removal or provide reasoning in the comment to the code if you'll recall it.

>>> +
>>> +        /* if we've got an interrupt pending on the channel, don't
>>> +         * try and deal with the residue as the hardware has likely
>>> +         * moved on to the next buffer. return all data moved.
>>> +         */
>>> +        if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>>> +            result = residual - sg_req->req_len;
>>> +            break;
>>> +        }
>>> +
>>> +        if (tdc->tdma->chip_data->support_separate_wcount_reg)
>>> +            wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>>> +        else
>>> +            wcount = status;
>>> +
>>> +        /* If the request is at the full point, then there is a
>>> +         * chance that we have read the status register in the
>>> +         * middle of the hardware reloading the next buffer.
>>> +         *
>>> +         * The sequence seems to be at the end of the buffer, to
>>> +         * load the new word count before raising the EOC flag (or
>>> +         * changing the ping-pong flag which could have also been
>>> +         * used to determine a new buffer). This  means there is a
>>                                two whitespaces here-----|
>>
>>> +         * small window where we cannot determine zero-done for the
>>> +         * current buffer, or moved to next buffer.
>>> +         *
>>
>>> +         * If done shows 0, then retry the load, as it may hit the
>>> +         * above hardware race. We will either get a new value which
>>> +         * is from the first buffer, or we get an EOC (new buffer)
>>> +         * or both a new value and an EOC...
>>
>> I think we just need to wait 20usec after reading out "words count"
>> and then re-check interrupt status, so transfer is done if interrupt
>> is set and otherwise "words count" value is actual and reliable.
> 
> At the moment I have no way of going back and re-testing this code
> against the problem it originally fixed, so would rather not change
> the algorithm in this.

Please tell what was the problem.

>>
>>> +         */
>>> +        done = get_current_xferred_count(tdc, sg_req, wcount);
>>> +        if (done != 0) {
>>> +            result = residual - done;
>>> +            break;
>>> +        }
>>> +
>>> +        ndelay(100);
>>
>> There is no ndelay() on ARM, hence your 20usec timeout is 200usec.
>> Please use udelay().
> 
> I thought there was one based on the lpj calculations, I will check later.
> 
>>> +    } while (--retries > 0);
>>> +
>>> +    if (retries <= 0) {
>>> +        dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
>>> +        result = residual;
>>> +    }
>>> +
>>> +done:
>>
>> Please rename goto label as it duplicates local variable name.
> 
> ok, fixed.
> 
>>> +    dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
>>> +         sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
>>
>> Whitespace just after tab not needed.
>>
>>> +
>>> +    return result;
>>> +}
>>> +
>>>  static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>>      dma_cookie_t cookie, struct dma_tx_state *txstate)
>>>  {
>>>      struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>      struct tegra_dma_desc *dma_desc;
>>> -    struct tegra_dma_sg_req *sg_req;
>>> +    struct tegra_dma_sg_req *sg_req = NULL;
>>>      enum dma_status ret;
>>>      unsigned long flags;
>>>      unsigned int residual;
>>> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>>          residual = dma_desc->bytes_requested -
>>>                 (dma_desc->bytes_transferred %
>>>                  dma_desc->bytes_requested);
>>> +        residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);

Line over 80 characters. Please make sure that "scripts/checkpatch.pl" doesn't have valid complains about the patches.

[snip]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-05 11:38 Jon Hunter
  0 siblings, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2018-11-05 11:38 UTC (permalink / raw)
  To: Ben Dooks, Dmitry Osipenko
  Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 05/11/2018 11:34, Ben Dooks wrote:
> On 05/11/18 11:32, Jon Hunter wrote:
>> Hi Ben,
>>
>> On 05/11/2018 09:03, Ben Dooks wrote:
>>> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>>>> On 31.10.2018 19:03, Ben Dooks wrote:
>>>>> The tx_status callback does not report the state of the transfer
>>>>> beyond complete segments. This causes problems with users such as
>>>>> ALSA when applications want to know accurately how much data has
>>>>> been moved.
>>>>>
>>>>> This patch addes a function tegra_dma_update_residual() to query
>>>>> the hardware and modify the residual information accordinly. It
>>>>> takes into account any hardware issues when trying to read the
>>>>> state, such as delays between finishing a buffer and signalling
>>>>> the interrupt.
>>>>>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> Can you CC linux-tegra on these patches? I am always interested in
>> updates to the DMA drivers.
> 
> Is linux-tegra@vger.kernel.org not the right address for the list then?

That's the right one. However, for some reason I did not receive the
complete series only a few responses to some patches.

Jon

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-05 11:34 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2018-11-05 11:34 UTC (permalink / raw)
  To: Jon Hunter, Dmitry Osipenko
  Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 05/11/18 11:32, Jon Hunter wrote:
> Hi Ben,
> 
> On 05/11/2018 09:03, Ben Dooks wrote:
>> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>>> On 31.10.2018 19:03, Ben Dooks wrote:
>>>> The tx_status callback does not report the state of the transfer
>>>> beyond complete segments. This causes problems with users such as
>>>> ALSA when applications want to know accurately how much data has
>>>> been moved.
>>>>
>>>> This patch addes a function tegra_dma_update_residual() to query
>>>> the hardware and modify the residual information accordinly. It
>>>> takes into account any hardware issues when trying to read the
>>>> state, such as delays between finishing a buffer and signalling
>>>> the interrupt.
>>>>
>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> Can you CC linux-tegra on these patches? I am always interested in
> updates to the DMA drivers.

Is linux-tegra@vger.kernel.org not the right address for the list then?

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-05 11:32 Jon Hunter
  0 siblings, 0 replies; 20+ messages in thread
From: Jon Hunter @ 2018-11-05 11:32 UTC (permalink / raw)
  To: Ben Dooks, Dmitry Osipenko
  Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

Hi Ben,

On 05/11/2018 09:03, Ben Dooks wrote:
> On 2018-11-03 12:24, Dmitry Osipenko wrote:
>> On 31.10.2018 19:03, Ben Dooks wrote:
>>> The tx_status callback does not report the state of the transfer
>>> beyond complete segments. This causes problems with users such as
>>> ALSA when applications want to know accurately how much data has
>>> been moved.
>>>
>>> This patch addes a function tegra_dma_update_residual() to query
>>> the hardware and modify the residual information accordinly. It
>>> takes into account any hardware issues when trying to read the
>>> state, such as delays between finishing a buffer and signalling
>>> the interrupt.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
Can you CC linux-tegra on these patches? I am always interested in
updates to the DMA drivers.

Cheers!
Jon

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-05  9:03 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2018-11-05  9:03 UTC (permalink / raw)
  To: Dmitry Osipenko; +Cc: dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

On 2018-11-03 12:24, Dmitry Osipenko wrote:
> On 31.10.2018 19:03, Ben Dooks wrote:
>> The tx_status callback does not report the state of the transfer
>> beyond complete segments. This causes problems with users such as
>> ALSA when applications want to know accurately how much data has
>> been moved.
>> 
>> This patch addes a function tegra_dma_update_residual() to query
>> the hardware and modify the residual information accordinly. It
>> takes into account any hardware issues when trying to read the
>> state, such as delays between finishing a buffer and signalling
>> the interrupt.
>> 
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> 
> Hello Ben,
> 
> Thank you very much for the patch! It fixes "farting sound" for
> chromium-browser and applications that use chromium-engine (tested on
> Tegra20) because apparently it tries to use low latency for everything
> and audio buffer is constantly underflowing without more detailed
> DMA-progress reporting. See couple more comments below.
> 
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 94 
>> ++++++++++++++++++++++++++++++++---
>>  1 file changed, 87 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/dma/tegra20-apb-dma.c 
>> b/drivers/dma/tegra20-apb-dma.c
>> index 4f7d1e576d03..3fa3a1ac4f57 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct 
>> dma_chan *dc)
>>  	return 0;
>>  }
>> 
>> +static unsigned int tegra_dma_update_residual(struct 
>> tegra_dma_channel *tdc,
>> +					      struct tegra_dma_sg_req *sg_req,
>> +					      struct tegra_dma_desc *dma_desc,
>> +					      unsigned int residual)
>> +{
>> +	unsigned long status = 0x0;
> 
> There is no need to initialize "status" variable.

ok, will check this.

>> +	unsigned long wcount;
>> +	unsigned long ahbptr;
>> +	unsigned long tmp = 0x0;
>> +	unsigned int result;
>> +	int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
>> +	int done;
>> +
>> +	/* if we're not the current request, then don't alter the residual 
>> */
>> +	if (sg_req != list_first_entry(&tdc->pending_sg_req,
>> +				       struct tegra_dma_sg_req, node)) {
>> +		result = residual;
>> +		ahbptr = 0xffffffff;
>> +		goto done;
>> +	}
>> +
>> +	/* loop until we have a reliable result for residual */
>> +	do {
>> +		ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
>> +		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> 
>> +		tmp =  tdc_read(tdc, 0x08);	/* total count for debug */
> 
> Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp"
> isn't used anywhere in the code. Please remove it entirely.

ok, fixed

>> +
>> +		/* check status, if channel isn't busy then skip */
>> +		if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
>> +			result = residual;
>> +			break;
>> +		}
> 
> If "BUSY" is unset, doesn't this mean that transaction could be
> completed already? I don't quite understand why you want to skip here.

I can't remember what the reasoning behind this was, this work was
originally done over a year ago and I am not sure if I can find any
of the notes from this.

>> +
>> +		/* if we've got an interrupt pending on the channel, don't
>> +		 * try and deal with the residue as the hardware has likely
>> +		 * moved on to the next buffer. return all data moved.
>> +		 */
>> +		if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
>> +			result = residual - sg_req->req_len;
>> +			break;
>> +		}
>> +
>> +		if (tdc->tdma->chip_data->support_separate_wcount_reg)
>> +			wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
>> +		else
>> +			wcount = status;
>> +
>> +		/* If the request is at the full point, then there is a
>> +		 * chance that we have read the status register in the
>> +		 * middle of the hardware reloading the next buffer.
>> +		 *
>> +		 * The sequence seems to be at the end of the buffer, to
>> +		 * load the new word count before raising the EOC flag (or
>> +		 * changing the ping-pong flag which could have also been
>> +		 * used to determine a new buffer). This  means there is a
>                                two whitespaces here-----|
> 
>> +		 * small window where we cannot determine zero-done for the
>> +		 * current buffer, or moved to next buffer.
>> +		 *
> 
>> +		 * If done shows 0, then retry the load, as it may hit the
>> +		 * above hardware race. We will either get a new value which
>> +		 * is from the first buffer, or we get an EOC (new buffer)
>> +		 * or both a new value and an EOC...
> 
> I think we just need to wait 20usec after reading out "words count"
> and then re-check interrupt status, so transfer is done if interrupt
> is set and otherwise "words count" value is actual and reliable.

At the moment I have no way of going back and re-testing this code
against the problem it originally fixed, so would rather not change
the algorithm in this.

> 
>> +		 */
>> +		done = get_current_xferred_count(tdc, sg_req, wcount);
>> +		if (done != 0) {
>> +			result = residual - done;
>> +			break;
>> +		}
>> +
>> +		ndelay(100);
> 
> There is no ndelay() on ARM, hence your 20usec timeout is 200usec.
> Please use udelay().

I thought there was one based on the lpj calculations, I will check 
later.

>> +	} while (--retries > 0);
>> +
>> +	if (retries <= 0) {
>> +		dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
>> +		result = residual;
>> +	}
>> +
>> +done:
> 
> Please rename goto label as it duplicates local variable name.

ok, fixed.

>> +	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, 
>> done %d\n",
>> +		 sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
> 
> Whitespace just after tab not needed.
> 
>> +
>> +	return result;
>> +}
>> +
>>  static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>>  	dma_cookie_t cookie, struct dma_tx_state *txstate)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>  	struct tegra_dma_desc *dma_desc;
>> -	struct tegra_dma_sg_req *sg_req;
>> +	struct tegra_dma_sg_req *sg_req = NULL;
>>  	enum dma_status ret;
>>  	unsigned long flags;
>>  	unsigned int residual;
>> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct 
>> dma_chan *dc,
>>  		residual = dma_desc->bytes_requested -
>>  			   (dma_desc->bytes_transferred %
>>  			    dma_desc->bytes_requested);
>> +		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, 
>> residual);
>>  		dma_set_residue(txstate, residual);
>>  	}
>> 
>> @@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct 
>> platform_device *pdev)
>>  		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>>  		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
>>  	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | 
>> BIT(DMA_MEM_TO_DEV);
>> -	/*
>> -	 * XXX The hardware appears to support
>> -	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
>> -	 * only used by this driver during tegra_dma_terminate_all()
>> -	 */
>> -	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
>> +	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>>  	tdma->dma_dev.device_config = tegra_dma_slave_config;
>>  	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
>>  	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
>> 
> 
> Summarizing all of the comments above, patch may look like this:
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c 
> b/drivers/dma/tegra20-apb-dma.c
> index 9a558e30c461..956e8130c059 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -799,12 +799,73 @@ static int tegra_dma_terminate_all(struct 
> dma_chan *dc)
>  	return 0;
>  }
> 
> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel 
> *tdc,
> +					      struct tegra_dma_sg_req *sg_req,
> +					      struct tegra_dma_desc *dma_desc,
> +					      unsigned int residual)
> +{
> +	unsigned long status;
> +	unsigned int result;
> +	int done;
> +
> +	/* if we're not the current request, then don't alter the residual */
> +	if (sg_req != list_first_entry(&tdc->pending_sg_req,
> +				       struct tegra_dma_sg_req, node))
> +		return residual;
> +
> +	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> +
> +	/* if we've got an interrupt pending on the channel, don't
> +	 * try and deal with the residue as the hardware has likely
> +	 * moved on to the next buffer. return all data moved.
> +	 */
> +	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> +		result = residual - sg_req->req_len;
> +		goto out;
> +	}
> +
> +	if (tdc->tdma->chip_data->support_separate_wcount_reg)
> +		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
> +
> +	/*
> +	 * If the request is at the full point, then there is a
> +	 * chance that we have read the status register in the
> +	 * middle of the hardware reloading the next buffer.
> +	 *
> +	 * The sequence seems to be at the end of the buffer, to
> +	 * load the new word count before raising the EOC flag (or
> +	 * changing the ping-pong flag which could have also been
> +	 * used to determine a new buffer). This  means there is a
> +	 * small window where we cannot determine zero-done for the
> +	 * current buffer, or moved to next buffer.
> +	 */
> +	done = get_current_xferred_count(tdc, sg_req, status);
> +
> +	udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
> +
> +	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
> +
> +	if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
> +		result = residual - sg_req->req_len;
> +	else
> +		result = residual - done;
> +out:
> +#ifdef DEBUG
> +	ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
> +
> +	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx,
> done %d\n",
> +		sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
> +#endif
> +
> +	return result;
> +}
> +
>  static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>  	dma_cookie_t cookie, struct dma_tx_state *txstate)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>  	struct tegra_dma_desc *dma_desc;
> -	struct tegra_dma_sg_req *sg_req;
> +	struct tegra_dma_sg_req *sg_req = NULL;
>  	enum dma_status ret;
>  	unsigned long flags;
>  	unsigned int residual;
> @@ -840,6 +901,7 @@ static enum dma_status tegra_dma_tx_status(struct
> dma_chan *dc,
>  		residual = dma_desc->bytes_requested -
>  			   (dma_desc->bytes_transferred %
>  			    dma_desc->bytes_requested);
> +		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, 
> residual);
>  		dma_set_residue(txstate, residual);
>  	}
> 
> @@ -1433,12 +1495,7 @@ static int tegra_dma_probe(struct 
> platform_device *pdev)
>  		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
>  	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> -	/*
> -	 * XXX The hardware appears to support
> -	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
> -	 * only used by this driver during tegra_dma_terminate_all()
> -	 */
> -	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> +	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>  	tdma->dma_dev.device_config = tegra_dma_slave_config;
>  	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
>  	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-11-03 12:24 Dmitry Osipenko
  0 siblings, 0 replies; 20+ messages in thread
From: Dmitry Osipenko @ 2018-11-03 12:24 UTC (permalink / raw)
  To: Ben Dooks, dan.j.williams, vkoul; +Cc: ldewangan, dmaengine, linux-tegra

On 31.10.2018 19:03, Ben Dooks wrote:
> The tx_status callback does not report the state of the transfer
> beyond complete segments. This causes problems with users such as
> ALSA when applications want to know accurately how much data has
> been moved.
> 
> This patch addes a function tegra_dma_update_residual() to query
> the hardware and modify the residual information accordinly. It
> takes into account any hardware issues when trying to read the
> state, such as delays between finishing a buffer and signalling
> the interrupt.
> 
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>

Hello Ben,

Thank you very much for the patch! It fixes "farting sound" for chromium-browser and applications that use chromium-engine (tested on Tegra20) because apparently it tries to use low latency for everything and audio buffer is constantly underflowing without more detailed DMA-progress reporting. See couple more comments below.

> ---
>  drivers/dma/tegra20-apb-dma.c | 94 ++++++++++++++++++++++++++++++++---
>  1 file changed, 87 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 4f7d1e576d03..3fa3a1ac4f57 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -802,12 +802,96 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>  	return 0;
>  }
>  
> +static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
> +					      struct tegra_dma_sg_req *sg_req,
> +					      struct tegra_dma_desc *dma_desc,
> +					      unsigned int residual)
> +{
> +	unsigned long status = 0x0;

There is no need to initialize "status" variable.

> +	unsigned long wcount;
> +	unsigned long ahbptr;
> +	unsigned long tmp = 0x0;
> +	unsigned int result;
> +	int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
> +	int done;
> +
> +	/* if we're not the current request, then don't alter the residual */
> +	if (sg_req != list_first_entry(&tdc->pending_sg_req,
> +				       struct tegra_dma_sg_req, node)) {
> +		result = residual;
> +		ahbptr = 0xffffffff;
> +		goto done;
> +	}
> +
> +	/* loop until we have a reliable result for residual */
> +	do {
> +		ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
> +		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);

> +		tmp =  tdc_read(tdc, 0x08);	/* total count for debug */

Register 0x08 (DMA_BYTE_STA) doesn't present on Tegra20 and "tmp" isn't used anywhere in the code. Please remove it entirely.

> +
> +		/* check status, if channel isn't busy then skip */
> +		if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
> +			result = residual;
> +			break;
> +		}

If "BUSY" is unset, doesn't this mean that transaction could be completed already? I don't quite understand why you want to skip here.

> +
> +		/* if we've got an interrupt pending on the channel, don't
> +		 * try and deal with the residue as the hardware has likely
> +		 * moved on to the next buffer. return all data moved.
> +		 */
> +		if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
> +			result = residual - sg_req->req_len;
> +			break;
> +		}
> +
> +		if (tdc->tdma->chip_data->support_separate_wcount_reg)
> +			wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
> +		else
> +			wcount = status;
> +
> +		/* If the request is at the full point, then there is a
> +		 * chance that we have read the status register in the
> +		 * middle of the hardware reloading the next buffer.
> +		 *
> +		 * The sequence seems to be at the end of the buffer, to
> +		 * load the new word count before raising the EOC flag (or
> +		 * changing the ping-pong flag which could have also been
> +		 * used to determine a new buffer). This  means there is a
                               two whitespaces here-----|                      

> +		 * small window where we cannot determine zero-done for the
> +		 * current buffer, or moved to next buffer.
> +		 *

> +		 * If done shows 0, then retry the load, as it may hit the
> +		 * above hardware race. We will either get a new value which
> +		 * is from the first buffer, or we get an EOC (new buffer)
> +		 * or both a new value and an EOC...

I think we just need to wait 20usec after reading out "words count" and then re-check interrupt status, so transfer is done if interrupt is set and otherwise "words count" value is actual and reliable.

> +		 */
> +		done = get_current_xferred_count(tdc, sg_req, wcount);
> +		if (done != 0) {
> +			result = residual - done;
> +			break;
> +		}
> +
> +		ndelay(100);

There is no ndelay() on ARM, hence your 20usec timeout is 200usec. Please use udelay().

> +	} while (--retries > 0);
> +
> +	if (retries <= 0) {
> +		dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
> +		result = residual;
> +	}
> +
> +done:	

Please rename goto label as it duplicates local variable name.

> +	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
> +		 sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);

Whitespace just after tab not needed.
 
> +
> +	return result;
> +}
> +
>  static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>  	dma_cookie_t cookie, struct dma_tx_state *txstate)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>  	struct tegra_dma_desc *dma_desc;
> -	struct tegra_dma_sg_req *sg_req;
> +	struct tegra_dma_sg_req *sg_req = NULL;
>  	enum dma_status ret;
>  	unsigned long flags;
>  	unsigned int residual;
> @@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
>  		residual = dma_desc->bytes_requested -
>  			   (dma_desc->bytes_transferred %
>  			    dma_desc->bytes_requested);
> +		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
>  		dma_set_residue(txstate, residual);
>  	}
>  
> @@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
>  		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
>  	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> -	/*
> -	 * XXX The hardware appears to support
> -	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
> -	 * only used by this driver during tegra_dma_terminate_all()
> -	 */
> -	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> +	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>  	tdma->dma_dev.device_config = tegra_dma_slave_config;
>  	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
>  	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
> 

Summarizing all of the comments above, patch may look like this:

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 9a558e30c461..956e8130c059 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -799,12 +799,73 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	return 0;
 }
 
+static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
+					      struct tegra_dma_sg_req *sg_req,
+					      struct tegra_dma_desc *dma_desc,
+					      unsigned int residual)
+{
+	unsigned long status;
+	unsigned int result;
+	int done;
+
+	/* if we're not the current request, then don't alter the residual */
+	if (sg_req != list_first_entry(&tdc->pending_sg_req,
+				       struct tegra_dma_sg_req, node))
+		return residual;
+
+	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+	/* if we've got an interrupt pending on the channel, don't
+	 * try and deal with the residue as the hardware has likely
+	 * moved on to the next buffer. return all data moved.
+	 */
+	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
+		result = residual - sg_req->req_len;
+		goto out;
+	}
+
+	if (tdc->tdma->chip_data->support_separate_wcount_reg)
+		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+
+	/*
+	 * If the request is at the full point, then there is a
+	 * chance that we have read the status register in the
+	 * middle of the hardware reloading the next buffer.
+	 *
+	 * The sequence seems to be at the end of the buffer, to
+	 * load the new word count before raising the EOC flag (or
+	 * changing the ping-pong flag which could have also been
+	 * used to determine a new buffer). This  means there is a
+	 * small window where we cannot determine zero-done for the
+	 * current buffer, or moved to next buffer.
+	 */
+	done = get_current_xferred_count(tdc, sg_req, status);
+
+	udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
+
+	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+
+	if (status & TEGRA_APBDMA_STATUS_ISE_EOC)
+		result = residual - sg_req->req_len;
+	else
+		result = residual - done;
+out:
+#ifdef DEBUG
+	ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
+
+	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
+		sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
+#endif
+
+	return result;
+}
+
 static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 	dma_cookie_t cookie, struct dma_tx_state *txstate)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 	struct tegra_dma_desc *dma_desc;
-	struct tegra_dma_sg_req *sg_req;
+	struct tegra_dma_sg_req *sg_req = NULL;
 	enum dma_status ret;
 	unsigned long flags;
 	unsigned int residual;
@@ -840,6 +901,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 		residual = dma_desc->bytes_requested -
 			   (dma_desc->bytes_transferred %
 			    dma_desc->bytes_requested);
+		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
 		dma_set_residue(txstate, residual);
 	}
 
@@ -1433,12 +1495,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
 		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
 	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
-	/*
-	 * XXX The hardware appears to support
-	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
-	 * only used by this driver during tegra_dma_terminate_all()
-	 */
-	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	tdma->dma_dev.device_config = tegra_dma_slave_config;
 	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
 	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-10-12 21:47 kbuild test robot
  0 siblings, 0 replies; 20+ messages in thread
From: kbuild test robot @ 2018-10-12 21:47 UTC (permalink / raw)
  To: Ben Dooks
  Cc: kbuild-all, dan.j.williams, vkoul, ldewangan, dmaengine, linux-tegra

Hi Ben,

I love your patch! Perhaps something to improve:

[auto build test WARNING on sof-driver-fuweitax/master]
[also build test WARNING on v4.19-rc7 next-20181012]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ben-Dooks/dma-tegra-avoid-overflow-of-byte-tracking/20181013-023951
base:   https://github.com/fuweitax/linux master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/linux/printk.h:332:0,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers//dma/tegra20-apb-dma.c:20:
   drivers//dma/tegra20-apb-dma.c: In function 'tegra_dma_tx_status':
   include/linux/dynamic_debug.h:135:3: warning: 'done' may be used uninitialized in this function [-Wmaybe-uninitialized]
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
      ^~~~~~~~~~~~~~~~~
   drivers//dma/tegra20-apb-dma.c:816:6: note: 'done' was declared here
     int done;
         ^~~~
   In file included from include/linux/printk.h:332:0,
                    from include/linux/kernel.h:14,
                    from include/linux/clk.h:16,
                    from drivers//dma/tegra20-apb-dma.c:20:
   include/linux/dynamic_debug.h:135:3: warning: 'wcount' may be used uninitialized in this function [-Wmaybe-uninitialized]
      __dynamic_dev_dbg(&descriptor, dev, fmt, \
      ^~~~~~~~~~~~~~~~~
   drivers//dma/tegra20-apb-dma.c:811:16: note: 'wcount' was declared here
     unsigned long wcount;
                   ^~~~~~
>> drivers//dma/tegra20-apb-dma.c:843:30: warning: 'sg_req' may be used uninitialized in this function [-Wmaybe-uninitialized]
       result = residual - sg_req->req_len;
                           ~~~~~~^~~~~~~~~
   drivers//dma/tegra20-apb-dma.c:894:27: note: 'sg_req' was declared here
     struct tegra_dma_sg_req *sg_req;
                              ^~~~~~

vim +/sg_req +843 drivers//dma/tegra20-apb-dma.c

   804	
   805	static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
   806						      struct tegra_dma_sg_req *sg_req,
   807						      struct tegra_dma_desc *dma_desc,
   808						      unsigned int residual)
   809	{
   810		unsigned long status = 0x0;
   811		unsigned long wcount;
   812		unsigned long ahbptr;
   813		unsigned long tmp = 0x0;
   814		unsigned int result;
   815		int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
   816		int done;
   817	
   818		/* if we're not the current request, then don't alter the residual */
   819		if (sg_req != list_first_entry(&tdc->pending_sg_req,
   820					       struct tegra_dma_sg_req, node)) {
   821			result = residual;
   822			ahbptr = 0xffffffff;
   823			goto done;
   824		}
   825	
   826		/* loop until we have a reliable result for residual */
   827		do {
   828			ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
   829			status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
   830			tmp =  tdc_read(tdc, 0x08);	/* total count for debug */
   831	
   832			/* check status, if channel isn't busy then skip */
   833			if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
   834				result = residual;
   835				break;
   836			}
   837	
   838			/* if we've got an interrupt pending on the channel, don't
   839			 * try and deal with the residue as the hardware has likely
   840			 * moved on to the next buffer. return all data moved.
   841			 */
   842			if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
 > 843				result = residual - sg_req->req_len;
   844				break;
   845			}
   846	
   847			if (tdc->tdma->chip_data->support_separate_wcount_reg)
   848				wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
   849			else
   850				wcount = status;
   851	
   852			/* If the request is at the full point, then there is a
   853			 * chance that we have read the status register in the
   854			 * middle of the hardware reloading the next buffer.
   855			 *
   856			 * The sequence seems to be at the end of the buffer, to
   857			 * load the new word count before raising the EOC flag (or
   858			 * changing the ping-pong flag which could have also been
   859			 * used to determine a new buffer). This  means there is a
   860			 * small window where we cannot determine zero-done for the
   861			 * current buffer, or moved to next buffer.
   862			 *
   863			 * If done shows 0, then retry the load, as it may hit the
   864			 * above hardware race. We will either get a new value which
   865			 * is from the first buffer, or we get an EOC (new buffer)
   866			 * or both a new value and an EOC...
   867			 */
   868			done = get_current_xferred_count(tdc, sg_req, wcount);
   869			if (done != 0) {
   870				result = residual - done;
   871				break;
   872			}
   873	
   874			ndelay(100);
   875		} while (--retries > 0);
   876	
   877		if (retries <= 0) {
   878			dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
   879			result = residual;
   880		}
   881
---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [4/6] dma: tegra: add accurate reporting of dma state
@ 2018-10-12  9:44 Ben Dooks
  0 siblings, 0 replies; 20+ messages in thread
From: Ben Dooks @ 2018-10-12  9:44 UTC (permalink / raw)
  To: dan.j.williams, vkoul; +Cc: ldewangan, dmaengine, linux-tegra, Ben Dooks

The tx_status callback does not report the state of the transfer
beyond complete segments. This causes problems with users such as
ALSA when applications want to know accurately how much data has
been moved.

This patch addes a function tegra_dma_update_residual() to query
the hardware and modify the residual information accordinly. It
takes into account any hardware issues when trying to read the
state, such as delays between finishing a buffer and signalling
the interrupt.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 drivers/dma/tegra20-apb-dma.c | 92 ++++++++++++++++++++++++++++++++---
 1 file changed, 86 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 4f7d1e576d03..ce2888f67254 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -802,6 +802,90 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	return 0;
 }
 
+static unsigned int tegra_dma_update_residual(struct tegra_dma_channel *tdc,
+					      struct tegra_dma_sg_req *sg_req,
+					      struct tegra_dma_desc *dma_desc,
+					      unsigned int residual)
+{
+	unsigned long status = 0x0;
+	unsigned long wcount;
+	unsigned long ahbptr;
+	unsigned long tmp = 0x0;
+	unsigned int result;
+	int retries = TEGRA_APBDMA_BURST_COMPLETE_TIME * 10;
+	int done;
+
+	/* if we're not the current request, then don't alter the residual */
+	if (sg_req != list_first_entry(&tdc->pending_sg_req,
+				       struct tegra_dma_sg_req, node)) {
+		result = residual;
+		ahbptr = 0xffffffff;
+		goto done;
+	}
+
+	/* loop until we have a reliable result for residual */
+	do {
+		ahbptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
+		status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
+		tmp =  tdc_read(tdc, 0x08);	/* total count for debug */
+
+		/* check status, if channel isn't busy then skip */
+		if (!(status & TEGRA_APBDMA_STATUS_BUSY)) {
+			result = residual;
+			break;
+		}
+
+		/* if we've got an interrupt pending on the channel, don't
+		 * try and deal with the residue as the hardware has likely
+		 * moved on to the next buffer. return all data moved.
+		 */
+		if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
+			result = residual - sg_req->req_len;
+			break;
+		}
+
+		if (tdc->tdma->chip_data->support_separate_wcount_reg)
+			wcount = tdc_read(tdc, TEGRA_APBDMA_CHAN_WORD_TRANSFER);
+		else
+			wcount = status;
+
+		/* If the request is at the full point, then there is a
+		 * chance that we have read the status register in the
+		 * middle of the hardware reloading the next buffer.
+		 *
+		 * The sequence seems to be at the end of the buffer, to
+		 * load the new word count before raising the EOC flag (or
+		 * changing the ping-pong flag which could have also been
+		 * used to determine a new buffer). This  means there is a
+		 * small window where we cannot determine zero-done for the
+		 * current buffer, or moved to next buffer.
+		 *
+		 * If done shows 0, then retry the load, as it may hit the
+		 * above hardware race. We will either get a new value which
+		 * is from the first buffer, or we get an EOC (new buffer)
+		 * or both a new value and an EOC...
+		 */
+		done = get_current_xferred_count(tdc, sg_req, wcount);
+		if (done != 0) {
+			result = residual - done;
+			break;
+		}
+
+		ndelay(100);
+	} while (--retries > 0);
+
+	if (retries <= 0) {
+		dev_err(tdc2dev(tdc), "timeout waiting for dma load\n");
+		result = residual;
+	}
+
+done:	
+	dev_dbg(tdc2dev(tdc), "residual: req %08lx, ahb@%08lx, wcount %08lx, done %d\n",
+		 sg_req->ch_regs.ahb_ptr, ahbptr, wcount, done);
+
+	return result;
+}
+
 static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 	dma_cookie_t cookie, struct dma_tx_state *txstate)
 {
@@ -843,6 +927,7 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 		residual = dma_desc->bytes_requested -
 			   (dma_desc->bytes_transferred %
 			    dma_desc->bytes_requested);
+		residual = tegra_dma_update_residual(tdc, sg_req, dma_desc, residual);
 		dma_set_residue(txstate, residual);
 	}
 
@@ -1436,12 +1521,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		BIT(DMA_SLAVE_BUSWIDTH_4_BYTES) |
 		BIT(DMA_SLAVE_BUSWIDTH_8_BYTES);
 	tdma->dma_dev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
-	/*
-	 * XXX The hardware appears to support
-	 * DMA_RESIDUE_GRANULARITY_BURST-level reporting, but it's
-	 * only used by this driver during tegra_dma_terminate_all()
-	 */
-	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	tdma->dma_dev.device_config = tegra_dma_slave_config;
 	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
 	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;

^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-04-15 16:56 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 17:23 [4/6] dma: tegra: add accurate reporting of dma state Ben Dooks
  -- strict thread matches above, loose matches on Subject: below --
2019-04-15 16:56 Dmitry Osipenko
2019-04-15 16:56 ` [PATCH 4/6] " Dmitry Osipenko
2019-04-15 15:01 [4/6] " Ben Dooks
2019-04-15 15:01 ` [PATCH 4/6] " Ben Dooks
2019-04-14 15:20 [4/6] " Dmitry Osipenko
2019-04-14 15:20 ` [PATCH 4/6] " Dmitry Osipenko
2019-02-22 18:10 [4/6] " Dmitry Osipenko
2019-02-21 13:02 Dmitry Osipenko
2019-02-21 10:06 Ben Dooks
2019-02-21  0:41 Dmitry Osipenko
2018-11-05 14:05 Dmitry Osipenko
2018-11-05 11:38 Jon Hunter
2018-11-05 11:34 Ben Dooks
2018-11-05 11:32 Jon Hunter
2018-11-05  9:03 Ben Dooks
2018-11-03 12:24 Dmitry Osipenko
2018-10-31 16:03 Ben Dooks
2018-10-12 21:47 kbuild test robot
2018-10-12  9:44 Ben Dooks

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.