[PATCH v4 2/2] dma: Add Xilinx AXI Video Direct Memory Access Engine driver support

[Jassi Brar]

On 11 March 2014 00:00, Srikanth Thokala <sthokal at xilinx.com> wrote:
> On Mon, Mar 10, 2014 at 9:30 PM, Jassi Brar <jassisinghbrar at gmail.com> wrote:
>> On Thu, Mar 6, 2014 at 7:18 PM, Srikanth Thokala <sthokal at xilinx.com> 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 :)