Re: [PATCH v9 09/34] ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp

[Wesley Cheng <quic_wcheng@quicinc.com>]

Hi Pierre,

On 10/18/2023 6:47 AM, Pierre-Louis Bossart wrote:
> 
>>>> Specifically, the QC ADSP can support all potential endpoints that are
>>>> exposed by the audio data interface.  This includes, feedback endpoints
>>>> (both implicit and explicit) as well as the isochronous (data)
>>>> endpoints.
>>>
>>> implicit feedback means support for capture. This is confusing...
>>>
>>
>> I mean, a USB device can expose a capture path, but as of now, we won't
>> enable the offloading to the audio DSP for it.  However, if we're
>> executing playback, and device does support implicit feedback, we will
>> pass that along to the audio DSP to utilize.
> 
> Not following. Implicit feedback means a capture stream *SHALL* be
> started. Are you saying this capture stream is hidden and handled at the
> DSP level only? If yes, what prevents you from exposing the capture
> stream to userspace as well?
> 
> I must be missing something.
> 

My understanding is that with implicit feedback endpoints, it allows for 
another data endpoint in the opposite direction to be utilized as a 
feedback endpoint (versus having to expose another EP, such as in the 
case of explicit feedback).  For example, if we are enabling the 
playback path (and the device does have a capture data ep) then the data 
ep used for the capture path can be used.

USB2.0 spec, section 5.12.4.3 (Implicit Feedback)
"
Two cases can arise:
• One or more asynchronous sink endpoints are accompanied by an 
asynchronous source endpoint. The
data rate on the source endpoint can be used as implicit feedback 
information to adjust the data rate on
the sink endpoint(s).
• One or more adaptive source endpoints are accompanied by an adaptive 
sink endpoint. The source
endpoint can adjust its data rate based on the data rate received by the 
sink endpoint.
"

The DSP will get this as part of the USB sync endpoint information which 
it will use to enable this EP.

>>>>    +static const struct snd_soc_dai_ops q6usb_ops = {
>>>> +    .probe        = msm_dai_q6_dai_probe,
>>>> +    .prepare    = q6afe_dai_prepare,
>>>> +    .hw_params    = q6usb_hw_params,
>>>
>>> this is rather confusing with two different layers used for hw_params
>>> and prepare? Additional comments or explanations wouldn't hurt.
>>>
>>
>> I thought this was how the ASoC design was.  Each DAI defined for a
>> particular path has it own set of callbacks implemented to bring up any
>> required resources for that entity.  So in this case, it initializes the
>> "cpu" DAI, which is the main component that handles communication with
>> the audio DSP.
> 
> Usually prepare and hw_params rely on the type of DAI callbacks, but
> here you are mixing "q6afe" and "q6usb" which are shown in your Patch0
> diagram as "cpu" and "codec" dais respectively. I don't think it's
> correct to tie the two, it's a clear layering violation IMHO. The codec
> dai .prepare should not invoke something that modifies the state of the
> CPU dai, which should have its own .prepare callback.

OK, I think I know where the misunderstanding is.  The API callback for 
hw_params() that is being registered here exists in q6afe, but with the 
q6usb prefix.  I will fix that in the next rev.

Thanks
Wesley Cheng