On Tue, Mar 11, 2014 at 7:14 PM, Jassi Brar <jaswinder.si...@linaro.org> wrote:
> On 11 March 2014 00:00, Srikanth Thokala <stho...@xilinx.com> wrote:
>> On Mon, Mar 10, 2014 at 9:30 PM, Jassi Brar <jassisinghb...@gmail.com> wrote:
>>> On Thu, Mar 6, 2014 at 7:18 PM, Srikanth Thokala <stho...@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 :)
I agree with you and I have no issues in fixing this now. I will fix
it in my v6
with the driver accepting only one frame/interleave_dma call.
But, I feel this information has to be documented to the description of
interleaved_dma API as it is leading to interpret the members of this
structure differently wrt video frame terminology.
Thanks
Srikanth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
