[alsa-devel] [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream.

[Takashi Iwai]

At Mon, 16 Sep 2013 12:01:23 +0100,
Liam Girdwood wrote:
> 
> On Mon, 2013-09-16 at 09:27 +0530, Vinod Koul wrote:
> > On Fri, Sep 13, 2013 at 05:43:16PM +0100, Liam Girdwood wrote:
> > > Currently we assume that userspace will shut down the compressed stream
> > > correctly. However, if userspcae dies (e.g. cplay & ctrl-C) we dont
> > > stop the stream before freeing it.
> > > 
> > > This now checks that the stream is stopped before freeing.
> > > 
> > > Signed-off-by: Liam Girdwood <liam.r.girdwood at linux.intel.com>
> > > ---
> > >  sound/core/compress_offload.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > > index 99db892..a9ae4f3 100644
> > > --- a/sound/core/compress_offload.c
> > > +++ b/sound/core/compress_offload.c
> > > @@ -139,6 +139,18 @@ static int snd_compr_open(struct inode *inode, struct file *f)
> > >  static int snd_compr_free(struct inode *inode, struct file *f)
> > >  {
> > >  	struct snd_compr_file *data = f->private_data;
> > > +	struct snd_compr_runtime *runtime = data->stream.runtime;
> > > +
> > > +	switch (runtime->state) {
> > > +	case SNDRV_PCM_STATE_RUNNING:
> > > +	case SNDRV_PCM_STATE_DRAINING:
> > > +	case SNDRV_PCM_STATE_PAUSED:
> > > +		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
> > when usermode dies the free will get invoked as we have below. The free is a
> > valid trigger from any state. So shouldnt the DSP then STOP the DMAs and free
> > the streams.
> 
> This means that the compressed stream ops don't follow the PCM ops wrt
> trigger() sequencing (i.e. we have to add extra logic in free to deal
> with this difference).
> 
> I think it would be better to follow the PCM operations use case here as
> it simplifies the driver code and stops people making the false
> assumption that trigger() works the same way throughout ALSA ?   

Well, the trigger of compress API has a few more jobs than PCM,
and it's exactly what should be fixed (e.g. the proper handling of
drain).  We need a bit more consensus over that.

So, I didn't apply this patch yet.  If Vinod is happy with this (even
as a temporary solution), let me know.


thanks,

Takashi

> > Given that most of the usage is a DSP then IMO it might make send to tell DSP
> > that stop and cleanup rather than saying stop and cleanup?
> > 
> 
> Sorry, don't follow you here ;)
> 
> Liam
> 
> > ~Vinod
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > >  	data->stream.ops->free(&data->stream);
> > >  	kfree(data->stream.runtime->buffer);
> > >  	kfree(data->stream.runtime);
> > > -- 
> > > 1.8.1.2
> > > 
> > 
> 
> 
> 
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.