-
Notifications
You must be signed in to change notification settings - Fork 140
Support BT audio offload on HDA machines #5123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You define #define BT_DAI_COUNT 1, INHO, it is better to use num_links -= BT_DAI_COUNT in case BT_DAI_COUNT is changed one day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
d197f34 to
6960b82
Compare
include/sound/soc-acpi.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code appends -bt-ssp%d tot he topology filename...
include/sound/soc-acpi.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a documented rule on what order these appends should be done, I believe they must be always <bit(0)><bit(1)>...<bit(last)> ?
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to initialize to 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!bt_link_mask_override) { ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New code should use %#x for hexadecimal numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be hard pressed to guess based on the number what device is detected..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a novel way, but I would rather not set this string as initial name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dai_name comes from topology and in a form of a magic string also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is from here:
SND_SOC_DAILINK_DEF(bt_offload_pin,
DAILINK_COMP_ARRAY(COMP_CPU("SSP%d Pin")));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this is going to break a lot of system presumably since we don not have any -bt-ssp topology out there..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all tplg name fixup code and let OSV to decide so we don't break all HDA PC...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed all tplg name fixup code and let OSV to decide so we don't break all HDA PC...
The problem is that we don't know who the OSV is, and we don't know how to distribute the required topology. So while the changes look ok at a high-level, who's going to use all this?
I don't want to merge something that creates a headache for support teams. We don't have a plan that makes any sense at the moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I should provide the background story instead of just shooting the patches.
Currently there is a Chrome reference design brox which enables HDA external codec. Since BT audio offload was enabled for I2S Chromebooks, we want this feature could be also supported for HDA Chromebooks as well.
For the kernel driver, we will backport these two patches to Chrome tree and update the module option since Chromebook does not support NHLT table. And for topology, we will backport the patch to cavs2.5 branch and request Google to release the topology.
146498b to
5ed91b9
Compare
plbossart
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good now @brentlu, that's an acceptable solution that allows for new functionality without breaking older setups. Thanks for taking this initiative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot if those flags still exist or have been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet removed. still doing some cleanups
https://patchwork.kernel.org/project/alsa-devel/list/?series=874792
Add an new variable bt_link_mask to snd_soc_acpi_mach_params structure. SSP port mask of BT offload found in NHLT table will be sent to machine driver to setup BE dai link with correct SSP port number. This patch only detects and enables the BT dailink. The functionality will only be unlocked with a topology file that makes a reference to that BT dailink. For backwards-compatibility reasons, this topology will not be used by default. Chromebooks and Linux users willing to experiment shall use the tplg_name kernel parameter to force the use of an enhanced topology. Signed-off-by: Brent Lu <brent.lu@intel.com>
Add BT offload BE link to dai link array if the BT offload link mask is valid (only one bit set). Signed-off-by: Brent Lu <brent.lu@intel.com>
SOF platform driver walks through HNLT endpoints and looks for endpoint which device type is BT sideband (0). Once a BT sideband endpoint is found, the SSP port number will be used to compose link mask and passed to machine driver to create BE link for BT audio offload.