* [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. @ 2013-09-13 16:43 Liam Girdwood 2013-09-13 16:43 ` [PATCH] ALSA: compress: Fix compress device unregister Liam Girdwood 2013-09-16 3:57 ` [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Vinod Koul 0 siblings, 2 replies; 11+ messages in thread From: Liam Girdwood @ 2013-09-13 16:43 UTC (permalink / raw) To: alsa-devel; +Cc: Vinod Koul, Takashi Iwai, Mark Brown, Liam Girdwood 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); + break; + default: + break; + } + data->stream.ops->free(&data->stream); kfree(data->stream.runtime->buffer); kfree(data->stream.runtime); -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] ALSA: compress: Fix compress device unregister. 2013-09-13 16:43 [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Liam Girdwood @ 2013-09-13 16:43 ` Liam Girdwood 2013-09-16 4:15 ` Vinod Koul 2013-09-16 3:57 ` [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Vinod Koul 1 sibling, 1 reply; 11+ messages in thread From: Liam Girdwood @ 2013-09-13 16:43 UTC (permalink / raw) To: alsa-devel; +Cc: Vinod Koul, Takashi Iwai, Mark Brown, Liam Girdwood snd_unregister_device() should return the device type and not stream direction. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> --- sound/core/compress_offload.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c index a9ae4f3..fba7e7a 100644 --- a/sound/core/compress_offload.c +++ b/sound/core/compress_offload.c @@ -849,7 +849,8 @@ static int snd_compress_dev_disconnect(struct snd_device *device) struct snd_compr *compr; compr = device->device_data; - snd_unregister_device(compr->direction, compr->card, compr->device); + snd_unregister_device(SNDRV_DEVICE_TYPE_COMPRESS, compr->card, + compr->device); return 0; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Fix compress device unregister. 2013-09-13 16:43 ` [PATCH] ALSA: compress: Fix compress device unregister Liam Girdwood @ 2013-09-16 4:15 ` Vinod Koul 2013-09-16 4:22 ` Vinod Koul 0 siblings, 1 reply; 11+ messages in thread From: Vinod Koul @ 2013-09-16 4:15 UTC (permalink / raw) To: Liam Girdwood; +Cc: Takashi Iwai, alsa-devel, Mark Brown On Fri, Sep 13, 2013 at 05:43:17PM +0100, Liam Girdwood wrote: > snd_unregister_device() should return the device type and not stream > direction. Thanks for fixing this, I saw this but didnt get time to fix it. Acked-By: Vinod Koul <vinod.koul@intel.com> Tested-By: Vinod Koul <vinod.koul@intel.com> Is there a Tested-Acked-?? ~Vinod > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > --- > sound/core/compress_offload.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > index a9ae4f3..fba7e7a 100644 > --- a/sound/core/compress_offload.c > +++ b/sound/core/compress_offload.c > @@ -849,7 +849,8 @@ static int snd_compress_dev_disconnect(struct snd_device *device) > struct snd_compr *compr; > > compr = device->device_data; > - snd_unregister_device(compr->direction, compr->card, compr->device); > + snd_unregister_device(SNDRV_DEVICE_TYPE_COMPRESS, compr->card, > + compr->device); > return 0; > } > > -- > 1.8.1.2 > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Fix compress device unregister. 2013-09-16 4:15 ` Vinod Koul @ 2013-09-16 4:22 ` Vinod Koul 2013-09-19 16:41 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Vinod Koul @ 2013-09-16 4:22 UTC (permalink / raw) To: Liam Girdwood; +Cc: Takashi Iwai, alsa-devel, Mark Brown On Mon, Sep 16, 2013 at 09:45:01AM +0530, Vinod Koul wrote: > On Fri, Sep 13, 2013 at 05:43:17PM +0100, Liam Girdwood wrote: > > snd_unregister_device() should return the device type and not stream > > direction. > Thanks for fixing this, I saw this but didnt get time to fix it. > > Acked-By: Vinod Koul <vinod.koul@intel.com> > Tested-By: Vinod Koul <vinod.koul@intel.com> Also pls CC stable, this fixed would help in older kernels too ~Vinod > > Is there a Tested-Acked-?? > > ~Vinod > > > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > > --- > > sound/core/compress_offload.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > > index a9ae4f3..fba7e7a 100644 > > --- a/sound/core/compress_offload.c > > +++ b/sound/core/compress_offload.c > > @@ -849,7 +849,8 @@ static int snd_compress_dev_disconnect(struct snd_device *device) > > struct snd_compr *compr; > > > > compr = device->device_data; > > - snd_unregister_device(compr->direction, compr->card, compr->device); > > + snd_unregister_device(SNDRV_DEVICE_TYPE_COMPRESS, compr->card, > > + compr->device); > > return 0; > > } > > > > -- > > 1.8.1.2 > > > > -- -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Fix compress device unregister. 2013-09-16 4:22 ` Vinod Koul @ 2013-09-19 16:41 ` Takashi Iwai 0 siblings, 0 replies; 11+ messages in thread From: Takashi Iwai @ 2013-09-19 16:41 UTC (permalink / raw) To: Vinod Koul; +Cc: alsa-devel, Liam Girdwood, Mark Brown At Mon, 16 Sep 2013 09:52:11 +0530, Vinod Koul wrote: > > On Mon, Sep 16, 2013 at 09:45:01AM +0530, Vinod Koul wrote: > > On Fri, Sep 13, 2013 at 05:43:17PM +0100, Liam Girdwood wrote: > > > snd_unregister_device() should return the device type and not stream > > > direction. > > Thanks for fixing this, I saw this but didnt get time to fix it. > > > > Acked-By: Vinod Koul <vinod.koul@intel.com> > > Tested-By: Vinod Koul <vinod.koul@intel.com> > > Also pls CC stable, this fixed would help in older kernels too I applied this now with these tags, but a pull request to Linus will be postponed until the next week since I've been traveling now. thanks, Takashi > > ~Vinod > > > > Is there a Tested-Acked-?? > > > > ~Vinod > > > > > > Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com> > > > --- > > > sound/core/compress_offload.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c > > > index a9ae4f3..fba7e7a 100644 > > > --- a/sound/core/compress_offload.c > > > +++ b/sound/core/compress_offload.c > > > @@ -849,7 +849,8 @@ static int snd_compress_dev_disconnect(struct snd_device *device) > > > struct snd_compr *compr; > > > > > > compr = device->device_data; > > > - snd_unregister_device(compr->direction, compr->card, compr->device); > > > + snd_unregister_device(SNDRV_DEVICE_TYPE_COMPRESS, compr->card, > > > + compr->device); > > > return 0; > > > } > > > > > > -- > > > 1.8.1.2 > > > > > > > -- > > -- > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-13 16:43 [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Liam Girdwood 2013-09-13 16:43 ` [PATCH] ALSA: compress: Fix compress device unregister Liam Girdwood @ 2013-09-16 3:57 ` Vinod Koul 2013-09-16 11:01 ` Liam Girdwood 1 sibling, 1 reply; 11+ messages in thread From: Vinod Koul @ 2013-09-16 3:57 UTC (permalink / raw) To: Liam Girdwood; +Cc: Takashi Iwai, alsa-devel, Mark Brown 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. 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? ~Vinod > + break; > + default: > + break; > + } > + > data->stream.ops->free(&data->stream); > kfree(data->stream.runtime->buffer); > kfree(data->stream.runtime); > -- > 1.8.1.2 > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-16 3:57 ` [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Vinod Koul @ 2013-09-16 11:01 ` Liam Girdwood 2013-09-19 16:43 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Liam Girdwood @ 2013-09-16 11:01 UTC (permalink / raw) To: Vinod Koul; +Cc: Takashi Iwai, alsa-devel, Liam Girdwood, Mark Brown 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-16 11:01 ` Liam Girdwood @ 2013-09-19 16:43 ` Takashi Iwai 2013-09-23 10:19 ` Liam Girdwood 0 siblings, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2013-09-19 16:43 UTC (permalink / raw) To: Liam Girdwood; +Cc: Vinod Koul, alsa-devel, Liam Girdwood, Mark Brown 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@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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-19 16:43 ` Takashi Iwai @ 2013-09-23 10:19 ` Liam Girdwood 2013-09-23 9:41 ` Vinod Koul 0 siblings, 1 reply; 11+ messages in thread From: Liam Girdwood @ 2013-09-23 10:19 UTC (permalink / raw) To: Takashi Iwai, Vinod Koul; +Cc: alsa-devel, Liam Girdwood, Mark Brown On Thu, 2013-09-19 at 18:43 +0200, Takashi Iwai wrote: > 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@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. Without this patch we have the two different sequences for shutdown depending on whether userspace dies or not. i.e. 1) Normal: trigger(start) -> play -> trigger(stop) -> close() 2) ctrl-C: trigger(start) -> play -> ctrl-C -> close() So we miss out the trigger(STOP) and close the device with the stream still in a trigger running state. Liam ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-23 10:19 ` Liam Girdwood @ 2013-09-23 9:41 ` Vinod Koul 2013-09-26 7:58 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Vinod Koul @ 2013-09-23 9:41 UTC (permalink / raw) Cc: Takashi Iwai, alsa-devel, Liam Girdwood, Mark Brown On Mon, Sep 23, 2013 at 11:19:39AM +0100, Liam Girdwood wrote: > On Thu, 2013-09-19 at 18:43 +0200, Takashi Iwai wrote: > > 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@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. Drain handling would be different, I am working on taht way, havent been able to get the time it needs... > > So, I didn't apply this patch yet. If Vinod is happy with this (even > > as a temporary solution), let me know. Sorry my bad, your and Liam mail went to some other folder due to my bad rules set. Sorry for not reverting earlier.. > > Without this patch we have the two different sequences for shutdown > depending on whether userspace dies or not. i.e. > > 1) Normal: trigger(start) -> play -> trigger(stop) -> close() > > 2) ctrl-C: trigger(start) -> play -> ctrl-C -> close() > > So we miss out the trigger(STOP) and close the device with the stream > still in a trigger running state. my point was since we have DSPs we can let DSP internally treat free in not stopped cases as goto stop and then free. We do this way in our DSPs and save us a cmd to be sent. Above way though fine will just add that cmd. The patch is okay but only argument i had was that we can save one cmd since we have a DSP. Am okay if we have this too.. ~Vinod -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream. 2013-09-23 9:41 ` Vinod Koul @ 2013-09-26 7:58 ` Takashi Iwai 0 siblings, 0 replies; 11+ messages in thread From: Takashi Iwai @ 2013-09-26 7:58 UTC (permalink / raw) To: Vinod Koul; +Cc: alsa-devel, Liam Girdwood, Mark Brown At Mon, 23 Sep 2013 15:11:00 +0530, Vinod Koul wrote: > > On Mon, Sep 23, 2013 at 11:19:39AM +0100, Liam Girdwood wrote: > > On Thu, 2013-09-19 at 18:43 +0200, Takashi Iwai wrote: > > > 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@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. > Drain handling would be different, I am working on taht way, havent been able to > get the time it needs... > > > > So, I didn't apply this patch yet. If Vinod is happy with this (even > > > as a temporary solution), let me know. > Sorry my bad, your and Liam mail went to some other folder due to my bad rules > set. Sorry for not reverting earlier.. > > > > Without this patch we have the two different sequences for shutdown > > depending on whether userspace dies or not. i.e. > > > > 1) Normal: trigger(start) -> play -> trigger(stop) -> close() > > > > 2) ctrl-C: trigger(start) -> play -> ctrl-C -> close() > > > > So we miss out the trigger(STOP) and close the device with the stream > > still in a trigger running state. > my point was since we have DSPs we can let DSP internally treat free in not > stopped cases as goto stop and then free. We do this way in our DSPs and save us > a cmd to be sent. Above way though fine will just add that cmd. > > The patch is okay but only argument i had was that we can save one cmd since we > have a DSP. Am okay if we have this too.. OK, applied this patch now, too. thanks, Takashi ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-09-26 7:56 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-13 16:43 [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Liam Girdwood 2013-09-13 16:43 ` [PATCH] ALSA: compress: Fix compress device unregister Liam Girdwood 2013-09-16 4:15 ` Vinod Koul 2013-09-16 4:22 ` Vinod Koul 2013-09-19 16:41 ` Takashi Iwai 2013-09-16 3:57 ` [PATCH] ALSA: compress: Make sure we trigger STOP before closing the stream Vinod Koul 2013-09-16 11:01 ` Liam Girdwood 2013-09-19 16:43 ` Takashi Iwai 2013-09-23 10:19 ` Liam Girdwood 2013-09-23 9:41 ` Vinod Koul 2013-09-26 7:58 ` Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).