diff mbox series

[v5,11/32] sound: usb: card: Introduce USB SND platform op callbacks

Message ID 20230829210657.9904-12-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng Aug. 29, 2023, 9:06 p.m. UTC 
Allow for different platforms to be notified on USB SND connect/disconnect
seqeunces.  This allows for platform USB SND modules to properly initialize
and populate internal structures with references to the USB SND chip
device.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 sound/usb/card.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 sound/usb/card.h |  9 +++++++++
 2 files changed, 54 insertions(+)

Comments

Takashi Iwai Sept. 7, 2023, 3:36 p.m. UTC | #1 
On Tue, 29 Aug 2023 23:06:36 +0200,
Wesley Cheng wrote:
> 
> Allow for different platforms to be notified on USB SND connect/disconnect
> seqeunces.  This allows for platform USB SND modules to properly initialize
> and populate internal structures with references to the USB SND chip
> device.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  sound/usb/card.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  sound/usb/card.h |  9 +++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/sound/usb/card.c b/sound/usb/card.c
> index 1b2edc0fd2e9..067a1e82f4bf 100644
> --- a/sound/usb/card.c
> +++ b/sound/usb/card.c
> @@ -118,6 +118,34 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
>  static DEFINE_MUTEX(register_mutex);
>  static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>  static struct usb_driver usb_audio_driver;
> +static struct snd_usb_platform_ops *platform_ops;
> +
> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
> +{
> +	int ret;
> +
> +	mutex_lock(&register_mutex);
> +	if (platform_ops) {
> +		ret = -EEXIST;
> +		goto out;
> +	}
> +
> +	platform_ops = ops;
> +out:
> +	mutex_unlock(&register_mutex);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);

For adding this kind of API, please give the proper comment.
Especially this API is special and need a caution, to mention that it
can be used only for a single instance.

Also, it should be mentioned that all callbacks are exclusive under
the global register_mutex.

> @@ -910,7 +938,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>  	chip->num_interfaces++;
>  	usb_set_intfdata(intf, chip);
>  	atomic_dec(&chip->active);
> +
> +	if (platform_ops && platform_ops->connect_cb)
> +		platform_ops->connect_cb(chip);
>  	mutex_unlock(&register_mutex);

One uncertain thing is the argument for connect_cb and disconnect_cb.
Those take snd_usb_audio object, but the callback gets called per
interface at each probe and disconnect.  How does the callee handle
multiple calls?

Last but not least, the patch subject should be with "ALSA:" prefix,
and in this case, at best "ALSA: usb-audio: xxx".


thanks,

Takashi
Wesley Cheng Sept. 11, 2023, 5:57 p.m. UTC | #2 
Hi Takashi,

On 9/7/2023 8:36 AM, Takashi Iwai wrote:
> On Tue, 29 Aug 2023 23:06:36 +0200,
> Wesley Cheng wrote:
>>
>> Allow for different platforms to be notified on USB SND connect/disconnect
>> seqeunces.  This allows for platform USB SND modules to properly initialize
>> and populate internal structures with references to the USB SND chip
>> device.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   sound/usb/card.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   sound/usb/card.h |  9 +++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>> index 1b2edc0fd2e9..067a1e82f4bf 100644
>> --- a/sound/usb/card.c
>> +++ b/sound/usb/card.c
>> @@ -118,6 +118,34 @@ MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
>>   static DEFINE_MUTEX(register_mutex);
>>   static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>>   static struct usb_driver usb_audio_driver;
>> +static struct snd_usb_platform_ops *platform_ops;
>> +
>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&register_mutex);
>> +	if (platform_ops) {
>> +		ret = -EEXIST;
>> +		goto out;
>> +	}
>> +
>> +	platform_ops = ops;
>> +out:
>> +	mutex_unlock(&register_mutex);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
> 
> For adding this kind of API, please give the proper comment.
> Especially this API is special and need a caution, to mention that it
> can be used only for a single instance.
> 
> Also, it should be mentioned that all callbacks are exclusive under
> the global register_mutex.
> 

Thanks for taking the time to review.  Sure, I'll add some comments in 
these new APIs to document what they are used for and how they are 
protected and limited.

>> @@ -910,7 +938,11 @@ static int usb_audio_probe(struct usb_interface *intf,
>>   	chip->num_interfaces++;
>>   	usb_set_intfdata(intf, chip);
>>   	atomic_dec(&chip->active);
>> +
>> +	if (platform_ops && platform_ops->connect_cb)
>> +		platform_ops->connect_cb(chip);
>>   	mutex_unlock(&register_mutex);
> 
> One uncertain thing is the argument for connect_cb and disconnect_cb.
> Those take snd_usb_audio object, but the callback gets called per
> interface at each probe and disconnect.  How does the callee handle
> multiple calls?

I guess it should depend on how the platform driver wants to handle it? 
  I haven't run into a device with multiple UAC interfaces before, so 
I'll need to mimic this configuration on a device, so I can see how it 
exposes itself.

Will investigate this a bit more on my end and come back with my findings.

> 
> Last but not least, the patch subject should be with "ALSA:" prefix,
> and in this case, at best "ALSA: usb-audio: xxx".
> 
> 

Got it, thanks!

Thanks
Wesley Cheng
Wesley Cheng Sept. 12, 2023, 10:36 p.m. UTC | #3 
Hi Takashi,

On 9/11/2023 10:57 AM, Wesley Cheng wrote:
> Hi Takashi,
> 
> On 9/7/2023 8:36 AM, Takashi Iwai wrote:
>> On Tue, 29 Aug 2023 23:06:36 +0200,
>> Wesley Cheng wrote:
>>>
>>> Allow for different platforms to be notified on USB SND 
>>> connect/disconnect
>>> seqeunces.  This allows for platform USB SND modules to properly 
>>> initialize
>>> and populate internal structures with references to the USB SND chip
>>> device.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>> ---
>>>   sound/usb/card.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>   sound/usb/card.h |  9 +++++++++
>>>   2 files changed, 54 insertions(+)
>>>
>>> diff --git a/sound/usb/card.c b/sound/usb/card.c
>>> index 1b2edc0fd2e9..067a1e82f4bf 100644
>>> --- a/sound/usb/card.c
>>> +++ b/sound/usb/card.c
>>> @@ -118,6 +118,34 @@ MODULE_PARM_DESC(skip_validation, "Skip unit 
>>> descriptor validation (default: no)
>>>   static DEFINE_MUTEX(register_mutex);
>>>   static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
>>>   static struct usb_driver usb_audio_driver;
>>> +static struct snd_usb_platform_ops *platform_ops;
>>> +
>>> +int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
>>> +{
>>> +    int ret;
>>> +
>>> +    mutex_lock(&register_mutex);
>>> +    if (platform_ops) {
>>> +        ret = -EEXIST;
>>> +        goto out;
>>> +    }
>>> +
>>> +    platform_ops = ops;
>>> +out:
>>> +    mutex_unlock(&register_mutex);
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
>>
>> For adding this kind of API, please give the proper comment.
>> Especially this API is special and need a caution, to mention that it
>> can be used only for a single instance.
>>
>> Also, it should be mentioned that all callbacks are exclusive under
>> the global register_mutex.
>>
> 
> Thanks for taking the time to review.  Sure, I'll add some comments in 
> these new APIs to document what they are used for and how they are 
> protected and limited.
> 
>>> @@ -910,7 +938,11 @@ static int usb_audio_probe(struct usb_interface 
>>> *intf,
>>>       chip->num_interfaces++;
>>>       usb_set_intfdata(intf, chip);
>>>       atomic_dec(&chip->active);
>>> +
>>> +    if (platform_ops && platform_ops->connect_cb)
>>> +        platform_ops->connect_cb(chip);
>>>       mutex_unlock(&register_mutex);
>>
>> One uncertain thing is the argument for connect_cb and disconnect_cb.
>> Those take snd_usb_audio object, but the callback gets called per
>> interface at each probe and disconnect.  How does the callee handle
>> multiple calls?
> 
> I guess it should depend on how the platform driver wants to handle it? 
>   I haven't run into a device with multiple UAC interfaces before, so 
> I'll need to mimic this configuration on a device, so I can see how it 
> exposes itself.
> 
> Will investigate this a bit more on my end and come back with my findings.
> 

So looks like if there is a device that has multiple UAC interfaces, 
then it just results in one USB sound card with multiple USB streams. 
As of now, we do expose a ksndcontrol that allows for userspace to 
specify which card and PCM device to issue the audio playback on.  Then 
based on the audio profile received by the audio DSP, it will narrow it 
down to see if that substream supports the requested profile.

This selection is done in the DPCM backend driver (q6usb), since the 
overall QMI USB stream request stems from the audio dsp, and will carry 
this information about which PCM device and card the USB offload driver 
should use.  This may differ based on the implementation of the offload 
path, hence why I mentioned that it might be platform specific.

There is one improvement I need to make in our QC offload driver to 
accommodate for this scenario, and that is to avoid having to 
re-register for the XHCI sideband if there is already a USB SND card 
allocated, which I will fix and submit in the next revision.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/sound/usb/card.c b/sound/usb/card.c
index 1b2edc0fd2e9..067a1e82f4bf 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -118,6 +118,34 @@  MODULE_PARM_DESC(skip_validation, "Skip unit descriptor validation (default: no)
 static DEFINE_MUTEX(register_mutex);
 static struct snd_usb_audio *usb_chip[SNDRV_CARDS];
 static struct usb_driver usb_audio_driver;
+static struct snd_usb_platform_ops *platform_ops;
+
+int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops)
+{
+	int ret;
+
+	mutex_lock(&register_mutex);
+	if (platform_ops) {
+		ret = -EEXIST;
+		goto out;
+	}
+
+	platform_ops = ops;
+out:
+	mutex_unlock(&register_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_usb_register_platform_ops);
+
+int snd_usb_unregister_platform_ops(void)
+{
+	mutex_lock(&register_mutex);
+	platform_ops = NULL;
+	mutex_unlock(&register_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_usb_unregister_platform_ops);
 
 /*
  * disconnect streams
@@ -910,7 +938,11 @@  static int usb_audio_probe(struct usb_interface *intf,
 	chip->num_interfaces++;
 	usb_set_intfdata(intf, chip);
 	atomic_dec(&chip->active);
+
+	if (platform_ops && platform_ops->connect_cb)
+		platform_ops->connect_cb(chip);
 	mutex_unlock(&register_mutex);
+
 	return 0;
 
  __error:
@@ -947,6 +979,9 @@  static void usb_audio_disconnect(struct usb_interface *intf)
 	card = chip->card;
 
 	mutex_lock(&register_mutex);
+	if (platform_ops && platform_ops->disconnect_cb)
+		platform_ops->disconnect_cb(chip);
+
 	if (atomic_inc_return(&chip->shutdown) == 1) {
 		struct snd_usb_stream *as;
 		struct snd_usb_endpoint *ep;
@@ -1090,6 +1125,11 @@  static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message)
 		chip->system_suspend = chip->num_suspended_intf;
 	}
 
+	mutex_lock(&register_mutex);
+	if (platform_ops && platform_ops->suspend_cb)
+		platform_ops->suspend_cb(intf, message);
+	mutex_unlock(&register_mutex);
+
 	return 0;
 }
 
@@ -1130,6 +1170,11 @@  static int usb_audio_resume(struct usb_interface *intf)
 
 	snd_usb_midi_v2_resume_all(chip);
 
+	mutex_lock(&register_mutex);
+	if (platform_ops && platform_ops->resume_cb)
+		platform_ops->resume_cb(intf);
+	mutex_unlock(&register_mutex);
+
  out:
 	if (chip->num_suspended_intf == chip->system_suspend) {
 		snd_power_change_state(chip->card, SNDRV_CTL_POWER_D0);
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 6ec95b2edf86..2884912adc96 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -207,4 +207,13 @@  struct snd_usb_stream {
 	struct list_head list;
 };
 
+struct snd_usb_platform_ops {
+	void (*connect_cb)(struct snd_usb_audio *chip);
+	void (*disconnect_cb)(struct snd_usb_audio *chip);
+	void (*suspend_cb)(struct usb_interface *intf, pm_message_t message);
+	void (*resume_cb)(struct usb_interface *intf);
+};
+
+int snd_usb_register_platform_ops(struct snd_usb_platform_ops *ops);
+int snd_usb_unregister_platform_ops(void);
 #endif /* __USBAUDIO_CARD_H */