| Message ID | 20240801011730.4797-10-quic_wcheng@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
> +/** > + * struct snd_soc_usb_device > + * @card_idx - sound card index associated with USB device > + * @pcm_idx - PCM device index associated with USB device > + * @chip_idx - USB sound chip array index > + * @num_playback - number of playback streams > + * @num_capture - number of capture streams so here we have a clear separation between playback and capture... > + * @list - list head for SoC USB devices > + **/ > +struct snd_soc_usb_device { > + int card_idx; > + int pcm_idx; > + int chip_idx; > + int num_playback; > + int num_capture; > + struct list_head list; > +}; > + > +/** > + * struct snd_soc_usb > + * @list - list head for SND SOC struct list > + * @component - reference to ASoC component > + * @num_supported_streams - number of supported concurrent sessions ... but here we don't. And it's not clear what the working 'sessions' means in the comment. > + * @connection_status_cb - callback to notify connection events > + * @priv_data - driver data > + **/ > +struct snd_soc_usb { > + struct list_head list; > + struct snd_soc_component *component; > + unsigned int num_supported_streams; > + int (*connection_status_cb)(struct snd_soc_usb *usb, > + struct snd_soc_usb_device *sdev, bool connected); > + void *priv_data; > +}; > +/** > + * snd_soc_usb_allocate_port() - allocate a SOC USB device USB port? > + * @component: USB DPCM backend DAI component > + * @num_streams: number of offloading sessions supported same comment, is this direction-specific or not? > + * @data: private data > + * > + * Allocate and initialize a SOC USB device. This will populate parameters that > + * are used in subsequent sequences. > + * > + */ > +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, > + int num_streams, void *data) > +{ > + struct snd_soc_usb *usb; > + > + usb = kzalloc(sizeof(*usb), GFP_KERNEL); > + if (!usb) > + return ERR_PTR(-ENOMEM); > + > + usb->component = component; > + usb->priv_data = data; > + usb->num_supported_streams = num_streams; > + > + return usb; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); > + > +/** > + * snd_soc_usb_free_port() - free a SOC USB device > + * @usb: allocated SOC USB device > + > + * Free and remove the SOC USB device from the available list of devices. Now I am lost again on the device:port relationship. I am sure you've explained this before but I forget things and the code isn't self-explanatory. > + * > + */ > +void snd_soc_usb_free_port(struct snd_soc_usb *usb) > +{ > + snd_soc_usb_remove_port(usb); > + kfree(usb); > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_free_port); > + > +/** > + * snd_soc_usb_add_port() - Add a USB backend port > + * @usb: soc usb device to add > + * > + * Register a USB backend device to the SND USB SOC framework. Memory is > + * allocated as part of the USB backend device. > + * > + */ > +void snd_soc_usb_add_port(struct snd_soc_usb *usb) > +{ > + mutex_lock(&ctx_mutex); > + list_add_tail(&usb->list, &usb_ctx_list); > + mutex_unlock(&ctx_mutex); > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); > + > +/** > + * snd_soc_usb_remove_port() - Remove a USB backend port > + * @usb: soc usb device to remove > + * > + * Remove a USB backend device from USB SND SOC. Memory is freed when USB > + * backend is removed. > + * > + */ > +void snd_soc_usb_remove_port(struct snd_soc_usb *usb) > +{ > + struct snd_soc_usb *ctx, *tmp; > + > + mutex_lock(&ctx_mutex); > + list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) { > + if (ctx == usb) { > + list_del(&ctx->list); > + break; > + } > + } > + mutex_unlock(&ctx_mutex); > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);
Hi Pierre, On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote: > > >> +/** >> + * struct snd_soc_usb_device >> + * @card_idx - sound card index associated with USB device >> + * @pcm_idx - PCM device index associated with USB device >> + * @chip_idx - USB sound chip array index >> + * @num_playback - number of playback streams >> + * @num_capture - number of capture streams > so here we have a clear separation between playback and capture... Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated. I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path. I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support. Is it safe to assume that this is the right thinking? If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below) Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented. >> + * @list - list head for SoC USB devices >> + **/ >> +struct snd_soc_usb_device { >> + int card_idx; >> + int pcm_idx; >> + int chip_idx; >> + int num_playback; >> + int num_capture; >> + struct list_head list; >> +}; >> + >> +/** >> + * struct snd_soc_usb >> + * @list - list head for SND SOC struct list >> + * @component - reference to ASoC component >> + * @num_supported_streams - number of supported concurrent sessions > ... but here we don't. And it's not clear what the working 'sessions' > means in the comment. > >> + * @connection_status_cb - callback to notify connection events >> + * @priv_data - driver data >> + **/ >> +struct snd_soc_usb { >> + struct list_head list; >> + struct snd_soc_component *component; >> + unsigned int num_supported_streams; >> + int (*connection_status_cb)(struct snd_soc_usb *usb, >> + struct snd_soc_usb_device *sdev, bool connected); >> + void *priv_data; >> +}; >> +/** >> + * snd_soc_usb_allocate_port() - allocate a SOC USB device > USB port? Noted, refer to the last comment. >> + * @component: USB DPCM backend DAI component >> + * @num_streams: number of offloading sessions supported > same comment, is this direction-specific or not? Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >> + * @data: private data >> + * >> + * Allocate and initialize a SOC USB device. This will populate parameters that >> + * are used in subsequent sequences. >> + * >> + */ >> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >> + int num_streams, void *data) >> +{ >> + struct snd_soc_usb *usb; >> + >> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >> + if (!usb) >> + return ERR_PTR(-ENOMEM); >> + >> + usb->component = component; >> + usb->priv_data = data; >> + usb->num_supported_streams = num_streams; >> + >> + return usb; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >> + >> +/** >> + * snd_soc_usb_free_port() - free a SOC USB device >> + * @usb: allocated SOC USB device >> + >> + * Free and remove the SOC USB device from the available list of devices. > Now I am lost again on the device:port relationship. I am sure you've > explained this before but I forget things and the code isn't > self-explanatory. > Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. Thanks Wesley Cheng >> + * >> + */ >> +void snd_soc_usb_free_port(struct snd_soc_usb *usb) >> +{ >> + snd_soc_usb_remove_port(usb); >> + kfree(usb); >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_free_port); >> + >> +/** >> + * snd_soc_usb_add_port() - Add a USB backend port >> + * @usb: soc usb device to add >> + * >> + * Register a USB backend device to the SND USB SOC framework. Memory is >> + * allocated as part of the USB backend device. >> + * >> + */ >> +void snd_soc_usb_add_port(struct snd_soc_usb *usb) >> +{ >> + mutex_lock(&ctx_mutex); >> + list_add_tail(&usb->list, &usb_ctx_list); >> + mutex_unlock(&ctx_mutex); >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); >> + >> +/** >> + * snd_soc_usb_remove_port() - Remove a USB backend port >> + * @usb: soc usb device to remove >> + * >> + * Remove a USB backend device from USB SND SOC. Memory is freed when USB >> + * backend is removed. >> + * >> + */ >> +void snd_soc_usb_remove_port(struct snd_soc_usb *usb) >> +{ >> + struct snd_soc_usb *ctx, *tmp; >> + >> + mutex_lock(&ctx_mutex); >> + list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) { >> + if (ctx == usb) { >> + list_del(&ctx->list); >> + break; >> + } >> + } >> + mutex_unlock(&ctx_mutex); >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port);
On 8/1/24 23:43, Wesley Cheng wrote: > Hi Pierre, > > On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote: >> >> >>> +/** >>> + * struct snd_soc_usb_device >>> + * @card_idx - sound card index associated with USB device >>> + * @pcm_idx - PCM device index associated with USB device >>> + * @chip_idx - USB sound chip array index >>> + * @num_playback - number of playback streams >>> + * @num_capture - number of capture streams >> so here we have a clear separation between playback and capture... > > Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated. > > I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path. I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support. Is it safe to assume that this is the right thinking? If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below) Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented. I don't think it's as simple as playback only or playback+capture. If there is no support for capture, then there is also no support for devices with implicit feedback - which uses the capture path. So you gradually start drawing a jagged boundary of what is supported and what isn't. My preference would be to add capture in APIs where we can, with TODOs added to make sure no one us under any illusion that the code is fully tested. But at least some of the basic plumbing will be in place. Takashi should chime in on this... >>> + * @list - list head for SoC USB devices >>> + **/ >>> +struct snd_soc_usb_device { >>> + int card_idx; >>> + int pcm_idx; >>> + int chip_idx; >>> + int num_playback; >>> + int num_capture; >>> + struct list_head list; >>> +}; >>> + >>> +/** >>> + * struct snd_soc_usb >>> + * @list - list head for SND SOC struct list >>> + * @component - reference to ASoC component >>> + * @num_supported_streams - number of supported concurrent sessions >> ... but here we don't. And it's not clear what the working 'sessions' >> means in the comment. >> >>> + * @connection_status_cb - callback to notify connection events >>> + * @priv_data - driver data >>> + **/ >>> +struct snd_soc_usb { >>> + struct list_head list; >>> + struct snd_soc_component *component; >>> + unsigned int num_supported_streams; >>> + int (*connection_status_cb)(struct snd_soc_usb *usb, >>> + struct snd_soc_usb_device *sdev, bool connected); >>> + void *priv_data; >>> +}; >>> +/** >>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device >> USB port? > Noted, refer to the last comment. >>> + * @component: USB DPCM backend DAI component >>> + * @num_streams: number of offloading sessions supported >> same comment, is this direction-specific or not? > Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >>> + * @data: private data >>> + * >>> + * Allocate and initialize a SOC USB device. This will populate parameters that >>> + * are used in subsequent sequences. >>> + * >>> + */ >>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >>> + int num_streams, void *data) >>> +{ >>> + struct snd_soc_usb *usb; >>> + >>> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >>> + if (!usb) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + usb->component = component; >>> + usb->priv_data = data; >>> + usb->num_supported_streams = num_streams; >>> + >>> + return usb; >>> +} >>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >>> + >>> +/** >>> + * snd_soc_usb_free_port() - free a SOC USB device >>> + * @usb: allocated SOC USB device >>> + >>> + * Free and remove the SOC USB device from the available list of devices. >> Now I am lost again on the device:port relationship. I am sure you've >> explained this before but I forget things and the code isn't >> self-explanatory. >> > Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. I am not sure USB uses 'port' at all. If by 'port' you meant 'connector' it's not quite right, USB audio works across hubs.
Hi Pierre, On 8/1/2024 11:26 PM, Pierre-Louis Bossart wrote: > > On 8/1/24 23:43, Wesley Cheng wrote: >> Hi Pierre, >> >> On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote: >>> >>>> +/** >>>> + * struct snd_soc_usb_device >>>> + * @card_idx - sound card index associated with USB device >>>> + * @pcm_idx - PCM device index associated with USB device >>>> + * @chip_idx - USB sound chip array index >>>> + * @num_playback - number of playback streams >>>> + * @num_capture - number of capture streams >>> so here we have a clear separation between playback and capture... >> Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated. >> >> I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path. I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support. Is it safe to assume that this is the right thinking? If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below) Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented. > I don't think it's as simple as playback only or playback+capture. If > there is no support for capture, then there is also no support for > devices with implicit feedback - which uses the capture path. So you > gradually start drawing a jagged boundary of what is supported and what > isn't. > > My preference would be to add capture in APIs where we can, with TODOs > added to make sure no one us under any illusion that the code is fully > tested. But at least some of the basic plumbing will be in place. > > Takashi should chime in on this... > >>>> + * @list - list head for SoC USB devices >>>> + **/ >>>> +struct snd_soc_usb_device { >>>> + int card_idx; >>>> + int pcm_idx; >>>> + int chip_idx; >>>> + int num_playback; >>>> + int num_capture; >>>> + struct list_head list; >>>> +}; >>>> + >>>> +/** >>>> + * struct snd_soc_usb >>>> + * @list - list head for SND SOC struct list >>>> + * @component - reference to ASoC component >>>> + * @num_supported_streams - number of supported concurrent sessions >>> ... but here we don't. And it's not clear what the working 'sessions' >>> means in the comment. >>> >>>> + * @connection_status_cb - callback to notify connection events >>>> + * @priv_data - driver data >>>> + **/ >>>> +struct snd_soc_usb { >>>> + struct list_head list; >>>> + struct snd_soc_component *component; >>>> + unsigned int num_supported_streams; >>>> + int (*connection_status_cb)(struct snd_soc_usb *usb, >>>> + struct snd_soc_usb_device *sdev, bool connected); >>>> + void *priv_data; >>>> +}; >>>> +/** >>>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device >>> USB port? >> Noted, refer to the last comment. >>>> + * @component: USB DPCM backend DAI component >>>> + * @num_streams: number of offloading sessions supported >>> same comment, is this direction-specific or not? >> Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >>>> + * @data: private data >>>> + * >>>> + * Allocate and initialize a SOC USB device. This will populate parameters that >>>> + * are used in subsequent sequences. >>>> + * >>>> + */ >>>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >>>> + int num_streams, void *data) >>>> +{ >>>> + struct snd_soc_usb *usb; >>>> + >>>> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >>>> + if (!usb) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + usb->component = component; >>>> + usb->priv_data = data; >>>> + usb->num_supported_streams = num_streams; >>>> + >>>> + return usb; >>>> +} >>>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >>>> + >>>> +/** >>>> + * snd_soc_usb_free_port() - free a SOC USB device >>>> + * @usb: allocated SOC USB device >>>> + >>>> + * Free and remove the SOC USB device from the available list of devices. >>> Now I am lost again on the device:port relationship. I am sure you've >>> explained this before but I forget things and the code isn't >>> self-explanatory. >>> >> Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. > I am not sure USB uses 'port' at all. If by 'port' you meant 'connector' > it's not quite right, USB audio works across hubs. > Remember, this is technically the term used to explain the channel created for ASoC to communicate w/ USB. If we use a term like "device," USB devices come and go, but this ASoC path won't be unallocated along with the USB device, since it does service/know about all the available USB devices connected to the system. (ie through usb hubs) Thanks Wesley Cheng
Hi Pierre, On 8/1/2024 11:26 PM, Pierre-Louis Bossart wrote: > > On 8/1/24 23:43, Wesley Cheng wrote: >> Hi Pierre, >> >> On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote: >>> >>>> +/** >>>> + * struct snd_soc_usb_device >>>> + * @card_idx - sound card index associated with USB device >>>> + * @pcm_idx - PCM device index associated with USB device >>>> + * @chip_idx - USB sound chip array index >>>> + * @num_playback - number of playback streams >>>> + * @num_capture - number of capture streams >>> so here we have a clear separation between playback and capture... >> Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated. >> >> I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path. I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support. Is it safe to assume that this is the right thinking? If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below) Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented. > I don't think it's as simple as playback only or playback+capture. If > there is no support for capture, then there is also no support for > devices with implicit feedback - which uses the capture path. So you > gradually start drawing a jagged boundary of what is supported and what > isn't. > > My preference would be to add capture in APIs where we can, with TODOs > added to make sure no one us under any illusion that the code is fully > tested. But at least some of the basic plumbing will be in place. > > Takashi should chime in on this... > >>>> + * @list - list head for SoC USB devices >>>> + **/ >>>> +struct snd_soc_usb_device { >>>> + int card_idx; >>>> + int pcm_idx; >>>> + int chip_idx; >>>> + int num_playback; >>>> + int num_capture; >>>> + struct list_head list; >>>> +}; >>>> + >>>> +/** >>>> + * struct snd_soc_usb >>>> + * @list - list head for SND SOC struct list >>>> + * @component - reference to ASoC component >>>> + * @num_supported_streams - number of supported concurrent sessions >>> ... but here we don't. And it's not clear what the working 'sessions' >>> means in the comment. After taking a look at this "num_supported_streams" naming a bit more, I wanted to check with you to see adds to the complexity of the terminology being used across soc-usb. The intention of this is to define how many concurrent USB devices the USB backend can support. So for example, if the audio DSP did support multiple USB devices at the same time, this would denote that. This is where I wanted to make sure the terminology was right.... So in this case, to me, it makes more sense if num_supported_streams --> num_supported_devices, because it determines how many USB devices the ASoC USB backend DAI can manage/support. This adds a bit to the reason why I think using the term "port" for explaining the SOC USB context is reasonable. Thanks Wesley Cheng >>>> + * @connection_status_cb - callback to notify connection events >>>> + * @priv_data - driver data >>>> + **/ >>>> +struct snd_soc_usb { >>>> + struct list_head list; >>>> + struct snd_soc_component *component; >>>> + unsigned int num_supported_streams; >>>> + int (*connection_status_cb)(struct snd_soc_usb *usb, >>>> + struct snd_soc_usb_device *sdev, bool connected); >>>> + void *priv_data; >>>> +}; >>>> +/** >>>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device >>> USB port? >> Noted, refer to the last comment. >>>> + * @component: USB DPCM backend DAI component >>>> + * @num_streams: number of offloading sessions supported >>> same comment, is this direction-specific or not? >> Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >>>> + * @data: private data >>>> + * >>>> + * Allocate and initialize a SOC USB device. This will populate parameters that >>>> + * are used in subsequent sequences. >>>> + * >>>> + */ >>>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >>>> + int num_streams, void *data) >>>> +{ >>>> + struct snd_soc_usb *usb; >>>> + >>>> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >>>> + if (!usb) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + usb->component = component; >>>> + usb->priv_data = data; >>>> + usb->num_supported_streams = num_streams; >>>> + >>>> + return usb; >>>> +} >>>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >>>> + >>>> +/** >>>> + * snd_soc_usb_free_port() - free a SOC USB device >>>> + * @usb: allocated SOC USB device >>>> + >>>> + * Free and remove the SOC USB device from the available list of devices. >>> Now I am lost again on the device:port relationship. I am sure you've >>> explained this before but I forget things and the code isn't >>> self-explanatory. >>> >> Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. > I am not sure USB uses 'port' at all. If by 'port' you meant 'connector' > it's not quite right, USB audio works across hubs. > >
>>>>> + * @list - list head for SoC USB devices >>>>> + **/ >>>>> +struct snd_soc_usb_device { >>>>> + int card_idx; >>>>> + int pcm_idx; >>>>> + int chip_idx; >>>>> + int num_playback; >>>>> + int num_capture; >>>>> + struct list_head list; >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct snd_soc_usb >>>>> + * @list - list head for SND SOC struct list >>>>> + * @component - reference to ASoC component >>>>> + * @num_supported_streams - number of supported concurrent sessions >>>> ... but here we don't. And it's not clear what the working 'sessions' >>>> means in the comment. > > After taking a look at this "num_supported_streams" naming a bit more, I wanted to check with you to see adds to the complexity of the terminology being used across soc-usb. > > The intention of this is to define how many concurrent USB devices the USB backend can support. So for example, if the audio DSP did support multiple USB devices at the same time, this would denote that. This is where I wanted to make sure the terminology was right.... So in this case, to me, it makes more sense if num_supported_streams --> num_supported_devices, because it determines how many USB devices the ASoC USB backend DAI can manage/support. This adds a bit to the reason why I think using the term "port" for explaining the SOC USB context is reasonable. IIRC the USB specs define a hierarchy of device/interface/endpoint concepts. For streaming the only thing that really matters is the number of data endpoints, isn't it? If you have two devices with a single endpoint each or one device with two endpoints it should be the same complexity at the DSP level?
Hi Pierre, On 8/6/2024 12:52 PM, Wesley Cheng wrote: > Hi Pierre, > > On 8/1/2024 11:26 PM, Pierre-Louis Bossart wrote: >> On 8/1/24 23:43, Wesley Cheng wrote: >>> Hi Pierre, >>> >>> On 8/1/2024 1:02 AM, Pierre-Louis Bossart wrote: >>>>> +/** >>>>> + * struct snd_soc_usb_device >>>>> + * @card_idx - sound card index associated with USB device >>>>> + * @pcm_idx - PCM device index associated with USB device >>>>> + * @chip_idx - USB sound chip array index >>>>> + * @num_playback - number of playback streams >>>>> + * @num_capture - number of capture streams >>>> so here we have a clear separation between playback and capture... >>> Thanks for the quick review of the series, I know that its a lot of work, so its much appreciated. >>> >>> I guess in the past revisions there was some discussions that highlighted on the fact that, currently, in our QC USB offload implementation we're supporting playback only, and maybe it should be considered to also expand on the capture path. I went ahead and added some sprinkles of that throughout the SOC USB layer, since its vendor agnostic, and some vendors may potentially have that type of support. Is it safe to assume that this is the right thinking? If so, I will go and review some of the spots that may need to consider both playback and capture paths ONLY for soc-usb. (as you highlighted one below) Else, I can note an assumption somewhere that soc-usb supports playback only and add the capture path when implemented. >> I don't think it's as simple as playback only or playback+capture. If >> there is no support for capture, then there is also no support for >> devices with implicit feedback - which uses the capture path. So you >> gradually start drawing a jagged boundary of what is supported and what >> isn't. >> >> My preference would be to add capture in APIs where we can, with TODOs >> added to make sure no one us under any illusion that the code is fully >> tested. But at least some of the basic plumbing will be in place. >> >> Takashi should chime in on this... >> >>>>> + * @list - list head for SoC USB devices >>>>> + **/ >>>>> +struct snd_soc_usb_device { >>>>> + int card_idx; >>>>> + int pcm_idx; >>>>> + int chip_idx; >>>>> + int num_playback; >>>>> + int num_capture; >>>>> + struct list_head list; >>>>> +}; >>>>> + >>>>> +/** >>>>> + * struct snd_soc_usb >>>>> + * @list - list head for SND SOC struct list >>>>> + * @component - reference to ASoC component >>>>> + * @num_supported_streams - number of supported concurrent sessions >>>> ... but here we don't. And it's not clear what the working 'sessions' >>>> means in the comment. >>>> >>>>> + * @connection_status_cb - callback to notify connection events >>>>> + * @priv_data - driver data >>>>> + **/ >>>>> +struct snd_soc_usb { >>>>> + struct list_head list; >>>>> + struct snd_soc_component *component; >>>>> + unsigned int num_supported_streams; >>>>> + int (*connection_status_cb)(struct snd_soc_usb *usb, >>>>> + struct snd_soc_usb_device *sdev, bool connected); >>>>> + void *priv_data; >>>>> +}; >>>>> +/** >>>>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device >>>> USB port? >>> Noted, refer to the last comment. >>>>> + * @component: USB DPCM backend DAI component >>>>> + * @num_streams: number of offloading sessions supported >>>> same comment, is this direction-specific or not? >>> Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >>>>> + * @data: private data >>>>> + * >>>>> + * Allocate and initialize a SOC USB device. This will populate parameters that >>>>> + * are used in subsequent sequences. >>>>> + * >>>>> + */ >>>>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >>>>> + int num_streams, void *data) >>>>> +{ >>>>> + struct snd_soc_usb *usb; >>>>> + >>>>> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >>>>> + if (!usb) >>>>> + return ERR_PTR(-ENOMEM); >>>>> + >>>>> + usb->component = component; >>>>> + usb->priv_data = data; >>>>> + usb->num_supported_streams = num_streams; >>>>> + >>>>> + return usb; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >>>>> + >>>>> +/** >>>>> + * snd_soc_usb_free_port() - free a SOC USB device >>>>> + * @usb: allocated SOC USB device >>>>> + >>>>> + * Free and remove the SOC USB device from the available list of devices. >>>> Now I am lost again on the device:port relationship. I am sure you've >>>> explained this before but I forget things and the code isn't >>>> self-explanatory. >>>> >>> Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. >> I am not sure USB uses 'port' at all. If by 'port' you meant 'connector' >> it's not quite right, USB audio works across hubs. >> > Remember, this is technically the term used to explain the channel created for ASoC to communicate w/ USB. If we use a term like "device," USB devices come and go, but this ASoC path won't be unallocated along with the USB device, since it does service/know about all the available USB devices connected to the system. (ie through usb hubs) > How about snd_soc_usb_allocate_link()? This is technically allocating the soc-usb structure which is the entity that connects the ASoC to ALSA. Thanks Wesley Cheng >
>>>>>> +/** >>>>>> + * snd_soc_usb_allocate_port() - allocate a SOC USB device >>>>> USB port? >>>> Noted, refer to the last comment. >>>>>> + * @component: USB DPCM backend DAI component >>>>>> + * @num_streams: number of offloading sessions supported >>>>> same comment, is this direction-specific or not? >>>> Depending on what you think about my first comment above, I'll also fix or remove the concept of direction entirely. >>>>>> + * @data: private data >>>>>> + * >>>>>> + * Allocate and initialize a SOC USB device. This will populate parameters that >>>>>> + * are used in subsequent sequences. >>>>>> + * >>>>>> + */ >>>>>> +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, >>>>>> + int num_streams, void *data) >>>>>> +{ >>>>>> + struct snd_soc_usb *usb; >>>>>> + >>>>>> + usb = kzalloc(sizeof(*usb), GFP_KERNEL); >>>>>> + if (!usb) >>>>>> + return ERR_PTR(-ENOMEM); >>>>>> + >>>>>> + usb->component = component; >>>>>> + usb->priv_data = data; >>>>>> + usb->num_supported_streams = num_streams; >>>>>> + >>>>>> + return usb; >>>>>> +} >>>>>> +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); >>>>>> + >>>>>> +/** >>>>>> + * snd_soc_usb_free_port() - free a SOC USB device >>>>>> + * @usb: allocated SOC USB device >>>>>> + >>>>>> + * Free and remove the SOC USB device from the available list of devices. >>>>> Now I am lost again on the device:port relationship. I am sure you've >>>>> explained this before but I forget things and the code isn't >>>>> self-explanatory. >>>>> >>>> Ok, I think the problem is that I'm interchanging the port and device terminology, because from the USB perspective its one device connected to a USB port, so its a one-to-one relation. Removing that mindset, I think the proper term here would still be "port," because in the end SOC USB is always only servicing a port. If this is the case, do you have any objections using this terminology in the Q6AFE as well as ASoC? I will use consistent wording throughout SOC USB if so. >>> I am not sure USB uses 'port' at all. If by 'port' you meant 'connector' >>> it's not quite right, USB audio works across hubs. >>> >> Remember, this is technically the term used to explain the channel created for ASoC to communicate w/ USB. If we use a term like "device," USB devices come and go, but this ASoC path won't be unallocated along with the USB device, since it does service/know about all the available USB devices connected to the system. (ie through usb hubs) >> > How about snd_soc_usb_allocate_link()? This is technically allocating the soc-usb structure which is the entity that connects the ASoC to ALSA. oh, so if this has nothing to do with a USB device proper, it'd be fine to use 'port', but explain it in the comments, e.g. something along those lines: snd_soc_usb_allocate_port() - allocate a soc-usb port for offload support. The soc-usb port may be used to stream data with ASoC support to different connected USB devices. Plug-unplug events are signaled with a notification but don't directly impact the soc-usb alloc/free.
diff --git a/include/sound/soc-usb.h b/include/sound/soc-usb.h new file mode 100644 index 000000000000..20394b3552cd --- /dev/null +++ b/include/sound/soc-usb.h @@ -0,0 +1,92 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +#ifndef __LINUX_SND_SOC_USB_H +#define __LINUX_SND_SOC_USB_H + +/** + * struct snd_soc_usb_device + * @card_idx - sound card index associated with USB device + * @pcm_idx - PCM device index associated with USB device + * @chip_idx - USB sound chip array index + * @num_playback - number of playback streams + * @num_capture - number of capture streams + * @list - list head for SoC USB devices + **/ +struct snd_soc_usb_device { + int card_idx; + int pcm_idx; + int chip_idx; + int num_playback; + int num_capture; + struct list_head list; +}; + +/** + * struct snd_soc_usb + * @list - list head for SND SOC struct list + * @component - reference to ASoC component + * @num_supported_streams - number of supported concurrent sessions + * @connection_status_cb - callback to notify connection events + * @priv_data - driver data + **/ +struct snd_soc_usb { + struct list_head list; + struct snd_soc_component *component; + unsigned int num_supported_streams; + int (*connection_status_cb)(struct snd_soc_usb *usb, + struct snd_soc_usb_device *sdev, bool connected); + void *priv_data; +}; + +#if IS_ENABLED(CONFIG_SND_SOC_USB) +int snd_soc_usb_connect(struct device *usbdev, struct snd_soc_usb_device *sdev); +int snd_soc_usb_disconnect(struct device *usbdev, struct snd_soc_usb_device *sdev); +void *snd_soc_usb_find_priv_data(struct device *dev); + +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, + int num_streams, void *data); +void snd_soc_usb_free_port(struct snd_soc_usb *usb); +void snd_soc_usb_add_port(struct snd_soc_usb *usb); +void snd_soc_usb_remove_port(struct snd_soc_usb *usb); +#else +static inline int snd_soc_usb_connect(struct device *usbdev, + struct snd_soc_usb_device *sdev) +{ + return -ENODEV; +} + +static inline int snd_soc_usb_disconnect(struct device *usbdev, + struct snd_soc_usb_device *sdev) +{ + return -EINVAL; +} + +static inline void *snd_soc_usb_find_priv_data(struct device *dev) +{ + return NULL; +} + +static inline struct snd_soc_usb *snd_soc_usb_allocate_port( + struct snd_soc_component *component, + int num_streams, void *data) +{ + return ERR_PTR(-ENOMEM); +} + +static inline void snd_soc_usb_free_port(struct snd_soc_usb *usb) +{ } + +static inline void snd_soc_usb_add_port(struct snd_soc_usb *usb) +{ + return -EINVAL; +} + +static inline void snd_soc_usb_remove_port(struct snd_soc_usb *usb) +{ + return -ENODEV; +} +#endif /* IS_ENABLED(CONFIG_SND_SOC_USB) */ +#endif /*__LINUX_SND_SOC_USB_H */ diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index a52afb423b46..c993705c8ac2 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -84,6 +84,16 @@ config SND_SOC_UTILS_KUNIT_TEST config SND_SOC_ACPI tristate +config SND_SOC_USB + tristate "SoC based USB audio offloading" + depends on SND_USB_AUDIO + help + Enable this option if an ASoC platform card has support to handle + USB audio offloading. This enables the SoC USB layer, which will + notifies the ASoC USB DPCM backend DAI link about available USB audio + devices. Based on the notifications, sequences to enable the audio + stream can be taken based on the design. + # All the supported SoCs source "sound/soc/adi/Kconfig" source "sound/soc/amd/Kconfig" diff --git a/sound/soc/Makefile b/sound/soc/Makefile index fd61847dd1eb..adf9d9203778 100644 --- a/sound/soc/Makefile +++ b/sound/soc/Makefile @@ -35,6 +35,8 @@ endif obj-$(CONFIG_SND_SOC_ACPI) += snd-soc-acpi.o +obj-$(CONFIG_SND_SOC_USB) += soc-usb.o + obj-$(CONFIG_SND_SOC) += snd-soc-core.o obj-$(CONFIG_SND_SOC) += codecs/ obj-$(CONFIG_SND_SOC) += generic/ diff --git a/sound/soc/soc-usb.c b/sound/soc/soc-usb.c new file mode 100644 index 000000000000..b8a87e3fc6b2 --- /dev/null +++ b/sound/soc/soc-usb.c @@ -0,0 +1,216 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2022-2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ +#include <linux/of.h> +#include <linux/usb.h> +#include <sound/soc.h> +#include <sound/soc-usb.h> +#include "../usb/card.h" + +static DEFINE_MUTEX(ctx_mutex); +static LIST_HEAD(usb_ctx_list); + +static struct device_node *snd_soc_find_phandle(struct device *dev) +{ + struct device_node *node; + + node = of_parse_phandle(dev->of_node, "usb-soc-be", 0); + if (!node) + return ERR_PTR(-ENODEV); + + return node; +} + +static struct snd_soc_usb *snd_soc_usb_ctx_lookup(struct device_node *node) +{ + struct snd_soc_usb *ctx; + + if (!node) + return NULL; + + mutex_lock(&ctx_mutex); + list_for_each_entry(ctx, &usb_ctx_list, list) { + if (ctx->component->dev->of_node == node) { + mutex_unlock(&ctx_mutex); + return ctx; + } + } + mutex_unlock(&ctx_mutex); + + return NULL; +} + +static struct snd_soc_usb *snd_soc_find_usb_ctx(struct device *dev) +{ + struct snd_soc_usb *ctx; + struct device_node *node; + + node = snd_soc_find_phandle(dev); + if (!IS_ERR(node)) { + ctx = snd_soc_usb_ctx_lookup(node); + of_node_put(node); + } else { + ctx = snd_soc_usb_ctx_lookup(dev->of_node); + } + + return ctx ? ctx : NULL; +} + +/** + * snd_soc_usb_find_priv_data() - Retrieve private data stored + * @dev: device reference + * + * Fetch the private data stored in the USB SND SOC structure. + * + */ +void *snd_soc_usb_find_priv_data(struct device *dev) +{ + struct snd_soc_usb *ctx; + + ctx = snd_soc_find_usb_ctx(dev); + + return ctx ? ctx->priv_data : NULL; +} +EXPORT_SYMBOL_GPL(snd_soc_usb_find_priv_data); + +/** + * snd_soc_usb_allocate_port() - allocate a SOC USB device + * @component: USB DPCM backend DAI component + * @num_streams: number of offloading sessions supported + * @data: private data + * + * Allocate and initialize a SOC USB device. This will populate parameters that + * are used in subsequent sequences. + * + */ +struct snd_soc_usb *snd_soc_usb_allocate_port(struct snd_soc_component *component, + int num_streams, void *data) +{ + struct snd_soc_usb *usb; + + usb = kzalloc(sizeof(*usb), GFP_KERNEL); + if (!usb) + return ERR_PTR(-ENOMEM); + + usb->component = component; + usb->priv_data = data; + usb->num_supported_streams = num_streams; + + return usb; +} +EXPORT_SYMBOL_GPL(snd_soc_usb_allocate_port); + +/** + * snd_soc_usb_free_port() - free a SOC USB device + * @usb: allocated SOC USB device + + * Free and remove the SOC USB device from the available list of devices. + * + */ +void snd_soc_usb_free_port(struct snd_soc_usb *usb) +{ + snd_soc_usb_remove_port(usb); + kfree(usb); +} +EXPORT_SYMBOL_GPL(snd_soc_usb_free_port); + +/** + * snd_soc_usb_add_port() - Add a USB backend port + * @usb: soc usb device to add + * + * Register a USB backend device to the SND USB SOC framework. Memory is + * allocated as part of the USB backend device. + * + */ +void snd_soc_usb_add_port(struct snd_soc_usb *usb) +{ + mutex_lock(&ctx_mutex); + list_add_tail(&usb->list, &usb_ctx_list); + mutex_unlock(&ctx_mutex); +} +EXPORT_SYMBOL_GPL(snd_soc_usb_add_port); + +/** + * snd_soc_usb_remove_port() - Remove a USB backend port + * @usb: soc usb device to remove + * + * Remove a USB backend device from USB SND SOC. Memory is freed when USB + * backend is removed. + * + */ +void snd_soc_usb_remove_port(struct snd_soc_usb *usb) +{ + struct snd_soc_usb *ctx, *tmp; + + mutex_lock(&ctx_mutex); + list_for_each_entry_safe(ctx, tmp, &usb_ctx_list, list) { + if (ctx == usb) { + list_del(&ctx->list); + break; + } + } + mutex_unlock(&ctx_mutex); +} +EXPORT_SYMBOL_GPL(snd_soc_usb_remove_port); + +/** + * snd_soc_usb_connect() - Notification of USB device connection + * @usbdev: USB bus device + * @sdev: USB SND device to add + * + * Notify of a new USB SND device connection. The sdev->card_idx can be used to + * handle how the DPCM backend selects, which device to enable USB offloading + * on. + * + */ +int snd_soc_usb_connect(struct device *usbdev, struct snd_soc_usb_device *sdev) +{ + struct snd_soc_usb *ctx; + + if (!usbdev) + return -ENODEV; + + ctx = snd_soc_find_usb_ctx(usbdev); + if (IS_ERR(ctx)) + return -ENODEV; + + mutex_lock(&ctx_mutex); + if (ctx->connection_status_cb) + ctx->connection_status_cb(ctx, sdev, true); + mutex_unlock(&ctx_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_usb_connect); + +/** + * snd_soc_usb_disconnect() - Notification of USB device disconnection + * @usbdev: USB bus device + * @sdev: USB SND device to remove + * + * Notify of a new USB SND device disconnection to the USB backend. + * + */ +int snd_soc_usb_disconnect(struct device *usbdev, struct snd_soc_usb_device *sdev) +{ + struct snd_soc_usb *ctx; + + if (!usbdev) + return -ENODEV; + + ctx = snd_soc_find_usb_ctx(usbdev); + if (IS_ERR(ctx)) + return -ENODEV; + + mutex_lock(&ctx_mutex); + if (ctx->connection_status_cb) + ctx->connection_status_cb(ctx, sdev, false); + mutex_unlock(&ctx_mutex); + + return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_usb_disconnect); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("SoC USB driver for offloading");
Some platforms may have support for offloading USB audio devices to a dedicated audio DSP. Introduce a set of APIs that allow for management of USB sound card and PCM devices enumerated by the USB SND class driver. This allows for the ASoC components to be aware of what USB devices are available for offloading. Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> --- include/sound/soc-usb.h | 92 +++++++++++++++++ sound/soc/Kconfig | 10 ++ sound/soc/Makefile | 2 + sound/soc/soc-usb.c | 216 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 320 insertions(+) create mode 100644 include/sound/soc-usb.h create mode 100644 sound/soc/soc-usb.c