答复: [RFC PATCH v3 1/5] usb: xhci-mtk: improve bandwidth scheduling with multi-TT

["Yaqii Wu (武亚奇)" <Yaqii.Wu@mediatek.com>]

HI Chunfeng
	请回复谷歌以下三个问题：
	Queston: Are there any reasons for doing this? can only one split packet be scheduled in a u-frame for isochronous out? This looks like overkill.
	Answer: We will calculate the number of uframes based on the isoc out ep maxp. Except for the last uframe, all other consumed bandwidth is 188. But to be safe, we also count the bandwidth 	consumed by the last SS as 188.

	Queston: same here. It would be much better to understand if you can provide some counterexamples of schedule that can happen when this bitmap checking logic is omitted.
	Answer: Maybe you are right, it will indeed double check. I will delete the bandwidth check, and then perform a regression test.

	Queston: I guess this is enough to check the bandwidth limit of the lower speed bus without a bitmap.
	Answer: I explained to you last time that ss cannot be overlapped. This is a hub compatibility issue. Therefore we need bitmap to prevent ss overlap.
BR
yaqii

-----邮件原件-----
发件人: Ikjoon Jang [mailto:ikjn@chromium.org] 
发送时间: 2021年1月13日 17:44
收件人: Chunfeng Yun (云春峰) <Chunfeng.Yun@mediatek.com>
抄送: Yaqii Wu (武亚奇) <Yaqii.Wu@mediatek.com>; Zhanyong Wang (王战勇) <zhanyong.wang@mediatek.com>; Zhanyong Wang <zhangyong.wang@mediatek.com>; linux-usb@vger.kernel.org; open list <linux-kernel@vger.kernel.org>; Tianping Fang (方天平) <Tianping.Fang@mediatek.com>; moderated list:ARM/Mediatek SoC support <linux-mediatek@lists.infradead.org>
主题: Re: [RFC PATCH v3 1/5] usb: xhci-mtk: improve bandwidth scheduling with multi-TT

On Tue, Dec 22, 2020 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> From: Zhanyong Wang <zhanyong.wang@mediatek.com>
>
> After inserted the usb type-c 3.5mm dongle with headset, dmesg showed:
> usb 1-1.1: new full-speed USB device number 5 using xhci-mtk usb 
> 1-1.1: New USB device found, idVendor=05ac, idProduct=110a, 
> bcdDevice=26.11 usb 1-1.1: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3 usb 1-1.1: Product: USB-C to 3.5mm Headphone Jack 
> Adapter usb 1-1.1: Manufacturer: Apple, Inc.
> usb 1-1.1: SerialNumber: DWH915501TFJKLTAM xhci-mtk 11200000.xhci: Not 
> enough bandwidth!
> usb 1-1.1: can't set config #2, error -28
>
> improve low-speed/full-speed INT/ISOC bandwidth scheduling with USB 
> muli-TT.
>
> Signed-off-by: Yaqii Wu <yaqii.wu@mediatek.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2~v3: no changes
> ---
>  drivers/usb/host/xhci-mtk-sch.c | 91 ++++++++++++++++++++++++++++-----
>  drivers/usb/host/xhci-mtk.h     |  8 ++-
>  2 files changed, 84 insertions(+), 15 deletions(-)  mode change 
> 100644 => 100755 drivers/usb/host/xhci-mtk-sch.c
>
> diff --git a/drivers/usb/host/xhci-mtk-sch.c 
> b/drivers/usb/host/xhci-mtk-sch.c old mode 100644 new mode 100755 
> index 45c54d56ecbd..94292b9bbc63
> --- a/drivers/usb/host/xhci-mtk-sch.c
> +++ b/drivers/usb/host/xhci-mtk-sch.c
> @@ -383,7 +383,9 @@ static int check_sch_tt(struct usb_device *udev,
>         u32 fs_budget_start;
>         u32 start_ss, last_ss;
>         u32 start_cs, last_cs;
> -       int i;
> +       u32 num_esit, base;
> +       int i, j;
> +       u32 tmp;
>
>         start_ss = offset % 8;
>         fs_budget_start = (start_ss + 1) % 8; @@ -398,10 +400,13 @@ 
> static int check_sch_tt(struct usb_device *udev,
>                 if (!(start_ss == 7 || last_ss < 6))
>                         return -ERANGE;
>
> -               for (i = 0; i < sch_ep->cs_count; i++)
> -                       if (test_bit(offset + i, tt->split_bit_map))
> +               for (i = 0; i < sch_ep->cs_count; i++) {
> +                       if (test_bit(offset + i, tt->ss_bit_map))
>                                 return -ERANGE;
>
> +                       if (test_bit(offset + i, tt->cs_bit_map))
> +                               return -ERANGE;
> +               }

Are there any reasons for doing this?
Why can only one split packet be scheduled in a u-frame for isochronous out?
This looks like overkill.

>         } else {
>                 u32 cs_count = DIV_ROUND_UP(sch_ep->maxpkt, 
> FS_PAYLOAD_MAX);
>
> @@ -428,8 +433,10 @@ static int check_sch_tt(struct usb_device *udev,
>                 if (cs_count > 7)
>                         cs_count = 7; /* HW limit */
>
> -               for (i = 0; i < cs_count + 2; i++) {
> -                       if (test_bit(offset + i, tt->split_bit_map))
> +               if (test_bit(offset, tt->ss_bit_map))
> +                       return -ERANGE;
> +               for (i = 0; i < cs_count; i++) {
> +                       if (test_bit(offset + 2 + i, tt->cs_bit_map))
>                                 return -ERANGE;
>                 }
>

same here. It would be much better to understand if you can provide some counterexamples of schedule that can happen when this bitmap checking logic is omitted.

> @@ -445,11 +452,22 @@ static int check_sch_tt(struct usb_device *udev,
>                         sch_ep->num_budget_microframes = sch_ep->esit;
>         }
>
> +       num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
> +       for (i = 0; i < num_esit; i++) {
> +               base = sch_ep->offset + i * sch_ep->esit;
> +               for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> +                       tmp = tt->fs_bus_bw[base + j]
> +                             + sch_ep->bw_cost_per_microframe;
> +                       if (tmp > FS_PAYLOAD_MAX)
> +                               return -ERANGE;
> +               }
> +       }

I guess this is enough to check the bandwidth limit of the lower speed bus without a bitmap.

> +
>         return 0;
>  }
>
>  static void update_sch_tt(struct usb_device *udev,
> -       struct mu3h_sch_ep_info *sch_ep)
> +       struct mu3h_sch_ep_info *sch_ep, bool used)
>  {
>         struct mu3h_sch_tt *tt = sch_ep->sch_tt;
>         u32 base, num_esit;
> @@ -458,11 +476,52 @@ static void update_sch_tt(struct usb_device *udev,
>         num_esit = XHCI_MTK_MAX_ESIT / sch_ep->esit;
>         for (i = 0; i < num_esit; i++) {
>                 base = sch_ep->offset + i * sch_ep->esit;
> -               for (j = 0; j < sch_ep->num_budget_microframes; j++)
> -                       set_bit(base + j, tt->split_bit_map);
> +               for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> +                       if (used)
> +                               set_bit(base + j, tt->split_bit_map);
> +                       else
> +                               clear_bit(base + j, tt->split_bit_map);
> +               }
> +
> +               if (sch_ep->ep_type == ISOC_OUT_EP) {
> +                       for (j = 0; j < sch_ep->num_budget_microframes; j++) {
> +                               if (used) {
> +                                       set_bit(base + j, tt->ss_bit_map);
> +                                       set_bit(base + j, tt->cs_bit_map);
> +                                       tt->fs_bus_bw[base + j] +=
> +                                               sch_ep->bw_cost_per_microframe;
> +                               } else {
> +                                       clear_bit(base + j, tt->ss_bit_map);
> +                                       clear_bit(base + j, tt->cs_bit_map);
> +                                       tt->fs_bus_bw[base + j] -=
> +                                               sch_ep->bw_cost_per_microframe;
> +                               }
> +                       }
> +               } else {
> +                       if (used)
> +                               set_bit(base, tt->ss_bit_map);
> +                       else
> +                               clear_bit(base, tt->ss_bit_map);
> +
> +                       for (j = 0; j < sch_ep->cs_count; j++) {
> +                               if (used) {
> +                                       set_bit(base + 2 + j, 
> + tt->cs_bit_map);
> +
> +                                       tt->fs_bus_bw[base + 2 + j] +=
> +                                               sch_ep->bw_cost_per_microframe;
> +                               } else {
> +                                       clear_bit(base + 2 + j, tt->cs_bit_map);
> +                                       tt->fs_bus_bw[base + 2 + j] -=
> +                                               sch_ep->bw_cost_per_microframe;
> +                               }
> +                       }
> +               }
>         }
>
> -       list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> +       if (used)
> +               list_add_tail(&sch_ep->tt_endpoint, &tt->ep_list);
> +       else
> +               list_del(&sch_ep->tt_endpoint);
>  }
>
>  static int check_sch_bw(struct usb_device *udev, @@ -470,6 +529,7 @@ 
> static int check_sch_bw(struct usb_device *udev,  {
>         u32 offset;
>         u32 esit;
> +       u32 boundary;
>         u32 min_bw;
>         u32 min_index;
>         u32 worst_bw;
> @@ -487,10 +547,13 @@ static int check_sch_bw(struct usb_device *udev,
>          */
>         min_bw = ~0;
>         min_index = 0;
> +       boundary = esit;
>         min_cs_count = sch_ep->cs_count;
>         min_num_budget = sch_ep->num_budget_microframes;
>         for (offset = 0; offset < esit; offset++) {
>                 if (is_fs_or_ls(udev->speed)) {
> +                       if (sch_ep->ep_type != ISOC_OUT_EP)
> +                               boundary = esit + 1;
>                         ret = check_sch_tt(udev, sch_ep, offset);
>                         if (ret)
>                                 continue; @@ -498,7 +561,7 @@ static 
> int check_sch_bw(struct usb_device *udev,
>                                 tt_offset_ok = true;
>                 }
>
> -               if ((offset + sch_ep->num_budget_microframes) > sch_ep->esit)
> +               if ((offset + sch_ep->num_budget_microframes) > 
> + boundary)
>                         break;
>
>                 worst_bw = get_max_bw(sch_bw, sch_ep, offset); @@ 
> -532,7 +595,7 @@ static int check_sch_bw(struct usb_device *udev,
>                 if (!tt_offset_ok)
>                         return -ERANGE;
>
> -               update_sch_tt(udev, sch_ep);
> +               update_sch_tt(udev, sch_ep, 1);
>         }
>
>         /* update bus bandwidth info */ @@ -696,12 +759,12 @@ void 
> xhci_mtk_drop_ep_quirk(struct usb_hcd *hcd, struct usb_device *udev,
>
>         list_for_each_entry(sch_ep, &sch_bw->bw_ep_list, endpoint) {
>                 if (sch_ep->ep == ep) {
> -                       update_bus_bw(sch_bw, sch_ep, 0);
> -                       list_del(&sch_ep->endpoint);
>                         if (is_fs_or_ls(udev->speed)) {
> -                               list_del(&sch_ep->tt_endpoint);
> +                               update_sch_tt(udev, sch_ep, 0);
>                                 drop_tt(udev);
>                         }
> +                       update_bus_bw(sch_bw, sch_ep, 0);
> +                       list_del(&sch_ep->endpoint);
>                         kfree(sch_ep);
>                         break;
>                 }
> diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h 
> index a93cfe817904..323b281933b9 100644
> --- a/drivers/usb/host/xhci-mtk.h
> +++ b/drivers/usb/host/xhci-mtk.h
> @@ -21,12 +21,18 @@
>
>  /**
>   * @split_bit_map: used to avoid split microframes overlay
> + * @ss_bit_map: used to avoid start split microframes overlay
> + * @cs_bit_map: used to avoid complete split microframes overlay
> + * @fs_bus_bw: array to keep track of bandwidth already used at full 
> + speed
>   * @ep_list: Endpoints using this TT
>   * @usb_tt: usb TT related
>   * @tt_port: TT port number
>   */
>  struct mu3h_sch_tt {
>         DECLARE_BITMAP(split_bit_map, XHCI_MTK_MAX_ESIT);
> +       DECLARE_BITMAP(ss_bit_map, XHCI_MTK_MAX_ESIT);
> +       DECLARE_BITMAP(cs_bit_map, XHCI_MTK_MAX_ESIT + 1);
> +       u32 fs_bus_bw[XHCI_MTK_MAX_ESIT];
>         struct list_head ep_list;
>         struct usb_tt *usb_tt;
>         int tt_port;
> @@ -42,7 +48,7 @@ struct mu3h_sch_tt {
>   * two bandwidth domains, one for IN eps and another for OUT eps.
>   */
>  struct mu3h_sch_bw_info {
> -       u32 bus_bw[XHCI_MTK_MAX_ESIT];
> +       u32 bus_bw[XHCI_MTK_MAX_ESIT + 1];
>         struct list_head bw_ep_list;
>  };
>
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek