| Message ID | 20240601120640.73556-1-joswang1221@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/1] usb: host: xhci-plat: add enable XHCI-AVOID-BEI quirk by dts | expand |
On 1.6.2024 15.06, joswang wrote: > From: joswang <joswang@lenovo.com> > > For Synopsys DWC31 2.00a and earlier versions, every isochronous > interval the BEI(Block Event Interrupt) flag is set for all except > the last Isoch TRB in a URB, host controller consumes the event > TRBs in the event ring, once the event ring is full, it will not > generate an interrupt and will stop all data transmission and command > execution. > > To avoid the problem of event ring full, the XHCI-AVOID-BEI quirk is > introduced. Currently, the XHCI-AVOID-BEI quirk has been applied to all > Intel xHCI controllers, see commit '227a4fd801c8 ("USB: xhci: apply > XHCI-AVOID-BEI quirk to all Intel xHCI controllers")'. > > For Linux system, each event ring consists of one or more event ring > segments and each segment is 4 KB that contains 256 TRBs. It seems that > the TRBs on the event ring are sufficient and the event ring will not be > full. In real application, if it does happen, event ring is full, host > controller no interrupt causes the driver to timeout. > > However, applying XHCI-AVOID-BEI quirk will also bring power consumption > issues. We can consider the application scenarios of the product to decide > whether to enable it. Therefore, we add the enable XHCI-AVOID-BEI quirk > through dts configuration to make it more flexible. Took a look at XHCI_AVOID_BEI quirk and it seems that it evolved from solving a hardware issue into a interrupt trigger optimization. How about making current XHCI_AVOID_BEI the default behavior, i.e. force an interrupt every 32nd isoc trb, and reduce it in case event ring has more than half a segments of events per interrupt. The actual XHCI_AVOID_BEI quirk would only be used for hardware that that can't handle BEI flag properly something like: diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 266fcbc4bb93..dd161ebf15a3 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -3991,16 +3991,17 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci, static bool trb_block_event_intr(struct xhci_hcd *xhci, int num_tds, int i, struct xhci_interrupter *ir) { - if (xhci->hci_version < 0x100) + if (xhci->hci_version < 0x100 || xhci->quirks & XHCI_AVOID_BEI) return false; + /* always generate an event interrupt for the last TD */ if (i == num_tds - 1) return false; /* - * If AVOID_BEI is set the host handles full event rings poorly, - * generate an event at least every 8th TD to clear the event ring + * host handles full event rings poorly, force an interrupt at least + * every 32 isoc TRB to clear the event ring. */ - if (i && ir->isoc_bei_interval && xhci->quirks & XHCI_AVOID_BEI) + if (i && ir->isoc_bei_interval) Thanks Mathias
On Mon, Jun 3, 2024 at 8:21 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 1.6.2024 15.06, joswang wrote: > > From: joswang <joswang@lenovo.com> > > > > For Synopsys DWC31 2.00a and earlier versions, every isochronous > > interval the BEI(Block Event Interrupt) flag is set for all except > > the last Isoch TRB in a URB, host controller consumes the event > > TRBs in the event ring, once the event ring is full, it will not > > generate an interrupt and will stop all data transmission and command > > execution. > > > > To avoid the problem of event ring full, the XHCI-AVOID-BEI quirk is > > introduced. Currently, the XHCI-AVOID-BEI quirk has been applied to all > > Intel xHCI controllers, see commit '227a4fd801c8 ("USB: xhci: apply > > XHCI-AVOID-BEI quirk to all Intel xHCI controllers")'. > > > > For Linux system, each event ring consists of one or more event ring > > segments and each segment is 4 KB that contains 256 TRBs. It seems that > > the TRBs on the event ring are sufficient and the event ring will not be > > full. In real application, if it does happen, event ring is full, host > > controller no interrupt causes the driver to timeout. > > > > However, applying XHCI-AVOID-BEI quirk will also bring power consumption > > issues. We can consider the application scenarios of the product to decide > > whether to enable it. Therefore, we add the enable XHCI-AVOID-BEI quirk > > through dts configuration to make it more flexible. > > Took a look at XHCI_AVOID_BEI quirk and it seems that it evolved from > solving a hardware issue into a interrupt trigger optimization. Thanks for reviewing the code. Yes, you optimized the interrupt triggering frequency. > > How about making current XHCI_AVOID_BEI the default behavior, i.e. force > an interrupt every 32nd isoc trb, and reduce it in case event ring > has more than half a segments of events per interrupt. Yes，enabling XHCI_AVOID_BEI quirk is to solve the problem of event ring fullness > > The actual XHCI_AVOID_BEI quirk would only be used for hardware that that > can't handle BEI flag properly > > something like: > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index 266fcbc4bb93..dd161ebf15a3 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -3991,16 +3991,17 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci, > static bool trb_block_event_intr(struct xhci_hcd *xhci, int num_tds, int i, > struct xhci_interrupter *ir) > { > - if (xhci->hci_version < 0x100) > + if (xhci->hci_version < 0x100 || xhci->quirks & XHCI_AVOID_BEI) > return false; > + > /* always generate an event interrupt for the last TD */ > if (i == num_tds - 1) > return false; > /* > - * If AVOID_BEI is set the host handles full event rings poorly, > - * generate an event at least every 8th TD to clear the event ring > + * host handles full event rings poorly, force an interrupt at least > + * every 32 isoc TRB to clear the event ring. > */ > - if (i && ir->isoc_bei_interval && xhci->quirks & XHCI_AVOID_BEI) > + if (i && ir->isoc_bei_interval) > For Synopsys DWC31 2.00a IP and earlier versions, the corresponding driver is in drivers/usb/dwc3/core.c. If XHCI_AVOID_BEI quirk is not enabled, in other words, an interrupt is triggered every 32nd isoc trb, then the event ring may be full. After the event ring is full, the controller cannot trigger an interrupt, causing the driver to timeout. My initial solution: diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index a171b27a7845..1e33e58c7281 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) int dwc3_host_init(struct dwc3 *dwc) { - struct property_entry props[5]; + struct property_entry props[6]; struct platform_device *xhci; int ret, irq; int prop_idx = 0; @@ -180,6 +180,9 @@ int dwc3_host_init(struct dwc3 *dwc) if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A)) props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-broken-port-ped"); + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 200A)) + props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-avoid-bei"); + if (prop_idx) { ret = device_create_managed_software_node(&xhci->dev, props, NULL); if (ret) { diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 3d071b875308..e1071827d4b3 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s if (device_property_read_bool(tmpdev, "quirk-broken-port-ped")) xhci->quirks |= XHCI_BROKEN_PORT_PED; + if (device_property_read_bool(tmpdev, "quirk-avoid-bei")) + xhci->quirks |= XHCI_AVOID_BEI; + if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk")) xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK; I consider that enabling XHCI_AVOID_BEI quirk will increase the number of isoc transmission interrupts, and some specific applications of products may not have full event rings. For Synopsys DWC31 2.00a IP and earlier versions, XHCI_AVOID_BEI quirk is forced to be enabled, which is not the best solution. Therefore, the second solution is generated, which is to remove the modification of drivers/usb/dwc3/host.c file, only keep drivers/usb/host/xhci-plat.c, enable XHCI_AVOID_BEI quirk by adding dts configuration. Let users decide whether to enable XHCI_AVOID_BEI quirk based on the specific application of the product. diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 3d071b875308..e1071827d4b3 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s if (device_property_read_bool(tmpdev, "quirk-broken-port-ped")) xhci->quirks |= XHCI_BROKEN_PORT_PED; + if (device_property_read_bool(tmpdev, "quirk-avoid-bei")) + xhci->quirks |= XHCI_AVOID_BEI; + if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk")) xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK; Please help evaluate, thank you. > > Thanks > Mathias >
On 5.6.2024 8.37, joswang wrote: > On Mon, Jun 3, 2024 at 8:21 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> >> On 1.6.2024 15.06, joswang wrote: >>> From: joswang <joswang@lenovo.com> >>> >>> For Synopsys DWC31 2.00a and earlier versions, every isochronous >>> interval the BEI(Block Event Interrupt) flag is set for all except >>> the last Isoch TRB in a URB, host controller consumes the event >>> TRBs in the event ring, once the event ring is full, it will not >>> generate an interrupt and will stop all data transmission and command >>> execution. >>> >>> To avoid the problem of event ring full, the XHCI-AVOID-BEI quirk is >>> introduced. Currently, the XHCI-AVOID-BEI quirk has been applied to all >>> Intel xHCI controllers, see commit '227a4fd801c8 ("USB: xhci: apply >>> XHCI-AVOID-BEI quirk to all Intel xHCI controllers")'. >>> >>> For Linux system, each event ring consists of one or more event ring >>> segments and each segment is 4 KB that contains 256 TRBs. It seems that >>> the TRBs on the event ring are sufficient and the event ring will not be >>> full. In real application, if it does happen, event ring is full, host >>> controller no interrupt causes the driver to timeout. >>> >>> However, applying XHCI-AVOID-BEI quirk will also bring power consumption >>> issues. We can consider the application scenarios of the product to decide >>> whether to enable it. Therefore, we add the enable XHCI-AVOID-BEI quirk >>> through dts configuration to make it more flexible. >> >> Took a look at XHCI_AVOID_BEI quirk and it seems that it evolved from >> solving a hardware issue into a interrupt trigger optimization. > Thanks for reviewing the code. > Yes, you optimized the interrupt triggering frequency. >> >> How about making current XHCI_AVOID_BEI the default behavior, i.e. force >> an interrupt every 32nd isoc trb, and reduce it in case event ring >> has more than half a segments of events per interrupt. > Yes，enabling XHCI_AVOID_BEI quirk is to solve the problem of event ring fullness >> >> The actual XHCI_AVOID_BEI quirk would only be used for hardware that that >> can't handle BEI flag properly >> >> something like: >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index 266fcbc4bb93..dd161ebf15a3 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -3991,16 +3991,17 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci, >> static bool trb_block_event_intr(struct xhci_hcd *xhci, int num_tds, int i, >> struct xhci_interrupter *ir) >> { >> - if (xhci->hci_version < 0x100) >> + if (xhci->hci_version < 0x100 || xhci->quirks & XHCI_AVOID_BEI) >> return false; >> + >> /* always generate an event interrupt for the last TD */ >> if (i == num_tds - 1) >> return false; >> /* >> - * If AVOID_BEI is set the host handles full event rings poorly, >> - * generate an event at least every 8th TD to clear the event ring >> + * host handles full event rings poorly, force an interrupt at least >> + * every 32 isoc TRB to clear the event ring. >> */ >> - if (i && ir->isoc_bei_interval && xhci->quirks & XHCI_AVOID_BEI) >> + if (i && ir->isoc_bei_interval) >> > For Synopsys DWC31 2.00a IP and earlier versions, the corresponding > driver is in drivers/usb/dwc3/core.c. > If XHCI_AVOID_BEI quirk is not enabled, in other words, an interrupt > is triggered every 32nd isoc trb, then > the event ring may be full. After the event ring is full, the > controller cannot trigger an interrupt, causing the driver > to timeout. I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no quirk flag would be needed: Currently without the quirk flag: - ISOC TRBs trigger interrupt if TRB is the last in the TD Currently with XHCI_AVOID_BEI quirk flag: - ISOC TRBs trigger interrupt if TRB is the last in the TD - Interrupt is additionally triggered every 32 isoc TRB (initially). - if more than 128 events are processed in one interrupt then the 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so on, 16 -> 8... I would remove the quirk flag, and make all controllers interrupt behave as if it was set. i.e. interrupt at least every 32 isoc TRB > My initial solution: > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index a171b27a7845..1e33e58c7281 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > > int dwc3_host_init(struct dwc3 *dwc) > { > - struct property_entry props[5]; > + struct property_entry props[6]; > struct platform_device *xhci; > int ret, irq; > int prop_idx = 0; > @@ -180,6 +180,9 @@ int dwc3_host_init(struct dwc3 *dwc) > if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A)) > props[prop_idx++] = > PROPERTY_ENTRY_BOOL("quirk-broken-port-ped"); > > + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 200A)) > + props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-avoid-bei"); > + > if (prop_idx) { > ret = device_create_managed_software_node(&xhci->dev, > props, NULL); > if (ret) { > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index 3d071b875308..e1071827d4b3 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev, > struct device *sysdev, const s > if (device_property_read_bool(tmpdev, "quirk-broken-port-ped")) > xhci->quirks |= XHCI_BROKEN_PORT_PED; > > + if (device_property_read_bool(tmpdev, "quirk-avoid-bei")) > + xhci->quirks |= XHCI_AVOID_BEI; > + > if (device_property_read_bool(tmpdev, > "xhci-sg-trb-cache-size-quirk")) > xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK; > > I consider that enabling XHCI_AVOID_BEI quirk will increase the number > of isoc transmission > interrupts, and some specific applications of products may not have > full event rings. > For Synopsys DWC31 2.00a IP and earlier versions, XHCI_AVOID_BEI quirk > is forced to be enabled, > which is not the best solution. Therefore, the second solution is > generated, which is to remove the > modification of drivers/usb/dwc3/host.c file, only keep > drivers/usb/host/xhci-plat.c, enable XHCI_AVOID_BEI > quirk by adding dts configuration. Let users decide whether to enable > XHCI_AVOID_BEI quirk based on > the specific application of the product. Is there an actual real world case where interrupting every 32nd ISOC TRB is too often? UVC driver for example has defined UVC_MAX_PACKETS 32, meaning a common isoc usb camera has max 32 TRBs per TD, and naturally interrupt at least every 32 isoc TRB To me the problem seems to be the other way around, we see cases like this where we are not interrupting often enough, and event ring fills up. Thanks Mathias
On Wed, Jun 5, 2024 at 6:31 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 5.6.2024 8.37, joswang wrote: > > On Mon, Jun 3, 2024 at 8:21 PM Mathias Nyman > > <mathias.nyman@linux.intel.com> wrote: > >> > >> On 1.6.2024 15.06, joswang wrote: > >>> From: joswang <joswang@lenovo.com> > >>> > >>> For Synopsys DWC31 2.00a and earlier versions, every isochronous > >>> interval the BEI(Block Event Interrupt) flag is set for all except > >>> the last Isoch TRB in a URB, host controller consumes the event > >>> TRBs in the event ring, once the event ring is full, it will not > >>> generate an interrupt and will stop all data transmission and command > >>> execution. > >>> > >>> To avoid the problem of event ring full, the XHCI-AVOID-BEI quirk is > >>> introduced. Currently, the XHCI-AVOID-BEI quirk has been applied to all > >>> Intel xHCI controllers, see commit '227a4fd801c8 ("USB: xhci: apply > >>> XHCI-AVOID-BEI quirk to all Intel xHCI controllers")'. > >>> > >>> For Linux system, each event ring consists of one or more event ring > >>> segments and each segment is 4 KB that contains 256 TRBs. It seems that > >>> the TRBs on the event ring are sufficient and the event ring will not be > >>> full. In real application, if it does happen, event ring is full, host > >>> controller no interrupt causes the driver to timeout. > >>> > >>> However, applying XHCI-AVOID-BEI quirk will also bring power consumption > >>> issues. We can consider the application scenarios of the product to decide > >>> whether to enable it. Therefore, we add the enable XHCI-AVOID-BEI quirk > >>> through dts configuration to make it more flexible. > >> > >> Took a look at XHCI_AVOID_BEI quirk and it seems that it evolved from > >> solving a hardware issue into a interrupt trigger optimization. > > Thanks for reviewing the code. > > Yes, you optimized the interrupt triggering frequency. > >> > >> How about making current XHCI_AVOID_BEI the default behavior, i.e. force > >> an interrupt every 32nd isoc trb, and reduce it in case event ring > >> has more than half a segments of events per interrupt. > > Yes，enabling XHCI_AVOID_BEI quirk is to solve the problem of event ring fullness > >> > >> The actual XHCI_AVOID_BEI quirk would only be used for hardware that that > >> can't handle BEI flag properly > >> > >> something like: > >> > >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > >> index 266fcbc4bb93..dd161ebf15a3 100644 > >> --- a/drivers/usb/host/xhci-ring.c > >> +++ b/drivers/usb/host/xhci-ring.c > >> @@ -3991,16 +3991,17 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd *xhci, > >> static bool trb_block_event_intr(struct xhci_hcd *xhci, int num_tds, int i, > >> struct xhci_interrupter *ir) > >> { > >> - if (xhci->hci_version < 0x100) > >> + if (xhci->hci_version < 0x100 || xhci->quirks & XHCI_AVOID_BEI) > >> return false; > >> + > >> /* always generate an event interrupt for the last TD */ > >> if (i == num_tds - 1) > >> return false; > >> /* > >> - * If AVOID_BEI is set the host handles full event rings poorly, > >> - * generate an event at least every 8th TD to clear the event ring > >> + * host handles full event rings poorly, force an interrupt at least > >> + * every 32 isoc TRB to clear the event ring. > >> */ > >> - if (i && ir->isoc_bei_interval && xhci->quirks & XHCI_AVOID_BEI) > >> + if (i && ir->isoc_bei_interval) > >> > > For Synopsys DWC31 2.00a IP and earlier versions, the corresponding > > driver is in drivers/usb/dwc3/core.c. > > If XHCI_AVOID_BEI quirk is not enabled, in other words, an interrupt > > is triggered every 32nd isoc trb, then > > the event ring may be full. After the event ring is full, the > > controller cannot trigger an interrupt, causing the driver > > to timeout. > > I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no > quirk flag would be needed: > > Currently without the quirk flag: > > - ISOC TRBs trigger interrupt if TRB is the last in the TD > > Currently with XHCI_AVOID_BEI quirk flag: > > - ISOC TRBs trigger interrupt if TRB is the last in the TD > - Interrupt is additionally triggered every 32 isoc TRB (initially). > - if more than 128 events are processed in one interrupt then the > 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so > on, 16 -> 8... > > I would remove the quirk flag, and make all controllers interrupt > behave as if it was set. i.e. interrupt at least every 32 isoc TRB Thank you for your detailed analysis. Excuse me, I have a question, do you mean to set "Currently with XHCI_AVOID_BEI quirk flag" as the default behavior? > > > My initial solution: > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > index a171b27a7845..1e33e58c7281 100644 > > --- a/drivers/usb/dwc3/host.c > > +++ b/drivers/usb/dwc3/host.c > > @@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > > > > int dwc3_host_init(struct dwc3 *dwc) > > { > > - struct property_entry props[5]; > > + struct property_entry props[6]; > > struct platform_device *xhci; > > int ret, irq; > > int prop_idx = 0; > > @@ -180,6 +180,9 @@ int dwc3_host_init(struct dwc3 *dwc) > > if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A)) > > props[prop_idx++] = > > PROPERTY_ENTRY_BOOL("quirk-broken-port-ped"); > > > > + if (DWC3_VER_IS_WITHIN(DWC31, ANY, 200A)) > > + props[prop_idx++] = PROPERTY_ENTRY_BOOL("quirk-avoid-bei"); > > + > > if (prop_idx) { > > ret = device_create_managed_software_node(&xhci->dev, > > props, NULL); > > if (ret) { > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > index 3d071b875308..e1071827d4b3 100644 > > --- a/drivers/usb/host/xhci-plat.c > > +++ b/drivers/usb/host/xhci-plat.c > > @@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev, > > struct device *sysdev, const s > > if (device_property_read_bool(tmpdev, "quirk-broken-port-ped")) > > xhci->quirks |= XHCI_BROKEN_PORT_PED; > > > > + if (device_property_read_bool(tmpdev, "quirk-avoid-bei")) > > + xhci->quirks |= XHCI_AVOID_BEI; > > + > > if (device_property_read_bool(tmpdev, > > "xhci-sg-trb-cache-size-quirk")) > > xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK; > > > > I consider that enabling XHCI_AVOID_BEI quirk will increase the number > > of isoc transmission > > interrupts, and some specific applications of products may not have > > full event rings. > > For Synopsys DWC31 2.00a IP and earlier versions, XHCI_AVOID_BEI quirk > > is forced to be enabled, > > which is not the best solution. Therefore, the second solution is > > generated, which is to remove the > > modification of drivers/usb/dwc3/host.c file, only keep > > drivers/usb/host/xhci-plat.c, enable XHCI_AVOID_BEI > > quirk by adding dts configuration. Let users decide whether to enable > > XHCI_AVOID_BEI quirk based on > > the specific application of the product. > > Is there an actual real world case where interrupting every 32nd ISOC TRB is > too often? I mean that if the XHCI_AVOID_BEI quirk flag is set, an interrupt will be triggered every 8 TRBs, which makes the interrupts seem to be quite frequent. Thanks Jos > > UVC driver for example has defined UVC_MAX_PACKETS 32, meaning a common isoc > usb camera has max 32 TRBs per TD, and naturally interrupt at least every 32 > isoc TRB > > To me the problem seems to be the other way around, we see cases like this where > we are not interrupting often enough, and event ring fills up. > > Thanks > Mathias >
On 6.6.2024 17.08, joswang wrote: > On Wed, Jun 5, 2024 at 6:31 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: ... >> >> I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no >> quirk flag would be needed: >> >> Currently without the quirk flag: >> >> - ISOC TRBs trigger interrupt if TRB is the last in the TD >> >> Currently with XHCI_AVOID_BEI quirk flag: >> >> - ISOC TRBs trigger interrupt if TRB is the last in the TD >> - Interrupt is additionally triggered every 32 isoc TRB (initially). >> - if more than 128 events are processed in one interrupt then the >> 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so >> on, 16 -> 8... >> >> I would remove the quirk flag, and make all controllers interrupt >> behave as if it was set. i.e. interrupt at least every 32 isoc TRB > > Thank you for your detailed analysis. > Excuse me, I have a question, do you mean to set "Currently with > XHCI_AVOID_BEI quirk flag" as the default behavior? Yes, unless it causes some issues or there are strong objections >> >> Is there an actual real world case where interrupting every 32nd ISOC TRB is >> too often? > > I mean that if the XHCI_AVOID_BEI quirk flag is set, an interrupt will > be triggered every 8 TRBs, which makes the interrupts seem to be quite > frequent. > Thanks > Jos > It should start with interrupting every 32nd isoc TD, not 8th #define AVOID_BEI_INTERVAL_MAX 32 ir->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX; Thanks Mathias
On Tue, Jun 11, 2024 at 6:01 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 6.6.2024 17.08, joswang wrote: > > On Wed, Jun 5, 2024 at 6:31 PM Mathias Nyman > > <mathias.nyman@linux.intel.com> wrote: > ... > >> > >> I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no > >> quirk flag would be needed: > >> > >> Currently without the quirk flag: > >> > >> - ISOC TRBs trigger interrupt if TRB is the last in the TD > >> > >> Currently with XHCI_AVOID_BEI quirk flag: > >> > >> - ISOC TRBs trigger interrupt if TRB is the last in the TD > >> - Interrupt is additionally triggered every 32 isoc TRB (initially). > >> - if more than 128 events are processed in one interrupt then the > >> 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so > >> on, 16 -> 8... > >> > >> I would remove the quirk flag, and make all controllers interrupt > >> behave as if it was set. i.e. interrupt at least every 32 isoc TRB > > > > Thank you for your detailed analysis. > > Excuse me, I have a question, do you mean to set "Currently with > > XHCI_AVOID_BEI quirk flag" as the default behavior? > > Yes, unless it causes some issues or there are strong objections > > >> > >> Is there an actual real world case where interrupting every 32nd ISOC TRB is > >> too often? > > > > I mean that if the XHCI_AVOID_BEI quirk flag is set, an interrupt will > > be triggered every 8 TRBs, which makes the interrupts seem to be quite > > frequent. > > Thanks > > Jos > > > > It should start with interrupting every 32nd isoc TD, not 8th > > #define AVOID_BEI_INTERVAL_MAX 32 > > ir->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX; > > Thanks > Mathias As you mentioned，if more than 128 events are processed in one interrupt then the 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so on, 16 -> 8... xhci_handle_events() ...... if (event_loop++ > TRBS_PER_SEGMENT / 2) { if (ir->isoc_bei_interval > AVOID_BEI_INTERVAL_MIN) ir->isoc_bei_interval = ir->isoc_bei_interval / 2; } ....... Thanks Jos Wang
On 11.6.2024 17.45, joswang wrote: > On Tue, Jun 11, 2024 at 6:01 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> >> On 6.6.2024 17.08, joswang wrote: >>> On Wed, Jun 5, 2024 at 6:31 PM Mathias Nyman >>> <mathias.nyman@linux.intel.com> wrote: >> ... >>>> >>>> I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no >>>> quirk flag would be needed: >>>> >>>> Currently without the quirk flag: >>>> >>>> - ISOC TRBs trigger interrupt if TRB is the last in the TD >>>> >>>> Currently with XHCI_AVOID_BEI quirk flag: >>>> >>>> - ISOC TRBs trigger interrupt if TRB is the last in the TD >>>> - Interrupt is additionally triggered every 32 isoc TRB (initially). >>>> - if more than 128 events are processed in one interrupt then the >>>> 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so >>>> on, 16 -> 8... >>>> >>>> I would remove the quirk flag, and make all controllers interrupt >>>> behave as if it was set. i.e. interrupt at least every 32 isoc TRB >>> >>> Thank you for your detailed analysis. >>> Excuse me, I have a question, do you mean to set "Currently with >>> XHCI_AVOID_BEI quirk flag" as the default behavior? >> >> Yes, unless it causes some issues or there are strong objections >> >>>> >>>> Is there an actual real world case where interrupting every 32nd ISOC TRB is >>>> too often? >>> >>> I mean that if the XHCI_AVOID_BEI quirk flag is set, an interrupt will >>> be triggered every 8 TRBs, which makes the interrupts seem to be quite >>> frequent. >>> Thanks >>> Jos >>> >> >> It should start with interrupting every 32nd isoc TD, not 8th >> >> #define AVOID_BEI_INTERVAL_MAX 32 >> >> ir->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX; >> >> Thanks >> Mathias > > As you mentioned，if more than 128 events are processed in one > interrupt then the 32 is halved, and we trigger an interrupts every > 16th isoc TRB, and so on, 16 -> 8... > xhci_handle_events() > ...... > if (event_loop++ > TRBS_PER_SEGMENT / 2) { > if (ir->isoc_bei_interval > AVOID_BEI_INTERVAL_MIN) > ir->isoc_bei_interval = ir->isoc_bei_interval / 2; > } > ....... Yes, and to me this seems reasonable . If there are over 128 events per interrupt then we should interrupt more often. Would this work as the default for the Synopsys hosts you are working with? Thanks Mathias
On Tue, Jun 11, 2024 at 11:00 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 11.6.2024 17.45, joswang wrote: > > On Tue, Jun 11, 2024 at 6:01 PM Mathias Nyman > > <mathias.nyman@linux.intel.com> wrote: > >> > >> On 6.6.2024 17.08, joswang wrote: > >>> On Wed, Jun 5, 2024 at 6:31 PM Mathias Nyman > >>> <mathias.nyman@linux.intel.com> wrote: > >> ... > >>>> > >>>> I was thinking of turning XHCI_AVOID_BEI behavior into the new default, so no > >>>> quirk flag would be needed: > >>>> > >>>> Currently without the quirk flag: > >>>> > >>>> - ISOC TRBs trigger interrupt if TRB is the last in the TD > >>>> > >>>> Currently with XHCI_AVOID_BEI quirk flag: > >>>> > >>>> - ISOC TRBs trigger interrupt if TRB is the last in the TD > >>>> - Interrupt is additionally triggered every 32 isoc TRB (initially). > >>>> - if more than 128 events are processed in one interrupt then the > >>>> 32 is halved, and we trigger an interrupts every 16th isoc TRB, and so > >>>> on, 16 -> 8... > >>>> > >>>> I would remove the quirk flag, and make all controllers interrupt > >>>> behave as if it was set. i.e. interrupt at least every 32 isoc TRB > >>> > >>> Thank you for your detailed analysis. > >>> Excuse me, I have a question, do you mean to set "Currently with > >>> XHCI_AVOID_BEI quirk flag" as the default behavior? > >> > >> Yes, unless it causes some issues or there are strong objections > >> > >>>> > >>>> Is there an actual real world case where interrupting every 32nd ISOC TRB is > >>>> too often? > >>> > >>> I mean that if the XHCI_AVOID_BEI quirk flag is set, an interrupt will > >>> be triggered every 8 TRBs, which makes the interrupts seem to be quite > >>> frequent. > >>> Thanks > >>> Jos > >>> > >> > >> It should start with interrupting every 32nd isoc TD, not 8th > >> > >> #define AVOID_BEI_INTERVAL_MAX 32 > >> > >> ir->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX; > >> > >> Thanks > >> Mathias > > > > As you mentioned，if more than 128 events are processed in one > > interrupt then the 32 is halved, and we trigger an interrupts every > > 16th isoc TRB, and so on, 16 -> 8... > > xhci_handle_events() > > ...... > > if (event_loop++ > TRBS_PER_SEGMENT / 2) { > > if (ir->isoc_bei_interval > AVOID_BEI_INTERVAL_MIN) > > ir->isoc_bei_interval = ir->isoc_bei_interval / 2; > > } > > ....... > > Yes, and to me this seems reasonable . > If there are over 128 events per interrupt then we should interrupt more often. > > Would this work as the default for the Synopsys hosts you are working with? > > Thanks > Mathias Without enabling XHCI_AVOID_BEI quirk, that is, triggering an interrupt once for every 32TRB: For some USB cameras (such as high-resolution cameras, etc.), the data volume of one frame is relatively large, and the host controller will frequently trigger interrupts during data transmission. For some SOC platforms (such as rockchip platforms, which also use Synopsys USB IP) with poor performance, it is possible that the CPU cannot respond to interrupts in time, resulting in frame loss. Therefore, I think that using XHCI_AVOID_BEI quirk by default may aggravate the above problem. Please help evaluate, thank you. Thanks, Jos Wang
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 3d071b875308..e1071827d4b3 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s if (device_property_read_bool(tmpdev, "quirk-broken-port-ped")) xhci->quirks |= XHCI_BROKEN_PORT_PED; + if (device_property_read_bool(tmpdev, "quirk-avoid-bei")) + xhci->quirks |= XHCI_AVOID_BEI; + if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk")) xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;