DMIC: eliminate the gain ramping task #5852

lyakh · 2022-05-24T10:44:23Z

The DMIC driver currently uses a separate task to perform gain ramping on start up. This can be achieved from the .copy() context avoiding an additional asynchronous context and races associated with it.

marcinszkudlinski · 2022-05-24T11:06:10Z

@juimonen, pls take a look, we need to sync changes with zephyr dai.

src/drivers/intel/dmic/dmic.c

wszypelt · 2022-05-24T15:37:28Z

Internal tests are not working properly now.
Please be patient, there are problems with the internal service, we will try to fix it within 24 hours

lgirdwood

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?

marcinszkudlinski · 2022-05-25T11:59:17Z

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?
@lgirdwood, Dmic driver code is not involved in copy operation, so there's nothing like "start of copy()" in dmic.c code.

lgirdwood · 2022-05-25T14:37:08Z

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?
@lgirdwood, Dmic driver code is not involved in copy operation, so there's nothing like "start of copy()" in dmic.c code.

I think it makes sense here to add copy() to the DAIs (like everything else) and keep the interface simple (and saves an extra call)

marcinszkudlinski · 2022-05-25T14:43:48Z

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?
@lgirdwood, Dmic driver code is not involved in copy operation, so there's nothing like "start of copy()" in dmic.c code.

I think it makes sense here to add copy() to the DAIs (like everything else) and keep the interface simple (and saves an extra call)

It is just a matter of name, .copy or .pre_copy - would be called anyway at the same moment. The point is - this method does not do any copying, it is a dma/dai job
.pre_copy maybe a more accurate name in fact

lgirdwood · 2022-05-25T19:33:34Z

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?
@lgirdwood, Dmic driver code is not involved in copy operation, so there's nothing like "start of copy()" in dmic.c code.

I think it makes sense here to add copy() to the DAIs (like everything else) and keep the interface simple (and saves an extra call)

It is just a matter of name, .copy or .pre_copy - would be called anyway at the same moment. The point is - this method does not do any copying, it is a dma/dai job .pre_copy maybe a more accurate name in fact

Agree, copy() naming does not align with usage today - but soon this call will be updated to the module C API and be called process() and this is a better naming/meaning match. Adding a new API adds a bit more complexity when it comes to the module API and I'd like to avoid and reuse (where good mapping permits).

lyakh · 2022-05-27T05:28:25Z

Can we simplify and do the ramp at the start of copy() instead of adding another callback ?
@lgirdwood, Dmic driver code is not involved in copy operation, so there's nothing like "start of copy()" in dmic.c code.

I think it makes sense here to add copy() to the DAIs (like everything else) and keep the interface simple (and saves an extra call)

It is just a matter of name, .copy or .pre_copy - would be called anyway at the same moment. The point is - this method does not do any copying, it is a dma/dai job .pre_copy maybe a more accurate name in fact

Agree, copy() naming does not align with usage today - but soon this call will be updated to the module C API and be called process() and this is a better naming/meaning match. Adding a new API adds a bit more complexity when it comes to the module API and I'd like to avoid and reuse (where good mapping permits).

@lgirdwood but I should call it .copy() now, not .process() directly?

lgirdwood · 2022-05-27T13:23:39Z

@lgirdwood but I should call it .copy() now, not .process() directly?

Yes, call it copy() today - there will be a subsequent update that then renames later and aligns with the module API.

lgirdwood · 2022-05-30T14:08:40Z

@lyakh @singalsu do we expect the gain ramp to change (and fail tests) ? we are seeing a failure on the DMIC tests on internal CI.

singalsu · 2022-05-30T15:03:06Z

@lyakh @singalsu do we expect the gain ramp to change (and fail tests) ? we are seeing a failure on the DMIC tests on internal CI.

The capture ramps have been the same for a long time. If something has changed there could be a new bug. But I don't think CI tests the ramp shape, it can only detect no audio or much too silent audio kind of cases.

src/drivers/intel/dmic/dmic.c

lyakh · 2022-05-31T15:04:11Z

Two CI tests failed with a timeout: suspend-resume-with-playback on WHL_UPEXT_HDA_ZEPHYR https://sof-ci.01.org/sofpr/PR5852/build515/devicetest/?model=WHL_UPEXT_HDA_ZEPHYR&testcase=check-suspend-resume-with-playback-5 and ipc-flood on BYT_MB_NOCODEC https://sof-ci.01.org/sofpr/PR5852/build515/devicetest/?model=BYT_MB_NOCODEC&testcase=check-ipc-flood - both supposedly unrelated

singalsu

Looks good to me, great work! There's some conflict, so a rebase is needed.

lgirdwood · 2022-06-01T10:02:03Z

Rerun CI as not expecting a build failure on only 1 platform.

lgirdwood · 2022-06-01T10:02:09Z

SOFCI TEST

kv2019i

@lyakh Documentation of the new copy() method could be improved in the first patch, but otherwise this looks great.

kv2019i · 2022-06-06T09:54:14Z

src/include/sof/lib/dai.h

@@ -86,6 +86,7 @@ struct dai_ops {
 	int (*remove)(struct dai *dai);
 	uint32_t (*get_init_delay_ms)(struct dai *dai);
 	int (*get_fifo_depth)(struct dai *dai, int direction);
+	void (*copy)(struct dai *dai);


A bit of documentation would be nice. In the context of the dmic problem this is easily understandable, but if you zoom out a bit, a copy method with no arguments looks very odd. Copy what and where?

lgirdwood · 2022-06-08T13:55:25Z

@mmaka1 did you have any comments here - is this OK as part of the path forward to a copier API ?

Add a DAI copy method to be called before dma_copy(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

DAI drivers sometimes need to access the underlying component object. Make this possible by adding a pointer to struct dai_data into struct dai and to struct comp_dev into struct dai_data and by assigning those pointers accordingly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Currently the DMIC driver uses a separate task to implement its gain ramping. This creates a race during driver clean up. Avoid an additional asynchronous context by performing the ramping in DAI's .copy() context. Use the DAI's .copy() method for that. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2022-06-09T10:35:44Z

@lgirdwood CI on-device are 2 unrelated suspend -resume failures: https://sof-ci.01.org/sofpr/PR5852/build602/devicetest/?model=TGLU_UP_HDA_ZEPHYR&testcase=check-suspend-resume-5 and https://sof-ci.01.org/sofpr/PR5852/build602/devicetest/?model=WHL_UPEXT_HDA_ZEPHYR&testcase=check-suspend-resume-with-capture-5, checkpatch is complaining about a line, ending in '(' which I agree isn't pretty but looked like the best option in that case to avoid way too long a line

lgirdwood · 2022-06-09T13:50:25Z

@mmaka1 ping ?

lgirdwood · 2022-06-10T15:18:17Z

Merging since this fixes an issue, we can align to copier APIs incrementally.

lyakh requested review from singalsu, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners May 24, 2022 10:44

marcinszkudlinski requested a review from juimonen May 24, 2022 11:06

singalsu reviewed May 24, 2022

View reviewed changes

src/drivers/intel/dmic/dmic.c Outdated Show resolved Hide resolved

singalsu reviewed May 24, 2022

View reviewed changes

src/drivers/intel/dmic/dmic.c Show resolved Hide resolved

lgirdwood reviewed May 25, 2022

View reviewed changes

lyakh mentioned this pull request May 27, 2022

[BUG] [ADLP] KMOD test failed on ADLP_GMB_I2S_ZEPHYR #5733

Closed

lyakh force-pushed the dmic-ramp branch from cb97c7f to 6580a32 Compare May 31, 2022 12:43

singalsu reviewed May 31, 2022

View reviewed changes

src/drivers/intel/dmic/dmic.c Outdated Show resolved Hide resolved

lyakh force-pushed the dmic-ramp branch from 6580a32 to 7084794 Compare May 31, 2022 15:20

singalsu reviewed May 31, 2022

View reviewed changes

lyakh force-pushed the dmic-ramp branch from 7084794 to 561e2ee Compare May 31, 2022 15:28

singalsu approved these changes Jun 1, 2022

View reviewed changes

lyakh force-pushed the dmic-ramp branch from 561e2ee to bd75c96 Compare June 2, 2022 06:41

kv2019i approved these changes Jun 6, 2022

View reviewed changes

lyakh added 3 commits June 8, 2022 17:27

dai: add a DAI operations copy method

f6ea849

Add a DAI copy method to be called before dma_copy(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the dmic-ramp branch from bd75c96 to 01d3937 Compare June 8, 2022 15:28

lgirdwood merged commit e56e88d into thesofproject:main Jun 10, 2022

lyakh deleted the dmic-ramp branch June 13, 2022 06:15

abonislawski mentioned this pull request Jun 29, 2022

dai-zephyr: add a DAI PRE_COPY trigger #5963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DMIC: eliminate the gain ramping task #5852

DMIC: eliminate the gain ramping task #5852

lyakh commented May 24, 2022

marcinszkudlinski commented May 24, 2022 •

edited

Loading

wszypelt commented May 24, 2022 •

edited

Loading

lgirdwood left a comment

marcinszkudlinski commented May 25, 2022

lgirdwood commented May 25, 2022

marcinszkudlinski commented May 25, 2022

lgirdwood commented May 25, 2022

lyakh commented May 27, 2022

lgirdwood commented May 27, 2022

lgirdwood commented May 30, 2022

singalsu commented May 30, 2022

lyakh commented May 31, 2022

singalsu left a comment

lgirdwood commented Jun 1, 2022

lgirdwood commented Jun 1, 2022

kv2019i left a comment

kv2019i Jun 6, 2022

lgirdwood commented Jun 8, 2022

lyakh commented Jun 9, 2022

lgirdwood commented Jun 9, 2022

lgirdwood commented Jun 10, 2022

DMIC: eliminate the gain ramping task #5852

DMIC: eliminate the gain ramping task #5852

Conversation

lyakh commented May 24, 2022

marcinszkudlinski commented May 24, 2022 • edited Loading

wszypelt commented May 24, 2022 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

marcinszkudlinski commented May 25, 2022

lgirdwood commented May 25, 2022

marcinszkudlinski commented May 25, 2022

lgirdwood commented May 25, 2022

lyakh commented May 27, 2022

lgirdwood commented May 27, 2022

lgirdwood commented May 30, 2022

singalsu commented May 30, 2022

lyakh commented May 31, 2022

singalsu left a comment

Choose a reason for hiding this comment

lgirdwood commented Jun 1, 2022

lgirdwood commented Jun 1, 2022

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Jun 6, 2022

Choose a reason for hiding this comment

lgirdwood commented Jun 8, 2022

lyakh commented Jun 9, 2022

lgirdwood commented Jun 9, 2022

lgirdwood commented Jun 10, 2022

marcinszkudlinski commented May 24, 2022 •

edited

Loading

wszypelt commented May 24, 2022 •

edited

Loading