Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758560AbcLOPc5 (ORCPT ); Thu, 15 Dec 2016 10:32:57 -0500 Received: from smtprelay.synopsys.com ([198.182.47.9]:41668 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752131AbcLOPcz (ORCPT ); Thu, 15 Dec 2016 10:32:55 -0500 Subject: Re: [PATCH] dmaengine: xilinx_dma: Add support for multiple buffers To: Appana Durga Kedareswara Rao , Jose Abreu , "dmaengine@vger.kernel.org" References: <40ba7c24927eeffdcc66425f8ca1203589276c08.1481812291.git.joabreu@synopsys.com> CC: Carlos Palminha , Vinod Koul , Dan Williams , "Laurent Pinchart" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" From: Jose Abreu Message-ID: <72be3955-1ec8-05f4-c8ee-ac29da69904d@synopsys.com> Date: Thu, 15 Dec 2016 15:31:07 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.107.19.59] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7548 Lines: 241 Hi Kedar, On 15-12-2016 15:19, Appana Durga Kedareswara Rao wrote: > Hi Jose Abreu, > > Thanks for the patch... > >> Xilinx VDMA supports multiple framebuffers. This patch adds correct handling for >> the scenario where multiple framebuffers are available in the HW and parking >> mode is not set. >> >> We corrected these situations: >> 1) Do not start VDMA until all the framebuffers >> have been programmed with a valid address. >> 2) Restart variables when VDMA halts/resets. >> 3) Halt channel when all the framebuffers have >> finished and there is not anymore segments >> pending. >> 4) Do not try to overlap framebuffers until they >> are completed. >> >> All these situations, without this patch, can lead to data corruption and even >> system memory corruption. If, for example, user has a VDMA with 3 >> framebuffers, with direction DMA_DEV_TO_MEM and user only submits one >> segment, VDMA will write first to the segment the user submitted BUT if the >> user doesn't submit another segment in a timelly manner then VDMA will write >> to position 0 of system mem in the following VSYNC. > I have posted different patch series for fixing these issues just now... > Please take a look into it... > > Regards, > Kedar. Thanks, I will review them. Best regards, Jose Miguel Abreu > >> Signed-off-by: Jose Abreu >> Cc: Carlos Palminha >> Cc: Vinod Koul >> Cc: Dan Williams >> Cc: Kedareswara rao Appana >> Cc: Laurent Pinchart >> Cc: dmaengine@vger.kernel.org >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> --- >> drivers/dma/xilinx/xilinx_dma.c | 80 ++++++++++++++++++++++++++++++++++- >> ------ >> 1 file changed, 68 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c >> index 8288fe4..30eec5a 100644 >> --- a/drivers/dma/xilinx/xilinx_dma.c >> +++ b/drivers/dma/xilinx/xilinx_dma.c >> @@ -351,10 +351,12 @@ struct xilinx_dma_chan { >> bool cyclic; >> bool genlock; >> bool err; >> + bool running; >> struct tasklet_struct tasklet; >> struct xilinx_vdma_config config; >> bool flush_on_fsync; >> u32 desc_pendingcount; >> + u32 seg_pendingcount; >> bool ext_addr; >> u32 desc_submitcount; >> u32 residue; >> @@ -946,6 +948,17 @@ static bool xilinx_dma_is_idle(struct xilinx_dma_chan >> *chan) } >> >> /** >> + * xilinx_vdma_is_multi_buffer - Check if VDMA has multiple >> +framebuffers >> + * @chan: Driver specific DMA channel >> + * >> + * Return: '1' if is multi buffer, '0' if not. >> + */ >> +static bool xilinx_vdma_is_multi_buffer(struct xilinx_dma_chan *chan) { >> + return chan->num_frms > 1; >> +} >> + >> +/** >> * xilinx_dma_halt - Halt DMA channel >> * @chan: Driver specific DMA channel >> */ >> @@ -966,6 +979,10 @@ static void xilinx_dma_halt(struct xilinx_dma_chan >> *chan) >> chan, dma_ctrl_read(chan, >> XILINX_DMA_REG_DMASR)); >> chan->err = true; >> } >> + >> + chan->seg_pendingcount = 0; >> + chan->desc_submitcount = 0; >> + chan->running = false; >> } >> >> /** >> @@ -1002,14 +1019,35 @@ static void xilinx_vdma_start_transfer(struct >> xilinx_dma_chan *chan) >> struct xilinx_dma_tx_descriptor *desc, *tail_desc; >> u32 reg; >> struct xilinx_vdma_tx_segment *tail_segment; >> + bool mbf = xilinx_vdma_is_multi_buffer(chan) && !config->park; >> >> /* This function was invoked with lock held */ >> if (chan->err) >> return; >> >> - if (list_empty(&chan->pending_list)) >> + /* >> + * Can't continue if we have already consumed all the available >> + * framebuffers and they are not done yet. >> + */ >> + if (mbf && (chan->seg_pendingcount >= chan->num_frms)) >> return; >> >> + if (list_empty(&chan->pending_list)) { >> + /* >> + * Can't keep running if there are no pending segments. Halt >> + * the channel as security measure. Notice that this will not >> + * corrupt current transactions because this function is >> + * called after the pendingcount is decreased and after the >> + * current transaction has finished. >> + */ >> + if (mbf && chan->running && !chan->seg_pendingcount) { >> + dev_dbg(chan->dev, "pending list empty: halting\n"); >> + xilinx_dma_halt(chan); >> + } >> + >> + return; >> + } >> + >> desc = list_first_entry(&chan->pending_list, >> struct xilinx_dma_tx_descriptor, node); >> tail_desc = list_last_entry(&chan->pending_list, >> @@ -1079,6 +1117,8 @@ static void xilinx_vdma_start_transfer(struct >> xilinx_dma_chan *chan) >> if (chan->has_sg) { >> dma_ctrl_write(chan, XILINX_DMA_REG_TAILDESC, >> tail_segment->phys); >> + list_splice_tail_init(&chan->pending_list, &chan->active_list); >> + chan->desc_pendingcount = 0; >> } else { >> struct xilinx_vdma_tx_segment *segment, *last = NULL; >> int i = 0; >> @@ -1097,29 +1137,34 @@ static void xilinx_vdma_start_transfer(struct >> xilinx_dma_chan *chan) >> >> XILINX_VDMA_REG_START_ADDRESS(i++), >> segment->hw.buf_addr); >> >> + chan->seg_pendingcount++; >> last = segment; >> } >> >> if (!last) >> return; >> >> - /* HW expects these parameters to be same for one >> transaction */ >> - vdma_desc_write(chan, XILINX_DMA_REG_HSIZE, last- >>> hw.hsize); >> - vdma_desc_write(chan, XILINX_DMA_REG_FRMDLY_STRIDE, >> - last->hw.stride); >> - vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last- >>> hw.vsize); >> - } >> - >> - if (!chan->has_sg) { >> list_del(&desc->node); >> list_add_tail(&desc->node, &chan->active_list); >> chan->desc_submitcount++; >> chan->desc_pendingcount--; >> if (chan->desc_submitcount == chan->num_frms) >> chan->desc_submitcount = 0; >> - } else { >> - list_splice_tail_init(&chan->pending_list, &chan->active_list); >> - chan->desc_pendingcount = 0; >> + >> + /* >> + * Can't start until all the framebuffers have been programmed >> + * or else corruption can occur. >> + */ >> + if (mbf && !chan->running && >> + (chan->seg_pendingcount < chan->num_frms)) >> + return; >> + >> + /* HW expects these parameters to be same for one >> transaction */ >> + vdma_desc_write(chan, XILINX_DMA_REG_HSIZE, last- >>> hw.hsize); >> + vdma_desc_write(chan, XILINX_DMA_REG_FRMDLY_STRIDE, >> + last->hw.stride); >> + vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last- >>> hw.vsize); >> + chan->running = true; >> } >> } >> >> @@ -1327,12 +1372,20 @@ static void xilinx_dma_issue_pending(struct >> dma_chan *dchan) static void xilinx_dma_complete_descriptor(struct >> xilinx_dma_chan *chan) { >> struct xilinx_dma_tx_descriptor *desc, *next; >> + struct xilinx_vdma_tx_segment *segment; >> >> /* This function was invoked with lock held */ >> if (list_empty(&chan->active_list)) >> return; >> >> list_for_each_entry_safe(desc, next, &chan->active_list, node) { >> + if (chan->xdev->dma_config->dmatype == XDMA_TYPE_VDMA) >> { >> + list_for_each_entry(segment, &desc->segments, node) >> { >> + if (chan->seg_pendingcount > 0) >> + chan->seg_pendingcount--; >> + } >> + } >> + >> list_del(&desc->node); >> if (!desc->cyclic) >> dma_cookie_complete(&desc->async_tx); >> @@ -1366,6 +1419,9 @@ static int xilinx_dma_reset(struct xilinx_dma_chan >> *chan) >> } >> >> chan->err = false; >> + chan->seg_pendingcount = 0; >> + chan->desc_submitcount = 0; >> + chan->running = false; >> >> return err; >> } >> -- >> 1.9.1 >>