ASoC: SOF: Don't read reply if its not used by cujomalainey · Pull Request #3925 · thesofproject/linux

cujomalainey · 2022-10-14T00:10:46Z

Most uses of the IPC TX reply only care about the return code which can be returned without passing a stack allocated struct. Let's save those copy cycles.

Signed-off-by: Curtis Malainey cujomalainey@chromium.org

ujfalusi

I would be tempted to change the subject to "ASoC: SOF: ipc3: ..." as this change only affects IPC3 code path.

cujomalainey · 2022-10-14T14:53:19Z

I would be tempted to change the subject to "ASoC: SOF: ipc3: ..." as this change only affects IPC3 code path.

That isn't quite correct, the majority are, but there are changes in the compress code as well as the flood test.

cujomalainey · 2022-10-14T20:32:57Z

Found a couple more replies that could be removed and added them to the chain.

cujomalainey · 2022-10-14T20:39:48Z

Apologies for the rapid pushes, fixed a typo that then introduced a change-id

plbossart · 2022-10-26T00:43:37Z

quite a few build issues @cujomalainey

cujomalainey · 2022-10-28T03:52:51Z

quite a few build issues @cujomalainey

Yea saw that, sorry for the delay, was out sick Monday/Tuesday, trying to catch up

cujomalainey · 2022-11-02T00:08:16Z

Ah forgot the inline keyword. Will push a fix

cujomalainey · 2022-11-02T00:36:49Z

FYI I cannot open the checkpatch bot as I am getting SSL errors.

cujomalainey · 2022-11-03T21:08:09Z

@plbossart can we do a review? The longer the delay the likelyhood of more uses slipping in that the patches won't catch, my fix and rebase already caught 2 more.

plbossart · 2022-11-03T22:36:12Z

I'll be honest @cujomalainey, we have too many PRs in flight:
#3984
#3888
#3986
#3933
#3978
CI is undergoing a painful migration and a number of unfortunate regressions.
And we have a number of really important PRs
#3972
#3962
#3953

Your PR makes complete sense but we have to be realistic on the urgency/priority.
Thanks for your understanding.

cujomalainey · 2022-11-03T22:42:40Z

@plbossart understood, thanks for the update. Let me know when things calm down, I'll rebase to make sure I get any new cases then re-review.

cujomalainey · 2022-11-22T22:15:39Z

@plbossart just wanted to let you know I rebased these so if things have calmed down on your end they should be able to go in

iuliana-prodan

LGT, but can't we use only patches 2 and 3 - patch 1 is modified by 3?
Add no reply calls and use them where reply is "thrown away".

cujomalainey · 2022-11-28T23:23:02Z

@iuliana-prodan It was more to keep the conceptually isolated since the first was produced on its own and is clean up, the other two are the prevention

95% of the calls inside SOF to TX an IPC don't care about a reply. Yet the previous commit cleaned up a bunch of replies that were being populated and then thrown away. This adds some functions so users who do not need replies don't feel obligated to provide the space to the API. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

Convert all existing calls that pass "NULL, 0" for reply data to the new no_reply calls. Also convert any calls that pass in data but don't actually parse the result. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

plbossart · 2022-12-02T19:26:46Z

@ujfalusi @bardliao @ranj063 can you take a look at this PR please?

ranj063 · 2022-12-02T20:52:06Z


 int sof_client_ipc_tx_message(struct sof_client_dev *cdev, void *ipc_msg,
 void *reply_data, size_t reply_bytes);
+static inline int sof_client_ipc_tx_message_no_reply(struct sof_client_dev *cdev, void *ipc_msg)


@cujomalainey the change itself is OK but i think the name with the no_reply suffix is a bit misleading which seems to indicate that there's no reply for this IPC TX. It's not that there's no reply but that there's no data expected with the reply right?

No the concept is that we don't care about the reply, only the return code. I.e don't fetch the reply data. Name was recommended by @plbossart

@cujomalainey the return code that you mention here is part of the reply isnt it?

Technically yes, but you do not need to fetch the data to get the return code. See error code fetch here but the copy out check is down here and this is what we are skipping

Here is the ipc4 equivalent code

cujomalainey requested review from ajitkupandey, bardliao, dbaluta, kuanhsuncheng, kv2019i, lgirdwood, libinyang, plbossart, ranj063, ujfalusi and yaochunhung as code owners October 14, 2022 00:10

plbossart reviewed Oct 14, 2022
View reviewed changes

Comment threadsound/soc/sof/compress.c Outdated

ujfalusi reviewed Oct 14, 2022
View reviewed changes

cujomalainey requested review from plbossart and ujfalusi October 14, 2022 20:27

cujomalainey force-pushed the reply branch from e439538 to 889e5ebCompare October 14, 2022 20:32

cujomalainey requested review from juimonen and lyakh as code owners October 14, 2022 20:32

cujomalainey force-pushed the reply branch 2 times, most recently from 64eca90 to 1cbdfacCompare October 14, 2022 20:39

cujomalainey force-pushed the reply branch 2 times, most recently from a5cd7d4 to e1e9446Compare November 2, 2022 00:20

cujomalainey force-pushed the reply branch from e1e9446 to b6786afCompare November 2, 2022 00:41

cujomalainey force-pushed the reply branch from b6786af to c92cd5dCompare November 22, 2022 22:15

iuliana-prodan reviewed Nov 23, 2022
View reviewed changes

cujomalainey added 2 commits November 28, 2022 15:33

ASoC: SOF: Use no_reply calls for TX
364ec09
Convert all existing calls that pass "NULL, 0" for reply data to the new no_reply calls. Also convert any calls that pass in data but don't actually parse the result. Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>

cujomalainey force-pushed the reply branch from c92cd5d to 364ec09Compare November 28, 2022 23:45

cujomalainey requested a review from iuliana-prodan November 28, 2022 23:50

plbossart approved these changes Nov 29, 2022
View reviewed changes

ranj063 reviewed Dec 2, 2022
View reviewed changes

iuliana-prodan approved these changes Dec 5, 2022
View reviewed changes

bardliao approved these changes Dec 5, 2022
View reviewed changes

ujfalusi approved these changes Dec 5, 2022
View reviewed changes

ranj063 approved these changes Dec 5, 2022
View reviewed changes

cujomalainey merged commit 3210285 into thesofproject:topic/sof-devDec 5, 2022

cujomalainey deleted the reply branch December 5, 2022 18:53

cujomalainey mentioned this pull request Dec 5, 2022
[SQUASHME] Into "ASoC: SOF: Use no_reply calls for TX" #4075
Merged

Uh oh!

Conversation

cujomalainey commented Oct 14, 2022

Uh oh!

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Oct 14, 2022

Uh oh!

cujomalainey commented Oct 14, 2022

Uh oh!

cujomalainey commented Oct 14, 2022

Uh oh!

plbossart commented Oct 26, 2022

Uh oh!

cujomalainey commented Oct 28, 2022

Uh oh!

cujomalainey commented Nov 2, 2022

Uh oh!

cujomalainey commented Nov 2, 2022

Uh oh!

cujomalainey commented Nov 3, 2022

Uh oh!

plbossart commented Nov 3, 2022

Uh oh!

cujomalainey commented Nov 3, 2022

Uh oh!

cujomalainey commented Nov 22, 2022

Uh oh!

iuliana-prodan left a comment

Choose a reason for hiding this comment

Uh oh!

cujomalainey commented Nov 28, 2022

Uh oh!

plbossart commented Dec 2, 2022

Uh oh!

ranj063Dec 2, 2022

Choose a reason for hiding this comment

Uh oh!

cujomalaineyDec 2, 2022

Choose a reason for hiding this comment

Uh oh!

ranj063Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

cujomalaineyDec 4, 2022

Choose a reason for hiding this comment

Uh oh!

cujomalaineyDec 4, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants