* [PATCH 0/2] usb: dwc3: gadget: improve isoc handling @ 2019-11-11 15:26 Michael Olbrich 2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich 2019-11-11 15:26 ` [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late Michael Olbrich 0 siblings, 2 replies; 24+ messages in thread From: Michael Olbrich @ 2019-11-11 15:26 UTC (permalink / raw) To: linux-usb Cc: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel, Michael Olbrich These two patches improve the isoc handling and make the dwc3 gadget driver somewhat usable with the UVC gadget for isochronous endpoints. The first patch makes starting isochronous transfers more reliable. I think it's more less, what Thinh suggested some time ago[1]. It's still not perfect because the first request must still be queued within 2 seconds but it's a lot better than the current situation. The second patch makes it possible to have gaps in the data stream. The UVC gadget relies on such behaviour. Without this, using the UVC gadget with a live stream stops after the first frame, because all later frames are dropped. I'm not sure if this is the correct solution, but all other drivers currently work this way, from what I can tell. Regards, Michael [1] https://marc.info/?l=linux-usb&m=156088170723824&w=4 Michael Olbrich (2): usb: dwc3: gadget: make starting isoc transfers more robust usb: dwc3: gadget: restart the transfer if a isoc request is queued too late drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 39 +++++++++++++++++++++++++++++++-------- 2 files changed, 32 insertions(+), 8 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2019-11-11 15:26 [PATCH 0/2] usb: dwc3: gadget: improve isoc handling Michael Olbrich @ 2019-11-11 15:26 ` Michael Olbrich 2019-11-12 20:41 ` kbuild test robot 2019-11-13 3:55 ` Thinh Nguyen 2019-11-11 15:26 ` [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late Michael Olbrich 1 sibling, 2 replies; 24+ messages in thread From: Michael Olbrich @ 2019-11-11 15:26 UTC (permalink / raw) To: linux-usb Cc: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel, Michael Olbrich Currently __dwc3_gadget_start_isoc must be called very shortly after XferNotReady. Otherwise the frame number is outdated and start transfer will fail, even with several retries. DSTS provides the lower 14 bit of the frame number. Use it in combination with the frame number provided by XferNotReady to guess the current frame number. This will succeed unless more than one 14 rollover has happened since XferNotReady. Start transfer might still fail if the frame number is increased immediately after DSTS is read. So retries are still needed. Don't drop the current request if this happens. This way it is not lost and can be used immediately to try again with the next frame number. With this change, __dwc3_gadget_start_isoc is still not successfully in all cases bit it increases the acceptable delay after XferNotReady significantly. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> --- drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 3dd783b889cb..c5b223656e08 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -709,6 +709,7 @@ struct dwc3_ep { u8 type; u8 resource_index; u32 frame_number; + u32 last_frame_number; u32 interval; char name[20]; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 173f5329d3d9..ac4673d91939 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) } } -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) { struct dwc3_gadget_ep_cmd_params params; struct dwc3_request *req; @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) } ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); - if (ret < 0) { + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { /* * FIXME we need to iterate over the list of requests * here and stop, unmap, free and del each of the linked @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) return ret; } - return 0; + return ret; } static int __dwc3_gadget_get_frame(struct dwc3 *dwc) @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) dep->start_cmd_status = 0; dep->combo_num = 0; - return __dwc3_gadget_kick_transfer(dep); + return __dwc3_gadget_kick_transfer(dep, false); } static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) } for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); + /* + * last_frame_number is set from XferNotReady and may be + * already out of date. DSTS only provides the lower 14 bit + * of the current frame number. So add the upper two bits of + * last_frame_number and handle a possible rollover. + * This will provide the correct frame_number unless more than + * rollover has happened since XferNotReady. + */ + u32 frame = __dwc3_gadget_get_frame(dwc); + + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame; + if (frame < (dep->last_frame_number & 0x3fff)) + dep->frame_number += 0x4000; + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); - ret = __dwc3_gadget_kick_transfer(dep); + ret = __dwc3_gadget_kick_transfer(dep, + i + 1 < DWC3_ISOC_MAX_RETRIES); if (ret != -EAGAIN) break; } @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) } } - return __dwc3_gadget_kick_transfer(dep); + return __dwc3_gadget_kick_transfer(dep, false); } static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, if (!dwc3_gadget_ep_request_completed(req) && req->num_pending_sgs) { - __dwc3_gadget_kick_transfer(dep); + __dwc3_gadget_kick_transfer(dep, false); goto out; } @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, const struct dwc3_event_depevt *event) { dep->frame_number = event->parameters; + dep->last_frame_number = event->parameters; } static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich @ 2019-11-12 20:41 ` kbuild test robot 2019-11-13 3:55 ` Thinh Nguyen 1 sibling, 0 replies; 24+ messages in thread From: kbuild test robot @ 2019-11-12 20:41 UTC (permalink / raw) To: Michael Olbrich Cc: kbuild-all, linux-usb, Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel, Michael Olbrich [-- Attachment #1: Type: text/plain, Size: 16044 bytes --] Hi Michael, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on balbi-usb/next] [also build test WARNING on v5.4-rc7 next-20191111] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Michael-Olbrich/usb-dwc3-gadget-improve-isoc-handling/20191113-032659 base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next reproduce: make htmldocs If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> All warnings (new ones prefixed by >>): Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme. WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints' include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops' Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file drivers/dma-buf/reservation.c Error: Cannot open file include/linux/reservation.h Error: Cannot open file include/linux/reservation.h include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quotactl' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'quota_on' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'getprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'setprocattr' not described in 'security_list_options' include/linux/lsm_hooks.h:1822: warning: Function parameter or member 'locked_down' not described in 'security_list_options' lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found lib/genalloc.c:1: warning: 'gen_pool_alloc' not found lib/genalloc.c:1: warning: 'gen_pool_free' not found lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found drivers/gpio/gpiolib-of.c:92: warning: Excess function parameter 'dev' description in 'of_gpio_need_valid_mask' include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client' >> drivers/usb/dwc3/core.h:724: warning: Function parameter or member 'last_frame_number' not described in 'dwc3_ep' drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found include/linux/w1.h:277: warning: Function parameter or member 'of_match_table' not described in 'w1_family' fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode' fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode' kernel/dma/coherent.c:1: warning: no structured comments found include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry' include/linux/skbuff.h:888: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'list' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'head_frag' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encapsulation' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_valid' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'vlan_present' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'csum_level' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff' include/linux/skbuff.h:888: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff' include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common' include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common' include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock' include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock' include/net/sock.h:2450: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE' include/net/sock.h:2450: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE' include/net/sock.h:2450: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE' include/linux/netdevice.h:2053: warning: Function parameter or member 'gso_partial_features' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'name_assign_type' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'mpls_ptr' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'xdp_prog' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'qdisc_hash' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device' include/linux/netdevice.h:2053: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device' include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state' include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state' include/linux/rculist.h:374: warning: Excess function parameter 'cond' description in 'list_for_each_entry_rcu' include/linux/rculist.h:651: warning: Excess function parameter 'cond' description in 'hlist_for_each_entry_rcu' mm/util.c:1: warning: 'get_user_pages_fast' not found drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:335: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export' drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c:336: warning: Excess function parameter 'dev' description in 'amdgpu_gem_prime_export' drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor ' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor ' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:821: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1283: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags' drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2821: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:378: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch' drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: 'pp_dpm_sclk pp_dpm_mclk pp_dpm_pcie' not found drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:132: warning: Incorrect use of kernel-doc format: * @atomic_obj drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:238: warning: Incorrect use of kernel-doc format: * gpu_info FW provided soc bounding box struct or 0 if not drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:243: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager' vim +724 drivers/usb/dwc3/core.h 72246da40f3719 Felipe Balbi 2011-08-19 @724 :::::: The code at line 724 was first introduced by commit :::::: 72246da40f3719af3bfd104a2365b32537c27d83 usb: Introduce DesignWare USB3 DRD Driver :::::: TO: Felipe Balbi <balbi@ti.com> :::::: CC: Greg Kroah-Hartman <gregkh@suse.de> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 7277 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich 2019-11-12 20:41 ` kbuild test robot @ 2019-11-13 3:55 ` Thinh Nguyen 2019-11-13 8:02 ` Michael Olbrich 1 sibling, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2019-11-13 3:55 UTC (permalink / raw) To: Michael Olbrich, linux-usb Cc: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel Hi, Michael Olbrich wrote: > Currently __dwc3_gadget_start_isoc must be called very shortly after > XferNotReady. Otherwise the frame number is outdated and start transfer > will fail, even with several retries. > > DSTS provides the lower 14 bit of the frame number. Use it in combination > with the frame number provided by XferNotReady to guess the current frame > number. This will succeed unless more than one 14 rollover has happened > since XferNotReady. > > Start transfer might still fail if the frame number is increased > immediately after DSTS is read. So retries are still needed. > Don't drop the current request if this happens. This way it is not lost and > can be used immediately to try again with the next frame number. > > With this change, __dwc3_gadget_start_isoc is still not successfully in all > cases bit it increases the acceptable delay after XferNotReady > significantly. > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > --- > drivers/usb/dwc3/core.h | 1 + > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 3dd783b889cb..c5b223656e08 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -709,6 +709,7 @@ struct dwc3_ep { > u8 type; > u8 resource_index; > u32 frame_number; > + u32 last_frame_number; There's no need to add a new field for last_frame_number. Just store the value in a local variable in __dwc3_gadget_start_isoc(). > u32 interval; > > char name[20]; > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 173f5329d3d9..ac4673d91939 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) > } > } > > -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) > { > struct dwc3_gadget_ep_cmd_params params; > struct dwc3_request *req; > @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > } > > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > - if (ret < 0) { > + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { > /* > * FIXME we need to iterate over the list of requests > * here and stop, unmap, free and del each of the linked > @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > return ret; > } > > - return 0; > + return ret; > } > > static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) > dep->start_cmd_status = 0; > dep->combo_num = 0; > > - return __dwc3_gadget_kick_transfer(dep); > + return __dwc3_gadget_kick_transfer(dep, false); > } > > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > } > > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { > - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); > + /* > + * last_frame_number is set from XferNotReady and may be > + * already out of date. DSTS only provides the lower 14 bit > + * of the current frame number. So add the upper two bits of > + * last_frame_number and handle a possible rollover. > + * This will provide the correct frame_number unless more than > + * rollover has happened since XferNotReady. > + */ > + u32 frame = __dwc3_gadget_get_frame(dwc); > + > + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame; > + if (frame < (dep->last_frame_number & 0x3fff)) > + dep->frame_number += 0x4000; Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We started using 0x3fff in multiple places now, can we create a macro for that? Also, add an empty line here. > + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); > > - ret = __dwc3_gadget_kick_transfer(dep); > + ret = __dwc3_gadget_kick_transfer(dep, > + i + 1 < DWC3_ISOC_MAX_RETRIES); > if (ret != -EAGAIN) > break; > } > @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > } > } > > - return __dwc3_gadget_kick_transfer(dep); > + return __dwc3_gadget_kick_transfer(dep, false); > } > > static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, > @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > > if (!dwc3_gadget_ep_request_completed(req) && > req->num_pending_sgs) { > - __dwc3_gadget_kick_transfer(dep); > + __dwc3_gadget_kick_transfer(dep, false); > goto out; > } > > @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, > const struct dwc3_event_depevt *event) > { > dep->frame_number = event->parameters; > + dep->last_frame_number = event->parameters; > } > > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, Other than the comments I provided, this patch looks fine to me. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2019-11-13 3:55 ` Thinh Nguyen @ 2019-11-13 8:02 ` Michael Olbrich 2019-11-13 19:40 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-11-13 8:02 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi, On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote: > Michael Olbrich wrote: > > Currently __dwc3_gadget_start_isoc must be called very shortly after > > XferNotReady. Otherwise the frame number is outdated and start transfer > > will fail, even with several retries. > > > > DSTS provides the lower 14 bit of the frame number. Use it in combination > > with the frame number provided by XferNotReady to guess the current frame > > number. This will succeed unless more than one 14 rollover has happened > > since XferNotReady. > > > > Start transfer might still fail if the frame number is increased > > immediately after DSTS is read. So retries are still needed. > > Don't drop the current request if this happens. This way it is not lost and > > can be used immediately to try again with the next frame number. > > > > With this change, __dwc3_gadget_start_isoc is still not successfully in all > > cases bit it increases the acceptable delay after XferNotReady > > significantly. > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > --- > > drivers/usb/dwc3/core.h | 1 + > > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- > > 2 files changed, 24 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 3dd783b889cb..c5b223656e08 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -709,6 +709,7 @@ struct dwc3_ep { > > u8 type; > > u8 resource_index; > > u32 frame_number; > > + u32 last_frame_number; > > There's no need to add a new field for last_frame_number. Just store the > value in a local variable in __dwc3_gadget_start_isoc(). I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not help. I introduced it because I caused a second (incorrect) rollover when the first try failed. But now that I think about it, it should be possible without the extra variable. > > u32 interval; > > > > char name[20]; > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 173f5329d3d9..ac4673d91939 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) > > } > > } > > > > -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > > +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) > > { > > struct dwc3_gadget_ep_cmd_params params; > > struct dwc3_request *req; > > @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > > } > > > > ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); > > - if (ret < 0) { > > + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { > > /* > > * FIXME we need to iterate over the list of requests > > * here and stop, unmap, free and del each of the linked > > @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > > return ret; > > } > > > > - return 0; > > + return ret; > > } > > > > static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > > @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) > > dep->start_cmd_status = 0; > > dep->combo_num = 0; > > > > - return __dwc3_gadget_kick_transfer(dep); > > + return __dwc3_gadget_kick_transfer(dep, false); > > } > > > > static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > > @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) > > } > > > > for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { > > - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); > > + /* > > + * last_frame_number is set from XferNotReady and may be > > + * already out of date. DSTS only provides the lower 14 bit > > + * of the current frame number. So add the upper two bits of > > + * last_frame_number and handle a possible rollover. > > + * This will provide the correct frame_number unless more than > > + * rollover has happened since XferNotReady. > > + */ > > + u32 frame = __dwc3_gadget_get_frame(dwc); > > + > > + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame; > > + if (frame < (dep->last_frame_number & 0x3fff)) > > + dep->frame_number += 0x4000; > > Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We Ok. > started using 0x3fff in multiple places now, can we create a macro for that? Makes sense. Any preferences for the name? <something>_MASK I guess, but I don't know what the correct name for the 14 bit frame number should be. > Also, add an empty line here. ok. > > + dep->frame_number = DWC3_ALIGN_FRAME(dep, 1); > > > > - ret = __dwc3_gadget_kick_transfer(dep); > > + ret = __dwc3_gadget_kick_transfer(dep, > > + i + 1 < DWC3_ISOC_MAX_RETRIES); > > if (ret != -EAGAIN) > > break; > > } > > @@ -1461,7 +1475,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > > } > > } > > > > - return __dwc3_gadget_kick_transfer(dep); > > + return __dwc3_gadget_kick_transfer(dep, false); > > } > > > > static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request, > > @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > > > > if (!dwc3_gadget_ep_request_completed(req) && > > req->num_pending_sgs) { > > - __dwc3_gadget_kick_transfer(dep); > > + __dwc3_gadget_kick_transfer(dep, false); > > goto out; > > } > > > > @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, > > const struct dwc3_event_depevt *event) > > { > > dep->frame_number = event->parameters; > > + dep->last_frame_number = event->parameters; > > } > > > > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, > > Other than the comments I provided, this patch looks fine to me. Great. Thanks for the review. Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust 2019-11-13 8:02 ` Michael Olbrich @ 2019-11-13 19:40 ` Thinh Nguyen 0 siblings, 0 replies; 24+ messages in thread From: Thinh Nguyen @ 2019-11-13 19:40 UTC (permalink / raw) To: Thinh Nguyen, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi, Michael Olbrich wrote: > Hi, > > On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote: >> Michael Olbrich wrote: >>> Currently __dwc3_gadget_start_isoc must be called very shortly after >>> XferNotReady. Otherwise the frame number is outdated and start transfer >>> will fail, even with several retries. >>> >>> DSTS provides the lower 14 bit of the frame number. Use it in combination >>> with the frame number provided by XferNotReady to guess the current frame >>> number. This will succeed unless more than one 14 rollover has happened >>> since XferNotReady. >>> >>> Start transfer might still fail if the frame number is increased >>> immediately after DSTS is read. So retries are still needed. >>> Don't drop the current request if this happens. This way it is not lost and >>> can be used immediately to try again with the next frame number. >>> >>> With this change, __dwc3_gadget_start_isoc is still not successfully in all >>> cases bit it increases the acceptable delay after XferNotReady >>> significantly. >>> >>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> >>> --- >>> drivers/usb/dwc3/core.h | 1 + >>> drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- >>> 2 files changed, 24 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>> index 3dd783b889cb..c5b223656e08 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >>> @@ -709,6 +709,7 @@ struct dwc3_ep { >>> u8 type; >>> u8 resource_index; >>> u32 frame_number; >>> + u32 last_frame_number; >> There's no need to add a new field for last_frame_number. Just store the >> value in a local variable in __dwc3_gadget_start_isoc(). > I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not > help. I introduced it because I caused a second (incorrect) rollover when > the first try failed. But now that I think about it, it should be possible > without the extra variable. We don't need this extra field for dwc3_ep. If you need to add a new one, then you need to also describe that field in the struct dwc3_ep. > >>> u32 interval; >>> >>> char name[20]; >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index 173f5329d3d9..ac4673d91939 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >>> } >>> } >>> >>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req) >>> { >>> struct dwc3_gadget_ep_cmd_params params; >>> struct dwc3_request *req; >>> @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>> } >>> >>> ret = dwc3_send_gadget_ep_cmd(dep, cmd, ¶ms); >>> - if (ret < 0) { >>> + if (ret < 0 && (!keep_req || ret != -EAGAIN)) { >>> /* >>> * FIXME we need to iterate over the list of requests >>> * here and stop, unmap, free and del each of the linked >>> @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) >>> return ret; >>> } >>> >>> - return 0; >>> + return ret; >>> } >>> >>> static int __dwc3_gadget_get_frame(struct dwc3 *dwc) >>> @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep) >>> dep->start_cmd_status = 0; >>> dep->combo_num = 0; >>> >>> - return __dwc3_gadget_kick_transfer(dep); >>> + return __dwc3_gadget_kick_transfer(dep, false); >>> } >>> >>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>> @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep) >>> } >>> >>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) { >>> - dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1); >>> + /* >>> + * last_frame_number is set from XferNotReady and may be >>> + * already out of date. DSTS only provides the lower 14 bit >>> + * of the current frame number. So add the upper two bits of >>> + * last_frame_number and handle a possible rollover. >>> + * This will provide the correct frame_number unless more than >>> + * rollover has happened since XferNotReady. >>> + */ >>> + u32 frame = __dwc3_gadget_get_frame(dwc); >>> + >>> + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame; >>> + if (frame < (dep->last_frame_number & 0x3fff)) >>> + dep->frame_number += 0x4000; >> Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We > Ok. > >> started using 0x3fff in multiple places now, can we create a macro for that? > Makes sense. Any preferences for the name? <something>_MASK I guess, but I > don't know what the correct name for the 14 bit frame number should be. Maybe you can use DWC3_DSTS_SOFFN(~0) as a mask. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-11 15:26 [PATCH 0/2] usb: dwc3: gadget: improve isoc handling Michael Olbrich 2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich @ 2019-11-11 15:26 ` Michael Olbrich 2019-11-13 3:55 ` Thinh Nguyen 1 sibling, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-11-11 15:26 UTC (permalink / raw) To: linux-usb Cc: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel, Michael Olbrich Currently, most gadget drivers handle isoc transfers on a best effort bases: If the request queue runs empty, then there will simply be gaps in the isoc data stream. The UVC gadget depends on this behaviour. It simply provides new requests when video frames are available and assumes that they are sent as soon as possible. The dwc3 gadget currently works differently: It assumes that there is a contiguous stream of requests without any gaps. If a request is too late, then it is dropped by the hardware. For the UVC gadget this means that a live stream stops after the first frame because all following requests are late. This patch changes the behaviour of the dwc3 gadget driver to match the expectations. If a request arrives too late, then the transfer will be restart to create the needed gap. A request is late if: 1. There are currently no requests queued in the hardware 2. The current frame number provided by DSTS does not match the frame number returned by the last transfer. If this happens transfers are stopped. The following XferNotReady provides the new correct frame number and restarts the transfer. Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> --- drivers/usb/dwc3/gadget.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index ac4673d91939..eb7f09929f28 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -1473,6 +1473,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) return __dwc3_gadget_start_isoc(dep); } } + if ((dep->flags & DWC3_EP_TRANSFER_STARTED) && + list_empty(&dep->started_list) && + ((dep->frame_number & 0x3fff) != + __dwc3_gadget_get_frame(dwc))) { + dwc3_stop_active_transfer(dep, true, true); + dep->flags = DWC3_EP_ENABLED; + return 0; + } } return __dwc3_gadget_kick_transfer(dep, false); -- 2.20.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-11 15:26 ` [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late Michael Olbrich @ 2019-11-13 3:55 ` Thinh Nguyen 2019-11-13 7:53 ` Michael Olbrich 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2019-11-13 3:55 UTC (permalink / raw) To: Michael Olbrich, linux-usb Cc: Felipe Balbi, Thinh Nguyen, Greg Kroah-Hartman, linux-kernel, kernel Hi Michael, Michael Olbrich wrote: > Currently, most gadget drivers handle isoc transfers on a best effort > bases: If the request queue runs empty, then there will simply be gaps in > the isoc data stream. > > The UVC gadget depends on this behaviour. It simply provides new requests > when video frames are available and assumes that they are sent as soon as > possible. > > The dwc3 gadget currently works differently: It assumes that there is a > contiguous stream of requests without any gaps. If a request is too late, > then it is dropped by the hardware. > For the UVC gadget this means that a live stream stops after the first > frame because all following requests are late. Can you explain little more how UVC gadget fails? dwc3 controller expects a steady stream of data otherwise it will result in missed_isoc status, and it should be fine as long as new requests are queued. The controller doesn't just drop the request unless there's some other failure. > This patch changes the behaviour of the dwc3 gadget driver to match the > expectations. If a request arrives too late, then the transfer will be > restart to create the needed gap. > > A request is late if: > 1. There are currently no requests queued in the hardware > 2. The current frame number provided by DSTS does not match the frame > number returned by the last transfer. > > If this happens transfers are stopped. The following XferNotReady provides > the new correct frame number and restarts the transfer. > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > --- > drivers/usb/dwc3/gadget.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index ac4673d91939..eb7f09929f28 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1473,6 +1473,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > return __dwc3_gadget_start_isoc(dep); > } > } > + if ((dep->flags & DWC3_EP_TRANSFER_STARTED) && > + list_empty(&dep->started_list) && > + ((dep->frame_number & 0x3fff) != > + __dwc3_gadget_get_frame(dwc))) { > + dwc3_stop_active_transfer(dep, true, true); > + dep->flags = DWC3_EP_ENABLED; > + return 0; > + } > } > > return __dwc3_gadget_kick_transfer(dep, false); BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-13 3:55 ` Thinh Nguyen @ 2019-11-13 7:53 ` Michael Olbrich 2019-11-13 15:39 ` Alan Stern 0 siblings, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-11-13 7:53 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: > Michael Olbrich wrote: > > Currently, most gadget drivers handle isoc transfers on a best effort > > bases: If the request queue runs empty, then there will simply be gaps in > > the isoc data stream. > > > > The UVC gadget depends on this behaviour. It simply provides new requests > > when video frames are available and assumes that they are sent as soon as > > possible. > > > > The dwc3 gadget currently works differently: It assumes that there is a > > contiguous stream of requests without any gaps. If a request is too late, > > then it is dropped by the hardware. > > For the UVC gadget this means that a live stream stops after the first > > frame because all following requests are late. > > Can you explain little more how UVC gadget fails? > dwc3 controller expects a steady stream of data otherwise it will result > in missed_isoc status, and it should be fine as long as new requests are > queued. The controller doesn't just drop the request unless there's some > other failure. UVC (with a live stream) does not fill the complete bandwidth of an isochronous endpoint. Let's assume for the example that one video frame fills 3 requests. Because it is a live stream, there will be a gap between video frames. This is unavoidable, especially for compressed video. So the UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 15 and so on. The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 10 11 12. So except for the fist few requests, all are late and result in a missed_isoc. I tried to just ignore the missed_isoc but that did not work for me. I only received the first frame at the other end. Maybe I missing something here, i don't have access to the hardware documentation, so I can only guess from the existing driver. Regards, Michael > > This patch changes the behaviour of the dwc3 gadget driver to match the > > expectations. If a request arrives too late, then the transfer will be > > restart to create the needed gap. > > > > A request is late if: > > 1. There are currently no requests queued in the hardware > > 2. The current frame number provided by DSTS does not match the frame > > number returned by the last transfer. > > > > If this happens transfers are stopped. The following XferNotReady provides > > the new correct frame number and restarts the transfer. > > > > Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de> > > --- > > drivers/usb/dwc3/gadget.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index ac4673d91939..eb7f09929f28 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -1473,6 +1473,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) > > return __dwc3_gadget_start_isoc(dep); > > } > > } > > + if ((dep->flags & DWC3_EP_TRANSFER_STARTED) && > > + list_empty(&dep->started_list) && > > + ((dep->frame_number & 0x3fff) != > > + __dwc3_gadget_get_frame(dwc))) { > > + dwc3_stop_active_transfer(dep, true, true); > > + dep->flags = DWC3_EP_ENABLED; > > + return 0; > > + } > > } > > > > return __dwc3_gadget_kick_transfer(dep, false); > > BR, > Thinh > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-13 7:53 ` Michael Olbrich @ 2019-11-13 15:39 ` Alan Stern 2019-11-13 19:14 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Alan Stern @ 2019-11-13 15:39 UTC (permalink / raw) To: Michael Olbrich Cc: Thinh Nguyen, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel On Wed, 13 Nov 2019, Michael Olbrich wrote: > On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: > > Michael Olbrich wrote: > > > Currently, most gadget drivers handle isoc transfers on a best effort > > > bases: If the request queue runs empty, then there will simply be gaps in > > > the isoc data stream. > > > > > > The UVC gadget depends on this behaviour. It simply provides new requests > > > when video frames are available and assumes that they are sent as soon as > > > possible. > > > > > > The dwc3 gadget currently works differently: It assumes that there is a > > > contiguous stream of requests without any gaps. If a request is too late, > > > then it is dropped by the hardware. > > > For the UVC gadget this means that a live stream stops after the first > > > frame because all following requests are late. > > > > Can you explain little more how UVC gadget fails? > > dwc3 controller expects a steady stream of data otherwise it will result > > in missed_isoc status, and it should be fine as long as new requests are > > queued. The controller doesn't just drop the request unless there's some > > other failure. > > UVC (with a live stream) does not fill the complete bandwidth of an > isochronous endpoint. Let's assume for the example that one video frame > fills 3 requests. Because it is a live stream, there will be a gap between > video frames. This is unavoidable, especially for compressed video. So the > UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 > 15 and so on. > The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 > 10 11 12. So except for the fist few requests, all are late and result in a > missed_isoc. I tried to just ignore the missed_isoc but that did not work > for me. I only received the first frame at the other end. > Maybe I missing something here, i don't have access to the hardware > documentation, so I can only guess from the existing driver. How about changing the gadget driver instead? For frames where the UVC gadget knows no video frame data is available (numbers 4, 8, 12, and so on in the example above), queue a zero-length request. Then there won't be any gaps in the isochronous packet stream. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-13 15:39 ` Alan Stern @ 2019-11-13 19:14 ` Thinh Nguyen 2019-11-14 12:14 ` Michael Olbrich 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2019-11-13 19:14 UTC (permalink / raw) To: Alan Stern, Michael Olbrich Cc: Thinh Nguyen, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi Michael, Alan Stern wrote: > On Wed, 13 Nov 2019, Michael Olbrich wrote: > >> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>> Michael Olbrich wrote: >>>> Currently, most gadget drivers handle isoc transfers on a best effort >>>> bases: If the request queue runs empty, then there will simply be gaps in >>>> the isoc data stream. >>>> >>>> The UVC gadget depends on this behaviour. It simply provides new requests >>>> when video frames are available and assumes that they are sent as soon as >>>> possible. >>>> >>>> The dwc3 gadget currently works differently: It assumes that there is a >>>> contiguous stream of requests without any gaps. If a request is too late, >>>> then it is dropped by the hardware. >>>> For the UVC gadget this means that a live stream stops after the first >>>> frame because all following requests are late. >>> Can you explain little more how UVC gadget fails? >>> dwc3 controller expects a steady stream of data otherwise it will result >>> in missed_isoc status, and it should be fine as long as new requests are >>> queued. The controller doesn't just drop the request unless there's some >>> other failure. >> UVC (with a live stream) does not fill the complete bandwidth of an >> isochronous endpoint. Let's assume for the example that one video frame >> fills 3 requests. Because it is a live stream, there will be a gap between >> video frames. This is unavoidable, especially for compressed video. So the >> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 >> 15 and so on. >> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 >> 10 11 12. So except for the fist few requests, all are late and result in a >> missed_isoc. I tried to just ignore the missed_isoc but that did not work >> for me. I only received the first frame at the other end. >> Maybe I missing something here, i don't have access to the hardware >> documentation, so I can only guess from the existing driver. The reason I asked is because your patch doesn't seem to address the actual issue. For the 2 checks you do here 1. There are currently no requests queued in the hardware 2. The current frame number provided by DSTS does not match the frame number returned by the last transfer. For #1, it's already done in the dwc3 driver. (check dwc3_gadget_endpoint_transfer_in_progress()) For #2, it's unlikely that DSTS current frame number will match with the XferNotReady's frame number. So this check doesn't mean much. So I'm still wondering how does this patch help resolve your issue. > How about changing the gadget driver instead? For frames where the UVC > gadget knows no video frame data is available (numbers 4, 8, 12, and so > on in the example above), queue a zero-length request. Then there > won't be any gaps in the isochronous packet stream. > > Alan Stern What Alan suggests may work. Have you tried this? BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-13 19:14 ` Thinh Nguyen @ 2019-11-14 12:14 ` Michael Olbrich 2019-11-14 20:11 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-11-14 12:14 UTC (permalink / raw) To: Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: > Alan Stern wrote: > > On Wed, 13 Nov 2019, Michael Olbrich wrote: > >> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: > >>> Michael Olbrich wrote: > >>>> Currently, most gadget drivers handle isoc transfers on a best effort > >>>> bases: If the request queue runs empty, then there will simply be gaps in > >>>> the isoc data stream. > >>>> > >>>> The UVC gadget depends on this behaviour. It simply provides new requests > >>>> when video frames are available and assumes that they are sent as soon as > >>>> possible. > >>>> > >>>> The dwc3 gadget currently works differently: It assumes that there is a > >>>> contiguous stream of requests without any gaps. If a request is too late, > >>>> then it is dropped by the hardware. > >>>> For the UVC gadget this means that a live stream stops after the first > >>>> frame because all following requests are late. > >>> Can you explain little more how UVC gadget fails? > >>> dwc3 controller expects a steady stream of data otherwise it will result > >>> in missed_isoc status, and it should be fine as long as new requests are > >>> queued. The controller doesn't just drop the request unless there's some > >>> other failure. > >> UVC (with a live stream) does not fill the complete bandwidth of an > >> isochronous endpoint. Let's assume for the example that one video frame > >> fills 3 requests. Because it is a live stream, there will be a gap between > >> video frames. This is unavoidable, especially for compressed video. So the > >> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 > >> 15 and so on. > >> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 > >> 10 11 12. So except for the fist few requests, all are late and result in a > >> missed_isoc. I tried to just ignore the missed_isoc but that did not work > >> for me. I only received the first frame at the other end. > >> Maybe I missing something here, i don't have access to the hardware > >> documentation, so I can only guess from the existing driver. > > The reason I asked is because your patch doesn't seem to address the > actual issue. > > For the 2 checks you do here > 1. There are currently no requests queued in the hardware > 2. The current frame number provided by DSTS does not match the frame > number returned by the last transfer. > > For #1, it's already done in the dwc3 driver. (check > dwc3_gadget_endpoint_transfer_in_progress()) But that's only after a isoc_missed occurred. What exactly does that mean? Was the request transferred or not? My tests suggest that it was not transferred, so I wanted to catch this before it happens. > For #2, it's unlikely that DSTS current frame number will match with the > XferNotReady's frame number. So this check doesn't mean much. The frame number is also updated for each "Transfer In Progress" interrupt. If they match, then there a new request can still be queued successfully. Without this I got unnecessary stop/start transfers in the middle of a video frame. But maybe something else was wrong here. I'd need to recheck. > So I'm still wondering how does this patch help resolve your issue. With this patch, the transfer is restarted for every video frame. Otherwise, I just get a lot of isoc_missed and ignoring those did not help. No valid data arrived after the first video frame. > > How about changing the gadget driver instead? For frames where the UVC > > gadget knows no video frame data is available (numbers 4, 8, 12, and so > > on in the example above), queue a zero-length request. Then there > > won't be any gaps in the isochronous packet stream. > > What Alan suggests may work. Have you tried this? Yes and it works in general. There are however some problems with that approach that I want to avoid: 1. It adds extra overhead to handle the extra zero-length request. Especially for encoded video the available bandwidth can be quite a bit larger that what is actually used. I want to avoid that. 2. The UVC gadget currently does no know how many zero-length request must added. So it needs fill all available request until a new video frame arrives. With the current 4 requests that is not a problem right now. But that does not scale for USB3 bandwidths. So one thing that I want to do is to queue many requests but only enable the interrupt for a few of than. From what I can tell from the code, the gadget framework and the dwc3 driver should already support this. This will result in extra latency. There is probably an acceptable trade-off with an acceptable interrupt load and latency. But I would like to avoid that if possible. Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-14 12:14 ` Michael Olbrich @ 2019-11-14 20:11 ` Thinh Nguyen 2019-11-15 21:06 ` Alan Stern 2020-04-09 7:59 ` Michael Grzeschik 0 siblings, 2 replies; 24+ messages in thread From: Thinh Nguyen @ 2019-11-14 20:11 UTC (permalink / raw) To: Thinh Nguyen, Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Michael Olbrich wrote: > On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >> Alan Stern wrote: >>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>> Michael Olbrich wrote: >>>>>> Currently, most gadget drivers handle isoc transfers on a best effort >>>>>> bases: If the request queue runs empty, then there will simply be gaps in >>>>>> the isoc data stream. >>>>>> >>>>>> The UVC gadget depends on this behaviour. It simply provides new requests >>>>>> when video frames are available and assumes that they are sent as soon as >>>>>> possible. >>>>>> >>>>>> The dwc3 gadget currently works differently: It assumes that there is a >>>>>> contiguous stream of requests without any gaps. If a request is too late, >>>>>> then it is dropped by the hardware. >>>>>> For the UVC gadget this means that a live stream stops after the first >>>>>> frame because all following requests are late. >>>>> Can you explain little more how UVC gadget fails? >>>>> dwc3 controller expects a steady stream of data otherwise it will result >>>>> in missed_isoc status, and it should be fine as long as new requests are >>>>> queued. The controller doesn't just drop the request unless there's some >>>>> other failure. >>>> UVC (with a live stream) does not fill the complete bandwidth of an >>>> isochronous endpoint. Let's assume for the example that one video frame >>>> fills 3 requests. Because it is a live stream, there will be a gap between >>>> video frames. This is unavoidable, especially for compressed video. So the >>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 >>>> 15 and so on. >>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 >>>> 10 11 12. So except for the fist few requests, all are late and result in a >>>> missed_isoc. I tried to just ignore the missed_isoc but that did not work >>>> for me. I only received the first frame at the other end. >>>> Maybe I missing something here, i don't have access to the hardware >>>> documentation, so I can only guess from the existing driver. >> The reason I asked is because your patch doesn't seem to address the >> actual issue. >> >> For the 2 checks you do here >> 1. There are currently no requests queued in the hardware >> 2. The current frame number provided by DSTS does not match the frame >> number returned by the last transfer. >> >> For #1, it's already done in the dwc3 driver. (check >> dwc3_gadget_endpoint_transfer_in_progress()) > But that's only after a isoc_missed occurred. What exactly does that mean? > Was the request transferred or not? My tests suggest that it was not > transferred, so I wanted to catch this before it happens. Missed_isoc status means that the controller did not move all the data in an interval. > >> For #2, it's unlikely that DSTS current frame number will match with the >> XferNotReady's frame number. So this check doesn't mean much. > The frame number is also updated for each "Transfer In Progress" interrupt. > If they match, then there a new request can still be queued successfully. > Without this I got unnecessary stop/start transfers in the middle of a > video frame. But maybe something else was wrong here. I'd need to recheck. The reason they may not match is 1) the frame_number is only updated after the software handles the XferInProgress interrupt. Depends on system latency, that value may not be updated at the time that we check the frame_number. 2) This check doesn't work if the service interval is greater than 1 uframe. That is, it doesn't have to match exactly the time to be consider not late. Though, the second reason can easily be fixed. > >> So I'm still wondering how does this patch help resolve your issue. > With this patch, the transfer is restarted for every video frame. > Otherwise, I just get a lot of isoc_missed and ignoring those did not help. > No valid data arrived after the first video frame. > >>> How about changing the gadget driver instead? For frames where the UVC >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so >>> on in the example above), queue a zero-length request. Then there >>> won't be any gaps in the isochronous packet stream. >> What Alan suggests may work. Have you tried this? > Yes and it works in general. There are however some problems with that > approach that I want to avoid: > > 1. It adds extra overhead to handle the extra zero-length request. > Especially for encoded video the available bandwidth can be quite a bit > larger that what is actually used. I want to avoid that. > > 2. The UVC gadget currently does no know how many zero-length request must > added. So it needs fill all available request until a new video frame > arrives. With the current 4 requests that is not a problem right now. But > that does not scale for USB3 bandwidths. So one thing that I want to do is > to queue many requests but only enable the interrupt for a few of than. > From what I can tell from the code, the gadget framework and the dwc3 > driver should already support this. > This will result in extra latency. There is probably an acceptable > trade-off with an acceptable interrupt load and latency. But I would like > to avoid that if possible. > > Regards, > Michael I think I understand the problem you're trying to solve now. The dwc3 driver does not know that there's a gap until after a new request was queued, which then it will send an END_TRANSFER command and dequeue all the requests to restart the transfer due to missed_isoc. We do this because the dwc3 driver does not know whether the new request is actually stale data, and we should not change this behavior. Now, with UVC, it needs to communicate to the dwc3 driver that there will be a gap after a certain request (and that the device is expecting to send 0-length data). This is not a normal operation for isoc transfer. You may need to introduce a new way for the function driver to do that, possibly a new field in usb_request structure to indicate that. However, this seems a little awkward. Maybe others can comment on this. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-14 20:11 ` Thinh Nguyen @ 2019-11-15 21:06 ` Alan Stern 2019-11-28 11:36 ` Michael Olbrich 2020-04-09 7:59 ` Michael Grzeschik 1 sibling, 1 reply; 24+ messages in thread From: Alan Stern @ 2019-11-15 21:06 UTC (permalink / raw) To: Thinh Nguyen Cc: linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel On Thu, 14 Nov 2019, Thinh Nguyen wrote: > Michael Olbrich wrote: > >>> How about changing the gadget driver instead? For frames where the UVC > >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so > >>> on in the example above), queue a zero-length request. Then there > >>> won't be any gaps in the isochronous packet stream. > >> What Alan suggests may work. Have you tried this? > > Yes and it works in general. There are however some problems with that > > approach that I want to avoid: > > > > 1. It adds extra overhead to handle the extra zero-length request. > > Especially for encoded video the available bandwidth can be quite a bit > > larger that what is actually used. I want to avoid that. This comment doesn't seem to make sense. If the available bandwidth is much _larger_ than what is actually used, what's the problem? You don't run into difficulties until the available bandwidth is too _small_. The extra overhead of a zero-length request should be pretty small. After all, the gadget expects to send a packet for every frame anyway, more or less. > > 2. The UVC gadget currently does no know how many zero-length request must > > added. So it needs fill all available request until a new video frame > > arrives. With the current 4 requests that is not a problem right now. But > > that does not scale for USB3 bandwidths. So one thing that I want to do is > > to queue many requests but only enable the interrupt for a few of than. > > From what I can tell from the code, the gadget framework and the dwc3 > > driver should already support this. > > This will result in extra latency. There is probably an acceptable > > trade-off with an acceptable interrupt load and latency. But I would like > > to avoid that if possible. There are two different situations to consider: In the middle of a video stream, latency isn't an issue. The gadget should expect to send a new packet for each frame, and it doesn't know what to put in that packet until it receives the video data or it knows there won't be any data. At the start of a video stream, latency can be an issue. But in this situation the gadget doesn't have to send 0-length requests until there actually is some data available. Either way, it should be okay. As far as interrupt load is concerned, I don't see how it relates to the issue of sending 0-length requests. > I think I understand the problem you're trying to solve now. > > The dwc3 driver does not know that there's a gap until after a new > request was queued, which then it will send an END_TRANSFER command and > dequeue all the requests to restart the transfer due to missed_isoc. > We do this because the dwc3 driver does not know whether the new request > is actually stale data, and we should not change this behavior. > > Now, with UVC, it needs to communicate to the dwc3 driver that there > will be a gap after a certain request (and that the device is expecting > to send 0-length data). This is not a normal operation for isoc > transfer. You may need to introduce a new way for the function driver to > do that, possibly a new field in usb_request structure to indicate that. > However, this seems a little awkward. Maybe others can comment on this. Note that on the host side, there is a difference between receiving a 0-length packet and receiving no packet at all. As long as both the host and the gadget expect the isochronous stream to be running, there shouldn't be any gaps if you can avoid it. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-15 21:06 ` Alan Stern @ 2019-11-28 11:36 ` Michael Olbrich 2019-11-28 17:59 ` Alan Stern 0 siblings, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-11-28 11:36 UTC (permalink / raw) To: Alan Stern Cc: Thinh Nguyen, Felipe Balbi, linux-usb, linux-kernel, kernel, Greg Kroah-Hartman On Fri, Nov 15, 2019 at 04:06:10PM -0500, Alan Stern wrote: > On Thu, 14 Nov 2019, Thinh Nguyen wrote: > > > Michael Olbrich wrote: > > > >>> How about changing the gadget driver instead? For frames where the UVC > > >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so > > >>> on in the example above), queue a zero-length request. Then there > > >>> won't be any gaps in the isochronous packet stream. > > >> What Alan suggests may work. Have you tried this? > > > Yes and it works in general. There are however some problems with that > > > approach that I want to avoid: > > > > > > 1. It adds extra overhead to handle the extra zero-length request. > > > Especially for encoded video the available bandwidth can be quite a bit > > > larger that what is actually used. I want to avoid that. > > This comment doesn't seem to make sense. If the available bandwidth is > much _larger_ than what is actually used, what's the problem? You > don't run into difficulties until the available bandwidth is too > _small_. > > The extra overhead of a zero-length request should be pretty small. > After all, the gadget expects to send a packet for every frame anyway, > more or less. My current test-case is video frames with 450kB on average at 30fps. This currently results in ~10 CPU load for the threaded interrupt handler. At least in my test, filling the actual video data into the frame has very little impact. So if I reserve 900kB to support occasionally larger video frames, then I expect that this CPU load will almost double in all cases, not just when the video frames are larger. > > > 2. The UVC gadget currently does no know how many zero-length request must > > > added. So it needs fill all available request until a new video frame > > > arrives. With the current 4 requests that is not a problem right now. But > > > that does not scale for USB3 bandwidths. So one thing that I want to do is > > > to queue many requests but only enable the interrupt for a few of than. > > > From what I can tell from the code, the gadget framework and the dwc3 > > > driver should already support this. > > > This will result in extra latency. There is probably an acceptable > > > trade-off with an acceptable interrupt load and latency. But I would like > > > to avoid that if possible. > > There are two different situations to consider: > > In the middle of a video stream, latency isn't an issue. > The gadget should expect to send a new packet for each frame, > and it doesn't know what to put in that packet until it > receives the video data or it knows there won't be any data. > > At the start of a video stream, latency can be an issue. But > in this situation the gadget doesn't have to send 0-length > requests until there actually is some data available. > > Either way, it should be okay. > > As far as interrupt load is concerned, I don't see how it relates to > the issue of sending 0-length requests. Maybe I don't understand, how 0-length requests work. My current understanding is, that they are queued like any other request. If I want to reduce the number of interrupts then I need to queue more requests and only ask for an interrupt for some of them. This means that potentially a lot of 0-length requests requests are queued when a new video frame arrives and this means extra latency for the frame. I think the worst-case latency is 2x the time between two interrupts. So less interrupts mean more latency. The stop/start transfer this patch implements, the video frame can be sent immediately without any extra latency. > > I think I understand the problem you're trying to solve now. > > > > The dwc3 driver does not know that there's a gap until after a new > > request was queued, which then it will send an END_TRANSFER command and > > dequeue all the requests to restart the transfer due to missed_isoc. > > We do this because the dwc3 driver does not know whether the new request > > is actually stale data, and we should not change this behavior. > > > > Now, with UVC, it needs to communicate to the dwc3 driver that there > > will be a gap after a certain request (and that the device is expecting > > to send 0-length data). This is not a normal operation for isoc > > transfer. You may need to introduce a new way for the function driver to > > do that, possibly a new field in usb_request structure to indicate that. > > However, this seems a little awkward. Maybe others can comment on this. I'm not sure how this is supposed to work. What exactly can the dwc3 driver / hardware do to handle a gap? > Note that on the host side, there is a difference between receiving > a 0-length packet and receiving no packet at all. As long as both the > host and the gadget expect the isochronous stream to be running, there > shouldn't be any gaps if you can avoid it. Huh, so how is this handled on other hardware? From what I can tell the UVC gadget works with other drivers and I've not found any special handling for this. Is there no packet sent or are 0-length packet generated implicitly somewhere? Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-28 11:36 ` Michael Olbrich @ 2019-11-28 17:59 ` Alan Stern 2019-12-02 15:41 ` Michael Olbrich 0 siblings, 1 reply; 24+ messages in thread From: Alan Stern @ 2019-11-28 17:59 UTC (permalink / raw) To: Michael Olbrich Cc: Thinh Nguyen, Felipe Balbi, linux-usb, linux-kernel, kernel, Greg Kroah-Hartman On Thu, 28 Nov 2019, Michael Olbrich wrote: > On Fri, Nov 15, 2019 at 04:06:10PM -0500, Alan Stern wrote: > > On Thu, 14 Nov 2019, Thinh Nguyen wrote: > > > > > Michael Olbrich wrote: > > > > > >>> How about changing the gadget driver instead? For frames where the UVC > > > >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so > > > >>> on in the example above), queue a zero-length request. Then there > > > >>> won't be any gaps in the isochronous packet stream. > > > >> What Alan suggests may work. Have you tried this? > > > > Yes and it works in general. There are however some problems with that > > > > approach that I want to avoid: > > > > > > > > 1. It adds extra overhead to handle the extra zero-length request. > > > > Especially for encoded video the available bandwidth can be quite a bit > > > > larger that what is actually used. I want to avoid that. > > > > This comment doesn't seem to make sense. If the available bandwidth is > > much _larger_ than what is actually used, what's the problem? You > > don't run into difficulties until the available bandwidth is too > > _small_. > > > > The extra overhead of a zero-length request should be pretty small. > > After all, the gadget expects to send a packet for every frame anyway, > > more or less. > > My current test-case is video frames with 450kB on average at 30fps. This > currently results in ~10 CPU load for the threaded interrupt handler. > At least in my test, filling the actual video data into the frame has very > little impact. So if I reserve 900kB to support occasionally larger video > frames, then I expect that this CPU load will almost double in all cases, > not just when the video frames are larger. This is the sort of thing you need to confirm by experimenting. It is not at all clear that doubling the interrupt rate will also double the CPU load, especially if half of the interrupts don't require the CPU to do much work. > > > > 2. The UVC gadget currently does no know how many zero-length request must > > > > added. So it needs fill all available request until a new video frame > > > > arrives. With the current 4 requests that is not a problem right now. But > > > > that does not scale for USB3 bandwidths. So one thing that I want to do is > > > > to queue many requests but only enable the interrupt for a few of than. > > > > From what I can tell from the code, the gadget framework and the dwc3 > > > > driver should already support this. > > > > This will result in extra latency. There is probably an acceptable > > > > trade-off with an acceptable interrupt load and latency. But I would like > > > > to avoid that if possible. > > > > There are two different situations to consider: > > > > In the middle of a video stream, latency isn't an issue. > > The gadget should expect to send a new packet for each frame, > > and it doesn't know what to put in that packet until it > > receives the video data or it knows there won't be any data. > > > > At the start of a video stream, latency can be an issue. But > > in this situation the gadget doesn't have to send 0-length > > requests until there actually is some data available. > > > > Either way, it should be okay. > > > > As far as interrupt load is concerned, I don't see how it relates to > > the issue of sending 0-length requests. > > Maybe I don't understand, how 0-length requests work. My current > understanding is, that they are queued like any other request. That's right. > If I want to reduce the number of interrupts then I need to queue more > requests and only ask for an interrupt for some of them. This means that > potentially a lot of 0-length requests requests are queued when a new video > frame arrives and this means extra latency for the frame. Let's say you ask for an interrupt for only even-numbered requests. Then there would be at most one 0-length request queued when a new video frame arrives. Processing that 0-length request should require very little CPU time (almost none) because it contains no data, so the extra latency would be negligible. > I think the worst-case latency is 2x the time between two interrupts. > So less interrupts mean more latency. What matters is the interrupt rate. If you double the rate at which transfers are queued but ask for an interrupt on only half of them, then the overall interrupt rate will remain the same and so will the average latency. > The stop/start transfer this patch implements, the video frame can be sent > immediately without any extra latency. The same would be true if you queued a request at the full isochronous rate. If video data is present, put it in the request; if not then queue a 0-length request. > > > Now, with UVC, it needs to communicate to the dwc3 driver that there > > > will be a gap after a certain request (and that the device is expecting > > > to send 0-length data). This is not a normal operation for isoc > > > transfer. You may need to introduce a new way for the function driver to > > > do that, possibly a new field in usb_request structure to indicate that. > > > However, this seems a little awkward. Maybe others can comment on this. > > I'm not sure how this is supposed to work. What exactly can the dwc3 driver > / hardware do to handle a gap? Are you talking about the driver on the gadget side or on the host side? The rules for the host-side driver are spelled out in the kerneldoc for usb_submit_urb(). As far as I know, there is no equivalent set of rules for the gadget-side drivers. > > Note that on the host side, there is a difference between receiving > > a 0-length packet and receiving no packet at all. As long as both the > > host and the gadget expect the isochronous stream to be running, there > > shouldn't be any gaps if you can avoid it. > > Huh, so how is this handled on other hardware? From what I can tell the UVC > gadget works with other drivers and I've not found any special handling for > this. Is there no packet sent or are 0-length packet generated implicitly > somewhere? I don't know. You can ask the UVC maintainer for more information; my guess is that it treats 0-length packets and missing packets the same. After all, what else could it do? Either way, there is no data. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-28 17:59 ` Alan Stern @ 2019-12-02 15:41 ` Michael Olbrich 2019-12-02 17:02 ` Alan Stern 0 siblings, 1 reply; 24+ messages in thread From: Michael Olbrich @ 2019-12-02 15:41 UTC (permalink / raw) To: Alan Stern Cc: Thinh Nguyen, Felipe Balbi, linux-usb, linux-kernel, kernel, Greg Kroah-Hartman On Thu, Nov 28, 2019 at 12:59:53PM -0500, Alan Stern wrote: > On Thu, 28 Nov 2019, Michael Olbrich wrote: > > On Fri, Nov 15, 2019 at 04:06:10PM -0500, Alan Stern wrote: > > > On Thu, 14 Nov 2019, Thinh Nguyen wrote: > > > > Michael Olbrich wrote: > > > > >>> How about changing the gadget driver instead? For frames where the UVC > > > > >>> gadget knows no video frame data is available (numbers 4, 8, 12, and so > > > > >>> on in the example above), queue a zero-length request. Then there > > > > >>> won't be any gaps in the isochronous packet stream. > > > > >> What Alan suggests may work. Have you tried this? > > > > > Yes and it works in general. There are however some problems with that > > > > > approach that I want to avoid: > > > > > > > > > > 1. It adds extra overhead to handle the extra zero-length request. > > > > > Especially for encoded video the available bandwidth can be quite a bit > > > > > larger that what is actually used. I want to avoid that. > > > > > > This comment doesn't seem to make sense. If the available bandwidth is > > > much _larger_ than what is actually used, what's the problem? You > > > don't run into difficulties until the available bandwidth is too > > > _small_. > > > > > > The extra overhead of a zero-length request should be pretty small. > > > After all, the gadget expects to send a packet for every frame anyway, > > > more or less. > > > > My current test-case is video frames with 450kB on average at 30fps. This > > currently results in ~10 CPU load for the threaded interrupt handler. > > At least in my test, filling the actual video data into the frame has very > > little impact. So if I reserve 900kB to support occasionally larger video > > frames, then I expect that this CPU load will almost double in all cases, > > not just when the video frames are larger. > > This is the sort of thing you need to confirm by experimenting. It is > not at all clear that doubling the interrupt rate will also double the > CPU load, especially if half of the interrupts don't require the CPU to > do much work. As I noted before, actually filling in the video data is only a small part of the measured CPU load. To put in in more precise numbers: From my limited understanding, there are 8000 interrupts per second regardless of the bandwidth (or maybe less for very low bandwidth configurations?). Just queuing requests without any content (so skipping the buffer handling and 'encode()' in uvc_video_complete()) results in ~17% CPU load. If I fill in the data for a video stream with ~1.8MB per frame and 30 fps (and empty requests for the rest) then the CPU load goes up to ~19.5%. This number remains the same for different bandwidths (and therefore different request sizes and a different zero-length request percentage). With my patches the CPU load changes as expected. The 2.5% to fill the data remains and the rest goes down with less interrupts. I am hoping that more batching will help here a bit. But either way the overhead of queuing zero-length request is significant. > > > > > 2. The UVC gadget currently does no know how many zero-length request must > > > > > added. So it needs fill all available request until a new video frame > > > > > arrives. With the current 4 requests that is not a problem right now. But > > > > > that does not scale for USB3 bandwidths. So one thing that I want to do is > > > > > to queue many requests but only enable the interrupt for a few of than. > > > > > From what I can tell from the code, the gadget framework and the dwc3 > > > > > driver should already support this. > > > > > This will result in extra latency. There is probably an acceptable > > > > > trade-off with an acceptable interrupt load and latency. But I would like > > > > > to avoid that if possible. > > > > > > There are two different situations to consider: > > > > > > In the middle of a video stream, latency isn't an issue. > > > The gadget should expect to send a new packet for each frame, > > > and it doesn't know what to put in that packet until it > > > receives the video data or it knows there won't be any data. > > > > > > At the start of a video stream, latency can be an issue. But > > > in this situation the gadget doesn't have to send 0-length > > > requests until there actually is some data available. > > > > > > Either way, it should be okay. > > > > > > As far as interrupt load is concerned, I don't see how it relates to > > > the issue of sending 0-length requests. > > > > Maybe I don't understand, how 0-length requests work. My current > > understanding is, that they are queued like any other request. > > That's right. > > > If I want to reduce the number of interrupts then I need to queue more > > requests and only ask for an interrupt for some of them. This means that > > potentially a lot of 0-length requests requests are queued when a new video > > frame arrives and this means extra latency for the frame. > > Let's say you ask for an interrupt for only even-numbered requests. > Then there would be at most one 0-length request queued when a new > video frame arrives. Processing that 0-length request should require > very little CPU time (almost none) because it contains no data, so the > extra latency would be negligible. > > > I think the worst-case latency is 2x the time between two interrupts. > > So less interrupts mean more latency. > > What matters is the interrupt rate. If you double the rate at which > transfers are queued but ask for an interrupt on only half of them, > then the overall interrupt rate will remain the same and so will the > average latency. > > > The stop/start transfer this patch implements, the video frame can be sent > > immediately without any extra latency. > > The same would be true if you queued a request at the full isochronous > rate. If video data is present, put it in the request; if not then > queue a 0-length request. My problem is this: Let's assume a (for simplicity) that I have a video stream that fills almost the full available bandwidth. And two reduce the bandwidth, two interrupts will be requested for each video frame. - When the first frame arrives, the whole frame is queued. - When the first half of the frame is transmitted the first interrupt arrives. - The second frame has not yet arrived so half a frame worth of 0-length requests are queued. - When the second half of the frame is transmitted the second interrupt arrives. - The second frame has still not yet arrived so another half a frame worth of 0-length requests are queued. - Immediately afterwards the second frame arrives from userspace. At this point, almost one frame worth of 0-length requests are queued so the second frame will have an extra latency of almost one frame. With my patch this does not happen and the transmission of the second starts immediately. > > > > Now, with UVC, it needs to communicate to the dwc3 driver that there > > > > will be a gap after a certain request (and that the device is expecting > > > > to send 0-length data). This is not a normal operation for isoc > > > > transfer. You may need to introduce a new way for the function driver to > > > > do that, possibly a new field in usb_request structure to indicate that. > > > > However, this seems a little awkward. Maybe others can comment on this. > > > > I'm not sure how this is supposed to work. What exactly can the dwc3 driver > > / hardware do to handle a gap? > > Are you talking about the driver on the gadget side or on the host > side? The rules for the host-side driver are spelled out in the > kerneldoc for usb_submit_urb(). As far as I know, there is no > equivalent set of rules for the gadget-side drivers. I'm unsure about the extra API Thinh was talking about. Right now in my tests I'm queuing normal requests with 'request->length = 0' when no video data is available. This has the problems described above. The question is, can we do better than that? What could be done in the driver if it knows that 0-length requests must be transmitted because no new data is available? > > > Note that on the host side, there is a difference between receiving > > > a 0-length packet and receiving no packet at all. As long as both the > > > host and the gadget expect the isochronous stream to be running, there > > > shouldn't be any gaps if you can avoid it. > > > > Huh, so how is this handled on other hardware? From what I can tell the UVC > > gadget works with other drivers and I've not found any special handling for > > this. Is there no packet sent or are 0-length packet generated implicitly > > somewhere? > > I don't know. You can ask the UVC maintainer for more information; my > guess is that it treats 0-length packets and missing packets the same. > After all, what else could it do? Either way, there is no data. Ok. Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-12-02 15:41 ` Michael Olbrich @ 2019-12-02 17:02 ` Alan Stern 0 siblings, 0 replies; 24+ messages in thread From: Alan Stern @ 2019-12-02 17:02 UTC (permalink / raw) To: Michael Olbrich Cc: Thinh Nguyen, Felipe Balbi, linux-usb, linux-kernel, kernel, Greg Kroah-Hartman, Laurent Pinchart, linux-media On Mon, 2 Dec 2019, Michael Olbrich wrote: > > > My current test-case is video frames with 450kB on average at 30fps. This > > > currently results in ~10 CPU load for the threaded interrupt handler. > > > At least in my test, filling the actual video data into the frame has very > > > little impact. So if I reserve 900kB to support occasionally larger video > > > frames, then I expect that this CPU load will almost double in all cases, > > > not just when the video frames are larger. > > > > This is the sort of thing you need to confirm by experimenting. It is > > not at all clear that doubling the interrupt rate will also double the > > CPU load, especially if half of the interrupts don't require the CPU to > > do much work. > > As I noted before, actually filling in the video data is only a small part > of the measured CPU load. To put in in more precise numbers: > > From my limited understanding, there are 8000 interrupts per second > regardless of the bandwidth (or maybe less for very low bandwidth > configurations?). > Just queuing requests without any content (so skipping the buffer handling > and 'encode()' in uvc_video_complete()) results in ~17% CPU load. Can you tell dwc3 to request only 4000 interrupts per second? Or 2000? Or even 60? > If I fill in the data for a video stream with ~1.8MB per frame and 30 fps > (and empty requests for the rest) then the CPU load goes up to ~19.5%. I assume you're talking about a SuperSpeed USB connection here, since high speed can handle no more than 0.8 MB per frame at 30 fps. > This number remains the same for different bandwidths (and therefore > different request sizes and a different zero-length request percentage). > > With my patches the CPU load changes as expected. The 2.5% to fill the data > remains and the rest goes down with less interrupts. > > I am hoping that more batching will help here a bit. But either way the > overhead of queuing zero-length request is significant. Hmmm. It may be that my thinking has been prejudiced by a host-centered attitude, and things may work out different on the gadget side. There's one aspect I'm not clear on. Suppose you decide to skip submitting requests for USB microframes 5 - 7 (at 8000 microframes per second, as you mentioned) and then submit a request to be queued for microframe 8. How does the UDC driver know which microframe to use for that request? Does it always use the first available microframe? > My problem is this: > Let's assume a (for simplicity) that I have a video stream that fills > almost the full available bandwidth. And two reduce the bandwidth, two > interrupts will be requested for each video frame. Let's assume the video frame rate is 30 fps. Since you say almost the full available bandwidth is in use, the number of USB microframes per video frame must be just under 8000 / 30, or around 266. > - When the first frame arrives, the whole frame is queued. Requiring 266 microframes, let's say starting at microframe 0. > - When the first half of the frame is transmitted the first interrupt > arrives. That would be at microframe 133. > - The second frame has not yet arrived so half a frame worth of 0-length > requests are queued. 133 microframes, extending out to microframe 399. > - When the second half of the frame is transmitted the second interrupt > arrives. At the end of microframe 266. > - The second frame has still not yet arrived so another half a frame worth > of 0-length requests are queued. At this point, it has been about 266/8000 s = 33.25 ms since the first video frame arrived. But at a rate of 30 fps, the interval between frames should be 33.33 ms. So either something is wrong or else the second frame is just about to arrive. (Since the frame rate is slightly lower than the rate of transmission of the USB data, it's inevitable that every so often you'll get an interrupt just before a frame arrives.) > - Immediately afterwards the second frame arrives from userspace. At this > point, almost one frame worth of 0-length requests are queued so the > second frame will have an extra latency of almost one frame. I see. There is another way to approach this, however. When you get a USB interrupt and no video data is available, nothing says you have to queue an entire frame (or half-frame) worth of 0-length packets. Instead, queue a request containing 2 ms (for example) of 0-length packets. Presumably the next video frame will arrive during those 2 ms (since it's _expected_ to arrive in no more than 0.08 ms), and from then on you'll be okay until once again the USB data has caught up to the video data. On the other hand, I don't know how the UVC driver manages the alignment between USB packets and video frames on the host end. Perhaps it would prefer something slightly different -- that would be another good question to ask the UVC maintainer (CC'ed). > With my patch this does not happen and the transmission of the second > starts immediately. > The question is, can we do better than that? What could be done in the > driver if it knows that 0-length requests must be transmitted because no > new data is available? It does seems reasonable to require that when a UDC driver encounters a gap in an isochronous stream, it should associate the next request with the first available microframe. If dcw3 did that then you wouldn't have to worry about filling extra space with 0-length packets. But if we make this change, it should be documented in an appropriate place and it should apply to all UDC drivers -- not just dwc3. Alan Stern ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2019-11-14 20:11 ` Thinh Nguyen 2019-11-15 21:06 ` Alan Stern @ 2020-04-09 7:59 ` Michael Grzeschik 2020-04-10 1:09 ` Thinh Nguyen 1 sibling, 1 reply; 24+ messages in thread From: Michael Grzeschik @ 2020-04-09 7:59 UTC (permalink / raw) To: Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 6084 bytes --] On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >Michael Olbrich wrote: >> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>> Alan Stern wrote: >>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>> Michael Olbrich wrote: >>>>>>> Currently, most gadget drivers handle isoc transfers on a best effort >>>>>>> bases: If the request queue runs empty, then there will simply be gaps in >>>>>>> the isoc data stream. >>>>>>> >>>>>>> The UVC gadget depends on this behaviour. It simply provides new requests >>>>>>> when video frames are available and assumes that they are sent as soon as >>>>>>> possible. >>>>>>> >>>>>>> The dwc3 gadget currently works differently: It assumes that there is a >>>>>>> contiguous stream of requests without any gaps. If a request is too late, >>>>>>> then it is dropped by the hardware. >>>>>>> For the UVC gadget this means that a live stream stops after the first >>>>>>> frame because all following requests are late. >>>>>> Can you explain little more how UVC gadget fails? >>>>>> dwc3 controller expects a steady stream of data otherwise it will result >>>>>> in missed_isoc status, and it should be fine as long as new requests are >>>>>> queued. The controller doesn't just drop the request unless there's some >>>>>> other failure. >>>>> UVC (with a live stream) does not fill the complete bandwidth of an >>>>> isochronous endpoint. Let's assume for the example that one video frame >>>>> fills 3 requests. Because it is a live stream, there will be a gap between >>>>> video frames. This is unavoidable, especially for compressed video. So the >>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 10 11 13 14 >>>>> 15 and so on. >>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 5 6 7 8 9 >>>>> 10 11 12. So except for the fist few requests, all are late and result in a >>>>> missed_isoc. I tried to just ignore the missed_isoc but that did not work >>>>> for me. I only received the first frame at the other end. >>>>> Maybe I missing something here, i don't have access to the hardware >>>>> documentation, so I can only guess from the existing driver. >>> The reason I asked is because your patch doesn't seem to address the >>> actual issue. >>> >>> For the 2 checks you do here >>> 1. There are currently no requests queued in the hardware >>> 2. The current frame number provided by DSTS does not match the frame >>> number returned by the last transfer. >>> >>> For #1, it's already done in the dwc3 driver. (check >>> dwc3_gadget_endpoint_transfer_in_progress()) >> But that's only after a isoc_missed occurred. What exactly does that mean? >> Was the request transferred or not? My tests suggest that it was not >> transferred, so I wanted to catch this before it happens. > >Missed_isoc status means that the controller did not move all the data >in an interval. I read in some Processor documentation that in case the host tries to fetch data from the client and no active TRB (HWO=1) is available the XferInProgress Interrupt will be produced, with the missed status set. This is done because the hardware will produce zero length packets on its own, to keep the stream running. >>> For #2, it's unlikely that DSTS current frame number will match with the >>> XferNotReady's frame number. So this check doesn't mean much. >> The frame number is also updated for each "Transfer In Progress" interrupt. >> If they match, then there a new request can still be queued successfully. >> Without this I got unnecessary stop/start transfers in the middle of a >> video frame. But maybe something else was wrong here. I'd need to recheck. > >The reason they may not match is 1) the frame_number is only updated >after the software handles the XferInProgress interrupt. Depends on >system latency, that value may not be updated at the time that we check >the frame_number. >2) This check doesn't work if the service interval is greater than 1 >uframe. That is, it doesn't have to match exactly the time to be >consider not late. Though, the second reason can easily be fixed. In the empty trb case, after the Hardware has send enough zero packets this active transfer has to be stopped with endtransfer cmd. Because every next update transfer on that active transfer will likely lead to further missed transfers, as the newly updated trb will be handled to late anyway. The odd thing here is, that I don't see the refered XferInProgress Interrupts with the missed event, when the started_list is empty. But this would be the only case to fall into this condition and handle it properly. Like alredy assumed in the following code: static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, const struct dwc3_event_depevt *event) { ... if (event->status & DEPEVT_STATUS_MISSED_ISOC) { status = -EXDEV; if (list_empty(&dep->started_list)) stop = true; } ... if (stop) dwc3_stop_active_transfer(dep, true, true); ... } In fact I did sometimes see these XferInProgress Interrupts on empty trb queue after I stoped the tansfer when the started_list was empty right after ep_cleanup_completed_requests has moved all trbs out of the queue. These Interrupts appeared right after the ENDTRANSFER cmd was send. (But I could no verify this every time) Anyways in that case these Interrupts are not useful anymore, as I already implied the same stop, with ENDTRANSFER after we know that there are no other trbs in the chain. Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2020-04-09 7:59 ` Michael Grzeschik @ 2020-04-10 1:09 ` Thinh Nguyen 2020-04-10 22:03 ` Michael Grzeschik 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2020-04-10 1:09 UTC (permalink / raw) To: Michael Grzeschik, Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi, Michael Grzeschik wrote: > On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >> Michael Olbrich wrote: >>> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>>> Alan Stern wrote: >>>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>>> Michael Olbrich wrote: >>>>>>>> Currently, most gadget drivers handle isoc transfers on a best >>>>>>>> effort >>>>>>>> bases: If the request queue runs empty, then there will simply >>>>>>>> be gaps in >>>>>>>> the isoc data stream. >>>>>>>> >>>>>>>> The UVC gadget depends on this behaviour. It simply provides >>>>>>>> new requests >>>>>>>> when video frames are available and assumes that they are sent >>>>>>>> as soon as >>>>>>>> possible. >>>>>>>> >>>>>>>> The dwc3 gadget currently works differently: It assumes that >>>>>>>> there is a >>>>>>>> contiguous stream of requests without any gaps. If a request is >>>>>>>> too late, >>>>>>>> then it is dropped by the hardware. >>>>>>>> For the UVC gadget this means that a live stream stops after >>>>>>>> the first >>>>>>>> frame because all following requests are late. >>>>>>> Can you explain little more how UVC gadget fails? >>>>>>> dwc3 controller expects a steady stream of data otherwise it >>>>>>> will result >>>>>>> in missed_isoc status, and it should be fine as long as new >>>>>>> requests are >>>>>>> queued. The controller doesn't just drop the request unless >>>>>>> there's some >>>>>>> other failure. >>>>>> UVC (with a live stream) does not fill the complete bandwidth of an >>>>>> isochronous endpoint. Let's assume for the example that one video >>>>>> frame >>>>>> fills 3 requests. Because it is a live stream, there will be a >>>>>> gap between >>>>>> video frames. This is unavoidable, especially for compressed >>>>>> video. So the >>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 >>>>>> 10 11 13 14 >>>>>> 15 and so on. >>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 >>>>>> 5 6 7 8 9 >>>>>> 10 11 12. So except for the fist few requests, all are late and >>>>>> result in a >>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did >>>>>> not work >>>>>> for me. I only received the first frame at the other end. >>>>>> Maybe I missing something here, i don't have access to the hardware >>>>>> documentation, so I can only guess from the existing driver. >>>> The reason I asked is because your patch doesn't seem to address the >>>> actual issue. >>>> >>>> For the 2 checks you do here >>>> 1. There are currently no requests queued in the hardware >>>> 2. The current frame number provided by DSTS does not match the frame >>>> number returned by the last transfer. >>>> >>>> For #1, it's already done in the dwc3 driver. (check >>>> dwc3_gadget_endpoint_transfer_in_progress()) >>> But that's only after a isoc_missed occurred. What exactly does that >>> mean? >>> Was the request transferred or not? My tests suggest that it was not >>> transferred, so I wanted to catch this before it happens. >> >> Missed_isoc status means that the controller did not move all the data >> in an interval. > > I read in some Processor documentation that in case the host tries to > fetch data from the client and no active TRB (HWO=1) is available the > XferInProgress Interrupt will be produced, with the missed status set. > This is done because the hardware will produce zero length packets > on its own, to keep the stream running. The controller only generates XferInProgress if it had processed a TRB (with specific control bits). For IN direction, if the controller is starved of TRB, it will send a ZLP if the host requests for data. > >>>> For #2, it's unlikely that DSTS current frame number will match >>>> with the >>>> XferNotReady's frame number. So this check doesn't mean much. >>> The frame number is also updated for each "Transfer In Progress" >>> interrupt. >>> If they match, then there a new request can still be queued >>> successfully. >>> Without this I got unnecessary stop/start transfers in the middle of a >>> video frame. But maybe something else was wrong here. I'd need to >>> recheck. >> >> The reason they may not match is 1) the frame_number is only updated >> after the software handles the XferInProgress interrupt. Depends on >> system latency, that value may not be updated at the time that we check >> the frame_number. >> 2) This check doesn't work if the service interval is greater than 1 >> uframe. That is, it doesn't have to match exactly the time to be >> consider not late. Though, the second reason can easily be fixed. > > In the empty trb case, after the Hardware has send enough zero packets > this > active transfer has to be stopped with endtransfer cmd. Because every > next > update transfer on that active transfer will likely lead to further > missed > transfers, as the newly updated trb will be handled to late anyway. The controller is expecting the function driver to feed TRBs to the controller for every interval. If it's late, then the controller will consider that data "missed_isoc". In your case, the UVC driver seems to queue requests to the controller driver as if it is bulk requests, and the UVC expects those data to go out at the time it queues. To achieve what UVC needs, then you may want to issue END_TRANSFER command before the next burst of data. This way, the controller can restart the isoc endpoint and not consider the next video frame data late. There are some corner cases that you need to watch out for. If you're going for this route, we can look further. Also, you'd need provide a way for the UVC to communicate to the dwc3 to let it know to expect the next burst of data. > > > The odd thing here is, that I don't see the refered XferInProgress > Interrupts with the missed event, when the started_list is empty. See my first comment. > > But this would be the only case to fall into this condition and handle it > properly. Like alredy assumed in the following code: > > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep > *dep, > const struct dwc3_event_depevt *event) > { > ... > > if (event->status & DEPEVT_STATUS_MISSED_ISOC) { > status = -EXDEV; > > if (list_empty(&dep->started_list)) > stop = true; > } > > ... > > if (stop) > dwc3_stop_active_transfer(dep, true, true); > ... > } > > In fact I did sometimes see these XferInProgress Interrupts on empty > trb queue > after I stoped the tansfer when the started_list was empty right after > ep_cleanup_completed_requests has moved all trbs out of the queue. > > These Interrupts appeared right after the ENDTRANSFER cmd was send. > (But I > could no verify this every time) If END_TRANSFER command completes, then you should not see XferInProgress event. The next event should ber XferNotReady. > > Anyways in that case these Interrupts are not useful anymore, as I > already > implied the same stop, with ENDTRANSFER after we know that there are > no other > trbs in the chain. > Just curious, do you know if there's a reason for UVC to behave this way? Seems like what it's trying to do is more for bulk. Maybe it wants bandwidth priority perhaps? BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2020-04-10 1:09 ` Thinh Nguyen @ 2020-04-10 22:03 ` Michael Grzeschik 2020-04-11 0:59 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Michael Grzeschik @ 2020-04-10 22:03 UTC (permalink / raw) To: Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 10029 bytes --] On Fri, Apr 10, 2020 at 01:09:23AM +0000, Thinh Nguyen wrote: >Hi, > >Michael Grzeschik wrote: >> On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >>> Michael Olbrich wrote: >>>> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>>>> Alan Stern wrote: >>>>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>>>> Michael Olbrich wrote: >>>>>>>>> Currently, most gadget drivers handle isoc transfers on a best >>>>>>>>> effort >>>>>>>>> bases: If the request queue runs empty, then there will simply >>>>>>>>> be gaps in >>>>>>>>> the isoc data stream. >>>>>>>>> >>>>>>>>> The UVC gadget depends on this behaviour. It simply provides >>>>>>>>> new requests >>>>>>>>> when video frames are available and assumes that they are sent >>>>>>>>> as soon as >>>>>>>>> possible. >>>>>>>>> >>>>>>>>> The dwc3 gadget currently works differently: It assumes that >>>>>>>>> there is a >>>>>>>>> contiguous stream of requests without any gaps. If a request is >>>>>>>>> too late, >>>>>>>>> then it is dropped by the hardware. >>>>>>>>> For the UVC gadget this means that a live stream stops after >>>>>>>>> the first >>>>>>>>> frame because all following requests are late. >>>>>>>> Can you explain little more how UVC gadget fails? >>>>>>>> dwc3 controller expects a steady stream of data otherwise it >>>>>>>> will result >>>>>>>> in missed_isoc status, and it should be fine as long as new >>>>>>>> requests are >>>>>>>> queued. The controller doesn't just drop the request unless >>>>>>>> there's some >>>>>>>> other failure. >>>>>>> UVC (with a live stream) does not fill the complete bandwidth of an >>>>>>> isochronous endpoint. Let's assume for the example that one video >>>>>>> frame >>>>>>> fills 3 requests. Because it is a live stream, there will be a >>>>>>> gap between >>>>>>> video frames. This is unavoidable, especially for compressed >>>>>>> video. So the >>>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 >>>>>>> 10 11 13 14 >>>>>>> 15 and so on. >>>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 >>>>>>> 5 6 7 8 9 >>>>>>> 10 11 12. So except for the fist few requests, all are late and >>>>>>> result in a >>>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did >>>>>>> not work >>>>>>> for me. I only received the first frame at the other end. >>>>>>> Maybe I missing something here, i don't have access to the hardware >>>>>>> documentation, so I can only guess from the existing driver. >>>>> The reason I asked is because your patch doesn't seem to address the >>>>> actual issue. >>>>> >>>>> For the 2 checks you do here >>>>> 1. There are currently no requests queued in the hardware >>>>> 2. The current frame number provided by DSTS does not match the frame >>>>> number returned by the last transfer. >>>>> >>>>> For #1, it's already done in the dwc3 driver. (check >>>>> dwc3_gadget_endpoint_transfer_in_progress()) >>>> But that's only after a isoc_missed occurred. What exactly does that >>>> mean? >>>> Was the request transferred or not? My tests suggest that it was not >>>> transferred, so I wanted to catch this before it happens. >>> >>> Missed_isoc status means that the controller did not move all the data >>> in an interval. >> >> I read in some Processor documentation that in case the host tries to >> fetch data from the client and no active TRB (HWO=1) is available the >> XferInProgress Interrupt will be produced, with the missed status set. >> This is done because the hardware will produce zero length packets >> on its own, to keep the stream running. > >The controller only generates XferInProgress if it had processed a TRB >(with specific control bits). For IN direction, if the controller is >starved of TRB, it will send a ZLP if the host requests for data. Which control bits are those? ISOC-First, Chain and Last bits? I see the Scatter-Gather preparation is using these pattern. >>>>> For #2, it's unlikely that DSTS current frame number will match >>>>> with the >>>>> XferNotReady's frame number. So this check doesn't mean much. >>>> The frame number is also updated for each "Transfer In Progress" >>>> interrupt. >>>> If they match, then there a new request can still be queued >>>> successfully. >>>> Without this I got unnecessary stop/start transfers in the middle of a >>>> video frame. But maybe something else was wrong here. I'd need to >>>> recheck. >>> >>> The reason they may not match is 1) the frame_number is only updated >>> after the software handles the XferInProgress interrupt. Depends on >>> system latency, that value may not be updated at the time that we check >>> the frame_number. >>> 2) This check doesn't work if the service interval is greater than 1 >>> uframe. That is, it doesn't have to match exactly the time to be >>> consider not late. Though, the second reason can easily be fixed. >> >> In the empty trb case, after the Hardware has send enough zero packets >> this >> active transfer has to be stopped with endtransfer cmd. Because every >> next >> update transfer on that active transfer will likely lead to further >> missed >> transfers, as the newly updated trb will be handled to late anyway. > >The controller is expecting the function driver to feed TRBs to the >controller for every interval. If it's late, then the controller will >consider that data "missed_isoc". > >In your case, the UVC driver seems to queue requests to the controller >driver as if it is bulk requests, and the UVC expects those data to go >out at the time it queues. To achieve what UVC needs, then you may want >to issue END_TRANSFER command before the next burst of data. This way, >the controller can restart the isoc endpoint and not consider the next >video frame data late. There are some corner cases that you need to >watch out for. If you're going for this route, we can look further. Right, for now the drivers is doing: - Strart Transfer (with the right frame number) once. - Update Transfer On every ep_queue with the corresponding TRB setting CHN = 0, IOC = 1, First-ISOC = 1 - End Transfer is somehow not handled right for this case. See my first comment. I think dwc3_prepare_one_trb_sg does the proper chain handling already. >Also, you'd need provide a way for the UVC to communicate to the dwc3 to >let it know to expect the next burst of data. Can the UVC not just enqueue one big sg-request, which will represent one burst and not one TRB. Also that is what the SG code already seem to handle right. >> The odd thing here is, that I don't see the refered XferInProgress >> Interrupts with the missed event, when the started_list is empty. > >See my first comment. > >> >> But this would be the only case to fall into this condition and handle it >> properly. Like alredy assumed in the following code: >> >> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep >> *dep, >> const struct dwc3_event_depevt *event) >> { >> ... >> >> if (event->status & DEPEVT_STATUS_MISSED_ISOC) { >> status = -EXDEV; >> >> if (list_empty(&dep->started_list)) >> stop = true; >> } >> >> ... >> >> if (stop) >> dwc3_stop_active_transfer(dep, true, true); >> ... >> } >> >> In fact I did sometimes see these XferInProgress Interrupts on empty >> trb queue >> after I stoped the tansfer when the started_list was empty right after >> ep_cleanup_completed_requests has moved all trbs out of the queue. >> >> These Interrupts appeared right after the ENDTRANSFER cmd was send. >> (But I >> could no verify this every time) > >If END_TRANSFER command completes, then you should not see >XferInProgress event. The next event should ber XferNotReady. Right. This also stops, after the Command Complete Interrupt arrives. >> Anyways in that case these Interrupts are not useful anymore, as I >> already >> implied the same stop, with ENDTRANSFER after we know that there are >> no other >> trbs in the chain. >> > >Just curious, do you know if there's a reason for UVC to behave this >way? Seems like what it's trying to do is more for bulk. Maybe it wants >bandwidth priority perhaps? I don't know, probably it was more likely for USB 2.0 controllers to be handled this way. As mentioned the current uvc code is also working when we add this changes. diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index ec357f64f319..a5dc44f2e9d8 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2629,6 +2629,9 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); + if (list_empty(&dep->started_list)) + stop = true; + if (stop) dwc3_stop_active_transfer(dep, true, true); else if (dwc3_gadget_ep_should_continue(dep)) diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index da6ba8ba4bca..a3dac5d91aae 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -183,6 +183,7 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) switch (req->status) { case 0: + case -EXDEV: /* we ignore missed transfers */ break; case -ESHUTDOWN: /* disconnect from host. */ Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2020-04-10 22:03 ` Michael Grzeschik @ 2020-04-11 0:59 ` Thinh Nguyen 2020-04-21 6:51 ` Michael Grzeschik 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2020-04-11 0:59 UTC (permalink / raw) To: Michael Grzeschik, Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi, Michael Grzeschik wrote: > On Fri, Apr 10, 2020 at 01:09:23AM +0000, Thinh Nguyen wrote: >> Hi, >> >> Michael Grzeschik wrote: >>> On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >>>> Michael Olbrich wrote: >>>>> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>>>>> Alan Stern wrote: >>>>>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>>>>> Michael Olbrich wrote: >>>>>>>>>> Currently, most gadget drivers handle isoc transfers on a best >>>>>>>>>> effort >>>>>>>>>> bases: If the request queue runs empty, then there will simply >>>>>>>>>> be gaps in >>>>>>>>>> the isoc data stream. >>>>>>>>>> >>>>>>>>>> The UVC gadget depends on this behaviour. It simply provides >>>>>>>>>> new requests >>>>>>>>>> when video frames are available and assumes that they are sent >>>>>>>>>> as soon as >>>>>>>>>> possible. >>>>>>>>>> >>>>>>>>>> The dwc3 gadget currently works differently: It assumes that >>>>>>>>>> there is a >>>>>>>>>> contiguous stream of requests without any gaps. If a request is >>>>>>>>>> too late, >>>>>>>>>> then it is dropped by the hardware. >>>>>>>>>> For the UVC gadget this means that a live stream stops after >>>>>>>>>> the first >>>>>>>>>> frame because all following requests are late. >>>>>>>>> Can you explain little more how UVC gadget fails? >>>>>>>>> dwc3 controller expects a steady stream of data otherwise it >>>>>>>>> will result >>>>>>>>> in missed_isoc status, and it should be fine as long as new >>>>>>>>> requests are >>>>>>>>> queued. The controller doesn't just drop the request unless >>>>>>>>> there's some >>>>>>>>> other failure. >>>>>>>> UVC (with a live stream) does not fill the complete bandwidth >>>>>>>> of an >>>>>>>> isochronous endpoint. Let's assume for the example that one video >>>>>>>> frame >>>>>>>> fills 3 requests. Because it is a live stream, there will be a >>>>>>>> gap between >>>>>>>> video frames. This is unavoidable, especially for compressed >>>>>>>> video. So the >>>>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 >>>>>>>> 10 11 13 14 >>>>>>>> 15 and so on. >>>>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 >>>>>>>> 5 6 7 8 9 >>>>>>>> 10 11 12. So except for the fist few requests, all are late and >>>>>>>> result in a >>>>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did >>>>>>>> not work >>>>>>>> for me. I only received the first frame at the other end. >>>>>>>> Maybe I missing something here, i don't have access to the >>>>>>>> hardware >>>>>>>> documentation, so I can only guess from the existing driver. >>>>>> The reason I asked is because your patch doesn't seem to address the >>>>>> actual issue. >>>>>> >>>>>> For the 2 checks you do here >>>>>> 1. There are currently no requests queued in the hardware >>>>>> 2. The current frame number provided by DSTS does not match the >>>>>> frame >>>>>> number returned by the last transfer. >>>>>> >>>>>> For #1, it's already done in the dwc3 driver. (check >>>>>> dwc3_gadget_endpoint_transfer_in_progress()) >>>>> But that's only after a isoc_missed occurred. What exactly does that >>>>> mean? >>>>> Was the request transferred or not? My tests suggest that it was not >>>>> transferred, so I wanted to catch this before it happens. >>>> >>>> Missed_isoc status means that the controller did not move all the data >>>> in an interval. >>> >>> I read in some Processor documentation that in case the host tries to >>> fetch data from the client and no active TRB (HWO=1) is available the >>> XferInProgress Interrupt will be produced, with the missed status set. >>> This is done because the hardware will produce zero length packets >>> on its own, to keep the stream running. >> >> The controller only generates XferInProgress if it had processed a TRB >> (with specific control bits). For IN direction, if the controller is >> starved of TRB, it will send a ZLP if the host requests for data. > > Which control bits are those? ISOC-First, Chain and Last bits? > > I see the Scatter-Gather preparation is using these pattern. The IOC bit. You can check the programming guide for more detail on how to setup the TRBs, but what we have in dwc3 is fine. > >>>>>> For #2, it's unlikely that DSTS current frame number will match >>>>>> with the >>>>>> XferNotReady's frame number. So this check doesn't mean much. >>>>> The frame number is also updated for each "Transfer In Progress" >>>>> interrupt. >>>>> If they match, then there a new request can still be queued >>>>> successfully. >>>>> Without this I got unnecessary stop/start transfers in the middle >>>>> of a >>>>> video frame. But maybe something else was wrong here. I'd need to >>>>> recheck. >>>> >>>> The reason they may not match is 1) the frame_number is only updated >>>> after the software handles the XferInProgress interrupt. Depends on >>>> system latency, that value may not be updated at the time that we >>>> check >>>> the frame_number. >>>> 2) This check doesn't work if the service interval is greater than 1 >>>> uframe. That is, it doesn't have to match exactly the time to be >>>> consider not late. Though, the second reason can easily be fixed. >>> >>> In the empty trb case, after the Hardware has send enough zero packets >>> this >>> active transfer has to be stopped with endtransfer cmd. Because every >>> next >>> update transfer on that active transfer will likely lead to further >>> missed >>> transfers, as the newly updated trb will be handled to late anyway. >> >> The controller is expecting the function driver to feed TRBs to the >> controller for every interval. If it's late, then the controller will >> consider that data "missed_isoc". >> >> In your case, the UVC driver seems to queue requests to the controller >> driver as if it is bulk requests, and the UVC expects those data to go >> out at the time it queues. To achieve what UVC needs, then you may want >> to issue END_TRANSFER command before the next burst of data. This way, >> the controller can restart the isoc endpoint and not consider the next >> video frame data late. There are some corner cases that you need to >> watch out for. If you're going for this route, we can look further. > > Right, for now the drivers is doing: > > - Strart Transfer (with the right frame number) once. > > - Update Transfer On every ep_queue with the corresponding TRB > setting CHN = 0, IOC = 1, First-ISOC = 1 > > - End Transfer is somehow not handled right for this case. > > See my first comment. I think dwc3_prepare_one_trb_sg does the proper > chain > handling already. > >> Also, you'd need provide a way for the UVC to communicate to the dwc3 to >> let it know to expect the next burst of data. > > Can the UVC not just enqueue one big sg-request, which will represent > one burst > and not one TRB. Also that is what the SG code already seem to handle > right. Do you need SG? What size does video class driver setup its isoc URB? If it's superspeed, max is 48K, and depending on the type of platform you're running UVC on, you can setup a single 48K buffer per request if you want to do that. However, it's probably not a good idea since many host controllers can't even handle even close to 48K/uframe. What I was saying is if UVC knows when to wait for the next video data, it can tell dwc3 to stop the isoc endpoint before queuing the next video data in a set of requests. If UVC doesn't know that, then it needs to tell dwc3 to change its handling of isoc requests. > >>> The odd thing here is, that I don't see the refered XferInProgress >>> Interrupts with the missed event, when the started_list is empty. >> >> See my first comment. >> >>> >>> But this would be the only case to fall into this condition and >>> handle it >>> properly. Like alredy assumed in the following code: >>> >>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep >>> *dep, >>> const struct dwc3_event_depevt *event) >>> { >>> ... >>> >>> if (event->status & DEPEVT_STATUS_MISSED_ISOC) { >>> status = -EXDEV; >>> >>> if (list_empty(&dep->started_list)) >>> stop = true; >>> } >>> >>> ... >>> >>> if (stop) >>> dwc3_stop_active_transfer(dep, true, true); >>> ... >>> } >>> >>> In fact I did sometimes see these XferInProgress Interrupts on empty >>> trb queue >>> after I stoped the tansfer when the started_list was empty right after >>> ep_cleanup_completed_requests has moved all trbs out of the queue. >>> >>> These Interrupts appeared right after the ENDTRANSFER cmd was send. >>> (But I >>> could no verify this every time) >> >> If END_TRANSFER command completes, then you should not see >> XferInProgress event. The next event should ber XferNotReady. > > Right. This also stops, after the Command Complete Interrupt arrives. > >>> Anyways in that case these Interrupts are not useful anymore, as I >>> already >>> implied the same stop, with ENDTRANSFER after we know that there are >>> no other >>> trbs in the chain. >>> >> >> Just curious, do you know if there's a reason for UVC to behave this >> way? Seems like what it's trying to do is more for bulk. Maybe it wants >> bandwidth priority perhaps? > > I don't know, probably it was more likely for USB 2.0 controllers to > be handled > this way. > > As mentioned the current uvc code is also working when we add this > changes. > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index ec357f64f319..a5dc44f2e9d8 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2629,6 +2629,9 @@ static void > dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, > > dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); > > + if (list_empty(&dep->started_list)) > + stop = true; > + You should check the pending list too and either drop them or prepare those requests (maybe too late). This happens when there's no available TRB but UVC queues more requests. Also, make sure this change only applies for isoc. This may work. Just keep in mind that the timing parameter of the XferNotReady will be expired by the time the UVC queues the next isoc request. (Maybe that's why you tried to check for the current frame number via DSTS instead in your first patch?). With the new changes in Felipe testing/next branch, this may be ok. > if (stop) > dwc3_stop_active_transfer(dep, true, true); > else if (dwc3_gadget_ep_should_continue(dep)) > > diff --git a/drivers/usb/gadget/function/uvc_video.c > b/drivers/usb/gadget/function/uvc_video.c > index da6ba8ba4bca..a3dac5d91aae 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -183,6 +183,7 @@ uvc_video_complete(struct usb_ep *ep, struct > usb_request *req) > > switch (req->status) { > case 0: > + case -EXDEV: /* we ignore missed transfers */ Any time you see missed_isoc, you have data dropped. May want to check to see what's going on. > break; > > case -ESHUTDOWN: /* disconnect from host. */ > > BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2020-04-11 0:59 ` Thinh Nguyen @ 2020-04-21 6:51 ` Michael Grzeschik 2020-04-21 23:41 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Michael Grzeschik @ 2020-04-21 6:51 UTC (permalink / raw) To: Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 13050 bytes --] Hi, On Sat, Apr 11, 2020 at 12:59:47AM +0000, Thinh Nguyen wrote: >Hi, > >Michael Grzeschik wrote: >> On Fri, Apr 10, 2020 at 01:09:23AM +0000, Thinh Nguyen wrote: >>> Hi, >>> >>> Michael Grzeschik wrote: >>>> On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >>>>> Michael Olbrich wrote: >>>>>> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>>>>>> Alan Stern wrote: >>>>>>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>>>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>>>>>> Michael Olbrich wrote: >>>>>>>>>>> Currently, most gadget drivers handle isoc transfers on a best >>>>>>>>>>> effort >>>>>>>>>>> bases: If the request queue runs empty, then there will simply >>>>>>>>>>> be gaps in >>>>>>>>>>> the isoc data stream. >>>>>>>>>>> >>>>>>>>>>> The UVC gadget depends on this behaviour. It simply provides >>>>>>>>>>> new requests >>>>>>>>>>> when video frames are available and assumes that they are sent >>>>>>>>>>> as soon as >>>>>>>>>>> possible. >>>>>>>>>>> >>>>>>>>>>> The dwc3 gadget currently works differently: It assumes that >>>>>>>>>>> there is a >>>>>>>>>>> contiguous stream of requests without any gaps. If a request is >>>>>>>>>>> too late, >>>>>>>>>>> then it is dropped by the hardware. >>>>>>>>>>> For the UVC gadget this means that a live stream stops after >>>>>>>>>>> the first >>>>>>>>>>> frame because all following requests are late. >>>>>>>>>> Can you explain little more how UVC gadget fails? >>>>>>>>>> dwc3 controller expects a steady stream of data otherwise it >>>>>>>>>> will result >>>>>>>>>> in missed_isoc status, and it should be fine as long as new >>>>>>>>>> requests are >>>>>>>>>> queued. The controller doesn't just drop the request unless >>>>>>>>>> there's some >>>>>>>>>> other failure. >>>>>>>>> UVC (with a live stream) does not fill the complete bandwidth >>>>>>>>> of an >>>>>>>>> isochronous endpoint. Let's assume for the example that one video >>>>>>>>> frame >>>>>>>>> fills 3 requests. Because it is a live stream, there will be a >>>>>>>>> gap between >>>>>>>>> video frames. This is unavoidable, especially for compressed >>>>>>>>> video. So the >>>>>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 7 9 >>>>>>>>> 10 11 13 14 >>>>>>>>> 15 and so on. >>>>>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 >>>>>>>>> 5 6 7 8 9 >>>>>>>>> 10 11 12. So except for the fist few requests, all are late and >>>>>>>>> result in a >>>>>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did >>>>>>>>> not work >>>>>>>>> for me. I only received the first frame at the other end. >>>>>>>>> Maybe I missing something here, i don't have access to the >>>>>>>>> hardware >>>>>>>>> documentation, so I can only guess from the existing driver. >>>>>>> The reason I asked is because your patch doesn't seem to address the >>>>>>> actual issue. >>>>>>> >>>>>>> For the 2 checks you do here >>>>>>> 1. There are currently no requests queued in the hardware >>>>>>> 2. The current frame number provided by DSTS does not match the >>>>>>> frame >>>>>>> number returned by the last transfer. >>>>>>> >>>>>>> For #1, it's already done in the dwc3 driver. (check >>>>>>> dwc3_gadget_endpoint_transfer_in_progress()) >>>>>> But that's only after a isoc_missed occurred. What exactly does that >>>>>> mean? >>>>>> Was the request transferred or not? My tests suggest that it was not >>>>>> transferred, so I wanted to catch this before it happens. >>>>> >>>>> Missed_isoc status means that the controller did not move all the data >>>>> in an interval. >>>> >>>> I read in some Processor documentation that in case the host tries to >>>> fetch data from the client and no active TRB (HWO=1) is available the >>>> XferInProgress Interrupt will be produced, with the missed status set. >>>> This is done because the hardware will produce zero length packets >>>> on its own, to keep the stream running. >>> >>> The controller only generates XferInProgress if it had processed a TRB >>> (with specific control bits). For IN direction, if the controller is >>> starved of TRB, it will send a ZLP if the host requests for data. >> >> Which control bits are those? ISOC-First, Chain and Last bits? >> >> I see the Scatter-Gather preparation is using these pattern. > >The IOC bit. You can check the programming guide for more detail on how >to setup the TRBs, but what we have in dwc3 is fine. OK. >>>>>>> For #2, it's unlikely that DSTS current frame number will match >>>>>>> with the >>>>>>> XferNotReady's frame number. So this check doesn't mean much. >>>>>> The frame number is also updated for each "Transfer In Progress" >>>>>> interrupt. >>>>>> If they match, then there a new request can still be queued >>>>>> successfully. >>>>>> Without this I got unnecessary stop/start transfers in the middle >>>>>> of a >>>>>> video frame. But maybe something else was wrong here. I'd need to >>>>>> recheck. >>>>> >>>>> The reason they may not match is 1) the frame_number is only updated >>>>> after the software handles the XferInProgress interrupt. Depends on >>>>> system latency, that value may not be updated at the time that we >>>>> check >>>>> the frame_number. >>>>> 2) This check doesn't work if the service interval is greater than 1 >>>>> uframe. That is, it doesn't have to match exactly the time to be >>>>> consider not late. Though, the second reason can easily be fixed. >>>> >>>> In the empty trb case, after the Hardware has send enough zero packets >>>> this >>>> active transfer has to be stopped with endtransfer cmd. Because every >>>> next >>>> update transfer on that active transfer will likely lead to further >>>> missed >>>> transfers, as the newly updated trb will be handled to late anyway. >>> >>> The controller is expecting the function driver to feed TRBs to the >>> controller for every interval. If it's late, then the controller will >>> consider that data "missed_isoc". >>> >>> In your case, the UVC driver seems to queue requests to the controller >>> driver as if it is bulk requests, and the UVC expects those data to go >>> out at the time it queues. To achieve what UVC needs, then you may want >>> to issue END_TRANSFER command before the next burst of data. This way, >>> the controller can restart the isoc endpoint and not consider the next >>> video frame data late. There are some corner cases that you need to >>> watch out for. If you're going for this route, we can look further. >> >> Right, for now the drivers is doing: >> >> - Strart Transfer (with the right frame number) once. >> >> - Update Transfer On every ep_queue with the corresponding TRB >> setting CHN = 0, IOC = 1, First-ISOC = 1 >> >> - End Transfer is somehow not handled right for this case. >> >> See my first comment. I think dwc3_prepare_one_trb_sg does the proper >> chain >> handling already. >> >>> Also, you'd need provide a way for the UVC to communicate to the dwc3 to >>> let it know to expect the next burst of data. >> >> Can the UVC not just enqueue one big sg-request, which will represent >> one burst >> and not one TRB. Also that is what the SG code already seem to handle >> right. > >Do you need SG? What size does video class driver setup its isoc URB? If >it's superspeed, max is 48K, and depending on the type of platform >you're running UVC on, you can setup a single 48K buffer per request if >you want to do that. However, it's probably not a good idea since many >host controllers can't even handle even close to 48K/uframe. We wan't to transfer uncached 4K Video frames via UVC. So Scatter-Gather is a must anyway. >What I was saying is if UVC knows when to wait for the next video data, >it can tell dwc3 to stop the isoc endpoint before queuing the next video >data in a set of requests. If UVC doesn't know that, then it needs to >tell dwc3 to change its handling of isoc requests. In function/gadget/uvc_video.c we take a buffer and pump it into many available requests as we find. On the way the driver can drain that video frame queue, in that case we could stop the transfer. I have prepared a patch where we have only one source, queueing new requests, so this could work. For now to limit the interrupt load on the dwc3 driver we already set the request no_interrupt on every nth request, and make sure the interrupt is on the very last one of the frame. But with that we still sometimes run into missed transfers as the queued trb list somehow ends up with a TRB not having the IOC bit set. >>>> The odd thing here is, that I don't see the refered XferInProgress >>>> Interrupts with the missed event, when the started_list is empty. >>> >>> See my first comment. >>> >>>> >>>> But this would be the only case to fall into this condition and >>>> handle it >>>> properly. Like alredy assumed in the following code: >>>> >>>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep >>>> *dep, >>>> const struct dwc3_event_depevt *event) >>>> { >>>> ... >>>> >>>> if (event->status & DEPEVT_STATUS_MISSED_ISOC) { >>>> status = -EXDEV; >>>> >>>> if (list_empty(&dep->started_list)) >>>> stop = true; >>>> } >>>> >>>> ... >>>> >>>> if (stop) >>>> dwc3_stop_active_transfer(dep, true, true); >>>> ... >>>> } >>>> >>>> In fact I did sometimes see these XferInProgress Interrupts on empty >>>> trb queue >>>> after I stoped the tansfer when the started_list was empty right after >>>> ep_cleanup_completed_requests has moved all trbs out of the queue. >>>> >>>> These Interrupts appeared right after the ENDTRANSFER cmd was send. >>>> (But I >>>> could no verify this every time) >>> >>> If END_TRANSFER command completes, then you should not see >>> XferInProgress event. The next event should ber XferNotReady. >> >> Right. This also stops, after the Command Complete Interrupt arrives. >> >>>> Anyways in that case these Interrupts are not useful anymore, as I >>>> already >>>> implied the same stop, with ENDTRANSFER after we know that there are >>>> no other >>>> trbs in the chain. >>>> >>> >>> Just curious, do you know if there's a reason for UVC to behave this >>> way? Seems like what it's trying to do is more for bulk. Maybe it wants >>> bandwidth priority perhaps? >> >> I don't know, probably it was more likely for USB 2.0 controllers to >> be handled >> this way. >> >> As mentioned the current uvc code is also working when we add this >> changes. >> >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index ec357f64f319..a5dc44f2e9d8 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2629,6 +2629,9 @@ static void >> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >> >> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); >> >> + if (list_empty(&dep->started_list)) >> + stop = true; >> + > >You should check the pending list too and either drop them or prepare >those requests (maybe too late). This happens when there's no available >TRB but UVC queues more requests. >Also, make sure this change only applies for isoc. I will send the patches for that so we can discuss the right handling for that in a separate thread. >This may work. Just keep in mind that the timing parameter of the >XferNotReady will be expired by the time the UVC queues the next isoc >request. (Maybe that's why you tried to check for the current frame >number via DSTS instead in your first patch?). >With the new changes in Felipe testing/next branch, this may be ok. > > >> if (stop) >> dwc3_stop_active_transfer(dep, true, true); >> else if (dwc3_gadget_ep_should_continue(dep)) >> >> diff --git a/drivers/usb/gadget/function/uvc_video.c >> b/drivers/usb/gadget/function/uvc_video.c >> index da6ba8ba4bca..a3dac5d91aae 100644 >> --- a/drivers/usb/gadget/function/uvc_video.c >> +++ b/drivers/usb/gadget/function/uvc_video.c >> @@ -183,6 +183,7 @@ uvc_video_complete(struct usb_ep *ep, struct >> usb_request *req) >> >> switch (req->status) { >> case 0: >> + case -EXDEV: /* we ignore missed transfers */ > >Any time you see missed_isoc, you have data dropped. May want to check >to see what's going on. See my comment above. >> break; >> >> case -ESHUTDOWN: /* disconnect from host. */ >> >> > >BR, >Thinh > Regards, Michael -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late 2020-04-21 6:51 ` Michael Grzeschik @ 2020-04-21 23:41 ` Thinh Nguyen 0 siblings, 0 replies; 24+ messages in thread From: Thinh Nguyen @ 2020-04-21 23:41 UTC (permalink / raw) To: Michael Grzeschik, Thinh Nguyen Cc: Alan Stern, linux-usb, Felipe Balbi, Greg Kroah-Hartman, linux-kernel, kernel Hi, Michael Grzeschik wrote: > Hi, > > On Sat, Apr 11, 2020 at 12:59:47AM +0000, Thinh Nguyen wrote: >> Hi, >> >> Michael Grzeschik wrote: >>> On Fri, Apr 10, 2020 at 01:09:23AM +0000, Thinh Nguyen wrote: >>>> Hi, >>>> >>>> Michael Grzeschik wrote: >>>>> On Thu, Nov 14, 2019 at 08:11:56PM +0000, Thinh Nguyen wrote: >>>>>> Michael Olbrich wrote: >>>>>>> On Wed, Nov 13, 2019 at 07:14:59PM +0000, Thinh Nguyen wrote: >>>>>>>> Alan Stern wrote: >>>>>>>>> On Wed, 13 Nov 2019, Michael Olbrich wrote: >>>>>>>>>> On Wed, Nov 13, 2019 at 03:55:01AM +0000, Thinh Nguyen wrote: >>>>>>>>>>> Michael Olbrich wrote: >>>>>>>>>>>> Currently, most gadget drivers handle isoc transfers on a best >>>>>>>>>>>> effort >>>>>>>>>>>> bases: If the request queue runs empty, then there will simply >>>>>>>>>>>> be gaps in >>>>>>>>>>>> the isoc data stream. >>>>>>>>>>>> >>>>>>>>>>>> The UVC gadget depends on this behaviour. It simply provides >>>>>>>>>>>> new requests >>>>>>>>>>>> when video frames are available and assumes that they are sent >>>>>>>>>>>> as soon as >>>>>>>>>>>> possible. >>>>>>>>>>>> >>>>>>>>>>>> The dwc3 gadget currently works differently: It assumes that >>>>>>>>>>>> there is a >>>>>>>>>>>> contiguous stream of requests without any gaps. If a >>>>>>>>>>>> request is >>>>>>>>>>>> too late, >>>>>>>>>>>> then it is dropped by the hardware. >>>>>>>>>>>> For the UVC gadget this means that a live stream stops after >>>>>>>>>>>> the first >>>>>>>>>>>> frame because all following requests are late. >>>>>>>>>>> Can you explain little more how UVC gadget fails? >>>>>>>>>>> dwc3 controller expects a steady stream of data otherwise it >>>>>>>>>>> will result >>>>>>>>>>> in missed_isoc status, and it should be fine as long as new >>>>>>>>>>> requests are >>>>>>>>>>> queued. The controller doesn't just drop the request unless >>>>>>>>>>> there's some >>>>>>>>>>> other failure. >>>>>>>>>> UVC (with a live stream) does not fill the complete bandwidth >>>>>>>>>> of an >>>>>>>>>> isochronous endpoint. Let's assume for the example that one >>>>>>>>>> video >>>>>>>>>> frame >>>>>>>>>> fills 3 requests. Because it is a live stream, there will be a >>>>>>>>>> gap between >>>>>>>>>> video frames. This is unavoidable, especially for compressed >>>>>>>>>> video. So the >>>>>>>>>> UVC gadget will have requests for the frame numbers 1 2 3 5 6 >>>>>>>>>> 7 9 >>>>>>>>>> 10 11 13 14 >>>>>>>>>> 15 and so on. >>>>>>>>>> The dwc3 hardware tries to send those with frame numbers 1 2 3 4 >>>>>>>>>> 5 6 7 8 9 >>>>>>>>>> 10 11 12. So except for the fist few requests, all are late and >>>>>>>>>> result in a >>>>>>>>>> missed_isoc. I tried to just ignore the missed_isoc but that did >>>>>>>>>> not work >>>>>>>>>> for me. I only received the first frame at the other end. >>>>>>>>>> Maybe I missing something here, i don't have access to the >>>>>>>>>> hardware >>>>>>>>>> documentation, so I can only guess from the existing driver. >>>>>>>> The reason I asked is because your patch doesn't seem to >>>>>>>> address the >>>>>>>> actual issue. >>>>>>>> >>>>>>>> For the 2 checks you do here >>>>>>>> 1. There are currently no requests queued in the hardware >>>>>>>> 2. The current frame number provided by DSTS does not match the >>>>>>>> frame >>>>>>>> number returned by the last transfer. >>>>>>>> >>>>>>>> For #1, it's already done in the dwc3 driver. (check >>>>>>>> dwc3_gadget_endpoint_transfer_in_progress()) >>>>>>> But that's only after a isoc_missed occurred. What exactly does >>>>>>> that >>>>>>> mean? >>>>>>> Was the request transferred or not? My tests suggest that it was >>>>>>> not >>>>>>> transferred, so I wanted to catch this before it happens. >>>>>> >>>>>> Missed_isoc status means that the controller did not move all the >>>>>> data >>>>>> in an interval. >>>>> >>>>> I read in some Processor documentation that in case the host tries to >>>>> fetch data from the client and no active TRB (HWO=1) is available the >>>>> XferInProgress Interrupt will be produced, with the missed status >>>>> set. >>>>> This is done because the hardware will produce zero length packets >>>>> on its own, to keep the stream running. >>>> >>>> The controller only generates XferInProgress if it had processed a TRB >>>> (with specific control bits). For IN direction, if the controller is >>>> starved of TRB, it will send a ZLP if the host requests for data. >>> >>> Which control bits are those? ISOC-First, Chain and Last bits? >>> >>> I see the Scatter-Gather preparation is using these pattern. >> >> The IOC bit. You can check the programming guide for more detail on how >> to setup the TRBs, but what we have in dwc3 is fine. > > OK. > >>>>>>>> For #2, it's unlikely that DSTS current frame number will match >>>>>>>> with the >>>>>>>> XferNotReady's frame number. So this check doesn't mean much. >>>>>>> The frame number is also updated for each "Transfer In Progress" >>>>>>> interrupt. >>>>>>> If they match, then there a new request can still be queued >>>>>>> successfully. >>>>>>> Without this I got unnecessary stop/start transfers in the middle >>>>>>> of a >>>>>>> video frame. But maybe something else was wrong here. I'd need to >>>>>>> recheck. >>>>>> >>>>>> The reason they may not match is 1) the frame_number is only updated >>>>>> after the software handles the XferInProgress interrupt. Depends on >>>>>> system latency, that value may not be updated at the time that we >>>>>> check >>>>>> the frame_number. >>>>>> 2) This check doesn't work if the service interval is greater than 1 >>>>>> uframe. That is, it doesn't have to match exactly the time to be >>>>>> consider not late. Though, the second reason can easily be fixed. >>>>> >>>>> In the empty trb case, after the Hardware has send enough zero >>>>> packets >>>>> this >>>>> active transfer has to be stopped with endtransfer cmd. Because every >>>>> next >>>>> update transfer on that active transfer will likely lead to further >>>>> missed >>>>> transfers, as the newly updated trb will be handled to late anyway. >>>> >>>> The controller is expecting the function driver to feed TRBs to the >>>> controller for every interval. If it's late, then the controller will >>>> consider that data "missed_isoc". >>>> >>>> In your case, the UVC driver seems to queue requests to the controller >>>> driver as if it is bulk requests, and the UVC expects those data to go >>>> out at the time it queues. To achieve what UVC needs, then you may >>>> want >>>> to issue END_TRANSFER command before the next burst of data. This way, >>>> the controller can restart the isoc endpoint and not consider the next >>>> video frame data late. There are some corner cases that you need to >>>> watch out for. If you're going for this route, we can look further. >>> >>> Right, for now the drivers is doing: >>> >>> - Strart Transfer (with the right frame number) once. >>> >>> - Update Transfer On every ep_queue with the corresponding TRB >>> setting CHN = 0, IOC = 1, First-ISOC = 1 >>> >>> - End Transfer is somehow not handled right for this case. >>> >>> See my first comment. I think dwc3_prepare_one_trb_sg does the proper >>> chain >>> handling already. >>> >>>> Also, you'd need provide a way for the UVC to communicate to the >>>> dwc3 to >>>> let it know to expect the next burst of data. >>> >>> Can the UVC not just enqueue one big sg-request, which will represent >>> one burst >>> and not one TRB. Also that is what the SG code already seem to handle >>> right. >> >> Do you need SG? What size does video class driver setup its isoc URB? If >> it's superspeed, max is 48K, and depending on the type of platform >> you're running UVC on, you can setup a single 48K buffer per request if >> you want to do that. However, it's probably not a good idea since many >> host controllers can't even handle even close to 48K/uframe. > > We wan't to transfer uncached 4K Video frames via UVC. So Scatter-Gather > is a must anyway. > >> What I was saying is if UVC knows when to wait for the next video data, >> it can tell dwc3 to stop the isoc endpoint before queuing the next video >> data in a set of requests. If UVC doesn't know that, then it needs to >> tell dwc3 to change its handling of isoc requests. > > In function/gadget/uvc_video.c we take a buffer and pump it into many > available requests as we find. On the way the driver can drain that > video frame queue, in that case we could stop the transfer. I have > prepared a patch where we have only one source, queueing new requests, > so this could work. > > For now to limit the interrupt load on the dwc3 driver we already set > the request no_interrupt on every nth request, and make sure the > interrupt is on the very last one of the frame. But with that we still > sometimes run into missed transfers as the queued trb list somehow ends > up with a TRB not having the IOC bit set. > >>>>> The odd thing here is, that I don't see the refered XferInProgress >>>>> Interrupts with the missed event, when the started_list is empty. >>>> >>>> See my first comment. >>>> >>>>> >>>>> But this would be the only case to fall into this condition and >>>>> handle it >>>>> properly. Like alredy assumed in the following code: >>>>> >>>>> static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep >>>>> *dep, >>>>> const struct dwc3_event_depevt *event) >>>>> { >>>>> ... >>>>> >>>>> if (event->status & DEPEVT_STATUS_MISSED_ISOC) { >>>>> status = -EXDEV; >>>>> >>>>> if (list_empty(&dep->started_list)) >>>>> stop = true; >>>>> } >>>>> >>>>> ... >>>>> >>>>> if (stop) >>>>> dwc3_stop_active_transfer(dep, true, true); >>>>> ... >>>>> } >>>>> >>>>> In fact I did sometimes see these XferInProgress Interrupts on empty >>>>> trb queue >>>>> after I stoped the tansfer when the started_list was empty right >>>>> after >>>>> ep_cleanup_completed_requests has moved all trbs out of the queue. >>>>> >>>>> These Interrupts appeared right after the ENDTRANSFER cmd was send. >>>>> (But I >>>>> could no verify this every time) >>>> >>>> If END_TRANSFER command completes, then you should not see >>>> XferInProgress event. The next event should ber XferNotReady. >>> >>> Right. This also stops, after the Command Complete Interrupt arrives. >>> >>>>> Anyways in that case these Interrupts are not useful anymore, as I >>>>> already >>>>> implied the same stop, with ENDTRANSFER after we know that there are >>>>> no other >>>>> trbs in the chain. >>>>> >>>> >>>> Just curious, do you know if there's a reason for UVC to behave this >>>> way? Seems like what it's trying to do is more for bulk. Maybe it >>>> wants >>>> bandwidth priority perhaps? >>> >>> I don't know, probably it was more likely for USB 2.0 controllers to >>> be handled >>> this way. >>> >>> As mentioned the current uvc code is also working when we add this >>> changes. >>> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>> index ec357f64f319..a5dc44f2e9d8 100644 >>> --- a/drivers/usb/dwc3/gadget.c >>> +++ b/drivers/usb/dwc3/gadget.c >>> @@ -2629,6 +2629,9 @@ static void >>> dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >>> >>> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); >>> >>> + if (list_empty(&dep->started_list)) >>> + stop = true; >>> + >> >> You should check the pending list too and either drop them or prepare >> those requests (maybe too late). This happens when there's no available >> TRB but UVC queues more requests. >> Also, make sure this change only applies for isoc. > > I will send the patches for that so we can discuss the right handling > for that in a separate thread. Sure, we can review further then. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2020-04-21 23:41 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-11 15:26 [PATCH 0/2] usb: dwc3: gadget: improve isoc handling Michael Olbrich 2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich 2019-11-12 20:41 ` kbuild test robot 2019-11-13 3:55 ` Thinh Nguyen 2019-11-13 8:02 ` Michael Olbrich 2019-11-13 19:40 ` Thinh Nguyen 2019-11-11 15:26 ` [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late Michael Olbrich 2019-11-13 3:55 ` Thinh Nguyen 2019-11-13 7:53 ` Michael Olbrich 2019-11-13 15:39 ` Alan Stern 2019-11-13 19:14 ` Thinh Nguyen 2019-11-14 12:14 ` Michael Olbrich 2019-11-14 20:11 ` Thinh Nguyen 2019-11-15 21:06 ` Alan Stern 2019-11-28 11:36 ` Michael Olbrich 2019-11-28 17:59 ` Alan Stern 2019-12-02 15:41 ` Michael Olbrich 2019-12-02 17:02 ` Alan Stern 2020-04-09 7:59 ` Michael Grzeschik 2020-04-10 1:09 ` Thinh Nguyen 2020-04-10 22:03 ` Michael Grzeschik 2020-04-11 0:59 ` Thinh Nguyen 2020-04-21 6:51 ` Michael Grzeschik 2020-04-21 23:41 ` Thinh Nguyen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).