ASoC: SOF: add sof vBE and vFE support by libinyang · Pull Request #142 · thesofproject/linux

libinyang · 2018-09-17T09:03:03Z

This patch adds the sof virtio BE and FE support.
It will create a dev node for the communication to the userspace.

It also handles the virtio vq event. It dispatches the different
vq kicks to different handlers. In the virtio vq handling, it handles
all the ipc events from vFE.

When there is position update from FW, it will check whether there is
an available entry in notification vq. If yes, send the position update
notification immediately. If there is no available entry, add the position
update event in a list. As soon as the notification vq has an available
entry, send the position update to the vFE.

For vFE, it will create a virtual audio device driver and communite with
vBE audio driver to create PCMs and playback/capture streams.

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

lgirdwood

Can you update C files where possible with runtime checks for vfe or vbe and resend (using same PR)

lgirdwood · 2018-09-17T10:00:24Z

Why do we need this ? If it's for tmp debug then lets put a big comment here stating it's tmp debug for feature XXX and will be removed.

This is a tmp solution for BE link. I will change it to a new solution with the new topology mode later.

lgirdwood · 2018-09-17T10:06:42Z

As suggested on V3, lets use a runtime sdev->is_vfe or sdev->is_vbe to select this code at runtime. These states will be set at probe() time.

Although we can use such flag to judge whether it is vFE or not, we still need such '#ifdef' because acrn has compile conflict as you know. We have already talked about that before. vFE and vBE must use such '#ifdef' to avoid the compiling error. After acrn has fixed these issues, I will switch to flag and remove the '#ifdef'

@libinyang the point I'm making is that we only use the ifdef in as few places as possible. To minimize the contamination.

lgirdwood · 2018-09-25T14:02:13Z

so this looks like a boolean ? fedev should be "is_vfe"

lgirdwood · 2018-09-25T14:14:41Z

So the above ifdef could be replaced with if (sdev->is_vbe) and then the assignement above could be static inline within the header VBE section. Just trying to eliminate all the ifdefs in common code and keeping it to header and one other place.

libinyang · 2018-10-08T08:41:17Z

@lgirdwood
updated, removed the ifdef which is caused by the conflict of FE and BE header files in ACRN.

lgirdwood

@libinyang getting close, but I need to see comments on each conditional piece of code so that anyone can understand each descision the code is making. Some funcs have this but some functions dont have any comments at all.

lgirdwood · 2018-10-08T15:18:14Z

I think we only need to do this if vBE support has been selected by kconfig ?

Yes. And I define static inline function to do nothing in the header file if vBE is not selected.

ok, change comment to "optionally register virtio miscdev "

I just need ?* optionally register virtio miscdev */ here, the remaining comment detail can go in sof_virtio_miscdev_register()

lgirdwood · 2018-10-08T15:24:04Z

do we always need to do this ? - we have three operating modes, VBE, VFE and standalone. sure we dont need to always do this ?

This function is also defined to update the position only in vBE. It will do nothing in standalone and vFE.

ok, change comment to "optionally update position for vBE". Also func prefix should be sof_vbe_

libinyang · 2018-10-09T05:54:11Z

@lgirdwood
updated:
Add more comments;
add sdev->is_vfe;
use goto xxxx

lgirdwood · 2018-10-11T09:45:42Z

Why do we need raw when this in in the type enum ? The requester would send a request SOF_IPC_TPLG_REQUEST and fill in the type, the replier would then reply with the data or an error.

This is added in your commit: aebf20cc in very-clean branch. What I guess is that we will use it later for FW transition. As We don't use it currently, I will remove it.

lgirdwood · 2018-10-11T10:03:24Z

ok, change comment to "optionally register virtio miscdev "

lgirdwood · 2018-10-11T10:05:03Z

ok, change comment to "optionally update position for vBE". Also func prefix should be sof_vbe_

libinyang · 2018-10-12T01:26:12Z

@lgirdwood

patch updated based on Liam's comments.

lgirdwood

Mainly cosmetic changes needed now.

lgirdwood · 2018-10-16T10:35:52Z

if (sdev->is_vfe)

Can this whole vFE init be placed in sof_virtio_vfe_init(sdev) and be placed after all the list heads are initialised.

yes, i will do it.

lgirdwood · 2018-10-16T10:36:36Z

This comment uses magic numbers whence we should be using macros

lgirdwood · 2018-10-16T10:41:55Z

keep the new line

lgirdwood · 2018-10-16T10:42:57Z

I just need ?* optionally register virtio miscdev */ here, the remaining comment detail can go in sof_virtio_miscdev_register()

lgirdwood · 2018-10-16T10:43:22Z

keep the new line

lgirdwood · 2018-10-16T11:02:06Z

dont return void *

lgirdwood · 2018-10-16T11:04:26Z

fix line length - which userspace API doe this use - Pls add link.

I don't understand this comment. What shall I do here? Make the line shorter?

lgirdwood · 2018-10-16T11:07:07Z

these static func declarations can be deleted if the functions are moved further up the file

lgirdwood · 2018-10-16T11:08:13Z

no need for the snd_ prefix as this is static and only used here.

ghost · 2018-10-16T11:08:30Z

snd_ not needed

ghost · 2018-10-17T08:39:06Z

@lgirdwood

updated based on Liam's comments

ghost

Still a few minor comments to cleanup.

ghost · 2018-10-17T10:06:58Z

A new line should always precede a comment to make it obvious where the comment applies and to break the code into easier to follow blocks. This need fixed in several places.

ghost · 2018-10-17T10:07:12Z

ghost · 2018-10-17T10:08:03Z

indentation of variable names - some members use a tab and others a space in this struct.

got it. I misunderstood your meaning last time.

ghost · 2018-10-17T10:10:39Z

pls comment what this function does

ghost · 2018-10-17T10:11:39Z

no dev is available here

ghost · 2018-10-17T10:13:35Z

system includes should precede local includes

This patch adds the sof virtio BE and FE support. It will create a dev node for the communication to the userspace. It also handles the virtio vq event. It dispatches the different vq kicks to different handlers. In the virtio vq handling, it handles all the ipc events from vFE. When there is position update from FW, it will check whether there is an available entry in notification vq. If yes, send the position update notification immediately. If there is no available entry, add the position update event in a list. As soon as the notification vq has an available entry, send the position update to the vFE. For vFE, it will create a virtual audio device driver and communite with vBE audio driver to create PCMs and playback/capture streams. Signed-off-by: Libin Yang <libin.yang@intel.com>

ghost · 2018-10-18T06:30:32Z

@lgirdwood
updated based on the comments

libinyang requested review from lgirdwood and plbossart September 17, 2018 09:03

lgirdwood requested changes Sep 17, 2018

View reviewed changes

lgirdwood requested changes Sep 25, 2018

View reviewed changes

libinyang mentioned this pull request Sep 29, 2018

vBE patches v2 #105

Closed

libinyang force-pushed the porting_4 branch from 4ac8f37 to fa2bd85 Compare October 8, 2018 08:38

lgirdwood requested changes Oct 8, 2018

View reviewed changes

libinyang force-pushed the porting_4 branch from fa2bd85 to de846a5 Compare October 9, 2018 05:51

lgirdwood requested changes Oct 11, 2018

View reviewed changes

libinyang force-pushed the porting_4 branch from de846a5 to 3d1644f Compare October 12, 2018 01:18

lgirdwood requested changes Oct 16, 2018

View reviewed changes

libinyang force-pushed the porting_4 branch from 3d1644f to 69d2198 Compare October 17, 2018 08:35

lgirdwood requested changes Oct 17, 2018

View reviewed changes

libinyang force-pushed the porting_4 branch from 69d2198 to 60d7e21 Compare October 18, 2018 06:29

lgirdwood merged commit 481973c into thesofproject:topic/sof-dev Oct 18, 2018

Conversation

libinyang commented Sep 17, 2018

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!

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 Oct 8, 2018

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!

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!

Uh oh!

Uh oh!

Uh oh!

libinyang commented Oct 9, 2018

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!

libinyang commented Oct 12, 2018

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!

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