Re: [PATCH 04/17] ASoC: Intel: avs: Inter process communication

On 2022-02-25 1:56 AM, Pierre-Louis Bossart wrote:

...

The boot process involving ROM-code requires specific handling with 'unstall' operations which are not required post-boot with normal IPC so separate set of send-message handlers is added for each of the usecases.

consider splitting this long sentence and use simpler logic. It's quite unclear how you went from boot to use cases.

Agree, ack.

...

+#include "messages.h"

avs_messages.h?

Same as previously, this header is for internal use only and is found within directory already named 'avs'. "avs.h" header covers a wider range of types and that's why its name is generic. All others are specific and thus are not prefixed with "avs_".

...

struct avs_dev;

@@ -18,6 +19,9 @@ struct avs_dsp_ops { int (* const power)(struct avs_dev *, u32, bool); int (* const reset)(struct avs_dev *, u32, bool); int (* const stall)(struct avs_dev *, u32, bool);

• irqreturn_t (* const irq_handler)(int, void *);
• irqreturn_t (* const irq_thread)(int, void *);
• void (* const int_control)(struct avs_dev *, bool);

kernel-doc or comments on what the last op might mean?

Sure, will add a comment.

...

};

#define avs_dsp_op(adev, op, ...) \ @@ -34,6 +38,18 @@ struct avs_spec {

const u32 core_init_mask; /* used during DSP boot */ const u64 attributes; /* bitmask of AVS_PLATATTR_* */

• const u32 sram_base_offset;
• const u32 sram_window_size;
• const u32 rom_status;
• const u32 hipc_req_offset;
• const u32 hipc_req_ext_offset;
• const u32 hipc_req_busy_mask;
• const u32 hipc_ack_offset;
• const u32 hipc_ack_done_mask;
• const u32 hipc_rsp_offset;
• const u32 hipc_rsp_busy_mask;
• const u32 hipc_ctl_offset;

is this really desirable to describe the IPC registers, when we know there were 3 generations of Intel IPC registers. this is ipc-1.5 only.

Indeed, this abstraction could be removed, ack.

...

};

struct avs_dev { @@ -42,6 +58,9 @@ struct avs_dev {

void __iomem *adsp_ba; const struct avs_spec *spec;

• struct avs_ipc *ipc;

• struct completion fw_ready; };

  /* from hda_bus to avs_dev */

@@ -61,4 +80,78 @@ int avs_dsp_core_stall(struct avs_dev *adev, u32 core_mask, bool stall); int avs_dsp_core_enable(struct avs_dev *adev, u32 core_mask); int avs_dsp_core_disable(struct avs_dev *adev, u32 core_mask);

+/* Inter Process Communication */

+struct avs_ipc_msg {

• union {

• u64 header;
  

• union avs_global_msg glb;
  

• union avs_reply_msg rsp;
  

• };
• void *data;
• size_t size;

+};

+struct avs_ipc {

• struct device *dev;
• struct avs_ipc_msg rx;
• u32 default_timeout_ms;
• bool ready;

ready for what? This should be described or documented.

In the past this field was called "fw_ready" until we have decided to split struct avs_ipc from struct avs_dev. In my opinion "ipc->ready" looks very intuitive in the code, given that it translates to: inter process communication ready!

No problem with adding a comment though.

...

• bool rx_completed;
• spinlock_t rx_lock;
• struct mutex msg_mutex;

checkpatch would tell you to add a comment for spinlock and mutex. it's quite unclear what they might describe and if they are related.

I'll add a kernel-doc for just like for the ->ready field.

...

• struct completion done_completion;
• struct completion busy_completion;

+};

+#define AVS_EIPC EREMOTEIO +/*

• • IPC handlers may return positive value (firmware error code) what denotes
• • successful HOST <-> DSP communication yet failure to process specific request.
• • Below macro converts returned value to linux kernel error code.
• • All IPC callers MUST use it as soon as firmware error code is consumed.
• */

+#define AVS_IPC_RET(ret) \

• (((ret) <= 0) ? (ret) : -AVS_EIPC)

why not use -EREMOTEIO directly? -AVS_EIPC is not very useful for the reader.

And why -EREMOTEIO? I see that you used it in catpt but that's a very surprising code that no one else uses in sound/

Well, the question: "Which kernel error should represent an error coming from remote process AKA audio firmware" needed an answer. EREMOTEIO fits the description and so it was chosen.

...

+static inline void avs_ipc_err(struct avs_dev *adev, struct avs_ipc_msg *tx,

• 	       const char *name, int error)
  

+{

• /*

• * If IPC channel is blocked e.g.: due to ongoing recovery,
  

• * -EPERM error code is expected and thus it's not an actual error.
  

• */
  

• if (error == -EPERM)

• dev_dbg(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,
  

• 	tx->glb.primary, tx->glb.ext.val, error);
  

• else

• dev_err(adev->dev, "%s 0x%08x 0x%08x failed: %d\n", name,
  

• 	tx->glb.primary, tx->glb.ext.val, error);
  

+}

we've used such functions before and the feedback, e.g. from GregKH and Mark Brown, has consistenly been that this is pushing the use of dev_dbg too far.

In basically all cases the outcome is going to be dev_err(). dev_dbg() is here to help keep DSP-recovery scenario viewer-friendly when checking dmesg. Ideally, there should be no DSP-recoveries to begin with : )

...

+#define AVS_IPC_TIMEOUT_MS 300

skl-sst-ipc.c:#define IPC_TIMEOUT_MSECS 3000

that's one order of magniture lower. please add a comment or align.

...

+static void avs_dsp_receive_rx(struct avs_dev *adev, u64 header) +{

• struct avs_ipc *ipc = adev->ipc;
• union avs_reply_msg msg = AVS_MSG(header);
• ipc->rx.header = header;
• if (!msg.status)

• memcpy_fromio(ipc->rx.data, avs_uplink_addr(adev),
  

• 	      ipc->rx.size);
  

it wouldn't hurt to describe that the status determines whether additional information can be read from a mailbox.

Isn't that consisted with the behaviour of typical API function? Do not copy memory and return it to the caller if something went wrong along the way?

...

+}

+static void avs_dsp_process_notification(struct avs_dev *adev, u64 header) +{

• struct avs_notify_mod_data mod_data;
• union avs_notify_msg msg = AVS_MSG(header);
• size_t data_size = 0;
• void *data = NULL;
• if (!adev->ipc->ready && msg.notify_msg_type != AVS_NOTIFY_FW_READY) {

• dev_dbg(adev->dev, "FW not ready, skip notification: 0x%08x\n",
  

• 	msg.primary);
  

can this happen?

you should add a comment on what could be sent before the first 'real' sign of life from the DSP.

it's also unclear why this dev_dbg() when 'unknown notifications' below are handled as dev_warn()

I would like to say: "no, this situation cannot happen" very much, but that's simply not true. Any notification could be sent prior to FW_READY as the internal queue may not always get flushed between the firmware restoring.

Ack on the s/warn/info/ part.

...

• return;
  

• }
• /* Calculate notification payload size. */
• switch (msg.notify_msg_type) {
• case AVS_NOTIFY_FW_READY:

• break;
  

• case AVS_NOTIFY_PHRASE_DETECTED:

• data_size = sizeof(struct avs_notify_voice_data);
  

• break;
  

• case AVS_NOTIFY_RESOURCE_EVENT:

• data_size = sizeof(struct avs_notify_res_data);
  

• break;
  

• case AVS_NOTIFY_MODULE_EVENT:

• memcpy_fromio(&mod_data, avs_uplink_addr(adev), sizeof(mod_data));
  

• data_size = sizeof(mod_data) + mod_data.data_size;
  

it wouldn't hurt to describe the layout behing this formula.

The layout is kind of implied by the structure itself but a comment wouldn't hurt, agree.

...

• break;
  

• default:

• dev_warn(adev->dev, "unknown notification: 0x%08x\n",
  

• 	 msg.primary);
  

• break;
  

• }
• if (data_size) {

• data = kmalloc(data_size, GFP_KERNEL);
  

• if (!data)
  

• 	return;
  

• memcpy_fromio(data, avs_uplink_addr(adev), data_size);
  

• }
• /* Perform notification-specific operations. */
• switch (msg.notify_msg_type) {
• case AVS_NOTIFY_FW_READY:

• dev_dbg(adev->dev, "FW READY 0x%08x\n", msg.primary);
  

• adev->ipc->ready = true;
  

avs->ipc->fw_ready?

As I have explained earlier, this was the case until we have separated struct avs_ipc from struct avs_dev. I'll provide a kernel-doc instead.

...

• complete(&adev->fw_ready);> +		break;
  

• default:

• break;
  

• }
• kfree(data);

+}

+void avs_dsp_process_response(struct avs_dev *adev, u64 header) +{

• struct avs_ipc *ipc = adev->ipc;
• if (avs_msg_is_reply(header)) {

the naming is confusing, it's difficult for me to understand that a 'response' could not be a 'reply'. The two terms are synonyms, aren't they?

Those two are not the same from the firmware's point of view and thus they are not the same here. Response is either a reply or a notification. Replies are solicited, a request has been sent beforehand. Notifications are unsolicited, you are not sure when exactly and if at all they arrive.

Just so I'm not called a heretic later: yes, from English dictionary point of view these two words are synonyms. In general, wording found in this drivers matches firmware equivalents wherever possible to allow developers to switch between these two worlds with minimal adaptation period possible.

...

• /* DSP acked host's request */
• if (hipc_ack & spec->hipc_ack_done_mask) {

• /* mask done interrupt */
  

• snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset,
  

• 		      AVS_ADSP_HIPCCTL_DONE, 0);
  

• complete(&ipc->done_completion);
  

• /* tell DSP it has our attention */
  

• snd_hdac_adsp_updatel(adev, spec->hipc_ack_offset,
  

• 		      spec->hipc_ack_done_mask,
  

• 		      spec->hipc_ack_done_mask);
  

• /* unmask done interrupt */
  

• snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset,
  

• 		      AVS_ADSP_HIPCCTL_DONE,
  

• 		      AVS_ADSP_HIPCCTL_DONE);
  

does the order between the complete() and the next two register updates matter?

I would have updated the registers immediately and signal the completion later.

I am also not sure why it's necessary to mask the done interrupt then unmask it. There is nothing that seems to require this masking?

Or are you expecting the code blocked on wait_for_completion to be handled with interrupts masked, which could be quite racy?

Given how the things turned out in cAVS, some steps are not always required. Here, we have very strict implementation and so interrupt are masked.

I'm unsure if relocating complete() to the bottom would bring any consequences. And no, the code waiting_for_completion is not expecting interrupts to be masked as there is no reply for ROM messages.

...

• ret = IRQ_HANDLED;
  

• }
• /* DSP sent new response to process */
• if (hipc_rsp & spec->hipc_rsp_busy_mask) {

• /* mask busy interrupt */
  

• snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset,
  

• 		      AVS_ADSP_HIPCCTL_BUSY, 0);
  

• ret = IRQ_WAKE_THREAD;
  

• }
• return ret;

+}

...

+static int avs_ipc_wait_busy_completion(struct avs_ipc *ipc, int timeout) +{

• int ret;

+again:

• ret = wait_for_completion_timeout(&ipc->busy_completion,

• 			  msecs_to_jiffies(timeout));
  

• /*

• * DSP could be unresponsive at this point e.g. manifested by
  

• * EXCEPTION_CAUGHT notification. If so, no point in continuing.
  

EXCEPTION_CAUGHT doesn't seem to be described in this patchset, so not sure what this comment might mean.

Comment describes the circumstances for the communication failures and arrival of EXCEPTION_CAUGHT notification is one of them.

...

• */
  

• if (!ipc->ready)

• return -EPERM;
  

• if (!ret) {

• if (!avs_ipc_is_busy(ipc))
  

• 	return -ETIMEDOUT;
  

• /*
  

•  * Firmware did its job, either notification or reply
  

•  * has been received - now wait until it's processed.
  

•  */
  

• wait_for_completion_killable(&ipc->busy_completion);
  

can you elaborate on why wait_for_completion() is not enough? I haven't seen the 'killable' attribute been used by anyone in sound/

This is connected to how firmware handles messaging i.e. via queue. you may get BUSY interrupt caused by a notification while waiting for the reply for your request. Being 'disturbed' by a notification does not mean firmware is dead, it's just busy and so we wait until previous response is processed entirely.

...

• }
• /* Ongoing notification's bottom-half may cause early wakeup */
• spin_lock(&ipc->rx_lock);
• if (!ipc->rx_completed) {

• /* Reply delayed due to notification. */
  

• reinit_completion(&ipc->busy_completion);
  

• spin_unlock(&ipc->rx_lock);
  

• goto again;
  

shouldn't there be some counter to avoid potential infinite loops here?

This is not a bad idea although the counter is going to be high e.g.: 128. With DEBUG-level logs enabled you can get ton of messages before your reply gets finally sent.

...

• }
• spin_unlock(&ipc->rx_lock);
• return 0;

+}

...

+static int avs_dsp_do_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request,

• 	       struct avs_ipc_msg *reply, int timeout)
  

+{

• struct avs_ipc *ipc = adev->ipc;
• int ret;
• if (!ipc->ready)

• return -EPERM;
  

• mutex_lock(&ipc->msg_mutex);
• spin_lock(&ipc->rx_lock);
• avs_ipc_msg_init(ipc, reply);
• avs_dsp_send_tx(adev, request);
• spin_unlock(&ipc->rx_lock);
• ret = avs_ipc_wait_busy_completion(ipc, timeout);
• if (ret) {

• if (ret == -ETIMEDOUT) {
  

• 	dev_crit(adev->dev, "communication severed: %d, rebooting dsp..\n",
  

• 		 ret);
  

dev_crit() seems over the top if there is a recovery mechanism

There is just one dev_crit() within entire driver and it's there for a reason - communication failure is critical and in practice, should never occur in any scenario on the production hardware.

...

• 	avs_ipc_block(ipc);
  

• }
  

• goto exit;
  

• }
• ret = ipc->rx.rsp.status;
• if (reply) {

• reply->header = ipc->rx.header;
  

• if (reply->data && ipc->rx.size)
  

• 	memcpy(reply->data, ipc->rx.data, reply->size);
  

• }

+exit:

• mutex_unlock(&ipc->msg_mutex);
• return ret;

+}

+static int avs_dsp_send_msg_sequence(struct avs_dev *adev,

• 		     struct avs_ipc_msg *request,
  

• 		     struct avs_ipc_msg *reply, int timeout,
  

• 		     bool wake_d0i0, bool schedule_d0ix)
  

the last two arguments are not used. is this intentional?

Used by the d0ix implementation that is not part of this part. Can relocate.

...

+{

• return avs_dsp_do_send_msg(adev, request, reply, timeout);

+}

+int avs_dsp_send_msg_timeout(struct avs_dev *adev, struct avs_ipc_msg *request,

• 	     struct avs_ipc_msg *reply, int timeout)
  

+{

• return avs_dsp_send_msg_sequence(adev, request, reply, timeout,

• 			 false, false);
  

+}

+int avs_dsp_send_msg(struct avs_dev *adev, struct avs_ipc_msg *request,

•      struct avs_ipc_msg *reply)
  

+{

• return avs_dsp_send_msg_timeout(adev, request, reply,

• 			adev->ipc->default_timeout_ms);
  

+}

is there really a 4-level nesting in your helpers?

avs_dsp_send_msg avs_dsp_send_msg_timeout avs_dsp_send_msg_sequence avs_dsp_do_send_msg

this seems complicated, no?

At the very least you should explain what a message and message sequence are, and why this is split this way.

With d0ix handling added, it becomes clear why such separation exists. I left these parts here to reduce the delta in patches that update this code later on. Can simplify here and update the d0ix implementation accordingly.

...

+int avs_dsp_send_pm_msg_timeout(struct avs_dev *adev,

• 		struct avs_ipc_msg *request,
  

• 		struct avs_ipc_msg *reply, int timeout,
  

• 		bool wake_d0i0)
  

+{

• return avs_dsp_send_msg_sequence(adev, request, reply, timeout,

• 			 wake_d0i0, false);
  

+}

so the 'pm' means 'wake-d0i0'? that's far from intuitive.

avs_dsp_send_d0i0_msg_timeout() would better describe what you are trying to do.

In addition you need an explanation that d0i0 is a *firmware* concept without direct links to the *device* Dx status.

This goes for both, Dx and D0ix related messages.

...

+void avs_dsp_interrupt_control(struct avs_dev *adev, bool enable) +{

• const struct avs_spec *const spec = adev->spec;
• u32 value;
• value = enable ? AVS_ADSP_ADSPIC_IPC : 0;
• snd_hdac_adsp_updatel(adev, AVS_ADSP_REG_ADSPIC,

• 	      AVS_ADSP_ADSPIC_IPC, value);
  

• value = enable ? AVS_ADSP_HIPCCTL_DONE : 0;
• snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset,

• 	      AVS_ADSP_HIPCCTL_DONE, value);
  

• value = enable ? AVS_ADSP_HIPCCTL_BUSY : 0;
• snd_hdac_adsp_updatel(adev, spec->hipc_ctl_offset,

• 	      AVS_ADSP_HIPCCTL_BUSY, value);
  

does the order matter? please add a comment.

interrupt_control() is only used during probing and teardown procedures so the order does not matter. You need all those bits up before firmware loading can even begin. And when you are done with firmware, you zero these out but and the order does not matter either as again, there's none to "talk" to. Note: ADSPIC_IPC is higher in hierarchy than DONE and BUSY.