ASoC: SOF: Intel/ipc4: Add support for two staged library loading #4521

ujfalusi · 2023-08-14T10:12:34Z

Hi,

It was determined that the existing library loading flow cannot follow the hardware programming sequence for the DMA, which is:
DSP side sets buffer and sets the GEN bit
Host side sets the RUN bit

With the single message the current flow is:
Host sets the RUN bit
Host sends LOAD_LIBRARY
DSP side sets buffer and sets the GEN bit

The agreed solution is that we will add a new LOAD_LIBRARY_PREPARE message and by using it in host and firmware side we can have correct programming flow:
host sends LOAD_LIBRARY_PREPARE
DSP side sets it's buffer and sets the GEN bit
Host sets the RUN bit
Host sends LOAD_LIBRARY
DSP recceives the library data.

In case the firmware does not implement this new message (returns with not supported message) then we will proceed and send only the LOAD_LIBRARY message as it was defined by legacy and to maintain backwards compatibility.

plbossart

a couple of cosmetic/nit-picks and a more fundamental question on ABI and definition of "legacy".

sound/soc/sof/ipc4.c

plbossart · 2023-08-14T21:06:53Z

include/sound/sof/ipc4/header.h

@@ -108,6 +108,11 @@ enum sof_ipc4_global_msg {

 	/* Loads library (using Code Load or HD/A Host Output DMA) */
 	SOF_IPC4_GLB_LOAD_LIBRARY,
+	/*
+	 * Prepare the host DMA channel for library loading, must be followed by
+	 * a  SOF_IPC4_GLB_LOAD_LIBRARY message as the library loading step


extra space after 'a'

include/sound/sof/ipc4/header.h

plbossart · 2023-08-14T21:12:23Z

sound/soc/sof/intel/hda-loader.c

+	 * message with valid dma_id and invalid (0) lib_id
+	 * if the firmware does not have support for the message, we will
+	 * receive -EOPNOTSUPP. In this case we assume legacy, single step library
+	 * loading support and proceed to send the LOAD_LIBRARY message.


this goes with my question on ABI. Do we even want to support those 'legacy' firmware and why?

I think supporting firmware which does not implement this mode of library loading can be useful and only developers will encounter them.

plbossart · 2023-08-14T21:13:13Z

sound/soc/sof/intel/hda-loader.c

+
+	/*
+	 * 1st stage: SOF_IPC4_GLB_LOAD_LIBRARY_PREPARE
+	 * message with valid dma_id and invalid (0) lib_id


why do you even need to pass an invalid lib_id here?

True, we only send the dma_id.

lyakh

subject to addressing Pierre's comments - LGTM otherwise

ujfalusi · 2023-08-15T08:56:36Z

Changes since v1:

commit messages updated
removed the note about message id 25 being reserved (as we are using it)
clarify and cleanup the library loading flow, drop the lib_id reference from LOAD_LIBRARY_PREPARE message for example

plbossart

Approved but commit messages can be improved for grammar and clarity, see below.

include/sound/sof/ipc4/header.h

jsarha

Code-wise I see nothing wrong. For commit messages, the message in the 3rd commit is a bit hard to follow and mostly redundant, as 4th commit message explains the same thing. Maybe remove most of the 3rd commit message, and just tell that a new message is needed. The explanation comes in the 4th message. From the 4rd I'd just drop "There is" from the beginning. These are only suggestions, so approved.

ujfalusi · 2023-08-16T12:32:23Z

Changes since v2:

commit message rewritten for the new message type introduction (SOF_IPC4_GLB_LOAD_LIBRARY_PREPARE)

ranj063 · 2023-08-16T16:02:13Z

sound/soc/sof/intel/hda-loader.c

+					status,
+					(status & SOF_HDA_SD_FIFOSIZE_FIFOS_MASK),
+					HDA_DSP_REG_POLL_INTERVAL_US,
+					HDA_DSP_BASEFW_TIMEOUT_US);


@ujfalusi is this just a paranoia check or is it really required to check the FIFO value?

ping @ujfalusi, if we want this in 6.6 we don't have much time left

it is paranoia check, but I feel more comfortable doing this, The firmware will set the GEN bit before it sends the reply.

Oh, the FIFO value check? It is paranoia and also to make sure that we only set the RUN bit after the GEN was set. It should be, but I do feel that this a good practice and provides additional point if the loading fails (like the firmware have bug and not set the GEN bit and returns with success)

ping @ujfalusi, if we want this in 6.6 we don't have much time left

I missed the comment, sorry.

lyakh · 2023-08-18T07:34:53Z

sound/soc/sof/intel/hda-loader.c

+		ret = snd_sof_dsp_read_poll_timeout(sdev, HDA_DSP_HDA_BAR,
+					sd_offset + SOF_HDA_ADSP_REG_SD_FIFOSIZE,
+					status,
+					(status & SOF_HDA_SD_FIFOSIZE_FIFOS_MASK),


superfluous parentheses

OK, fixing it, for some reason checkpatch was fine with it.

oh, checkpatch is perfectly happy to swallow as many parentheses as you want to throw at it x = (((((0)))));

The status code 2 and 15 can be translated to -EOPNOTSUPP, so convert them to a meaningful error number. Signed-off-by: Peter Ujfalusi <[email protected]>

The FIFOS (FIFO Size) field is in bit 0-15 of the register. Use the defined mask instead of a magic number for the FIFOS value masking in hda_dsp_stream_hw_params(). Signed-off-by: Peter Ujfalusi <[email protected]>

On Intel platforms there is a strict order requirement for the DMA programming: DSP side configures the buffer and sets the GEN bit Host side sets the RUN bit. In order to follow this flow, a new global message type has been added to prepare the DSP side of the DMA: host sends LOAD_LIBRARY_PREPARE with the dma_id DSP side sets its buffer and sets the GEN bit Host sets the RUN bit Host sends LOAD_LIBRARY with dma_id and lib_id DSP receives the library data. It is up to the platform code to use the new prepare stage message and how to handle the reply to it from the firmware, which can indicate that the message type is not supported/handled. In this case the kernel should proceed to the LOAD_LIBRARY stage assuming a single stage library loading: host sends LOAD_LIBRARY_PREPARE with the dma_id DSP replies that the message type is not supported/handled Host acknowledges the return code and sets the RUN bit Host sends LOAD_LIBRARY with dma_id and lib_id DSP receives the library data. Signed-off-by: Peter Ujfalusi <[email protected]>

There is a certain sequence needs to be followed when configuring the HDA DMA in host and DSP. The firmware provides a way to handle this two stage sequencing by splitting the library loading into two stage: 1st stage: LOAD_LIBRARY_PREPARE message the lib_id is 0, used to configure the DMA on DSP side 2nd stage: LOAD_LIBRARY message both dma_id and lib_id is valid, used for the actual transfer of the library In case a firmware without support for this two stage loading is used then the second stage message will trigger the loading and the first stage will return with error, which is ignored by the kernel. Signed-off-by: Peter Ujfalusi <[email protected]>

ujfalusi · 2023-08-18T08:09:32Z

Changes since v3:

remove unneeded parenthesis in snd_sof_dsp_read_poll_timeout() call

mengdonglin · 2023-08-21T10:54:40Z

This is kernel part for loading a module library. The FW part has been merged thesofproject/sof#7986

ujfalusi · 2023-08-21T11:41:09Z

This is kernel part for loading a module library. The FW part has been merged thesofproject/sof#7986

@mengdonglin, that PR was the cleanup and fixes, the two staged one is not yet submitted.

plbossart · 2023-08-21T14:18:46Z

@ranj063 can you review this one please?

ranj063 · 2023-08-21T19:54:15Z

@ujfalusi there's a genuine checkpatch warning. Maybe you can submit a fixup?

ujfalusi · 2023-08-22T05:56:53Z

@ujfalusi there's a genuine checkpatch warning. Maybe you can submit a fixup?

There are two warnings, one is for 'hda' (misspelled had?) the other is alignment, but that I cannot fix unless we prefer long line warning. Almost all uses of snd_sof_dsp_read_poll_timeout() are like this.

aiChaoSONG · 2023-08-22T09:32:29Z

@ujfalusi there's a genuine checkpatch warning. Maybe you can submit a fixup?

There are two warnings, one is for 'hda' (misspelled had?) the other is alignment, but that I cannot fix unless we prefer long line warning. Almost all uses of snd_sof_dsp_read_poll_timeout() are like this.

Guess what, the checkpatch issue could be fixed without the long line warning.

ujfalusi requested a review from jxstelter August 14, 2023 10:12

ujfalusi requested review from libinyang, bardliao, lyakh, lgirdwood, plbossart, ranj063, kv2019i, dbaluta, vijendarmukunda, kuanhsuncheng and yaochunhung as code owners August 14, 2023 10:12

ujfalusi force-pushed the peter/sof/pr/two_stage_library_loading_02 branch from 82add0a to 9a23d47 Compare August 14, 2023 10:16

plbossart requested changes Aug 14, 2023

View reviewed changes

lyakh reviewed Aug 15, 2023

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/two_stage_library_loading_02 branch from 9a23d47 to 6d78705 Compare August 15, 2023 08:54

plbossart approved these changes Aug 15, 2023

View reviewed changes

include/sound/sof/ipc4/header.h Show resolved Hide resolved

include/sound/sof/ipc4/header.h Show resolved Hide resolved

jsarha previously approved these changes Aug 15, 2023

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/two_stage_library_loading_02 branch from 6d78705 to 3d89465 Compare August 16, 2023 12:31

plbossart previously approved these changes Aug 16, 2023

View reviewed changes

ranj063 reviewed Aug 16, 2023

View reviewed changes

plbossart requested a review from lyakh August 17, 2023 20:58

lyakh reviewed Aug 18, 2023

View reviewed changes

ujfalusi dismissed stale reviews from plbossart and jsarha via b0a7d2b August 18, 2023 08:06

ujfalusi force-pushed the peter/sof/pr/two_stage_library_loading_02 branch from 3d89465 to b0a7d2b Compare August 18, 2023 08:06

ujfalusi added 2 commits August 18, 2023 11:07

ASoC: SOF: ipc4: Convert status code 2 and 15 to -EOPNOTSUPP

fd73e9a

The status code 2 and 15 can be translated to -EOPNOTSUPP, so convert them to a meaningful error number. Signed-off-by: Peter Ujfalusi <[email protected]>

ASoC: SOF: Intel: hda: Add definition for SDxFIFOS.FIFOS mask

eeeeeb1

The FIFOS (FIFO Size) field is in bit 0-15 of the register. Use the defined mask instead of a magic number for the FIFOS value masking in hda_dsp_stream_hw_params(). Signed-off-by: Peter Ujfalusi <[email protected]>

ujfalusi added 2 commits August 18, 2023 11:07

plbossart approved these changes Aug 18, 2023

View reviewed changes

plbossart requested review from lyakh, jsarha, ranj063 and plbossart August 18, 2023 15:37

plbossart approved these changes Aug 18, 2023

View reviewed changes

jsarha approved these changes Aug 20, 2023

View reviewed changes

lyakh approved these changes Aug 21, 2023

View reviewed changes

ranj063 approved these changes Aug 21, 2023

View reviewed changes

aiChaoSONG approved these changes Aug 22, 2023

View reviewed changes

plbossart merged commit 1083fe3 into thesofproject:topic/sof-dev Aug 22, 2023

ujfalusi deleted the peter/sof/pr/two_stage_library_loading_02 branch November 30, 2023 06:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: Intel/ipc4: Add support for two staged library loading #4521

ASoC: SOF: Intel/ipc4: Add support for two staged library loading #4521

ujfalusi commented Aug 14, 2023

plbossart left a comment

plbossart Aug 14, 2023

ujfalusi Aug 15, 2023

plbossart Aug 14, 2023

ujfalusi Aug 15, 2023

plbossart Aug 14, 2023

ujfalusi Aug 15, 2023

lyakh left a comment

ujfalusi commented Aug 15, 2023

plbossart left a comment

jsarha left a comment

ujfalusi commented Aug 16, 2023

ranj063 Aug 16, 2023

plbossart Aug 17, 2023

ujfalusi Aug 18, 2023

ujfalusi Aug 18, 2023

ujfalusi Aug 18, 2023

lyakh Aug 18, 2023

ujfalusi Aug 18, 2023

lyakh Aug 18, 2023

ujfalusi commented Aug 18, 2023

mengdonglin commented Aug 21, 2023

ujfalusi commented Aug 21, 2023

plbossart commented Aug 21, 2023

ranj063 commented Aug 21, 2023

ujfalusi commented Aug 22, 2023

aiChaoSONG commented Aug 22, 2023

ASoC: SOF: Intel/ipc4: Add support for two staged library loading #4521

ASoC: SOF: Intel/ipc4: Add support for two staged library loading #4521

Conversation

ujfalusi commented Aug 14, 2023

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyakh left a comment

Choose a reason for hiding this comment

ujfalusi commented Aug 15, 2023

plbossart left a comment

Choose a reason for hiding this comment

jsarha left a comment

Choose a reason for hiding this comment

ujfalusi commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi commented Aug 18, 2023

mengdonglin commented Aug 21, 2023

ujfalusi commented Aug 21, 2023

plbossart commented Aug 21, 2023

ranj063 commented Aug 21, 2023

ujfalusi commented Aug 22, 2023

aiChaoSONG commented Aug 22, 2023