| Message ID | 20210909083120.15350-1-jackp@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: gadget: Skip resizing EP's TX FIFO if already resized | expand |
Hi Jack, Jack Pham <jackp@codeaurora.org> writes: >> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> > index 804b50548163..c647c76d7361 100644 >> > --- a/drivers/usb/dwc3/gadget.c >> > +++ b/drivers/usb/dwc3/gadget.c >> > @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> > if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) >> > return 0; >> > >> > + /* bail if already resized */ >> > + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) >> > + return 0; >> > + >> >> heh, not to say "I told you so", but... >> >> That being said, your test is not very good. The whole idea for resizing >> the FIFOs is that in some applications we only use e.g. 2 endpoints and >> there is considerable FIFO space left unused. >> >> The goal is to use that unused FIFO space to squeeze more throughput out >> of the pipe, since it amortizes SW latency. >> >> This patch is essentially the same as reverting the original commit :-) > > No, it's not quite the same as nullifying the resizing algorithm. This > patch is predicated on a key part of the resizing algorithm where > dwc3_gadget_clear_tx_fifos() occurs upon receiving Set_Configuration in > ep0.c. Which means that each new connection starts off with a blank > slate with all the GTXFIFOSIZ(n) registers cleared. Then each EP gets > resized one at a time when usb_ep_enable() is called. > > The problem this patch is fixing is avoiding *re-resizing*, the idea > being that if an EP was already resized once during a session (from > Set Configuration until the next reset or disconnect), then > it should be good to go even if it gets disabled and re-enabled again. that's not a safe assumption, though. What happens in cases where Configuration 1 is wildly different from Configuration 2? Say Config 1 is a mass storage device and Config 2 is a collection of several CDC interfaces? > Since we lack any boolean state variable in struct dwc3_ep reflecting > whether it had already been resized, re-reading the GTXFIFOSIZ register it might be a better idea to introduce such a flag and make the intention clear. But in any case, I still think the assumption you're making is not very good. > is the next best equivalent. Note also that this check occurs after > the if (!dwc->do_fifo_resize) check so this is applicable only if the > entire "tx-fifo-resize" mechanism is enabled. Right, that's fine :-)
On 9/9/2021 6:15 PM, Thinh Nguyen wrote: > Jack Pham wrote: >> Some functions may dynamically enable and disable their endpoints >> regularly throughout their operation, particularly when Set Interface >> is employed to switch between Alternate Settings. For instance the >> UAC2 function has its respective endpoints for playback & capture >> associated with AltSetting 1, in which case those endpoints would not >> get enabled until the host activates the AltSetting. And they >> conversely become disabled when the interfaces' AltSetting 0 is >> chosen. >> >> With the DWC3 FIFO resizing algorithm recently added, every >> usb_ep_enable() call results in a call to resize that EP's TXFIFO, >> but if the same endpoint is enabled again and again, this incorrectly >> leads to FIFO RAM allocation exhaustion as the mechanism did not >> account for the possibility that endpoints can be re-enabled many >> times. >> >> Example log splat: >> >> dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 >> configfs-gadget gadget: u_audio_start_capture:521 Error! >> dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in >> >> This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos() >> if an endpoint is already resized, avoiding the calculation error >> resulting from accumulating the EP's FIFO depth repeatedly. >> >> Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements") >> Signed-off-by: Jack Pham <jackp@codeaurora.org> >> --- >> drivers/usb/dwc3/gadget.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 804b50548163..c647c76d7361 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) >> return 0; >> >> + /* bail if already resized */ >> + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) >> + return 0; >> + >> ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >> >> if ((dep->endpoint.maxburst > 1 && >> Hi Thinh, > > This seems like a workaround more than a fix. As previously pointed out, > there will be problems when there are multiple alternate setting > interface [2]. If we're doing this way, are we properly allocating the > fifo size for the alternate setting that requires the most fifo size and > not just the first alt-setting 0? Also different alt-setting can have Each alt interface will call usb_ep_autoconfig() which should be assigned different endpoint numbers. This would mean that if alt intf#0 gets selected, and EP is enabled, then we will resize the TXFIFO and map that FIFO to the physical EP. Then when/if the host requests the other alt intf#1, and that calls EP enable, then the logic will then attempt to resize based on the parameters, and again map that FIFO to the physical EP. (since we call autoconfig on all interfaces, they should be assigned different endpoints) I agree that there is currently a limitation because we are going to reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is only 1 interface active at a time. The missing logic that we might be missing is seeing how we can re-purpose the FIFO that is being disabled. However, I think Jack's fix here would be applicable to the improvement in logic to re-use/re-assign FIFO space allocated by disabled EPs also. > different endpoints, the logic handling this may not be simple. > > There are a few review comments for Wesley. Hopefully they get resolved > eventually. As mentioned above, there is a lot of considerations we need to make when looking at the amount of combinations that can be done for a USB configuration. We obviously want to see if we can find a way to re-allocate FIFO space, but it gets complicated if we run into a "fragmented" situation where the RAM associated to the EP being re-allocated is in between 2 that are active. Thanks Wesley Cheng
Wesley Cheng wrote: > > > On 9/9/2021 6:15 PM, Thinh Nguyen wrote: >> Jack Pham wrote: >>> Some functions may dynamically enable and disable their endpoints >>> regularly throughout their operation, particularly when Set Interface >>> is employed to switch between Alternate Settings. For instance the >>> UAC2 function has its respective endpoints for playback & capture >>> associated with AltSetting 1, in which case those endpoints would not >>> get enabled until the host activates the AltSetting. And they >>> conversely become disabled when the interfaces' AltSetting 0 is >>> chosen. >>> >>> With the DWC3 FIFO resizing algorithm recently added, every >>> usb_ep_enable() call results in a call to resize that EP's TXFIFO, >>> but if the same endpoint is enabled again and again, this incorrectly >>> leads to FIFO RAM allocation exhaustion as the mechanism did not >>> account for the possibility that endpoints can be re-enabled many >>> times. >>> >>> Example log splat: >>> >>> dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 >>> configfs-gadget gadget: u_audio_start_capture:521 Error! >>> dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in >>> >>> This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos() >>> if an endpoint is already resized, avoiding the calculation error >>> resulting from accumulating the EP's FIFO depth repeatedly. >>> >>> Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements") >>> Signed-off-by: Jack Pham <jackp@codeaurora.org> >>> --- >>> drivers/usb/dwc3/gadget.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 804b50548163..c647c76d7361 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >>> if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) >>> return 0; >>> >>> + /* bail if already resized */ >>> + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) >>> + return 0; >>> + >>> ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >>> >>> if ((dep->endpoint.maxburst > 1 && >>> > > Hi Thinh, > >> >> This seems like a workaround more than a fix. As previously pointed out, >> there will be problems when there are multiple alternate setting >> interface [2]. If we're doing this way, are we properly allocating the >> fifo size for the alternate setting that requires the most fifo size and >> not just the first alt-setting 0? Also different alt-setting can have > > Each alt interface will call usb_ep_autoconfig() which should be > assigned different endpoint numbers. This would mean that if alt intf#0 > gets selected, and EP is enabled, then we will resize the TXFIFO and map > that FIFO to the physical EP. Then when/if the host requests the other > alt intf#1, and that calls EP enable, then the logic will then attempt > to resize based on the parameters, and again map that FIFO to the > physical EP. (since we call autoconfig on all interfaces, they should be > assigned different endpoints) That's not true. Different alt-settings of an interface can share endpoint numbers. This is often the case for UASP driver where alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we switch alt-setting, we disable the current endpoints and enable the old/new ones. > > I agree that there is currently a limitation because we are going to > reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is > only 1 interface active at a time. The missing logic that we might be > missing is seeing how we can re-purpose the FIFO that is being disabled. > However, I think Jack's fix here would be applicable to the improvement > in logic to re-use/re-assign FIFO space allocated by disabled EPs also. > Improvement is always great. I just hope we don't just stop where we are now. Since you're working on this feature at the moment, it would be good to also resolve some of the outstanding issues as Jack's fix seems to be incomplete. >> different endpoints, the logic handling this may not be simple. >> >> There are a few review comments for Wesley. Hopefully they get resolved >> eventually. > > As mentioned above, there is a lot of considerations we need to make > when looking at the amount of combinations that can be done for a USB > configuration. We obviously want to see if we can find a way to > re-allocate FIFO space, but it gets complicated if we run into a > "fragmented" situation where the RAM associated to the EP being > re-allocated is in between 2 that are active. > I'd like to have this feature added, and it would be great if it can overcome some of the current limitations. At the moment, if this feature is enabled, it may improve some applications, but it may also cause regression for some. As I noted, the fix may not be simple, but I hope this feature can work for various applications and not just a limited few. Thanks, Thinh
On 9/10/2021 8:08 PM, Thinh Nguyen wrote: > Wesley Cheng wrote: >> >> >> On 9/9/2021 6:15 PM, Thinh Nguyen wrote: >>> Jack Pham wrote: >>>> Some functions may dynamically enable and disable their endpoints >>>> regularly throughout their operation, particularly when Set Interface >>>> is employed to switch between Alternate Settings. For instance the >>>> UAC2 function has its respective endpoints for playback & capture >>>> associated with AltSetting 1, in which case those endpoints would not >>>> get enabled until the host activates the AltSetting. And they >>>> conversely become disabled when the interfaces' AltSetting 0 is >>>> chosen. >>>> >>>> With the DWC3 FIFO resizing algorithm recently added, every >>>> usb_ep_enable() call results in a call to resize that EP's TXFIFO, >>>> but if the same endpoint is enabled again and again, this incorrectly >>>> leads to FIFO RAM allocation exhaustion as the mechanism did not >>>> account for the possibility that endpoints can be re-enabled many >>>> times. >>>> >>>> Example log splat: >>>> >>>> dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 >>>> configfs-gadget gadget: u_audio_start_capture:521 Error! >>>> dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in >>>> >>>> This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos() >>>> if an endpoint is already resized, avoiding the calculation error >>>> resulting from accumulating the EP's FIFO depth repeatedly. >>>> >>>> Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements") >>>> Signed-off-by: Jack Pham <jackp@codeaurora.org> >>>> --- >>>> drivers/usb/dwc3/gadget.c | 4 ++++ >>>> 1 file changed, 4 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>> index 804b50548163..c647c76d7361 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >>>> if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) >>>> return 0; >>>> >>>> + /* bail if already resized */ >>>> + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) >>>> + return 0; >>>> + >>>> ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >>>> >>>> if ((dep->endpoint.maxburst > 1 && >>>> >> >> Hi Thinh, >> >>> >>> This seems like a workaround more than a fix. As previously pointed out, >>> there will be problems when there are multiple alternate setting >>> interface [2]. If we're doing this way, are we properly allocating the >>> fifo size for the alternate setting that requires the most fifo size and >>> not just the first alt-setting 0? Also different alt-setting can have >> >> Each alt interface will call usb_ep_autoconfig() which should be >> assigned different endpoint numbers. This would mean that if alt intf#0 >> gets selected, and EP is enabled, then we will resize the TXFIFO and map >> that FIFO to the physical EP. Then when/if the host requests the other >> alt intf#1, and that calls EP enable, then the logic will then attempt >> to resize based on the parameters, and again map that FIFO to the >> physical EP. (since we call autoconfig on all interfaces, they should be >> assigned different endpoints) Hi Thinh, > > That's not true. Different alt-settings of an interface can share > endpoint numbers. This is often the case for UASP driver where > alt-setting 0 is for BOT protocol and alt-setting 1 is UASP. When we > switch alt-setting, we disable the current endpoints and enable the > old/new ones. > Thanks for pointing that use case out. Maybe we can consider seeing if we can walk through all alternate interfaces for a particular function, and resize for the largest setting? That might be a possible improvement made to the check_config() function. Let me start makign the changes for this and verifying it. >> >> I agree that there is currently a limitation because we are going to >> reserve at minimum 1 FIFO for BOTH alt interfaces, even though there is >> only 1 interface active at a time. The missing logic that we might be >> missing is seeing how we can re-purpose the FIFO that is being disabled. >> However, I think Jack's fix here would be applicable to the improvement >> in logic to re-use/re-assign FIFO space allocated by disabled EPs also. >> > > Improvement is always great. I just hope we don't just stop where we are > now. Since you're working on this feature at the moment, it would be > good to also resolve some of the outstanding issues as Jack's fix seems > to be incomplete. > If we implement the improvement mentioned above, I think Jack's fix will be applicable there as well. If we resize for the largest alternate interface, then there would be no reason for us to resize again. >>> different endpoints, the logic handling this may not be simple. >>> >>> There are a few review comments for Wesley. Hopefully they get resolved >>> eventually. >> >> As mentioned above, there is a lot of considerations we need to make >> when looking at the amount of combinations that can be done for a USB >> configuration. We obviously want to see if we can find a way to >> re-allocate FIFO space, but it gets complicated if we run into a >> "fragmented" situation where the RAM associated to the EP being >> re-allocated is in between 2 that are active. >> > > I'd like to have this feature added, and it would be great if it can > overcome some of the current limitations. At the moment, if this feature > is enabled, it may improve some applications, but it may also cause > regression for some. As I noted, the fix may not be simple, but I hope > this feature can work for various applications and not just a limited few. > Agreed, there are some use cases that we may not consider in our platform, so I appreciate the input. Thanks Wesley Cheng -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 804b50548163..c647c76d7361 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -747,6 +747,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) if (!usb_endpoint_dir_in(dep->endpoint.desc) || dep->number <= 1) return 0; + /* bail if already resized */ + if (dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(dep->number >> 1))) + return 0; + ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); if ((dep->endpoint.maxburst > 1 &&
Some functions may dynamically enable and disable their endpoints regularly throughout their operation, particularly when Set Interface is employed to switch between Alternate Settings. For instance the UAC2 function has its respective endpoints for playback & capture associated with AltSetting 1, in which case those endpoints would not get enabled until the host activates the AltSetting. And they conversely become disabled when the interfaces' AltSetting 0 is chosen. With the DWC3 FIFO resizing algorithm recently added, every usb_ep_enable() call results in a call to resize that EP's TXFIFO, but if the same endpoint is enabled again and again, this incorrectly leads to FIFO RAM allocation exhaustion as the mechanism did not account for the possibility that endpoints can be re-enabled many times. Example log splat: dwc3 a600000.dwc3: Fifosize(3717) > RAM size(3462) ep3in depth:217973127 configfs-gadget gadget: u_audio_start_capture:521 Error! dwc3 a600000.dwc3: request 000000000be13e18 was not queued to ep3in This is easily fixed by bailing out of dwc3_gadget_resize_tx_fifos() if an endpoint is already resized, avoiding the calculation error resulting from accumulating the EP's FIFO depth repeatedly. Fixes: 9f607a309fbe9 ("usb: dwc3: Resize TX FIFOs to meet EP bursting requirements") Signed-off-by: Jack Pham <jackp@codeaurora.org> --- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)