diff mbox series

ASoC: SOF: Intel: hda: Add support for persistent Code Loader DMA buffers

Message ID 20241107121440.1472-1-peter.ujfalusi@linux.intel.com (mailing list archive)
State Accepted
Commit 1862e847bf115a3ccbf38dd035ea0118be57f2e2
Headers show
Series ASoC: SOF: Intel: hda: Add support for persistent Code Loader DMA buffers | expand

Commit Message

Peter Ujfalusi Nov. 7, 2024, 12:14 p.m. UTC 
It has been reported that the DMA memory allocation for firmware download
can fail after extended period of uptime on systems with relatively small
amount of RAM when the system memory becomes fragmented.

The issue primarily happens  when the system is waking up from system
suspend, swap might not be available and the MM system cannot move things
around to allow for successful allocation.

If the IMR boot is not supported then for each DSP boot we would need to
allocate the DMA buffer for firmware transfer, which can increase the
chances of the issue to be hit.

This patch adds support for allocating the DMA buffers once at first boot
time and keep it until the system is shut down, rebooted or the drivers
re-loaded and makes this as the default operation.

With persistent_cl_buffer module parameter the persistent Code Loader
DMA buffer can be disabled to fall back to on demand allocation.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Liam Girdwood <liam.r.girdwood@intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/soc/sof/intel/hda-loader.c | 100 ++++++++++++++++++++++---------
 sound/soc/sof/intel/hda.c        |   6 ++
 sound/soc/sof/intel/hda.h        |  14 ++++-
 3 files changed, 90 insertions(+), 30 deletions(-)

Comments

Mark Brown Nov. 7, 2024, 3:23 p.m. UTC | #1 
On Thu, 07 Nov 2024 14:14:40 +0200, Peter Ujfalusi wrote:
> It has been reported that the DMA memory allocation for firmware download
> can fail after extended period of uptime on systems with relatively small
> amount of RAM when the system memory becomes fragmented.
> 
> The issue primarily happens  when the system is waking up from system
> suspend, swap might not be available and the MM system cannot move things
> around to allow for successful allocation.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: SOF: Intel: hda: Add support for persistent Code Loader DMA buffers
      commit: 1862e847bf115a3ccbf38dd035ea0118be57f2e2

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
diff mbox series

Patch

diff --git a/sound/soc/sof/intel/hda-loader.c b/sound/soc/sof/intel/hda-loader.c
index 9d8ebb7c6a10..76a03b6b2728 100644
--- a/sound/soc/sof/intel/hda-loader.c
+++ b/sound/soc/sof/intel/hda-loader.c
@@ -26,6 +26,11 @@ 
 #include "../sof-priv.h"
 #include "hda.h"
 
+static bool persistent_cl_buffer = true;
+module_param(persistent_cl_buffer, bool, 0444);
+MODULE_PARM_DESC(persistent_cl_buffer, "Persistent Code Loader DMA buffer "
+		 "(default = Y, use N to force buffer re-allocation)");
+
 static void hda_ssp_set_cbp_cfp(struct snd_sof_dev *sdev)
 {
 	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
@@ -43,9 +48,10 @@  static void hda_ssp_set_cbp_cfp(struct snd_sof_dev *sdev)
 	}
 }
 
-struct hdac_ext_stream *hda_cl_prepare(struct device *dev, unsigned int format,
-				       unsigned int size, struct snd_dma_buffer *dmab,
-				       int direction, bool is_iccmax)
+struct hdac_ext_stream*
+hda_cl_prepare(struct device *dev, unsigned int format, unsigned int size,
+	       struct snd_dma_buffer *dmab, bool persistent_buffer, int direction,
+	       bool is_iccmax)
 {
 	struct snd_sof_dev *sdev = dev_get_drvdata(dev);
 	struct hdac_ext_stream *hext_stream;
@@ -61,11 +67,19 @@  struct hdac_ext_stream *hda_cl_prepare(struct device *dev, unsigned int format,
 	hstream = &hext_stream->hstream;
 	hstream->substream = NULL;
 
-	/* allocate DMA buffer */
-	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG, dev, size, dmab);
-	if (ret < 0) {
-		dev_err(sdev->dev, "error: memory alloc failed: %d\n", ret);
-		goto out_put;
+	/*
+	 * Allocate DMA buffer if it is temporary or if the buffer is intended
+	 * to be persistent but not yet allocated.
+	 * We cannot rely solely on !dmab->area as caller might use a struct on
+	 * stack (when it is temporary) without clearing it to 0.
+	 */
+	if (!persistent_buffer || !dmab->area) {
+		ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_SG, dev, size, dmab);
+		if (ret < 0) {
+			dev_err(sdev->dev, "%s: memory alloc failed: %d\n",
+				__func__, ret);
+			goto out_put;
+		}
 	}
 
 	hstream->period_bytes = 0;/* initialize period_bytes */
@@ -91,6 +105,10 @@  struct hdac_ext_stream *hda_cl_prepare(struct device *dev, unsigned int format,
 
 out_free:
 	snd_dma_free_pages(dmab);
+	dmab->area = NULL;
+	dmab->bytes = 0;
+	hstream->bufsize = 0;
+	hstream->format_val = 0;
 out_put:
 	hda_dsp_stream_put(sdev, direction, hstream->stream_tag);
 	return ERR_PTR(ret);
@@ -255,7 +273,7 @@  int hda_cl_trigger(struct device *dev, struct hdac_ext_stream *hext_stream, int
 EXPORT_SYMBOL_NS(hda_cl_trigger, SND_SOC_SOF_INTEL_HDA_COMMON);
 
 int hda_cl_cleanup(struct device *dev, struct snd_dma_buffer *dmab,
-		   struct hdac_ext_stream *hext_stream)
+			  bool persistent_buffer, struct hdac_ext_stream *hext_stream)
 {
 	struct snd_sof_dev *sdev =  dev_get_drvdata(dev);
 	struct hdac_stream *hstream = &hext_stream->hstream;
@@ -279,10 +297,14 @@  int hda_cl_cleanup(struct device *dev, struct snd_dma_buffer *dmab,
 			  sd_offset + SOF_HDA_ADSP_REG_SD_BDLPU, 0);
 
 	snd_sof_dsp_write(sdev, HDA_DSP_HDA_BAR, sd_offset, 0);
-	snd_dma_free_pages(dmab);
-	dmab->area = NULL;
-	hstream->bufsize = 0;
-	hstream->format_val = 0;
+
+	if (!persistent_buffer) {
+		snd_dma_free_pages(dmab);
+		dmab->area = NULL;
+		dmab->bytes = 0;
+		hstream->bufsize = 0;
+		hstream->format_val = 0;
+	}
 
 	return ret;
 }
@@ -340,8 +362,8 @@  int hda_cl_copy_fw(struct snd_sof_dev *sdev, struct hdac_ext_stream *hext_stream
 
 int hda_dsp_cl_boot_firmware_iccmax(struct snd_sof_dev *sdev)
 {
+	struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
 	struct hdac_ext_stream *iccmax_stream;
-	struct snd_dma_buffer dmab_bdl;
 	int ret, ret1;
 	u8 original_gb;
 
@@ -354,7 +376,8 @@  int hda_dsp_cl_boot_firmware_iccmax(struct snd_sof_dev *sdev)
 	 * the data, so use a buffer of PAGE_SIZE for receiving.
 	 */
 	iccmax_stream = hda_cl_prepare(sdev->dev, HDA_CL_STREAM_FORMAT, PAGE_SIZE,
-				       &dmab_bdl, SNDRV_PCM_STREAM_CAPTURE, true);
+				       &hda->iccmax_dmab, persistent_cl_buffer,
+				       SNDRV_PCM_STREAM_CAPTURE, true);
 	if (IS_ERR(iccmax_stream)) {
 		dev_err(sdev->dev, "error: dma prepare for ICCMAX stream failed\n");
 		return PTR_ERR(iccmax_stream);
@@ -366,7 +389,8 @@  int hda_dsp_cl_boot_firmware_iccmax(struct snd_sof_dev *sdev)
 	 * Perform iccmax stream cleanup. This should be done even if firmware loading fails.
 	 * If the cleanup also fails, we return the initial error
 	 */
-	ret1 = hda_cl_cleanup(sdev->dev, &dmab_bdl, iccmax_stream);
+	ret1 = hda_cl_cleanup(sdev->dev, &hda->iccmax_dmab,
+			      persistent_cl_buffer, iccmax_stream);
 	if (ret1 < 0) {
 		dev_err(sdev->dev, "error: ICCMAX stream cleanup failed\n");
 
@@ -408,7 +432,6 @@  int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	const struct sof_intel_dsp_desc *chip_info;
 	struct hdac_ext_stream *hext_stream;
 	struct firmware stripped_firmware;
-	struct snd_dma_buffer dmab;
 	int ret, ret1, i;
 
 	if (hda->imrboot_supported && !sdev->first_boot && !hda->skip_imr_boot) {
@@ -432,23 +455,31 @@  int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 		return -EINVAL;
 	}
 
-	stripped_firmware.data = sdev->basefw.fw->data + sdev->basefw.payload_offset;
-	stripped_firmware.size = sdev->basefw.fw->size - sdev->basefw.payload_offset;
-
 	/* init for booting wait */
 	init_waitqueue_head(&sdev->boot_wait);
 
 	/* prepare DMA for code loader stream */
+	stripped_firmware.size = sdev->basefw.fw->size - sdev->basefw.payload_offset;
 	hext_stream = hda_cl_prepare(sdev->dev, HDA_CL_STREAM_FORMAT,
 				     stripped_firmware.size,
-				     &dmab, SNDRV_PCM_STREAM_PLAYBACK, false);
+				     &hda->cl_dmab, persistent_cl_buffer,
+				     SNDRV_PCM_STREAM_PLAYBACK, false);
 	if (IS_ERR(hext_stream)) {
 		dev_err(sdev->dev, "error: dma prepare for fw loading failed\n");
 		return PTR_ERR(hext_stream);
 	}
 
-	memcpy(dmab.area, stripped_firmware.data,
-	       stripped_firmware.size);
+	/*
+	 * Copy the payload to the DMA buffer if it is temporary or if the
+	 * buffer is  persistent but it does not have the basefw payload either
+	 * because this is the first boot and the buffer needs to be initialized,
+	 * or a library got loaded and it replaced the basefw.
+	 */
+	if (!persistent_cl_buffer || !hda->cl_dmab_contains_basefw) {
+		stripped_firmware.data = sdev->basefw.fw->data + sdev->basefw.payload_offset;
+		memcpy(hda->cl_dmab.area, stripped_firmware.data, stripped_firmware.size);
+		hda->cl_dmab_contains_basefw = true;
+	}
 
 	/* try ROM init a few times before giving up */
 	for (i = 0; i < HDA_FW_BOOT_ATTEMPTS; i++) {
@@ -514,7 +545,8 @@  int hda_dsp_cl_boot_firmware(struct snd_sof_dev *sdev)
 	 * This should be done even if firmware loading fails.
 	 * If the cleanup also fails, we return the initial error
 	 */
-	ret1 = hda_cl_cleanup(sdev->dev, &dmab, hext_stream);
+	ret1 = hda_cl_cleanup(sdev->dev, &hda->cl_dmab,
+			      persistent_cl_buffer, hext_stream);
 	if (ret1 < 0) {
 		dev_err(sdev->dev, "error: Code loader DSP cleanup failed\n");
 
@@ -545,7 +577,6 @@  int hda_dsp_ipc4_load_library(struct snd_sof_dev *sdev,
 	struct hdac_ext_stream *hext_stream;
 	struct firmware stripped_firmware;
 	struct sof_ipc4_msg msg = {};
-	struct snd_dma_buffer dmab;
 	int ret, ret1;
 
 	/* if IMR booting is enabled and fw context is saved for D3 state, skip the loading */
@@ -556,16 +587,28 @@  int hda_dsp_ipc4_load_library(struct snd_sof_dev *sdev,
 	stripped_firmware.data = fw_lib->sof_fw.fw->data + fw_lib->sof_fw.payload_offset;
 	stripped_firmware.size = fw_lib->sof_fw.fw->size - fw_lib->sof_fw.payload_offset;
 
+	/*
+	 * force re-allocation of the cl_dmab if the preserved DMA buffer is
+	 * smaller than what is needed for the library
+	 */
+	if (persistent_cl_buffer && stripped_firmware.size > hda->cl_dmab.bytes) {
+		snd_dma_free_pages(&hda->cl_dmab);
+		hda->cl_dmab.area = NULL;
+		hda->cl_dmab.bytes = 0;
+	}
+
 	/* prepare DMA for code loader stream */
 	hext_stream = hda_cl_prepare(sdev->dev, HDA_CL_STREAM_FORMAT,
 				     stripped_firmware.size,
-				     &dmab, SNDRV_PCM_STREAM_PLAYBACK, false);
+				     &hda->cl_dmab, persistent_cl_buffer,
+				     SNDRV_PCM_STREAM_PLAYBACK, false);
 	if (IS_ERR(hext_stream)) {
 		dev_err(sdev->dev, "%s: DMA prepare failed\n", __func__);
 		return PTR_ERR(hext_stream);
 	}
 
-	memcpy(dmab.area, stripped_firmware.data, stripped_firmware.size);
+	memcpy(hda->cl_dmab.area, stripped_firmware.data, stripped_firmware.size);
+	hda->cl_dmab_contains_basefw = false;
 
 	/*
 	 * 1st stage: SOF_IPC4_GLB_LOAD_LIBRARY_PREPARE
@@ -628,7 +671,8 @@  int hda_dsp_ipc4_load_library(struct snd_sof_dev *sdev,
 
 cleanup:
 	/* clean up even in case of error and return the first error */
-	ret1 = hda_cl_cleanup(sdev->dev, &dmab, hext_stream);
+	ret1 = hda_cl_cleanup(sdev->dev, &hda->cl_dmab, persistent_cl_buffer,
+			      hext_stream);
 	if (ret1 < 0) {
 		dev_err(sdev->dev, "%s: Code loader DSP cleanup failed\n", __func__);
 
diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 38921c0db84e..01b135068b1f 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -936,6 +936,12 @@  void hda_dsp_remove(struct snd_sof_dev *sdev)
 	/* disable DSP */
 	hda_dsp_ctrl_ppcap_enable(sdev, false);
 
+	/* Free the persistent DMA buffers used for base firmware download */
+	if (hda->cl_dmab.area)
+		snd_dma_free_pages(&hda->cl_dmab);
+	if (hda->iccmax_dmab.area)
+		snd_dma_free_pages(&hda->iccmax_dmab);
+
 skip_disable_dsp:
 	free_irq(sdev->ipc_irq, sdev);
 	if (sdev->msi_enabled)
diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
index b74a472435b5..22bd9c3c8216 100644
--- a/sound/soc/sof/intel/hda.h
+++ b/sound/soc/sof/intel/hda.h
@@ -495,6 +495,15 @@  struct sof_intel_hda_dev {
 
 	int boot_iteration;
 
+	/*
+	 * DMA buffers for base firmware download. By default the buffers are
+	 * allocated once and kept through the lifetime of the driver.
+	 * See module parameter: persistent_cl_buffer
+	 */
+	struct snd_dma_buffer cl_dmab;
+	bool cl_dmab_contains_basefw;
+	struct snd_dma_buffer iccmax_dmab;
+
 	struct hda_bus hbus;
 
 	/* hw config */
@@ -714,11 +723,12 @@  int hda_cl_copy_fw(struct snd_sof_dev *sdev, struct hdac_ext_stream *hext_stream
 
 struct hdac_ext_stream *hda_cl_prepare(struct device *dev, unsigned int format,
 				       unsigned int size, struct snd_dma_buffer *dmab,
-				       int direction, bool is_iccmax);
+				       bool persistent_buffer, int direction,
+				       bool is_iccmax);
 int hda_cl_trigger(struct device *dev, struct hdac_ext_stream *hext_stream, int cmd);
 
 int hda_cl_cleanup(struct device *dev, struct snd_dma_buffer *dmab,
-		   struct hdac_ext_stream *hext_stream);
+		   bool persistent_buffer, struct hdac_ext_stream *hext_stream);
 int cl_dsp_init(struct snd_sof_dev *sdev, int stream_tag, bool imr_boot);
 #define HDA_CL_STREAM_FORMAT 0x40