Re: [PATCH 2/4] ASoC: meson: aiu-encoder-i2s: use gx_iface and gx_stream structures

[Valerio Setti <vsetti@baylibre.com>]

>>   static void aiu_encoder_i2s_shutdown(struct snd_pcm_substream *substream,
>>   				     struct snd_soc_dai *dai)
>>   {
>>   	struct aiu *aiu = snd_soc_component_get_drvdata(dai->component);
>> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
>> +	struct gx_iface *iface = ts->iface;
>> +
>> +	if (!snd_soc_dai_active(dai))
>> +		iface->rate = 0;
> 
> The ASoC core invokes the shutdown callback before decrementing the stream's
> active count.
> 
> Because of this, does snd_soc_dai_active(dai) always evaluate to at least 1
> during the shutdown of the final stream?
> 
> If so, iface->rate will never be reset to 0, which would cause subsequent
> streams with different sample rates to permanently fail with -EINVAL until
> the driver is reloaded.
> 

I might be missing something also here, but I don't think this is 
correct. It seems to me that when 'aiu_encoder_i2s_shutdown' is called 
the number of active streams have already been decreased so everything 
works as expected.
In support of my hypothesis I have tried this patch series together with 
the upcoming one adding also capture stream and I didn't experience 
problems, so I think that 'iface->date' is correctly reset when expected.

>> +static int aiu_encoder_i2s_trigger(struct snd_pcm_substream *substream,
>> +				   int cmd,
>> +				   struct snd_soc_dai *dai)
>> +{
>> +	struct gx_stream *ts = snd_soc_dai_get_dma_data(dai, substream);
>> +	int ret;
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +		ret = gx_stream_start(ts);
>> +		break;
>> +	case SNDRV_PCM_TRIGGER_SUSPEND:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> +	case SNDRV_PCM_TRIGGER_STOP:
>> +		gx_stream_stop(ts);
>> +		ret = 0;
>> +		break;
> 
> The PCM trigger callback typically executes in an atomic context with the
> PCM stream spinlock held and hardware interrupts disabled, unless the DAI
> is explicitly marked with nonatomic = 1.
> 
> Looking at gx_stream_start() and gx_stream_stop(), they both unconditionally
> acquire a sleeping lock via mutex_lock(&ts->lock).
> 
> Does acquiring a mutex in this atomic context trigger a scheduling while
> atomic kernel panic?
> 

This is one of the parts that has been copied from AXG. I would say that 
the problem highlighted here would only appear in case two formatters 
are started/stopped on same stream concurrently. If that's the case then 
this is not a problem for GX because we're going to have 1 single 
formatter per stream.

-- 
Valerio


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic