ERS DAI: Echo Reference Signal DAI and DMA implementation. by akloniex · Pull Request #1652 · thesofproject/sof

akloniex · 2019-07-18T13:26:47Z

Implementation of virtual DAI and DMA dedicated for echo reference signal capture.

jajanusz · 2019-07-18T13:48:12Z

jajanusz · 2019-07-18T13:53:05Z

@akloniex Assigning to ABI MINOR 9 atm

tlauda · 2019-07-18T21:28:08Z

@akloniex Could you describe how should the pipeline look like, what exactly does this dai do, why do we need additional DMA etc?

In order to allow multiple callbacks registered to single buffer component, buffer_callback structure is created and list of callbacks nested into comp_buffer. Signed-off-by: ArturX Kloniecki <arturx.kloniecki@linux.intel.com>

akloniex · 2019-07-19T08:18:49Z

@tlauda The main reason for creation of new DMA type is to provide continuous signal as echo reference, even when there is no playback running. In such case, ers-dma generates zeros at steady rate and pushed ers dai forward.
With previous implementation of echo reference, based on demux component, 2 pipelines were connected together, with echo pipe being dailess, which rendered it impossible to generate silence as output.

Signed-off-by: ArturX Kloniecki <arturx.kloniecki@linux.intel.com>

tlauda · 2019-07-22T10:06:29Z

@akloniex @mmaka1 @lbetlej
I have concerns about adding the new DAI and DMA. DAI isn't really doing anything and DMA is "fake", but it's on the same list of platform's DMAs. Also it's not really flexible, because the naming suggests usage only for echo reference and the whole scheduling logic is inside DMA.
In my opinion adding additional "Virtual DAI" as audio component would be a better idea. The functionality would be the same. Scheduling would be set to timer, so no additional work here. Such component could be easily used in any other scenario e.g. probe with no additional implementations needed.

akloniex · 2019-07-22T16:29:30Z

@tlauda
My intention was to make DAI reusable for probes and possibly other future features, but I ran short on names for things up my sleeve, so went the easy route of naming it as feature it was initially developed for. And naming it 'virtual' sounded very vague for me, so didn't decide to go with that name.
Nevertheless, if I understand correctly, your concern about DAI is mostly about it's name, not it's behaviour.
As for DMA, I believe that having it along with 'real' DMAs adds to consistency of the code, and in the long run should do more good than harm. Even though it doesn't represent any hardware behind it, from user point of view it behaves like one. Also, it shouldn't be able to choose by mistake, as it has unique supported device to connect to, which excludes it from dma_get() calls which don't request this type explicitly.

tlauda · 2019-07-23T06:26:51Z

@akloniex I haven't yet checked the DMA implementation itself. Wanted to start with the most basic thing, which is the structure and the location of the code. I fully understand the reasoning behind reusing DAI component and extracting the new functionality to DMA layer, but I just don't think it's the way to go in this particular case. There is nothing outstanding you need from the DAI's implementation and I would even say, that it adds complexity to your code, since you need to call different type of callbacks, take dma_sg_array etc.

lgirdwood

This should be split up into a patch per change/feature with commit messages. I've no idea what this is trying to achieve.

lgirdwood · 2019-07-25T15:38:21Z

+ * of the callback.
+ *
+ * BUFF_CB_TYPE_PRODUCE - uint32_t* pointing to amount of bytes produced
+ * BUFF_CB_TYPE_CONSUME - uint32_t* pointing to amount of bytes consumed


I've just reviewed this in another PR ???

lgirdwood · 2019-07-25T15:39:19Z

 			dd->chan = DMA_CHAN_INVALID;
 		}
 		break;
+	case SOF_DAI_INTEL_ERS:


I dont think echo reference has anything Intel specific about it so it should be SOF_DAI_ECHO_REF

lgirdwood · 2019-07-25T15:54:41Z

 add_local_sources(sof
 	hda-dma.c
 	hda.c
+	ers.c


ers -> echo_ref makes it easier to find and search.

lgirdwood · 2019-07-25T15:55:41Z

+	pdata->cb(pdata->cb_data, DMA_CB_TYPE_IRQ, NULL);
+
+	// reschedule
+	return 1000;


This seem hard coded to 1ms ?

oh and C style comment please

lgirdwood · 2019-07-25T15:57:21Z

+
+	/* if we're falling behind too much, attempt to catch up */
+	if (pdata->src_data_size >= *avail * pdata->elem_array->count)
+		*avail *= 2;


Shouldn't we tell the user here via some trace ?

lgirdwood · 2019-07-25T15:59:44Z

+		return -ENODEV;
+	}
+
+	dai->plat_data.fifo[1].handshake = (uintptr_t)comp->cb;


are we casting a callback pointer to a handshake ? Please find a better way of doing this, it's a HACK.

@lgirdwood comp in this context is ipc_comp_dev*, so comp->cb refers to comp_buffer pointer, not callback function. In this case buffer component is being passed as src_dev to dma, during dma_set_config call.
If you think that it's still too hacky, then I could try to find a better way to pass this pointer to dma.

yep, too hacky. please find a better way.

lgirdwood · 2019-07-25T17:15:37Z

 	pm_runtime_put_sync(SSP_CLK, dai->index);

-	rfree(dma_get_drvdata(dai));
+	rfree(dai_get_drvdata(dai));


Looks like a separate PR is needed for this bug fix. Good find. May also need done on other SSP drivers.

lgirdwood · 2019-07-25T17:19:57Z

 #define DMA_CAP_HDA		BIT(0) /**< HDA DMA */
 #define DMA_CAP_GP_LP		BIT(1) /**< GP LP DMA */
 #define DMA_CAP_GP_HP		BIT(2) /**< GP HP DMA */
+#define DMA_CAP_BUF_CP		BIT(3) /**< Buffer copier DMA */


Why is this special and needs it's own CAP ?

lgirdwood · 2019-07-26T08:17:01Z

 #define TRACE_CLASS_SCHEDULE_LL	(31 << 24)
+#define TRACE_CLASS_SOUNDWIRE	(32 << 24)
+#define TRACE_CLASS_KEYWORD	(33 << 24)
+#define TRACE_CLASS_ERS		(34 << 24)


This looks like it should be a separate patch.

lgirdwood · 2019-07-26T08:17:48Z

 #define DMA_HOST_OUT_DMAC	5
 #define DMA_LINK_IN_DMAC	6
 #define DMA_LINK_OUT_DMAC	7
+#define DMA_ERS_DMAC		8


I'm really not following why this needs it's own DMAC ?

jajanusz · 2019-08-01T04:57:50Z

@akloniex Please resolve conflicts. Also there came at least 1 new dma ops that you need to add - get_attribute.

lgirdwood · 2020-01-06T16:24:50Z

@akloniex any update ?

lbetlej · 2020-01-27T09:29:08Z

@akloniex What are your plans regarding PR ? Do you agree to close it ?

akloniex · 2020-01-28T06:37:35Z

@lgirdwood @lbetlej I'm currently working on rebasing these changes and getting them back and running, but on a 2nd priority, after Probes. We could close this PR and open new one once I get it up, or we can keep it open and I'll just update it.

lgirdwood · 2020-01-28T10:51:21Z

@akloniex lets say v1.6 then. We can leave it open to keep it in ABI tracker

jajanusz · 2020-03-25T14:31:19Z

@akloniex Looks like stalled PR. Can I close it?

paulstelian97 · 2020-03-25T14:45:13Z

@akloniex Looks like stalled PR. Can I close it?

Activity in related PRs today (the optional Xrun things)

akloniex added the ABI ABI change involved label Jul 18, 2019

akloniex requested review from jajanusz, juimonen, lbetlej, lgirdwood, mmaka1, plbossart and tlauda July 18, 2019 13:26

akloniex requested review from a team, bardliao, bkokoszx and xiulipan as code owners July 18, 2019 13:26

akloniex force-pushed the ERS branch 2 times, most recently from 828c159 to a483656 Compare July 19, 2019 07:56

ERS DAI: Echo Reference Signal DAI and DMA implementation.

8ceb5b6

Signed-off-by: ArturX Kloniecki <arturx.kloniecki@linux.intel.com>

akloniex force-pushed the ERS branch from a483656 to 8ceb5b6 Compare July 19, 2019 09:01

lgirdwood requested changes Jul 26, 2019

View reviewed changes

lgirdwood added this to the v1.6 milestone Jan 28, 2020

akloniex mentioned this pull request Feb 25, 2020

echo reference signal: Interface and skeleton implementation. #2407

Closed

akloniex closed this Mar 26, 2020

akloniex deleted the ERS branch May 27, 2020 08:11

Conversation

akloniex commented Jul 18, 2019

Uh oh!

jajanusz commented Jul 18, 2019

Uh oh!

jajanusz commented Jul 18, 2019

Uh oh!

tlauda commented Jul 18, 2019

Uh oh!

akloniex commented Jul 19, 2019

Uh oh!

tlauda commented Jul 22, 2019

Uh oh!

akloniex commented Jul 22, 2019

Uh oh!

tlauda commented Jul 23, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jajanusz commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jan 6, 2020

Uh oh!

lbetlej commented Jan 27, 2020

Uh oh!

akloniex commented Jan 28, 2020

Uh oh!

lgirdwood commented Jan 28, 2020

Uh oh!

jajanusz commented Mar 25, 2020

Uh oh!

paulstelian97 commented Mar 25, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jajanusz commented Aug 1, 2019 •

edited

Loading