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.

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

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

Open
wants to merge 1 commit into
base: topic/sof-dev
Choose a base branch
from

Conversation

ujfalusi
Copy link
Collaborator

@ujfalusi ujfalusi commented Sep 18, 2024

Hi,

every now and then we see the DMA buffer allocation failure coming back [1]. While the issue is not originating from SOF stack, it does pops up from the logs.

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.

[1] #5161

sound/soc/sof/intel/hda-loader.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-loader.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-loader.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda-loader.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda.c Outdated Show resolved Hide resolved
sound/soc/sof/intel/hda.h Outdated Show resolved Hide resolved
@ujfalusi ujfalusi force-pushed the peter/sof/pr/hda-cache-fw-dma-buffer-01 branch from b081c9d to 4e0def8 Compare September 19, 2024 06:35
@ujfalusi
Copy link
Collaborator Author

Changes since v1:

  • single patch
  • move the module parameter to hda-loader.c and use it directly
  • the firmware download DMA buffer is by default persistent
  • add explanatory comments

@ujfalusi ujfalusi force-pushed the peter/sof/pr/hda-cache-fw-dma-buffer-01 branch from 4e0def8 to 9c76e97 Compare September 19, 2024 09:58
@ujfalusi
Copy link
Collaborator Author

Changes since v2:

  • Use persistent_buffer as parameter names in functions

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 19, 2024

I very painfully found this error buried deep down in the Jenkins logs. See internal issue 607 for more details.

It's failing only the NOCODEC build, the regular build is fine.

Yet Jenkins thinks the build was fine, keeps going and that's why things get super confusing.

Considering Jenkins is basically orphaned, you want to add the NODEC build to Github Actions. It shouldn't be hard, I can review if you want.

  CC      net/ethtool/rss.o
  CC      drivers/pnp/support.o
  CC      drivers/acpi/acpica/rsdumpinfo.o
/srv/home/jenkins/workspace/linux_config_build/linux/sound/soc/sof/intel/hda.c: In function ‘hda_dsp_pre_fw_run’:
/srv/home/jenkins/workspace/linux_config_build/linux/sound/soc/sof/intel/hda.c:404:21: error: ‘hda_keep_fw_dma_buffer’ undeclared (first use in this function)
  404 |                 if (hda_keep_fw_dma_buffer == 0 || hda_keep_fw_dma_buffer == 1)
      |                     ^~~~~~~~~~~~~~~~~~~~~~
/srv/home/jenkins/workspace/linux_config_build/linux/sound/soc/sof/intel/hda.c:404:21: note: each undeclared identifier is reported only once for each function it appears in
make[10]: *** [/srv/home/jenkins/workspace/linux_config_build/linux/scripts/Makefile.build:244: sound/soc/sof/intel/hda.o] Error 1
make[9]: *** [/srv/home/jenkins/workspace/linux_config_build/linux/scripts/Makefile.build:485: sound/soc/sof/intel] Error 2
make[9]: *** Waiting for unfinished jobs....
  AR      drivers/amba/built-in.a
  CC      drivers/pnp/interface.o
  CC      drivers/acpi/acpica/rsinfo.o

@ujfalusi
Copy link
Collaborator Author

SOFCI TEST

@marc-hb
Copy link
Collaborator

marc-hb commented Sep 19, 2024

Considering Jenkins is basically orphaned, you want to add the NODEC build to Github Actions. It shouldn't be hard, I can review if you want.

BTW you should really use a matrix to avoid the massive copy/paste/diverge in .github/workflows/. Simple matrix example in: thesofproject/sof@425a5b5

@ujfalusi ujfalusi force-pushed the peter/sof/pr/hda-cache-fw-dma-buffer-01 branch from 9c76e97 to d340693 Compare September 19, 2024 18:43
@marc-hb
Copy link
Collaborator

marc-hb commented Sep 19, 2024

SOFCI TEST

@ujfalusi ujfalusi force-pushed the peter/sof/pr/hda-cache-fw-dma-buffer-01 branch from d340693 to 1eab284 Compare September 20, 2024 08:26
@ujfalusi ujfalusi changed the title ASoC: SOF: Intel: hda: Add support for keeping the DMA buffer used for basefw download ASoC: SOF: Intel: hda: Preserve DMA buffers used for base firmware download Sep 20, 2024
@ujfalusi
Copy link
Collaborator Author

Changes since v3:

  • Preserve the iccmax DMA buffer if it is used by the platform
  • With this update the whole base firmware handling is covered, IPC4 library DMA buffer caching will be added as a followup series as it needs to touch top-level IPC4 headers and structures - only Intel is using struct snd_dma_buffer for download afaik.

@ujfalusi ujfalusi force-pushed the peter/sof/pr/hda-cache-fw-dma-buffer-01 branch 2 times, most recently from 5d9fc9b to a7abc5d Compare September 20, 2024 17:38
…fers

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>
@ujfalusi
Copy link
Collaborator Author

Changes since v4:

  • cover library loading as well with persistent code loader buffer
  • single cl_dmab buffer to be used by basefw and library loading alike.

@ujfalusi ujfalusi changed the title ASoC: SOF: Intel: hda: Preserve DMA buffers used for base firmware download ASoC: SOF: Intel: hda: Add support for persistent Code Loader DMA buffers Sep 20, 2024
if (hda->cl_dmab.area)
snd_dma_free_pages(&hda->cl_dmab);
if (hda->iccmax_dmab.area)
snd_dma_free_pages(&hda->iccmax_dmab);
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 fully remember the iccmax details but would it be possible to use the same trick and use the same buffer as for regular firmware?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I rather not mix the two, while iccmax afaik is not running a capture, the buffer needs to be valid before we set up the code loader DMA.
I think it would result rather confusing code to follow what is going on.

/*
* DMA buffers for base firmware download. By default the buffers are
* allocated once and kept through the lifetime of the driver.
* See module parameter: keep_firmware_dma_buffer
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is this module parameter defined?

* 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 time and needs to be initialized or a
* library got loaded since the last basefw boot.
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment is not easy to follow. What are we trying to say here? Also, if I understand the patch correctly, we're trying to propose to only keep the code loader DMA buffer allocation be pesistent isnt it? Why not then use the temp dmab in the iccmax and the library cases so that we never use the same buffer in those cases?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants