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

[Wesley Cheng <quic_wcheng@quicinc.com>]

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