[alsa-devel] [PATCH v2 0/3] ASoC: SOF: suspend/resume debug tools
These patches were submitted in an earlier series but not merged and then conflicted with the IPC flood test.
Resending, no new functionality. These helpers are really helpful for e.g. SoundWire or HDaudio bringup.
Pierre-Louis Bossart (3): ASoC: SOF: trace: move to opt-in with Kconfig and module parameter ASoC: SOF: acpi: add module param to disable pm_runtime ASoC: SOF: pci: add module param to disable pm_runtime
sound/soc/sof/Kconfig | 8 ++++++++ sound/soc/sof/core.c | 26 ++++++++++++++++++++------ sound/soc/sof/sof-acpi-dev.c | 12 +++++++++++- sound/soc/sof/sof-pci-dev.c | 12 +++++++++++- sound/soc/sof/sof-priv.h | 1 + sound/soc/sof/trace.c | 17 ++++++++++++++++- 6 files changed, 67 insertions(+), 9 deletions(-)
In a number of debug cases, the DMA-based trace can add problems (e.g. with HDaudio channel allocation). It also generates additional traffic on the bus and if the DMA handling is unreliable will prevent audio use-cases from working normally. Using the trace also requires tools to be installed on the target.
The trace can be instead handled as dynamic debug. We can use a Kconfig to force the trace to be enabled in all cases, or use a module parameter to enable it on a need-basis, e.g. by setting "options snd_sof sof_debug=0x1" in a /etc/modprobe.d file.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/Kconfig | 8 ++++++++ sound/soc/sof/core.c | 26 ++++++++++++++++++++------ sound/soc/sof/sof-priv.h | 1 + sound/soc/sof/trace.c | 17 ++++++++++++++++- 4 files changed, 45 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sof/Kconfig b/sound/soc/sof/Kconfig index 41f79cdcbf47..efb4cf3eec56 100644 --- a/sound/soc/sof/Kconfig +++ b/sound/soc/sof/Kconfig @@ -132,6 +132,14 @@ config SND_SOC_SOF_DEBUG_ENABLE_DEBUGFS_CACHE Say Y if you want to enable caching the memory windows. If unsure, select "N".
+config SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE + bool "SOF enable firmware trace" + help + The firmware trace can be enabled either at build-time with + this option, or dynamically by setting flags in the SOF core + module parameter (similar to dynamic debug) + If unsure, select "N". + config SND_SOC_SOF_DEBUG_IPC_FLOOD_TEST bool "SOF enable IPC flood test" help diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c index 5beda47cdf9f..70b471be07b2 100644 --- a/sound/soc/sof/core.c +++ b/sound/soc/sof/core.c @@ -16,6 +16,12 @@ #include "sof-priv.h" #include "ops.h"
+static int sof_core_debug; +module_param_named(sof_debug, sof_core_debug, int, 0444); +MODULE_PARM_DESC(sof_debug, "SOF core debug options (0x0 all off)"); + +#define SOF_CORE_ENABLE_TRACE BIT(0) + /* SOF defaults if not provided by the platform in ms */ #define TIMEOUT_DEFAULT_IPC_MS 5 #define TIMEOUT_DEFAULT_BOOT_MS 100 @@ -350,12 +356,20 @@ static int sof_probe_continue(struct snd_sof_dev *sdev) goto fw_run_err; }
- /* init DMA trace */ - ret = snd_sof_init_trace(sdev); - if (ret < 0) { - /* non fatal */ - dev_warn(sdev->dev, - "warning: failed to initialize trace %d\n", ret); + if (IS_ENABLED(CONFIG_SND_SOC_SOF_DEBUG_ENABLE_FIRMWARE_TRACE) || + (sof_core_debug & SOF_CORE_ENABLE_TRACE)) { + sdev->dtrace_is_supported = true; + + /* init DMA trace */ + ret = snd_sof_init_trace(sdev); + if (ret < 0) { + /* non fatal */ + dev_warn(sdev->dev, + "warning: failed to initialize trace %d\n", + ret); + } + } else { + dev_dbg(sdev->dev, "SOF firmware trace disabled\n"); }
/* hereafter all FW boot flows are for PM reasons */ diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h index 17f3d2a5a701..563623bcaad6 100644 --- a/sound/soc/sof/sof-priv.h +++ b/sound/soc/sof/sof-priv.h @@ -419,6 +419,7 @@ struct snd_sof_dev { int dma_trace_pages; wait_queue_head_t trace_sleep; u32 host_offset; + u32 dtrace_is_supported; /* set with Kconfig or module parameter */ u32 dtrace_is_enabled; u32 dtrace_error; u32 dtrace_draining; diff --git a/sound/soc/sof/trace.c b/sound/soc/sof/trace.c index befed975161c..84d89206a0e0 100644 --- a/sound/soc/sof/trace.c +++ b/sound/soc/sof/trace.c @@ -167,6 +167,9 @@ int snd_sof_init_trace_ipc(struct snd_sof_dev *sdev) struct sof_ipc_reply ipc_reply; int ret;
+ if (!sdev->dtrace_is_supported) + return 0; + if (sdev->dtrace_is_enabled || !sdev->dma_trace_pages) return -EINVAL;
@@ -227,6 +230,9 @@ int snd_sof_init_trace(struct snd_sof_dev *sdev) { int ret;
+ if (!sdev->dtrace_is_supported) + return 0; + /* set false before start initialization */ sdev->dtrace_is_enabled = false;
@@ -282,6 +288,9 @@ EXPORT_SYMBOL(snd_sof_init_trace); int snd_sof_trace_update_pos(struct snd_sof_dev *sdev, struct sof_ipc_dma_trace_posn *posn) { + if (!sdev->dtrace_is_supported) + return 0; + if (sdev->dtrace_is_enabled && sdev->host_offset != posn->host_offset) { sdev->host_offset = posn->host_offset; wake_up(&sdev->trace_sleep); @@ -298,6 +307,9 @@ int snd_sof_trace_update_pos(struct snd_sof_dev *sdev, /* an error has occurred within the DSP that prevents further trace */ void snd_sof_trace_notify_for_error(struct snd_sof_dev *sdev) { + if (!sdev->dtrace_is_supported) + return; + if (sdev->dtrace_is_enabled) { dev_err(sdev->dev, "error: waking up any trace sleepers\n"); sdev->dtrace_error = true; @@ -310,7 +322,7 @@ void snd_sof_release_trace(struct snd_sof_dev *sdev) { int ret;
- if (!sdev->dtrace_is_enabled) + if (!sdev->dtrace_is_supported || !sdev->dtrace_is_enabled) return;
ret = snd_sof_dma_trace_trigger(sdev, SNDRV_PCM_TRIGGER_STOP); @@ -331,6 +343,9 @@ EXPORT_SYMBOL(snd_sof_release_trace);
void snd_sof_free_trace(struct snd_sof_dev *sdev) { + if (!sdev->dtrace_is_supported) + return; + snd_sof_release_trace(sdev);
snd_dma_free_pages(&sdev->dmatb);
On Wed, Jun 12, 2019 at 11:47:24AM -0500, Pierre-Louis Bossart wrote:
(sof_core_debug & SOF_CORE_ENABLE_TRACE)) {sdev->dtrace_is_supported = true;
Given that dtrace is a widely known product with a Linux implementation can we not call this dtrace? use_dma_trace or something? A long name doesn't seem too bad here but a collision with the more common usage of the name isn't great.
On 6/13/19 8:44 PM, Mark Brown wrote:
On Wed, Jun 12, 2019 at 11:47:24AM -0500, Pierre-Louis Bossart wrote:
(sof_core_debug & SOF_CORE_ENABLE_TRACE)) {sdev->dtrace_is_supported = true;Given that dtrace is a widely known product with a Linux implementation can we not call this dtrace? use_dma_trace or something? A long name doesn't seem too bad here but a collision with the more common usage of the name isn't great.
Sure, we can rename these fields. I'll send a v3.
Add debug option to disable pm_runtime. This is not intended for production devices but is very useful for platform bringup.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/sof-acpi-dev.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/sof-acpi-dev.c b/sound/soc/sof/sof-acpi-dev.c index c8dafb1ac54e..93a8e15bbd2c 100644 --- a/sound/soc/sof/sof-acpi-dev.c +++ b/sound/soc/sof/sof-acpi-dev.c @@ -29,6 +29,12 @@ static char *tplg_path; module_param(tplg_path, charp, 0444); MODULE_PARM_DESC(tplg_path, "alternate path for SOF topology.");
+static int sof_acpi_debug; +module_param_named(sof_debug, sof_acpi_debug, int, 0444); +MODULE_PARM_DESC(sof_debug, "SOF ACPI debug options (0x0 all off)"); + +#define SOF_ACPI_DISABLE_PM_RUNTIME BIT(0) + #if IS_ENABLED(CONFIG_SND_SOC_SOF_HASWELL) static const struct sof_dev_desc sof_acpi_haswell_desc = { .machines = snd_soc_acpi_intel_haswell_machines, @@ -121,6 +127,9 @@ static const struct dev_pm_ops sof_acpi_pm = {
static void sof_acpi_probe_complete(struct device *dev) { + if (sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME) + return; + /* allow runtime_pm */ pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS); pm_runtime_use_autosuspend(dev); @@ -221,7 +230,8 @@ static int sof_acpi_probe(struct platform_device *pdev)
static int sof_acpi_remove(struct platform_device *pdev) { - pm_runtime_disable(&pdev->dev); + if (!(sof_acpi_debug & SOF_ACPI_DISABLE_PM_RUNTIME)) + pm_runtime_disable(&pdev->dev);
/* call sof helper for DSP hardware remove */ snd_sof_device_remove(&pdev->dev);
On Wed, Jun 12, 2019 at 11:47:25AM -0500, Pierre-Louis Bossart wrote:
Add debug option to disable pm_runtime. This is not intended for production devices but is very useful for platform bringup.
I can't immediately find it right now but isn't there some generic way of doing this in the runtime PM framework? If not it seems like it'd be a good thing to add, these can't be the only devices where it'd be useful.
On Thu, 13 Jun 2019 20:48:01 +0200, Mark Brown wrote:
On Wed, Jun 12, 2019 at 11:47:25AM -0500, Pierre-Louis Bossart wrote:
Add debug option to disable pm_runtime. This is not intended for production devices but is very useful for platform bringup.
I can't immediately find it right now but isn't there some generic way of doing this in the runtime PM framework? If not it seems like it'd be a good thing to add, these can't be the only devices where it'd be useful.
Well, runtime PM can be fully controlled via sysfs, but the problem is that the driver declares itself being runtime-enabled. So, either we leave it default and let user-space enabling it (via udev or other way), or introduce some condition in the driver side.
Takashi
On Thu, Jun 13, 2019 at 09:04:28PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
I can't immediately find it right now but isn't there some generic way of doing this in the runtime PM framework? If not it seems like it'd be a good thing to add, these can't be the only devices where it'd be useful.
Well, runtime PM can be fully controlled via sysfs, but the problem is that the driver declares itself being runtime-enabled. So, either we leave it default and let user-space enabling it (via udev or other way), or introduce some condition in the driver side.
I thought someone had added a command line parameter to do it based on dev_name(), perhaps they were just talking about it or it was in some BSP somewhere though.
I can't immediately find it right now but isn't there some generic way of doing this in the runtime PM framework? If not it seems like it'd be a good thing to add, these can't be the only devices where it'd be useful.
Well, runtime PM can be fully controlled via sysfs, but the problem is that the driver declares itself being runtime-enabled. So, either we leave it default and let user-space enabling it (via udev or other way), or introduce some condition in the driver side.
I thought someone had added a command line parameter to do it based on dev_name(), perhaps they were just talking about it or it was in some BSP somewhere though.
If there is a better way I am all ears. It's indeed not very elegant to duplicate the same parameter for two different modules and it's not an SOF-specific need.
The only way I am aware of is to play with /sys/bus/pci/devices/xyz/power/ files but it's not very useful if you want to disable the initial runtime pm transition which is often the more problematic one. Completely removing runtime_pm support from all drivers at compile time is also not very good either.
Add debug option to disable pm_runtime. This is not intended for production devices but is very useful for platform bringup.
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/soc/sof/sof-pci-dev.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c index ab58d4f9119f..ffe485f4ba71 100644 --- a/sound/soc/sof/sof-pci-dev.c +++ b/sound/soc/sof/sof-pci-dev.c @@ -29,6 +29,12 @@ static char *tplg_path; module_param(tplg_path, charp, 0444); MODULE_PARM_DESC(tplg_path, "alternate path for SOF topology.");
+static int sof_pci_debug; +module_param_named(sof_debug, sof_pci_debug, int, 0444); +MODULE_PARM_DESC(sof_debug, "SOF PCI debug options (0x0 all off)"); + +#define SOF_PCI_DISABLE_PM_RUNTIME BIT(0) + #if IS_ENABLED(CONFIG_SND_SOC_SOF_APOLLOLAKE) static const struct sof_dev_desc bxt_desc = { .machines = snd_soc_acpi_intel_bxt_machines, @@ -213,6 +219,9 @@ static void sof_pci_probe_complete(struct device *dev) { dev_dbg(dev, "Completing SOF PCI probe");
+ if (sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME) + return; + /* allow runtime_pm */ pm_runtime_set_autosuspend_delay(dev, SND_SOF_SUSPEND_DELAY_MS); pm_runtime_use_autosuspend(dev); @@ -333,7 +342,8 @@ static void sof_pci_remove(struct pci_dev *pci) snd_sof_device_remove(&pci->dev);
/* follow recommendation in pci-driver.c to increment usage counter */ - pm_runtime_get_noresume(&pci->dev); + if (!(sof_pci_debug & SOF_PCI_DISABLE_PM_RUNTIME)) + pm_runtime_get_noresume(&pci->dev);
/* release pci regions and disable device */ pci_release_regions(pci);
participants (3)
-
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai