[1/7] ASoC: Intel: Skylake: Add Resume capability in PCM info.

Commit Message

Vinod Koul Nov. 23, 2015, 4:56 p.m. UTC

From: Jeeja KP <[e-mail]>

This patch adds pcm capability to support Resume.

Signed-off-by: Jeeja KP <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
---
 sound/soc/intel/skylake/skl-pcm.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Takashi Iwai Nov. 23, 2015, 6:45 p.m. UTC | #1

On Mon, 23 Nov 2015 17:56:23 +0100,
Vinod Koul wrote:
> 
> From: Jeeja KP <[e-mail]>
> 
> This patch adds pcm capability to support Resume.

The flag indicates that the driver is capable of the full resume --
i.e. it must resume everything at the exactly same point to be
suspended.  Is it really the case?

If it's not and it's instead a kind of restart of streams, don't add
this flag.  The lack of the flag doesn't mean that the driver can't
resume at all.


Takashi

> 
> Signed-off-by: Jeeja KP <[e-mail]>
> Signed-off-by: Vinod Koul <[e-mail]>
> ---
>  sound/soc/intel/skylake/skl-pcm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
> index dae332beea3f..4378bf452582 100644
> --- a/sound/soc/intel/skylake/skl-pcm.c
> +++ b/sound/soc/intel/skylake/skl-pcm.c
> @@ -35,6 +35,7 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>  				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
>  				 SNDRV_PCM_INFO_MMAP_VALID |
>  				 SNDRV_PCM_INFO_PAUSE |
> +				 SNDRV_PCM_INFO_RESUME |
>  				 SNDRV_PCM_INFO_SYNC_START |
>  				 SNDRV_PCM_INFO_HAS_WALL_CLOCK | /* legacy */
>  				 SNDRV_PCM_INFO_HAS_LINK_ATIME |
> -- 
> 1.9.1
> 
> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Vinod Koul Nov. 24, 2015, 2:44 a.m. UTC | #2

On Mon, Nov 23, 2015 at 07:45:20PM +0100, Takashi Iwai wrote:
> On Mon, 23 Nov 2015 17:56:23 +0100,
> Vinod Koul wrote:
> > 
> > From: Jeeja KP <[e-mail]>
> > 
> > This patch adds pcm capability to support Resume.
> 
> The flag indicates that the driver is capable of the full resume --
> i.e. it must resume everything at the exactly same point to be
> suspended.  Is it really the case?
> 
> If it's not and it's instead a kind of restart of streams, don't add
> this flag.  The lack of the flag doesn't mean that the driver can't
> resume at all.

Yes, with this patch series we are able to resume from previous position.

Here is my test:

# aplay -Dhw:0,0 play.wav
Playing WAVE 'play.wav' : Signed 16 bit Little Endian, Rate 48000 Hz,
Stereo
Suspended. Trying resume. Done.

Thanks

Takashi Iwai Nov. 24, 2015, 6:07 a.m. UTC | #3

On Tue, 24 Nov 2015 03:44:49 +0100,
Vinod Koul wrote:
> 
> On Mon, Nov 23, 2015 at 07:45:20PM +0100, Takashi Iwai wrote:
> > On Mon, 23 Nov 2015 17:56:23 +0100,
> > Vinod Koul wrote:
> > > 
> > > From: Jeeja KP <[e-mail]>
> > > 
> > > This patch adds pcm capability to support Resume.
> > 
> > The flag indicates that the driver is capable of the full resume --
> > i.e. it must resume everything at the exactly same point to be
> > suspended.  Is it really the case?
> > 
> > If it's not and it's instead a kind of restart of streams, don't add
> > this flag.  The lack of the flag doesn't mean that the driver can't
> > resume at all.
> 
> Yes, with this patch series we are able to resume from previous position.

I suppose you tested hibernation, too?
How does it assure that the in-flight data on FIFO are restored after
the link reset and re-prepare?

> Here is my test:
> 
> # aplay -Dhw:0,0 play.wav
> Playing WAVE 'play.wav' : Signed 16 bit Little Endian, Rate 48000 Hz,
> Stereo
> Suspended. Trying resume. Done.

As mentioned, the resume usually works (sort of), even without
SNDRV_PCM_INFO_RESUME bit.  It's because alsa-lib tries to re-prepare
the stuff by itself.

Did you try only the patch 2 without this patch?


Takashi

Vinod Koul Nov. 26, 2015, 8:56 a.m. UTC | #4

On Tue, Nov 24, 2015 at 07:07:11AM +0100, Takashi Iwai wrote:
> On Tue, 24 Nov 2015 03:44:49 +0100,
> Vinod Koul wrote:
> > 
> > On Mon, Nov 23, 2015 at 07:45:20PM +0100, Takashi Iwai wrote:
> > > On Mon, 23 Nov 2015 17:56:23 +0100,
> > > Vinod Koul wrote:
> > > > 
> > > > From: Jeeja KP <[e-mail]>
> > > > 
> > > > This patch adds pcm capability to support Resume.
> > > 
> > > The flag indicates that the driver is capable of the full resume --
> > > i.e. it must resume everything at the exactly same point to be
> > > suspended.  Is it really the case?
> > > 
> > > If it's not and it's instead a kind of restart of streams, don't add
> > > this flag.  The lack of the flag doesn't mean that the driver can't
> > > resume at all.
> > 
> > Yes, with this patch series we are able to resume from previous position.
> 
> I suppose you tested hibernation, too?

No, only suspend and freeze

> How does it assure that the in-flight data on FIFO are restored after
> the link reset and re-prepare?

SKL has DMA resume capability, so we can restore the registers, Jeeja is
preparing those patches and we should be able to send those shortly :)

> > Here is my test:
> > 
> > # aplay -Dhw:0,0 play.wav
> > Playing WAVE 'play.wav' : Signed 16 bit Little Endian, Rate 48000 Hz,
> > Stereo
> > Suspended. Trying resume. Done.
> 
> As mentioned, the resume usually works (sort of), even without
> SNDRV_PCM_INFO_RESUME bit.  It's because alsa-lib tries to re-prepare
> the stuff by itself.
> 
> Did you try only the patch 2 without this patch?

Yes Jeeja did that and in that case it does prepare and resumes

Takashi Iwai Nov. 26, 2015, 9:06 a.m. UTC | #5

On Thu, 26 Nov 2015 09:56:01 +0100,
Vinod Koul wrote:
> 
> On Tue, Nov 24, 2015 at 07:07:11AM +0100, Takashi Iwai wrote:
> > On Tue, 24 Nov 2015 03:44:49 +0100,
> > Vinod Koul wrote:
> > > 
> > > On Mon, Nov 23, 2015 at 07:45:20PM +0100, Takashi Iwai wrote:
> > > > On Mon, 23 Nov 2015 17:56:23 +0100,
> > > > Vinod Koul wrote:
> > > > > 
> > > > > From: Jeeja KP <[e-mail]>
> > > > > 
> > > > > This patch adds pcm capability to support Resume.
> > > > 
> > > > The flag indicates that the driver is capable of the full resume --
> > > > i.e. it must resume everything at the exactly same point to be
> > > > suspended.  Is it really the case?
> > > > 
> > > > If it's not and it's instead a kind of restart of streams, don't add
> > > > this flag.  The lack of the flag doesn't mean that the driver can't
> > > > resume at all.
> > > 
> > > Yes, with this patch series we are able to resume from previous position.
> > 
> > I suppose you tested hibernation, too?
> 
> No, only suspend and freeze
> 
> > How does it assure that the in-flight data on FIFO are restored after
> > the link reset and re-prepare?
> 
> SKL has DMA resume capability, so we can restore the registers, Jeeja is
> preparing those patches and we should be able to send those shortly :)

Well, the question is about hibernation.  Then the in-flight data on
the device is basically gone, and we can handle only the remaining CPU
data.  If your DMA can do that, it's fine, we can go in that way.

> > > Here is my test:
> > > 
> > > # aplay -Dhw:0,0 play.wav
> > > Playing WAVE 'play.wav' : Signed 16 bit Little Endian, Rate 48000 Hz,
> > > Stereo
> > > Suspended. Trying resume. Done.
> > 
> > As mentioned, the resume usually works (sort of), even without
> > SNDRV_PCM_INFO_RESUME bit.  It's because alsa-lib tries to re-prepare
> > the stuff by itself.
> > 
> > Did you try only the patch 2 without this patch?
> 
> Yes Jeeja did that and in that case it does prepare and resumes

That's the expected result.  As a fallback, user-space expects
prepares and restarts.


Takashi

Patch

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index dae332beea3f..4378bf452582 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -35,6 +35,7 @@  static struct snd_pcm_hardware azx_pcm_hw = {
 				 SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				 SNDRV_PCM_INFO_MMAP_VALID |
 				 SNDRV_PCM_INFO_PAUSE |
+				 SNDRV_PCM_INFO_RESUME |
 				 SNDRV_PCM_INFO_SYNC_START |
 				 SNDRV_PCM_INFO_HAS_WALL_CLOCK | /* legacy */
 				 SNDRV_PCM_INFO_HAS_LINK_ATIME |