Re: [PATCH v4 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource

[Sanjay R Mehta <sanmehta@amd.com>]

On 5/4/2020 11:44 AM, Vinod Koul wrote:
> [CAUTION: External Email]
> 
> On 28-04-20, 16:13, Sanjay R Mehta wrote:
> 
>> +static void pt_do_cmd_complete(unsigned long data)
>> +{
>> +     struct pt_tasklet_data *tdata = (struct pt_tasklet_data *)data;
>> +     struct pt_cmd *cmd = tdata->cmd;
>> +     struct pt_cmd_queue *cmd_q = &cmd->pt->cmd_q;
>> +     u32 tail;
>> +
>> +     tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
>> +     if (cmd_q->cmd_error) {
>> +            /*
>> +             * Log the error and flush the queue by
>> +             * moving the head pointer
>> +             */
>> +             pt_log_error(cmd_q->pt, cmd_q->cmd_error);
>> +             iowrite32(tail, cmd_q->reg_head_lo);
>> +     }
>> +
>> +     cmd->pt_cmd_callback(cmd->data, cmd->ret);
> 
> So in the isr you schedule this tasklet and this invokes the calback..
> this is very inefficient.
> 
> You should submit the next txn to dmaengine in your isr, keeping the dma
> idle at this point is very inefficient.
> 
Sure, will incorporate the changes in the next version of patch.

>> +static void pt_cmd_callback(void *data, int err)
>> +{
>> +     struct pt_dma_desc *desc = data;
>> +     struct pt_dma_chan *chan;
>> +     int ret;
> 
> This is called as callback from pt layer..
Right.

>> +
>> +     if (err == -EINPROGRESS)
>> +             return;
>> +
>> +     chan = container_of(desc->vd.tx.chan, struct pt_dma_chan,
>> +                         vc.chan);
>> +
>> +     dev_dbg(chan->pt->dev, "%s - tx %d callback, err=%d\n",
>> +             __func__, desc->vd.tx.cookie, err);
>> +
>> +     if (err)
>> +             desc->status = DMA_ERROR;
>> +
>> +     while (true) {
>> +             /* Check for DMA descriptor completion */
>> +             desc = pt_handle_active_desc(chan, desc);
>> +
>> +             /* Don't submit cmd if no descriptor or DMA is paused */
>> +             if (!desc || chan->status == DMA_PAUSED)
>> +                     break;
>> +
>> +             ret = pt_issue_next_cmd(desc);
> 
> And you call this to issue next cmd... The missing thing I am seeing
> here is vchan_cookie_complete() you need to call that here for correct
> vchan list mgmt
> 
Here before making call to issue next cmd, "vchan_vdesc_fini()" is been used in place of
vchan_cookie_complete() in the pt_handle_active_desc() function.


>> +static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>> +                                       struct scatterlist *dst_sg,
>> +                                         unsigned int dst_nents,
>> +                                         struct scatterlist *src_sg,
>> +                                         unsigned int src_nents,
>> +                                         unsigned long flags)
> 
> unaligned add indentation! Pls run checkpatch --strict to check for
> these oddities
> 
Sure, will incorporate the changes in the next version of patch.

>> +     dma_dev->dev = pt->dev;
>> +     dma_dev->src_addr_widths = PT_DMA_WIDTH(dma_get_mask(pt->dev));
>> +     dma_dev->dst_addr_widths = PT_DMA_WIDTH(dma_get_mask(pt->dev));
>> +     dma_dev->directions = DMA_MEM_TO_MEM;
>> +     dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>> +     dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
>> +
>> +     INIT_LIST_HEAD(&dma_dev->channels);
>> +
>> +     chan = pt->pt_dma_chan;
>> +     chan->pt = pt;
>> +     dma_chan = &chan->vc.chan;
>> +
>> +     dma_dev->device_free_chan_resources = pt_free_chan_resources;
>> +     dma_dev->device_prep_dma_memcpy = pt_prep_dma_memcpy;
>> +     dma_dev->device_prep_dma_interrupt = pt_prep_dma_interrupt;
>> +     dma_dev->device_issue_pending = pt_issue_pending;
>> +     dma_dev->device_tx_status = pt_tx_status;
>> +     dma_dev->device_pause = pt_pause;
>> +     dma_dev->device_resume = pt_resume;
>> +     dma_dev->device_terminate_all = pt_terminate_all;
> 
> Pls implement .device_synchronize as well
> 
Sure, will incorporate the changes in the next version of patch.

>> +struct pt_dma_desc {
>> +     struct virt_dma_desc vd;
>> +
>> +     struct pt_device *pt;
>> +
>> +     struct list_head pending;
>> +     struct list_head active;
> 
> why not use vc->desc_submitted, desc_issued instead!
> 
Sure, will incorporate the changes in the next version of patch.

>> +
>> +     enum dma_status status;
>> +
>> +     size_t len;
>> +};
>> +
>> +struct pt_dma_chan {
>> +     struct virt_dma_chan vc;
>> +
>> +     struct pt_device *pt;
>> +
>> +     enum dma_status status;
> 
> channel status as well as desc, why do you need both?
You are right. will remove channel status from here.

> --
> ~Vinod
>