reduce IPC platform driver API by lyakh · Pull Request #740 · thesofproject/linux

lyakh · 2019-03-21T16:17:46Z

Most importantly this patch series removes 2 IPC platform driver methods: .get_ready and .cmd_done, additionally it contains some simple improvements. Removing those methods is not a completely "lossless" process, e.g. some register access operations had to be swapped. That shouldn't cause problems, but it has to be thoroughly tested for any regressions. I've only tested on Up^2 with a HifiBerry DAC+ on its RCA output.

plbossart

@lyakh code looks good, and I like the way your patches progress logically, but I need your help to split them in a different way so that the fixes apply in a more direct way to the code already submitted upstream (which doesn't include haswell, skylake/kabylake, compressed, spi). Thanks!

plbossart · 2019-03-21T16:25:53Z

@lyakh please add a more complete prefix in your patches, e.g. for this one this should be ASoC: SOF: Intel since you are only touching intel-specific patches.

@lyakh this comment still applies. ASoC: SOF: Intel prefix please, it makes things easier to track for reviewers (some of whom may not care about Intel specifics)

plbossart · 2019-03-21T16:29:24Z

@lyakh can you do me a favor and not bundle things aren't ready for upstream with things that are. Specifically Haswell/SKL/KBL/Compressed/SPI are not shared upstream just yet, so it helps if your patches follow this split.

sorry, what branch should I be making patches against? I was using topic/sof-dev, should I use integration/... instead?

@lyakh we should use topic/sof-dev but if you look at the patches posted upstream HSW/KBL/SKL/Compressed/SPI are not included in them. So it will be easier to maintain the squash/rebase cycles if you split for ex. your commit: "ASoC: SOF: eliminate redundant mailbox reads" into 2. One intended for the files in the upstream patches and another one for those not in it (hsw.c etc).

I see, but how should I know which ones are and which ones aren't included? I think we need a separate branch, really.

@lyakh check topic/sof-dev-rebase, everything after 'add build support' is not upstream yet. Ideally your patches should apply cleanly on the initial batch. Thanks!

@plbossart let me take a semi-qualified guess: is this the commit, that I should be branching from?

commit af23c43
Author: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Date: Mon May 21 18:19:44 2018 -0500

ASoC: Intel: boards: initial support for Hifiberry Digiplus w/ WM8804

lgirdwood

This all looks good - more pink than green in the diffs :)

lgirdwood · 2019-03-21T17:03:41Z

Does %u make it obvious to the user that it's base16 ?

%u is for size_t and will print in decimal. Andy commented that the sizes should be decimal, not hex and I agree.

Oh, I missed that there are two sizes printed in this one, so both have to be made %u. And for size_t it might be %zu, I'll have to double check.

plbossart · 2019-03-22T16:35:48Z

@plbossart <https://github.com/plbossart> let me take a semi-qualified guess: is this the commit, that I should be branching from?

the last patch submitted upstream is c12f340 ('ASoC: Intel: select relevant machine drivers for SOF')

lyakh · 2019-03-25T12:03:47Z

@plbossart I forked from an earlier commit that didn't contain hsw, spi, etc., rebased my patches on top of it and pushed it. Now gh is trying to merge this to the head, which creates a mess, of course. Can you maybe just pull / cherry pick from that my branch?

plbossart · 2019-03-25T13:31:16Z

@plbossart I forked from an earlier commit that didn't contain hsw, spi, etc., rebased my patches on top of it and pushed it. Now gh is trying to merge this to the head, which creates a mess, of course. Can you maybe just pull / cherry pick from that my branch?

sorry no, I can only review and use the web interface this week. What's the issue with splitting the patches in a part that will apply on the v5 set and one that applies on top of topic/sof-dev?

lyakh · 2019-03-25T15:50:40Z

@plbossart I forked from an earlier commit that didn't contain hsw, spi, etc., rebased my patches on top of it and pushed it. Now gh is trying to merge this to the head, which creates a mess, of course. Can you maybe just pull / cherry pick from that my branch?

sorry no, I can only review and use the web interface this week. What's the issue with splitting the patches in a part that will apply on the v5 set and one that applies on top of topic/sof-dev?

If there were a branch somewhere with exactly v5 there would be no issue at all. But if I understood correctly I was supposed to make patches that apply "to a branch but not to its head but N patches before the head." That's what I've done here, but when pushing and making a PR the github produced a PR with 47 commits and a mountain of conflicts. Maybe I've done something wrong, but it really would have been much easier to have a clean branch with just v5 and nothing else in it.

plbossart · 2019-03-25T15:54:39Z

topic/sof-def-rebase branch, commit e92423f contains exactly the v5 patches.

lyakh · 2019-03-25T16:08:48Z

topic/sof-def-rebase branch, commit e92423f contains exactly the v5 patches.

right, and it's 24 commits behind the topic/sof-dev-rebase head, which is exactly the problem

lyakh · 2019-03-28T07:25:28Z

@plbossart how do we progress with this?

plbossart · 2019-03-28T12:56:08Z

@plbossart how do we progress with this?

I really don't get the issue. Split the patches in two, and make sure the first patches apply on top of v5, and the second batch on top of sof-dev, then submit the whole thing as one PR.

lyakh · 2019-03-28T13:51:10Z

@plbossart how do we progress with this?

I really don't get the issue. Split the patches in two, and make sure the first patches apply on top of v5, and the second batch on top of sof-dev, then submit the whole thing as one PR.

@plbossart ah, so you want me to split my patches into two lots so that the first one can apply to v5 and the second one applies on top of sof-dev+my first lot, and then (hopefully the first lot will also apply on top of sof-dev) apply both lots in that order on top of sof-dev and push to you. After that you'll again split them...

plbossart · 2019-03-28T14:09:51Z

@plbossart how do we progress with this?

I really don't get the issue. Split the patches in two, and make sure the first patches apply on top of v5, and the second batch on top of sof-dev, then submit the whole thing as one PR.

@plbossart ah, so you want me to split my patches into two lots so that the first one can apply to v5 and the second one applies on top of sof-dev+my first lot, and then (hopefully the first lot will also apply on top of sof-dev) apply both lots in that order on top of sof-dev and push to you. After that you'll again split them...

I would actually cherry-pick the first lot to push it as either incremental patches to alsa-devel if Mark merges the v5, or squash them into a v6 otherwise. You will save me time with this split, thanks in advance!

lyakh · 2019-03-28T14:54:21Z

@plbossart I think something went wrong with github links, both your references to c12f340 and e92423f point to a commit from 2017.

plbossart · 2019-03-28T14:57:25Z

@plbossart I think something went wrong with github links, both your references to c12f340 and e92423f point to a commit from 2017.

the commit are quite old, i've been squashing for 1.5 years now. It's the branch that matters.

lyakh · 2019-03-28T15:30:01Z

@plbossart I think this should do it now: the first 7 commits are for upstream, the remaining 5 are for sof-dev

plbossart

@lyakh the changes look good, thanks. I only have cosmetic comments and would like more consistent subjects with a prefix, and also split between hsw, skl, spi since they are also handled in 3 steps.

plbossart · 2019-03-28T17:19:56Z

@lyakh this comment still applies. ASoC: SOF: Intel prefix please, it makes things easier to track for reviewers (some of whom may not care about Intel specifics)

plbossart · 2019-03-28T17:22:09Z

comment on subject: ASoC: SOF: Intel again

plbossart · 2019-03-28T17:24:53Z

ASoC: SOF: ipc: for the subject line please

plbossart · 2019-03-28T17:27:02Z

ASoC: SOF: ipc prefix

plbossart · 2019-03-28T17:27:49Z

ASoC: SOF: core: prefix for subject, this isn't really related to the IPC.

plbossart · 2019-03-28T17:28:44Z

ASoC: SOF: hsw: simplify .get_reply() implementations

plbossart · 2019-03-28T17:30:38Z

I am probably getting on your nerves but even this needs to be split in 3. My topic/sof-def-rebase have haswell, SKL and SPI added in 3 separate steps.

dbaluta · 2019-03-28T20:36:38Z

So, besides removing the else clause your patch also modifies the format for dev_err. I think that should be the subject of another patch.

All SOF .get_reply() implementations follow the same pattern, which can be trivially simplified to remove an "else" clause. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Arguably for most humans sizes, printed out in decimal format are easier to read than in hexadecimal. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

sof_ipc_tx_msg_reply_complete() is a static function with two arguments, of which one isn't used, it only contains two lines and is only called once. Removing it makes the flow more readable. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

msg->msg_data is pre-allocated during SOF probing, if the allocation fails, the probing is aborted. Moreover, in hda-ipc.c that buffer is used before being checked. Remove the redundant test. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

The .get_reply() callback is always called by the IPC core immediately upon a wake-up from the platform IPC drived's interrupt handler. Therefore it can be directly called from the interrupt handler, avoiding the indirection. To make this possible the message has to be accessible from the "struct snd_sof_dev" instance and the .ipc_lock spin-lock has to be taken while manipulating the message. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

*_get_reply() functions on all platforms are very similar and they share the same flaw: in case of an error the response is read from the mailbox twice. This patch replaces the second of the two reads with a memcpy(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

.ipc_done() is always called after an IPC IRQ, either immediately or in a task, woken up by the IRQ. This can be simplified by directly executing the respective code from the IRQ thread. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

All SOF .get_reply() implementations follow the same pattern, which can be trivially simplified to remove an "else" clause. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Unlike addresses sizes are usually easier to read in decimal format. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

msg->msg_data is pre-allocated during SOF probing, if the allocation fails, the probing is aborted. Moreover, in hda-ipc.c that buffer is used before being checked. Remove the redundant test. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

The .get_reply() callback is always called by the IPC core immediately upon a wake-up from the platform IPC drived's interrupt handler. Therefore it can be directly called from the interrupt handler, avoiding the indirection. To make this possible the message has to be accessible from the "struct snd_sof_dev" instance and the .ipc_lock spin-lock has to be taken while manipulating the message. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

*_get_reply() functions on all platforms are very similar and they share the same flaw: in case of an error the response is read from the mailbox twice. This patch replaces the second of the two reads with a memcpy(). Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

.ipc_done() is always called after an IPC IRQ, either immediately or in a task, woken up by the IRQ. This can be simplified by directly executing the respective code from the IRQ thread. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2019-04-01T15:30:05Z

@plbossart I've got the next lot of IPC patches, basically just removing .mailbox_read() and .mailbox_write(), should I wait until this is merged or push on top?

plbossart

Thanks @lyakh, this looks good, will merge now. Please submit the next batch in a separate PR.

wenqingfu · 2019-04-02T13:32:23Z

regression reported on #773 narrowed down to this PR.

plbossart requested changes Mar 21, 2019

View reviewed changes

lgirdwood reviewed Mar 21, 2019

View reviewed changes

lyakh force-pushed the ipc-20190321 branch from 1809651 to 3603c98 Compare March 22, 2019 16:44

lyakh force-pushed the ipc-20190321 branch from 3603c98 to 6a95bd6 Compare March 28, 2019 15:28

plbossart requested changes Mar 28, 2019

View reviewed changes

dbaluta reviewed Mar 28, 2019

View reviewed changes

lyakh added 11 commits March 29, 2019 09:01

ASoC: SOF: Intel: simplify multiple .get_reply() implementations

bedc1d3

All SOF .get_reply() implementations follow the same pattern, which can be trivially simplified to remove an "else" clause. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ASoC: SOF: Intel: print sizes in decimal format

64b1050

Arguably for most humans sizes, printed out in decimal format are easier to read than in hexadecimal. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ASoC: SOF: hsw: simplify .get_reply() implementations

d435088

All SOF .get_reply() implementations follow the same pattern, which can be trivially simplified to remove an "else" clause. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ASoC: SOF: hsw: print sizes in decimal format

ae4e51e

Unlike addresses sizes are usually easier to read in decimal format. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh added 6 commits March 29, 2019 09:05

lyakh force-pushed the ipc-20190321 branch from 6a95bd6 to 5c78516 Compare March 29, 2019 08:11

plbossart approved these changes Apr 1, 2019

View reviewed changes

plbossart merged commit f93c4ee into thesofproject:topic/sof-dev Apr 1, 2019

lyakh deleted the ipc-20190321 branch April 2, 2019 06:15

Jiangxinx mentioned this pull request Apr 2, 2019

[BUG][S3] Regression test: Trying resume playback failed after suspend/resume on APL-pcm512x and APL-nocodec. #773

Closed

wenqingfu mentioned this pull request Apr 2, 2019

[WHL HDA] firmware boot failure on WHL platform. #767

Closed

Conversation

lyakh commented Mar 21, 2019

Uh oh!

plbossart 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!

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!

plbossart commented Mar 22, 2019 via email

Uh oh!

lyakh commented Mar 25, 2019

Uh oh!

plbossart commented Mar 25, 2019

Uh oh!

lyakh commented Mar 25, 2019

Uh oh!

plbossart commented Mar 25, 2019

Uh oh!

lyakh commented Mar 25, 2019

Uh oh!

lyakh commented Mar 28, 2019

Uh oh!

plbossart commented Mar 28, 2019

Uh oh!

lyakh commented Mar 28, 2019

Uh oh!

plbossart commented Mar 28, 2019

Uh oh!

lyakh commented Mar 28, 2019

Uh oh!

plbossart commented Mar 28, 2019

Uh oh!

lyakh commented Mar 28, 2019

Uh oh!

plbossart 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!

lyakh commented Apr 1, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment