Message ID | 20240823200101.26755-1-quic_wcheng@quicinc.com |
---|---|
Headers | show |
Series | Introduce QC USB SND audio offloading support | expand |
On 8/23/24 22:00, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Expose xhci_stop_endpoint_sync() which is a synchronous variant of > xhci_queue_stop_endpoint(). This is useful for client drivers that are > using the secondary interrupters, and need to stop/clean up the current > session. The stop endpoint command handler will also take care of cleaning > up the ring. > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > --- > drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++ > drivers/usb/host/xhci.h | 2 ++ > 2 files changed, 41 insertions(+) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index 37eb37b0affa..3a051ed32907 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -2784,6 +2784,45 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci, > return -ENOMEM; > } > > +/* > + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and > + * waits for the command completion before returning. > + */ > +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend, > + gfp_t gfp_flags) > +{ > + struct xhci_command *command; > + unsigned long flags; > + int ret; > + > + command = xhci_alloc_command(xhci, true, gfp_flags); > + if (!command) > + return -ENOMEM; > + > + spin_lock_irqsave(&xhci->lock, flags); > + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id, > + ep->ep_index, suspend); > + if (ret < 0) { > + spin_unlock_irqrestore(&xhci->lock, flags); > + goto out; > + } > + > + xhci_ring_cmd_db(xhci); > + spin_unlock_irqrestore(&xhci->lock, flags); > + > + wait_for_completion(command->completion); > + > + if (command->status == COMP_COMMAND_ABORTED || > + command->status == COMP_COMMAND_RING_STOPPED) { > + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n"); nit-pick: is this really a timeout? In that case you would have used wait_for_completion_timeout(), no? > + ret = -ETIME; > + } > +out: > + xhci_free_command(xhci, command); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(xhci_stop_endpoint_sync); > > /* Issue a configure endpoint command or evaluate context command > * and wait for it to finish. > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h > index 30415158ed3c..1c6126ed55b0 100644 > --- a/drivers/usb/host/xhci.h > +++ b/drivers/usb/host/xhci.h > @@ -1914,6 +1914,8 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci, > void xhci_cleanup_command_queue(struct xhci_hcd *xhci); > void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring); > unsigned int count_trbs(u64 addr, u64 len); > +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, > + int suspend, gfp_t gfp_flags); > > /* xHCI roothub code */ > void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,
> +config SND_USB_OFFLOAD_MIXER > + tristate "Qualcomm USB Audio Offload mixer control" > + help > + Say Y to enable the Qualcomm USB audio offloading mixer controls. > + This exposes an USB offload capable kcontrol to signal to > + applications about which platform sound card can support USB > + audio offload. This can potentially be used to fetch further > + information about the offloading status from the platform sound > + card. I would remove reference to Qualcomm for this Kconfig, all the code seems generic to me? Probably a left-over from the previous version.
> Changelog > -------------------------------------------- > Changes in v25: > - Cleanups on typos mentioned within the xHCI layers > - Modified the xHCI interrupter search if clients specify interrupter index > - Moved mixer_usb_offload into its own module, so that other vendor offload USB > modules can utilize it also. > - Added support for USB audio devices that may have multiple PCM streams, as > previous implementation only assumed a single PCM device. SOC USB will be > able to handle an array of PCM indexes supported by the USB audio device. > - Added some additional checks in the QC USB offload driver to check that device > has at least one playback stream before allowing to bind > - Reordered DT bindings to fix the error found by Rob's bot. The patch that > added USB_RX was after the example was updated. > - Updated comments within SOC USB to clarify terminology and to keep it consistent > - Added SND_USB_JACK type for notifying of USB device audio connections I went through the code and didn't find anything that looked like a major blocker. There are still a number of cosmetic things you'd want to fix such as using checkpatch.pl --strict --codespell to look for obvious style issues and typos, see selection below. git am also complains about EOF lines. Overall this is starting to look good and ready for other reviewers to look at. WARNING: 'reaquire' may be misspelled - perhaps 'reacquire'? #54: FILE: drivers/usb/host/xhci-ring.c:3037: + * for non OS owned interrupter event ring. It may drop and reaquire xhci->lock ^^^^^^^^ WARNING: 'compliation' may be misspelled - perhaps 'compilation'? #16: module compliation added by Wesley Cheng to complete original concept code ^^^^^^^^^^^ CHECK: Prefer kzalloc(sizeof(*sgt)...) over kzalloc(sizeof(struct sg_table)...) #105: FILE: drivers/usb/host/xhci-sideband.c:35: + sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL); CHECK: struct mutex definition without comment #557: FILE: include/linux/usb/xhci-sideband.h:35: + struct mutex mutex; WARNING: 'straightfoward' may be misspelled - perhaps 'straightforward'? #22: straightfoward, as the ASoC components have direct references to the ASoC ^^^^^^^^^^^^^^ CHECK: Unnecessary parentheses around 'card == sdev->card_idx' #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217: + if ((card == sdev->card_idx) && + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) { CHECK: Unnecessary parentheses around 'pcm == sdev->ppcm_idx[sdev->num_playback - 1]' #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217: + if ((card == sdev->card_idx) && + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) { WARNING: 'seqeunces' may be misspelled - perhaps 'sequences'? #8: seqeunces. This allows for platform USB SND modules to properly initialize ^^^^^^^^^ WARNING: 'exisiting' may be misspelled - perhaps 'existing'? #12: exisiting parameters. ^^^^^^^^^ CHECK: Please use a blank line after function/struct/union/enum declarations #1020: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:98: +}; +#define QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN 46 CHECK: Please use a blank line after function/struct/union/enum declarations #1054: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:132: +}; +#define QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN 202 CHECK: Please use a blank line after function/struct/union/enum declarations #1081: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:159: +}; +#define QMI_UAUDIO_STREAM_IND_MSG_V01_MAX_MSG_LEN 181 CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues #100: FILE: sound/usb/mixer_usb_offload.c:19: +#define PCM_IDX(n) (n & 0xffff) CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues #101: FILE: sound/usb/mixer_usb_offload.c:20: +#define CARD_IDX(n) (n >> 16)
Hi Pierre, On 8/26/2024 1:48 AM, Pierre-Louis Bossart wrote: > > On 8/23/24 22:00, Wesley Cheng wrote: >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> >> Expose xhci_stop_endpoint_sync() which is a synchronous variant of >> xhci_queue_stop_endpoint(). This is useful for client drivers that are >> using the secondary interrupters, and need to stop/clean up the current >> session. The stop endpoint command handler will also take care of cleaning >> up the ring. >> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> --- >> drivers/usb/host/xhci.c | 39 +++++++++++++++++++++++++++++++++++++++ >> drivers/usb/host/xhci.h | 2 ++ >> 2 files changed, 41 insertions(+) >> >> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >> index 37eb37b0affa..3a051ed32907 100644 >> --- a/drivers/usb/host/xhci.c >> +++ b/drivers/usb/host/xhci.c >> @@ -2784,6 +2784,45 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci, >> return -ENOMEM; >> } >> >> +/* >> + * Synchronous XHCI stop endpoint helper. Issues the stop endpoint command and >> + * waits for the command completion before returning. >> + */ >> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend, >> + gfp_t gfp_flags) >> +{ >> + struct xhci_command *command; >> + unsigned long flags; >> + int ret; >> + >> + command = xhci_alloc_command(xhci, true, gfp_flags); >> + if (!command) >> + return -ENOMEM; >> + >> + spin_lock_irqsave(&xhci->lock, flags); >> + ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id, >> + ep->ep_index, suspend); >> + if (ret < 0) { >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + goto out; >> + } >> + >> + xhci_ring_cmd_db(xhci); >> + spin_unlock_irqrestore(&xhci->lock, flags); >> + >> + wait_for_completion(command->completion); >> + >> + if (command->status == COMP_COMMAND_ABORTED || >> + command->status == COMP_COMMAND_RING_STOPPED) { >> + xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n"); > nit-pick: is this really a timeout? In that case you would have used > wait_for_completion_timeout(), no? With respects to the xHCI command implementation, every time a command is queued to the host controller, it arms timer (xhci->cmd_timer) that is used to handle the timeout conditions. This is the reason for not using the _timeout() variant, as we can let the xHCI command timeout handler do the cleanup and stopping of the HCD. (marking as dead) It will also ensure that any completion events are completed as part of the timeout handler as well (xhci_handle_command_timeout() --> xhci_abort_cmd_ring()) Thanks Wesley Cheng >> + ret = -ETIME; >> + } >> +out: >> + xhci_free_command(xhci, command); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(xhci_stop_endpoint_sync); >> >> /* Issue a configure endpoint command or evaluate context command >> * and wait for it to finish. >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >> index 30415158ed3c..1c6126ed55b0 100644 >> --- a/drivers/usb/host/xhci.h >> +++ b/drivers/usb/host/xhci.h >> @@ -1914,6 +1914,8 @@ void xhci_ring_doorbell_for_active_rings(struct xhci_hcd *xhci, >> void xhci_cleanup_command_queue(struct xhci_hcd *xhci); >> void inc_deq(struct xhci_hcd *xhci, struct xhci_ring *ring); >> unsigned int count_trbs(u64 addr, u64 len); >> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, >> + int suspend, gfp_t gfp_flags); >> >> /* xHCI roothub code */ >> void xhci_set_link_state(struct xhci_hcd *xhci, struct xhci_port *port,
Hi Pierre, On 8/26/2024 2:09 AM, Pierre-Louis Bossart wrote: > >> +config SND_USB_OFFLOAD_MIXER >> + tristate "Qualcomm USB Audio Offload mixer control" >> + help >> + Say Y to enable the Qualcomm USB audio offloading mixer controls. >> + This exposes an USB offload capable kcontrol to signal to >> + applications about which platform sound card can support USB >> + audio offload. This can potentially be used to fetch further >> + information about the offloading status from the platform sound >> + card. > I would remove reference to Qualcomm for this Kconfig, all the code > seems generic to me? Probably a left-over from the previous version. Ah, yes will remove any vendor stuff. Thanks Wesley Cheng
Hi Pierre, On 8/26/2024 2:28 AM, Pierre-Louis Bossart wrote: >> Changelog >> -------------------------------------------- >> Changes in v25: >> - Cleanups on typos mentioned within the xHCI layers >> - Modified the xHCI interrupter search if clients specify interrupter index >> - Moved mixer_usb_offload into its own module, so that other vendor offload USB >> modules can utilize it also. >> - Added support for USB audio devices that may have multiple PCM streams, as >> previous implementation only assumed a single PCM device. SOC USB will be >> able to handle an array of PCM indexes supported by the USB audio device. >> - Added some additional checks in the QC USB offload driver to check that device >> has at least one playback stream before allowing to bind >> - Reordered DT bindings to fix the error found by Rob's bot. The patch that >> added USB_RX was after the example was updated. >> - Updated comments within SOC USB to clarify terminology and to keep it consistent >> - Added SND_USB_JACK type for notifying of USB device audio connections > I went through the code and didn't find anything that looked like a > major blocker. There are still a number of cosmetic things you'd want to > fix such as using checkpatch.pl --strict --codespell to look for obvious > style issues and typos, see selection below. git am also complains about > EOF lines. Thanks for the consistent reviews. Will fix the checkpatch errors and mis-spells. I didn't have codespell added so fixed that and resolved the typos. Thanks Wesley Cheng > Overall this is starting to look good and ready for other reviewers to > look at. > > > > WARNING: 'reaquire' may be misspelled - perhaps 'reacquire'? > #54: FILE: drivers/usb/host/xhci-ring.c:3037: > + * for non OS owned interrupter event ring. It may drop and reaquire > xhci->lock > ^^^^^^^^ > WARNING: 'compliation' may be misspelled - perhaps 'compilation'? > #16: > module compliation added by Wesley Cheng to complete original concept code > ^^^^^^^^^^^ > CHECK: Prefer kzalloc(sizeof(*sgt)...) over kzalloc(sizeof(struct > sg_table)...) > #105: FILE: drivers/usb/host/xhci-sideband.c:35: > + sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL); > > CHECK: struct mutex definition without comment > #557: FILE: include/linux/usb/xhci-sideband.h:35: > + struct mutex mutex; > > WARNING: 'straightfoward' may be misspelled - perhaps 'straightforward'? > #22: > straightfoward, as the ASoC components have direct references to the ASoC > ^^^^^^^^^^^^^^ > CHECK: Unnecessary parentheses around 'card == sdev->card_idx' > #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217: > + if ((card == sdev->card_idx) && > + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) { > > CHECK: Unnecessary parentheses around 'pcm == > sdev->ppcm_idx[sdev->num_playback - 1]' > #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217: > + if ((card == sdev->card_idx) && > + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) { > > WARNING: 'seqeunces' may be misspelled - perhaps 'sequences'? > #8: > seqeunces. This allows for platform USB SND modules to properly initialize > ^^^^^^^^^ > > WARNING: 'exisiting' may be misspelled - perhaps 'existing'? > #12: > exisiting parameters. > ^^^^^^^^^ > > CHECK: Please use a blank line after function/struct/union/enum declarations > #1020: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:98: > +}; > +#define QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN 46 > > CHECK: Please use a blank line after function/struct/union/enum declarations > #1054: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:132: > +}; > +#define QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN 202 > > CHECK: Please use a blank line after function/struct/union/enum declarations > #1081: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:159: > +}; > +#define QMI_UAUDIO_STREAM_IND_MSG_V01_MAX_MSG_LEN 181 > > CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues > #100: FILE: sound/usb/mixer_usb_offload.c:19: > +#define PCM_IDX(n) (n & 0xffff) > > CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues > #101: FILE: sound/usb/mixer_usb_offload.c:20: > +#define CARD_IDX(n) (n >> 16) >