-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC]Suspend/Resume flow for APL #32
Changes from all commits
8398e9a
644e4bf
8880888
9bc5251
47d23a6
e1658fe
7f21cdb
2824587
a1e21c6
aa8cb23
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ | |
#include <sound/sof.h> | ||
#include <sound/pcm_params.h> | ||
#include <linux/pm_runtime.h> | ||
|
||
#include <uapi/sound/sof-ipc.h> | ||
#include "../sof-priv.h" | ||
#include "../ops.h" | ||
#include "hda.h" | ||
|
@@ -239,3 +239,18 @@ int hda_dsp_core_reset_power_down(struct snd_sof_dev *sdev, | |
return ret; | ||
} | ||
|
||
int hda_dsp_suspend(struct snd_sof_dev *sdev, int state) | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd expect most of this function to be in pm.c as generic code. The HW specific part hda_dsp_core_reset_power_down() can be an HW abstracted operation (add ops to the operations callback structure). e.g. we could have snd_sof_dsp_suspend(sdev), snd_sof_dsp_resume(sdev) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with Liam, this feels too APL-specific. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @plbossart @lgirdwood I have updated this to use the chip cores_mask to make it generic. |
||
const struct sof_intel_dsp_desc *chip = sdev->hda->desc; | ||
|
||
/* power down DSP */ | ||
return hda_dsp_core_reset_power_down(sdev, chip->cores_mask); | ||
} | ||
|
||
int hda_dsp_resume(struct snd_sof_dev *sdev) | ||
{ | ||
const struct sof_intel_dsp_desc *chip = sdev->hda->desc; | ||
|
||
/* power up the DSP */ | ||
return hda_dsp_core_power_up(sdev, chip->cores_mask); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -158,6 +158,9 @@ static int sof_pcm_hw_params(struct snd_pcm_substream *substream, | |
spcm->posn_offset[substream->stream] = | ||
sdev->stream_box.offset + posn_offset; | ||
|
||
/* save pcm hw_params */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be squashed? |
||
memcpy(&spcm->params[substream->stream], params, sizeof(*params)); | ||
|
||
return ret; | ||
} | ||
|
||
|
@@ -190,6 +193,33 @@ static int sof_pcm_hw_free(struct snd_pcm_substream *substream) | |
return ret; | ||
} | ||
|
||
static int sof_restore_hw_params(struct snd_pcm_substream *substream, | ||
struct snd_sof_pcm *spcm, | ||
struct snd_sof_dev *sdev) | ||
{ | ||
snd_pcm_uframes_t host = 0; | ||
u64 host_posn; | ||
int ret = 0; | ||
|
||
/* resume stream */ | ||
host_posn = spcm->stream[substream->stream].posn.host_posn; | ||
host = bytes_to_frames(substream->runtime, host_posn); | ||
dev_dbg(sdev->dev, | ||
"PCM: resume stream %d dir %d DMA position %lu\n", | ||
spcm->pcm.pcm_id, substream->stream, host); | ||
|
||
/* set hw_params */ | ||
ret = sof_pcm_hw_params(substream, | ||
&spcm->params[substream->stream]); | ||
if (ret < 0) { | ||
dev_err(sdev->dev, | ||
"error: set pcm hw_params after resume\n"); | ||
return ret; | ||
} | ||
|
||
return 0; | ||
} | ||
|
||
static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd) | ||
{ | ||
struct snd_soc_pcm_runtime *rtd = substream->private_data; | ||
|
@@ -198,6 +228,8 @@ static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd) | |
struct snd_sof_pcm *spcm = rtd->sof; | ||
struct sof_ipc_stream stream; | ||
struct sof_ipc_reply reply; | ||
snd_pcm_uframes_t host = 0; | ||
u64 host_posn; | ||
int ret = 0; | ||
|
||
/* nothing todo for BE */ | ||
|
@@ -216,15 +248,56 @@ static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd) | |
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START; | ||
break; | ||
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: | ||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE; | ||
|
||
/* check if the stream hw_params needs to be restored */ | ||
if (spcm->restore_stream[substream->stream]) { | ||
|
||
/* restore hw_params */ | ||
ret = sof_restore_hw_params(substream, spcm, sdev); | ||
if (ret < 0) | ||
return ret; | ||
|
||
/* unset restore_stream */ | ||
spcm->restore_stream[substream->stream] = 0; | ||
|
||
/* trigger start */ | ||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START; | ||
} else { | ||
|
||
/* trigger pause release */ | ||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE; | ||
} | ||
break; | ||
case SNDRV_PCM_TRIGGER_STOP: | ||
|
||
/* | ||
* Check if stream was marked for restore before suspend | ||
*/ | ||
if (spcm->restore_stream[substream->stream]) { | ||
|
||
/* unset restore_stream */ | ||
spcm->restore_stream[substream->stream] = 0; | ||
|
||
/* do not send ipc as the stream hasn't been set up */ | ||
return 0; | ||
} | ||
|
||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_STOP; | ||
break; | ||
case SNDRV_PCM_TRIGGER_PAUSE_PUSH: | ||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_PAUSE; | ||
break; | ||
case SNDRV_PCM_TRIGGER_RESUME: | ||
|
||
/* restore hw_params */ | ||
ret = sof_restore_hw_params(substream, spcm, sdev); | ||
if (ret < 0) | ||
return ret; | ||
|
||
/* trigger stream */ | ||
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_START; | ||
|
||
break; | ||
case SNDRV_PCM_TRIGGER_SUSPEND: | ||
break; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't there be some IPC sent on suspend, so that e.g. if there is any sort of context saving at the firmware level they'd know about it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @plbossart I do send the CTX_SAVE ipc during suspend in the suspend callback but not here as it is not a stream ipc message. |
||
default: | ||
|
@@ -580,13 +653,6 @@ static int sof_pcm_probe(struct snd_soc_platform *platform) | |
goto err; | ||
} | ||
|
||
/* enable runtime PM with auto suspend */ | ||
pm_runtime_set_autosuspend_delay(platform->dev, | ||
SND_SOF_SUSPEND_DELAY); | ||
pm_runtime_use_autosuspend(platform->dev); | ||
pm_runtime_enable(platform->dev); | ||
pm_runtime_idle(platform->dev); | ||
|
||
err: | ||
return ret; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is correct. I would tend to think that the ACPI/PCI device are at the top in the device hierarchy, so wondering if the pm operations should be done at the highest level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart @lgirdwood does it make sense to add the suspend/resume to the pci device and the runtime_suspend/runtime_resume() to the platform device?
Or should the ACPI/PCI device be the only one with all the callbacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart @lgirdwood runtime_enable for the pci device shows as forbidden. So doesnt look like I can enable it there.
I could use some guidance here. Is it OK to set the PM callbacks for the sof-audio device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the skylake driver the pm_ops are set in the pci driver, see sound/soc/intel/skylake/skl.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@plbossart I tried to do it the same way as in the skl driver. The problem I am facing is that the runtime_enable for the pci device shows as forbidden. Could this be because I havent enabled it for its child device? I'll check on that