Re: [RFC PATCH 09/14] sound: usb: Introduce QC USB SND offloading support

Wesley Cheng
2023-01-04T22:40:19Z

Hi Takashi,

On 1/2/2023 9:28 AM, Takashi Iwai wrote:
> On Sat, 24 Dec 2022 00:31:55 +0100,
> Wesley Cheng wrote:
>>
>> Several Qualcomm SoCs have a dedicated audio DSP, which has the ability to
>> support USB sound devices. This vendor driver will implement the required
>> handshaking with the DSP, in order to pass along required resources that
>> will be utilized by the DSP's USB SW. The communication channel used for
>> this handshaking will be using the QMI protocol. Required resources
>> include:
>> - Allocated secondary event ring address
>> - EP transfer ring address
>> - Interrupter number
>>
>> The above information will allow for the audio DSP to execute USB transfers
>> over the USB bus. It will also be able to support devices that have an
>> implicit feedback and sync endpoint as well. Offloading these data
>> transfers will allow the main/applications processor to enter lower CPU
>> power modes, and sustain a longer duration in those modes.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>
> Hmm, this must be the main part that works to bypass the normal USB
> packet handling in USB audio driver but hooks to the own offload one,
> but there is no description how to take over and manage.
> A missing "big picture" makes it difficult to understand and review.
>

Technically, we are not taking over the functionality of the USB SND, as
we still want the normal path to be accessible in case there is an audio
profile/format that can't be supported by the audio DSP. I can add some
more information on how this offload driver co-exists with the USB SND.

> Also, since both drivers are asynchronous, we may need some proper
> locking.
>

Yes, I think locking is needed in some places. Will add that in the
next revision.

> More on the code change:
>
>> +static int snd_interval_refine_set(struct snd_interval *i, unsigned int val)
>> +{
>> + struct snd_interval t;
>> +
>> + t.empty = 0;
>> + t.min = t.max = val;
>> + t.openmin = t.openmax = 0;
>> + t.integer = 1;
>> + return snd_interval_refine(i, &t);
>> +}
>> +
>> +static int _snd_pcm_hw_param_set(struct snd_pcm_hw_params *params,
>> + snd_pcm_hw_param_t var, unsigned int val,
>> + int dir)
>> +{
>> + int changed;
>> +
>> + if (hw_is_mask(var)) {
>> + struct snd_mask *m = hw_param_mask(params, var);
>> +
>> + if (val == 0 && dir < 0) {
>> + changed = -EINVAL;
>> + snd_mask_none(m);
>> + } else {
>> + if (dir > 0)
>> + val++;
>> + else if (dir < 0)
>> + val--;
>> + changed = snd_mask_refine_set(
>> + hw_param_mask(params, var), val);
>> + }
>> + } else if (hw_is_interval(var)) {
>> + struct snd_interval *i = hw_param_interval(params, var);
>> +
>> + if (val == 0 && dir < 0) {
>> + changed = -EINVAL;
>> + snd_interval_none(i);
>> + } else if (dir == 0)
>> + changed = snd_interval_refine_set(i, val);
>> + else {
>> + struct snd_interval t;
>> +
>> + t.openmin = 1;
>> + t.openmax = 1;
>> + t.empty = 0;
>> + t.integer = 0;
>> + if (dir < 0) {
>> + t.min = val - 1;
>> + t.max = val;
>> + } else {
>> + t.min = val;
>> + t.max = val+1;
>> + }
>> + changed = snd_interval_refine(i, &t);
>> + }
>> + } else
>> + return -EINVAL;
>> + if (changed) {
>> + params->cmask |= 1 << var;
>> + params->rmask |= 1 << var;
>> + }
>> + return changed;
>> +}
>
> Those are taken from sound/core/oss/pcm_oss.c? We may put to the
> common PCM helper instead of duplication.
>

Sure, I can do that.

>> +static void disable_audio_stream(struct snd_usb_substream *subs)
>> +{
>> + struct snd_usb_audio *chip = subs->stream->chip;
>> +
>> + if (subs->data_endpoint || subs->sync_endpoint) {
>> + close_endpoints(chip, subs);
>> +
>> + mutex_lock(&chip->mutex);
>> + subs->cur_audiofmt = NULL;
>> + mutex_unlock(&chip->mutex);
>> + }
>> +
>> + snd_usb_autosuspend(chip);
>> +}
>> +
>> +static int enable_audio_stream(struct snd_usb_substream *subs,
>> + snd_pcm_format_t pcm_format,
>> + unsigned int channels, unsigned int cur_rate,
>> + int datainterval)
>> +{
>> + struct snd_usb_audio *chip = subs->stream->chip;
>> + struct snd_pcm_hw_params params;
>> + const struct audioformat *fmt;
>> + int ret;
>> +
>> + _snd_pcm_hw_params_any(&params);
>> + _snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_FORMAT,
>> + pcm_format, 0);
>> + _snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_CHANNELS,
>> + channels, 0);
>> + _snd_pcm_hw_param_set(&params, SNDRV_PCM_HW_PARAM_RATE,
>> + cur_rate, 0);
>
> What about other parameters like period / buffer sizes?
>

I don't think we will need those parameters on the audio DSP. The
"params" here is used to pass the pcm format into the qmi response.

>> +struct qmi_uaudio_stream_req_msg_v01 {
>> + u8 enable;
>> + u32 usb_token;
>> + u8 audio_format_valid;
>> + u32 audio_format;
>> + u8 number_of_ch_valid;
>> + u32 number_of_ch;
>> + u8 bit_rate_valid;
>> + u32 bit_rate;
>> + u8 xfer_buff_size_valid;
>> + u32 xfer_buff_size;
>> + u8 service_interval_valid;
>> + u32 service_interval;
>> +};
>
> Are this and the other structs a part of DSP ABI?
> Or is it a definition only used in kernel? I'm asking because
> __packed attribute is required for most of ABI definitions with
> different field types.
>

This would be in the kernel only.

Thanks
Wesley Cheng