| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z-J2WnrZHP6iMIhT@linaro.org>
Date: Tue, 25 Mar 2025 10:24:42 +0100
From: Stephan Gerhold <stephan.gerhold@...aro.org>
To: Wesley Cheng <quic_wcheng@...cinc.com>
Cc: srinivas.kandagatla@...aro.org, mathias.nyman@...el.com, perex@...ex.cz,
conor+dt@...nel.org, dmitry.torokhov@...il.com, corbet@....net,
broonie@...nel.org, lgirdwood@...il.com, krzk+dt@...nel.org,
pierre-louis.bossart@...ux.intel.com, Thinh.Nguyen@...opsys.com,
tiwai@...e.com, robh@...nel.org, gregkh@...uxfoundation.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-sound@...r.kernel.org, linux-input@...r.kernel.org,
linux-usb@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-doc@...r.kernel.org, Luca Weiss <luca.weiss@...rphone.com>
Subject: Re: [PATCH v36 22/31] ASoC: qcom: qdsp6: Introduce USB AFE port to
q6dsp
On Tue, Mar 18, 2025 at 05:51:32PM -0700, Wesley Cheng wrote:
> The QC ADSP is able to support USB playback endpoints, so that the main
> application processor can be placed into lower CPU power modes. This adds
> the required AFE port configurations and port start command to start an
> audio session.
>
> Specifically, the QC ADSP can support all potential endpoints that are
> exposed by the audio data interface. This includes isochronous data
> endpoints, in either synchronous mode or asynchronous mode. In the latter
> case both implicit or explicit feedback endpoints are supported. The size
> of audio samples sent per USB frame (microframe) will be adjusted based on
> information received on the feedback endpoint.
>
> Some pre-requisites are needed before issuing the AFE port start command,
> such as setting the USB AFE dev_token. This carries information about the
> available USB SND cards and PCM devices that have been discovered on the
> USB bus. The dev_token field is used by the audio DSP to notify the USB
> offload driver of which card and PCM index to enable playback on.
>
> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
> ---
> sound/soc/qcom/qdsp6/q6afe-dai.c | 60 +++++++
> sound/soc/qcom/qdsp6/q6afe.c | 192 ++++++++++++++++++++++-
> sound/soc/qcom/qdsp6/q6afe.h | 36 ++++-
> sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c | 23 +++
> sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h | 1 +
> sound/soc/qcom/qdsp6/q6routing.c | 32 +++-
> 6 files changed, 341 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/qcom/qdsp6/q6afe-dai.c b/sound/soc/qcom/qdsp6/q6afe-dai.c
> index 7d9628cda875..0f47aadaabe1 100644
> --- a/sound/soc/qcom/qdsp6/q6afe-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6afe-dai.c
> [...]
> @@ -513,12 +520,96 @@ struct afe_param_id_cdc_dma_cfg {
> u16 active_channels_mask;
> } __packed;
>
> +struct afe_param_id_usb_cfg {
> +/* Minor version used for tracking USB audio device configuration.
> + * Supported values: AFE_API_MINOR_VERSION_USB_AUDIO_CONFIG
> + */
> + u32 cfg_minor_version;
> +/* Sampling rate of the port.
> + * Supported values:
> + * - AFE_PORT_SAMPLE_RATE_8K
> + * - AFE_PORT_SAMPLE_RATE_11025
> + * - AFE_PORT_SAMPLE_RATE_12K
> + * - AFE_PORT_SAMPLE_RATE_16K
> + * - AFE_PORT_SAMPLE_RATE_22050
> + * - AFE_PORT_SAMPLE_RATE_24K
> + * - AFE_PORT_SAMPLE_RATE_32K
> + * - AFE_PORT_SAMPLE_RATE_44P1K
> + * - AFE_PORT_SAMPLE_RATE_48K
> + * - AFE_PORT_SAMPLE_RATE_96K
> + * - AFE_PORT_SAMPLE_RATE_192K
> + */
> + u32 sample_rate;
> +/* Bit width of the sample.
> + * Supported values: 16, 24
> + */
> + u16 bit_width;
> +/* Number of channels.
> + * Supported values: 1 and 2
> + */
> + u16 num_channels;
> +/* Data format supported by the USB. The supported value is
> + * 0 (#AFE_USB_AUDIO_DATA_FORMAT_LINEAR_PCM).
> + */
> + u16 data_format;
> +/* this field must be 0 */
> + u16 reserved;
> +/* device token of actual end USB audio device */
> + u32 dev_token;
> +/* endianness of this interface */
> + u32 endian;
Nitpick: The indentation between u32 and the struct field names is odd,
can you use a single tab character like in the afe_param_id_cdc_dma_cfg
instead?
> +/* service interval */
> + u32 service_interval;
> +} __packed;
> +
> + [...]
> diff --git a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c
> index 4919001de08b..4a96b11f7fd1 100644
> --- a/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c
> +++ b/sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c
> @@ -97,6 +97,26 @@
> }
>
> static struct snd_soc_dai_driver q6dsp_audio_fe_dais[] = {
> + {
> + .playback = {
> + .stream_name = "USB Playback",
> + .rates = SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_11025 |
> + SNDRV_PCM_RATE_16000 | SNDRV_PCM_RATE_22050 |
> + SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_44100 |
> + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |
> + SNDRV_PCM_RATE_192000,
> + .formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |
> + SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |
> + SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |
> + SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE,
> + .channels_min = 1,
> + .channels_max = 2,
> + .rate_min = 8000,
> + .rate_max = 192000,
Nitpick: Indentation after rate_max is also odd here, please choose one
of the styles, either
.rate_min = 8000,
or
.rate_max = 192000,
> + },
> + .id = USB_RX,
> + .name = "USB_RX",
> + },
> {
> .playback = {
> .stream_name = "HDMI Playback",
> [...]
> diff --git a/sound/soc/qcom/qdsp6/q6routing.c b/sound/soc/qcom/qdsp6/q6routing.c
> index 90228699ba7d..b7439420b425 100644
> --- a/sound/soc/qcom/qdsp6/q6routing.c
> +++ b/sound/soc/qcom/qdsp6/q6routing.c
> @@ -435,6 +435,26 @@ static struct session_data *get_session_from_id(struct msm_routing_data *data,
>
> return NULL;
> }
> +
> +static bool is_usb_routing_enabled(struct msm_routing_data *data)
> +{
> + int i;
> +
> + /*
> + * Loop through current sessions to see if there are active routes
> + * to the USB_RX backend DAI. The USB offload routing is designed
> + * similarly to the non offload path. If there are multiple PCM
> + * devices associated with the ASoC platform card, only one active
> + * path can be routed to the USB offloaded endpoint.
> + */
> + for (i = 0; i < MAX_SESSIONS; i++) {
> + if (data->sessions[i].port_id == USB_RX)
> + return true;
> + }
> +
> + return false;
> +}
What is different about USB_RX compared to other output ports we have in
Q6AFE? Obviously, we can only play one stream on an output port. But
doesn't the ADSP mix streams together when you have multiple routes?
Also, this doesn't actually check for *active* routes only. It just
looks if any other MultiMedia DAI is configured to output to USB_RX.
That doesn't mean they will ever be active at the same time.
I might for example want to have MultiMedia1 and MultiMedia2 both
configured to output to USB_RX. Let's assume MultiMedia1 is a normal PCM
DAI, MultiMedia2 is a compress offload DAI. When I want to playback
normal audio, I go through MultiMedia1, when I want to play compressed
audio, I go through MultiMedia2. Only one of them active at a time.
Why can't I set this up statically in the mixers?
If you confirm that it is really impossible to have multiple streams
mixed together to the USB_RX output in the ADSP, then this should be a
runtime check instead when starting the stream IMO.
> +
> /**
> * q6routing_stream_close() - Deregister a stream
> *
> @@ -499,7 +519,8 @@ static int msm_routing_put_audio_mixer(struct snd_kcontrol *kcontrol,
> struct session_data *session = &data->sessions[session_id];
>
> if (ucontrol->value.integer.value[0]) {
> - if (session->port_id == be_id)
> + if (session->port_id == be_id ||
> + (be_id == USB_RX && is_usb_routing_enabled(data)))
> return 0;
>
> session->port_id = be_id;
> @@ -515,6 +536,9 @@ static int msm_routing_put_audio_mixer(struct snd_kcontrol *kcontrol,
> return 1;
> }
>
> +static const struct snd_kcontrol_new usb_mixer_controls[] = {
usb_rx_mixer_controls
> + Q6ROUTING_RX_MIXERS(USB_RX) };
> +
> static const struct snd_kcontrol_new hdmi_mixer_controls[] = {
> Q6ROUTING_RX_MIXERS(HDMI_RX) };
>
> @@ -950,6 +974,10 @@ static const struct snd_soc_dapm_widget msm_qdsp6_widgets[] = {
> SND_SOC_DAPM_MIXER("MultiMedia8 Mixer", SND_SOC_NOPM, 0, 0,
> mmul8_mixer_controls, ARRAY_SIZE(mmul8_mixer_controls)),
>
> + SND_SOC_DAPM_MIXER("USB Mixer", SND_SOC_NOPM, 0, 0,
> + usb_mixer_controls,
> + ARRAY_SIZE(usb_mixer_controls)),
Please put this next to the other playback mixers above (below
"RX_CODEC_DMA_RX_7 Audio Mixer").
I think it would also be more clear if you call this "USB_RX Mixer"
instead for consistency with the other playback mixers. This would also
avoid confusion later when USB_TX is added in addition to USB_RX.
Are you planning to send follow-up patches for USB recording offload
(USB_TX) later? Me and Luca successfully used your series to playback
voice call audio via the ADSP to an USB headset, recording would be also
needed to use this fully. :-)
Thanks,
Stephan
Powered by blists - more mailing lists