Page MenuHomeFreeBSD

HD Audio Emulation For Bhyve (Based on D7840)
AbandonedPublic

Authored by araujo on Sep 20 2017, 7:48 AM.

Details

Reviewers
mav
rgrimes
Group Reviewers
bhyve
Summary
  • Just fixed some style(9).
  • Add BSD License and a proper copyright for the author.

Note: I did open another review because I don't want to command the review D7840.

Test Plan
  • Tested with HEAD r323795.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 11776
Build 12120: arc lint + arc unit

Event Timeline

araujo created this revision.Sep 20 2017, 7:48 AM

There are a bunch of minor style(9) issues.

I was able to apply the diff and build cleanly. However, with an 11.1/amd64 guest and using mpg123 to play an MP3 file, I had an unimplemented instruction exit - I'll look more into this.

usr.sbin/bhyve/audio.c
64

Not strict style(9) - variables declarations don't have initializers.

70

style(9) "Values in return statements should be enclosed in parentheses"

usr.sbin/bhyve/hda_codec.c
144

See style(9) on sorting struct elements.

870

No need to cast a void *

921

No error return to assert on here.

941

Not in style(9), but the FreeBSD convention for function pointers is to make it explicit with a dereference e.g. use (*actx->do_setup)(actx->priv)

usr.sbin/bhyve/hdac_reg.h
270

style(9), should be '#endif /* _HDAC_REG_H_ */

usr.sbin/bhyve/pci_hda.c
297

No need for return statements in void routines.

araujo updated this revision to Diff 33479.Sep 27 2017, 5:02 AM
  • Address grehan@ request about some minor style(9) issues all over the place.
  • Some structs were sorted by use and some by size and in alphabetical order.
  • Remove the return from void functions.
usr.sbin/bhyve/audio.c
64

There is nothing wrong to initialize the variable, we can do it explicit or let the compiler does it for us.

usr.sbin/bhyve/hda_codec.c
921

Actually that assert should not be there according with pthread_set_name_np(3).

I disabled capsicum by commenting out the cap_enter() call, and with the above struct revert, was able to cat a .WAV file and also use mpg123 to play a long mp3 file on a FreeBSD 11.1/amd64 guest, with a cheap USB audio adapter for output. Volume control seemed to work fine with mpg123. There was a very small glitch/skip when the audio was first played (or on resume after pause in mpg123).

The instruction-emlation patch for the OR instruction is at https://people.freebsd.org/~grehan/patches/vmm_instruction_emul.c.OR.diff

usr.sbin/bhyve/pci_hda.c
79

I should have added that struct element ordering only applies for internal-use structs, and not ones that are used to represent in-memory device-emulation data structures (in this case, the HDA BDL struct).

This one has to go back to the original ordering. You may want to have a look at any others that were re-ordered in case they were used for HDA device data structures.

grehan added a comment.Nov 1 2017, 3:29 AM

The OR emulation has been checked in with r325261

I've tested with Rhythmbox and browsers on Ubuntu 17.10, Edge and Skype (full audio call, audio in/out via HDA) on Win10.

Just need to add some Capsicum-ization and this will be good to go.

grehan added a comment.Nov 1 2017, 3:29 AM

The OR emulation has been checked in with r325261

I've tested with Rhythmbox and browsers on Ubuntu 17.10, Edge and Skype (full audio call, audio in/out via HDA) on Win10.

Just need to add some Capsicum-ization and this will be good to go.

Can this be merged in already? It's 2018....

Needs capsicum patches or it won't work (testing was done with Capsicum disabled).

Oooops! I completely forgot this patch, I will add capsicum this week.
My bad!

novel added a subscriber: novel.May 14 2018, 6:37 AM
mmacy added a subscriber: mmacy.Jul 15 2018, 9:47 PM

Any update on this? I've tried this diff on a recent current but it seems like it doesn't work anymore?

Any update on this? I've tried this diff on a recent current but it seems like it doesn't work anymore?

I will come back to this patch soon. I'm waiting some capsicum patches do land. I didn't have enough time to check if those were committed or not.

emaste added a subscriber: emaste.Oct 4 2018, 1:35 PM

Just a hands up, this patch needs the following patches:

Committed already: https://reviews.freebsd.org/D14557
Pending: https://reviews.freebsd.org/D14407

Hi @araujo, are there plans to move forward with this now that https://reviews.freebsd.org/rS340373 (libcasper: introduce cap_fileargs service) was commited?

araujo added a comment.Dec 2 2018, 8:56 AM

Hi @araujo, are there plans to move forward with this now that https://reviews.freebsd.org/rS340373 (libcasper: introduce cap_fileargs service) was commited?

Yes, definitely! I'm working on it, I got some issues with cap_fileargs and I have asked Mariuz support to help me debug the issue. But yeah, I'm on it right now!

Hi @araujo, are there plans to move forward with this now that https://reviews.freebsd.org/rS340373 (libcasper: introduce cap_fileargs service) was commited?

Yes, definitely! I'm working on it, I got some issues with cap_fileargs and I have asked Mariuz support to help me debug the issue. But yeah, I'm on it right now!

I have implemented cap_fileargs(3) and now the driver is in capabilities mode and works relatively fine.
Still there are some adjustments that I need to make and I hope till next week I will update this review with the new code.

rgrimes removed a reviewer: grehan.Feb 20 2019, 5:55 PM
araujo abandoned this revision.Feb 22 2019, 1:25 AM

If anybody else is interested on this patch, ping me and I can share my latest changes. However I'm not interested on it anymore.

If anybody else is interested on this patch, ping me and I can share my latest changes. However I'm not interested on it anymore.

I am interested seeing this patch hit bhyve main.

If anybody else is interested on this patch, ping me and I can share my latest changes. However I'm not interested on it anymore.

I am interested seeing this patch hit bhyve main.

Yes! Please do share your latest changes!

yvyz_protonmail.com added a comment.EditedMar 12 2019, 6:49 PM

If anybody else is interested on this patch, ping me and I can share my latest changes. However I'm not interested on it anymore.

I am interested seeing this patch hit bhyve main.

Yes! Please do share your latest changes!

For anybody interested in this: This patch correctly applies to 12.0-Release-Stable.

I would like to see more development on this as it provides a native bandaid to the lack of WideVine DRM/EME support.

lme added a comment.EditedJun 4 2019, 3:21 PM

Is anybody still working on this?

As far as I can see it's ready to get committed. So let's get this into bhyve after all these years...

scottl added a subscriber: scottl.Jun 21 2019, 7:46 PM

Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.

Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.

Please read the whole review, Peter Grehan points out several times that it has capsicum issues, and that it has only been shown to work with Capsicum disabled.