diff mbox series

[RFC,1/1] usb: host: xhci-plat: add enable XHCI-AVOID-BEI quirk by dts

Message ID 20240601120640.73556-1-joswang1221@gmail.com
State New
Headers show
Series [RFC,1/1] usb: host: xhci-plat: add enable XHCI-AVOID-BEI quirk by dts | expand

Commit Message

joswang June 1, 2024, 12:06 p.m. UTC 
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.

Signed-off-by: joswang <joswang@lenovo.com>
---
 drivers/usb/host/xhci-plat.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Mathias Nyman June 11, 2024, 10:03 a.m. UTC | #1 
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
joswang June 11, 2024, 2:45 p.m. UTC | #2 
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
Mathias Nyman June 11, 2024, 2:58 p.m. UTC | #3 
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
joswang June 12, 2024, 2:23 p.m. UTC | #4 
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 mbox series

Patch

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;