[PATCH 0/3] Tegra Specific fixes

[PATCH 0/3] Tegra Specific fixes

[Mohan Kumar]

This series proposes following fixes to Tegra HDA driver.
* Align the buffer to 128 bytes for Tegra.
* Add new behavior flag dma_stop_delay in hdac_bus structure.
* Add 100us as dma stop delay for Tegra.

Mohan Kumar (3):
  ASoC: hda/tegra: Set buffer alignment to 128 bytes
  ALSA: hda: Add behaviour flag for dma stop delay
  ALSA: hda/tegra: Add 100us dma stop delay

 include/sound/hdaudio.h   | 2 ++
 sound/hda/hdac_stream.c   | 7 +++++++
 sound/pci/hda/hda_tegra.c | 3 +++
 3 files changed, 12 insertions(+)

-- 
2.17.1

[PATCH 0/3] Tegra Specific fixes

[Mohan Kumar]

This series proposes following fixes to Tegra HDA driver.
* Align the buffer to 128 bytes for Tegra.
* Add new behavior flag dma_stop_delay in hdac_bus structure.
* Add 100us as dma stop delay for Tegra.

Mohan Kumar (3):
  ASoC: hda/tegra: Set buffer alignment to 128 bytes
  ALSA: hda: Add behaviour flag for dma stop delay
  ALSA: hda/tegra: Add 100us dma stop delay

 include/sound/hdaudio.h   | 2 ++
 sound/hda/hdac_stream.c   | 7 +++++++
 sound/pci/hda/hda_tegra.c | 3 +++
 3 files changed, 12 insertions(+)

-- 
2.17.1

[PATCH 1/3] ASoC: hda/tegra: Set buffer alignment to 128 bytes

[Mohan Kumar]

Set  chip->align_buffer_size to 1 for Tegra platforms to make the buffer
alignment to be multiple of 128 bytes. This fix is applied as gstreamer
alsasink gets stuck with the default buffer-time and latency-time
parameters with 4 byte buffer alignment.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 5637f0129932..ecf98eb9df36 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -333,6 +333,8 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
 	gcap = azx_readw(chip, GCAP);
 	dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);
 
+	chip->align_buffer_size = 1;
+
 	/* read number of streams from GCAP register instead of using
 	 * hardcoded value
 	 */
-- 
2.17.1

[PATCH 1/3] ASoC: hda/tegra: Set buffer alignment to 128 bytes

[Mohan Kumar]

Set  chip->align_buffer_size to 1 for Tegra platforms to make the buffer
alignment to be multiple of 128 bytes. This fix is applied as gstreamer
alsasink gets stuck with the default buffer-time and latency-time
parameters with 4 byte buffer alignment.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 5637f0129932..ecf98eb9df36 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -333,6 +333,8 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
 	gcap = azx_readw(chip, GCAP);
 	dev_dbg(card->dev, "chipset global capabilities = 0x%x\n", gcap);
 
+	chip->align_buffer_size = 1;
+
 	/* read number of streams from GCAP register instead of using
 	 * hardcoded value
 	 */
-- 
2.17.1

[PATCH 2/3] ALSA: hda: Add behaviour flag for dma stop delay

[Mohan Kumar]

Behaviour flag dma_stop_delay is added as a new member in hdac_bus
structure to avoid memory decode error incase DMA RUN bit is not
disabled in the given timeout from snd_hdac_stream_sync function and
followed by stream reset which results in memory decode error between
reset set and clear operation.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 include/sound/hdaudio.h | 2 ++
 sound/hda/hdac_stream.c | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index c1f78d9a6e47..cf77c6b83016 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -347,6 +347,8 @@ struct hdac_bus {
 
 	int bdl_pos_adj;		/* BDL position adjustment */
 
+	unsigned int dma_stop_delay;
+
 	/* locks */
 	spinlock_t reg_lock;
 	struct mutex cmd_mutex;
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index a38a2af1654f..abe7a1b16fe1 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -150,9 +150,12 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
 {
 	unsigned char val;
 	int timeout;
+	int dma_run_state;
 
 	snd_hdac_stream_clear(azx_dev);
 
+	dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
+
 	snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
 	udelay(3);
 	timeout = 300;
@@ -162,6 +165,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
 		if (val)
 			break;
 	} while (--timeout);
+
+	if (azx_dev->bus->dma_stop_delay && dma_run_state)
+		udelay(azx_dev->bus->dma_stop_delay);
+
 	val &= ~SD_CTL_STREAM_RESET;
 	snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
 	udelay(3);
-- 
2.17.1

[PATCH 2/3] ALSA: hda: Add behaviour flag for dma stop delay

[Mohan Kumar]

Behaviour flag dma_stop_delay is added as a new member in hdac_bus
structure to avoid memory decode error incase DMA RUN bit is not
disabled in the given timeout from snd_hdac_stream_sync function and
followed by stream reset which results in memory decode error between
reset set and clear operation.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 include/sound/hdaudio.h | 2 ++
 sound/hda/hdac_stream.c | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index c1f78d9a6e47..cf77c6b83016 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -347,6 +347,8 @@ struct hdac_bus {
 
 	int bdl_pos_adj;		/* BDL position adjustment */
 
+	unsigned int dma_stop_delay;
+
 	/* locks */
 	spinlock_t reg_lock;
 	struct mutex cmd_mutex;
diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
index a38a2af1654f..abe7a1b16fe1 100644
--- a/sound/hda/hdac_stream.c
+++ b/sound/hda/hdac_stream.c
@@ -150,9 +150,12 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
 {
 	unsigned char val;
 	int timeout;
+	int dma_run_state;
 
 	snd_hdac_stream_clear(azx_dev);
 
+	dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
+
 	snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
 	udelay(3);
 	timeout = 300;
@@ -162,6 +165,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
 		if (val)
 			break;
 	} while (--timeout);
+
+	if (azx_dev->bus->dma_stop_delay && dma_run_state)
+		udelay(azx_dev->bus->dma_stop_delay);
+
 	val &= ~SD_CTL_STREAM_RESET;
 	snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
 	udelay(3);
-- 
2.17.1

Re: [PATCH 2/3] ALSA: hda: Add behaviour flag for dma stop delay

[Takashi Iwai]

On Wed, 05 Aug 2020 11:11:15 +0200,
Mohan Kumar wrote:
> 
> Behaviour flag dma_stop_delay is added as a new member in hdac_bus
> structure to avoid memory decode error incase DMA RUN bit is not
> disabled in the given timeout from snd_hdac_stream_sync function and
> followed by stream reset which results in memory decode error between
> reset set and clear operation.

The new field looks like not only a flag but also contains the actual
delay time in usec.  Please give a short comment at the struct field.


thanks,

Takashi

> 
> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> ---
>  include/sound/hdaudio.h | 2 ++
>  sound/hda/hdac_stream.c | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index c1f78d9a6e47..cf77c6b83016 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -347,6 +347,8 @@ struct hdac_bus {
>  
>  	int bdl_pos_adj;		/* BDL position adjustment */
>  
> +	unsigned int dma_stop_delay;
> +
>  	/* locks */
>  	spinlock_t reg_lock;
>  	struct mutex cmd_mutex;
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index a38a2af1654f..abe7a1b16fe1 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -150,9 +150,12 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
>  {
>  	unsigned char val;
>  	int timeout;
> +	int dma_run_state;
>  
>  	snd_hdac_stream_clear(azx_dev);
>  
> +	dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
> +
>  	snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
>  	udelay(3);
>  	timeout = 300;
> @@ -162,6 +165,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
>  		if (val)
>  			break;
>  	} while (--timeout);
> +
> +	if (azx_dev->bus->dma_stop_delay && dma_run_state)
> +		udelay(azx_dev->bus->dma_stop_delay);
> +
>  	val &= ~SD_CTL_STREAM_RESET;
>  	snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
>  	udelay(3);
> -- 
> 2.17.1
> 

Re: [PATCH 2/3] ALSA: hda: Add behaviour flag for dma stop delay

[Takashi Iwai]

On Wed, 05 Aug 2020 11:11:15 +0200,
Mohan Kumar wrote:
> 
> Behaviour flag dma_stop_delay is added as a new member in hdac_bus
> structure to avoid memory decode error incase DMA RUN bit is not
> disabled in the given timeout from snd_hdac_stream_sync function and
> followed by stream reset which results in memory decode error between
> reset set and clear operation.

The new field looks like not only a flag but also contains the actual
delay time in usec.  Please give a short comment at the struct field.


thanks,

Takashi

> 
> Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
> ---
>  include/sound/hdaudio.h | 2 ++
>  sound/hda/hdac_stream.c | 7 +++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index c1f78d9a6e47..cf77c6b83016 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -347,6 +347,8 @@ struct hdac_bus {
>  
>  	int bdl_pos_adj;		/* BDL position adjustment */
>  
> +	unsigned int dma_stop_delay;
> +
>  	/* locks */
>  	spinlock_t reg_lock;
>  	struct mutex cmd_mutex;
> diff --git a/sound/hda/hdac_stream.c b/sound/hda/hdac_stream.c
> index a38a2af1654f..abe7a1b16fe1 100644
> --- a/sound/hda/hdac_stream.c
> +++ b/sound/hda/hdac_stream.c
> @@ -150,9 +150,12 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
>  {
>  	unsigned char val;
>  	int timeout;
> +	int dma_run_state;
>  
>  	snd_hdac_stream_clear(azx_dev);
>  
> +	dma_run_state = snd_hdac_stream_readb(azx_dev, SD_CTL) & SD_CTL_DMA_START;
> +
>  	snd_hdac_stream_updateb(azx_dev, SD_CTL, 0, SD_CTL_STREAM_RESET);
>  	udelay(3);
>  	timeout = 300;
> @@ -162,6 +165,10 @@ void snd_hdac_stream_reset(struct hdac_stream *azx_dev)
>  		if (val)
>  			break;
>  	} while (--timeout);
> +
> +	if (azx_dev->bus->dma_stop_delay && dma_run_state)
> +		udelay(azx_dev->bus->dma_stop_delay);
> +
>  	val &= ~SD_CTL_STREAM_RESET;
>  	snd_hdac_stream_writeb(azx_dev, SD_CTL, val);
>  	udelay(3);
> -- 
> 2.17.1
> 

[PATCH 3/3] ALSA: hda/tegra: Add 100us dma stop delay

[Mohan Kumar]

Tegra HDA has audio data buffer for upto tens of frames, this buffer
can help to avoid underflow. HW will keep issuing new data fetch
request when buffers are not full and current BDL is not done. When SW
disable DMA RUN bit for a stream, HW can't cancel the already issued data
fetch request and hence it can't stop DMA. HW has to wait for all issued
data fetch request get data returned before it stops DMA.

This HW behavior is not in sync with HDA spec which says DMA RUN bit
should be cleared within 1 audio frame. For Tegra, DMA RUN bit was
active for more than one audio frame, due to this the timeout in
snd_hdac_stream_sync function is not helping. When Stream reset set
and clear happens during DMA RUN bit active state it results in Memory
Decode error.

Unfortunately, there is no way to detect when these data accesses have
completed, but testing has shown that a 100us delay between Stream reset
set and clear operation for Tegra avoids the memory decode error.
Therefore, adding a 100us dma stop delay.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index ecf98eb9df36..c94553bcca88 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -308,6 +308,7 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
 		return err;
 	}
 	bus->irq = irq_id;
+	bus->dma_stop_delay = 100;
 	card->sync_irq = bus->irq;
 
 	/*
-- 
2.17.1

[PATCH 3/3] ALSA: hda/tegra: Add 100us dma stop delay

[Mohan Kumar]

Tegra HDA has audio data buffer for upto tens of frames, this buffer
can help to avoid underflow. HW will keep issuing new data fetch
request when buffers are not full and current BDL is not done. When SW
disable DMA RUN bit for a stream, HW can't cancel the already issued data
fetch request and hence it can't stop DMA. HW has to wait for all issued
data fetch request get data returned before it stops DMA.

This HW behavior is not in sync with HDA spec which says DMA RUN bit
should be cleared within 1 audio frame. For Tegra, DMA RUN bit was
active for more than one audio frame, due to this the timeout in
snd_hdac_stream_sync function is not helping. When Stream reset set
and clear happens during DMA RUN bit active state it results in Memory
Decode error.

Unfortunately, there is no way to detect when these data accesses have
completed, but testing has shown that a 100us delay between Stream reset
set and clear operation for Tegra avoids the memory decode error.
Therefore, adding a 100us dma stop delay.

Signed-off-by: Mohan Kumar <mkumard@nvidia.com>
---
 sound/pci/hda/hda_tegra.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index ecf98eb9df36..c94553bcca88 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -308,6 +308,7 @@ static int hda_tegra_first_init(struct azx *chip, struct platform_device *pdev)
 		return err;
 	}
 	bus->irq = irq_id;
+	bus->dma_stop_delay = 100;
 	card->sync_irq = bus->irq;
 
 	/*
-- 
2.17.1