Interrupt subsystem update for cascaded interrupts by lyakh · Pull Request #1053 · thesofproject/sof

lyakh · 2019-02-26T21:56:21Z

The first 35 patches are the actual interrupt subsystem modification to support cascaded interrupt controllers. Then 3 patches re-add the DW UART driver, here just for illustration, since they are handled in #957 . The last 2 patches add a DW INTC driver and enable it on Sue Creek. I expect a couple of comments here and there at least, so I pushed all those patches to this branch. In the final version, of course, they should not be present, at least the UART patches.
This patch series allows the removal of interrupt-map.h, but it is also included from host tools, so I didn't remove it for now. Once the header is removed, we should also remove "select IRQ_MAP" and "config IRQ_MAP" entries from Kconfig files.

RanderWang · 2019-03-04T09:35:57Z

@lyakh It is a huge list. I checked some of them and enjoy them. Could you split them to small ones ?

jajanusz · 2019-03-04T09:51:13Z

It shouldn't be there. You just choose it in *_defconfig file or with select statements

If you place them in defconfigs, that means, they would be selectable by the user, does that really make sense? What sense can it make to enable TRACE_DMA on a system, that doesn't have that capability? But yes, I can make this a "select" from all platforms apart from SUECREEK, that would be better, I think, thanks.

For capability issue you have depends on. Decision to use feature should be selectable.

jajanusz · 2019-03-04T09:53:46Z

Can you add some comments to fields? I know there are already lot of undocumented stuff but we should be more verbose when adding new code.

jajanusz · 2019-03-04T09:56:36Z

I think this one can be visible to the user.

The idea is, that the user or "selects" or *_defconfig select what specific hardware is available on the system. Some platforms can have DW_UART, others X_UART, others still Y_UART etc. And there are features in the SOF, that depend on "some" UART. So, all those specific UART implementations select UART to tell the code, that yes, UARTs are available on this system. Why would you make this selectable by the user? Are you proposing to make it possible to select DW_UART and deselect UART? What would that result in?

jajanusz · 2019-03-04T09:59:51Z

0x90000 - magic number, also I don't know if we should add trace points for things that may be called many times, I guess you will be masking/unmasking. I thought trace points are for things that should be done once and in order, like boot sequence, fw load etc.
@lgirdwood Should give you more input about this

oops, that's a debug left-over, will remove, thanks for spotting!

singalsu · 2019-03-04T13:25:06Z

For what triggered review request for me (the small DMIC related changes) this looks OK. Apparently there's need to do fixes to pass the CI checks.

lyakh · 2019-03-05T07:29:03Z

@RanderWang thanks, I'm glad you liked it :-) But TBH I'm not sure I understand why splitting it would make it easier. Whether you make it 5 series of 7 commits each, or 7 series of 5 commits each or whatever - it's still 35. And they are (almost) all interdependent. So, it wouldn't be possible to, say, extract 10 of them, that are independent, that touch other files. Some of them can be swapped of course, some have no dependencies and can be extracted from the series, but most of it is really a huge gradual change. So, if you split it into 5 series with 7 commits each, you'll have to wait with each series until all previous ones have been reviewed and merged.

lgirdwood

I've stopped reviewing, looks like this needs squashed and split into multiple PRs. Overall right direction and good work.

lgirdwood · 2019-03-06T15:20:43Z

Lets not use NULL here since it's not a UUID for removal. We could safely use timer (even though we don't use it in client code)

lgirdwood · 2019-03-06T15:21:39Z

why ? please state reason in the comment.

lgirdwood · 2019-03-06T15:22:07Z

ditto for all NULL unregisters.

lgirdwood · 2019-03-06T15:27:51Z

please comment what this block is checking for

lgirdwood · 2019-03-06T16:11:07Z

net loss of 4 bytes for no gain ?

This is needed, because interrupts will become virtual later. They will be mapped at run-time based on their interrupt controller parent and the physical interrupt number on that controller. So, we want to map interrupts once and then use the virtual IRQ number. See patch 493b086

Ok, but is the virtual number constant for each platform. i.e. APL virt IRQ 10 is always SSP0 ? If not then this will be painful to debug.

As long as the board configuration doesn't change, yes, they will stay constant. The only thing, that can change them is adding new or removing existing interrupt controllers. I don't think that would happen too often. We can add trace to the mapping function, as long as the IPC interrupt is a DSP native one. Or we can dump a mapping at the end of booting and then print any new mappings.

Dumping the mapping is nice. Lets do this is a subsequent PR later on.

lgirdwood · 2019-03-06T16:12:11Z

ditto, if compiler is providing this then there is no need to store it locally.

lgirdwood · 2019-03-06T16:13:39Z

Lets not use NULL since its being used internally as a UUID.

lgirdwood · 2019-03-06T16:18:10Z

The CPU core for each task will only be known at task creation time. i.e. after topology has been loaded. So we cannot hard code to current CPU unless this is being run on ALL CPUs.

@lgirdwood Sorry, where is this done? I see

interrupt_register(PLATFORM_IRQ_TASK_LOW, IRQ_AUTO_UNMASK, _irq_task, task_irq_low_get());

where PLATFORM_IRQ_TASK_LOW is IRQ_NUM_SOFTWARE[234] on all platforms, i.e. a DSP interrupt. Similar for MED and HIGH. That means they are registered on the current CPU. What am I missing?

Not yet implemented on other cores atm, topology can request core > 0 but code will assign 0 (although this will change with upcoming PRs).

hm, which PR is this? I think it should actually become better with these changes: instead of mangling on bitfields you'd call interrupt_enable() with a specific CPU, that's it. And it would be a good test case for that!

PRs for scheduling pipelines on other cores are in progress by @tlauda @mmaka1 , I dont know when they will be ready for merging though.

would it be possible to merge this first and then port their work on top?

possibly, both are large pieces of work, but have a small overlap surface so merging should not be too bad.

abonislawski · 2019-03-11T09:57:17Z

Please start using #if (CONFIG_*) instead of defined, like in recent changes in sof (#if defined CONFIG_ -> #if CONFIG_)

lyakh · 2019-03-11T14:26:20Z

@lgirdwood what's your preferred way to proceed with this PR? I realise that it is rather large, but splitting it doesn't really make it easier IMHO. Bear in mind, that the last 5 patches don't really belong to the PR. As for squashing - I think that would make patches less clear. I can see a couple of patches that are already a bit bigger than what one would normally want to have, but I don't see any patches, that are too small :-) Github really isn't the best tool for such reviews... I can email them, if that helps?

lgirdwood · 2019-03-11T17:47:02Z

@lgirdwood what's your preferred way to proceed with this PR? I realise that it is rather large, but splitting it doesn't really make it easier IMHO. Bear in mind, that the last 5 patches don't really belong to the PR. As for squashing - I think that would make patches less clear. I can see a couple of patches that are already a bit bigger than what one would normally want to have, but I don't see any patches, that are too small :-) Github really isn't the best tool for such reviews... I can email them, if that helps?

First step is to address comments and resend. We don't need the macro updates/fixes, they can go in another PR that can be merged immediately. Dont need the UART driver either in the PR (it can be a another PR).

lyakh · 2019-03-12T08:28:16Z

@lgirdwood sure, most comments are non-controversial and easy to address. I'll take out two macro-fixing patches as well of course. I see at least two larger issues:

cache. I've implemented smp-safe list-manipulation primitives like "for_each" or "add" or "delete." But all that has to be carefully double-checked...
have I managed to convince you to let me temporarily use platform-specific functions for interrupt (un)masking, enabling / disabling etc? I think it really makes the series easier to make and verify. That way we know, that we don't break those systems. After we've converted interrupt numbers, we should bring those drivers back to using standard methods.

lgirdwood · 2019-03-12T11:00:28Z

2. have I managed to convince you to let me temporarily use platform-specific functions for interrupt (un)masking, enabling / disabling etc? I think it really makes the series easier to make and verify. That way we know, that we don't break those systems. After we've converted interrupt numbers, we should bring those drivers back to using standard methods.

No, changing an API to later change it back makes no sense. It only creates more uncertainty.

lyakh · 2019-03-12T11:42:10Z

have I managed to convince you to let me temporarily use platform-specific functions for interrupt (un)masking, enabling / disabling etc? I think it really makes the series easier to make and verify. That way we know, that we don't break those systems. After we've converted interrupt numbers, we should bring those drivers back to using standard methods.

No, changing an API to later change it back makes no sense. It only creates more uncertainty.

It isn't very pretty, I agree, but I thought easier development, debugging and review were a good reason for that. But np, I can redo that part.

lyakh · 2019-03-13T15:26:51Z

@tlauda I'm beginning (too late but still) to doubt: are we really sure that caches aren't coherent on xtensa between cores?

tlauda · 2019-03-13T15:44:39Z

I'm sure.

lyakh · 2019-03-13T16:31:29Z

@tlauda that's a pity... Do we have any datasheets where this is described?

lyakh · 2019-03-14T08:08:56Z

@tlauda @lgirdwood this manual cache management really gets very intrusive, error-prone, pollutes the sources a lot, and probably causes non-negligible performance reduction. Are there other ways to handle that? If we collect all that data, that we know, that different CPUs will access in a common area or even section, maybe we can simplify that management. Is it possible to make that area non-cacheable? That would possibly cost even more performance-wise. How are caches invalidated / written back - with a cache-line granularity? We could try to manually optimise for cache-line lengths, but as any manual optimisation, that might be hard to do right and to keep maintaining. Or we could manually flush / invalidate that whole area when beginning to handle any of our SMP-critical data and before returning from those functions. Or would flushing a couple of K of data be worse than repeated flushing of a handful of bytes? Any other options?

lgirdwood · 2019-03-14T11:22:47Z

@lyakh manual cache management is unavoidable. It compiles out on platforms that manage it in HW. Having cache aware list accessors and locks APIs will simplify things a lot, so these should probably be created first as a new PR.

e.g.

spin_lock_invalidate(lock, object, object_size);

list_for_item_invalidate(item_ptr, list, item_size)

We also need to be aware that some operation are global (like IRQ registration) and some are local like the IRQ handler flow. This would influence when and where we would need to use cache ops i.e. IRQ handler would not need to invalidate global IRQ list but rather a sub list/object for that IRQ/level.

lyakh · 2019-07-25T14:40:33Z

@xiulipan I don't understand this log https://sof-ci.01.org/sofpr/PR1053/build2609/devicetest/CFL_RVP_HDA/multiple-pipeline-capture.sh/multiple-pipeline-capture.sh.txt - is it a timeout? Is this what you'd like me to check?

lyakh · 2019-07-25T14:48:30Z

@xiulipan I've just run

arecord -D hw:0,6 --dump-hw-param -c 2 -r 48000 -f s16_le /dev/null -d 5 &
arecord -D hw:0,7 --dump-hw-param -c 2 -r 16000 -f s16_le /dev/null -d 5 &
arecord -D hw:0,0 --dump-hw-param -c 2 -r 48000 -f s16_le /dev/null -d 5 &
aplay -D hw:0,3 --dump-hw-param -c 2 -r 48000 -f s16_le /dev/zero -d 5

on an HDA system and they all completed normally

lyakh · 2019-07-25T18:33:58Z

I cannot find the actual failure in the above test at https://quickbuild.igk.intel.com/build/4062204 - it says KW found something, but there's no information what and where it found.

tlauda · 2019-07-25T20:08:34Z

SOFCI TEST

tlauda · 2019-07-25T20:44:41Z

@xiulipan Is this real fail or some problem with kernel?

xiulipan · 2019-07-26T03:28:42Z

@tlauda @lyakh
Not sure now. I am thinking that we have code base that can not pass multiple-pipeline-capture.sh now. Need manually check to confirm.

@ClarexZhou @Jiangxinx
Could you help to run some manual test like multiple-pipeline-capture.sh base on this pr.

tlauda · 2019-07-26T06:29:29Z

@xiulipan @ClarexZhou @Jiangxinx This is urgent. I would like to merge it as soon as possible. Could you execute manual tests?

tlauda · 2019-07-26T06:29:48Z

SOFCI TEST

tlauda · 2019-07-26T07:36:08Z

SOFCI TEST

lyakh · 2019-07-26T08:54:10Z

@tlauda when you follow the "details" link, you get all fields as "passed"...

tlauda · 2019-07-26T09:00:44Z

@lyakh @xiulipan Yes, but when I go into multiple-pipeline-capture on CFL I see some fails. I really don't know what is going on. Could someone actually confirm if it's the real issue or not?

Jiangxinx · 2019-07-26T09:08:29Z

I tested BYT and APL platform, found one new issue on BYT platform:

Failed while do playback and capture at same time: I/O error (100%).

By the way, apl still can not pass multiple-pipeline-capture.sh ,and also can not pass multiple-pipeline-capture.sh on daily version.

Env
Kernel: f745362 on topic/sof-dev
platform: apl-up2 & BYT

Step
aplay -Dhw:0,0 -fS16_LE -c2 -r 48000 audio-files/raw/test-c2-r48000-fs16_le-d60.raw & arecord -Dhw:0,0 -c2 -fS16_LE test.wav -vv -i

Output

[1] 991
Recording WAVE 'test.wav' : Signed 16 bit Little Endian, Rate 8000 Hz, Stereo
Warning: rate is not accurate (requested = 8000Hz, got = 48000Hz)
         please, try the plug plugin 
Hardware PCM card 0 'sof-nocodec' device 0 subdevice 0
Its setup is:
  stream       : CAPTURE
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 48000
  exact rate   : 48000 (48000/1)
  msbits       : 16
  buffer_size  : 16320
  period_size  : 4080
  period_time  : 85000
  tstamp_mode  : NONE
  tstamp_type  : MONOTONIC
  period_step  : 1
  avail_min    : 4080
  period_event : 0
  start_threshold  : 1
  stop_threshold   : 16320
  silence_threshold: 0
  silence_size : 0
  boundary     : 9187343239835811840
  appl_ptr     : 0
  hw_ptr       : 0
Playing raw data 'audio-files/raw/test-c2-r48000-fs16_le-d60.raw' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
#+                                                 | 00%arecord: pcm_read:2103: read error: Input/output error
[1]+  Done                    aplay -Dhw:0,0 -fS16_LE -c2 -r 48000 audio-files/raw/test-c2-r48000-fs16_le-d60.raw

log file
logger.log
dmesg.log

tlauda · 2019-07-26T09:12:23Z

@Jiangxinx If I understand you correctly multiple-pipeline-capture on APL is no regression from this PR, but simultaneous playback and capture on BYT is.

Jiangxinx · 2019-07-26T09:17:47Z

@tlauda Yes ,I have updated the simultaneous playback and capture issue information.

ClarexZhou · 2019-07-26T09:29:26Z

multiple-pipeline-capture isn't a regression, also failed on daily sof on CFL. see #1674

And test this PR on GLK i2s notebook, CML HDA notebook, CFL-S HDA RVP and ICL-HDA RVP: No regression.

Kernel(topic/sof-dev): f745362
FW: https://sof-ci-storage.sh.intel.com/pr/sof/PR1053-9/

tlauda · 2019-07-26T09:32:29Z

@ClarexZhou @Jiangxinx Thanks for checking! Why the playback + capture fails on BYT, but CI reports PASS?

Jiangxinx · 2019-07-26T09:43:14Z

@tlauda I think CI have not finished aplay, this new regression only can be reproduced after finished aplay.

lyakh · 2019-07-26T10:01:04Z

@Jiangxinx what topology did you test with BYT? And what BYT platform? A MinnowBoard?

Jiangxinx · 2019-07-26T10:03:56Z

@lyakh MinnowBoard: sof-byt-nocodec.tplg & sof-byt-rt5651.tplg

xiulipan · 2019-07-26T10:09:59Z

@lyakh @tlauda @Jiangxinx @ClarexZhou
Thanks for help testing and looking for detail in test result.

That is why I would like to manually notify here, it seem with issue #1674 we will get a timeout in real. But it seems our parser for the test result could not recognize timeout there. I have raise a bug for the test framework.

Also we find that the new added multiple capture test case may cause some test failure, not sure if this comes from code base or the PR. So I think we can check the daily build test result and then we can see if the PR have issue.

We are still debugging about the new test case to see if we are doing some invalid test.

xiulipan · 2019-07-26T10:14:08Z

@lyakh @tlauda
For the BYT issue, I checked the CI test logs. It seems we only start a capture for the test case and no other PCM at same time.

@fredoh9
Please help to check the test case, why only start one capture. Actually we can start the playback for the same PCM.
https://sof-ci.01.org/sofpr/PR1053/build2622/devicetest/BYT_MB_RT5682/multiple-pipeline-capture.sh/multiple-pipeline-capture.sh.txt
https://sof-ci.01.org/sofpr/PR1053/build2622/devicetest/BYT_MB_NOCODEC/multiple-pipeline-capture.sh/multiple-pipeline-capture.sh.txt

tlauda · 2019-07-26T10:16:05Z

@xiulipan Great, thanks for the update. Yes, we should definitely add also playback to extend our test coverage. This way we won't need such heavy manual PR testing in the future.

lyakh · 2019-07-26T10:34:10Z

@Jiangxinx @xiulipan have you actually tried aplay audio on MinnowBoard with rt5651? It comes out distorted for me with the head of the current master branch (I think it's a known issue when pulseaudio is used). As for this PR - I've just tried simultaneous playback and record with my PR and with the master head and I get the same result - both work (apart from the audio quality of course). In the trace there's also a flood of

dw_dma_avail_data_size() size is 0! Channel enable = 0.

messages with both firmware versions. I used

arecord -d 3 -f dat -D hw:0,0 /dev/null & aplay -Dhw:0,0 -f dat -d 3 /dev/zero

to test, what do you use?

lgirdwood · 2019-07-26T12:04:04Z

@tlauda @lyakh @Jiangxinx @xiulipan - I'm going to merge this today as it looks like this BYT+PA issue is unrelated.

tlauda · 2019-07-26T12:25:06Z

@lgirdwood Let's do this before merging any other patch, so there is no need for rebase.

lgirdwood · 2019-07-26T12:31:24Z

@lyakh @tlauda done.

lyakh · 2019-07-26T13:10:37Z

@lgirdwood thanks, appreciated!

lyakh requested review from RanderWang, jajanusz, lbetlej, lgirdwood, libinyang, mmaka1, plbossart, singalsu, tlauda and xiulipan as code owners February 26, 2019 21:56

lyakh added enhancement New feature or request SUE Applies to Sue Creek platform labels Feb 26, 2019

jajanusz suggested changes Mar 4, 2019

View reviewed changes

tlauda reviewed Mar 5, 2019

View reviewed changes

Comment thread src/drivers/intel/cavs/interrupt.c Outdated

Comment thread src/lib/interrupt.c Outdated

lgirdwood requested changes Mar 6, 2019

View reviewed changes

abonislawski reviewed Mar 11, 2019

View reviewed changes

lyakh mentioned this pull request Mar 13, 2019

More cosmetic fixes and clean up #1025

Merged

lgirdwood merged commit 842e4b5 into thesofproject:master Jul 26, 2019

lyakh deleted the interrupt branch July 26, 2019 13:10

Jiangxinx mentioned this pull request Jul 29, 2019

[BUG][BYT] Failed while do playback and capture at same time on BYT MinnowBoard platform: I/O error (100%) #1677

Closed

Uh oh!

Conversation

lyakh commented Feb 26, 2019

Uh oh!

RanderWang commented Mar 4, 2019

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!

singalsu commented Mar 4, 2019

Uh oh!

lyakh commented Mar 5, 2019

Uh oh!

Uh oh!

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!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Mar 11, 2019

Uh oh!

lgirdwood commented Mar 11, 2019

Uh oh!

lyakh commented Mar 12, 2019

Uh oh!

lgirdwood commented Mar 12, 2019

Uh oh!

lyakh commented Mar 12, 2019

lgirdwood commented Mar 14, 2019 •

edited

Loading

Jiangxinx commented Jul 26, 2019 •

edited

Loading