DWC3-Gadget: Flickering with ISOC Streaming (UVC) while multiplier set on Superspeed

Hi Thinh!

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Perhaps you can capture some tracepoints of the problem?

Hi Thinh

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 3 lines

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Yes, I see missed isoc errors. But this is just a symptom in this case.

I can increase the maxburst or maxpacket parameters stepwise and on one point see the flickering appear. But when I increase the TXFIFOSIZE for the endpoint the flickering is gone again. Until I increase one of the parameters maxpacket or maxburst to much again.

So due to the memory bandwidth is under pressure, it seems like the hardware is not fast enough with sending the expected data per transfer, due to the txfifo is not long enough and needs to be refilled more often.

This sounds like a fifo underrun issue in the hardware.

Cc'ing: Stanley Chang <[email protected]>

Quoted 45 lines · Michael Grzeschik

Hi Thinh

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 3 lines

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Yes, I see missed isoc errors. But this is just a symptom in this case.

I can increase the maxburst or maxpacket parameters stepwise and on one point see the flickering appear. But when I increase the TXFIFOSIZE for the endpoint the flickering is gone again. Until I increase one of the parameters maxpacket or maxburst to much again.

So due to the memory bandwidth is under pressure, it seems like the hardware is not fast enough with sending the expected data per transfer, due to the txfifo is not long enough and needs to be refilled more often.

Quoted 1 line

This sounds like a fifo underrun issue in the hardware.

This whole issue sound like a case of stanleys patches:

https://lore.kernel.org/all/[email protected]/

For now I was unluky while adjusting the paramaters. Are those registers anywhere documented?

Quoted 52 lines · Michael Grzeschik

Cc'ing: Stanley Chang <[email protected]>

Quoted 40 lines · Michael Grzeschik

Hi Thinh

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 3 lines

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Yes, I see missed isoc errors. But this is just a symptom in this case.

I can increase the maxburst or maxpacket parameters stepwise and on one point see the flickering appear. But when I increase the TXFIFOSIZE for the endpoint the flickering is gone again. Until I increase one of the parameters maxpacket or maxburst to much again.

Quoted 6 lines

So due to the memory bandwidth is under pressure, it seems like the hardware is not fast enough with sending the expected data per transfer, due to the txfifo is not long enough and needs to be refilled more often.

This sounds like a fifo underrun issue in the hardware.

This whole issue sound like a case of stanleys patches:

Quoted 5 lines

https://lore.kernel.org/all/[email protected]/

For now I was unluky while adjusting the paramaters. Are those registers anywhere documented?

How does his change impact isoc endpoint? His change should only affect non-periodic endpoint. Also periodic settings is only for host.

Hi Thinh

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 3 lines

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Yes, I see missed isoc errors. But this is just a symptom in this case.

I can increase the maxburst or maxpacket parameters stepwise and on one point see the flickering appear. But when I increase the TXFIFOSIZE for the endpoint the flickering is gone again. Until I increase one of the parameters maxpacket or maxburst to much again.

So due to the memory bandwidth is under pressure, it seems like the hardware is not fast enough with sending the expected data per transfer, due to the txfifo is not long enough and needs to be refilled more often.

This sounds like a fifo underrun issue in the hardware.

I am currently looking into the fifo_resize device tree paramter, and try to figure out how the calculation is done.

From the software design point of view, having the fifo calculation parameterized is a bad idea. We probably want to analyze how many endpoints are going to be used in the active gadget config and use the finite fifo length to calculate some fair parts for every ep once and then never touch them again. Dynamic resizing should not be necessary or do I overlook something?

What do you think?

Quoted 2 lines

Perhaps you can capture some tracepoints of the problem?

IMHO tracepoints are probably not necessary here anymore.

Quoted 48 lines · Michael Grzeschik

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

I just stumbled over a similar issue we already solved for the High Speed Case when streaming ISOC packages and using a multiplier higher then one. Last time we saw some bad frame artefacts when using the higher multiplier value. The Frames were distorted due to truncated transfers.

Since the last case we have patch

8affe37c525d ("usb: dwc3: gadget: fix high speed multiplier setting")

that fixes the calculation of the mult PCM1 parameter when using high speed transfers. After that no truncations were reported again.

However I came across a similar issue which is just a little less easy to trigger and only occurs with Superspeed. Now, while the memory bandwidth of the machine runs on higher load, the UVC frames are similarly distorted when we use a multiplier higher then one.

Quoted 6 lines

I looked over the implications the multiplier has on the Superspeed case in the dwc3 gadget driver, but could only find some TXFIFO adjustments and no other extra bits e.g. in the transfer descriptors. Do you have some pointers how the multiplier parameter of the endpoint is used in hardware?

Quoted 3 lines

As you already know, PCM1 is only for highspeed not Superspeed. What failure did the dwc3 driver reported? Missed isoc? What's the request transfer size?

Yes, I see missed isoc errors. But this is just a symptom in this case.

I can increase the maxburst or maxpacket parameters stepwise and on one point see the flickering appear. But when I increase the TXFIFOSIZE for the endpoint the flickering is gone again. Until I increase one of the parameters maxpacket or maxburst to much again.

So due to the memory bandwidth is under pressure, it seems like the hardware is not fast enough with sending the expected data per transfer, due to the txfifo is not long enough and needs to be refilled more often.

This sounds like a fifo underrun issue in the hardware.

I am currently looking into the fifo_resize device tree paramter, and try to figure out how the calculation is done.

Quoted 8 lines

From the software design point of view, having the fifo calculation parameterized is a bad idea. We probably want to analyze how many endpoints are going to be used in the active gadget config and use the finite fifo length to calculate some fair parts for every ep once and then never touch them again. Dynamic resizing should not be necessary or do I overlook something?

What do you think?

There are multiple factors that impact the isoc performance:
1) FIFO size
Typically the FIFO size is recommended to a minimum of the output of dwc3_gadget_calc_tx_fifo_size(). For isoc, depending on the request size, if it's 48KB/uframe, you probably need more than the minimum. Each physical endpoint has a pre-configured TX FIFO size. Without touching the FIFO reconfiguration parameter, then the driver will just pick the first physical endpoint that can be used, but it may not be the best fit for your purpose.

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

3) Internal cache size
The controller caches X number of TRBs every time you queue a new request. If a request has a lot small TRBs, then it needs to recache multiple times just to complete the request. Typically the controller caches 8 or 16 TRBs. I notice that UVC uses SG to split up the request to small SG entries base on logical parts rather than for performance or hardware size contraints. So, there can be improvements here.

4) Host bandwidth constraint
The host can limit the burst threshold and do less burst than what the device is capable of due to the host's bandwidth contraint. If there are other endpoint traffic on top of isoc endpoint, then the host would have to reserve bandwidth for other endpoints. Depending on the host controller implementation, it may reserve more or less for isoc. Also, if there are hubs or other devices in the topology, then it impacts the bandwidth too.

Base on your description, looks like modifying TX FIFO size impacts the most. We already have some properties to calculate and resize the TX FIFO size, did you try to see if it improves for you? (check dwc->do_fifo_resize).

Quoted 4 lines

Quoted 1 line

Perhaps you can capture some tracepoints of the problem?

IMHO tracepoints are probably not necessary here anymore.

This helps to see how uvc setups the transfer and see what could impact the isoc transfers (for point 3 and 4 above). However, if just resizing the TX FIFO works for you, perhaps we can just focus into this.

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

Quoted 8 lines · Michael Grzeschik

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

The dwc3 driver checks the endpoint descriptor from the UVC function driver to setup the burst. So just setup the max 16 bursts (or 15 in the descriptor). The dwc3 controller supports that. Whether the host would do 16 bursts is another thing. Note that there's no "mult" setting for SuperSpeed.

I recall that UVC tries to pack a lot of data in a single request. All the while some intervals it would send 0-length data because of idle time. I would spread to more requests with a little less data to give the host a little more breathing room and bandwidth.

BR,
Thinh

Quoted 10 lines · Thinh Nguyen

Quoted 8 lines · Michael Grzeschik

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

Quoted 5 lines

The dwc3 driver checks the endpoint descriptor from the UVC function driver to setup the burst. So just setup the max 16 bursts (or 15 in the descriptor). The dwc3 controller supports that. Whether the host would do 16 bursts is another thing. Note that there's no "mult" setting for SuperSpeed.

Clarification: no mult setting for the dwc3 controller when operate in SuperSpeed.

Quoted 4 lines

I recall that UVC tries to pack a lot of data in a single request. All the while some intervals it would send 0-length data because of idle time. I would spread to more requests with a little less data to give the host a little more breathing room and bandwidth.

Hi Thinh

Thanks for the update!

Quoted 17 lines · Thinh Nguyen

Quoted 15 lines · Thinh Nguyen

Quoted 8 lines · Michael Grzeschik

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

The dwc3 driver checks the endpoint descriptor from the UVC function driver to setup the burst. So just setup the max 16 bursts (or 15 in the descriptor). The dwc3 controller supports that. Whether the host would do 16 bursts is another thing. Note that there's no "mult" setting for SuperSpeed.

Quoted 2 lines

Clarification: no mult setting for the dwc3 controller when operate in SuperSpeed.

I was somehow mistaken by the wording "burst setting" and thought of the axi-bus burst setting to the controller instead of the usb3 maxburst setting as you ment actually.

However. This is usefull input anyway. I never thought of maximizing the burst and packagesize and let the host side make the decision. But we will probably will eat up a lot of usb bandwidth by doing so.

Before your note I was somehow mistaken that the maxpacket and maxburst size had to correlate with the actually transfered data we queue into the hardware per request.

Quoted 5 lines

Quoted 5 lines

I recall that UVC tries to pack a lot of data in a single request. All the while some intervals it would send 0-length data because of idle time. I would spread to more requests with a little less data to give the host a little more breathing room and bandwidth.

The higher load per request is due to the fact that the uvc gadget is using the multiplier, maxpacket and maxburst parameters for the size calculation of the request.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/function/uvc_video.c#n331

Since it is clear now that those parameters are not necessary coupled it makes total sense to split the requests into smaller chunks.

Quoted 18 lines · Michael Grzeschik

Quoted 17 lines · Thinh Nguyen

Quoted 15 lines · Thinh Nguyen

Quoted 8 lines · Michael Grzeschik

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

The dwc3 driver checks the endpoint descriptor from the UVC function driver to setup the burst. So just setup the max 16 bursts (or 15 in the descriptor). The dwc3 controller supports that. Whether the host would do 16 bursts is another thing. Note that there's no "mult" setting for SuperSpeed.

Quoted 6 lines

Quoted 2 lines

Clarification: no mult setting for the dwc3 controller when operate in SuperSpeed.

I was somehow mistaken by the wording "burst setting" and thought of the axi-bus burst setting to the controller instead of the usb3 maxburst setting as you ment actually.

I see. You were referring to the axi-bus burst. If your platform takes a long time to DMA out the data, it will impact the performance also. You can play around with GSBUSCFG0 to enable/restrict certain burst sizes to see any improvement. However, I would expect the default coreConfiguration values should be optimal for your platform design.

Quoted 7 lines

However. This is usefull input anyway. I never thought of maximizing the burst and packagesize and let the host side make the decision. But we will probably will eat up a lot of usb bandwidth by doing so.

Before your note I was somehow mistaken that the maxpacket and maxburst size had to correlate with the actually transfered data we queue into the hardware per request.

Right. The maxpacket, maxburst, and mult limit the max request data length you can send.

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I recall that UVC tries to pack a lot of data in a single request. All the while some intervals it would send 0-length data because of idle time. I would spread to more requests with a little less data to give the host a little more breathing room and bandwidth.

The higher load per request is due to the fact that the uvc gadget is using the multiplier, maxpacket and maxburst parameters for the size calculation of the request.

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/function/uvc_video.c#n331

Since it is clear now that those parameters are not necessary coupled it makes total sense to split the requests into smaller chunks.

Ok.

Hi Thinh,

Quoted 26 lines · Thinh Nguyen

Quoted 18 lines · Michael Grzeschik

Quoted 17 lines · Thinh Nguyen

Quoted 15 lines · Thinh Nguyen

Quoted 8 lines · Michael Grzeschik

Quoted 4 lines

2) Burst setting
I think this is self-explainatory. Large data request needs higher burst.

I will have to find out if the burst setting can be changed on the rk3568 somehow. This sounds very likely.

The dwc3 driver checks the endpoint descriptor from the UVC function driver to setup the burst. So just setup the max 16 bursts (or 15 in the descriptor). The dwc3 controller supports that. Whether the host would do 16 bursts is another thing. Note that there's no "mult" setting for SuperSpeed.

Quoted 6 lines

Quoted 2 lines

Clarification: no mult setting for the dwc3 controller when operate in SuperSpeed.

I was somehow mistaken by the wording "burst setting" and thought of the axi-bus burst setting to the controller instead of the usb3 maxburst setting as you ment actually.

Quoted 5 lines

I see. You were referring to the axi-bus burst. If your platform takes a long time to DMA out the data, it will impact the performance also. You can play around with GSBUSCFG0 to enable/restrict certain burst sizes to see any improvement. However, I would expect the default coreConfiguration values should be optimal for your platform design.

I was not lucky with that parameters. Under heavy memory load the system still runs into fifo underruns.

Quoted 20 lines

Quoted 7 lines

However. This is usefull input anyway. I never thought of maximizing the burst and packagesize and let the host side make the decision. But we will probably will eat up a lot of usb bandwidth by doing so.

Before your note I was somehow mistaken that the maxpacket and maxburst size had to correlate with the actually transfered data we queue into the hardware per request.

Right. The maxpacket, maxburst, and mult limit the max request data length you can send.

Quoted 9 lines

Quoted 4 lines

Quoted 4 lines

I recall that UVC tries to pack a lot of data in a single request. All the while some intervals it would send 0-length data because of idle time. I would spread to more requests with a little less data to give the host a little more breathing room and bandwidth.

The higher load per request is due to the fact that the uvc gadget is using the multiplier, maxpacket and maxburst parameters for the size calculation of the request.

Quoted 7 lines

Quoted 5 lines

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/gadget/function/uvc_video.c#n331

Since it is clear now that those parameters are not necessary coupled it makes total sense to split the requests into smaller chunks.

Ok.

So changing the req_size to smaller chunks indeed did increase the stability. The main misunderstanding here was that the that not every request corresponds with the timeslot of one interval.

After reading this thread once again, it is clear that this is not the case and we still find all possible bandwidth by decreasing the size of each request.

The main takeway was that with the hardware will cache several trbs into the queue. So when there are not enough trbs available because they are just to big, the fifo runs into underruns.

In our case with the high memory bandwidth usage, the trbs could probably not even read out that few trbs in time and did trigger the case earlier.

So with smaller requests the fifo will be filled with more smaller trbs and does not run out that fast, since more trbs are cached to begin with.

One more question: Does the caching amount of the fifo directly correlate with the endpoint setting in DWC3_DEPCFG_BURST_SIZE then?

One misconception of queueing request to the usb gadget controller is, that the amount of data that one usb_request is representing is the same as the hardware is able to transfer in one interval.

This exact idea applies to the uvc_video gadget that assumes that the request needs to have the size as the gadgets bandwidth parameters are configured with. (maxpacket, multiplier and burst)

Although it is actually no problem for the hardware to queue a big request, in the case of isochronous transfers it is impractical to queue big amount of data per request to the hardware. Especially if the necessary bandwidth was increased on purpose to maintain high amounts of data.

E.g. in the dwc3-controller it has the negative impact that the endpoint FIFO will not be filled fast enough by the internal hardware engine. Since each transfer buffer (TRB) represents one big request, the hardware will need a long time to prefill the hardware FIFO. This can be avoided by queueing more smaller requests which will then lead to smaller TRBs which the hardware engine can keep up with.

This patch is simply dropping the maxburst as an multiplier for the size calculation and therefor decreases the overall maximum request size.

Signed-off-by Michael Grzeschik <[email protected]>

---
This patch is created as an result from the discussion in the following thread:

https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/__;!!A4F2R9G_pg!fTaIo4tDljSbEvUY5SZLkNvKWcz0YeN0Ekzs0CPWyD73RGRmErRC2frODFgnMB1M4Nse0oKKgwxC65gePhGAtauKJq1Vnzlj$

 drivers/usb/gadget/function/uvc_queue.c | 1 -
 drivers/usb/gadget/function/uvc_video.c | 1 -
 2 files changed, 2 deletions(-)

drivers/usb/gadget/function/uvc_queue.c +0 -0

index 0aa3d7e1f3cc32..1d3c3c09ff97cb 100644

drivers/usb/gadget/function/uvc_queue.c +0 -0

 		 * (video->ep->mult);
 
 	/* We divide by two, to increase the chance to run

drivers/usb/gadget/function/uvc_video.c +0 -1

index 91af3b1ef0d412..c6b61843bad3d7 100644
@@ -329,7 +329,6 @@ uvc_video_alloc_requests(struct uvc_video *video)
 	BUG_ON(video->req_size);
 
 	req_size = video->ep->maxpacket
-		 * max_t(unsigned int, video->ep->maxburst, 1)
 		 * (video->ep->mult);
 
 	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);

Signature · 1 line

2.39.2

Hi,

Quoted 14 lines · Michael Grzeschik

One misconception of queueing request to the usb gadget controller is, that the amount of data that one usb_request is representing is the same as the hardware is able to transfer in one interval.

This exact idea applies to the uvc_video gadget that assumes that the request needs to have the size as the gadgets bandwidth parameters are configured with. (maxpacket, multiplier and burst)

Although it is actually no problem for the hardware to queue a big request, in the case of isochronous transfers it is impractical to queue big amount of data per request to the hardware. Especially if the necessary bandwidth was increased on purpose to maintain high amounts of data.

Quoted 6 lines

E.g. in the dwc3-controller it has the negative impact that the endpoint FIFO will not be filled fast enough by the internal hardware engine. Since each transfer buffer (TRB) represents one big request, the hardware will need a long time to prefill the hardware FIFO. This can be avoided by queueing more smaller requests which will then lead to smaller TRBs which the hardware engine can keep up with.

Just want to clarify here to avoid any confusion, the hardware TX FIFO size is relatively small, usually can be smaller than the TRB. It should be fine whether the TRB is larger or smaller than the FIFO size. The hardware does not "prefill" the FIFO. It just fills whichever TRB it's currently processing (I think you may be mixing up with TRB cache).

The performance impact from this change is to reduce the USB bandwidth usage. The current calculation in uvc function can use 48KB/uframe for each request in SuperSpeed, the max size for isoc in SuperSpeed. I know many hosts can't handle this kind of transfer rate from their hardware. (e.g. It gets worse when scheduling transfers for multiple endpoints and under multiple tier hubs).

The bandwidth can be impacted by multiple factors and not just from the gadget side as noted in the discussion before.

Quoted 21 lines

This patch is simply dropping the maxburst as an multiplier for the size calculation and therefor decreases the overall maximum request size.

Signed-off-by Michael Grzeschik <[email protected]>

---
This patch is created as an result from the discussion in the following thread:

https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/__;!!A4F2R9G_pg!fTaIo4tDljSbEvUY5SZLkNvKWcz0YeN0Ekzs0CPWyD73RGRmErRC2frODFgnMB1M4Nse0oKKgwxC65gePhGAtauKJq1Vnzlj$

 drivers/usb/gadget/function/uvc_queue.c | 1 -
 drivers/usb/gadget/function/uvc_video.c | 1 -
 2 files changed, 2 deletions(-)

drivers/usb/gadget/function/uvc_queue.c +0 -0

index 0aa3d7e1f3cc32..1d3c3c09ff97cb 100644
@@ -55,7 +55,6 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 	sizes[0] = video->imagesize;
 

drivers/usb/gadget/function/uvc_queue.c static int uvc_queue_setup(struct vb2_queue *vq, · line 58

Quoted 2 lines

drivers/usb/gadget/function/uvc_queue.c +0 -1

 	req_size = video->ep->maxpacket
-		 * max_t(unsigned int, video->ep->maxburst, 1)

I think you're reducing a bit too much here? Also, take advantage of burst. So, perhaps keep request size to max(16K, MPS * maxburst)?

Can be more or less depending on how much video data is needed to transfer over a video frame.

Quoted 14 lines · Michael Grzeschik

One misconception of queueing request to the usb gadget controller is, that the amount of data that one usb_request is representing is the same as the hardware is able to transfer in one interval.

This exact idea applies to the uvc_video gadget that assumes that the request needs to have the size as the gadgets bandwidth parameters are configured with. (maxpacket, multiplier and burst)

Although it is actually no problem for the hardware to queue a big request, in the case of isochronous transfers it is impractical to queue big amount of data per request to the hardware. Especially if the necessary bandwidth was increased on purpose to maintain high amounts of data.

Quoted 6 lines

E.g. in the dwc3-controller it has the negative impact that the endpoint FIFO will not be filled fast enough by the internal hardware engine. Since each transfer buffer (TRB) represents one big request, the hardware will need a long time to prefill the hardware FIFO. This can be avoided by queueing more smaller requests which will then lead to smaller TRBs which the hardware engine can keep up with.

Just want to clarify here to avoid any confusion, the hardware TX FIFO size is relatively small, usually can be smaller than the TRB. It should be fine whether the TRB is larger or smaller than the FIFO size. The hardware does not "prefill" the FIFO. It just fills whichever TRB it's currently processing (I think you may be mixing up with TRB cache).

The performance impact from this change is to reduce the USB bandwidth usage. The current calculation in uvc function can use 48KB/uframe for each request in SuperSpeed, the max size for isoc in SuperSpeed. I know many hosts can't handle this kind of transfer rate from their hardware. (e.g. It gets worse when scheduling transfers for multiple endpoints and under multiple tier hubs).

The bandwidth can be impacted by multiple factors and not just from the gadget side as noted in the discussion before.

Quoted 18 lines

This patch is simply dropping the maxburst as an multiplier for the size calculation and therefor decreases the overall maximum request size.

Signed-off-by Michael Grzeschik <[email protected]>

---
This patch is created as an result from the discussion in the following thread:

https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/__;!!A4F2R9G_pg!fTaIo4tDljSbEvUY5SZLkNvKWcz0YeN0Ekzs0CPWyD73RGRmErRC2frODFgnMB1M4Nse0oKKgwxC65gePhGAtauKJq1Vnzlj$

 drivers/usb/gadget/function/uvc_queue.c | 1 -
 drivers/usb/gadget/function/uvc_video.c | 1 -
 2 files changed, 2 deletions(-)

drivers/usb/gadget/function/uvc_queue.c +0 -0

index 0aa3d7e1f3cc32..1d3c3c09ff97cb 100644

drivers/usb/gadget/function/uvc_queue.c static int uvc_queue_setup(struct vb2_queue *vq, · line 58

Quoted 4 lines

drivers/usb/gadget/function/uvc_queue.c +0 -1

@@ -55,7 +55,6 @@ static int uvc_queue_setup(struct vb2_queue *vq,
 	sizes[0] = video->imagesize;
 
 	req_size = video->ep->maxpacket
-		 * max_t(unsigned int, video->ep->maxburst, 1)

I think you're reducing a bit too much here? Also, take advantage of burst. So, perhaps keep request size to max(16K, MPS * maxburst)?

Can be more or less depending on how much video data is needed to transfer over a video frame.

BR,
Thinh

Quoted 19 lines

drivers/usb/gadget/function/uvc_queue.c +0 -0

 		 * (video->ep->mult);
 
 	/* We divide by two, to increase the chance to run

drivers/usb/gadget/function/uvc_video.c +0 -2

index 91af3b1ef0d412..c6b61843bad3d7 100644
@@ -329,7 +329,6 @@ uvc_video_alloc_requests(struct uvc_video *video)
 	BUG_ON(video->req_size);
 
 	req_size = video->ep->maxpacket
-		 * max_t(unsigned int, video->ep->maxburst, 1)
 		 * (video->ep->mult);
 
 	video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL);
--

2.39.2

Quoted 22 lines · Thinh Nguyen

Quoted 14 lines · Michael Grzeschik

One misconception of queueing request to the usb gadget controller is, that the amount of data that one usb_request is representing is the same as the hardware is able to transfer in one interval.

This exact idea applies to the uvc_video gadget that assumes that the request needs to have the size as the gadgets bandwidth parameters are configured with. (maxpacket, multiplier and burst)

Although it is actually no problem for the hardware to queue a big request, in the case of isochronous transfers it is impractical to queue big amount of data per request to the hardware. Especially if the necessary bandwidth was increased on purpose to maintain high amounts of data.

Quoted 6 lines

E.g. in the dwc3-controller it has the negative impact that the endpoint FIFO will not be filled fast enough by the internal hardware engine. Since each transfer buffer (TRB) represents one big request, the hardware will need a long time to prefill the hardware FIFO. This can be avoided by queueing more smaller requests which will then lead to smaller TRBs which the hardware engine can keep up with.

Quoted 5 lines

Just want to clarify here to avoid any confusion, the hardware TX FIFO size is relatively small, usually can be smaller than the TRB. It should be fine whether the TRB is larger or smaller than the FIFO size. The hardware does not "prefill" the FIFO. It just fills whichever TRB it's currently processing (I think you may be mixing up with TRB cache).

What I see is, that by using bigger TRBs the hardware is not able to keep up with reading from the memory when the system is under heavy memory pressure. This is the main reason for this change.

Since we found out that increasing the FIFO size had an effect to how high we are able to set the hardware endpoint configuration on our gadget side (params.param0), until we saw the issue reoccur.

So the Idea here was to have a tweak on how the hardware handles the data from the memory to the Hardware-FIFO which seems not to underrun with smaller TRBs.

Quoted 6 lines

The performance impact from this change is to reduce the USB bandwidth usage. The current calculation in uvc function can use 48KB/uframe for each request in SuperSpeed, the max size for isoc in SuperSpeed. I know many hosts can't handle this kind of transfer rate from their hardware. (e.g. It gets worse when scheduling transfers for multiple endpoints and under multiple tier hubs).

I think I don't fully understand here.

We change the overall buffersize of the usb_request and therefor limit the size of possible TRBs. This should even only have most effect on the trbsize for the memcopy path, since the scatter gather requests are already split into multiple trbs which is capped to the maximum mappable memory range of PAGE_SIZE (4k).

Other then that, the parameterization of the endpoint on our gadget side is not changed by this patch. The endpoint configuration is set as follows:

params.param0 |= DWC3_DEPCFG_BURST_SIZE(burst - 1) |

                 DWC3_DEPCFG_MAX_PACKET_SIZE(usb_endpoint_maxp(desc));

So by changing the request_size there should not be any other change in the gadget side hardware configuration.

How is the overall bandwidth usage affected by this change then other than we have more smaller potential trbs in the queue.

If the Intervallength is not coupled to the amount of to be transfered TRBs in any case, it should not have an effect to the bandwidth.

If I am mistaken here, can you point me to some code?