Re: [PATCH v2] dmaengine: qcom-bam: Process multiple pending descriptors

[Sricharan R <sricharan@codeaurora.org>]

Hi Abhishek,

On 8/9/2017 7:48 PM, Abhishek Sahu wrote:
> On 2017-08-03 18:51, Sricharan R wrote:
>> The bam dmaengine has a circular FIFO to which we
>> add hw descriptors that describes the transaction.
>> The FIFO has space for about 4096 hw descriptors.
>>
>> Currently we add one descriptor and wait for it to
>> complete with interrupt and then add the next pending
>> descriptor. In this way, the FIFO is underutilized
>> since only one descriptor is processed at a time, although
>> there is space in FIFO for the BAM to process more.
>>
>> Instead keep adding descriptors to FIFO till its full,
>> that allows BAM to continue to work on the next descriptor
>> immediately after signalling completion interrupt for the
>> previous descriptor.
>>
>> Also when the client has not set the DMA_PREP_INTERRUPT for
>> a descriptor, then do not configure BAM to trigger a interrupt
>> upon completion of that descriptor. This way we get a interrupt
>> only for the descriptor for which DMA_PREP_INTERRUPT was
>> requested and there signal completion of all the previous completed
>> descriptors. So we still do callbacks for all requested descriptors,
>> but just that the number of interrupts are reduced.
>>
>> CURRENT:
>>
>>             ------      -------   ---------------
>>             |DES 0|     |DESC 1|  |DESC 2 + INT |
>>             ------      -------   ---------------
>>                |           |            |
>>                |           |            |
>> INTERRUPT:   (INT)       (INT)          (INT)
>> CALLBACK:     (CB)        (CB)         (CB)
>>
>>         MTD_SPEEDTEST READ PAGE: 3560 KiB/s
>>         MTD_SPEEDTEST WRITE PAGE: 2664 KiB/s
>>         IOZONE READ: 2456 KB/s
>>         IOZONE WRITE: 1230 KB/s
>>
>>     bam dma interrupts (after tests): 96508
>>
>> CHANGE:
>>
>>         ------  -------    -------------
>>         |DES 0| |DESC 1   |DESC 2 + INT |
>>         ------  -------   --------------
>>                 |
>>                 |
>>                         (INT)
>>                   (CB for 0, 1, 2)
>>
>>         MTD_SPEEDTEST READ PAGE: 3860 KiB/s
>>         MTD_SPEEDTEST WRITE PAGE: 2837 KiB/s
>>         IOZONE READ: 2677 KB/s
>>         IOZONE WRITE: 1308 KB/s
>>
>>     bam dma interrupts (after tests): 58806
>>
>> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> 
>  Thanks Sricharan for your patch to do the descriptor
>  clubbing in BAM DMA driver.
> 
>  Verified this patch with my NAND QPIC patches
>  https://www.spinics.net/lists/kernel/msg2573736.html
> 
>  I run the MTD test overnight and no failure
>  observed. Also, achieved significant improvement in
>  NAND speed. Following are the numbers for IPQ4019
>  DK04 board.
> 
>  Test                    Speed in KiB/s
>                         Before     After
> 
>  eraseblock write speed  4716    5483
>  eraseblock read speed   6855    8294
>  page write speed        4678    5436
>  page read speed         6784    8217
> 
> Tested-by: Abhishek Sahu <absahu@codeaurora.org>
> 

 Thanks for the testing.

>  Also, I reviewed the patch and following are
>  minor comments.

 ok.


>> &bchan->vc.desc_issued);
>> -        bchan->curr_txd = NULL;
>> -    }
>> +    list_for_each_entry(async_desc, &bchan->desc_list, desc_node)
>> +        list_add(&async_desc->vd.node, &bchan->vc.desc_issued);
> 
>   should we free the list also since we are adding these descriptor
>   back to issued and vchan_dma_desc_free_list will free all theses
>   descriptors
> 
>   When the IRQ will be triggered then it will traverse this list
>   and fetch the async descriptor for which already we freed the
>   memory.

 ha ok, should add list_del here.

<snip..>

>> 1);
>> +
>> +        list_for_each_entry_safe(async_desc, tmp,
>> +                     &bchan->desc_list, desc_node) {
>> +            if (async_desc) {
> 
>  Do we need this check since async_desc will be always not NULL.

 not needed, will remove.

<snip..>

>>      /* if work pending and idle, start a transaction */
>> -    if (vchan_issue_pending(&bchan->vc) && !bchan->curr_txd)
>> +    if (vchan_issue_pending(&bchan->vc) && !bchan->is_busy)
>>          bam_start_dma(bchan);
> 
>  can we get rid of these bchan->is_busy since it is being used
>  whether we have space in actual hardware FIFO and same we can
>  check with CIRC_SPACE(bchan->tail, bchan->head, MAX_DESCRIPTORS + 1)
> 
 yeah, this variable can be removed and simplified. Will do in V3.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus