[PATCH 0/4] Expand Xilinx CDMA functions

[PATCH 0/4] Expand Xilinx CDMA functions

[Adrian Larumbe]

Recently at Imgtec we had to provide GLES API buffers with DMA transfer
capabilities to device memory. We had access to a Xilinx CDMA IP module,
but even though the hardware supports scatter-gather operations,
the driver did not. This patch series' goal is to extend the driver
to support SG transfers on CDMA devices.

It also fixes a couple of issues I found in the driver: lack of support
for HW descriptors allocated in an extended address space (above 32 bits)
and an unusual race condition when closing a DMA channel.

Adrian Larumbe (4):
  dmaengine: xilinx_dma: Add extended address support in CDMA
  dmaengine: xilinx_dma: Add channel configuration setting callback
  dmaengine: xilinx_dma: Add CDMA SG transfer support
  dmaengine: xilinx_dma: Add device synchronisation callback

 drivers/dma/xilinx/xilinx_dma.c | 186 ++++++++++++++++++++++++++++++--
 1 file changed, 177 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH 1/4] dmaengine: xilinx_dma: Add extended address support in CDMA

[Adrian Larumbe]

Not accounting for this means DMA HW descriptors can only be sourced from
the lower 32 bits of the address space. CDMA MSB descriptor registers were
also missing in the driver file, so this change also adds their register
offset definitions, which were taken from Xilpinx 'AXI Central Direct
Memory Access v4.1' LogiCORE IP Product Guide.

Signed-off-by: Adrian Larumbe <adrian.martinezlarumbe@imgtec.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 3aded7861fef..3f859de593dc 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -98,7 +98,9 @@
 #define XILINX_DMA_DMASR_FRAME_COUNT_MASK	GENMASK(23, 16)
 
 #define XILINX_DMA_REG_CURDESC			0x0008
+#define XILINX_DMA_REG_CURDESC_MSB		0x000C
 #define XILINX_DMA_REG_TAILDESC		0x0010
+#define XILINX_DMA_REG_TAILDESC_MSB	0x0014
 #define XILINX_DMA_REG_REG_INDEX		0x0014
 #define XILINX_DMA_REG_FRMSTORE		0x0018
 #define XILINX_DMA_REG_THRESHOLD		0x001c
@@ -184,6 +186,8 @@
 /* AXI CDMA Specific Registers/Offsets */
 #define XILINX_CDMA_REG_SRCADDR		0x18
 #define XILINX_CDMA_REG_DSTADDR		0x20
+#define XILINX_CDMA_REG_MSB_DSTADR	0x0024
+#define XILINX_CDMA_REG_MSB_SRCADDR	0x001C
 
 /* AXI CDMA Specific Masks */
 #define XILINX_CDMA_CR_SGMODE          BIT(3)
@@ -1459,9 +1463,19 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 		dma_ctrl_set(chan, XILINX_DMA_REG_DMACR,
 			     XILINX_CDMA_CR_SGMODE);
 
+		if (chan->ext_addr) {
+			xilinx_write(chan, XILINX_DMA_REG_CURDESC_MSB,
+				     upper_32_bits(head_desc->async_tx.phys));
+		}
+
 		xilinx_write(chan, XILINX_DMA_REG_CURDESC,
 			     head_desc->async_tx.phys);
 
+		if (chan->ext_addr) {
+			xilinx_write(chan, XILINX_DMA_REG_TAILDESC_MSB,
+				     upper_32_bits(tail_segment->phys));
+		}
+
 		/* Update tail ptr register which will start the transfer */
 		xilinx_write(chan, XILINX_DMA_REG_TAILDESC,
 			     tail_segment->phys);
@@ -1476,11 +1490,16 @@ static void xilinx_cdma_start_transfer(struct xilinx_dma_chan *chan)
 
 		hw = &segment->hw;
 
-		xilinx_write(chan, XILINX_CDMA_REG_SRCADDR,
-			     xilinx_prep_dma_addr_t(hw->src_addr));
-		xilinx_write(chan, XILINX_CDMA_REG_DSTADDR,
-			     xilinx_prep_dma_addr_t(hw->dest_addr));
-
+		xilinx_write(chan, XILINX_CDMA_REG_SRCADDR, hw->src_addr);
+		xilinx_write(chan, XILINX_CDMA_REG_DSTADDR, hw->dest_addr);
+		if (chan->ext_addr) {
+			xilinx_write(chan,
+				     XILINX_CDMA_REG_MSB_SRCADDR,
+				     hw->src_addr_msb);
+			xilinx_write(chan,
+				     XILINX_CDMA_REG_MSB_DSTADR,
+				     hw->dest_addr_msb);
+		}
 		/* Start the transfer */
 		dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
 				hw->control & chan->xdev->max_buffer_len);
-- 
2.17.1

[PATCH 2/4] dmaengine: xilinx_dma: Add channel configuration setting callback

[Adrian Larumbe]

This callback allows the DMA Engine API user to set a per-channel
configuration structure before calling dmaengine_prep_slave_sg.
This is a prerequisite for being able to do CDMA SG transfers, because
one of the transfer ends is meant to be a preconfigured contiguous
block of memory.

A later implementation of CDMA SG transfers will access this configuration
structure, and is therefore dependent on this change.

Signed-off-by: Adrian Larumbe <adrian.martinezlarumbe@imgtec.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 3f859de593dc..49c7093e2487 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -392,6 +392,7 @@ struct xilinx_dma_tx_descriptor {
  * @irq: Channel IRQ
  * @id: Channel ID
  * @direction: Transfer direction
+ * @cfg: DMA slave channel configuration
  * @num_frms: Number of frames
  * @has_sg: Support scatter transfers
  * @cyclic: Check for cyclic transfers.
@@ -429,6 +430,7 @@ struct xilinx_dma_chan {
 	int irq;
 	int id;
 	enum dma_transfer_direction direction;
+	struct dma_slave_config cfg;
 	int num_frms;
 	bool has_sg;
 	bool cyclic;
@@ -2565,6 +2567,21 @@ static void xilinx_dma_chan_remove(struct xilinx_dma_chan *chan)
 	list_del(&chan->common.device_node);
 }
 
+/**
+ * xilinx_dma_slave_config - Set the channel config
+ * @dchan: DMA channel
+ * @cfg: DMA slave channel configuration
+ */
+static int xilinx_dma_slave_config(struct dma_chan *dchan,
+				   struct dma_slave_config *cfg)
+{
+	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
+
+	chan->cfg = *cfg;
+
+	return 0;
+}
+
 static int axidma_clk_init(struct platform_device *pdev, struct clk **axi_clk,
 			    struct clk **tx_clk, struct clk **rx_clk,
 			    struct clk **sg_clk, struct clk **tmp_clk)
@@ -3095,6 +3112,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	xdev->common.device_terminate_all = xilinx_dma_terminate_all;
 	xdev->common.device_tx_status = xilinx_dma_tx_status;
 	xdev->common.device_issue_pending = xilinx_dma_issue_pending;
+	xdev->common.device_config = xilinx_dma_slave_config;
 	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		dma_cap_set(DMA_CYCLIC, xdev->common.cap_mask);
 		xdev->common.device_prep_slave_sg = xilinx_dma_prep_slave_sg;
-- 
2.17.1

[PATCH 3/4] dmaengine: xilinx_dma: Add CDMA SG transfer support

[Adrian Larumbe]

CDMA devices have built-in SG capability, but it was not leveraged by the
driver. It adds a new binding for CDMA's device_prep_slave_sg, which
looks up the memory address supplied in a previous call to
dmaengine_slave_config as the beginning of a contiguous chunk of memory,
for either the source or destination of the transfer.

DMA_PRIVATE capability needs to be set for a CDMA channel too, or else
the DMA Engine core will take a reference upon registration, and it
won't be further available for callers of dma_request_chan.

Signed-off-by: Adrian Larumbe <adrian.martinezlarumbe@imgtec.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 131 +++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 49c7093e2487..40c6cf8bf0e6 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -2433,6 +2433,130 @@ xilinx_mcdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
 	return NULL;
 }
 
+/**
+ * xilinx_cdma_prep_slave_sg - prepare descriptors for a DMA_SLAVE transaction
+ * @dchan: DMA channel
+ * @sgl: scatterlist to transfer to/from
+ * @sg_len: number of entries in @scatterlist
+ * @direction: DMA direction
+ * @flags: transfer ack flags
+ * @context: APP words of the descriptor
+ *
+ * Return: Async transaction descriptor on success and NULL on failure
+ */
+static struct dma_async_tx_descriptor
+*xilinx_cdma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
+						   unsigned int sg_len,
+						   enum dma_transfer_direction direction,
+						   unsigned long flags, void *context)
+{
+	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
+	struct xilinx_cdma_tx_segment *segment, *prev = NULL;
+	struct xilinx_dma_tx_descriptor *desc;
+	struct xilinx_cdma_desc_hw *hw;
+	dma_addr_t dma_dst, dma_src, dma_addr;
+	size_t len, avail;
+
+	(void)flags;
+	(void)context;
+
+	if (!is_slave_direction(direction)) {
+		dev_err(chan->xdev->dev,
+			"%s: bad direction?\n", __func__);
+		return NULL;
+	}
+
+	if (unlikely(sg_len == 0 || !sgl))
+		return NULL;
+
+	dma_src = 0;
+	dma_dst = 0;
+
+	if (direction == DMA_DEV_TO_MEM)
+		dma_src = chan->cfg.src_addr;
+	else
+		dma_dst = chan->cfg.dst_addr;
+
+	desc = xilinx_dma_alloc_tx_descriptor(chan);
+	if (!desc)
+		return NULL;
+
+	dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
+	desc->async_tx.tx_submit = xilinx_dma_tx_submit;
+
+	avail = sg_dma_len(sgl);
+	/*
+	 * Loop until there is either no more source or no more
+	 * destination scatterlist entries
+	 */
+	while (true) {
+		len = min_t(size_t, avail, chan->xdev->max_buffer_len);
+		if (len == 0)
+			goto fetch;
+
+		/* Allocate the link descriptor from DMA pool */
+		segment = xilinx_cdma_alloc_tx_segment(chan);
+		if (!segment)
+			goto error;
+
+		dma_addr = sg_dma_address(sgl) + sg_dma_len(sgl) - avail;
+
+		if (direction == DMA_DEV_TO_MEM)
+			dma_dst = dma_addr;
+		else
+			dma_src = dma_addr;
+
+		hw = &segment->hw;
+		hw->control = len;
+		hw->src_addr = dma_src;
+		hw->dest_addr = dma_dst;
+		if (chan->ext_addr) {
+			hw->src_addr_msb = upper_32_bits(dma_src);
+			hw->dest_addr_msb = upper_32_bits(dma_dst);
+		}
+
+		if (prev) {
+			prev->hw.next_desc = segment->phys;
+			if (chan->ext_addr)
+				prev->hw.next_desc_msb = upper_32_bits(segment->phys);
+		}
+
+		prev = segment;
+		avail -= len;
+		list_add_tail(&segment->node, &desc->segments);
+
+		if (direction == DMA_DEV_TO_MEM)
+			dma_src += len;
+		else
+			dma_dst += len;
+fetch:
+		/* Fetch the next scatterlist entry */
+		if (avail == 0) {
+			if (sg_len == 0)
+				break;
+			sgl = sg_next(sgl);
+			if (!sgl)
+				break;
+			sg_len--;
+			avail = sg_dma_len(sgl);
+		}
+	}
+
+	/* Link the last hardware descriptor with the first. */
+	segment = list_first_entry(&desc->segments,
+							   struct xilinx_cdma_tx_segment, node);
+	desc->async_tx.phys = segment->phys;
+	prev->hw.next_desc = segment->phys;
+	if (chan->ext_addr)
+		prev->hw.next_desc_msb = upper_32_bits(segment->phys);
+
+	return &desc->async_tx;
+
+error:
+	xilinx_dma_free_tx_descriptor(chan, desc);
+	return NULL;
+}
+
 /**
  * xilinx_dma_terminate_all - Halt the channel and free descriptors
  * @dchan: Driver specific DMA Channel pointer
@@ -3100,10 +3224,8 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	xdev->common.dev = &pdev->dev;
 
 	INIT_LIST_HEAD(&xdev->common.channels);
-	if (!(xdev->dma_config->dmatype == XDMA_TYPE_CDMA)) {
-		dma_cap_set(DMA_SLAVE, xdev->common.cap_mask);
-		dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
-	}
+	dma_cap_set(DMA_SLAVE, xdev->common.cap_mask);
+	dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask);
 
 	xdev->common.device_alloc_chan_resources =
 				xilinx_dma_alloc_chan_resources;
@@ -3124,6 +3246,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	} else if (xdev->dma_config->dmatype == XDMA_TYPE_CDMA) {
 		dma_cap_set(DMA_MEMCPY, xdev->common.cap_mask);
 		xdev->common.device_prep_dma_memcpy = xilinx_cdma_prep_memcpy;
+		xdev->common.device_prep_slave_sg = xilinx_cdma_prep_slave_sg;
 		/* Residue calculation is supported by only AXI DMA and CDMA */
 		xdev->common.residue_granularity =
 					  DMA_RESIDUE_GRANULARITY_SEGMENT;
-- 
2.17.1

[PATCH 4/4] dmaengine: xilinx_dma: Add device synchronisation callback

[Adrian Larumbe]

This was required as an answer to a very unusual race condition, whereby a
thread waiting on a completion signaled in a callback triggered by
dmaengine_desc_callback_invoke might immediately attempt to release the
DMA channel whilst the channel lock was still free. This would cause the
remaining descriptors to be deallocated, and xilinx_dma_chan_desc_cleanup
to perform an invalid memory access when attempting to traverse the rest
of the channel's done_list.

Now, when releasing a DMA channel, it will wait until the tasklet has
finished.

Signed-off-by: Adrian Larumbe <adrian.martinezlarumbe@imgtec.com>
---
 drivers/dma/xilinx/xilinx_dma.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index 40c6cf8bf0e6..d4bad999e7f9 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -942,6 +942,13 @@ static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
 
 }
 
+static void xilinx_dma_synchronize(struct dma_chan *dchan)
+{
+	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
+
+	tasklet_kill(&chan->tasklet);
+}
+
 /**
  * xilinx_dma_get_residue - Compute residue for a given descriptor
  * @chan: Driver specific dma channel
@@ -3235,6 +3242,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
 	xdev->common.device_tx_status = xilinx_dma_tx_status;
 	xdev->common.device_issue_pending = xilinx_dma_issue_pending;
 	xdev->common.device_config = xilinx_dma_slave_config;
+	xdev->common.device_synchronize = xilinx_dma_synchronize;
 	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
 		dma_cap_set(DMA_CYCLIC, xdev->common.cap_mask);
 		xdev->common.device_prep_slave_sg = xilinx_dma_prep_slave_sg;
-- 
2.17.1

Re: [PATCH 4/4] dmaengine: xilinx_dma: Add device synchronisation callback

[Lars-Peter Clausen]

On 4/23/21 3:19 AM, Adrian Larumbe wrote:
> This was required as an answer to a very unusual race condition, whereby a
> thread waiting on a completion signaled in a callback triggered by
> dmaengine_desc_callback_invoke might immediately attempt to release the
> DMA channel whilst the channel lock was still free. This would cause the
> remaining descriptors to be deallocated, and xilinx_dma_chan_desc_cleanup
> to perform an invalid memory access when attempting to traverse the rest
> of the channel's done_list.
>
> Now, when releasing a DMA channel, it will wait until the tasklet has
> finished.
>
> Signed-off-by: Adrian Larumbe <adrian.martinezlarumbe@imgtec.com>

Hi,


Patch looks good, but basically the same got already applied a few weeks 
ago.

See 
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=next&id=50db2050faf854cbaf4b6557a7a8ca21bff302ae

- Lars

> ---
>   drivers/dma/xilinx/xilinx_dma.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 40c6cf8bf0e6..d4bad999e7f9 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -942,6 +942,13 @@ static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
>   
>   }
>   
> +static void xilinx_dma_synchronize(struct dma_chan *dchan)
> +{
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> +
> +	tasklet_kill(&chan->tasklet);
> +}
> +
>   /**
>    * xilinx_dma_get_residue - Compute residue for a given descriptor
>    * @chan: Driver specific dma channel
> @@ -3235,6 +3242,7 @@ static int xilinx_dma_probe(struct platform_device *pdev)
>   	xdev->common.device_tx_status = xilinx_dma_tx_status;
>   	xdev->common.device_issue_pending = xilinx_dma_issue_pending;
>   	xdev->common.device_config = xilinx_dma_slave_config;
> +	xdev->common.device_synchronize = xilinx_dma_synchronize;
>   	if (xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
>   		dma_cap_set(DMA_CYCLIC, xdev->common.cap_mask);
>   		xdev->common.device_prep_slave_sg = xilinx_dma_prep_slave_sg;

Re: [EXTERNAL] Re: [PATCH 4/4] dmaengine: xilinx_dma: Add device synchronisation callback

[Adrian Larumbe]

On 23.04.2021 08:33, Lars-Peter Clausen wrote:

> Hi,
> 
> 
> Patch looks good, but basically the same got already applied a few weeks ago.

I'll get rid of mine altogether in a later version.

Cheers,
Adrian

Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Lars-Peter Clausen]

On 4/23/21 3:19 AM, Adrian Larumbe wrote:
> Recently at Imgtec we had to provide GLES API buffers with DMA transfer
> capabilities to device memory. We had access to a Xilinx CDMA IP module,
> but even though the hardware supports scatter-gather operations,
> the driver did not. This patch series' goal is to extend the driver
> to support SG transfers on CDMA devices.

Hi,

Thanks for the patch series. From an implementation point of view this 
looks good. But I'm a bit concerned about dmaengine API compliance.

The device_config() and device_prep_slave_sg() APIs are meant for 
memory-to-device or device-to-memory transfers. This means the device 
address is fixed and usually maps to a FIFO in the device.

What you are implementing is memory-to-memory, which means addresses are 
incremented on both sides of transfer. And this works if you know what 
you are doing, e.g. you know that you are using a specific dmaengine 
driver that implements the dmaengine API in a way that does not comply 
with the specification. But it breaks for generic dmaengine API clients 
that expect compliant behavior. And it is the reason why we have an API 
in the first place, to get consistent behavior. If you have to know 
about the driver specific semantics you might as well just bypass the 
framework and use driver specific functions.

And the CDMA seems to support the dmaengine API intended behavior 
through what it calls the keyhole feature where the address is not 
incremented. And if somebody wanted to use that feature they wouldn't be 
able to add support for it because it will break your usage of the 
prep_slave_sg() API.

It seems to me what we are missing from the DMAengine API is the 
equivalent of device_prep_dma_memcpy() that is able to take SG lists. 
There is already a memset_sg, it should be possible to add something 
similar for memcpy.

  - Lars

Re: [EXTERNAL] Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Adrian Larumbe]

On 23.04.2021 11:17, Lars-Peter Clausen wrote:

> The device_config() and device_prep_slave_sg() APIs are meant for
> memory-to-device or device-to-memory transfers. This means the device address
> is fixed and usually maps to a FIFO in the device.
> 
> What you are implementing is memory-to-memory, which means addresses are
> incremented on both sides of transfer. And this works if you know what you are
> doing, e.g. you know that you are using a specific dmaengine driver that
> implements the dmaengine API in a way that does not comply with the
> specification. But it breaks for generic dmaengine API clients that expect
> compliant behavior. And it is the reason why we have an API in the first
> place, to get consistent behavior. If you have to know about the driver
> specific semantics you might as well just bypass the framework and use driver
> specific functions.

You're absolutely right about this. I think I might've been confused when
picking the right DMA Engine API callback to write this new functionality into
by the fact that, on our particular configuration, transfers were being done
between main system memory and DDR memory on a PCI card. However, from the point
of view of the semantics of the API, these are still memory-to-memory transfers,
just like you said.

> It seems to me what we are missing from the DMAengine API is the equivalent of
> device_prep_dma_memcpy() that is able to take SG lists. There is already a
> memset_sg, it should be possible to add something similar for memcpy.

I wasn't aware of the existence of this callback. Grepping the sources reveals a
single consumer at the moment, I guess this also might've led me to wrongly
conclude device_prep_slave_sg was the right choice. Perhaps I can convert most
of the code currently in xilinx_cdma_prep_slave_sg to fit the semantics of this
DMAengine API function instead. Any thoughts on this?

Cheers,
Adrian

Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Vinod Koul]

On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> 
> It seems to me what we are missing from the DMAengine API is the equivalent
> of device_prep_dma_memcpy() that is able to take SG lists. There is already
> a memset_sg, it should be possible to add something similar for memcpy.

You mean something like dmaengine_prep_dma_sg() which was removed?

static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
               struct dma_chan *chan,
               struct scatterlist *dst_sg, unsigned int dst_nents,
               struct scatterlist *src_sg, unsigned int src_nents,
               unsigned long flags)

The problem with this API is that it would work only when src_sg and
dst_sg is of similar nature, if not then how should one go about
copying...should we fill without a care for dst_sg being different than
src_sg as long as total data to be copied has enough space in dst...

We can always add this back if we have in-kernel user but the semantics
of the API needs to be thought thru

Thanks
-- 
~Vinod

Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Lars-Peter Clausen]

On 4/23/21 3:24 PM, Vinod Koul wrote:
> On 23-04-21, 11:17, Lars-Peter Clausen wrote:
>> It seems to me what we are missing from the DMAengine API is the equivalent
>> of device_prep_dma_memcpy() that is able to take SG lists. There is already
>> a memset_sg, it should be possible to add something similar for memcpy.
> You mean something like dmaengine_prep_dma_sg() which was removed?
>
Ah, that's why I could have sworn we already had this!

> static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
>                 struct dma_chan *chan,
>                 struct scatterlist *dst_sg, unsigned int dst_nents,
>                 struct scatterlist *src_sg, unsigned int src_nents,
>                 unsigned long flags)
>
> The problem with this API is that it would work only when src_sg and
> dst_sg is of similar nature, if not then how should one go about
> copying...should we fill without a care for dst_sg being different than
> src_sg as long as total data to be copied has enough space in dst...
At least for the CDMA the only requirement is that both buffers have the 
same total size.

Re: [EXTERNAL] Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Adrian Larumbe]

On 23.04.2021 15:51, Lars-Peter Clausen wrote:

> > On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> > > It seems to me what we are missing from the DMAengine API is the equivalent
> > > of device_prep_dma_memcpy() that is able to take SG lists. There is already
> > > a memset_sg, it should be possible to add something similar for memcpy.
> > You mean something like dmaengine_prep_dma_sg() which was removed?
> > 
> Ah, that's why I could have sworn we already had this!

Yes, after that API function was removed, the Xilinx DMA driver effectively
ceased to support memory-to-memory SG transfers. My assumption was if it wasn't
ever reimplemented through the callback you mentioned before, is because there
isn't truly much interest in this device.

However we do need this functionality in our system. At the moment, and for our
particular use of it, we can always guarantee that either the source or
destination will be one contiguous chunk of memory, so just one scatterlist
pointer array was enough to fully program an operation. I think this would fit
alright into memset_sg.  What do you think?

Also, at the moment we're using it for transfers between main memory and
CPU-mapped PCI memory, which cannot be allocated with kmalloc. Wouldn't this
fall into the use case for device_prep_slave_sg?

Adrian

Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Vinod Koul]

On 23-04-21, 15:51, Lars-Peter Clausen wrote:
> On 4/23/21 3:24 PM, Vinod Koul wrote:
> > On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> > > It seems to me what we are missing from the DMAengine API is the equivalent
> > > of device_prep_dma_memcpy() that is able to take SG lists. There is already
> > > a memset_sg, it should be possible to add something similar for memcpy.
> > You mean something like dmaengine_prep_dma_sg() which was removed?
> > 
> Ah, that's why I could have sworn we already had this!

Even at that time we had the premise that we can bring the API back if
we had users. I think many have asked for it, but I havent seen a patch
with user yet :)

> > static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
> >                 struct dma_chan *chan,
> >                 struct scatterlist *dst_sg, unsigned int dst_nents,
> >                 struct scatterlist *src_sg, unsigned int src_nents,
> >                 unsigned long flags)
> > 
> > The problem with this API is that it would work only when src_sg and
> > dst_sg is of similar nature, if not then how should one go about
> > copying...should we fill without a care for dst_sg being different than
> > src_sg as long as total data to be copied has enough space in dst...
> At least for the CDMA the only requirement is that both buffers have the
> same total size.

I will merge if with a user but semantics need to be absolutely clear on
what is allowed and not, do I hear a volunteer ?
-- 
~Vinod

Re: [PATCH 0/4] Expand Xilinx CDMA functions

[radhey pandey]

On Fri, Apr 23, 2021 at 10:51 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> On 23-04-21, 15:51, Lars-Peter Clausen wrote:
> > On 4/23/21 3:24 PM, Vinod Koul wrote:
> > > On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> > > > It seems to me what we are missing from the DMAengine API is the equivalent
> > > > of device_prep_dma_memcpy() that is able to take SG lists. There is already
> > > > a memset_sg, it should be possible to add something similar for memcpy.
> > > You mean something like dmaengine_prep_dma_sg() which was removed?
> > >
> > Ah, that's why I could have sworn we already had this!
>
> Even at that time we had the premise that we can bring the API back if
> we had users. I think many have asked for it, but I havent seen a patch
> with user yet :)
Right.  Back then we had also discussed bringing the dma_sg API
but the idea was dropped as we didn't had a xilinx/any consumer
client driver for it in the mainline kernel.

I think it's the same state now.
>
> > > static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
> > >                 struct dma_chan *chan,
> > >                 struct scatterlist *dst_sg, unsigned int dst_nents,
> > >                 struct scatterlist *src_sg, unsigned int src_nents,
> > >                 unsigned long flags)
> > >
> > > The problem with this API is that it would work only when src_sg and
> > > dst_sg is of similar nature, if not then how should one go about
> > > copying...should we fill without a care for dst_sg being different than
> > > src_sg as long as total data to be copied has enough space in dst...
> > At least for the CDMA the only requirement is that both buffers have the
> > same total size.
>
> I will merge if with a user but semantics need to be absolutely clear on
> what is allowed and not, do I hear a volunteer ?
> --
> ~Vinod

Re: [EXTERNAL] Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Adrian Larumbe]

On 01.06.2021 15:59, radhey pandey wrote:
> On Fri, Apr 23, 2021 at 10:51 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > On 23-04-21, 15:51, Lars-Peter Clausen wrote:
> > > On 4/23/21 3:24 PM, Vinod Koul wrote:
> > > > On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> > > > > It seems to me what we are missing from the DMAengine API is the equivalent
> > > > > of device_prep_dma_memcpy() that is able to take SG lists. There is already
> > > > > a memset_sg, it should be possible to add something similar for memcpy.
> > > > You mean something like dmaengine_prep_dma_sg() which was removed?
> > > >
> > > Ah, that's why I could have sworn we already had this!
> >
> > Even at that time we had the premise that we can bring the API back if
> > we had users. I think many have asked for it, but I havent seen a patch
> > with user yet :)
> Right.  Back then we had also discussed bringing the dma_sg API
> but the idea was dropped as we didn't had a xilinx/any consumer
> client driver for it in the mainline kernel.
> 
> I think it's the same state now.

Would it be alright if I brought back the old DMA_SG interface that was removed
in commit c678fa66341c?  It seems that what I've effectively done is
implementing the semantics of that API call under the guise of
dma_prep_slave. However I still need mem2mem SG transfer support on CDMA, which
seems long gone from the driver, even though the HW does offer it.

If people are fine with it I can restore that interface and CDMA as the sole consumer.

>
> > > static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_sg(
> > >                 struct dma_chan *chan,
> > >                 struct scatterlist *dst_sg, unsigned int dst_nents,
> > >                 struct scatterlist *src_sg, unsigned int src_nents,
> > >                 unsigned long flags)
> > >
> > > The problem with this API is that it would work only when src_sg and
> > > dst_sg is of similar nature, if not then how should one go about
> > > copying...should we fill without a care for dst_sg being different than
> > > src_sg as long as total data to be copied has enough space in dst...
> > At least for the CDMA the only requirement is that both buffers have the
> > same total size.
>
> I will merge if with a user but semantics need to be absolutely clear on
> what is allowed and not, do I hear a volunteer ?
> --
> ~Vinod

Re: [EXTERNAL] Re: [PATCH 0/4] Expand Xilinx CDMA functions

[Vinod Koul]

On 02-07-21, 15:23, Adrian Larumbe wrote:
> On 01.06.2021 15:59, radhey pandey wrote:
> > On Fri, Apr 23, 2021 at 10:51 PM Vinod Koul <vkoul@kernel.org> wrote:
> > >
> > > On 23-04-21, 15:51, Lars-Peter Clausen wrote:
> > > > On 4/23/21 3:24 PM, Vinod Koul wrote:
> > > > > On 23-04-21, 11:17, Lars-Peter Clausen wrote:
> > > > > > It seems to me what we are missing from the DMAengine API is the equivalent
> > > > > > of device_prep_dma_memcpy() that is able to take SG lists. There is already
> > > > > > a memset_sg, it should be possible to add something similar for memcpy.
> > > > > You mean something like dmaengine_prep_dma_sg() which was removed?
> > > > >
> > > > Ah, that's why I could have sworn we already had this!
> > >
> > > Even at that time we had the premise that we can bring the API back if
> > > we had users. I think many have asked for it, but I havent seen a patch
> > > with user yet :)
> > Right.  Back then we had also discussed bringing the dma_sg API
> > but the idea was dropped as we didn't had a xilinx/any consumer
> > client driver for it in the mainline kernel.
> > 
> > I think it's the same state now.
> 
> Would it be alright if I brought back the old DMA_SG interface that was removed
> in commit c678fa66341c?  It seems that what I've effectively done is
> implementing the semantics of that API call under the guise of
> dma_prep_slave. However I still need mem2mem SG transfer support on CDMA, which
> seems long gone from the driver, even though the HW does offer it.
> 
> If people are fine with it I can restore that interface and CDMA as the sole consumer.

I guess I should start putting this regularly now! Yes it is okay to
bring dma sg support, provided
1. We have a user
2. the sematics of how that api works is well defined. Esp in the case where is src_sg and dstn_sg are different

Also, I would not accept patches which implement this in guise of a
different API.

Thanks

-- 
~Vinod