Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RFC]Suspend/Resume flow for APL #32

Closed
wants to merge 10 commits into from
Closed

[RFC]Suspend/Resume flow for APL #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8398e9a
ASoC: SOF: add suspend/resume callbacks for APL
ranj063 Jul 13, 2018
644e4bf
ASoC: SOF: store ipc comp data as private data
ranj063 Jul 16, 2018
8880888
ASoC: SOF: create route list in snd_sof_dev to store pipeline connect…
ranj063 Jul 17, 2018
9bc5251
ASoC: SOF: move ipc for initializing trace into a separate function
ranj063 Jul 17, 2018
47d23a6
ASoC: SOF: make sof_complete_pipeline non static
ranj063 Jul 17, 2018
e1658fe
ASoC: SOF: suspend/resume and runtime PM implementation
ranj063 Jul 13, 2018
7f21cdb
ASoC: SOF: implement resume trigger flow
ranj063 Jul 18, 2018
2824587
ASoC: SOF: invoke runtime_put after booting firmware
ranj063 Jul 20, 2018
a1e21c6
ASoC: SOF: set kcontrol cmd for pga widget
ranj063 Jul 21, 2018
aa8cb23
ASoC: SOF: move PM callbacks to the SOF platform device
ranj063 Jul 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Author: Liam Girdwood <[email protected]>
*/

#include <linux/pm_runtime.h>
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/slab.h>
Expand Down Expand Up @@ -249,6 +250,7 @@ static int sof_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&sdev->kcontrol_list);
INIT_LIST_HEAD(&sdev->widget_list);
INIT_LIST_HEAD(&sdev->dai_list);
INIT_LIST_HEAD(&sdev->route_list);
dev_set_drvdata(&pdev->dev, sdev);
spin_lock_init(&sdev->ipc_lock);
spin_lock_init(&sdev->hw_lock);
Expand Down Expand Up @@ -330,6 +332,9 @@ static int sof_probe(struct platform_device *pdev)
"warning: failed to initialize trace %d\n", ret);
}

pm_runtime_mark_last_busy(sdev->dev);
pm_runtime_put_autosuspend(sdev->dev);

return 0;

comp_err:
Expand Down Expand Up @@ -366,9 +371,17 @@ void snd_sof_shutdown(struct device *dev)
}
EXPORT_SYMBOL(snd_sof_shutdown);

static const struct dev_pm_ops sof_platform_pm = {
Copy link
Member

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?

Copy link
Collaborator Author

@ranj063 ranj063 Jul 21, 2018 •
edited
Loading

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?

Copy link
Collaborator Author

@ranj063 ranj063 Jul 22, 2018 •
edited
Loading

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?

Copy link
Member

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

Copy link
Collaborator Author

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

SET_SYSTEM_SLEEP_PM_OPS(snd_sof_suspend, snd_sof_resume)
SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
NULL)
.suspend_late = snd_sof_suspend_late,
};

static struct platform_driver sof_driver = {
.driver = {
.name = "sof-audio",
.pm = &sof_platform_pm,
},

.probe = sof_probe,
Expand Down
4 changes: 4 additions & 0 deletions sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,9 @@ struct snd_sof_dsp_ops sof_apl_ops = {
/* DAI drivers */
.drv = skl_dai,
.num_drv = SOF_SKL_NUM_DAIS,

/* PM */
.suspend = hda_dsp_suspend,
.resume = hda_dsp_resume,
};
EXPORT_SYMBOL(sof_apl_ops);
17 changes: 16 additions & 1 deletion sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
{
Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Liam, this feels too APL-specific.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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);
}
2 changes: 2 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,8 @@ bool hda_dsp_core_is_enabled(struct snd_sof_dev *sdev,
unsigned int core_mask);
int hda_dsp_core_reset_power_down(struct snd_sof_dev *sdev,
unsigned int core_mask);
int hda_dsp_suspend(struct snd_sof_dev *sdev, int state);
int hda_dsp_resume(struct snd_sof_dev *sdev);
void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags);

/*
Expand Down
82 changes: 74 additions & 8 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The 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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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 */
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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:
Expand Down Expand Up @@ -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;
}
Expand Down
Loading