diff mbox series

[v3] ASoC: amd: acp: Initialize list to store acp_stream during pcm_open

Message ID 20220728124057.54259-1-venkataprasad.potturu@amd.com (mailing list archive)
State Superseded
Headers show
Series [v3] ASoC: amd: acp: Initialize list to store acp_stream during pcm_open | expand

Commit Message

Venkata Prasad Potturu July 28, 2022, 12:40 p.m. UTC 
From: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>

We are currently allocating acp_stream during pcm_open and saving
it in static array corresponds to array index calculated based on
cpu dai->driver id. This approach will fail if we have single dai
linked to multiple pcm device as we will have same dai->driver id
or array index for multiple pcm open. Initialize new linked list
stream_list to store opened pcm stream info dynamically.

Signed-off-by: Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
Signed-off-by: Venkata Prasad Potturu <venkataprasad.potturu@amd.com>
Reviewed-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
Changes since v2:
    -- Add locking mechanism in irq handler, linked list updatation and deletion.
Changes since v1:
    -- Fix compile error and remove unused variable.

 sound/soc/amd/acp/acp-platform.c | 43 ++++++++++++++++----------------
 sound/soc/amd/acp/amd.h          |  4 ++-
 2 files changed, 25 insertions(+), 22 deletions(-)

Comments

Mark Brown July 28, 2022, 12:49 p.m. UTC | #1 
On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote:

> @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>  
>  	ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>  
> -	for (i = 0; i < ACP_MAX_STREAM; i++) {
> -		stream = adata->stream[i];
> +	spin_lock_irqsave(&adata->acp_lock, flags);
> +	list_for_each_entry(stream, &adata->stream_list, list) {

If we're already in an interrupt handler here (presumably not a threaded
one) why are we using irqsave?
Venkata Prasad Potturu July 29, 2022, 10:34 a.m. UTC | #2 
On 7/28/22 18:19, Mark Brown wrote:
Thanks for your time.
>
> On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote:
>
>> @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>>   
>>   	ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>>   
>> -	for (i = 0; i < ACP_MAX_STREAM; i++) {
>> -		stream = adata->stream[i];
>> +	spin_lock_irqsave(&adata->acp_lock, flags);
>> +	list_for_each_entry(stream, &adata->stream_list, list) {
> If we're already in an interrupt handler here (presumably not a threaded
> one) why are we using irqsave?

Yes, your statement make sense, I have followed below statement in 
kernel document. so used irqsave in interrupt context as well.

We will change it to spin_lock() and send it in the next version.

*statement:- *spin_lock_irqsave() will turn off interrupts if they are 
on, otherwise does nothing (if we are already in an interrupt handler), 
hence these functions are safe to call from any context.
Takashi Iwai July 29, 2022, 10:49 a.m. UTC | #3 
On Fri, 29 Jul 2022 12:34:51 +0200,
Venkata Prasad Potturu wrote:
> 
> 
> On 7/28/22 18:19, Mark Brown wrote:
> Thanks for your time.
> 
>     On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote:
> 
>         @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>          
>               ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>          
>         -     for (i = 0; i < ACP_MAX_STREAM; i++) {
>         -                           stream = adata->stream[i];
>         +     spin_lock_irqsave(&adata->acp_lock, flags);
>         +     list_for_each_entry(stream, &adata->stream_list, list) {
>         
>     If we're already in an interrupt handler here (presumably not a threaded
>     one) why are we using irqsave?
>     
> Yes, your statement make sense, I have followed below statement in kernel
> document. so used irqsave in interrupt context as well.
> 
> We will change it to spin_lock() and send it in the next version.
> 
> statement:- spin_lock_irqsave() will turn off interrupts if they are on,
> otherwise does nothing (if we are already in an interrupt handler), hence
> these functions are safe to call from any context.

Also the open and close callbacks are certainly non-irq context, hence
you can use spin_lock_irq() instead of irqsave(), too.


Takashi
Venkata Prasad Potturu July 29, 2022, 11:17 a.m. UTC | #4 
On 7/29/22 16:19, Takashi Iwai wrote:
Thanks for your time.
> [CAUTION: External Email]
>
> On Fri, 29 Jul 2022 12:34:51 +0200,
> Venkata Prasad Potturu wrote:
>>
>> On 7/28/22 18:19, Mark Brown wrote:
>> Thanks for your time.
>>
>>      On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote:
>>
>>          @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>>
>>                ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>>
>>          -     for (i = 0; i < ACP_MAX_STREAM; i++) {
>>          -                           stream = adata->stream[i];
>>          +     spin_lock_irqsave(&adata->acp_lock, flags);
>>          +     list_for_each_entry(stream, &adata->stream_list, list) {
>>
>>      If we're already in an interrupt handler here (presumably not a threaded
>>      one) why are we using irqsave?
>>
>> Yes, your statement make sense, I have followed below statement in kernel
>> document. so used irqsave in interrupt context as well.
>>
>> We will change it to spin_lock() and send it in the next version.
>>
>> statement:- spin_lock_irqsave() will turn off interrupts if they are on,
>> otherwise does nothing (if we are already in an interrupt handler), hence
>> these functions are safe to call from any context.
> Also the open and close callbacks are certainly non-irq context, hence
> you can use spin_lock_irq() instead of irqsave(), too.

Okay. Thanks for your suggestion.

We will use accordingly.

>
>
> Takashi
Venkata Prasad Potturu Aug. 1, 2022, 5:01 a.m. UTC | #5 
On 7/29/2022 4:19 PM, Takashi Iwai wrote:
Thanks for your time.
> [CAUTION: External Email]
>
> On Fri, 29 Jul 2022 12:34:51 +0200,
> Venkata Prasad Potturu wrote:
>> On 7/28/22 18:19, Mark Brown wrote:
>> Thanks for your time.
>>
>>      On Thu, Jul 28, 2022 at 06:10:50PM +0530, Venkata Prasad Potturu wrote:
>>
>>          @@ -104,14 +105,13 @@ static irqreturn_t i2s_irq_handler(int irq, void *data)
>>
>>                ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
>>
>>          -     for (i = 0; i < ACP_MAX_STREAM; i++) {
>>          -                           stream = adata->stream[i];
>>          +     spin_lock_irqsave(&adata->acp_lock, flags);
>>          +     list_for_each_entry(stream, &adata->stream_list, list) {
>>
>>      If we're already in an interrupt handler here (presumably not a threaded
>>      one) why are we using irqsave?
>>
>> Yes, your statement make sense, I have followed below statement in kernel
>> document. so used irqsave in interrupt context as well.
>>
>> We will change it to spin_lock() and send it in the next version.
>>
>> statement:- spin_lock_irqsave() will turn off interrupts if they are on,
>> otherwise does nothing (if we are already in an interrupt handler), hence
>> these functions are safe to call from any context.
> Also the open and close callbacks are certainly non-irq context, hence
> you can use spin_lock_irq() instead of irqsave(), too.

One doubt, if we use *spin_lock_irq()* it will do *local_irq_disable() 
*even if interrupts are already disabled, and when we call 
*spin_unlock_irq() *after critical section it  will forcibly re-enable 
interrupts in a potentially unwanted manner.

If we use *spin_lock_irqsave();* and *spin_unlock_irqrestore(); *will 
save and restore the interrupt state.

My understanding is *spin_lock_irqsave();* is better instead of improper 
interrupt enable and disable as like *spin_lock_irq();*

Could you please let me know is there any other thing to use 
*spin_lock_irq(); *particularly here.

> Takashi
diff mbox series

Patch

diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c
index f561d39b33e2..4d35c75611d3 100644
--- a/sound/soc/amd/acp/acp-platform.c
+++ b/sound/soc/amd/acp/acp-platform.c
@@ -93,8 +93,9 @@  static irqreturn_t i2s_irq_handler(int irq, void *data)
 	struct acp_dev_data *adata = data;
 	struct acp_resource *rsrc = adata->rsrc;
 	struct acp_stream *stream;
+	unsigned long flags;
 	u16 i2s_flag = 0;
-	u32 ext_intr_stat, ext_intr_stat1, i;
+	u32 ext_intr_stat, ext_intr_stat1;
 
 	if (!adata)
 		return IRQ_NONE;
@@ -104,14 +105,13 @@  static irqreturn_t i2s_irq_handler(int irq, void *data)
 
 	ext_intr_stat = readl(ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
 
-	for (i = 0; i < ACP_MAX_STREAM; i++) {
-		stream = adata->stream[i];
+	spin_lock_irqsave(&adata->acp_lock, flags);
+	list_for_each_entry(stream, &adata->stream_list, list) {
 		if (stream && (ext_intr_stat & stream->irq_bit)) {
 			writel(stream->irq_bit,
 			       ACP_EXTERNAL_INTR_STAT(adata, rsrc->irqp_used));
 			snd_pcm_period_elapsed(stream->substream);
 			i2s_flag = 1;
-			break;
 		}
 		if (adata->rsrc->no_of_ctrls == 2) {
 			if (stream && (ext_intr_stat1 & stream->irq_bit)) {
@@ -119,10 +119,10 @@  static irqreturn_t i2s_irq_handler(int irq, void *data)
 				       (rsrc->irqp_used - 1)));
 				snd_pcm_period_elapsed(stream->substream);
 				i2s_flag = 1;
-				break;
 			}
 		}
 	}
+	spin_unlock_irqrestore(&adata->acp_lock, flags);
 	if (i2s_flag)
 		return IRQ_HANDLED;
 
@@ -146,9 +146,8 @@  static void config_pte_for_stream(struct acp_dev_data *adata, struct acp_stream
 	writel(0x01, adata->acp_base + ACPAXI2AXI_ATU_CTRL);
 }
 
-static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
+static void config_acp_dma(struct acp_dev_data *adata, struct acp_stream *stream, int size)
 {
-	struct acp_stream *stream = adata->stream[cpu_id];
 	struct snd_pcm_substream *substream = stream->substream;
 	struct acp_resource *rsrc = adata->rsrc;
 	dma_addr_t addr = substream->dma_buffer.addr;
@@ -174,21 +173,22 @@  static void config_acp_dma(struct acp_dev_data *adata, int cpu_id, int size)
 
 static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_substream *substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct device *dev = component->dev;
 	struct acp_dev_data *adata = dev_get_drvdata(dev);
 	struct acp_stream *stream;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
 	int ret;
+	unsigned long flags;
 
 	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
 	if (!stream)
 		return -ENOMEM;
 
 	stream->substream = substream;
-	adata->stream[stream_id] = stream;
+
+	spin_lock_irqsave(&adata->acp_lock, flags);
+	list_add_tail(&stream->list, &adata->stream_list);
+	spin_unlock_irqrestore(&adata->acp_lock, flags);
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 		runtime->hw = acp_pcm_hardware_playback;
@@ -212,16 +212,13 @@  static int acp_dma_hw_params(struct snd_soc_component *component,
 			     struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *params)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
 	struct acp_dev_data *adata = snd_soc_component_get_drvdata(component);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
 	struct acp_stream *stream = substream->runtime->private_data;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
 	u64 size = params_buffer_bytes(params);
 
 	/* Configure ACP DMA block with params */
 	config_pte_for_stream(adata, stream);
-	config_acp_dma(adata, stream_id, size);
+	config_acp_dma(adata, stream, size);
 
 	return 0;
 }
@@ -261,16 +258,16 @@  static int acp_dma_new(struct snd_soc_component *component,
 static int acp_dma_close(struct snd_soc_component *component,
 			 struct snd_pcm_substream *substream)
 {
-	struct snd_soc_pcm_runtime *soc_runtime = asoc_substream_to_rtd(substream);
-	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(soc_runtime, 0);
 	struct device *dev = component->dev;
 	struct acp_dev_data *adata = dev_get_drvdata(dev);
-	struct acp_stream *stream;
-	int stream_id = cpu_dai->driver->id * 2 + substream->stream;
+	struct acp_stream *stream = substream->runtime->private_data;
+	unsigned long flags;
 
-	stream = adata->stream[stream_id];
+	/* Remove entry from list */
+	spin_lock_irqsave(&adata->acp_lock, flags);
+	list_del(&stream->list);
+	spin_unlock_irqrestore(&adata->acp_lock, flags);
 	kfree(stream);
-	adata->stream[stream_id] = NULL;
 
 	return 0;
 }
@@ -305,6 +302,10 @@  int acp_platform_register(struct device *dev)
 		dev_err(dev, "Fail to register acp i2s component\n");
 		return status;
 	}
+
+	INIT_LIST_HEAD(&adata->stream_list);
+	spin_lock_init(&adata->acp_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(acp_platform_register, SND_SOC_ACP_COMMON);
diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
index 60a7c95f134f..be8bb8247c4e 100644
--- a/sound/soc/amd/acp/amd.h
+++ b/sound/soc/amd/acp/amd.h
@@ -91,6 +91,7 @@  struct acp_chip_info {
 };
 
 struct acp_stream {
+	struct list_head list;
 	struct snd_pcm_substream *substream;
 	int irq_bit;
 	int dai_id;
@@ -123,7 +124,8 @@  struct acp_dev_data {
 	struct snd_soc_dai_driver *dai_driver;
 	int num_dai;
 
-	struct acp_stream *stream[ACP_MAX_STREAM];
+	struct list_head stream_list;
+	spinlock_t acp_lock;
 
 	struct snd_soc_acpi_mach *machines;
 	struct platform_device *mach_dev;