| Message ID | 20230803091053.9714-4-quic_linyyuan@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3c5b006f3ee800b4bd9ed37b3a8f271b8560126e |
| Headers | show |
| Series | remove some usage of gadget_is_{*}speed() API | expand |
On Thu, Aug 03, 2023 at 05:10:49PM +0800, Linyu Yuan wrote: > when call uvc_function_bind(), gadget still have no connection speed, > just follow other gadget function, use fs endpoint descriptor to allocate > a video endpoint, remove gadget_is_{super|dual}speed() API call. > > Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > --- > v2: no change > > drivers/usb/gadget/function/f_uvc.c | 10 +--------- > 1 file changed, 1 insertion(+), 9 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index 5e919fb65833..c8e149f8315f 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -719,21 +719,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) > } > uvc->enable_interrupt_ep = opts->enable_interrupt_ep; > > - if (gadget_is_superspeed(c->cdev->gadget)) > - ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, > - &uvc_ss_streaming_comp); > - else if (gadget_is_dualspeed(cdev->gadget)) > - ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep); > - else > - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); > - > + ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); Some UDC driver use gadget_check_config() and match_ep() to allocate EP internal fifo memory resource, if only pass download full speed EP. UDC will allocate too much internal memory to each EP. It may failure when use ss config. Generally, ss config have bigger max package size. Frank > if (!ep) { > uvcg_info(f, "Unable to allocate streaming EP\n"); > goto error; > } > uvc->video.ep = ep; > > - uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address; > uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address; > uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address; > > -- > 2.17.1 >
On 2023/12/20 00:17, Frank Li wrote: > On Thu, Aug 03, 2023 at 05:10:49PM +0800, Linyu Yuan wrote: >> when call uvc_function_bind(), gadget still have no connection speed, >> just follow other gadget function, use fs endpoint descriptor to allocate >> a video endpoint, remove gadget_is_{super|dual}speed() API call. >> >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> >> --- >> v2: no change >> >> drivers/usb/gadget/function/f_uvc.c | 10 +--------- >> 1 file changed, 1 insertion(+), 9 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c >> index 5e919fb65833..c8e149f8315f 100644 >> --- a/drivers/usb/gadget/function/f_uvc.c >> +++ b/drivers/usb/gadget/function/f_uvc.c >> @@ -719,21 +719,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) >> } >> uvc->enable_interrupt_ep = opts->enable_interrupt_ep; >> >> - if (gadget_is_superspeed(c->cdev->gadget)) >> - ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, >> - &uvc_ss_streaming_comp); >> - else if (gadget_is_dualspeed(cdev->gadget)) >> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep); >> - else >> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); >> - >> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); > Some UDC driver use gadget_check_config() and match_ep() to allocate EP > internal fifo memory resource, if only pass download full speed EP. Could you share the detail of problem ? do you mean find another different endpoint compared with change before? From my understanding, according to configfs gadget driver design, when find a endpoint, there is no working speed, this means each hardware endpoint should support all possible speeds. > > UDC will allocate too much internal memory to each EP. It may failure when > use ss config. Generally, ss config have bigger max package size. is there another way to solve your issue in your driver ? > > Frank > >> if (!ep) { >> uvcg_info(f, "Unable to allocate streaming EP\n"); >> goto error; >> } >> uvc->video.ep = ep; >> >> - uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address; >> uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address; >> uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address; >> >> -- >> 2.17.1 >>
On Wed, Dec 20, 2023 at 10:33:17PM +0800, yuan linyu wrote: > > On 2023/12/20 00:17, Frank Li wrote: > > On Thu, Aug 03, 2023 at 05:10:49PM +0800, Linyu Yuan wrote: > >> when call uvc_function_bind(), gadget still have no connection speed, > >> just follow other gadget function, use fs endpoint descriptor to allocate > >> a video endpoint, remove gadget_is_{super|dual}speed() API call. > >> > >> Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> > >> --- > >> v2: no change > >> > >> drivers/usb/gadget/function/f_uvc.c | 10 +--------- > >> 1 file changed, 1 insertion(+), 9 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > >> index 5e919fb65833..c8e149f8315f 100644 > >> --- a/drivers/usb/gadget/function/f_uvc.c > >> +++ b/drivers/usb/gadget/function/f_uvc.c > >> @@ -719,21 +719,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) > >> } > >> uvc->enable_interrupt_ep = opts->enable_interrupt_ep; > >> > >> - if (gadget_is_superspeed(c->cdev->gadget)) > >> - ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, > >> - &uvc_ss_streaming_comp); > >> - else if (gadget_is_dualspeed(cdev->gadget)) > >> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep); > >> - else > >> - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); > >> - > >> + ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); > > Some UDC driver use gadget_check_config() and match_ep() to allocate EP > > internal fifo memory resource, if only pass download full speed EP. > Could you share the detail of problem ? do you mean find another different endpoint compared The problem is little bit complex. I try to use simple words. The background: Generally, UDC have some EP<0..15> and have some internal memory as FIFO. for example 16K. You can simple assign EP<n> to 1K memory, which can hold whole package. But for UVC, some controller required internal FIFO hold whole frame data (mult+1) * (MaxBurst +1) * wPackageSize. For most case, not every gadget use all 16 EPs. So you can assgin more memory into one EP, so it will reduce bus 'ping' package number and reduce NACK to improve transfer speed. The problem: pass fs_stream to udc driver, udc driver's check_config function will see mult and maxburst is 0. so only reserve 1K for ISO EP, but when try to enable EP, mult is 2, so there are not enough internal memory for it because more memory already assign to other EPs. Ideally, when gadget frame work can call check_config again when know usb speed, but it is not easy to fix it. Simple method use ss_stream_ep here and other function drviers. Super speed's package size is bigger than high/full speed. If resource can support super speed, it can support high/full speed. /** * gadget_is_superspeed() - return true if the hardware handles superspeed * @g: controller that might support superspeed */ @max_speed: Highest speed the driver handles And according to gadget_is_superspeed() define, it indicate if udc controller support supersped, not link speed. Orignial code is correct. If UDC support superspeed, then use ss_stream_ep. becasue superspeed is worse case compared as high and full speed. So I think original is correct. Frank. > > with change before? > > > >From my understanding, according to configfs gadget driver design, when find a endpoint, there is no > > working speed, this means each hardware endpoint should support all possible speeds. > > > > UDC will allocate too much internal memory to each EP. It may failure when > > use ss config. Generally, ss config have bigger max package size. > is there another way to solve your issue in your driver ? > > > > Frank > > > >> if (!ep) { > >> uvcg_info(f, "Unable to allocate streaming EP\n"); > >> goto error; > >> } > >> uvc->video.ep = ep; > >> > >> - uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address; > >> uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address; > >> uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address; > >> > >> -- > >> 2.17.1 > >> >
On 2023/12/21 00:02, Frank Li wrote: >>> Some UDC driver use gadget_check_config() and match_ep() to allocate EP >>> internal fifo memory resource, if only pass download full speed EP. >> Could you share the detail of problem ? do you mean find another different endpoint compared > The problem is little bit complex. I try to use simple words. > > The background: > > Generally, UDC have some EP<0..15> and have some internal memory as FIFO. > for example 16K. You can simple assign EP<n> to 1K memory, which can hold > whole package. > > But for UVC, some controller required internal FIFO hold whole frame data > > (mult+1) * (MaxBurst +1) * wPackageSize. > > For most case, not every gadget use all 16 EPs. So you can assgin more > memory into one EP, so it will reduce bus 'ping' package number and reduce > NACK to improve transfer speed. > > The problem: > pass fs_stream to udc driver, udc driver's check_config function will see > mult and maxburst is 0. so only reserve 1K for ISO EP, but when try to > enable EP, mult is 2, so there are not enough internal memory for it > because more memory already assign to other EPs. > > Ideally, when gadget frame work can call check_config again when know > usb speed, but it is not easy to fix it. > > Simple method use ss_stream_ep here and other function drviers. Super > speed's package size is bigger than high/full speed. If resource can > support super speed, it can support high/full speed. I don't find any difference of uvc_ss_streaming_ep, uvc_hs_streaming_ep, uvc_fs_streaming_ep descriptors. how difference happen in UDC ? > > > /** > * gadget_is_superspeed() - return true if the hardware handles superspeed > * @g: controller that might support superspeed > */ > > @max_speed: Highest speed the driver handles > > And according to gadget_is_superspeed() define, it indicate if udc > controller support supersped, not link speed. > > Orignial code is correct. If UDC support superspeed, then use ss_stream_ep. > > becasue superspeed is worse case compared as high and full speed. > > So I think original is correct. > > Frank. > >
On Thu, Dec 21, 2023 at 10:14:16PM +0800, yuan linyu wrote: > > On 2023/12/21 00:02, Frank Li wrote: > >>> Some UDC driver use gadget_check_config() and match_ep() to allocate EP > >>> internal fifo memory resource, if only pass download full speed EP. > >> Could you share the detail of problem ? do you mean find another different endpoint compared > > The problem is little bit complex. I try to use simple words. > > > > The background: > > > > Generally, UDC have some EP<0..15> and have some internal memory as FIFO. > > for example 16K. You can simple assign EP<n> to 1K memory, which can hold > > whole package. > > > > But for UVC, some controller required internal FIFO hold whole frame data > > > > (mult+1) * (MaxBurst +1) * wPackageSize. > > > > For most case, not every gadget use all 16 EPs. So you can assgin more > > memory into one EP, so it will reduce bus 'ping' package number and reduce > > NACK to improve transfer speed. > > > > The problem: > > pass fs_stream to udc driver, udc driver's check_config function will see > > mult and maxburst is 0. so only reserve 1K for ISO EP, but when try to > > enable EP, mult is 2, so there are not enough internal memory for it > > because more memory already assign to other EPs. > > > > Ideally, when gadget frame work can call check_config again when know > > usb speed, but it is not easy to fix it. > > > > Simple method use ss_stream_ep here and other function drviers. Super > > speed's package size is bigger than high/full speed. If resource can > > support super speed, it can support high/full speed. > > > I don't find any difference of uvc_ss_streaming_ep, uvc_hs_streaming_ep, uvc_fs_streaming_ep > > descriptors. how difference happen in UDC ? uvc_hs_streaming_ep.wMaxPacketSize = cpu_to_le16(max_packet_size | ((max_packet_mult - 1) << 11)); Hight speed will use bit [12:11] as mult uvc_ss_streaming_comp.bmAttributes = max_packet_mult - 1; uvc_ss_streaming_comp.bMaxBurst = opts->streaming_maxburst; ss will pass down uvc_ss_streaming_comp descriptor, which have bMaxBurst and mult information. Frank > > > > > > > > /** > > * gadget_is_superspeed() - return true if the hardware handles superspeed > > * @g: controller that might support superspeed > > */ > > > > @max_speed: Highest speed the driver handles > > > > And according to gadget_is_superspeed() define, it indicate if udc > > controller support supersped, not link speed. > > > > Orignial code is correct. If UDC support superspeed, then use ss_stream_ep. > > > > becasue superspeed is worse case compared as high and full speed. > > > > So I think original is correct. > > > > Frank. > > > > >
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 5e919fb65833..c8e149f8315f 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -719,21 +719,13 @@ uvc_function_bind(struct usb_configuration *c, struct usb_function *f) } uvc->enable_interrupt_ep = opts->enable_interrupt_ep; - if (gadget_is_superspeed(c->cdev->gadget)) - ep = usb_ep_autoconfig_ss(cdev->gadget, &uvc_ss_streaming_ep, - &uvc_ss_streaming_comp); - else if (gadget_is_dualspeed(cdev->gadget)) - ep = usb_ep_autoconfig(cdev->gadget, &uvc_hs_streaming_ep); - else - ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); - + ep = usb_ep_autoconfig(cdev->gadget, &uvc_fs_streaming_ep); if (!ep) { uvcg_info(f, "Unable to allocate streaming EP\n"); goto error; } uvc->video.ep = ep; - uvc_fs_streaming_ep.bEndpointAddress = uvc->video.ep->address; uvc_hs_streaming_ep.bEndpointAddress = uvc->video.ep->address; uvc_ss_streaming_ep.bEndpointAddress = uvc->video.ep->address;
when call uvc_function_bind(), gadget still have no connection speed, just follow other gadget function, use fs endpoint descriptor to allocate a video endpoint, remove gadget_is_{super|dual}speed() API call. Signed-off-by: Linyu Yuan <quic_linyyuan@quicinc.com> --- v2: no change drivers/usb/gadget/function/f_uvc.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)