January 2019 - Sound-open-firmware - mailman.alsa-project.org

[Sound-open-firmware] [PATCH v3 00/14] Sound Open Firmware (SOF) core
by Pierre-Louis Bossart 30 Jan '19

30 Jan '19

Sound Open Firmware (SOF) is a host and DSP architecture agnostic audio DSP firmware. SOF is not tied to any specific host architecture or any specific physical IO communication type (it will work with on SoC DSPs, or DSP connected via SPI/I2C). SOF is also not coupled to any particular DSP architecture and has abstraction similar to Linux to allow porting to other DSP architectures. https://www.sofproject.org/ This patch series introduces the SOF core and utilities. Support for Intel devices is provided as a follow-up series. The SOF core manages all the core DSP services and ALSA/ASoC IO. The core is responsible for loading firmware, parsing topology, exposing PCMs and kcontrols, providing debug and trace mechanisms and performing IPC between host and DSP. The SOF core also has logic to allow reuse of existing machine drivers for other platforms/machines without any code modification. i.e. DAI links can be modified at runtime to bind with SOF and SOF topologies instead of existing hard coded DAI links and topology. Main changes since v2 (end of August): There are honestly too many changes to be listed... The biggest changes are the introduction of an ABI check and runtime_pm, as well as countless improvements coming from product validation. We will provide a revision history starting with the next update. Precisions: Since I've been maintaining the SOF kernel parts for most of 2018, Liam and I mutually agreed that it would be more efficient for me to send the patches upstream to avoid any disconnects. The code in this patchset is directly squashed from the SOF development branch [1], which tracks Mark Brown's for-next branch on a weekly basis and the configurations used for testing are based on the defconfigs at [2] Full disclosure on known limitation and issues (full list at [3]) a) the interaction with ASoC is not always perfect, there are a couple of points where a better solution is desired (use of private data, support for link DMA, etc). The known points are explicitly documented in the code and will be updated. To be clearer, it's our belief that SOF does not cripple ASoC in any way, and that merging this SOF core does not harm others. It's just complicated to get things right, especially on a couple of cases where the topology and DPCM frameworks generate complex flows that very few people understand. b) The get/put methods for controls generate an IPC and possibly a wake-up. This is not optimal and is being fixed. c) The debugfs parts will be updated for runtime_pm parts d) The topology free, firmware release and Hdaudio exit sequences will be updated. It is our intention to fully support module load/unload and free all resources. e) runtime_pm is being hardened and the use of SMART_SUSPEND was suggested. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre [1] https://github.com/thesofproject/linux [2] https://github.com/thesofproject/kconfig [3] https://github.com/thesofproject/linux/issues Liam Girdwood (12): ASoC: SOF: Add Sound Open Firmware driver core ASoC: SOF: Add Sound Open Firmware KControl support ASoC: SOF: Add driver debug support. ASoC: SOF: Add support for IPC IO between DSP and Host ASoC: SOF: Add PCM operations support ASoC: SOF: Add support for loading topologies ASoC: SOF: Add DSP firmware logger support ASoC: SOF: Add DSP HW abstraction operations ASoC: SOF: Add firmware loader support ASoC: SOF: Add userspace ABI support ASoC: SOF: Add PM support ASoC: SOF: Add Nocodec machine driver support Pierre-Louis Bossart (2): ASoC: SOF: Add xtensa support ASoC: SOF: Add utils include/sound/soc.h | 3 + include/sound/sof.h | 91 + include/sound/sof/control.h | 125 ++ include/sound/sof/dai-intel.h | 175 ++ include/sound/sof/dai.h | 75 + include/sound/sof/header.h | 158 ++ include/sound/sof/info.h | 118 ++ include/sound/sof/pm.h | 48 + include/sound/sof/stream.h | 149 ++ include/sound/sof/topology.h | 256 +++ include/sound/sof/trace.h | 66 + include/sound/sof/xtensa.h | 44 + include/uapi/sound/sof/abi.h | 63 + include/uapi/sound/sof/eq.h | 164 ++ include/uapi/sound/sof/fw.h | 67 + include/uapi/sound/sof/header.h | 27 + include/uapi/sound/sof/manifest.h | 188 ++ include/uapi/sound/sof/tokens.h | 95 + include/uapi/sound/sof/tone.h | 21 + include/uapi/sound/sof/trace.h | 93 + sound/soc/sof/control.c | 421 +++++ sound/soc/sof/core.c | 398 +++++ sound/soc/sof/debug.c | 176 ++ sound/soc/sof/ipc.c | 813 +++++++++ sound/soc/sof/loader.c | 322 ++++ sound/soc/sof/nocodec.c | 81 + sound/soc/sof/ops.c | 200 +++ sound/soc/sof/ops.h | 379 ++++ sound/soc/sof/pcm.c | 755 ++++++++ sound/soc/sof/pm.c | 403 +++++ sound/soc/sof/sof-priv.h | 574 ++++++ sound/soc/sof/topology.c | 2775 +++++++++++++++++++++++++++++ sound/soc/sof/trace.c | 295 +++ sound/soc/sof/utils.c | 173 ++ sound/soc/sof/xtensa/Kconfig | 3 + sound/soc/sof/xtensa/Makefile | 5 + sound/soc/sof/xtensa/core.c | 152 ++ 37 files changed, 9951 insertions(+) create mode 100644 include/sound/sof.h create mode 100644 include/sound/sof/control.h create mode 100644 include/sound/sof/dai-intel.h create mode 100644 include/sound/sof/dai.h create mode 100644 include/sound/sof/header.h create mode 100644 include/sound/sof/info.h create mode 100644 include/sound/sof/pm.h create mode 100644 include/sound/sof/stream.h create mode 100644 include/sound/sof/topology.h create mode 100644 include/sound/sof/trace.h create mode 100644 include/sound/sof/xtensa.h create mode 100644 include/uapi/sound/sof/abi.h create mode 100644 include/uapi/sound/sof/eq.h create mode 100644 include/uapi/sound/sof/fw.h create mode 100644 include/uapi/sound/sof/header.h create mode 100644 include/uapi/sound/sof/manifest.h create mode 100644 include/uapi/sound/sof/tokens.h create mode 100644 include/uapi/sound/sof/tone.h create mode 100644 include/uapi/sound/sof/trace.h create mode 100644 sound/soc/sof/control.c create mode 100644 sound/soc/sof/core.c create mode 100644 sound/soc/sof/debug.c create mode 100644 sound/soc/sof/ipc.c create mode 100644 sound/soc/sof/loader.c create mode 100644 sound/soc/sof/nocodec.c create mode 100644 sound/soc/sof/ops.c create mode 100644 sound/soc/sof/ops.h create mode 100644 sound/soc/sof/pcm.c create mode 100644 sound/soc/sof/pm.c create mode 100644 sound/soc/sof/sof-priv.h create mode 100644 sound/soc/sof/topology.c create mode 100644 sound/soc/sof/trace.c create mode 100644 sound/soc/sof/utils.c create mode 100644 sound/soc/sof/xtensa/Kconfig create mode 100644 sound/soc/sof/xtensa/Makefile create mode 100644 sound/soc/sof/xtensa/core.c -- 2.17.1

5 47

[Sound-open-firmware] sound-next or linux-next sof patches
by Daniel Baluta 28 Jan '19

28 Jan '19

Hi, What's the closest SOF branch to linux-next or sound/for-next? Our i.MX8 boards support was only recently added in various trees and it is currently only in linux-next. thanks, Daniel.

2 2

[Sound-open-firmware] Timer implementation: Haswell vs Baytrail
by Daniel Baluta 28 Jan '19

28 Jan '19

Hi all, Can someone explain what is the difference between hsw vs byt timer implementation? It looks like hsw uses a generic xtensa timer implementation based on directly reading Xtensa special registers while Byt uses some bits in SHIM layer. What is the advantage of byt implementation? Also, the implementation for hsw looks generic enough so that it can be used by other architectures that do not have a shim layer, right? thanks, Daniel

2 1

[Sound-open-firmware] SOF: Where is the purpose of host_msg ?
by Daniel Baluta 23 Jan '19

23 Jan '19

Hi Pierre/Liam, In src/include/sof/ipc.h there is: struct ipc { uint32_t host_msg;» » /* current message from host */ // } This keeps the message received from IA core. Anyhow, DSP core doesn't seem to use it at all. Except maybe for some logging: src/drivers/intel/baytrail/ipc.c: _ipc->host_msg = msg; src/drivers/intel/baytrail/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/drivers/intel/cavs/ipc.c: _ipc->host_msg = msg; src/drivers/intel/cavs/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/drivers/intel/cavs/sue-ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/drivers/intel/haswell/ipc.c: _ipc->host_msg = msg; src/drivers/intel/haswell/ipc.c: trace_ipc("ipc: msg rx -> 0x%x", ipc->host_msg); src/include/sof/ipc.h: uint32_t host_msg; /* current message from host */ I am asking this because initially I thought the communication between IA core and DSP involves: - sending cmd using shim layer - sending actual data using mailbox. But it seems that all the information is exchanged via the mailbox memory. So, going back to my question will host_msg be used in the future? If not, I think it is best to remove it because it is confusing. The same happens when DSP tries to send something to IA. For example it writes msg->header. But this is never used on the other side. ipc_platform_send_msg » /* now interrupt host to tell it we have message sent */ » shim_write(SHIM_IPCDL, msg->header); » shim_write(SHIM_IPCDH, SHIM_IPCDH_BUSY); Also, would be easier for me on the ARM side to only send info via mailbox and notification via what you call shim layer. thanks, Daniel.

3 7

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host
by Pierre-Louis Bossart 23 Jan '19

23 Jan '19

On 1/22/19 1:04 PM, Mark Brown wrote: > On Thu, Jan 10, 2019 at 02:11:32PM -0600, Pierre-Louis Bossart wrote: > >>>> + /* attach any data */ >>>> + if (msg_bytes) >>>> + memcpy(msg->msg_data, msg_data, msg_bytes); >>> How big do these messages get? Do we need to hold the lock while we >>> memcpy()? >> Messages can be as big as the mailbox, which is hardware dependent. It could >> be from a few bytes to a larger e.g. 4k page or more, and indeed we need to >> keep the lock. > Is this copying into an actual mailbox or into some data structure in > memory? It looked like it's copying into a buffer for sending rather > than the mailbox. I realize between your feedback and Daniel's that we have a terminology issue. On the Intel side we don't have a dedicated hardware mailbox unit, it's only doorbell registers and shared memory windows. > >>>> + /* schedule the message if not busy */ >>>> + if (snd_sof_dsp_is_ready(sdev)) >>>> + schedule_work(&ipc->tx_kwork); >>> If the DSP is idle is there a reason this has to happen in another >>> thread? >> we will rename this as snd_sof_dsp_is_ipc_ready() to avoid any confusion >> with the DSP state. We only care about IPC registers/doorbells at this >> point, not the fact that the DSP is in its idle loop. > You're missing the point - why can't we just immediately send the > message to the DSP from here, what's the benefit of scheduling some work > to do that? I realize this might be Intel-specific and will likely have to evolve. Keyon/Liam, this is your code, can you comment on Mark's comments (here) > >>>> + spin_unlock_irqrestore(&sdev->ipc_lock, flags); >>> The thread is also going to take an irq spinlock after all. >> didn't get this point, sorry. > One reason to bounce to another thread would be to do something that you > can't do in this context like take a lock in a place you can't take > locks but here we're taking a lock that needs thread context already. Liam/Keyon comment needed here as well. I think there are multiple questions that should be better answered to 1. why do we schedule a thread when the DSP is not busy 2. what happens if it is? 3. can we avoid freeing the lock to take it again when we schedule?

2 1

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

> It's got less and less useful as time goes on I think :( For this one > did you try reporting the bug to the authors? Nope, mostly because I don't even know who the contributors are these days. Will report this when I get a chance (trying to submit another SOF update before Chinese New Year).

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

>>> Why was sparse complaining - was it spotting something that is an actual >>> issue here and you've just masked the warning? >> It was about the use of typeof/sizeof() in min(), not an actual issue in the >> code >> sound/soc/sof/trace.c:90:17: warning: expression using sizeof(void) >> sound/soc/sof/trace.c:90:17: warning: expression using sizeof(void) > I don't understand that warning - avail and count are both size_t so > where's it deciding that there's a void involved? no idea really, I am only a user of sparse, I don't know how it generated that warning. there are equally confusing ones such as include/linux/slab.h:332:43: warning: dubious: x & !y include/linux/slab.h:665:13: warning: call with no type! include/linux/slab.h:665:13: error: undefined identifier '__builtin_mul_overflow The main benefit of sparse is that it is the only tool that found endianess and iomem space issues, but it's often buried in heaps of false alarms in include files.

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

>>>> + /* make sure count is <= avail */ >>>> + count = avail > count ? count : avail; >>> min()? >> I tried to use min() but then Sparse started complaining so went back to an >> explicit test+assign. >> I don't have a strong opinion on this, but we've found so many improvements >> with the tools that I tend to favor warning-free code. > Why was sparse complaining - was it spotting something that is an actual > issue here and you've just masked the warning? It was about the use of typeof/sizeof() in min(), not an actual issue in the code sound/soc/sof/trace.c:90:17: warning: expression using sizeof(void) sound/soc/sof/trace.c:90:17: warning: expression using sizeof(void)

1 0

[Sound-open-firmware] [PATCH 00/21] Sound Open Firmware (SOF) - Intel support
by Pierre-Louis Bossart 22 Jan '19

22 Jan '19

This patch series introduces the support for Intel devices (Baytrail, CherryTrail, Broadwell/Haswell, ApolloLake, GeminiLake, CannonLake, IceLake). Support for Skylake and KabyLake is incomplete at the moment but the basic building blocks required are already present. The series starts with the 'legacy' devices, then all the more recent ones and concludes with ACPI/PCI hooks. SKL+ platforms can be compiled without HDaudio link/codec support, in which case the hdac library is not used. When support for HDMI or HDaudio codecs is selected, parts of this library are used. With Takashi's latest patches, it is understood some fixes will be needed before merge, they will be provided as an update when merged by Mark. Patch 19/21 ('ALSA-HDA-export-process_unsol_events') was submitted as a dependency last week but not yet merged. It is provided for completeness should anyone try to compile the code. The machine drivers supported by SOF are not provided as part of this series, mainly because of likely conflicts with other patches in flight to support HDaudio fallback and additional work required for upstream. The build will only work in 'nocodec' mode for now. Support for SPI-based devices, compressed streams is also not provided for now since it's not ready for upstream just yet. Thank you for reviews and comments, we appreciate the time spent commenting on this large patchset. Thanks in particular to Alan Cox and Andy Shevchenko for their comments on an earlier version. Pierre Keyon Jie (3): ASoC: SOF: Intel: Add hda-bus support and initialization ASoC: SOF: Intel: SKL, CNL, APL platform DAIs ALSA: HDA: export process_unsol_events() Liam Girdwood (15): ASoC: SOF: Intel: Add BYT, CHT and BSW DSP HW support. ASoC: SOF: Intel: Add HSW HW DSP support ASoC: SOF: Intel: Add support for BDW HW DSP support ASoC: SOF: Intel: Add APL/CNL HW DSP support ASoC: SOF: Intel: Add HDA controller for Intel DSP ASoC: SOF: Intel: Add Intel specific HDA DSP HW operations ASoC: SOF: Intel: Add Intel specific HDA IPC mechanisms. ASoC: SOF: Intel: Add Intel specific HDA firmware loader ASoC: SOF: Intel: Add Intel specific HDA PCM operations ASoC: SOF: Intel: Add Intel specific HDA stream operations ASoC: SOF: Intel: Add Intel specific HDA trace operations ASoC: SOF: Intel: Add platform differentiation for SKL, APL and CNL ASoC: SOF: Add ACPI device support ASoC: SOF: Add PCI device support ASoC: SOF: Add Build support for SOF core and Intel drivers Pierre-Louis Bossart (3): ASoC: SOF: Intel: Add support for HDAudio codecs ASoC: SOF: Intel: Add SKL-specific code loader ASoC: Intel: Kconfig: expose common option between SST and SOF drivers include/sound/hdaudio.h | 1 + sound/hda/hdac_bus.c | 7 +- sound/soc/Kconfig | 1 + sound/soc/Makefile | 1 + sound/soc/intel/Kconfig | 7 +- sound/soc/sof/Kconfig | 82 +++ sound/soc/sof/Makefile | 17 + sound/soc/sof/intel/Kconfig | 136 +++++ sound/soc/sof/intel/Makefile | 17 + sound/soc/sof/intel/apl.c | 99 ++++ sound/soc/sof/intel/bdw.c | 700 +++++++++++++++++++++++ sound/soc/sof/intel/byt.c | 805 +++++++++++++++++++++++++++ sound/soc/sof/intel/cnl.c | 242 ++++++++ sound/soc/sof/intel/hda-bus.c | 107 ++++ sound/soc/sof/intel/hda-codec.c | 185 ++++++ sound/soc/sof/intel/hda-ctrl.c | 166 ++++++ sound/soc/sof/intel/hda-dai.c | 368 ++++++++++++ sound/soc/sof/intel/hda-dsp.c | 417 ++++++++++++++ sound/soc/sof/intel/hda-ipc.c | 387 +++++++++++++ sound/soc/sof/intel/hda-loader-skl.c | 531 ++++++++++++++++++ sound/soc/sof/intel/hda-loader.c | 369 ++++++++++++ sound/soc/sof/intel/hda-pcm.c | 232 ++++++++ sound/soc/sof/intel/hda-stream.c | 661 ++++++++++++++++++++++ sound/soc/sof/intel/hda-trace.c | 79 +++ sound/soc/sof/intel/hda.c | 704 +++++++++++++++++++++++ sound/soc/sof/intel/hda.h | 537 ++++++++++++++++++ sound/soc/sof/intel/hsw.c | 701 +++++++++++++++++++++++ sound/soc/sof/intel/shim.h | 159 ++++++ sound/soc/sof/intel/skl.c | 92 +++ sound/soc/sof/sof-acpi-dev.c | 269 +++++++++ sound/soc/sof/sof-pci-dev.c | 361 ++++++++++++ 31 files changed, 8435 insertions(+), 5 deletions(-) create mode 100644 sound/soc/sof/Kconfig create mode 100644 sound/soc/sof/Makefile create mode 100644 sound/soc/sof/intel/Kconfig create mode 100644 sound/soc/sof/intel/Makefile create mode 100644 sound/soc/sof/intel/apl.c create mode 100644 sound/soc/sof/intel/bdw.c create mode 100644 sound/soc/sof/intel/byt.c create mode 100644 sound/soc/sof/intel/cnl.c create mode 100644 sound/soc/sof/intel/hda-bus.c create mode 100644 sound/soc/sof/intel/hda-codec.c create mode 100644 sound/soc/sof/intel/hda-ctrl.c create mode 100644 sound/soc/sof/intel/hda-dai.c create mode 100644 sound/soc/sof/intel/hda-dsp.c create mode 100644 sound/soc/sof/intel/hda-ipc.c create mode 100644 sound/soc/sof/intel/hda-loader-skl.c create mode 100644 sound/soc/sof/intel/hda-loader.c create mode 100644 sound/soc/sof/intel/hda-pcm.c create mode 100644 sound/soc/sof/intel/hda-stream.c create mode 100644 sound/soc/sof/intel/hda-trace.c create mode 100644 sound/soc/sof/intel/hda.c create mode 100644 sound/soc/sof/intel/hda.h create mode 100644 sound/soc/sof/intel/hsw.c create mode 100644 sound/soc/sof/intel/shim.h create mode 100644 sound/soc/sof/intel/skl.c create mode 100644 sound/soc/sof/sof-acpi-dev.c create mode 100644 sound/soc/sof/sof-pci-dev.c -- 2.17.1

3 32

Re: [Sound-open-firmware] [PATCH v3 03/14] ASoC: SOF: Add driver debug support.
by Pierre-Louis Bossart 10 Jan '19

10 Jan '19

[combined feedback from Ranjani, Liam and me] >> + /* copy from DSP MMIO */ >> + pm_runtime_get_noresume(sdev->dev); > Why are we doing a _get_noresume() here? It won't actually do anything > to the device... the reason we currently do a get_noresume() is so that we can still read the debugfs entries in case of DSP panic, where a regular resume would not succeed. That said this part will be reworked based on feedback from Rafael whose suggestion was to get rid of the pm calls while reading debugfs. We can make sure that all the values are flushed out of the dsp before suspending which will make the need to wake it up redundant. > >> + >> + memcpy_fromio(buf, dfse->buf + pos, size); > Extra space? yep. we run checkpatch.pl --strict all the time and missed this somehow. > >> + /* >> + * TODO: revisit to check if we need mark_last_busy, or if we >> + * should change to use xxx_put_sync[_suspend](). >> + */ >> + ret = pm_runtime_put_sync_autosuspend(sdev->dev); >> + if (ret < 0) >> + dev_warn(sdev->dev, "warn: debugFS failed to autosuspend %zd\n", >> + ret); > It rather depends what you're doing... I'm definitely confused as to > why you need a _sync operation - if you're doing autosuspend stuff > presumably you don't care so much if the device gets powered down > immediately, and I can't in general see why that'd be important. yes, as stated above we'll rework all this > >> + dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root, >> + dfse, &sof_dfs_fops); >> + if (!dfse->dfsentry) { >> + dev_err(sdev->dev, "error: cannot create debugfs entry.\n"); >> + return -ENODEV; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_debugfs_io_create_item); > debugfs uses EXPORT_SYMBOL_GPL(). We don't have a burning desire to keep this as EXPORT_SYMBOL since debugfs is really Linux specific, so will move all these statements as EXPORT_SYMBOL_GPL. Thanks Mark for your time, much appreciated. We've already dealt with most of the feedback from Takashi/Andy/Daniel so should have a new version of these patches in 1-2 weeks.

1 0