Re: [PATCH v3 1/3] dma: Support multiple interleaved frames with non-contiguous memory

[Srikanth Thokala]

On Mon, Feb 17, 2014 at 3:14 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 02/17/2014 10:42 AM, Srikanth Thokala wrote:
>>
>> On Mon, Feb 17, 2014 at 3:05 PM, Lars-Peter Clausen <lars@xxxxxxxxxx>
>> wrote:
>>>
>>> On 02/17/2014 10:29 AM, Srikanth Thokala wrote:
>>>>
>>>>
>>>> On Mon, Feb 17, 2014 at 2:13 PM, Vinod Koul <vinod.koul@xxxxxxxxx>
>>>> wrote:
>>>>>
>>>>>
>>>>> On Sat, Feb 15, 2014 at 05:30:17PM +0530, Srikanth Thokala wrote:
>>>>>>
>>>>>>
>>>>>> The current implementation of interleaved DMA API support multiple
>>>>>> frames only when the memory is contiguous by incrementing src_start/
>>>>>> dst_start members of interleaved template.
>>>>>>
>>>>>> But, when the memory is non-contiguous it will restrict slave device
>>>>>> to not submit multiple frames in a batch.  This patch handles this
>>>>>> issue by allowing the slave device to send array of interleaved dma
>>>>>> templates each having a different memory location.
>>>>>
>>>>>
>>>>> This seems to be missing the numbers of templates you are sending,
>>>>> wouldnt this
>>>>> require sending ARRAY_SiZE too?
>>>>>
>>>>> And why send double pointer?
>>>>
>>>>
>>>>
>>>> Array size is not required, when we pass the double pointer.  The last
>>>> element would be
>>>> pointed to NULL and we could get the number of templates from this
>>>> condition.
>>>> Here is an example snippet,
>>>>
>>>> In slave device driver,
>>>>
>>>>           struct dma_interleaved_template **xts;
>>>>
>>>>           xts = kcalloc(frm_cnt+1, sizeof(struct
>>>> dma_interleaved_template *), GFP_KERNEL);
>>>>           /* Error check for xts */
>>>>           for (i = 0; i < frm_cnt; i++) {
>>>>                   xts[i] = kmalloc(sizeof(struct
>>>> dma_interleaved_template), GFP_KERNEL);
>>>>                   /* Error check for xts[i] */
>>>>           }
>>>>           xts[i] = NULL;
>>>>
>>>> In DMA engine driver,  we could get the number of frames by,
>>>>
>>>>           for (; xts[frmno] != NULL; frmno++);
>>>>
>>>> I felt this way is simpler than adding an extra argument to the API.
>>>> Please let me know
>>>> your opinion and suggest me a better way.
>>>
>>>
>>>
>>> I think Vinod's suggestion of passing in an array of
>>> interleaved_templates
>>> and the size of the array is better than what you are currently doing.
>>
>>
>> Ok, Lars.  I will update with this in my v4. Thanks.
>>
>>>
>>> Btw. you also need to update the current implementations and users of the
>>> API accordingly.
>>
>>
>> Yes, I have updated them in this patch.
>
>
> But you didn't update them accordingly to your proposed semantics. The
> caller didn't NULL terminate the array and the drivers did blindly assume
> there will always be exactly one transfer.

Ok, got it.  I will correct in v4.  Thanks for pointing this.

Srikanth

>
>
>>
>> Thanks
>> Srikanth
>>
>>
>>>
>>>
>>>>
>>>>>
>>>>> --
>>>>> ~Vinod
>>>>>
>>>>>>
>>>>>> Signed-off-by: Srikanth Thokala <sthokal@xxxxxxxxxx>
>>>>>> ---
>>>>>>    Documentation/dmaengine.txt              |    2 +-
>>>>>>    drivers/dma/imx-dma.c                    |    3 ++-
>>>>>>    drivers/dma/sirf-dma.c                   |    3 ++-
>>>>>>    drivers/media/platform/m2m-deinterlace.c |    2 +-
>>>>>>    include/linux/dmaengine.h                |    6 +++---
>>>>>>    5 files changed, 9 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/dmaengine.txt b/Documentation/dmaengine.txt
>>>>>> index 879b6e3..c642614 100644
>>>>>> --- a/Documentation/dmaengine.txt
>>>>>> +++ b/Documentation/dmaengine.txt
>>>>>> @@ -94,7 +94,7 @@ The slave DMA usage consists of following steps:
>>>>>>                 size_t period_len, enum dma_data_direction direction);
>>>>>>
>>>>>>         struct dma_async_tx_descriptor
>>>>>> *(*device_prep_interleaved_dma)(
>>>>>> -             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> *xt,
>>>>>> +             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> **xts,
>>>>>>                 unsigned long flags);
>>>>>>
>>>>>>       The peripheral driver is expected to have mapped the scatterlist
>>>>>> for
>>>>>> diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
>>>>>> index 6f9ac20..e2c52ce 100644
>>>>>> --- a/drivers/dma/imx-dma.c
>>>>>> +++ b/drivers/dma/imx-dma.c
>>>>>> @@ -954,12 +954,13 @@ static struct dma_async_tx_descriptor
>>>>>> *imxdma_prep_dma_memcpy(
>>>>>>    }
>>>>>>
>>>>>>    static struct dma_async_tx_descriptor *imxdma_prep_dma_interleaved(
>>>>>> -     struct dma_chan *chan, struct dma_interleaved_template *xt,
>>>>>> +     struct dma_chan *chan, struct dma_interleaved_template **xts,
>>>>>>         unsigned long flags)
>>>>>>    {
>>>>>>         struct imxdma_channel *imxdmac = to_imxdma_chan(chan);
>>>>>>         struct imxdma_engine *imxdma = imxdmac->imxdma;
>>>>>>         struct imxdma_desc *desc;
>>>>>> +     struct dma_interleaved_template *xt = *xts;
>>>>>>
>>>>>>         dev_dbg(imxdma->dev, "%s channel: %d src_start=0x%llx
>>>>>> dst_start=0x%llx\n"
>>>>>>                 "   src_sgl=%s dst_sgl=%s numf=%zu frame_size=%zu\n",
>>>>>> __func__,
>>>>>> diff --git a/drivers/dma/sirf-dma.c b/drivers/dma/sirf-dma.c
>>>>>> index d4d3a31..b6a150b 100644
>>>>>> --- a/drivers/dma/sirf-dma.c
>>>>>> +++ b/drivers/dma/sirf-dma.c
>>>>>> @@ -509,12 +509,13 @@ sirfsoc_dma_tx_status(struct dma_chan *chan,
>>>>>> dma_cookie_t cookie,
>>>>>>    }
>>>>>>
>>>>>>    static struct dma_async_tx_descriptor
>>>>>> *sirfsoc_dma_prep_interleaved(
>>>>>> -     struct dma_chan *chan, struct dma_interleaved_template *xt,
>>>>>> +     struct dma_chan *chan, struct dma_interleaved_template **xts,
>>>>>>         unsigned long flags)
>>>>>>    {
>>>>>>         struct sirfsoc_dma *sdma = dma_chan_to_sirfsoc_dma(chan);
>>>>>>         struct sirfsoc_dma_chan *schan =
>>>>>> dma_chan_to_sirfsoc_dma_chan(chan);
>>>>>>         struct sirfsoc_dma_desc *sdesc = NULL;
>>>>>> +     struct dma_interleaved_template *xt = *xts;
>>>>>>         unsigned long iflags;
>>>>>>         int ret;
>>>>>>
>>>>>> diff --git a/drivers/media/platform/m2m-deinterlace.c
>>>>>> b/drivers/media/platform/m2m-deinterlace.c
>>>>>> index 6bb86b5..468110a 100644
>>>>>> --- a/drivers/media/platform/m2m-deinterlace.c
>>>>>> +++ b/drivers/media/platform/m2m-deinterlace.c
>>>>>> @@ -343,7 +343,7 @@ static void deinterlace_issue_dma(struct
>>>>>> deinterlace_ctx *ctx, int op,
>>>>>>         ctx->xt->dst_sgl = true;
>>>>>>         flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
>>>>>>
>>>>>> -     tx = dmadev->device_prep_interleaved_dma(chan, ctx->xt, flags);
>>>>>> +     tx = dmadev->device_prep_interleaved_dma(chan, &ctx->xt, flags);
>>>>>>         if (tx == NULL) {
>>>>>>                 v4l2_warn(&pcdev->v4l2_dev, "DMA interleaved prep
>>>>>> error\n");
>>>>>>                 return;
>>>>>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>>>>>> index c5c92d5..2f77a9a 100644
>>>>>> --- a/include/linux/dmaengine.h
>>>>>> +++ b/include/linux/dmaengine.h
>>>>>> @@ -675,7 +675,7 @@ struct dma_device {
>>>>>>                 size_t period_len, enum dma_transfer_direction
>>>>>> direction,
>>>>>>                 unsigned long flags, void *context);
>>>>>>         struct dma_async_tx_descriptor
>>>>>> *(*device_prep_interleaved_dma)(
>>>>>> -             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> *xt,
>>>>>> +             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> **xts,
>>>>>>                 unsigned long flags);
>>>>>>         int (*device_control)(struct dma_chan *chan, enum dma_ctrl_cmd
>>>>>> cmd,
>>>>>>                 unsigned long arg);
>>>>>> @@ -752,10 +752,10 @@ static inline struct dma_async_tx_descriptor
>>>>>> *dmaengine_prep_dma_cyclic(
>>>>>>    }
>>>>>>
>>>>>>    static inline struct dma_async_tx_descriptor
>>>>>> *dmaengine_prep_interleaved_dma(
>>>>>> -             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> *xt,
>>>>>> +             struct dma_chan *chan, struct dma_interleaved_template
>>>>>> **xts,
>>>>>>                 unsigned long flags)
>>>>>>    {
>>>>>> -     return chan->device->device_prep_interleaved_dma(chan, xt,
>>>>>> flags);
>>>>>> +     return chan->device->device_prep_interleaved_dma(chan, xts,
>>>>>> flags);
>>>>>>    }
>>>>>>
>>>>>>    static inline int dma_get_slave_caps(struct dma_chan *chan, struct
>>>>>> dma_slave_caps *caps)
>>>>>> --
>>>>>> 1.7.9.5
>>>>>>
>>>>>> --
>>>>>> 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
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
>>>>> in
>>>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>>>>> 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 majordomo@xxxxxxxxxxxxxxx
>>> 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 majordomo@xxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/