Upstream: Patch 5/14: pcm: need comments on non-atomic trigger

[plbossart]

need to explain why the trigger is not atomic (and that it's based on FEs with .nonatomic=true)

[ranj063]

@plbossart I think that the whole pcm trigger in SOF is non-atomic and the reason is that we use the ipc to trigger the operation which is a schedulable call and is not atomic.

But we do not explicitly set nonatomic to true for the FE's. Should we do that?

Furthermore, I just checked the machine driver and Isee that all the FE dai defs there set nonatomic to 1.

[plbossart]

@plbossart <https://github.com/plbossart> I think that the pcm trigger in SOF is non-atomic and the reason is that we use the ipc to trigger the operation which is a schedulable call and is not atomic. But we do not explicitly set nonatomic to true for the FE's. Should we do that?

I wonder what we really do. I see this in the code: topology.c:     link->nonatomic = true; @plbossart oh I missed this. Yes this sets nonatomic to true for all dai links. So FE dai links would be fine. But i wonder why we do this for BE dai links. But it seems to apply to a link (which makes zero sense). And indeed we don't signal nonatomic operation for FEs. This looks counter-intuitive? Probably for @lgirdwood to comment here, no idea why things are done this way. @lgirdwood please shed some light one this. Thanks!

[libinyang]

I think Pierre is right. We do set the nonatomic in the FE. This will make sure that alsa core will call our functions in nonatomic environment.
In soc-pcm, it will set snd_pcm->nonatomic = rtd->dai_link->nonatomic.
In alsa core pcm_native.c, it will check snd_pcm->nonatomic to determine whether use spin_lock or not.
And alsa uses our FE snd_pcm as the interface. So I believe it is safe for our cases. And just adding some comments like Takashi suggested should be enough.

[plbossart]

@libinyang thanks for looking into this. So we are covered for FEs, but as Ranjani mentions it I don't get what happens for BEs. It's supposed to be atomic at that level, at least if you look at the previous solutions with closed source firmware all BEs where configured with .nonatomic=false (implicitly).

[libinyang]

For BE, let me use hw_params as an example, I believe other are the same.

For dynamic link, dpcm_fe_dai_hw_params() is the interface. ALSA core will call this function, and then the dpcm_fe_dai_hw_params() will call dpcm_be_dai_hw_params(). This means BE hw_params is called by the FE hw_params. If we can make sure FE is nonatomic, then BE should be nonatomic too.

[plbossart]

@libinyang if you look at the machine drivers for all previous solutions based on closed-source firmware, the BEs are always marked as .nonatomic=false. So why would SOF require non-atomic operation when others using the same hardware did not? Either they are all wrong or we are.

[libinyang]

@plbossart
I grepped our source code, and didn't find the setting .nonatomic=false. Maybe the old code is not merged into our branch?
However, closed-source firmware may use a different solution. From our current code, we may sleep in our sof code, so we use .nonatomic=true. Maybe closed source doesn't sleep when ALSA core calls them?
And besides, for our SOF code, we use FE to expose PCM to the userspace. This means user will operate on FE directly in alsa core level. And ASoC will call BE functions when operating on FE callbacks. I'm not sure how closed source's behavior. Maybe @lgirdwood know the story.

[plbossart]

On 12/16/18 11:37 PM, Libin Yang wrote: @plbossart <https://github.com/plbossart> I grepped our source code, and didn't find the setting .nonatomic=false. Maybe the old code is not merged into our branch? However, closed-source firmware may use a different solution. From our current code, we may sleep in our sof code, so we use .nonatomic=true. Maybe closed source doesn't sleep when ALSA core calls them? And besides, for our SOF code, we use FE to expose PCM to the userspace. This means user will operate on FE directly in alsa core level. And ASoC will call BE functions when operating on FE callbacks. I'm not sure how closed source's behavior. Maybe @lgirdwood <https://github.com/lgirdwood> know the story.

when .nonatomic is not set, so it defaults to  false. It's just standard C99 behavior and there is no missing code