On 11 March 2014 00:00, Srikanth Thokala <sthokal@xxxxxxxxxx> wrote: > On Mon, Mar 10, 2014 at 9:30 PM, Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote: >> On Thu, Mar 6, 2014 at 7:18 PM, Srikanth Thokala <sthokal@xxxxxxxxxx> wrote: >> >>> +static struct dma_async_tx_descriptor * >>> +xilinx_vdma_dma_prep_interleaved(struct dma_chan *dchan, >>> + struct dma_interleaved_template *xt, >>> + unsigned long flags) >>> +{ >>> + struct xilinx_vdma_chan *chan = to_xilinx_chan(dchan); >>> + struct xilinx_vdma_tx_descriptor *desc; >>> + struct xilinx_vdma_tx_segment *segment; >>> + struct xilinx_vdma_tx_segment *prev = NULL; >>> + int i; >>> + >>> + if ((xt->dir != DMA_MEM_TO_DEV) && (xt->dir != DMA_DEV_TO_MEM)) >>> + return NULL; >>> + >>> + if (!xt->numf || !xt->sgl[0].size) >>> + return NULL; >>> + >>> + /* Allocate a transaction descriptor. */ >>> + desc = xilinx_vdma_alloc_tx_descriptor(chan); >>> + if (!desc) >>> + return NULL; >>> + >>> + dma_async_tx_descriptor_init(&desc->async_tx, &chan->common); >>> + desc->async_tx.tx_submit = xilinx_vdma_tx_submit; >>> + desc->async_tx.cookie = 0; >>> + async_tx_ack(&desc->async_tx); >>> + >>> + /* Build the list of transaction segments. */ >>> + for (i = 0; i < xt->frame_size; i++) { >>> + struct xilinx_vdma_desc_hw *hw; >>> + >>> + /* Allocate the link descriptor from DMA pool */ >>> + segment = xilinx_vdma_alloc_tx_segment(chan); >>> + if (!segment) >>> + goto error; >>> + >>> + /* Fill in the hardware descriptor */ >>> + hw = &segment->hw; >>> + hw->vsize = xt->numf; >>> + hw->hsize = xt->sgl[0].size; >>> + hw->stride = xt->sgl[0].icg << >>> + XILINX_VDMA_FRMDLY_STRIDE_STRIDE_SHIFT; >>> >> It seems the xt->frame_size is (should be?) always going to be 1? >> If yes, the for-loop isn't needed. >> If no, you should probably use 'i' as the index to sgl[], and not always 0. > > It can be either > '1': User can queue each segment and submit all the segments at once as a > single async tx descriptor. I am using this for scatter-gather mode of > operation where the src_start/dst_start is different for each segment. > 'Number of frames': In this case the driver gets contiguous frame buffer memory > and it prepares segments with addresses as multiples of > src_start/dst_start. > Quoting you from earlier thread .... " In video frame context, chunk.size -> hsize chunk.icg -> stride numf -> vsize and frame_size is always 1 as it will have only one chunk in a line. " ... which makes sense. I hope you realize that, 'frame_size' denotes the number of chunks in a frame and 'numf' denotes the number of frames in the transfer request. It seems wrong when you assume vsize:=numf but frame_size>1 ... i.e, each horizontal video line has two or more dis-contiguous chunks? This might work for client drivers written specifically for Xilinx AXI Video DMAC but will fail over any other controller driver. IMO, xilinx_vdma.c should expect only 1 video frame per device_prep_interleaved_dma() call (i.e, vsize=numf and frame_size==1) Let the client driver submit 1/2/.../16 requests before kick starting the DMA. Upon dma_async_tx_descriptor.callback(), the client 'refills' the frame and re-queues it back. This should work because the video-frames' attributes and locations are not going to change within a session. > Please let me know if you see any issues in this implementation. > I am ok if you want the driver upstream now and want to 'fix' it when you actually start seeing problems :) -- 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