* [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger @ 2017-07-31 10:47 Cheng-Yi Chiang 2017-08-01 3:30 ` Vinod Koul 0 siblings, 1 reply; 6+ messages in thread From: Cheng-Yi Chiang @ 2017-07-31 10:47 UTC (permalink / raw) To: linux-kernel Cc: Vinod Koul, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, U . Artie Eoff, Cheng-Yi Chiang, alsa-devel From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> Reset the hw_ptr before queuing the restore_stream_context work to eradicate a nasty white audio noise on resume. Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> --- sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c b/sound/soc/intel/baytrail/sst-baytrail-pcm.c index 4765ad474544..e0db7070cd42 100644 --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct snd_pcm_substream *substream, int cmd) sst_byt_stream_start(byt, pcm_data->stream, 0); break; case SNDRV_PCM_TRIGGER_RESUME: - if (pdata->restore_stream == true) + if (pdata->restore_stream == true) { + pcm_data->hw_ptr = 0; schedule_work(&pcm_data->work); + } else sst_byt_stream_resume(byt, pcm_data->stream); break; -- 2.12.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger 2017-07-31 10:47 [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger Cheng-Yi Chiang @ 2017-08-01 3:30 ` Vinod Koul 2017-08-01 4:32 ` Eoff, Ullysses A 2017-08-01 5:04 ` [alsa-devel] " Keyon Jie 0 siblings, 2 replies; 6+ messages in thread From: Vinod Koul @ 2017-08-01 3:30 UTC (permalink / raw) To: Cheng-Yi Chiang, Jie Yang Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, U . Artie Eoff, alsa-devel On Mon, Jul 31, 2017 at 06:47:34PM +0800, Cheng-Yi Chiang wrote: > From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> > > Reset the hw_ptr before queuing the restore_stream_context > work to eradicate a nasty white audio noise on resume. Liam, Jie? This on legacy BYT driver.. > > Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> > --- > sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c b/sound/soc/intel/baytrail/sst-baytrail-pcm.c > index 4765ad474544..e0db7070cd42 100644 > --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c > +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c > @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct snd_pcm_substream *substream, int cmd) > sst_byt_stream_start(byt, pcm_data->stream, 0); > break; > case SNDRV_PCM_TRIGGER_RESUME: > - if (pdata->restore_stream == true) > + if (pdata->restore_stream == true) { > + pcm_data->hw_ptr = 0; > schedule_work(&pcm_data->work); > + } > else > sst_byt_stream_resume(byt, pcm_data->stream); > break; > -- > 2.12.2 > -- ~Vinod ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger 2017-08-01 3:30 ` Vinod Koul @ 2017-08-01 4:32 ` Eoff, Ullysses A 2017-08-01 5:04 ` [alsa-devel] " Keyon Jie 1 sibling, 0 replies; 6+ messages in thread From: Eoff, Ullysses A @ 2017-08-01 4:32 UTC (permalink / raw) To: Koul, Vinod, Cheng-Yi Chiang, Jie, Yang Cc: linux-kernel@vger.kernel.org, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, alsa-devel@alsa-project.org This patch was originally submitted in the context of the ChromiumOS kernel 3.10 for BYT (https://groups.google.com/a/chromium.org/forum/#!topic/chromium-os-reviews/AsoBhfHzQg8). > -----Original Message----- > From: Koul, Vinod > Sent: Monday, July 31, 2017 8:30 PM > To: Cheng-Yi Chiang <cychiang@chromium.org>; Jie, Yang <yang.jie@intel.com> > Cc: linux-kernel@vger.kernel.org; Liam Girdwood <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Jaroslav Kysela > <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Eoff, Ullysses A <ullysses.a.eoff@intel.com>; alsa-devel@alsa-project.org > Subject: Re: [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger > > On Mon, Jul 31, 2017 at 06:47:34PM +0800, Cheng-Yi Chiang wrote: > > From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> > > > > Reset the hw_ptr before queuing the restore_stream_context > > work to eradicate a nasty white audio noise on resume. > > Liam, Jie? This on legacy BYT driver.. > > > > > Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> > > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> > > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> > > --- > > sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c b/sound/soc/intel/baytrail/sst-baytrail-pcm.c > > index 4765ad474544..e0db7070cd42 100644 > > --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c > > +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c > > @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct snd_pcm_substream *substream, int cmd) > > sst_byt_stream_start(byt, pcm_data->stream, 0); > > break; > > case SNDRV_PCM_TRIGGER_RESUME: > > - if (pdata->restore_stream == true) > > + if (pdata->restore_stream == true) { > > + pcm_data->hw_ptr = 0; > > schedule_work(&pcm_data->work); > > + } > > else > > sst_byt_stream_resume(byt, pcm_data->stream); > > break; > > -- > > 2.12.2 > > > > -- > ~Vinod ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [alsa-devel] [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger 2017-08-01 3:30 ` Vinod Koul 2017-08-01 4:32 ` Eoff, Ullysses A @ 2017-08-01 5:04 ` Keyon Jie 2017-08-03 13:47 ` Cheng-yi Chiang 1 sibling, 1 reply; 6+ messages in thread From: Keyon Jie @ 2017-08-01 5:04 UTC (permalink / raw) To: Vinod Koul, Cheng-Yi Chiang, Jie Yang Cc: alsa-devel, Liam Girdwood, linux-kernel, Takashi Iwai, Mark Brown, U . Artie Eoff On 2017年08月01日 11:30, Vinod Koul wrote: > On Mon, Jul 31, 2017 at 06:47:34PM +0800, Cheng-Yi Chiang wrote: >> From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> >> >> Reset the hw_ptr before queuing the restore_stream_context >> work to eradicate a nasty white audio noise on resume. > > Liam, Jie? This on legacy BYT driver.. > >> >> Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> >> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> >> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> >> --- >> sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >> index 4765ad474544..e0db7070cd42 100644 >> --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c >> +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >> @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct snd_pcm_substream *substream, int cmd) >> sst_byt_stream_start(byt, pcm_data->stream, 0); >> break; >> case SNDRV_PCM_TRIGGER_RESUME: >> - if (pdata->restore_stream == true) >> + if (pdata->restore_stream == true) { >> + pcm_data->hw_ptr = 0; Won't this break the hw_ptr and make the resuming won't play from the pausing point of the last suspending? Thanks, ~Keyon >> schedule_work(&pcm_data->work); >> + } >> else >> sst_byt_stream_resume(byt, pcm_data->stream); >> break; >> -- >> 2.12.2 >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [alsa-devel] [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger 2017-08-01 5:04 ` [alsa-devel] " Keyon Jie @ 2017-08-03 13:47 ` Cheng-yi Chiang 2017-08-07 6:36 ` Keyon Jie 0 siblings, 1 reply; 6+ messages in thread From: Cheng-yi Chiang @ 2017-08-03 13:47 UTC (permalink / raw) To: Keyon Jie Cc: Vinod Koul, Jie Yang, alsa-devel, Liam Girdwood, linux-kernel, Takashi Iwai, Mark Brown, U . Artie Eoff, Jarkko Nikula +Jarkko Nikula Hi Keyon, Sorry for the late reply. I spent some time trying to figure out previous suspend resume patch in BYT. From the patch for restore_stream flag: https://patchwork.kernel.org/patch/4706611/, the restore_stream flag means that ADSP was in power off state during system suspend/resume cycle. You are right that the resuming won't play from the pausing point of the last suspending. I am not sure whether it is intended in ADSP coming back from power off case. Jarkko and Ullysses might have better idea on this. In my testing I have found that this patch sometimes causes playback failure upon resume. I have put the logs in https://bugs.chromium.org/p/chromium/issues/detail?id=752107. I think we should not merge this until it is resolved. I have cc'ed Intel folks on the issue. Thanks! On Tue, Aug 1, 2017 at 1:04 PM, Keyon Jie <yang.jie@linux.intel.com> wrote: > > On 2017年08月01日 11:30, Vinod Koul wrote: >> >> On Mon, Jul 31, 2017 at 06:47:34PM +0800, Cheng-Yi Chiang wrote: >>> >>> From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> >>> >>> Reset the hw_ptr before queuing the restore_stream_context >>> work to eradicate a nasty white audio noise on resume. >> >> >> Liam, Jie? This on legacy BYT driver.. >> >>> >>> Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> >>> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> >>> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> >>> --- >>> sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>> b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>> index 4765ad474544..e0db7070cd42 100644 >>> --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>> +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>> @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct >>> snd_pcm_substream *substream, int cmd) >>> sst_byt_stream_start(byt, pcm_data->stream, 0); >>> break; >>> case SNDRV_PCM_TRIGGER_RESUME: >>> - if (pdata->restore_stream == true) >>> + if (pdata->restore_stream == true) { >>> + pcm_data->hw_ptr = 0; > > > Won't this break the hw_ptr and make the resuming won't play from the > pausing point of the last suspending? > > Thanks, > ~Keyon > > >>> schedule_work(&pcm_data->work); >>> + } >>> else >>> sst_byt_stream_resume(byt, pcm_data->stream); >>> break; >>> -- >>> 2.12.2 >>> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [alsa-devel] [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger 2017-08-03 13:47 ` Cheng-yi Chiang @ 2017-08-07 6:36 ` Keyon Jie 0 siblings, 0 replies; 6+ messages in thread From: Keyon Jie @ 2017-08-07 6:36 UTC (permalink / raw) To: Cheng-yi Chiang Cc: alsa-devel, Liam Girdwood, Vinod Koul, linux-kernel, Takashi Iwai, Mark Brown, Jarkko Nikula, U . Artie Eoff On 2017年08月03日 21:47, Cheng-yi Chiang wrote: > +Jarkko Nikula > > Hi Keyon, > Sorry for the late reply. I spent some time trying to figure out > previous suspend resume patch in BYT. > > From the patch for restore_stream flag: > https://patchwork.kernel.org/patch/4706611/, the restore_stream flag > means that ADSP was in power off state during system suspend/resume > cycle. You are right that the resuming won't play from the pausing > point of the last suspending. I am not sure whether it is intended in > ADSP coming back from power off case. Jarkko and Ullysses might have > better idea on this. > > In my testing I have found that this patch sometimes causes playback > failure upon resume. I have put the logs in > https://bugs.chromium.org/p/chromium/issues/detail?id=752107. I think > we should not merge this until it is resolved. I have cc'ed Intel > folks on the issue. Thanks for investigation on it, so looks it's better to root cause and fix this noise issue, other than workaround to reset pcm_data->hw_ptr which may introduce new issues. Thanks, ~Keyon > > Thanks! > > On Tue, Aug 1, 2017 at 1:04 PM, Keyon Jie <yang.jie@linux.intel.com> wrote: >> >> On 2017年08月01日 11:30, Vinod Koul wrote: >>> >>> On Mon, Jul 31, 2017 at 06:47:34PM +0800, Cheng-Yi Chiang wrote: >>>> >>>> From: "U. Artie Eoff" <ullysses.a.eoff@intel.com> >>>> >>>> Reset the hw_ptr before queuing the restore_stream_context >>>> work to eradicate a nasty white audio noise on resume. >>> >>> >>> Liam, Jie? This on legacy BYT driver.. >>> >>>> >>>> Tested-by: Cheng-Yi Chiang <cychiang@chromium.org> >>>> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com> >>>> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org> >>>> --- >>>> sound/soc/intel/baytrail/sst-baytrail-pcm.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>>> b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>>> index 4765ad474544..e0db7070cd42 100644 >>>> --- a/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>>> +++ b/sound/soc/intel/baytrail/sst-baytrail-pcm.c >>>> @@ -187,8 +187,10 @@ static int sst_byt_pcm_trigger(struct >>>> snd_pcm_substream *substream, int cmd) >>>> sst_byt_stream_start(byt, pcm_data->stream, 0); >>>> break; >>>> case SNDRV_PCM_TRIGGER_RESUME: >>>> - if (pdata->restore_stream == true) >>>> + if (pdata->restore_stream == true) { >>>> + pcm_data->hw_ptr = 0; >> >> >> Won't this break the hw_ptr and make the resuming won't play from the >> pausing point of the last suspending? >> >> Thanks, >> ~Keyon >> >> >>>> schedule_work(&pcm_data->work); >>>> + } >>>> else >>>> sst_byt_stream_resume(byt, pcm_data->stream); >>>> break; >>>> -- >>>> 2.12.2 >>>> >>> >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-07 6:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-07-31 10:47 [PATCH v2] ASoC: Intel: Reset hw_ptr on resume trigger Cheng-Yi Chiang 2017-08-01 3:30 ` Vinod Koul 2017-08-01 4:32 ` Eoff, Ullysses A 2017-08-01 5:04 ` [alsa-devel] " Keyon Jie 2017-08-03 13:47 ` Cheng-yi Chiang 2017-08-07 6:36 ` Keyon Jie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox