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

From: Sricharan R
Date: Wed Aug 09 2017 - 10:24:42 EST


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@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
>

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