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

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

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 10 Jan '19

10 Jan '19

[consolidated answers from Liam and me] >> +/* wait for IPC message reply */ >> +static int tx_wait_done(struct snd_sof_ipc *ipc, struct snd_sof_ipc_msg *msg, >> + void *reply_data) >> +{ > This has exactly one caller, why not inline it (or make both tx and rx > separate functions)? we'll look into it. > >> + spin_lock_irqsave(&sdev->ipc_lock, flags); >> + >> + /* get an empty message */ >> + msg = msg_get_empty(ipc); >> + if (!msg) { >> + spin_unlock_irqrestore(&sdev->ipc_lock, flags); >> + return -EBUSY; >> + } >> + >> + msg->header = header; >> + msg->msg_size = msg_bytes; >> + msg->reply_size = reply_bytes; >> + msg->ipc_complete = false; >> + >> + /* 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. > >> + /* 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. > >> + >> + 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. > >> + /* send first message in TX list */ >> + msg = list_first_entry(&ipc->tx_list, struct snd_sof_ipc_msg, list); >> + list_move(&msg->list, &ipc->reply_list); >> + snd_sof_dsp_send_msg(sdev, msg); > Should the move happen if the send fails (I see it can return an error > code, though we ignore it)? so far neither the HDaudio nor the BYT stuff return an error on send_msg, so we'll need to thing about what happens should this happen with other hardware, or demote the status to void. We'll look into this. > >> + int err = -EINVAL; >> + case SOF_IPC_FW_READY: >> + /* check for FW boot completion */ >> + if (!sdev->boot_complete) { >> + if (sdev->ops->fw_ready) >> + err = sdev->ops->fw_ready(sdev, cmd); >> + if (err < 0) { >> + dev_err(sdev->dev, "error: DSP firmware boot timeout %d\n", >> + err); > err defaults to -EINVAL here which doesn't seem like the right thing if > fw_ready() is really optional. Perhaps just validate the ops on > registration and call this unconditionally? Good catch, err should default to 0, thanks for pointing this out. > >> +void snd_sof_ipc_free(struct snd_sof_dev *sdev) >> +{ >> + cancel_work_sync(&sdev->ipc->tx_kwork); >> + cancel_work_sync(&sdev->ipc->rx_kwork); >> +} >> +EXPORT_SYMBOL(snd_sof_ipc_free); > It might be better to have something that stops any new messages being > processed as well, to prevent races on removal. ok, we will add something to stop the IPC, good suggestion indeed.

1 0

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

09 Jan '19

On 1/9/19 2:44 PM, Mark Brown wrote: > On Tue, Dec 11, 2018 at 03:23:11PM -0600, Pierre-Louis Bossart wrote: > >> + /* 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.

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 08/14] ASoC: SOF: Add DSP HW abstraction operations
by Pierre-Louis Bossart 09 Jan '19

09 Jan '19

On 1/9/19 2:51 PM, Mark Brown wrote: > On Tue, Dec 11, 2018 at 03:23:12PM -0600, Pierre-Louis Bossart wrote: > >> +int snd_sof_pci_update_bits_unlocked(struct snd_sof_dev *sdev, u32 offset, >> + u32 mask, u32 value) >> +{ >> + bool change; >> + unsigned int old, new; >> + u32 ret = ~0; /* explicit init to remove uninitialized use warnings */ > This looks a lot like you want to write regmap_pci_config... I think you made that note for the v2 a long time ago, not sure if I replied at the time. We only use this for 4 cases (power/clock gating on/off, traffic class, etc) and only during the hardware initialization. This is similar to the legacy and Skylake driver, I don't see a significant benefit with a regmap? intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_MISCBDCGE_MASK, val); intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_CGCTL, PCI_CGCTL_ADSPDCGE, val); intel/hda-ctrl.c: snd_sof_pci_update_bits(sdev, PCI_PGCTL, PCI_PGCTL_ADSPPGD, val); intel/hda-dsp.c: snd_sof_pci_update_bits(sdev, PCI_PGCTL, intel/hda-dsp.c: snd_sof_pci_update_bits(sdev, PCI_TCSEL, 0x07, 0); > >> +/* control */ >> +static inline int snd_sof_dsp_run(struct snd_sof_dev *sdev) >> +{ >> + if (sdev->ops->run) >> + return sdev->ops->run(sdev); >> + >> + return 0; >> +} > Do we really want to return 0 for all these ops if they're not > implemented? For some that seems sensible but there's others where it > seems like the caller might want to know they got ignored and an error > code like -ENOTSUPP might be better. Good point indeed. There are a set of ops that are really mandatory, we should rework this instead. Thanks for the comment.

1 0

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

09 Jan '19

Thanks for the reviews Mark, much appreciated. +int snd_sof_load_firmware_memcpy(struct snd_sof_dev *sdev) >> +{ >> + struct snd_sof_pdata *plat_data = dev_get_platdata(sdev->dev); >> + const char *fw_filename; >> + int ret; > This never actually calls the load_firmware() operation for the DSP > AFAICT? it's actually the implementation of the load_firmware callback, see e.g. for Baytrail/Broadwell /*Firmware loading */ .load_firmware = snd_sof_load_firmware_memcpy, > >> + ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: request firmware failed err: %d\n", >> + ret); >> + return ret; >> + } > I'd suggest logging the name of the firmware we tried to load, users > will thank you. yes indeed, thanks for the suggestion, will add this right away. > >> + /* create fw_version debugfs to store boot version info */ >> + if (sdev->first_boot) { >> + ret = snd_sof_debugfs_buf_create_item(sdev, &sdev->fw_version, >> + sizeof(sdev->fw_version), >> + "fw_version"); >> + >> + if (ret < 0) { >> + dev_err(sdev->dev, "error: cannot create debugfs for fw_version\n"); >> + return ret; >> + } >> + } > As Andy said elsewhere debugfs stuff like that probably shouldn't be > fatal. yes, we've fixed this already.

1 0