| Message ID | 20240827054956.28241-1-quic_akakum@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: gadget: Fix TX FIFO size for HS ISOC endpoints | expand |
On Tue, Aug 27, 2024, Akash Kumar wrote: > Use 2K TX FIFO size for low-resolution UVC cameras to support the > maximum possible UVC instances. Restrict 2K TX FIFO size based on > the minimum maxburst required to run low-resolution UVC cameras. > > Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> > --- > 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 89fc690fdf34..f342ccda6705 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > num_fifos = dwc->tx_fifo_resize_max_num; > > + if (dep->endpoint.maxburst <= 1 && > + usb_endpoint_xfer_isoc(dep->endpoint.desc)) > + num_fifos = 2; > + > /* FIFO size for a single buffer */ > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > -- > 2.17.1 > These settings are starting to get too specific for each application. Can we find a better calculation? Perhaps something like this? (code not tested) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 9a18973ebc05..d54b08f92aea 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); - if ((dep->endpoint.maxburst > 1 && - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || usb_endpoint_xfer_isoc(dep->endpoint.desc)) - num_fifos = 3; - - if (dep->endpoint.maxburst > 6 && - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) - num_fifos = dwc->tx_fifo_resize_max_num; + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, + dwc->tx_fifo_resize_max_num); /* FIFO size for a single buffer */ fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); Thanks, Thinh
On Tue, Aug 27, 2024, Thinh Nguyen wrote: > On Tue, Aug 27, 2024, Akash Kumar wrote: > > Use 2K TX FIFO size for low-resolution UVC cameras to support the > > maximum possible UVC instances. Restrict 2K TX FIFO size based on > > the minimum maxburst required to run low-resolution UVC cameras. > > > > Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> > > --- > > 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 89fc690fdf34..f342ccda6705 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > > num_fifos = dwc->tx_fifo_resize_max_num; > > > > + if (dep->endpoint.maxburst <= 1 && > > + usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > + num_fifos = 2; > > + > > /* FIFO size for a single buffer */ > > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > > > -- > > 2.17.1 > > > > These settings are starting to get too specific for each application. > Can we find a better calculation? > > Perhaps something like this? (code not tested) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 9a18973ebc05..d54b08f92aea 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > - if ((dep->endpoint.maxburst > 1 && > - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > usb_endpoint_xfer_isoc(dep->endpoint.desc)) > - num_fifos = 3; > - > - if (dep->endpoint.maxburst > 6 && > - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > - num_fifos = dwc->tx_fifo_resize_max_num; > + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, > + dwc->tx_fifo_resize_max_num); > > /* FIFO size for a single buffer */ > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > Also, for highspeed, we don't check maxburst. Check for additional packets properly. Thanks, Thinh
Hi Thinh, On 8/28/2024 4:45 AM, Thinh Nguyen wrote: > On Tue, Aug 27, 2024, Akash Kumar wrote: >> Use 2K TX FIFO size for low-resolution UVC cameras to support the >> maximum possible UVC instances. Restrict 2K TX FIFO size based on >> the minimum maxburst required to run low-resolution UVC cameras. >> >> Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> >> --- >> 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 89fc690fdf34..f342ccda6705 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >> usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) >> num_fifos = dwc->tx_fifo_resize_max_num; >> >> + if (dep->endpoint.maxburst <= 1 && >> + usb_endpoint_xfer_isoc(dep->endpoint.desc)) >> + num_fifos = 2; >> + >> /* FIFO size for a single buffer */ >> fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); >> >> -- >> 2.17.1 >> > These settings are starting to get too specific for each application. > Can we find a better calculation? > > Perhaps something like this? (code not tested) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 9a18973ebc05..d54b08f92aea 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > - if ((dep->endpoint.maxburst > 1 && > - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > usb_endpoint_xfer_isoc(dep->endpoint.desc)) > - num_fifos = 3; > - > - if (dep->endpoint.maxburst > 6 && > - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > - num_fifos = dwc->tx_fifo_resize_max_num; > + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, > + dwc->tx_fifo_resize_max_num); > > /* FIFO size for a single buffer */ > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > should be fine for me, as earlier there was no case handling for maxburst <= 1, by allocating fifo based on maxburst itself should be a good solution and will work for all as they customize based on maxburst through init scripts, let me test and update. Thanks, Akash
On Wed, Aug 28, 2024, AKASH KUMAR wrote: > Hi Thinh, > > On 8/28/2024 4:45 AM, Thinh Nguyen wrote: > > On Tue, Aug 27, 2024, Akash Kumar wrote: > > > Use 2K TX FIFO size for low-resolution UVC cameras to support the > > > maximum possible UVC instances. Restrict 2K TX FIFO size based on > > > the minimum maxburst required to run low-resolution UVC cameras. > > > > > > Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> > > > --- > > > 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 89fc690fdf34..f342ccda6705 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > > usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > > > num_fifos = dwc->tx_fifo_resize_max_num; > > > + if (dep->endpoint.maxburst <= 1 && > > > + usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > > + num_fifos = 2; > > > + > > > /* FIFO size for a single buffer */ > > > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > > -- > > > 2.17.1 > > > > > These settings are starting to get too specific for each application. > > Can we find a better calculation? > > > > Perhaps something like this? (code not tested) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 9a18973ebc05..d54b08f92aea 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > - if ((dep->endpoint.maxburst > 1 && > > - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || > > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > > usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > - num_fifos = 3; > > - > > - if (dep->endpoint.maxburst > 6 && > > - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > > - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > > - num_fifos = dwc->tx_fifo_resize_max_num; > > + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, > > + dwc->tx_fifo_resize_max_num); > > /* FIFO size for a single buffer */ > > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > > should be fine for me, as earlier there was no case handling for maxburst <= > 1, by allocating fifo based on maxburst itself > > should be a good solution and will work for all as they customize based on > maxburst through init scripts, let me test and update. > As I noted in the follow up response, you need to also account for additional transaction opportunities for isoc if operating in highspeed. (ie. use usb_endpoint_maxp_mult()). BR, Thinh
Hi Thinh, On 8/29/2024 5:15 AM, Thinh Nguyen wrote: > On Wed, Aug 28, 2024, AKASH KUMAR wrote: >> Hi Thinh, >> >> On 8/28/2024 4:45 AM, Thinh Nguyen wrote: >>> On Tue, Aug 27, 2024, Akash Kumar wrote: >>>> Use 2K TX FIFO size for low-resolution UVC cameras to support the >>>> maximum possible UVC instances. Restrict 2K TX FIFO size based on >>>> the minimum maxburst required to run low-resolution UVC cameras. >>>> >>>> Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> >>>> --- >>>> 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 89fc690fdf34..f342ccda6705 100644 >>>> --- a/drivers/usb/dwc3/gadget.c >>>> +++ b/drivers/usb/dwc3/gadget.c >>>> @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >>>> usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) >>>> num_fifos = dwc->tx_fifo_resize_max_num; >>>> + if (dep->endpoint.maxburst <= 1 && >>>> + usb_endpoint_xfer_isoc(dep->endpoint.desc)) >>>> + num_fifos = 2; >>>> + >>>> /* FIFO size for a single buffer */ >>>> fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); >>>> -- >>>> 2.17.1 >>>> >>> These settings are starting to get too specific for each application. >>> Can we find a better calculation? >>> >>> Perhaps something like this? (code not tested) >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 9a18973ebc05..d54b08f92aea 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) >>> ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); >>> - if ((dep->endpoint.maxburst > 1 && >>> - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || >>> + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || >>> usb_endpoint_xfer_isoc(dep->endpoint.desc)) >>> - num_fifos = 3; >>> - >>> - if (dep->endpoint.maxburst > 6 && >>> - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || >>> - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) >>> - num_fifos = dwc->tx_fifo_resize_max_num; >>> + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, >>> + dwc->tx_fifo_resize_max_num); >>> /* FIFO size for a single buffer */ >>> fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); >>> >> should be fine for me, as earlier there was no case handling for maxburst <= >> 1, by allocating fifo based on maxburst itself >> >> should be a good solution and will work for all as they customize based on >> maxburst through init scripts, let me test and update. >> > As I noted in the follow up response, you need to also account for > additional transaction opportunities for isoc if operating in highspeed. > (ie. use usb_endpoint_maxp_mult()). yeah i think that we can control with maxpacket size for high speed cams and we can work with maxburst requested only to allocate fifo as both are configurable entity and one should be free to adjust fifo as per his environment and not add any additional limitation. Let me know if we aligned and go ahead with above suggestion only. Thanks, Akash
On Thu, Aug 29, 2024, AKASH KUMAR wrote: > Hi Thinh, > > On 8/29/2024 5:15 AM, Thinh Nguyen wrote: > > On Wed, Aug 28, 2024, AKASH KUMAR wrote: > > > Hi Thinh, > > > > > > On 8/28/2024 4:45 AM, Thinh Nguyen wrote: > > > > On Tue, Aug 27, 2024, Akash Kumar wrote: > > > > > Use 2K TX FIFO size for low-resolution UVC cameras to support the > > > > > maximum possible UVC instances. Restrict 2K TX FIFO size based on > > > > > the minimum maxburst required to run low-resolution UVC cameras. > > > > > > > > > > Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> > > > > > --- > > > > > 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 89fc690fdf34..f342ccda6705 100644 > > > > > --- a/drivers/usb/dwc3/gadget.c > > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > > @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > > > > usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > > > > > num_fifos = dwc->tx_fifo_resize_max_num; > > > > > + if (dep->endpoint.maxburst <= 1 && > > > > > + usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > > > > + num_fifos = 2; > > > > > + > > > > > /* FIFO size for a single buffer */ > > > > > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > > > > -- > > > > > 2.17.1 > > > > > > > > > These settings are starting to get too specific for each application. > > > > Can we find a better calculation? > > > > > > > > Perhaps something like this? (code not tested) > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > > index 9a18973ebc05..d54b08f92aea 100644 > > > > --- a/drivers/usb/dwc3/gadget.c > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > @@ -908,15 +908,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) > > > > ram1_depth = DWC3_RAM1_DEPTH(dwc->hwparams.hwparams7); > > > > - if ((dep->endpoint.maxburst > 1 && > > > > - usb_endpoint_xfer_bulk(dep->endpoint.desc)) || > > > > + if (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > > > > usb_endpoint_xfer_isoc(dep->endpoint.desc)) > > > > - num_fifos = 3; > > > > - > > > > - if (dep->endpoint.maxburst > 6 && > > > > - (usb_endpoint_xfer_bulk(dep->endpoint.desc) || > > > > - usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) > > > > - num_fifos = dwc->tx_fifo_resize_max_num; > > > > + num_fifos = min_t(unsigned int, dep->endpoint.maxburst + 1, > > > > + dwc->tx_fifo_resize_max_num); > > > > /* FIFO size for a single buffer */ > > > > fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1); > > > > > > > should be fine for me, as earlier there was no case handling for maxburst <= > > > 1, by allocating fifo based on maxburst itself > > > > > > should be a good solution and will work for all as they customize based on > > > maxburst through init scripts, let me test and update. > > > > > As I noted in the follow up response, you need to also account for > > additional transaction opportunities for isoc if operating in highspeed. > > (ie. use usb_endpoint_maxp_mult()). > yeah i think that we can control with maxpacket size for high speed cams and > we can > work with maxburst requested only to allocate fifo as both are configurable > entity and one > should be free to adjust fifo as per his environment and not add any > additional limitation. > Let me know if we aligned and go ahead with above suggestion only. > I'm not quite clear here. Perhaps you can provide the change so we can review. BR, Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89fc690fdf34..f342ccda6705 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -788,6 +788,10 @@ static int dwc3_gadget_resize_tx_fifos(struct dwc3_ep *dep) usb_endpoint_xfer_isoc(dep->endpoint.desc)) && DWC3_IP_IS(DWC31)) num_fifos = dwc->tx_fifo_resize_max_num; + if (dep->endpoint.maxburst <= 1 && + usb_endpoint_xfer_isoc(dep->endpoint.desc)) + num_fifos = 2; + /* FIFO size for a single buffer */ fifo = dwc3_gadget_calc_tx_fifo_size(dwc, 1);
Use 2K TX FIFO size for low-resolution UVC cameras to support the maximum possible UVC instances. Restrict 2K TX FIFO size based on the minimum maxburst required to run low-resolution UVC cameras. Signed-off-by: Akash Kumar <quic_akakum@quicinc.com> --- drivers/usb/dwc3/gadget.c | 4 ++++ 1 file changed, 4 insertions(+)