diff mbox series

[v4,26/32] sound: Pass USB SND card and PCM information to SOC USB

Message ID 20230725023416.11205-27-quic_wcheng@quicinc.com
State New
Headers
Series Introduce QC USB SND audio offloading support |

Commit Message

Wesley Cheng July 25, 2023, 2:34 a.m. UTC 
  Currently, only the index to the USB SND card array is passed to the USB
backend.  Pass through more information, specifically the USB SND card
number and the number of PCM devices available.  The USB backend should
know about which sound resources are being shared between the ASoC and USB
SND paths.  This can be utilized to properly select and maintain the
offloading devices.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 include/sound/soc-usb.h           |  9 +++++----
 sound/soc/qcom/qdsp6/q6usb.c      | 20 ++++++++++++++++++--
 sound/soc/soc-usb.c               | 12 +++++++-----
 sound/usb/qcom/qc_audio_offload.c |  9 +++++----
 4 files changed, 35 insertions(+), 15 deletions(-)

Comments

Pierre-Louis Bossart July 25, 2023, 8:59 a.m. UTC | #1 
On 7/25/23 04:34, Wesley Cheng wrote:
> Currently, only the index to the USB SND card array is passed to the USB
> backend.  Pass through more information, specifically the USB SND card
> number and the number of PCM devices available.  The USB backend should
> know about which sound resources are being shared between the ASoC and USB
> SND paths.  This can be utilized to properly select and maintain the
> offloading devices.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  include/sound/soc-usb.h           |  9 +++++----
>  sound/soc/qcom/qdsp6/q6usb.c      | 20 ++++++++++++++++++--
>  sound/soc/soc-usb.c               | 12 +++++++-----
>  sound/usb/qcom/qc_audio_offload.c |  9 +++++----
>  4 files changed, 35 insertions(+), 15 deletions(-)
> 
> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
> index 71e6e75e600a..606128332044 100644
> --- a/include/sound/soc-usb.h
> +++ b/include/sound/soc-usb.h
> @@ -19,20 +19,21 @@ struct snd_soc_usb {
>  	struct device *dev;
>  	struct snd_soc_component *component;
>  	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
> -				int connected);
> +				int chip_idx, int num_pcm, int connected);

I don't know what 'chip_idx' is.

The 'num_pcm' sounds problematic if there are different devices for
playback and capture. I would guess this is for playback only, but this
doesn't scale.

>  	void *priv_data;
>  };

> +struct q6usb_status {
> +	unsigned int num_pcm;
> +	unsigned int chip_index;
> +	unsigned int pcm_index;
> +};
> +
>  struct q6usb_port_data {
>  	struct q6afe_usb_cfg usb_cfg;
>  	struct snd_soc_usb *usb;
>  	struct q6usb_offload priv;
> +	unsigned long available_card_slot;

what is a card slot?

> +	struct q6usb_status status[SNDRV_CARDS];
>  	int active_idx;
>  };
>  
> @@ -97,7 +105,7 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone
>  }
>  
>  static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
> -			int connected)
> +			int chip_idx, int num_pcm, int connected)
>  {
>  	struct snd_soc_dapm_context *dapm;
>  	struct q6usb_port_data *data;
> @@ -109,8 +117,16 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>  		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>  		/* We only track the latest USB headset plugged in */
>  		data->active_idx = card_idx;
> +
> +		set_bit(card_idx, &data->available_card_slot);
> +		data->status[card_idx].num_pcm = num_pcm;
> +		data->status[card_idx].chip_index = chip_idx;
>  	} else {
> -		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> +		clear_bit(card_idx, &data->available_card_slot);
> +		data->status[card_idx].num_pcm = 0;
> +		data->status[card_idx].chip_index = 0;
> +		if (!data->available_card_slot)
> +			snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");

not able to follow what this does, this patch is rather unclear and
lacks comments.

>  	}
>  	snd_soc_dapm_sync(dapm);
Wesley Cheng Aug. 15, 2023, 1:48 a.m. UTC | #2 
Hi Pierre,

On 7/25/2023 1:59 AM, Pierre-Louis Bossart wrote:
> 
> 
> On 7/25/23 04:34, Wesley Cheng wrote:
>> Currently, only the index to the USB SND card array is passed to the USB
>> backend.  Pass through more information, specifically the USB SND card
>> number and the number of PCM devices available.  The USB backend should
>> know about which sound resources are being shared between the ASoC and USB
>> SND paths.  This can be utilized to properly select and maintain the
>> offloading devices.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   include/sound/soc-usb.h           |  9 +++++----
>>   sound/soc/qcom/qdsp6/q6usb.c      | 20 ++++++++++++++++++--
>>   sound/soc/soc-usb.c               | 12 +++++++-----
>>   sound/usb/qcom/qc_audio_offload.c |  9 +++++----
>>   4 files changed, 35 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
>> index 71e6e75e600a..606128332044 100644
>> --- a/include/sound/soc-usb.h
>> +++ b/include/sound/soc-usb.h
>> @@ -19,20 +19,21 @@ struct snd_soc_usb {
>>   	struct device *dev;
>>   	struct snd_soc_component *component;
>>   	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
>> -				int connected);
>> +				int chip_idx, int num_pcm, int connected);
> 
> I don't know what 'chip_idx' is.
> 

chip_idx is the index to the USB SND chip array which carries 
information about each udev (USB device) connected to USB SND)

> The 'num_pcm' sounds problematic if there are different devices for
> playback and capture. I would guess this is for playback only, but this
> doesn't scale.
> 

I agree.  My plan is to address this by having another SND SOC USB 
structure that is created by the USB class driver, ie qc_usb_offload, 
which would populate the required parameters.

Main thing I wanted to avoid is to have to expose the main USB SND 
structure if we're only going to only have a few variables to check for.

>>   	void *priv_data;
>>   };
> 
>> +struct q6usb_status {
>> +	unsigned int num_pcm;
>> +	unsigned int chip_index;
>> +	unsigned int pcm_index;
>> +};
>> +
>>   struct q6usb_port_data {
>>   	struct q6afe_usb_cfg usb_cfg;
>>   	struct snd_soc_usb *usb;
>>   	struct q6usb_offload priv;
>> +	unsigned long available_card_slot;
> 
> what is a card slot?
> 

Idea was just to have a bitmapped value of all offload capable audio 
devices that are currently connected.  Then we can take the necessary 
actions when everything has been disconnected.

>> +	struct q6usb_status status[SNDRV_CARDS];
>>   	int active_idx;
>>   };
>>   
>> @@ -97,7 +105,7 @@ static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone
>>   }
>>   
>>   static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>> -			int connected)
>> +			int chip_idx, int num_pcm, int connected)
>>   {
>>   	struct snd_soc_dapm_context *dapm;
>>   	struct q6usb_port_data *data;
>> @@ -109,8 +117,16 @@ static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
>>   		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
>>   		/* We only track the latest USB headset plugged in */
>>   		data->active_idx = card_idx;
>> +
>> +		set_bit(card_idx, &data->available_card_slot);
>> +		data->status[card_idx].num_pcm = num_pcm;
>> +		data->status[card_idx].chip_index = chip_idx;
>>   	} else {
>> -		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
>> +		clear_bit(card_idx, &data->available_card_slot);
>> +		data->status[card_idx].num_pcm = 0;
>> +		data->status[card_idx].chip_index = 0;
>> +		if (!data->available_card_slot)
>> +			snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
> 
> not able to follow what this does, this patch is rather unclear and
> lacks comments.
> 

I will add comments, but it basically will populate some of the limits 
that we'll end up using for controlling the ksndcontrols.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h
index 71e6e75e600a..606128332044 100644
--- a/include/sound/soc-usb.h
+++ b/include/sound/soc-usb.h
@@ -19,20 +19,21 @@  struct snd_soc_usb {
 	struct device *dev;
 	struct snd_soc_component *component;
 	int (*connection_status_cb)(struct snd_soc_usb *usb, int card_idx,
-				int connected);
+				int chip_idx, int num_pcm, int connected);
 	void *priv_data;
 };
 
 int snd_soc_usb_find_format(int card_idx, struct snd_pcm_hw_params *params,
 			int direction);
 
-int snd_soc_usb_connect(struct device *usbdev, int card_idx);
-int snd_soc_usb_disconnect(struct device *usbdev);
+int snd_soc_usb_connect(struct device *usbdev, int card_idx, int chip_idx,
+			int num_pcm);
+int snd_soc_usb_disconnect(struct device *usbdev, int card_idx);
 void snd_soc_usb_set_priv_data(struct device *dev, void *priv);
 void *snd_soc_usb_get_priv_data(struct device *usbdev);
 
 struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
 			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
-			int connected));
+			int chip_idx, int num_pcm, int connected));
 int snd_soc_usb_remove_port(struct device *dev);
 #endif
diff --git a/sound/soc/qcom/qdsp6/q6usb.c b/sound/soc/qcom/qdsp6/q6usb.c
index c4f3039ef8c4..2fb6159bd2a8 100644
--- a/sound/soc/qcom/qdsp6/q6usb.c
+++ b/sound/soc/qcom/qdsp6/q6usb.c
@@ -25,10 +25,18 @@ 
 
 #define SID_MASK	0xF
 
+struct q6usb_status {
+	unsigned int num_pcm;
+	unsigned int chip_index;
+	unsigned int pcm_index;
+};
+
 struct q6usb_port_data {
 	struct q6afe_usb_cfg usb_cfg;
 	struct snd_soc_usb *usb;
 	struct q6usb_offload priv;
+	unsigned long available_card_slot;
+	struct q6usb_status status[SNDRV_CARDS];
 	int active_idx;
 };
 
@@ -97,7 +105,7 @@  static int q6usb_audio_ports_of_xlate_dai_name(struct snd_soc_component *compone
 }
 
 static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
-			int connected)
+			int chip_idx, int num_pcm, int connected)
 {
 	struct snd_soc_dapm_context *dapm;
 	struct q6usb_port_data *data;
@@ -109,8 +117,16 @@  static int q6usb_alsa_connection_cb(struct snd_soc_usb *usb, int card_idx,
 		snd_soc_dapm_enable_pin(dapm, "USB_RX_BE");
 		/* We only track the latest USB headset plugged in */
 		data->active_idx = card_idx;
+
+		set_bit(card_idx, &data->available_card_slot);
+		data->status[card_idx].num_pcm = num_pcm;
+		data->status[card_idx].chip_index = chip_idx;
 	} else {
-		snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
+		clear_bit(card_idx, &data->available_card_slot);
+		data->status[card_idx].num_pcm = 0;
+		data->status[card_idx].chip_index = 0;
+		if (!data->available_card_slot)
+			snd_soc_dapm_disable_pin(dapm, "USB_RX_BE");
 	}
 	snd_soc_dapm_sync(dapm);
 
diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c
index cdce1bb42df5..84dc6d0b2eab 100644
--- a/sound/soc/soc-usb.c
+++ b/sound/soc/soc-usb.c
@@ -95,7 +95,7 @@  EXPORT_SYMBOL_GPL(snd_soc_usb_find_format);
  */
 struct snd_soc_usb *snd_soc_usb_add_port(struct device *dev, void *priv,
 			int (*connection_cb)(struct snd_soc_usb *usb, int card_idx,
-			int connected))
+			int chip_idx, int num_pcm, int connected))
 {
 	struct snd_soc_usb *usb;
 
@@ -149,7 +149,8 @@  EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);
  * handle how the USB backend selects, which device to enable offloading on.
  *
  */
-int snd_soc_usb_connect(struct device *usbdev, int card_idx)
+int snd_soc_usb_connect(struct device *usbdev, int card_idx, int chip_idx,
+			int num_pcm)
 {
 	struct snd_soc_usb *ctx;
 
@@ -161,7 +162,8 @@  int snd_soc_usb_connect(struct device *usbdev, int card_idx)
 		return -ENODEV;
 
 	if (ctx->connection_status_cb)
-		ctx->connection_status_cb(ctx, card_idx, 1);
+		ctx->connection_status_cb(ctx, card_idx, chip_idx,
+						num_pcm, 1);
 
 	return 0;
 }
@@ -174,7 +176,7 @@  EXPORT_SYMBOL_GPL(snd_soc_usb_connect);
  * Notify of a new USB SND device disconnection to the USB backend.
  *
  */
-int snd_soc_usb_disconnect(struct device *usbdev)
+int snd_soc_usb_disconnect(struct device *usbdev, int card_idx)
 {
 	struct snd_soc_usb *ctx;
 
@@ -186,7 +188,7 @@  int snd_soc_usb_disconnect(struct device *usbdev)
 		return -ENODEV;
 
 	if (ctx->connection_status_cb)
-		ctx->connection_status_cb(ctx, -1, 0);
+		ctx->connection_status_cb(ctx, card_idx, 0, 0, 0);
 
 	return 0;
 }
diff --git a/sound/usb/qcom/qc_audio_offload.c b/sound/usb/qcom/qc_audio_offload.c
index a2ae0d31df97..c90824751da9 100644
--- a/sound/usb/qcom/qc_audio_offload.c
+++ b/sound/usb/qcom/qc_audio_offload.c
@@ -1591,7 +1591,8 @@  static void qc_usb_audio_offload_probe(struct snd_usb_audio *chip)
 	atomic_inc(&uaudio_qdev->qdev_in_use);
 	mutex_unlock(&qdev_mutex);
 
-	snd_soc_usb_connect(usb_get_usb_backend(udev), chip->index);
+	snd_soc_usb_connect(usb_get_usb_backend(udev), chip->card->number,
+				chip->index, chip->pcm_devs);
 }
 
 /**
@@ -1677,12 +1678,12 @@  static void qc_usb_audio_offload_disconnect(struct snd_usb_audio *chip)
 	dev->chip = NULL;
 	mutex_unlock(&chip->mutex);
 
+	snd_soc_usb_disconnect(usb_get_usb_backend(udev), card_num);
+
 	mutex_lock(&qdev_mutex);
 	atomic_dec(&uaudio_qdev->qdev_in_use);
-	if (!atomic_read(&uaudio_qdev->qdev_in_use)) {
-		snd_soc_usb_disconnect(usb_get_usb_backend(udev));
+	if (!atomic_read(&uaudio_qdev->qdev_in_use))
 		qc_usb_audio_cleanup_qmi_dev();
-	}
 	mutex_unlock(&qdev_mutex);
 
 	xhci_sideband_unregister(dev->sb);