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 <joabreu@xxxxxxxxxxxx>
>> Cc: Carlos Palminha <palminha@xxxxxxxxxxxx>
>> Cc: Vinod Koul <vinod.koul@xxxxxxxxx>
>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>> Cc: Kedareswara rao Appana <appana.durga.rao@xxxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: dmaengine@xxxxxxxxxxxxxxx
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> ---
>> 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
>>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
