Re: [PATCH 2/2] dmaengine: mediatek: Add MediaTek Command-Queue DMA controller for MT6765 SoC

Shun-Chih.Yu
2019-01-08T12:19:44Z

On Fri, 2019-01-04 at 18:08 +0530, Vinod Koul wrote:
> On 27-12-18, 21:10, shun-chih.yu@mediatek.com wrote:
> > From: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> >
>
> This should be v4 of the patch series, why is it not tagged so?
I would tag properly in the next revision.

> > MediaTek Command-Queue DMA controller (CQDMA) on MT6765 SoC is dedicated
> > to memory-to-memory transfer through queue based descriptor management.
>
> Have you tested this with dmatest, if so can you provide results of the
> test as well.
Yes, I tested with dmatest in multi-thread version.
The results shown below, and I would attach them in the next revision if needed.

dmatest: dma0chan0-copy2: summary 5000 tests, 0 failures 3500 iops 28037
KB/s (0)
dmatest: dma0chan0-copy4: summary 5000 tests, 0 failures 3494 iops 27612
KB/s (0)
dmatest: dma0chan0-copy1: summary 5000 tests, 0 failures 3491 iops 27749
KB/s (0)
dmatest: dma0chan0-copy7: summary 5000 tests, 0 failures 3673 iops 29092
KB/s (0)
dmatest: dma0chan0-copy6: summary 5000 tests, 0 failures 3763 iops 30237
KB/s (0)
dmatest: dma0chan0-copy0: summary 5000 tests, 0 failures 3730 iops 30131
KB/s (0)
dmatest: dma0chan0-copy3: summary 5000 tests, 0 failures 3717 iops 29569
KB/s (0)
dmatest: dma0chan0-copy5: summary 5000 tests, 0 failures 3699 iops 29302
KB/s (0)

> > There are only 3 physical channels inside CQDMA, while the driver is
> > extended to support 32 virtual channels for multiple dma users to issue
> > dma requests onto the CQDMA simultaneously.
> >
> > Change-Id: I1e8d116c5ecbbc49190ffc925cb59a0d035d886b
>
> Please remove this from upstream, checkpatch would have warned that!
I would remove it and be more careful about this.

> > Signed-off-by: Shun-Chih Yu <shun-chih.yu@mediatek.com>
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
>
> This is _WRONG_ I have never provided such tag, can you explain why this
> was added without my approval?
So sorry about this, I misunderstood the usage of reviewed-by tag and I
would remove this. Thanks for pointing out this mistake.

> > This controller provides the channels which is dedicated to
> > memory-to-memory transfer to offload from CPU through ring-
> > based descriptor management.
> > +
> > +config MTK_CQDMA
> > + tristate "MediaTek Command-Queue DMA controller support"
>
> Am not sure if I asked this earlier but, what is difference with HSDMA?
They are different in hardware design.
For example, HSDMA has ring-based descriptor management, while CQDMA only has
one descriptor.

> > +/**
> > + * struct mtk_cqdma_pchan - The struct holding info describing physical
> > + * channel (PC)
> > + * @queue: Queue for the CVDs issued to this PC
> > + * @base: The mapped register I/O base of this PC
> > + * @irq: The IRQ that this PC are using
> > + * @refcnt: Track how many VCs are using this PC
> > + * @lock: Lock protect agaisting multiple VCs access PC
>
> Please maintain alignment!
Sure, I would fix this.

> > + */
> > +struct mtk_cqdma_pchan {
> > + struct list_head queue;
> > + void __iomem *base;
> > + u32 irq;
> > + refcount_t refcnt;
> > +
> > + /* lock to protect PC */
>
> This is not required, you already have above !
Sure, I would remove this.

> > + spinlock_t lock;
> > +};
> > +
> > +/**
> > + * struct mtk_cqdma_vchan - The struct holding info describing virtual
> > + * channel (VC)
> > + * @vc: An instance for struct virt_dma_chan
> > + * @pc: The pointer to the underlying PC
> > + * @issue_completion: The wait for all issued descriptors completited
>
> typo completited , am not sure why you need this
>
This completion should be redundant and would be removed in the next
revision.

> > +static void mtk_dma_write(struct mtk_cqdma_pchan *pc, u32 reg, u32 val)
> > +{
> > + writel_relaxed(val, pc->base + reg);
>
> Why is it relaxed one?
Most of the operations to the CQDMA hardware could be relaxed, and the

> > +static void mtk_cqdma_consume_work_queue(struct mtk_cqdma_pchan *pc)
> > +{
> > + struct mtk_cqdma_vchan *cvc;
> > + struct mtk_cqdma_vdesc *cvd;
> > + size_t tlen;
> > +
> > + /* consume a CVD from PC's queue */
> > + cvd = list_first_entry_or_null(&pc->queue,
> > + struct mtk_cqdma_vdesc, node);
>
> you can use vchan_next_desc() and also remove your own queue as
> virt-desc already implements that logic!
This logic would be simplified and the queue would be removed in next revision.

> > + if (unlikely(!cvd))
> > + return;
> > +
> > + cvc = to_cqdma_vchan(cvd->ch);
> > +
> > + tlen = (cvd->len < MTK_CQDMA_MAX_LEN) ? cvd->len : MTK_CQDMA_MAX_LEN;
> > + cvd->len -= tlen;
> > + cvd->src += tlen;
> > + cvd->dest += tlen;
> > +
> > + /* check whether the CVD completed */
> > + if (!cvd->len) {
> > + /* delete CVD from PC's queue */
> > + list_del(&cvd->node);
> > +
> > + spin_lock(&cvc->vc.lock);
> > +
> > + /* add the VD into list desc_completed */
> > + vchan_cookie_complete(&cvd->vd);
> > +
> > + /* setup completion if this VC is under synchronization */
> > + if (cvc->issue_synchronize && !mtk_cqdma_is_vchan_active(cvc)) {
> > + complete(&cvc->issue_completion);
> > + cvc->issue_synchronize = false;
> > + }
>
> why do you need your own completion?
>
This completion should be redundant and would be removed in the next
revision.
> > +
> > + spin_unlock(&cvc->vc.lock);
> > + }
> > +
> > + /* iterate on the next CVD if the current CVD completed */
> > + if (!cvd->len)
> > + cvd = list_first_entry_or_null(&pc->queue,
> > + struct mtk_cqdma_vdesc, node);
> > +
> > + /* start the next transaction */
> > + if (cvd)
> > + mtk_cqdma_start(pc, cvd);
>
> most of this logic looks reduandant to me. Virt-dma was designed to do
> exactly this, have N physical channels and share with M virt channels.
> Please reuse and remove code from this driver.
>
I would re-factor this part for reuse on virt-dma and remove redundant
code.
> > +static struct virt_dma_desc *mtk_cqdma_find_active_desc(struct dma_chan *c,
> > + dma_cookie_t cookie)
> > +{
> > + struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > + struct virt_dma_desc *vd;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&cvc->pc->lock, flags);
> > + list_for_each_entry(vd, &cvc->pc->queue, node)
> > + if (vd->tx.cookie == cookie) {
> > + spin_unlock_irqrestore(&cvc->pc->lock, flags);
> > + return vd;
> > + }
> > + spin_unlock_irqrestore(&cvc->pc->lock, flags);
> > +
> > + list_for_each_entry(vd, &cvc->vc.desc_issued, node)
> > + if (vd->tx.cookie == cookie)
> > + return vd;
>
> vchan_find_desc() ??
>
This logic could be replaced with vchan_find_desc(), and I will fix in
the next revision.
> > +static enum dma_status mtk_cqdma_tx_status(struct dma_chan *c,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_cqdma_vchan *cvc = to_cqdma_vchan(c);
> > + struct mtk_cqdma_vdesc *cvd;
> > + struct virt_dma_desc *vd;
> > + enum dma_status ret;
> > + unsigned long flags;
> > + size_t bytes = 0;
> > +
> > + ret = dma_cookie_status(c, cookie, txstate);
> > + if (ret == DMA_COMPLETE || !txstate)
> > + return ret;
> > +
> > + spin_lock_irqsave(&cvc->vc.lock, flags);
> > + vd = mtk_cqdma_find_active_desc(c, cookie);
> > + spin_unlock_irqrestore(&cvc->vc.lock, flags);
> > +
> > + if (vd) {
> > + cvd = to_cqdma_vdesc(vd);
> > + bytes = cvd->len;
> > + }
> > +
> > + dma_set_residue(txstate, bytes);
>
> Have you tested this and are able to report residue properly?
>
I tested and thought the residue report properly. But after checking the
definition of residue in tx_status again, I found that should be always
return 0 in the driver instead, since there is no state DMA_IN_PROGRESS
or DMA_PAUSED in the driver. I would fix this in the next revision.

> > +static void mtk_cqdma_free_inactive_desc(struct dma_chan *c)
> > +{
> > + struct virt_dma_chan *vc = to_virt_chan(c);
> > + unsigned long flags;
> > + LIST_HEAD(head);
> > +
> > + /*
> > + * set desc_allocated, desc_submitted,
> > + * and desc_issued as the candicates to be freed
> > + */
> > + spin_lock_irqsave(&vc->lock, flags);
> > + list_splice_tail_init(&vc->desc_allocated, &head);
> > + list_splice_tail_init(&vc->desc_submitted, &head);
> > + list_splice_tail_init(&vc->desc_issued, &head);
>
> You missed completed and didnt use vchan_get_all_descriptors() ??
>
I missed desc_completed when I tried to move desc_completed into
mtk_cqdma_free_active_desc, yet this logic should be simplified in the
next revision. The separation on free_active_desc and free_inactive_desc
seems no necessary, and I would merge them and use
vchan_get_all_descriptors only.
>
> Looking at the driver, I feel things have been complicated a bit, you
> can reuse more code and routines in vchan layer and remove driver
> handling and make things with less bugs (used more tested generic code)
> and simpler to review ..
>
> Please do so in next rev and tag version properly!
>
Yes, the current implementation should be further simplified and
leverage much more code and routines in vchan layer, I am working on the
next patch and making sure all of them fixed in the next revision.
Thanks for your valuable review and sorry for the misuse on the review
tag,