| Message ID | 20241015212915.1206789-2-quic_wcheng@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce QC USB SND audio offloading support | expand |
On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: > From: Mathias Nyman <mathias.nyman@linux.intel.com> > > Allow creators of xHCI secondary interrupters to specify the interrupt > moderation interval value in nanoseconds when creating the interrupter. > > If not sure what value to use then use the xhci driver default > xhci->imod_interval > > Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/usb/host/xhci-mem.c | 8 +++++++- > drivers/usb/host/xhci.c | 4 ++-- > drivers/usb/host/xhci.h | 5 ++++- > 3 files changed, 13 insertions(+), 4 deletions(-) This is already in 6.12-rc1, which makes me confused as to what tree you made this series against. thanks, greg k-h
Hi Greg, On 10/16/2024 11:40 PM, Greg KH wrote: > On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: >> From: Mathias Nyman <mathias.nyman@linux.intel.com> >> >> Allow creators of xHCI secondary interrupters to specify the interrupt >> moderation interval value in nanoseconds when creating the interrupter. >> >> If not sure what value to use then use the xhci driver default >> xhci->imod_interval >> >> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> --- >> drivers/usb/host/xhci-mem.c | 8 +++++++- >> drivers/usb/host/xhci.c | 4 ++-- >> drivers/usb/host/xhci.h | 5 ++++- >> 3 files changed, 13 insertions(+), 4 deletions(-) > This is already in 6.12-rc1, which makes me confused as to what tree you > made this series against. Sorry, I didn't fetch the latest changes from usb-next. In this case, should I rebase and resbumit? Thanks Wesley Cheng > thanks, > > greg k-h
On Thu, Oct 17, 2024 at 05:07:12PM -0700, Wesley Cheng wrote: > Hi Greg, > > On 10/16/2024 11:40 PM, Greg KH wrote: > > On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: > >> From: Mathias Nyman <mathias.nyman@linux.intel.com> > >> > >> Allow creators of xHCI secondary interrupters to specify the interrupt > >> moderation interval value in nanoseconds when creating the interrupter. > >> > >> If not sure what value to use then use the xhci driver default > >> xhci->imod_interval > >> > >> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> > >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > >> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > >> --- > >> drivers/usb/host/xhci-mem.c | 8 +++++++- > >> drivers/usb/host/xhci.c | 4 ++-- > >> drivers/usb/host/xhci.h | 5 ++++- > >> 3 files changed, 13 insertions(+), 4 deletions(-) > > This is already in 6.12-rc1, which makes me confused as to what tree you > > made this series against. > > Sorry, I didn't fetch the latest changes from usb-next. It wasn't even usb-next, it was 6.12-rc1, so I don't know what tree you based this on :( > In this case, should I rebase and resbumit? As the series can't be applied as-is, probably. But I think you might want to collect some acks from the sound people and xhci developers, as I can't do anything with this until they look at the changes. thanks, greg k-h
On Fri, 18 Oct 2024 07:52:35 +0200, Greg KH wrote: > > On Thu, Oct 17, 2024 at 05:07:12PM -0700, Wesley Cheng wrote: > > Hi Greg, > > > > On 10/16/2024 11:40 PM, Greg KH wrote: > > > On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: > > >> From: Mathias Nyman <mathias.nyman@linux.intel.com> > > >> > > >> Allow creators of xHCI secondary interrupters to specify the interrupt > > >> moderation interval value in nanoseconds when creating the interrupter. > > >> > > >> If not sure what value to use then use the xhci driver default > > >> xhci->imod_interval > > >> > > >> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> > > >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > >> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com > > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > >> --- > > >> drivers/usb/host/xhci-mem.c | 8 +++++++- > > >> drivers/usb/host/xhci.c | 4 ++-- > > >> drivers/usb/host/xhci.h | 5 ++++- > > >> 3 files changed, 13 insertions(+), 4 deletions(-) > > > This is already in 6.12-rc1, which makes me confused as to what tree you > > > made this series against. > > > > Sorry, I didn't fetch the latest changes from usb-next. > > It wasn't even usb-next, it was 6.12-rc1, so I don't know what tree you > based this on :( > > > In this case, should I rebase and resbumit? > > As the series can't be applied as-is, probably. But I think you might > want to collect some acks from the sound people and xhci developers, as > I can't do anything with this until they look at the changes. Honestly speaking, I couldn't follow fully the discussions about the fundamental design -- IIRC, Pierre and others had concerns to the way to manage the offload device via kcontrols. Did we get consensus? I believe that's the biggest obstacle in the audio side, i.e. what's visible to users. The kernel internals can be corrected at any time later. thanks, Takashi
On Tue, Oct 22, 2024 at 03:56:44PM +0200, Takashi Iwai wrote: > On Fri, 18 Oct 2024 07:52:35 +0200, > Greg KH wrote: > > > > On Thu, Oct 17, 2024 at 05:07:12PM -0700, Wesley Cheng wrote: > > > Hi Greg, > > > > > > On 10/16/2024 11:40 PM, Greg KH wrote: > > > > On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: > > > >> From: Mathias Nyman <mathias.nyman@linux.intel.com> > > > >> > > > >> Allow creators of xHCI secondary interrupters to specify the interrupt > > > >> moderation interval value in nanoseconds when creating the interrupter. > > > >> > > > >> If not sure what value to use then use the xhci driver default > > > >> xhci->imod_interval > > > >> > > > >> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> > > > >> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > > > >> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com > > > >> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > >> --- > > > >> drivers/usb/host/xhci-mem.c | 8 +++++++- > > > >> drivers/usb/host/xhci.c | 4 ++-- > > > >> drivers/usb/host/xhci.h | 5 ++++- > > > >> 3 files changed, 13 insertions(+), 4 deletions(-) > > > > This is already in 6.12-rc1, which makes me confused as to what tree you > > > > made this series against. > > > > > > Sorry, I didn't fetch the latest changes from usb-next. > > > > It wasn't even usb-next, it was 6.12-rc1, so I don't know what tree you > > based this on :( > > > > > In this case, should I rebase and resbumit? > > > > As the series can't be applied as-is, probably. But I think you might > > want to collect some acks from the sound people and xhci developers, as > > I can't do anything with this until they look at the changes. > > Honestly speaking, I couldn't follow fully the discussions about the > fundamental design -- IIRC, Pierre and others had concerns to the way > to manage the offload device via kcontrols. Did we get consensus? I don't think so. > I believe that's the biggest obstacle in the audio side, i.e. what's > visible to users. The kernel internals can be corrected at any time > later. I would like to see that agreed on before I even look at the usb side. thanks, greg k-h
On 10/22/2024 4:02 PM, Greg KH wrote: > On Tue, Oct 22, 2024 at 03:56:44PM +0200, Takashi Iwai wrote: >> On Fri, 18 Oct 2024 07:52:35 +0200, >> Greg KH wrote: >>> >>> On Thu, Oct 17, 2024 at 05:07:12PM -0700, Wesley Cheng wrote: >>>> Hi Greg, >>>> >>>> On 10/16/2024 11:40 PM, Greg KH wrote: >>>>> On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: >>>>>> From: Mathias Nyman <mathias.nyman@linux.intel.com> >>>>>> >>>>>> Allow creators of xHCI secondary interrupters to specify the interrupt >>>>>> moderation interval value in nanoseconds when creating the interrupter. >>>>>> >>>>>> If not sure what value to use then use the xhci driver default >>>>>> xhci->imod_interval >>>>>> >>>>>> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> >>>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >>>>>> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com >>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>>> --- >>>>>> drivers/usb/host/xhci-mem.c | 8 +++++++- >>>>>> drivers/usb/host/xhci.c | 4 ++-- >>>>>> drivers/usb/host/xhci.h | 5 ++++- >>>>>> 3 files changed, 13 insertions(+), 4 deletions(-) >>>>> This is already in 6.12-rc1, which makes me confused as to what tree you >>>>> made this series against. >>>> >>>> Sorry, I didn't fetch the latest changes from usb-next. >>> >>> It wasn't even usb-next, it was 6.12-rc1, so I don't know what tree you >>> based this on :( >>> >>>> In this case, should I rebase and resbumit? >>> >>> As the series can't be applied as-is, probably. But I think you might >>> want to collect some acks from the sound people and xhci developers, as >>> I can't do anything with this until they look at the changes. >> >> Honestly speaking, I couldn't follow fully the discussions about the >> fundamental design -- IIRC, Pierre and others had concerns to the way >> to manage the offload device via kcontrols. Did we get consensus? > > I don't think so. > >> I believe that's the biggest obstacle in the audio side, i.e. what's >> visible to users. The kernel internals can be corrected at any time >> later. > > I would like to see that agreed on before I even look at the usb side. My main concern is still that one USB audio device can be accessed via two different cards exposed in userspace. Usual USB one, and the one from device which does "offload". Suggested implementation achieves it by adding additional controls, which need to be set in specific way to achieve offload. Overall while I understand the mechanism, I'm not exactly convinced that it is the best way from end user point of view. "Implementation" part in Documentation added in patch 19 shows how it looks in userspace now. If you don't mind two sound cards being used to access same piece of HW, current implementation looks ok to me. See also: https://lore.kernel.org/linux-sound/75ffde3a-7fef-4c15-bfc8-87756e1c3f11@linux.intel.com/ where I described how I would prefer it to look.
On 10/22/2024 8:04 AM, Amadeusz Sławiński wrote: > On 10/22/2024 4:02 PM, Greg KH wrote: >> On Tue, Oct 22, 2024 at 03:56:44PM +0200, Takashi Iwai wrote: >>> On Fri, 18 Oct 2024 07:52:35 +0200, >>> Greg KH wrote: >>>> >>>> On Thu, Oct 17, 2024 at 05:07:12PM -0700, Wesley Cheng wrote: >>>>> Hi Greg, >>>>> >>>>> On 10/16/2024 11:40 PM, Greg KH wrote: >>>>>> On Tue, Oct 15, 2024 at 02:28:43PM -0700, Wesley Cheng wrote: >>>>>>> From: Mathias Nyman <mathias.nyman@linux.intel.com> >>>>>>> >>>>>>> Allow creators of xHCI secondary interrupters to specify the interrupt >>>>>>> moderation interval value in nanoseconds when creating the interrupter. >>>>>>> >>>>>>> If not sure what value to use then use the xhci driver default >>>>>>> xhci->imod_interval >>>>>>> >>>>>>> Suggested-by: Wesley Cheng <quic_wcheng@quicinc.com> >>>>>>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> >>>>>>> Link: https://lore.kernel.org/r/20240905143300.1959279-13-mathias.nyman@linux.intel.com >>>>>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>>>>> --- >>>>>>> drivers/usb/host/xhci-mem.c | 8 +++++++- >>>>>>> drivers/usb/host/xhci.c | 4 ++-- >>>>>>> drivers/usb/host/xhci.h | 5 ++++- >>>>>>> 3 files changed, 13 insertions(+), 4 deletions(-) >>>>>> This is already in 6.12-rc1, which makes me confused as to what tree you >>>>>> made this series against. >>>>> >>>>> Sorry, I didn't fetch the latest changes from usb-next. >>>> >>>> It wasn't even usb-next, it was 6.12-rc1, so I don't know what tree you >>>> based this on :( >>>> >>>>> In this case, should I rebase and resbumit? >>>> >>>> As the series can't be applied as-is, probably. But I think you might >>>> want to collect some acks from the sound people and xhci developers, as >>>> I can't do anything with this until they look at the changes. >>> >>> Honestly speaking, I couldn't follow fully the discussions about the >>> fundamental design -- IIRC, Pierre and others had concerns to the way >>> to manage the offload device via kcontrols. Did we get consensus? >> >> I don't think so. As mentioned by Amadeusz, the overall USB offload concept hasn't changed significantly since the initial series, and will rely on having two sounds cards, ie leaving the one created by USB SND untouched (and still usable), while creating a path to an ASoC based platform card, which handles the offload path. The follow ups that I've had with Pierre was more towards how the offload parameters are going to be exposed to userspace, so that it can be properly utilized. I think for the most part, we've agreed that the set of kcontrols we have now are sufficient, and there is proper controls for userspace to know which devices to use. >> >>> I believe that's the biggest obstacle in the audio side, i.e. what's >>> visible to users. The kernel internals can be corrected at any time >>> later. >> >> I would like to see that agreed on before I even look at the usb side. > > My main concern is still that one USB audio device can be accessed via two different cards exposed in userspace. Usual USB one, and the one from device which does "offload". Suggested implementation achieves it by adding additional controls, which need to be set in specific way to achieve offload. Overall while I understand the mechanism, I'm not exactly convinced that it is the best way from end user point of view. > > "Implementation" part in Documentation added in patch 19 shows how it looks in userspace now. > > If you don't mind two sound cards being used to access same piece of HW, current implementation looks ok to me. > @Takashi, this was something we discussed really early on, even before the series was made, and I think it was agreed upon to avoid doing this with a single card. I remember putting in the initial work to scope out this path, but it was going to require significant/major modifications to USB SND core, hence why we decided on the path to have two sound cards. (USB SND legacy path still usable) Thanks Wesley Cheng > See also: > https://lore.kernel.org/linux-sound/75ffde3a-7fef-4c15-bfc8-87756e1c3f11@linux.intel.com/ > where I described how I would prefer it to look.
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3100219d6496..a25576f27e66 100644 --- a/drivers/usb/host/xhci-mem.c +++ b/drivers/usb/host/xhci-mem.c @@ -2334,7 +2334,8 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir, } struct xhci_interrupter * -xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs) +xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, + u32 imod_interval) { struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct xhci_interrupter *ir; @@ -2367,6 +2368,11 @@ xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs) return NULL; } + err = xhci_set_interrupter_moderation(ir, imod_interval); + if (err) + xhci_warn(xhci, "Failed to set interrupter %d moderation to %uns\n", + i, imod_interval); + xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters %d\n", i, xhci->max_interrupters); diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 37eb37b0affa..ed1bb7ed44b0 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -347,8 +347,8 @@ static int xhci_disable_interrupter(struct xhci_interrupter *ir) } /* interrupt moderation interval imod_interval in nanoseconds */ -static int xhci_set_interrupter_moderation(struct xhci_interrupter *ir, - u32 imod_interval) +int xhci_set_interrupter_moderation(struct xhci_interrupter *ir, + u32 imod_interval) { u32 imod; diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h index 30415158ed3c..324644165d93 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1827,7 +1827,8 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci, void xhci_free_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx); struct xhci_interrupter * -xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs); +xhci_create_secondary_interrupter(struct usb_hcd *hcd, unsigned int segs, + u32 imod_interval); void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir); @@ -1867,6 +1868,8 @@ int xhci_alloc_tt_info(struct xhci_hcd *xhci, struct xhci_virt_device *virt_dev, struct usb_device *hdev, struct usb_tt *tt, gfp_t mem_flags); +int xhci_set_interrupter_moderation(struct xhci_interrupter *ir, + u32 imod_interval); /* xHCI ring, segment, TRB, and TD functions */ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);