From: Ajit Kumar Pandey <[email protected]>
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 <[email protected]>
Signed-off-by: Venkata Prasad Potturu <[email protected]>
Reviewed-by: Vijendar Mukunda <[email protected]>
---
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(-)
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;
--
2.25.1
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
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