Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <8e08fd5e-91b8-c73e-1d97-7cf4d98573d4@quicinc.com>
Date:   Wed, 18 Oct 2023 18:39:16 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v9 29/34] ASoC: qcom: qdsp6: Add SND kcontrol to select
 offload device
Content-Language: en-US
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        <mathias.nyman@intel.com>, <gregkh@linuxfoundation.org>,
        <lgirdwood@gmail.com>, <broonie@kernel.org>, <perex@perex.cz>,
        <tiwai@suse.com>, <agross@kernel.org>, <andersson@kernel.org>,
        <konrad.dybcio@linaro.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <srinivas.kandagatla@linaro.org>, <bgoswami@quicinc.com>,
        <Thinh.Nguyen@synopsys.com>
CC:     <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <alsa-devel@alsa-project.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>
References: <20231017200109.11407-1-quic_wcheng@quicinc.com>
 <20231017200109.11407-30-quic_wcheng@quicinc.com>
 <d218b8e2-d7b9-40a3-bfb3-da6a90404a8c@linux.intel.com>
From:   Wesley Cheng <quic_wcheng@quicinc.com>
In-Reply-To: <d218b8e2-d7b9-40a3-bfb3-da6a90404a8c@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Pierre,

On 10/17/2023 3:50 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 10/17/23 15:01, Wesley Cheng wrote:
>> Expose a kcontrol on the platform sound card, which will allow for
>> userspace to determine which USB card number and PCM device to offload.
>> This allows for userspace to potentially tag an alternate path for a
>> specific USB SND card and PCM device.  Previously, control was absent, and
>> the offload path would be enabled on the last USB SND device which was
>> connected.  This logic will continue to be applicable if no mixer input is
>> received for specific device selection.
>>
>> An example to configure the offload device using tinymix:
>> tinymix -D 0 set 'Q6USB offload SND device select' 1 0
>>
>> The above will set the Q6AFE device token to choose offload on card#1 and
>> pcm#0.  Device selection is made possible by setting the Q6AFE device
>> token.  The audio DSP utilizes this parameter, and will pass this field
>> back to the USB offload driver within the QMI stream requests.
> 
> This still begs the question on how userspace would figure what the
> card1 is and which endpoint is used when PCM0 is opened?
> 

Assuming we have something maybe in the component string for card1, then 
maybe that will help in this case as you mentioned earlier.  Again, I 
think my understanding of "endpoint" here is referring to the USB 
endpoint.  However, that isn't normally how we should think about it. 
 From the USB sound perspective, it would be the USB interface that is 
selected, and the USB endpoints that that interface descriptor includes 
(up to 2 usb eps).

> Ideally userpace would not have to know anything about "Q6USB".
> Presumably when other vendors expose their USB offload solution, we
> would want a generic control name, no?
> 

Hmm...initially I was keeping the sound kcontrol implementation based on 
a vendor specific approach.  Although, it might be good to standardize 
it a bit.  I could see if we can add this to within soc-usb so whenever 
we just create a soc usb device, it would also create and initialize the 
kcontrols as well?

> Jaroslav should chime in on this one :-)
> 
> 
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   sound/soc/qcom/qdsp6/q6usb.c | 125 ++++++++++++++++++++++++++++++++++-
>>   1 file changed, 122 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
>> index d697cbe7f184..a95276b7d91d 100644
>> --- a/sound/soc/qcom/qdsp6/q6usb.c
>> +++ b/sound/soc/qcom/qdsp6/q6usb.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/dma-map-ops.h>
>>   
>>   #include <sound/pcm.h>
>> +#include <sound/control.h>
>>   #include <sound/soc.h>
>>   #include <sound/soc-usb.h>
>>   #include <sound/pcm_params.h>
>> @@ -35,9 +36,12 @@ struct q6usb_port_data {
>>   	struct q6afe_usb_cfg usb_cfg;
>>   	struct snd_soc_usb *usb;
>>   	struct q6usb_offload priv;
>> +	struct mutex mutex;
> 
> missing comment on what this protects. "mutex" is really a poor
> choice/name if I am honest.
> 
>>   	unsigned long available_card_slot;
>>   	struct q6usb_status status[SNDRV_CARDS];
>> -	int active_idx;
>> +	bool idx_valid;
>> +	int sel_card_idx;
>> +	int sel_pcm_idx;
>>   };
> 
>> +/* Build a mixer control for a UAC connector control (jack-detect) */
>> +static void q6usb_connector_control_init(struct snd_soc_component *component)
>> +{
>> +	int ret;
>> +
>> +	ret = snd_ctl_add(component->card->snd_card,
>> +				snd_ctl_new1(&q6usb_offload_dev_ctrl, component));
>> +	if (ret < 0)
>> +		return;
> 
> that error handling does not seem terribly useful...
> 

Based on your earlier comment, this might change :)

Thanks
Wesley Cheng