[RFC]topology: hda: add a volume component to convert 24bit format by RanderWang · Pull Request #866 · thesofproject/sof

RanderWang · 2019-01-21T09:40:26Z

Fix the bug: sof audio pipeline with the format of 24bit doesn't
work on HDA platform. It is caused by sample data layout. SOF supports
S24_LE, and each sample format is like pattern of 0ddd, d stands for
data. Samples are padded with 0‟s at the MSB.

Link dma copys samples directly to hda bus, but hda bus requires samples
layout like pattern of ddd0. Samples are padded with 0‟s at the LSB to
left justify the sample within the container.

Suggesting by Liam and Seppo, using volume to convert S24_LE to S32_LE
in topology file.

This fixes #700
Signed-off-by: Rander Wang rander.wang@linux.intel.com

singalsu · 2019-01-21T10:22:41Z

Hi, I wonder why the volume component is instantiated by DAI for the HDA type and not by the playback and capture pipelines? It makes the HDA DAI different from others and creates risk for unneccessary volume components duplication. Did I interpret the patch m4 code correctly?

lgirdwood

This means that all HDA pipes must have a volume ? Why not just use the sof volume pipeline on the HDA pipes that need them.

RanderWang · 2019-01-22T01:56:50Z

Hi, I wonder why the volume component is instantiated by DAI for the HDA type and not by the playback and capture pipelines? It makes the HDA DAI different from others and creates risk for unnecessary volume components duplication. Did I interpret the patch m4 code correctly?

At pipelines, I don't know this playback or capture pipeline is for HDA or not, so I can't add a volume here. At dai component, I add a implicit volume here to let user doesn't take care of HW detail. And if we let users know this limitation and do it by themselves, they could use volume optimally

RanderWang · 2019-01-22T02:01:05Z

This means that all HDA pipes must have a volume ? Why not just use the sof volume pipeline on the HDA pipes that need them.

No, only the hda pipes with 24bit dai. And if there is a 24bit dai, the hda pipe needs a sof volume. I add no redundant volume except the one mentioned by Seppo

please refer to following code, there is a ifese to add only volume to s24le.

define(`HDA_CAPTURE_P_GRAPH',
`ifelse($2, `s24le',
`HDA_INSERT_VOLUME()
P_GRAPH(DAI_NAME, PIPELINE_ID,
        LIST(`          ',
	`dapm(N_BUFFER(`HDA'PIPELINE_ID), $5)'
	`dapm(N_PGA(`HDA'PIPELINE_ID), N_BUFFER(`HDA'PIPELINE_ID))'
	`dapm($4, N_PGA(`HDA'PIPELINE_ID))'))',
`P_GRAPH(DAI_NAME, PIPELINE_ID,
	LIST(`          ', `dapm($4, $5)'))')'
)

ranj063 · 2019-01-22T02:11:02Z

Hi, I wonder why the volume component is instantiated by DAI for the HDA type and not by the playback and capture pipelines? It makes the HDA DAI different from others and creates risk for unnecessary volume components duplication. Did I interpret the patch m4 code correctly?

At here, I don't know this playback or capture pipeline is for HDA or not, so I can't add a volume here.
Here I add a implicit volume here to let user doesn't take care of HW detail. And if we let users know this limitation and do it by themselves, they could use volume optimally

@RanderWang this change is too difficult to follow.
I think the simplest change is as Liam suggested. When you create the top-level m4 file, you know if it is for a HDA pipeline, so you can just use pipe-volume-playback/capture for those.

RanderWang · 2019-01-22T02:27:28Z

Hi, I wonder why the volume component is instantiated by DAI for the HDA type and not by the playback and capture pipelines? It makes the HDA DAI different from others and creates risk for unnecessary volume components duplication. Did I interpret the patch m4 code correctly?

At here, I don't know this playback or capture pipeline is for HDA or not, so I can't add a volume here.
Here I add a implicit volume here to let user doesn't take care of HW detail. And if we let users know this limitation and do it by themselves, they could use volume optimally

@RanderWang this change is too difficult to follow.
I think the simplest change is as Liam suggested. When you create the top-level m4 file, you know if it is for a HDA pipeline, so you can just use sof-volume-playback/capture for those.

@ranj063 I have discussed with keyon and Liam, the conclusion is given by Liam:
This can be made transparent to user by adding the volume automatically to any pipe that uses this DAI endpoint. You will need to create an M4 file for HDA (called intel-hda.m4) like we already have for SSP. This will define some macros for creating HDA Intel DAI's that will automatically add the volume control.

RanderWang · 2019-01-22T02:28:23Z

@ranj063 please have a look at #700. it is a limitation of hda dma, and it should be transparent to user

ranj063 · 2019-01-22T02:59:26Z

@RanderWang why is this needed? Why are you overriding the s24le set in the top-level m4 to s32le?

Doesnt this change make the other changes redundant?

volume component in FW would check source and sink format to select conversion function, so I need to change dai format.

Yes, you are right if there is a volume at the end of pipeline. I am try to figure out how to know whether there is a volume at the end of pipeline and if I know it, I don't need to add volume again

@RanderWang I still see Intel specific logic in generic code. Why don't you take each pipeline in machine.m4 and use a volume pipeline(instead of passthrough) for every pipeline that has this requirement. Please see the diff I commented yesterday.

ranj063 · 2019-01-22T03:04:00Z

@ranj063 please have a look at #700. it is a limitation of hda dma, and it should be transparent to user

@RanderWang if you want to make it transparent, you need to have the change in the top-level m4. Besides, user is not the end user here right? It is someone like you or me who is quite aware of the limitation. I feel like this is a roundabout way of addressing the problem. The simpler solution is to add a comment before the PIPELINE_PCM_ADD for HDA pipelines about why they need to use the volume pipelines.

RanderWang · 2019-01-22T04:06:46Z

@ranj063 please have a look at #700. it is a limitation of hda dma, and it should be transparent to user

@RanderWang if you want to make it transparent, you need to have the change in the top-level m4. Besides, user is not the end user here right? It is someone like you or me who is quite aware of the limitation. I feel like this is a roundabout way of addressing the problem. The simpler solution is to add a comment before the PIPELINE_PCM_ADD for HDA pipelines about why they need to use the volume pipelines.

Yes, your idea is the simplest one, and I would get all feedback from others.
Here, users maybe other persons in community, so they may be confused about this.

ranj063 · 2019-01-22T04:13:35Z

@ranj063 please have a look at #700. it is a limitation of hda dma, and it should be transparent to user

@RanderWang if you want to make it transparent, you need to have the change in the top-level m4. Besides, user is not the end user here right? It is someone like you or me who is quite aware of the limitation. I feel like this is a roundabout way of addressing the problem. The simpler solution is to add a comment before the PIPELINE_PCM_ADD for HDA pipelines about why they need to use the volume pipelines.

Yes, your idea is the simplest one, and I would get all feedback from others.
Here, users maybe other persons in community, so they may be confused about this.

@RanderWang how about adding a debug check in debug.m4 for HDA DAI's which have s24le pipeline format?

RanderWang · 2019-01-22T06:49:26Z

Yes, your idea is the simplest one, and I would get all feedback from others.
Here, users maybe other persons in community, so they may be confused about this.

@RanderWang how about adding a debug check in debug.m4 for HDA DAI's which have s24le pipeline format?

so if it is s24le, then do what ?

RanderWang · 2019-01-22T06:51:14Z

update my PR. Now check whether there is a volume at the end of pipeline. Don't add volume if there is one.

RanderWang · 2019-01-22T06:53:48Z

Now if it is HDA dai && format == S24le && no volume at the end of pipeline, a volume would be added.

lgirdwood · 2019-01-22T09:54:10Z

@RanderWang I still see this as the wrong course as it adds complexity where it's not needed. Can you define pipelines for each required machine like

PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,

RanderWang · 2019-01-23T08:09:01Z

@RanderWang I still see this as the wrong course as it adds complexity where it's not needed. Can you define pipelines for each required machine like
PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,

Done, thanks!

lgirdwood · 2019-01-23T20:08:19Z

Sorry @RanderWang I mean this

diff --git a/tools/topology/sof-whl-hda.m4 b/tools/topology/sof-whl-hda.m4
index ff6aa5f2..a4fa719a 100644
--- a/tools/topology/sof-whl-hda.m4
+++ b/tools/topology/sof-whl-hda.m4
@@ -28,7 +28,7 @@ include(`platform/intel/bxt.m4')
 
 # Low Latency playback pipeline 1 on PCM 0 using max 2 channels of s24le.
 # Schedule 48 frames per 1000us deadline on core 0 with priority 0
-PIPELINE_PCM_ADD(sof/pipe-passthrough-playback.m4,
+PIPELINE_PCM_ADD(sof/pipe-volume-playback.m4,
        1, 0, 2, s24le,
        48, 1000, 0, 0)

This we we include a volume transformation for each pipeline and it does not impact other HDA users who may not need or want a volume.

RanderWang · 2019-01-24T07:27:09Z

Update my PR, thanks!

lgirdwood · 2019-01-24T15:39:58Z

@RanderWang I still see Intel specific logic in generic code. Why don't you take each pipeline in machine.m4 and use a volume pipeline(instead of passthrough) for every pipeline that has this requirement. Please see the diff I commented yesterday.

RanderWang · 2019-01-25T08:23:43Z

@lgirdwood

Update my PR, thanks!

your method is the simplest one. But I discussed with you

In bug: #700, I said, it is a limitation of hda dma, and it should be transparent to user. And you suggest to define some macros for creating HDA Intel DAI's that will automatically add the volume control. So I did so much work and try make user do it as usual and we do the left in our library.

RanderWang · 2019-01-25T09:04:01Z

@lgirdwood sof-hda-generic.m4 is used by intel platform. sof-whl-hda.m4 is not used now.
Do you mean: use sof-hda-whl.m4 for whl, sof-hda-icl.m4 for ICL, sof-hda-apl.m4 for APL?

Thanks!

lgirdwood · 2019-01-25T16:55:49Z

@RanderWang I mean that sof-hda-generic.m4 will be for all HDA capable platforms and not just Intel platforms so must be generic (you cant make any Intel only changes to this file). You can however make Intel specific changes to intel files. So dai.m4 is generic and should not contain Intel specific logic.

If you renamed sof-hda-generic.m4 to sof-hda-intel-common.m4 then there is no problem making these changes.

RanderWang · 2019-01-28T08:57:52Z

@RanderWang I mean that sof-hda-generic.m4 will be for all HDA capable platforms and not just Intel platforms so must be generic (you cant make any Intel only changes to this file). You can however make Intel specific changes to intel files. So dai.m4 is generic and should not contain Intel specific logic.

If you renamed sof-hda-generic.m4 to sof-hda-intel-common.m4 then there is no problem making these changes.

Yes, one problem is in our kernel code, the topology for intel is named sof-hda-generic.tplg. This is not
a complex issue. If we used sof-hda-intel-common.m4, we need to build the file and rename it to sof-hda-generic.tplg before scp

It is useless now and sof-hda-generic.m4 would be used by WHL. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

sof audio pipeline with the format of 24bit doesn't work on HDA platform. It is caused by sample data layout. SOF supports S24_LE, and each sample format is like pattern of 0ddd, d stands for data. Samples are padded with 0‟s at the MSB. Link dma copys samples directly to hda bus, but hda bus requires samples layout like pattern of ddd0. Samples are padded with 0‟s at the LSB to left justify the sample within the container. Now there is a volume at the end of hda pipeline. We only need to convert dai type from S24_LE to S32_LE to make volume convert hda data to correct format Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang · 2019-01-28T09:40:13Z

Update PR, thanks!

lgirdwood · 2019-01-28T10:44:20Z

@RanderWang pls check CI

RanderWang · 2019-01-29T06:38:46Z

@lgirdwood CI is successful.

RanderWang requested review from lbetlej, lgirdwood, mmaka1 and plbossart as code owners January 21, 2019 09:40

RanderWang requested review from ranj063 and singalsu and removed request for plbossart January 21, 2019 09:41

RanderWang force-pushed the hda_24bit branch from e508e63 to 9c567be Compare January 21, 2019 09:45

RanderWang requested a review from keyonjie January 21, 2019 09:49

lgirdwood requested changes Jan 21, 2019

View reviewed changes

ranj063 reviewed Jan 22, 2019

View reviewed changes

RanderWang force-pushed the hda_24bit branch from 9c567be to 83702f1 Compare January 22, 2019 06:47

RanderWang force-pushed the hda_24bit branch from 83702f1 to be577fe Compare January 22, 2019 07:10

RanderWang force-pushed the hda_24bit branch from be577fe to 29a82e1 Compare January 23, 2019 08:04

RanderWang requested review from jajanusz and xiulipan as code owners January 23, 2019 08:04

RanderWang force-pushed the hda_24bit branch from 29a82e1 to 3a5599f Compare January 24, 2019 07:26

lgirdwood requested changes Jan 24, 2019

View reviewed changes

RanderWang force-pushed the hda_24bit branch from 3a5599f to 1114e9c Compare January 25, 2019 08:15

topology: hda: delete sof-whl-hda.m4

56e440e

It is useless now and sof-hda-generic.m4 would be used by WHL. Signed-off-by: Rander Wang <rander.wang@linux.intel.com>

RanderWang force-pushed the hda_24bit branch from 1114e9c to 6619d31 Compare January 28, 2019 09:37

lgirdwood approved these changes Jan 28, 2019

View reviewed changes

ranj063 approved these changes Jan 28, 2019

View reviewed changes

lgirdwood merged commit a5d32b4 into thesofproject:master Jan 29, 2019

Conversation

RanderWang commented Jan 21, 2019

Uh oh!

singalsu commented Jan 21, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

ranj063 Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Jan 22, 2019

Choose a reason for hiding this comment

Uh oh!

RanderWang Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Jan 22, 2019

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

ranj063 commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

RanderWang commented Jan 22, 2019

Uh oh!

lgirdwood commented Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 23, 2019

Uh oh!

lgirdwood commented Jan 23, 2019

Uh oh!

RanderWang commented Jan 24, 2019

Uh oh!

lgirdwood Jan 24, 2019

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jan 25, 2019

Uh oh!

RanderWang commented Jan 25, 2019

Uh oh!

lgirdwood commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RanderWang commented Jan 28, 2019

Uh oh!

lgirdwood commented Jan 28, 2019

Uh oh!

RanderWang commented Jan 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RanderWang commented Jan 22, 2019 •

edited

Loading

RanderWang commented Jan 22, 2019 •

edited

Loading

ranj063 commented Jan 22, 2019 •

edited

Loading

RanderWang Jan 22, 2019 •

edited

Loading

ranj063 commented Jan 22, 2019 •

edited

Loading

lgirdwood commented Jan 22, 2019 •

edited

Loading

lgirdwood commented Jan 25, 2019 •

edited

Loading

RanderWang commented Jan 28, 2019 •

edited

Loading