January 2019 - 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 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

[Sound-open-firmware] sof_text_start address in baytrail.x.in
by Daniel Baluta 07 Jan '19

07 Jan '19

Hi, In src/platform/baytrail/baytrail.x.in we have: vector_double_text : org = XCHAL_DOUBLEEXC_VECTOR_PADDR, len = SOF_MEM_VECT_TEXT_SIZE sof_text_start : org = XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_SIZE, Is the address of sof_text_start correct here? vector_double_text ends at: XCHAL_DOUBLEEXC_VECTOR_PADDR + SOF_MEM_VECT_TEXT_SIZE but sof_text_start (which is the next segment in memory) doesn't start immediately after vector_double_text because SOF_MEM_VECT_SIZE = SOF_MEM_VECT_TEXT_SIZE + SOF_MEM_VECT_LIT_SIZE so there is a gap of SOF_MEM_VECT_LIT_SIZE (4 bytes) between vector_double_text and sof_text_start. Is this on purpose? thanks, Daniel.

2 1