Message-ID: <377a3b28-bec6-3922-889a-94614277d390@rock-chips.com>
Date:   Mon, 7 May 2018 21:28:26 +0800
From:   wlf <wulf@...k-chips.com>
To:     Allen Hsu (許嘉銘) <Allen.Hsu@...ntatw.com>,
        Doug Anderson <dianders@...gle.com>
Cc:     William Wu <william.wu@...k-chips.com>,
        "hminas@...opsys.com" <hminas@...opsys.com>,
        "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
        Heiko Stübner <heiko@...ech.de>,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Frank Wang <frank.wang@...k-chips.com>,
        黄涛 <huangtao@...k-chips.com>,
        "daniel.meng" <daniel.meng@...k-chips.com>,
        John Youn <John.Youn@...opsys.com>,
        Claud Chang (張恭築) 
        <Claud.Chang@...ntatw.com>,
        王征增 <wzz@...k-chips.com>,
        "zsq@...k-chips.com" <zsq@...k-chips.com>,
        Stan Tsui <StanTsui@...en.com>
Subject: Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split
 in

Dear Doug,

在 2018年05月07日 15:19, Allen Hsu (許嘉銘) 写道:
> Add more:
>
>
> Best regards,
> BU4 EE
> Allen Hsu
> QCI 886-3-327-2345 ext 15410
>
> -----Original Message-----
> From: Doug Anderson [mailto:dianders@...gle.com]
> Sent: Friday, May 4, 2018 11:58 PM
> To: wlf <wulf@...k-chips.com>
> Cc: William Wu <william.wu@...k-chips.com>; hminas@...opsys.com; felipe.balbi@...ux.intel.com; Greg Kroah-Hartman <gregkh@...uxfoundation.org>; Sergei Shtylyov <sergei.shtylyov@...entembedded.com>; Heiko Stübner <heiko@...ech.de>; LKML <linux-kernel@...r.kernel.org>; linux-usb@...r.kernel.org; open list:ARM/Rockchip SoC... <linux-rockchip@...ts.infradead.org>; Frank Wang <frank.wang@...k-chips.com>; 黄涛 <huangtao@...k-chips.com>; daniel.meng <daniel.meng@...k-chips.com>; John Youn <John.Youn@...opsys.com>; 王征增 <wzz@...k-chips.com>; zsq@...k-chips.com; Allen Hsu (許嘉銘) <Allen.Hsu@...ntatw.com>; Stan Tsui <StanTsui@...en.com>
> Subject: Re: [PATCH v2 1/2] usb: dwc2: alloc dma aligned buffer for isoc split in
>
> Hi,
>
> On Wed, May 2, 2018 at 10:14 AM, wlf <wulf@...k-chips.com> wrote:
>> It's a good way to allocate an extra 3 bytes in the original bounce
>> buffer for this unaligned
>> issue， it's similar to the tailroom of sk_buff. However, just as you
>> said, we'd better find the special cases where we need an oversized
>> bounce buffer, otherwise，we
>> need to allocate
>> a bounce buffer for all of urbs.
>>
>> It's hard for me to know the special cases in the
>> dwc2_alloc_dma_aligned_buffer(), because it's called from
>> usb_submit_urb() in the device class driver, and I hardly know the
>> split state in this process, much less if the split transaction need
>> aligned buffer. Do you have any idea?
>>
>> I suppose that we can't find the special cases where we need an
>> oversized bounce buffer in the dwc2_alloc_dma_aligned_buffer(), and if
>> we still want to re-use the original bounce buffer with extra 3 bytes,
>> then we need to allocate a  bounce buffer for all of urbs, and do
>> unnecessary  data copy for these urbs  whose transfer_buffer were
>> already aligned.  This may reduce the transmission rate of USB.
>>
>> Can we just pre-allocate an additional aligned buffer (the size is 200
>> bytes) for split transaction
>> in dwc2_map_urb_for_dma for all of urbs. And if we find the split
>> transaction is unaligned，
>> we can easily use the pre-allocated aligned buffer.
> OK, so thinking about this more...
>
> Previously things got really slow at interrupt time because we were trying to allocate as much as 64K at interrupt time.  That wasn't so great.  In your case, you're only allocating 200 bytes.  As I understand things, allocating 200 bytes at interrupt time is probably not a huge deal.
>
> ...so I guess it come down to a tradeoff here: is it worth eating 200 bytes for each URB to save an 200 byte allocation at interrupt time in this one rare case.
>
> I'd certainly welcome anyone's opinion here, but I'm going to go with saying it's fine to allocate the 200 bytes at interrupt time (like your patch does).  ...but, I _think_ you want to use
> kmem_cache_create() to create a cache and then kmem_cache_zalloc().
> Since all allocations are the same size and you want this to be fast, I think using kmem_cache is good.
Yes, it seems good to use kmem_cache. I'm trying to test a new patch 
with kmem_cache, and I will upstream v3 patch as soon as possible.
>>>> +       /* For non-dword aligned buffers */
>>>> +       if (hsotg->params.host_dma > 0 && qh->do_split &&
>>>> +           chan->ep_is_in && (chan->xfer_dma & 0x3)) {
>>> So what happens if were unaligned (AKA (chan->xfer_dma & 0x3)) but
>>> we're not doing split or it's not an IN EP?  Do we just fail then?
>>>
>>> I guess the rest of this patch only handles the "in" case and maybe
>>> you expect that the problems will only come about for do_split, but
>>> it still might be wise to at least print a warning in the other cases?
>>> >From reading dwc2_hc_init_xfer() it seems like you could run into 
>>>> this
>>> same problem in the "out" case?
>> Actually, I only find non-dword aligned issue in the case of split in
>> transaction.
>> And I think that if we're not doing split or it's an OUT EP, we can
>> always get aligned buffer in the current code. For non-split case, the
>> dwc2_alloc_dma_aligned_buffer() is enough. And for split out case, if
>> the transaction is subdivided into multiple start-splits, each with a
>> data payload of 188 bytes or less, so the DMA address is always
>> aligned.
> Can you at least print an error message if you end up with non-aligned DMA in one of the other cases?
That's an excellent suggestion. I will add warning message if end up 
with unexpected non-aligned DMA.
>>>> DMA_FROM_DEVICE);
>>>> +               memcpy(qtd->urb->buf + frame_desc->offset +
>>>> +                      qtd->isoc_split_offset,
>>>> + chan->qh->dw_align_buf,
>>>> len);
>>> Assuming I'm understanding this patch correctly, I think it would be
>>> better to write:
>>>
>>>     memcpy(qtd->xfer_dma, chan->qh->dw_align_buf, len);
>> Sorry, there's no "xfer_buf" in qtd, do you means the
>> "chan->xfer_dma"? If it's, I think we can't do memcpy from a transfer
>> buffer to a DMA address. Maybe chan->xfer_buf is more suitable, but it
>> seems that the dwc2 driver doesn't update the chan->xfer_buf for isoc
>> transfer with dma enabled in dwc2_hc_init_xfer().
> Yes, I meant chan->xfer_dma.  Ah, right.  xfer_dma is a DMA address.
>
> I guess you could in theory you could do:
>
> memcpy(qtd->urb->buf + (chan->xfer_dma - urb->dma),
>      chan->qh->dw_align_buf);
>
> That at least avoids duplicating the math.  Maybe either do that, or if you don't like it at least add a comment saying that the math needs to match the math in dwc2_hc_init_xfer().
Thanks, I will fix it in V3 patch according to your suggestion.
>>> Then if you ever end up having to align a transfer other than a split
>>> you won't be doing the wrong math.  As it is it's very non-obvious
>>> that you're hardcoding the same formula that's in dwc2_hc_init_xfer()
>> Actually, I'm hardcoding the same formula from the old code which has
>> been ripped out in the commit 3bc04e28a030 ("usb: dwc2: host: Get
>> aligned DMA in a more supported way").
> Ah, got it.  Well, I think the old code was just hardcoding the same formula in two places then.  ;-)
>
>
>>>> -       if (qh->desc_list)
>>>> +       if (qh->desc_list) {
>>>>                   dwc2_hcd_qh_free_ddma(hsotg, qh);
>>>> +       } else {
>>>> +               /* kfree(NULL) is safe */
>>>> +               kfree(qh->dw_align_buf);
>>>> +               qh->dw_align_buf_dma = (dma_addr_t)0;
>>> Why assign qh->dw_align_buf_dma to 0?  The next thing you're doing is
>>> kfree(qh).  If you want extra debugging, turn on slub_debug.
>> I just copy the same code from the old code which has been ripped out
>> in the commit 3bc04e28a030 ("usb: dwc2: host: Get aligned DMA in a
>> more supported way").
>> I really don't know why assign qh->dw_align_buf_dma to 0.
> There's no reason.  Just remove it.
OK, thank you for reminding me!
> -Doug
Best regards,
         wulf

Powered by blists - more mailing lists