ASoC: sof: add comments for non-atomic pcm trigger #506

libinyang · 2019-01-07T05:18:17Z

This fixes #421

Add the comments in pcm trigger to explain non-atomic is
set in topology parsing. So it is safe to call
sof_ipc_tx_message() in the trigger callback.

Signed-off-by: Libin Yang libin.yang@intel.com

mengdonglin · 2019-01-07T06:23:31Z

This is to fix the open #421

ranj063 · 2019-01-07T19:22:18Z

sound/soc/sof/pcm.c

@libinyang the comment here looks very ambiguous. A better place would be just a few lines above where we return 0 for BE:
/* nothing todo for BE */

We could call out that the trigger is purely for FE's and that the non-atomic field for FE's is explicitly set to true.

@libinyang The ask was to clarify when we use non-atomic and if there are any differences with the other drivers. The current behavior for other drivers is non-atomic for FEs only

@ranj063
Actually, it is not related to FE/BE trigger. Both FE/BE trigger will be in non-atomic environment in our case.

@plbossart
Do you mean I should add the comment about why FE is non-atomic will cause the trigger non-atomic? I think this is a alsa/asoc level general question. I'm afraid that the maintainers may be not interested in this? For the comments, I believe we just need to explain why our code is OK. We don't need to explain what other drivers flow. What do you think?
From Takashi's comments, my understanding is Takashi just want to make sure trigger is in non-atomic environment, and when it is set. Maybe I misunderstand Takashi's meaning. If you think we need to explain why setting non-atomic in FE can make sure the trigger is in non-atomic environment, I will add the details.

I just want to clarify what assumptions are made for FE and BE and check that this is aligned with the previous solutions. not sure what's so complicated?

@plbossart
I think I got you now. Do you want the clarify in the comments of the code or the description header of the patch?

Today, I will work on hdmi audio patch firstly. Maybe the comments are ready tomorrow.

libinyang · 2019-01-11T00:03:39Z

@plbossart
patch is updated for non-atomic comments.

ranj063 · 2019-01-11T07:55:04Z

sound/soc/sof/pcm.c

@libinyang this is too confusing to read. I think it is already quite clear that the trigger is in non-atomic context. The information we need to clarify is:

The SOF driver sets nonatomic to true for both BE and FE dai links.
But the legacy driver only sets nonatomic for FE dai links and not for BE dai links. Why is this different?

@libinyang I think it is meaningless to set BE dai link to nonatomic, which is what we do now. I think we should change that and then clarify here that FE dai links are set to non-atomic because the trigger is always in non-atomic context.

@libinyang I think it is meaningless to set BE dai link to nonatomic, which is what we do now. I think we should change that and then clarify here that FE dai links are set to non-atomic because the trigger is always in non-atomic context.

@ranj063
Yes, you got it :)
Do we set nonatomic for BE? I didn't find we define nonatomic for BE. Anyway, it is OK for us to define nonatomic for BE because it will be ignored whatever it is set.

plbossart · 2019-01-11T16:29:25Z

@libinyang <https://github.com/libinyang> this is too confusing to read. I think it is already quite clear that the trigger is in the non-atomic context. The information we need to clarify is: The SOF driver sets nonatomic to true for both BE and FE dai links. But the legacy driver only sets nonatomic for FE dai links and not for BE dai links. Why is this different?

Agree with Ranjani, her write-up is what I asked for initially. And in addition to comments we'd want actual fixes since we seem to do the opposite of what everyone else does.

libinyang · 2019-01-12T00:40:11Z

@libinyang https://github.com/libinyang this is too confusing to read. I think it is already quite clear that the trigger is in the non-atomic context. The information we need to clarify is: The SOF driver sets nonatomic to true for both BE and FE dai links. But the legacy driver only sets nonatomic for FE dai links and not for BE dai links. Why is this different?
Agree with Ranjani, her write-up is what I asked for initially. And in addition to comments we'd want actual fixes since we seem to do the opposite of what everyone else does.

@plbossart
Do we set nonatomic for BE? I'm afraid not. We only set FE as nonatomic. And please my updated patch, I have explained why set for FE is enough.

ranj063 · 2019-01-12T00:53:23Z

@libinyang https://github.com/libinyang this is too confusing to read. I think it is already quite clear that the trigger is in the non-atomic context. The information we need to clarify is: The SOF driver sets nonatomic to true for both BE and FE dai links. But the legacy driver only sets nonatomic for FE dai links and not for BE dai links. Why is this different?
Agree with Ranjani, her write-up is what I asked for initially. And in addition to comments we'd want actual fixes since we seem to do the opposite of what everyone else does.

@plbossart
Do we set nonatomic for BE? I'm afraid not. We only set FE as nonatomic. And please my updated patch, I have explained why set for FE is enough.

Yes, we do set it for BE's.
take a look at the line 2349 in topology.c in sof_link_load():
link->nonatomic = true;

libinyang · 2019-01-12T01:12:51Z

Yes, we do set it for BE's.
take a look at the line 2349 in topology.c in sof_link_load():
link->nonatomic = true;

@ranj063
Yes, it is set here :). However, it is OK to set nonatomic. It doesn't matter.

ranj063 · 2019-01-12T01:30:00Z

@libinyang maybe it doesnt matter but it cant be explained. So better to remove it and then mention that FE trigger is always in non-atomic context.

libinyang · 2019-01-14T01:19:27Z

@libinyang maybe it doesnt matter but it cant be explained. So better to remove it and then mention that FE trigger is always in non-atomic context.

@ranj063 If we remove it, it means we will set BE as atomic. Is there any benifit to set BE as atomic? I'm OK to set BE as atomic, but maybe it is not necessary to do it. What do you think?

ranj063 · 2019-01-14T01:46:42Z

@libinyang if we remove it, it doesnt mean we are setting it to atomic, we're just not setting it to non-atomic. Anyway, if you look at the FW dai link definitions in the legacy drivers, we explicitly set nonatomic to true only for FE's and not BE dai links. At least this way we'll be consistent with them.

libinyang · 2019-01-14T01:49:43Z

@libinyang if we remove it, it doesnt mean we are setting it to atomic, we're just not setting it to non-atomic. Anyway, if you look at the FW dai link definitions in the legacy drivers, we explicitly set nonatomic to true only for FE's and not BE dai links. At least this way we'll be consistent with them.

@ranj063
As Pierre mentioned, if you don't set nonatomic, it means nonatomic is default to 0, this means it nonatomic is false :)
We don't need to make SOF be the same as cAVS. Actually, I think our SOF is better than cAVS in the nonatomic setting ;-)

ranj063 · 2019-01-14T02:16:31Z

@libinyang I think the expectation for this PR was that we explain whatever we are doing properly. If you want to set BE dai link to non-atomic, thats fine. Just please make sure you can back it up with a proper explanation.

libinyang · 2019-01-14T02:20:12Z

@libinyang I think the expectation for this PR was that we explain whatever we are doing properly. If you want to set BE dai link to non-atomic, thats fine. Just please make sure you can back it up with a proper explanation.

@ranj063 I understand. If both you and Pierre are OK, I will remove the nonatomic setting for BE. I just think it is not necessary to do that.

ranj063 · 2019-01-14T02:26:19Z

@libinyang if I did this PR, i'd set the non-atomic only for FE's dai links and then in the trigger callback, i'd make it clear that the trigger is always in non-atomic context. This way, a reviewer will know to connect them both.

libinyang · 2019-01-14T03:00:15Z

@ranj063
OK. I will update the patch later. Maybe today or tomorrow after my test on the new patch.

libinyang · 2019-01-16T05:45:39Z

@plbossart

I have refined the patch based on your requirement. Could you please help review if it is OK?

plbossart

Sorry, more work needed on the wording. English isn't the most straightforward language so it helps to keep the explanations logical and simple.

plbossart · 2019-01-16T20:16:58Z

sound/soc/sof/topology.c

FE trigger is not executed in an atomic environment.?

BE->no_pcm is true and FE->no_pcm is false. So in our case (!link->no_pcm), FE will be true and will be set nonatomic = 1;

And in FE trigger, it is not executed in an atomic environment because sof_ipc_tx_message() needs to be in non-atomic environment.

@libinyang Pierre's only asking to re-word your comment to
" FE trigger is not executed in atomic context". not asking for an explanation

@libinyang Pierre's only asking to re-word your comment to
" FE trigger is not executed in atomic context". not asking for an explanation

@ranj063 @plbossart
I understand your means...
Sorry for my poor understanding...

plbossart · 2019-01-16T20:19:44Z

sound/soc/sof/pcm.c

Sorry @libinyang, I find your answers convoluted. I was expecting something like

The FE can be triggered in a non-atomic environment because ..... The non-atomic property is set in the DAI link and copied into the PCM fields.

The BE link does not set the non-atomic property because ...

Got it. I will change the comments.

plbossart

The comments are not quite what I was expecting. I'd like code reviewers to have a clear explanation as to why the FE trigger will happen in a non-atomic context and the BE trigger will happen in a atomic one.
In other words, I am not asking for how the .non-atomic information is propagated from dailink to PCM but WHY it's set in the first place.

plbossart · 2019-01-17T16:51:20Z

sound/soc/sof/pcm.c

Sorry @libinyang, you are describing the actions when the .non-atomic field is set in the dailink but not the rationale behind setting this .non-atomic field.
Again we need to explain WHY the FE needs this .non-atomic field set and the BE does not.

How about explaining "WHY FE PCM is set .non-atomic" in topology.c, and just mention here that PCMs are .non-atomic so sending IPC here is safe?

@plbossart
Really thanks for your patient on my patch. I will sync with Ranjani and Keyon firstly and update the patch later. Hope this can save your time and speed up this patch merging process. Thanks.

Add the comments to why explain non-atomic is set in topology parsing. So it is safe to call sof_ipc_tx_message() in the trigger callback. Only FE need nonatomic = 1 Signed-off-by: Libin Yang <libin.yang@intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

libinyang · 2019-01-22T05:45:12Z

@plbossart
Patch is updated. Thanks.

plbossart · 2019-01-22T17:10:27Z

allow me to double-check the logic:
We set the FEs as non-atomic because they involve IPC
So we set the BEs as atomic because they don't involve IPCs.
Is the latter point correct?

ranj063 · 2019-01-22T17:12:14Z

allow me to double-check the logic:
We set the FEs as non-atomic because they involve IPC
So we set the BEs as atomic because they don't involve IPCs.
Is the latter point correct?

@plbossart yes, that correct. We return after doing nothing in the trigger routine for BE dai links. So it is safe for it to be atomic.

libinyang · 2019-01-23T01:45:53Z

BE will do nothing for the pcm action.

plbossart

ok let's merge.

libinyang requested review from bardliao and plbossart January 7, 2019 05:18

ranj063 reviewed Jan 7, 2019

View reviewed changes

libinyang force-pushed the non-atomic_comments branch from cf2913c to c998df2 Compare January 10, 2019 13:23

ranj063 reviewed Jan 11, 2019

View reviewed changes

libinyang force-pushed the non-atomic_comments branch from c998df2 to befc2f8 Compare January 15, 2019 06:22

plbossart reviewed Jan 16, 2019

View reviewed changes

libinyang force-pushed the non-atomic_comments branch 3 times, most recently from 955e640 to d97dbd1 Compare January 17, 2019 07:05

plbossart reviewed Jan 17, 2019

View reviewed changes

libinyang force-pushed the non-atomic_comments branch from d97dbd1 to f375e19 Compare January 22, 2019 05:43

plbossart approved these changes Jan 23, 2019

View reviewed changes

plbossart merged commit 4b60986 into thesofproject:topic/sof-dev Jan 23, 2019

ASoC: sof: add comments for non-atomic pcm trigger #506

ASoC: sof: add comments for non-atomic pcm trigger #506

Uh oh!

Conversation

libinyang commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mengdonglin commented Jan 7, 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!

libinyang Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

libinyang commented Jan 11, 2019

Uh oh!

ranj063 Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 11, 2019 via email

Uh oh!

libinyang commented Jan 12, 2019

Uh oh!

ranj063 commented Jan 12, 2019

Uh oh!

libinyang commented Jan 12, 2019

Uh oh!

ranj063 commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

libinyang commented Jan 14, 2019

Uh oh!

ranj063 commented Jan 14, 2019

Uh oh!

libinyang commented Jan 14, 2019

Uh oh!

ranj063 commented Jan 14, 2019

Uh oh!

libinyang commented Jan 14, 2019

Uh oh!

ranj063 commented Jan 14, 2019

Uh oh!

libinyang commented Jan 14, 2019

Uh oh!

libinyang commented Jan 16, 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!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

libinyang commented Jan 7, 2019 •

edited

Loading

libinyang Jan 8, 2019 •

edited

Loading

ranj063 Jan 11, 2019 •

edited

Loading

ranj063 commented Jan 12, 2019 •

edited

Loading