On Fri, 08 Apr 2016 01:07:12 +0200, Vinod Koul wrote:
On Thu, 2016-04-07 at 09:28 +0100, Charles Keepax wrote:
On Thu, Apr 07, 2016 at 12:40:03AM +0000, Koul, Vinod wrote:
On Wed, 2016-04-06 at 11:21 +0100, Charles Keepax wrote:
+int snd_compr_stop_xrun(struct snd_compr_stream *stream) +{
- if (stream->runtime->state == SNDRV_PCM_STATE_XRUN)
return 0;- stream->runtime->state = SNDRV_PCM_STATE_XRUN;
- queue_delayed_work(system_power_efficient_wq, &stream
->xrun_work, 0);
why do we want to do this in workqueue and not stop the compress stream immediately.
We need to defer this to a work queue because it is very likely we will detect the errors whilst already in a callback in the driver. For example we notice the stream is bad whilst processing a read or a pointer callback in the driver. Because this call by definition goes right back to the top of the stack rather than unwinding the stack nicely as returning an error would do, we need to be careful of all the locks that are likely to be held in between.
snd_compr_ioctl - takes stream->device->lock snd_compr_tstamp soc_compr_pointer - takes rtd->pcm_mutex wm_adsp_compr_pointer - takes dsp->pwr_lock snd_compr_stop_xrun snd_compr_stop soc_compr_trigger - Deadlock as we take rtd->pcm_mutex again
This is what I suspected as well :D so this should be fine. I will take a detailed look at the changes once am back home.
Also should this be made a generic stop rather than xrun. Perhaps the reason can be specified as an argument.
Btw Takashi you okay with this approach?
Yes, it looks good to me.
Also if we do this, then why should pointer return error?
The first patch in the chain could indeed be changed to have pointer calls not return an error status. But I feel that would be making the code worse. Ok the situation I am most interested here indicates a failure of the stream, but its a very small leap to imagine situations where pointer fails temporarily and the stream is still good.
The point here is that we are anyway propagating error by invoking the new API so why return error here. Btw can you please explain how this makes code worse?
I think on PCM we don't do that.
PCM stops the stream from the lock context, so there is no leap.
thanks,
Takashi