ASoC: SOF: Intel: hda-stream: Always use at least two BDLE for transfers #5181

ujfalusi · 2024-09-17T12:30:19Z

The HDA specification states that the SDnLVI (Last Valid Index) must be at least 1 (two BDLE entry).
While the LVI=0 worked so far without issues, it is better to align with the specification to avoid unforeseen issues with FW loading.

bardliao · 2024-09-18T00:45:16Z

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

 	if (!period_bytes)
-		period_bytes = hstream->bufsize;
+		period_bytes = hstream->bufsize / 2;


Sorry, I didn't get the relationship between the comment and the change. Why do we change period_bytes from hstream->bufsize to hstream->bufsize / 2?

because then there'll be 2 period each of size bufsize/2

Thanks for the explanation. I got it now.

ujfalusi · 2024-09-18T07:16:44Z

Changes since v1:

commit message updated with details
only force two periods for a 'single shot' transfer when the DMA memory is continuous single area - otherwise we will use separate BDLE for each segment anyways.
debug prints updated for better information presentation.

plbossart

No objection on enforcing the two BDLE for audio streams, but I don't think we have any configurations where this isn't the case already?

This doesn't look like a fix to me, more a constraint in case a topology file is not correct.

plbossart · 2024-09-18T07:31:26Z

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


 	periods = hstream->bufsize / period_bytes;

-	dev_dbg(sdev->dev, "periods:%d\n", periods);
+	dev_dbg(sdev->dev, "periods: %d\n", periods);


The commit message says

"
While the LVI=0 worked so far without issues, it is better to align with
the specification to avoid unforeseen issues with FW loading.
"

But there are no issues with FW loading that are fixed by this commit.

That is true, LVI=0 worked correctly but the specification explicitly saying that LVI must be at least 1 for the DMA to start.
We don't really know why LVI=0 but it cannot be take granted as it is against the spec.
It could be that SPIB changes the rules to allow LVI=0, but it is not documented afaik.

SPIB will definitively prevent the DMA from continuing, so and since we use SPIB for firmware download I really don't see what this fixes.

We've been using this for over 10 years now, if it was wrong we would have detected it, no?

I think the risk of splitting firmware downloads in two chunks offsets the unproven benefit of fixing a hypothetical problem.

wow this almost sounds like fridge poetry haha.

plbossart · 2024-09-18T07:32:05Z

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

+	 * HDA spec demands at least two 'periods' for the DMA to work correctly
+	 * Note: period_bytes == 0 can only happen for firmware or library
+	 * loading, the binary sizes have the necessary alignment requirements
+	 * met


what alignment requirements are you referring to?

The only requirement for HDaudio DMA is that the start address be aligned on 128 bytes. The size can be arbitrary.

Right, I need to move parts of the comment with the new change where I force two periods to get two BLDEs.
The comment tries to say that the firmware size is such that if we split it to two (and it is in a continuous area) then the second chunk will have start address 128-byte align as well.

plbossart · 2024-09-18T07:39:20Z

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

+		 */
+		if (chunk_size == hstream->bufsize)
+			period_bytes /= 2;
+	}


The other open I have is that the number of periods comes from topology, and I don't think we've ever had a solution with a single period.

So the question is: what does this commit actually fix?

With audio we always have at least two periods. The only case when we don't is the firmware and library loading.

Hrm, we could have periods_min set to 1... Interesting, that will not work for sure.
This patch only handles the firmware/library loading (when hstream->period_bytes is set to 0).
PCM with periods=1 cannot work...

ujfalusi · 2024-09-18T08:53:55Z

Changes since v2:

comments updated and move
- moving to draft in hopes that this can be dropped when I get clarification from the hardware design team.

ujfalusi · 2024-09-19T08:54:27Z

Moving back from draft as I got confirmation that LVI=0 is invalid regardless of SPIB use. It is working by 'luck', LVI=0 has not been validated with any version of HDA by hw validation teams.

kv2019i

@ujfalusi Just typos, no other issues found.

kv2019i · 2024-10-04T07:39:36Z

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

+		 * before the DMA operation can begin. This means that there
+		 * must be at least two BDLE present for the transfer.
+		 *
+		 * If the buffer is not a single continuous are then the


s/are/area/

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

kv2019i · 2024-10-04T07:40:28Z

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

+		 * must be at least two BDLE present for the transfer.
+		 *
+		 * If the buffer is not a single continuous are then the
+		 * hda_setup_bdle() will create multiple BLDEs for each segment.


s/BLDE/BDLE/

The HDA specification states that the SDnLVI (Last Valid Index) must be at least 1 (two BDLE entry). Update the debug prints as well to provide better information. While the LVI=0 worked so far without issues, it is better to align with the specification to avoid unforeseen issues with FW loading. Notes: - The LVI > 0 rules is valid and honored for audio use cases - LVI == 0 is used with software controlled (SPIB) transfers only for firmware and library loading, which is permitted by the hardware - This is not spelled out in the specification and it is better to avoid cases Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

ujfalusi · 2024-10-04T08:37:49Z

Changes since v3:

typos fixed and one blank line added as spotted by @kv2019i

ujfalusi requested a review from bardliao as a code owner September 17, 2024 12:30

ujfalusi requested review from plbossart, ranj063, lgirdwood and kv2019i September 17, 2024 12:31

bardliao reviewed Sep 18, 2024

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/fw-loader-min-two-dble_02 branch from fff0164 to 448843a Compare September 18, 2024 07:14

plbossart reviewed Sep 18, 2024

View reviewed changes

ujfalusi force-pushed the peter/sof/pr/fw-loader-min-two-dble_02 branch from 448843a to 316650f Compare September 18, 2024 08:52

ujfalusi marked this pull request as draft September 18, 2024 08:54

ujfalusi marked this pull request as ready for review September 19, 2024 08:52

ranj063 previously approved these changes Sep 27, 2024

View reviewed changes

kv2019i reviewed Oct 4, 2024

View reviewed changes

ujfalusi dismissed ranj063’s stale review via 7a72c99 October 4, 2024 08:36

ujfalusi force-pushed the peter/sof/pr/fw-loader-min-two-dble_02 branch from 316650f to 7a72c99 Compare October 4, 2024 08:36

lgirdwood approved these changes Oct 4, 2024

View reviewed changes

kv2019i approved these changes Oct 4, 2024

View reviewed changes

bardliao approved these changes Oct 4, 2024

View reviewed changes

bardliao merged commit 62f4847 into thesofproject:topic/sof-dev Oct 4, 2024
10 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF: Intel: hda-stream: Always use at least two BDLE for transfers #5181

ASoC: SOF: Intel: hda-stream: Always use at least two BDLE for transfers #5181

ujfalusi commented Sep 17, 2024

bardliao Sep 18, 2024

ranj063 Sep 18, 2024

bardliao Sep 18, 2024

ujfalusi commented Sep 18, 2024

plbossart left a comment

plbossart Sep 18, 2024

ujfalusi Sep 18, 2024

plbossart Sep 18, 2024

plbossart Sep 18, 2024

ujfalusi Sep 18, 2024

plbossart Sep 18, 2024

ujfalusi Sep 18, 2024

ujfalusi Sep 18, 2024

ujfalusi commented Sep 18, 2024

ujfalusi commented Sep 19, 2024

kv2019i left a comment

kv2019i Oct 4, 2024

kv2019i Oct 4, 2024

ujfalusi commented Oct 4, 2024

ASoC: SOF: Intel: hda-stream: Always use at least two BDLE for transfers #5181

ASoC: SOF: Intel: hda-stream: Always use at least two BDLE for transfers #5181

Conversation

ujfalusi commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi commented Sep 18, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi commented Sep 18, 2024

ujfalusi commented Sep 19, 2024

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ujfalusi commented Oct 4, 2024