Hi Pierre,
On 8/30/2024 2:21 AM, Pierre-Louis Bossart wrote:
>
> On 8/29/24 21:40, Wesley Cheng wrote:
>> Create a USB BE component that will register a new USB port to the ASoC USB
>> framework. This will handle determination on if the requested audio
>> profile is supported by the USB device currently selected.
>>
>> Check for if the PCM format is supported during the hw_params callback. If
>> the profile is not supported then the userspace ALSA entity will receive an
>> error, and can take further action.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
>> ---
>> include/sound/q6usboffload.h | 20 +++
>> sound/soc/qcom/Kconfig | 10 ++
>> sound/soc/qcom/qdsp6/Makefile | 1 +
>> sound/soc/qcom/qdsp6/q6usb.c | 246 ++++++++++++++++++++++++++++++++++
>> 4 files changed, 277 insertions(+)
>> create mode 100644 include/sound/q6usboffload.h
>> create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
>>
>> diff --git a/include/sound/q6usboffload.h b/include/sound/q6usboffload.h
>> new file mode 100644
>> index 000000000000..49ab2c34b84c
>> --- /dev/null
>> +++ b/include/sound/q6usboffload.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0
>> + *
>> + * linux/sound/q6usboffload.h -- QDSP6 USB offload
> not sure about the linux/ prefix?
>
>> + *
>> + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +/**
>> + * struct q6usb_offload
>> + * @dev - dev handle to usb be
>> + * @sid - streamID for iommu
>> + * @intr_num - usb interrupter number
>> + * @domain - allocated iommu domain
>> + **/
>> +struct q6usb_offload {
>> + struct device *dev;
>> + long long sid;
>> + u16 intr_num;
>> + struct iommu_domain *domain;
>> +};
> consider reordering to avoid holes/alignment issues, e.g. all pointers
> first, then long long then u16
>
Will fix these.
>> +static int q6usb_hw_params(struct snd_pcm_substream *substream,
>> + struct snd_pcm_hw_params *params,
>> + struct snd_soc_dai *dai)
>> +{
>> + struct q6usb_port_data *data = dev_get_drvdata(dai->dev);
>> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> + struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
>> + int direction = substream->stream;
>> + struct q6afe_port *q6usb_afe;
>> + struct snd_soc_usb_device *sdev;
>> + int ret = -EINVAL;
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + /* No active chip index */
>> + if (list_empty(&data->devices))
>> + goto out;
> -ENODEV for the default return value>?
Sure.
>> +
>> + sdev = list_last_entry(&data->devices, struct snd_soc_usb_device, list);
>> +
>> + ret = snd_soc_usb_find_supported_format(sdev->chip_idx, params, direction);
>> + if (ret < 0)
>> + goto out;
>> +
>> + q6usb_afe = q6afe_port_get_from_id(cpu_dai->dev, USB_RX);
>> + if (IS_ERR(q6usb_afe))
>> + goto out;
>> +
>> + /* Notify audio DSP about the devices being offloaded */
>> + ret = afe_port_send_usb_dev_param(q6usb_afe, sdev->card_idx,
>> + sdev->ppcm_idx[sdev->num_playback - 1]);
> don't you need a symmetrical notification upon hw_free()?
>
> Also what happens if there are multiple calls to hw_params, which is
> quite legit in ALSA/ASoC?
The afe_port_send_usb_dev_param() is meant to just update the device selected for offload on the audio DSP end, and this won't be referenced until our Q6AFE DAI sends the port start command in its prepare() callback. Don't think we need to handle anything specific in the hw_free() case. As long as the hw_params() callback is called before any audio session is started, then we'll ensure that the device selected is always updated to the audio DSP.
Thanks
Wesley Cheng
