Message ID | 20240425215125.29761-1-quic_wcheng@quicinc.com |
---|---|
Headers | show |
Series | Introduce QC USB SND audio offloading support | expand |
On Thu, Apr 25, 2024 at 02:51:25PM -0700, Wesley Cheng wrote: > With the introduction of the soc-usb driver, add documentation highlighting > details on how to utilize the new driver and how it interacts with > different components in USB SND and ASoC. It provides examples on how to > implement the drivers that will need to be introduced in order to enable > USB audio offloading. > The doc LGTM, thanks! Reviewed-by: Bagas Sanjaya <bagasdotme@gmail.com>
On 4/25/2024 11:50 PM, Wesley Cheng wrote: > Some clients may operate only on a specific XHCI interrupter instance. > Allow for the associated class driver to request for the interrupter that > it requires. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- (...) > - > - /* Find available secondary interrupter, interrupter 0 is reserved for primary */ > + /* Find available secondary interrupter, interrupter 0 is reserverd for primary */ You introduce a typo in here.
On 4/25/2024 11:50 PM, Wesley Cheng wrote: > 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> > --- (...) > +const char *snd_soc_usb_get_components_tag(bool playback) > +{ > + if (playback) > + return "usbplybkoffld: 1"; > + else > + return "usbcapoffld: 1"; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_get_components_tag); Is this used to expose some information to userspace? Can those be some more readable strings if so, like: usbplaybackoffload, usbcaptureoffload (...) > + > + node = snd_soc_find_phandle(usbdev); > + if (IS_ERR(node)) > + return -ENODEV; > + > + ctx = snd_soc_find_usb_ctx(node); > + of_node_put(node); > + if (!ctx) > + return -ENODEV; Perhaps introduce some helper function, you do this snd_soc_find_phandle() followed by snd_soc_find_usb_ctx() in few places...
On 4/25/2024 11:51 PM, Wesley Cheng wrote: > Introduce a helper to check if a particular PCM format is supported by the > USB audio device connected. If the USB audio device does not have an > audio profile which can support the requested format, then notify the USB > backend. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- (...) > +/** > + * snd_soc_usb_find_format() - Check if audio format is supported > + * @card_idx: USB sound chip array index > + * @params: PCM parameters > + * @direction: capture or playback > + * > + * Ensure that a requested audio profile from the ASoC side is able to be > + * supported by the USB device. > + * > + * Return 0 on success, negative on error. > + * > + */ > +int snd_soc_usb_find_format(int card_idx, struct snd_pcm_hw_params *params, > + int direction) Perhaps name function similar to its snd_usb equivalent, so snd_soc_usb_find_supported_format? > +{ > + struct snd_usb_stream *as; > + > + as = snd_usb_find_suppported_substream(card_idx, params, direction); > + if (!as) > + return -EOPNOTSUPP; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_find_format); > + > /** > * snd_soc_usb_allocate_port() - allocate a SOC USB device > * @component: USB DPCM backend DAI component >
On 4/25/2024 11:51 PM, Wesley Cheng wrote: > Expose API for creation of a jack control for notifying of available > devices that are plugged in/discovered, and that support offloading. This > allows for control names to be standardized across implementations of USB > audio offloading. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- (...) > /* SOC USB sound kcontrols */ > +/** > + * snd_soc_usb_setup_offload_jack() - Create USB offloading jack > + * @component: USB DPCM backend DAI component > + * @jack: jack structure to create > + * > + * Creates a jack device for notifying userspace of the availability > + * of an offload capable device. > + * > + * Returns 0 on success, negative on error. > + * > + */ > +int snd_soc_usb_setup_offload_jack(struct snd_soc_component *component, > + struct snd_soc_jack *jack) > +{ > + int ret; > + > + ret = snd_soc_card_jack_new(component->card, "USB Offload Playback Jack", > + SND_JACK_HEADPHONE, jack); > + if (ret < 0) { > + dev_err(component->card->dev, "Unable to add USB offload jack\n"); > + return ret; > + } > + > + ret = snd_soc_component_set_jack(component, jack, NULL); > + if (ret) { > + dev_warn(component->card->dev, "Failed to set jack: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(snd_soc_usb_setup_offload_jack); > + > static int snd_soc_usb_get_offload_card_status(struct snd_kcontrol *kcontrol, > struct snd_ctl_elem_value *ucontrol) > { > I'm not sure if this should be handled in generic USB API, this feels like something that should be handled in specific device driver side, like all users currently do. Anyway I think there should also be some function that tears jack down, by calling: snd_soc_component_set_jack(component, NULL, NULL); so it can get cleaned up properly?
Hi Amadeusz, On 4/26/2024 6:24 AM, Amadeusz Sławiński wrote: > On 4/25/2024 11:50 PM, Wesley Cheng wrote: >> Some clients may operate only on a specific XHCI interrupter instance. >> Allow for the associated class driver to request for the interrupter that >> it requires. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- > > (...) > >> - >> - /* Find available secondary interrupter, interrupter 0 is >> reserved for primary */ >> + /* Find available secondary interrupter, interrupter 0 is >> reserverd for primary */ > > You introduce a typo in here. Thanks for the review! Will fix it. Thanks Wesley Cheng
Hi Amadeusz, On 4/26/2024 6:25 AM, Amadeusz Sławiński wrote: > On 4/25/2024 11:50 PM, Wesley Cheng wrote: >> 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> >> --- > > (...) > >> +const char *snd_soc_usb_get_components_tag(bool playback) >> +{ >> + if (playback) >> + return "usbplybkoffld: 1"; >> + else >> + return "usbcapoffld: 1"; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_get_components_tag); > > Is this used to expose some information to userspace? > Can those be some more readable strings if so, like: > usbplaybackoffload, usbcaptureoffload > Sure we can make it a bit more complete. Was trying to keep it short, but if the intention isn't clear on the tag, then we can keep the full form. > (...) > >> + >> + node = snd_soc_find_phandle(usbdev); >> + if (IS_ERR(node)) >> + return -ENODEV; >> + >> + ctx = snd_soc_find_usb_ctx(node); >> + of_node_put(node); >> + if (!ctx) >> + return -ENODEV; > > Perhaps introduce some helper function, you do this > snd_soc_find_phandle() followed by snd_soc_find_usb_ctx() in few places... > Will do. Will make a helper and replace instances with this. Thanks Wesley Cheng
Hi Amadeusz, On 4/26/2024 6:25 AM, Amadeusz Sławiński wrote: > On 4/25/2024 11:51 PM, Wesley Cheng wrote: >> Introduce a helper to check if a particular PCM format is supported by >> the >> USB audio device connected. If the USB audio device does not have an >> audio profile which can support the requested format, then notify the USB >> backend. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- > > (...) > >> +/** >> + * snd_soc_usb_find_format() - Check if audio format is supported >> + * @card_idx: USB sound chip array index >> + * @params: PCM parameters >> + * @direction: capture or playback >> + * >> + * Ensure that a requested audio profile from the ASoC side is able >> to be >> + * supported by the USB device. >> + * >> + * Return 0 on success, negative on error. >> + * >> + */ >> +int snd_soc_usb_find_format(int card_idx, struct snd_pcm_hw_params >> *params, >> + int direction) > > Perhaps name function similar to its snd_usb equivalent, so > snd_soc_usb_find_supported_format? > Will do. Thanks Wesley Cheng
Hi Mathias, On 4/30/2024 4:02 AM, Mathias Nyman wrote: > On 26.4.2024 0.50, Wesley Cheng wrote: >> Depending on the interrupter use case, the OS may only be used to handle >> the interrupter event ring clean up. In these scenarios, event TRBs >> don't >> need to be handled by the OS, so introduce an xhci interrupter flag to >> tag >> if the events from an interrupter needs to be handled or not. > > Could you elaborate on this a bit. > > If I understood correctly the whole point of requesting a secondary xhci > interrupter > for the sideband device without ever requesting a real interrupt for it > was to avoid > waking up the cpu and calling the interrupt handler. > Yes, this is the correct understanding. We don't currently register the separate interrupt line (from GIC) for the secondary interrupter, so the main apps proc doesn't get interrupted on events generated on the secondary interrupter. > with this flag is seems the normal xhci interrupt handler does get > called for > sideband transfer events. > Main intention was to utilize the refactoring you did to expose the xhci_handle_event_trb() for both handling events on the main interrupter, as well as the logic to skip events on the secondary interrupter. https://lore.kernel.org/linux-usb/44a3d4db-7759-dd93-782a-1efbebfdb22c@linux.intel.com/ >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> drivers/usb/host/xhci-ring.c | 17 +++++++++++++---- >> drivers/usb/host/xhci.h | 1 + >> 2 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index 52278afea94b..6c7a21f522cd 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -2973,14 +2973,22 @@ static int handle_tx_event(struct xhci_hcd *xhci, >> } >> /* >> - * This function handles one OS-owned event on the event ring. It may >> drop >> - * xhci->lock between event processing (e.g. to pass up port status >> changes). >> + * This function handles one OS-owned event on the event ring, or >> ignores one event >> + * on interrupters which are non-OS owned. It may drop xhci->lock >> between event >> + * processing (e.g. to pass up port status changes). >> */ >> static int xhci_handle_event_trb(struct xhci_hcd *xhci, struct >> xhci_interrupter *ir, >> union xhci_trb *event) >> { >> u32 trb_type; >> + /* >> + * Some interrupters do not need to handle event TRBs, as they >> may be >> + * managed by another entity, but rely on the OS to clean up. >> + */ >> + if (ir->skip_events) >> + return 0; >> + > > I think we need another solution than a skip_events flag. > > To make secondary xhci interrupters more useful in general it would make > more > sense to add an interrupt handler function pointer to struct > xhci_interrupter. > > Then call that function instead of xhci_handle_event_trb() > I agree that is how it should be for when support for actually utilizing secondary interrupters for routing events to different targets (instead of offloading). However, since I don't have an existing use case that will exercise this functionality, its a bit difficult to verify that it should be working the way it was intended. > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -3098,8 +3098,8 @@ static int xhci_handle_events(struct xhci_hcd > *xhci, struct xhci_interrupter *ir > > /* Process all OS owned event TRBs on this event ring */ > while (unhandled_event_trb(ir->event_ring)) { > - err = xhci_handle_event_trb(xhci, ir, > ir->event_ring->dequeue); > - > + if (ir->handle_event_trb) > + err = ir->handle_event_trb(xhci, ir, > ir->event_ring->dequeue); > /* > * If half a segment of events have been handled in one > go then > * update ERDP, and force isoc trbs to interrupt more > often > > The handler function would be passed to, and function pointer set in > xhci_create_secondary_interrupter() > > For primary interrupter it would always be set to xhci_handle_event_trb() > Yes, definitely agree with this for when we introduce support for handling the secondary interrupter GIC line within the apps proc itself. Would prefer if we took up that effort in another series, but willing to go back to the skip events loop previously implemented if the above change isn't where you want to go with this. Thanks Wesley Cheng > Thanks > Mathias >
Hi Amadeusz, On 4/26/2024 6:26 AM, Amadeusz Sławiński wrote: > On 4/25/2024 11:51 PM, Wesley Cheng wrote: >> Expose API for creation of a jack control for notifying of available >> devices that are plugged in/discovered, and that support offloading. >> This >> allows for control names to be standardized across implementations of USB >> audio offloading. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- > > (...) > >> /* SOC USB sound kcontrols */ >> +/** >> + * snd_soc_usb_setup_offload_jack() - Create USB offloading jack >> + * @component: USB DPCM backend DAI component >> + * @jack: jack structure to create >> + * >> + * Creates a jack device for notifying userspace of the availability >> + * of an offload capable device. >> + * >> + * Returns 0 on success, negative on error. >> + * >> + */ >> +int snd_soc_usb_setup_offload_jack(struct snd_soc_component *component, >> + struct snd_soc_jack *jack) >> +{ >> + int ret; >> + >> + ret = snd_soc_card_jack_new(component->card, "USB Offload >> Playback Jack", >> + SND_JACK_HEADPHONE, jack); >> + if (ret < 0) { >> + dev_err(component->card->dev, "Unable to add USB offload >> jack\n"); >> + return ret; >> + } >> + >> + ret = snd_soc_component_set_jack(component, jack, NULL); >> + if (ret) { >> + dev_warn(component->card->dev, "Failed to set jack: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(snd_soc_usb_setup_offload_jack); >> + >> static int snd_soc_usb_get_offload_card_status(struct snd_kcontrol >> *kcontrol, >> struct snd_ctl_elem_value *ucontrol) >> { >> > > I'm not sure if this should be handled in generic USB API, this feels > like something that should be handled in specific device driver side, > like all users currently do. > In some of the previous comments, it was mentioned that maybe it was better to have more consistent/defined naming across devices that do have support for audio offload. Initially, I did have these within our vendor specific ASoC platform driver also. > Anyway I think there should also be some function that tears jack down, > by calling: > snd_soc_component_set_jack(component, NULL, NULL); > so it can get cleaned up properly? I can add that. I didn't realize there were some situations where maybe components would want to disable the jack. I will leave the cleanup part to ASoC when the platform card is removed. Thanks Wesley Cheng
On 26.4.2024 0.50, Wesley Cheng wrote: > Some use cases maybe require that the secondary interrupter's events to > be handled by the OS. In this case, configure the IMOD and the > skip_events property to enable the interrupter's events. By default, > assume that the secondary interrupter doesn't want to enable OS event > handling. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > drivers/usb/host/xhci-sideband.c | 28 ++++++++++++++++++++++++++++ > include/linux/usb/xhci-sideband.h | 2 ++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c > index 255feae33c6e..6fdae9840c11 100644 > --- a/drivers/usb/host/xhci-sideband.c > +++ b/drivers/usb/host/xhci-sideband.c > @@ -237,6 +237,30 @@ xhci_sideband_get_event_buffer(struct xhci_sideband *sb) > } > EXPORT_SYMBOL_GPL(xhci_sideband_get_event_buffer); > > +/** > + * xhci_sideband_enable_interrupt - enable interrupt for secondary interrupter > + * @sb: sideband instance for this usb device > + * @imod_interval: number of event ring segments to allocate > + * > + * Enables OS owned event handling for a particular interrupter if client > + * requests for it. In addition, set the IMOD interval for this particular > + * interrupter. > + * > + * Returns 0 on success, negative error otherwise > + */ > +int xhci_sideband_enable_interrupt(struct xhci_sideband *sb, u32 imod_interval) > +{ > + if (!sb || !sb->ir) > + return -ENODEV; > + > + xhci_set_interrupter_moderation(sb->ir, imod_interval); Is there a need to adjust the moderation after initial setup? If not then maybe we could pass the imod_interval as a parameter to xhci_create_secondary_interrupter(), and avoid exporting xhci_set_interrupter_moderation() > + sb->ir->skip_events = false; > + xhci_enable_interrupter(sb->ir); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(xhci_sideband_enable_interrupt); I can't find the place where xhci_sideband_enable_interrupt() is called in this series. How is it planned to be used? Thanks Mathias
Hi Mathias, On 5/2/2024 4:07 AM, Mathias Nyman wrote: > On 26.4.2024 0.50, Wesley Cheng wrote: >> Some use cases maybe require that the secondary interrupter's events to >> be handled by the OS. In this case, configure the IMOD and the >> skip_events property to enable the interrupter's events. By default, >> assume that the secondary interrupter doesn't want to enable OS event >> handling. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> drivers/usb/host/xhci-sideband.c | 28 ++++++++++++++++++++++++++++ >> include/linux/usb/xhci-sideband.h | 2 ++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/drivers/usb/host/xhci-sideband.c >> b/drivers/usb/host/xhci-sideband.c >> index 255feae33c6e..6fdae9840c11 100644 >> --- a/drivers/usb/host/xhci-sideband.c >> +++ b/drivers/usb/host/xhci-sideband.c >> @@ -237,6 +237,30 @@ xhci_sideband_get_event_buffer(struct >> xhci_sideband *sb) >> } >> EXPORT_SYMBOL_GPL(xhci_sideband_get_event_buffer); >> +/** >> + * xhci_sideband_enable_interrupt - enable interrupt for secondary >> interrupter >> + * @sb: sideband instance for this usb device >> + * @imod_interval: number of event ring segments to allocate >> + * >> + * Enables OS owned event handling for a particular interrupter if >> client >> + * requests for it. In addition, set the IMOD interval for this >> particular >> + * interrupter. >> + * >> + * Returns 0 on success, negative error otherwise >> + */ >> +int xhci_sideband_enable_interrupt(struct xhci_sideband *sb, u32 >> imod_interval) >> +{ >> + if (!sb || !sb->ir) >> + return -ENODEV; >> + >> + xhci_set_interrupter_moderation(sb->ir, imod_interval); > > Is there a need to adjust the moderation after initial setup? > > If not then maybe we could pass the imod_interval as a parameter to > xhci_create_secondary_interrupter(), and avoid exporting > xhci_set_interrupter_moderation() > > Let me preface my comments by saying that I was trying to include some aspects of enabling the secondary interrupter line within the main apps proc. If this gets too confusing, I can remove these mechanisms for now. For example, as you mentioned below xhci_sideband_enable_interrupt() isn't going to be used in the offload path. However, I decided to add it so we can have some corresponding function that will utilize/set skip_events = false. (as it is "true" by default) Again, I can remove this part and revisit later when we actually have a use case to handle secondary interrupts on apps. As for the IMOD setting, depending on what you think, I can add it as part of xhci_create_secondary_interrupter() >> + sb->ir->skip_events = false; >> + xhci_enable_interrupter(sb->ir); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(xhci_sideband_enable_interrupt); > > I can't find the place where xhci_sideband_enable_interrupt() is called in > this series. How is it planned to be used? > > Thanks > Mathias Thanks Wesley Cheng