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

[Liam Girdwood <liam.r.girdwood@intel.com>]

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@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 ?   

> 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.