Messages in this thread | | | Date | Wed, 4 Jan 2023 14:38:53 -0800 | Subject | Re: [RFC PATCH 09/14] sound: usb: Introduce QC USB SND offloading support | From | Wesley Cheng <> |
| |
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(¶ms); >> + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_FORMAT, >> + pcm_format, 0); >> + _snd_pcm_hw_param_set(¶ms, SNDRV_PCM_HW_PARAM_CHANNELS, >> + channels, 0); >> + _snd_pcm_hw_param_set(¶ms, 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
| |