Re: [PATCH] usb: dwc3: gadget: Fix TX FIFO size for HS ISOC endpoints

[AKASH KUMAR]

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@xxxxxxxxxxx>
> > > > ---
> > > >    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