The metadata is best described as side band data or parameters traveling
alongside the data DMAd by the DMA engine. It is data
which is understood by the peripheral and the peripheral driver only, the
DMA engine see it only as data block and it is not interpreting it in any
way.
The metadata can be different per descriptor as it is a parameter for the
data being transferred.
If the DMA supports per descriptor metadata it can implement the attach,
get_ptr/set_len callbacks.
Client drivers must only use either attach or get_ptr/set_len to avoid
miss configuration.
Client driver can check if a given metadata mode is supported by the
channel during probe time with
dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_CLIENT);
dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_ENGINE);
and based on this information can use either mode.
Wrappers are also added for the metadata_ops.
To be used in DESC_METADATA_CLIENT mode:
dmaengine_desc_attach_metadata()
To be used in DESC_METADATA_ENGINE mode:
dmaengine_desc_get_metadata_ptr()
dmaengine_desc_set_metadata_len()
Signed-off-by: Peter Ujfalusi <[email protected]>
---
Hi,
Changes since rfc:
- DESC_METADATA_EMBEDDED renamed to DESC_METADATA_ENGINE
- Use flow is added for both CLIENT and ENGINE metadata modes
Regards,
Peter
include/linux/dmaengine.h | 144 ++++++++++++++++++++++++++++++++++++++
1 file changed, 144 insertions(+)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 3db833a8c542..f809635cfeaa 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -231,6 +231,57 @@ typedef struct { DECLARE_BITMAP(bits, DMA_TX_TYPE_END); } dma_cap_mask_t;
* @bytes_transferred: byte counter
*/
+/**
+ * enum dma_desc_metadata_mode - per descriptor metadata mode types supported
+ * @DESC_METADATA_CLIENT - the metadata buffer is allocated/provided by the
+ * client driver and it is attached (via the dmaengine_desc_attach_metadata()
+ * helper) to the descriptor.
+ *
+ * Client drivers interested to use this mode can follow:
+ * - DMA_MEM_TO_DEV:
+ * 1. prepare the descriptor (dmaengine_prep_*)
+ * construct the metadata in the clinet's buffer
+ * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
+ * descriptor
+ * 3. submit the transfer
+ * - DMA_DEV_TO_MEM:
+ * 1. prepare the descriptor (dmaengine_prep_*)
+ * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
+ * descriptor
+ * 3. submit the transfer
+ * 4. when the transfer is completed, the metadata should be available in the
+ * attached buffer
+ *
+ * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
+ * driver. The client driver can ask for the pointer, maximum size and the
+ * currently used size of the metadata and can directly update or read it.
+ * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
+ * provided as helper functions.
+ *
+ * Client drivers interested to use this mode can follow:
+ * - DMA_MEM_TO_DEV:
+ * 1. prepare the descriptor (dmaengine_prep_*)
+ * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
+ * metadata area
+ * 3. update the metadata at the pointer
+ * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
+ * of data the client has placed into the metadata buffer
+ * 5. submit the transfer
+ * - DMA_DEV_TO_MEM:
+ * 1. prepare the descriptor (dmaengine_prep_*)
+ * 2. submit the transfer
+ * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
+ * pointer to the engine's metadata are
+ * 4. Read out the metadate from the pointer
+ *
+ * Note: the two mode is not compatible and clients must use one mode for a
+ * descriptor.
+ */
+enum dma_desc_metadata_mode {
+ DESC_METADATA_CLIENT = (1 << 0),
+ DESC_METADATA_ENGINE = (1 << 1),
+};
+
struct dma_chan_percpu {
/* stats */
unsigned long memcpy_count;
@@ -494,6 +545,18 @@ struct dmaengine_unmap_data {
dma_addr_t addr[0];
};
+struct dma_async_tx_descriptor;
+
+struct dma_descriptor_metadata_ops {
+ int (*attach)(struct dma_async_tx_descriptor *desc, void *data,
+ size_t len);
+
+ void *(*get_ptr)(struct dma_async_tx_descriptor *desc,
+ size_t *payload_len, size_t *max_len);
+ int (*set_len)(struct dma_async_tx_descriptor *desc,
+ size_t payload_len);
+};
+
/**
* struct dma_async_tx_descriptor - async transaction descriptor
* ---dma generic offload fields---
@@ -523,6 +586,8 @@ struct dma_async_tx_descriptor {
dma_async_tx_callback_result callback_result;
void *callback_param;
struct dmaengine_unmap_data *unmap;
+ enum dma_desc_metadata_mode desc_metadata_mode;
+ struct dma_descriptor_metadata_ops *metadata_ops;
#ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
struct dma_async_tx_descriptor *next;
struct dma_async_tx_descriptor *parent;
@@ -685,6 +750,7 @@ struct dma_filter {
* @global_node: list_head for global dma_device_list
* @filter: information for device/slave to filter function/param mapping
* @cap_mask: one or more dma_capability flags
+ * @desc_metadata_modes: supported metadata modes by the DMA device
* @max_xor: maximum number of xor sources, 0 if no capability
* @max_pq: maximum number of PQ sources and PQ-continue capability
* @copy_align: alignment shift for memcpy operations
@@ -749,6 +815,7 @@ struct dma_device {
struct list_head global_node;
struct dma_filter filter;
dma_cap_mask_t cap_mask;
+ enum dma_desc_metadata_mode desc_metadata_modes;
unsigned short max_xor;
unsigned short max_pq;
enum dmaengine_alignment copy_align;
@@ -935,6 +1002,83 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy(
len, flags);
}
+static inline bool dmaengine_is_metadata_mode_supported(struct dma_chan *chan,
+ enum dma_desc_metadata_mode mode)
+{
+ return !!(chan->device->desc_metadata_modes & mode);
+}
+
+static inline int _desc_check_and_set_metadata_mode(
+ struct dma_async_tx_descriptor *desc, enum dma_desc_metadata_mode mode)
+{
+ /* Make sure that the metadata mode is not mixed */
+ if (!desc->desc_metadata_mode) {
+ if (dmaengine_is_metadata_mode_supported(desc->chan, mode))
+ desc->desc_metadata_mode = mode;
+ else
+ return -ENOTSUPP;
+ } else if (desc->desc_metadata_mode != mode) {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static inline int dmaengine_desc_attach_metadata(
+ struct dma_async_tx_descriptor *desc, void *data, size_t len)
+{
+ int ret;
+
+ if (!desc)
+ return -EINVAL;
+
+ ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_CLIENT);
+ if (ret)
+ return ret;
+
+ if (!desc->metadata_ops || !desc->metadata_ops->attach)
+ return -ENOTSUPP;
+
+ return desc->metadata_ops->attach(desc, data, len);
+}
+
+static inline void *dmaengine_desc_get_metadata_ptr(
+ struct dma_async_tx_descriptor *desc, size_t *payload_len,
+ size_t *max_len)
+{
+ int ret;
+
+ if (!desc)
+ return ERR_PTR(-EINVAL);
+
+ ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (!desc->metadata_ops || !desc->metadata_ops->get_ptr)
+ return ERR_PTR(-ENOTSUPP);
+
+ return desc->metadata_ops->get_ptr(desc, payload_len, max_len);
+}
+
+static inline int dmaengine_desc_set_metadata_len(
+ struct dma_async_tx_descriptor *desc, size_t payload_len)
+{
+ int ret;
+
+ if (!desc)
+ return -EINVAL;
+
+ ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
+ if (ret)
+ return ret;
+
+ if (!desc->metadata_ops || !desc->metadata_ops->set_len)
+ return -ENOTSUPP;
+
+ return desc->metadata_ops->set_len(desc, payload_len);
+}
+
/**
* dmaengine_terminate_all() - Terminate all active DMA transfers
* @chan: The channel for which to terminate the transfers
--
Peter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 23-08-18, 16:07, Peter Ujfalusi wrote:
> The metadata is best described as side band data or parameters traveling
> alongside the data DMAd by the DMA engine. It is data
> which is understood by the peripheral and the peripheral driver only, the
> DMA engine see it only as data block and it is not interpreting it in any
> way.
>
> The metadata can be different per descriptor as it is a parameter for the
> data being transferred.
>
> If the DMA supports per descriptor metadata it can implement the attach,
> get_ptr/set_len callbacks.
>
> Client drivers must only use either attach or get_ptr/set_len to avoid
> miss configuration.
misconfiguration?
> Client driver can check if a given metadata mode is supported by the
> channel during probe time with
> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_CLIENT);
> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_ENGINE);
>
> and based on this information can use either mode.
>
> Wrappers are also added for the metadata_ops.
>
> To be used in DESC_METADATA_CLIENT mode:
> dmaengine_desc_attach_metadata()
>
> To be used in DESC_METADATA_ENGINE mode:
> dmaengine_desc_get_metadata_ptr()
> dmaengine_desc_set_metadata_len()
>
> Signed-off-by: Peter Ujfalusi <[email protected]>
> ---
> Hi,
>
> Changes since rfc:
> - DESC_METADATA_EMBEDDED renamed to DESC_METADATA_ENGINE
> - Use flow is added for both CLIENT and ENGINE metadata modes
>
> Regards,
> Peter
>
> include/linux/dmaengine.h | 144 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 144 insertions(+)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 3db833a8c542..f809635cfeaa 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -231,6 +231,57 @@ typedef struct { DECLARE_BITMAP(bits, DMA_TX_TYPE_END); } dma_cap_mask_t;
> * @bytes_transferred: byte counter
> */
>
> +/**
> + * enum dma_desc_metadata_mode - per descriptor metadata mode types supported
> + * @DESC_METADATA_CLIENT - the metadata buffer is allocated/provided by the
> + * client driver and it is attached (via the dmaengine_desc_attach_metadata()
> + * helper) to the descriptor.
> + *
> + * Client drivers interested to use this mode can follow:
> + * - DMA_MEM_TO_DEV:
> + * 1. prepare the descriptor (dmaengine_prep_*)
> + * construct the metadata in the clinet's buffer
typo clinet
> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> + * descriptor
> + * 3. submit the transfer
> + * - DMA_DEV_TO_MEM:
> + * 1. prepare the descriptor (dmaengine_prep_*)
> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> + * descriptor
> + * 3. submit the transfer
> + * 4. when the transfer is completed, the metadata should be available in the
> + * attached buffer
I guess this is good to be moved into Documentation
also we dont allow this for memcpy txn?
> + *
> + * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
> + * driver. The client driver can ask for the pointer, maximum size and the
> + * currently used size of the metadata and can directly update or read it.
> + * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
> + * provided as helper functions.
> + *
> + * Client drivers interested to use this mode can follow:
> + * - DMA_MEM_TO_DEV:
> + * 1. prepare the descriptor (dmaengine_prep_*)
> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
> + * metadata area
> + * 3. update the metadata at the pointer
> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
> + * of data the client has placed into the metadata buffer
> + * 5. submit the transfer
> + * - DMA_DEV_TO_MEM:
> + * 1. prepare the descriptor (dmaengine_prep_*)
> + * 2. submit the transfer
> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
> + * pointer to the engine's metadata are
> + * 4. Read out the metadate from the pointer
> + *
> + * Note: the two mode is not compatible and clients must use one mode for a
> + * descriptor.
> + */
> +enum dma_desc_metadata_mode {
> + DESC_METADATA_CLIENT = (1 << 0),
> + DESC_METADATA_ENGINE = (1 << 1),
BIT(x)
> +};
> +
> struct dma_chan_percpu {
> /* stats */
> unsigned long memcpy_count;
> @@ -494,6 +545,18 @@ struct dmaengine_unmap_data {
> dma_addr_t addr[0];
> };
>
> +struct dma_async_tx_descriptor;
> +
> +struct dma_descriptor_metadata_ops {
> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data,
> + size_t len);
> +
> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc,
> + size_t *payload_len, size_t *max_len);
> + int (*set_len)(struct dma_async_tx_descriptor *desc,
> + size_t payload_len);
> +};
> +
> /**
> * struct dma_async_tx_descriptor - async transaction descriptor
> * ---dma generic offload fields---
> @@ -523,6 +586,8 @@ struct dma_async_tx_descriptor {
> dma_async_tx_callback_result callback_result;
> void *callback_param;
> struct dmaengine_unmap_data *unmap;
> + enum dma_desc_metadata_mode desc_metadata_mode;
> + struct dma_descriptor_metadata_ops *metadata_ops;
> #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
> struct dma_async_tx_descriptor *next;
> struct dma_async_tx_descriptor *parent;
> @@ -685,6 +750,7 @@ struct dma_filter {
> * @global_node: list_head for global dma_device_list
> * @filter: information for device/slave to filter function/param mapping
> * @cap_mask: one or more dma_capability flags
> + * @desc_metadata_modes: supported metadata modes by the DMA device
> * @max_xor: maximum number of xor sources, 0 if no capability
> * @max_pq: maximum number of PQ sources and PQ-continue capability
> * @copy_align: alignment shift for memcpy operations
> @@ -749,6 +815,7 @@ struct dma_device {
> struct list_head global_node;
> struct dma_filter filter;
> dma_cap_mask_t cap_mask;
> + enum dma_desc_metadata_mode desc_metadata_modes;
> unsigned short max_xor;
> unsigned short max_pq;
> enum dmaengine_alignment copy_align;
> @@ -935,6 +1002,83 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy(
> len, flags);
> }
>
> +static inline bool dmaengine_is_metadata_mode_supported(struct dma_chan *chan,
> + enum dma_desc_metadata_mode mode)
> +{
> + return !!(chan->device->desc_metadata_modes & mode);
> +}
> +
> +static inline int _desc_check_and_set_metadata_mode(
why does this need to start with _ ?
> + struct dma_async_tx_descriptor *desc, enum dma_desc_metadata_mode mode)
> +{
> + /* Make sure that the metadata mode is not mixed */
> + if (!desc->desc_metadata_mode) {
> + if (dmaengine_is_metadata_mode_supported(desc->chan, mode))
> + desc->desc_metadata_mode = mode;
> + else
> + return -ENOTSUPP;
> + } else if (desc->desc_metadata_mode != mode) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static inline int dmaengine_desc_attach_metadata(
> + struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> + int ret;
> +
> + if (!desc)
> + return -EINVAL;
> +
> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_CLIENT);
> + if (ret)
> + return ret;
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->attach)
> + return -ENOTSUPP;
> +
> + return desc->metadata_ops->attach(desc, data, len);
> +}
> +
> +static inline void *dmaengine_desc_get_metadata_ptr(
> + struct dma_async_tx_descriptor *desc, size_t *payload_len,
> + size_t *max_len)
> +{
> + int ret;
> +
> + if (!desc)
> + return ERR_PTR(-EINVAL);
> +
> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->get_ptr)
> + return ERR_PTR(-ENOTSUPP);
> +
> + return desc->metadata_ops->get_ptr(desc, payload_len, max_len);
> +}
> +
> +static inline int dmaengine_desc_set_metadata_len(
> + struct dma_async_tx_descriptor *desc, size_t payload_len)
> +{
> + int ret;
> +
> + if (!desc)
> + return -EINVAL;
> +
> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
> + if (ret)
> + return ret;
> +
> + if (!desc->metadata_ops || !desc->metadata_ops->set_len)
> + return -ENOTSUPP;
> +
> + return desc->metadata_ops->set_len(desc, payload_len);
> +}
thats bit too much code for a header file :( Lets move it to C file
please. We can utilize local dmaengine.h and not expose all these and
possible misuse by clients
Also I would like to see a use :-) before further comments.
--
~Vinod
Vinod,
On 08/29/2018 06:52 PM, Vinod wrote:
> On 23-08-18, 16:07, Peter Ujfalusi wrote:
>> The metadata is best described as side band data or parameters traveling
>> alongside the data DMAd by the DMA engine. It is data
>> which is understood by the peripheral and the peripheral driver only, the
>> DMA engine see it only as data block and it is not interpreting it in any
>> way.
>>
>> The metadata can be different per descriptor as it is a parameter for the
>> data being transferred.
>>
>> If the DMA supports per descriptor metadata it can implement the attach,
>> get_ptr/set_len callbacks.
>>
>> Client drivers must only use either attach or get_ptr/set_len to avoid
>> miss configuration.
>
> misconfiguration?
Sorry for the typos, I'll got through them again.
>> Client driver can check if a given metadata mode is supported by the
>> channel during probe time with
>> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_CLIENT);
>> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_ENGINE);
>>
>> and based on this information can use either mode.
>>
>> Wrappers are also added for the metadata_ops.
>>
>> To be used in DESC_METADATA_CLIENT mode:
>> dmaengine_desc_attach_metadata()
>>
>> To be used in DESC_METADATA_ENGINE mode:
>> dmaengine_desc_get_metadata_ptr()
>> dmaengine_desc_set_metadata_len()
>>
>> Signed-off-by: Peter Ujfalusi <[email protected]>
>> ---
>> Hi,
>>
>> Changes since rfc:
>> - DESC_METADATA_EMBEDDED renamed to DESC_METADATA_ENGINE
>> - Use flow is added for both CLIENT and ENGINE metadata modes
>>
>> Regards,
>> Peter
>>
>> include/linux/dmaengine.h | 144 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 144 insertions(+)
>>
>> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
>> index 3db833a8c542..f809635cfeaa 100644
>> --- a/include/linux/dmaengine.h
>> +++ b/include/linux/dmaengine.h
>> @@ -231,6 +231,57 @@ typedef struct { DECLARE_BITMAP(bits, DMA_TX_TYPE_END); } dma_cap_mask_t;
>> * @bytes_transferred: byte counter
>> */
>>
>> +/**
>> + * enum dma_desc_metadata_mode - per descriptor metadata mode types supported
>> + * @DESC_METADATA_CLIENT - the metadata buffer is allocated/provided by the
>> + * client driver and it is attached (via the dmaengine_desc_attach_metadata()
>> + * helper) to the descriptor.
>> + *
>> + * Client drivers interested to use this mode can follow:
>> + * - DMA_MEM_TO_DEV:
>> + * 1. prepare the descriptor (dmaengine_prep_*)
>> + * construct the metadata in the clinet's buffer
>
> typo clinet
>
>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
>> + * descriptor
>> + * 3. submit the transfer
>> + * - DMA_DEV_TO_MEM:
>> + * 1. prepare the descriptor (dmaengine_prep_*)
>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
>> + * descriptor
>> + * 3. submit the transfer
>> + * 4. when the transfer is completed, the metadata should be available in the
>> + * attached buffer
>
> I guess this is good to be moved into Documentation
Should I create a new file for metadata? I guess it would make sense as the
information is for both clients and engines.
>
> also we dont allow this for memcpy txn?
I have not thought about that, but if I think about it it should be along the
same lines as MEM_TO_DEV.
I'll add the MEM_TO_MEM as well to the documentation.
>> + *
>> + * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
>> + * driver. The client driver can ask for the pointer, maximum size and the
>> + * currently used size of the metadata and can directly update or read it.
>> + * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
>> + * provided as helper functions.
>> + *
>> + * Client drivers interested to use this mode can follow:
>> + * - DMA_MEM_TO_DEV:
>> + * 1. prepare the descriptor (dmaengine_prep_*)
>> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
>> + * metadata area
>> + * 3. update the metadata at the pointer
>> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
>> + * of data the client has placed into the metadata buffer
>> + * 5. submit the transfer
>> + * - DMA_DEV_TO_MEM:
>> + * 1. prepare the descriptor (dmaengine_prep_*)
>> + * 2. submit the transfer
>> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
>> + * pointer to the engine's metadata are
>> + * 4. Read out the metadate from the pointer
>> + *
>> + * Note: the two mode is not compatible and clients must use one mode for a
>> + * descriptor.
>> + */
>> +enum dma_desc_metadata_mode {
>> + DESC_METADATA_CLIENT = (1 << 0),
>> + DESC_METADATA_ENGINE = (1 << 1),
>
> BIT(x)
OK, I followed what we have in the header to not mix (1 << x) and BIT(x)
>
>> +};
>> +
>> struct dma_chan_percpu {
>> /* stats */
>> unsigned long memcpy_count;
>> @@ -494,6 +545,18 @@ struct dmaengine_unmap_data {
>> dma_addr_t addr[0];
>> };
>>
>> +struct dma_async_tx_descriptor;
>> +
>> +struct dma_descriptor_metadata_ops {
>> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data,
>> + size_t len);
>> +
>> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc,
>> + size_t *payload_len, size_t *max_len);
>> + int (*set_len)(struct dma_async_tx_descriptor *desc,
>> + size_t payload_len);
>> +};
>> +
>> /**
>> * struct dma_async_tx_descriptor - async transaction descriptor
>> * ---dma generic offload fields---
>> @@ -523,6 +586,8 @@ struct dma_async_tx_descriptor {
>> dma_async_tx_callback_result callback_result;
>> void *callback_param;
>> struct dmaengine_unmap_data *unmap;
>> + enum dma_desc_metadata_mode desc_metadata_mode;
>> + struct dma_descriptor_metadata_ops *metadata_ops;
I forgot to update the comment section for the dma_async_tx_descriptor, I'll
address it in v2.
>> #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
>> struct dma_async_tx_descriptor *next;
>> struct dma_async_tx_descriptor *parent;
>> @@ -685,6 +750,7 @@ struct dma_filter {
>> * @global_node: list_head for global dma_device_list
>> * @filter: information for device/slave to filter function/param mapping
>> * @cap_mask: one or more dma_capability flags
>> + * @desc_metadata_modes: supported metadata modes by the DMA device
>> * @max_xor: maximum number of xor sources, 0 if no capability
>> * @max_pq: maximum number of PQ sources and PQ-continue capability
>> * @copy_align: alignment shift for memcpy operations
>> @@ -749,6 +815,7 @@ struct dma_device {
>> struct list_head global_node;
>> struct dma_filter filter;
>> dma_cap_mask_t cap_mask;
>> + enum dma_desc_metadata_mode desc_metadata_modes;
>> unsigned short max_xor;
>> unsigned short max_pq;
>> enum dmaengine_alignment copy_align;
>> @@ -935,6 +1002,83 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy(
>> len, flags);
>> }
>>
>> +static inline bool dmaengine_is_metadata_mode_supported(struct dma_chan *chan,
>> + enum dma_desc_metadata_mode mode)
>> +{
>> + return !!(chan->device->desc_metadata_modes & mode);
>> +}
>> +
>> +static inline int _desc_check_and_set_metadata_mode(
>
> why does this need to start with _ ?
To scare people to use in client code ;)
>
>> + struct dma_async_tx_descriptor *desc, enum dma_desc_metadata_mode mode)
>> +{
>> + /* Make sure that the metadata mode is not mixed */
>> + if (!desc->desc_metadata_mode) {
>> + if (dmaengine_is_metadata_mode_supported(desc->chan, mode))
>> + desc->desc_metadata_mode = mode;
>> + else
>> + return -ENOTSUPP;
>> + } else if (desc->desc_metadata_mode != mode) {
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static inline int dmaengine_desc_attach_metadata(
>> + struct dma_async_tx_descriptor *desc, void *data, size_t len)
>> +{
>> + int ret;
>> +
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_CLIENT);
>> + if (ret)
>> + return ret;
>> +
>> + if (!desc->metadata_ops || !desc->metadata_ops->attach)
>> + return -ENOTSUPP;
>> +
>> + return desc->metadata_ops->attach(desc, data, len);
>> +}
>> +
>> +static inline void *dmaengine_desc_get_metadata_ptr(
>> + struct dma_async_tx_descriptor *desc, size_t *payload_len,
>> + size_t *max_len)
>> +{
>> + int ret;
>> +
>> + if (!desc)
>> + return ERR_PTR(-EINVAL);
>> +
>> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
>> + if (ret)
>> + return ERR_PTR(ret);
>> +
>> + if (!desc->metadata_ops || !desc->metadata_ops->get_ptr)
>> + return ERR_PTR(-ENOTSUPP);
>> +
>> + return desc->metadata_ops->get_ptr(desc, payload_len, max_len);
>> +}
>> +
>> +static inline int dmaengine_desc_set_metadata_len(
>> + struct dma_async_tx_descriptor *desc, size_t payload_len)
>> +{
>> + int ret;
>> +
>> + if (!desc)
>> + return -EINVAL;
>> +
>> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
>> + if (ret)
>> + return ret;
>> +
>> + if (!desc->metadata_ops || !desc->metadata_ops->set_len)
>> + return -ENOTSUPP;
>> +
>> + return desc->metadata_ops->set_len(desc, payload_len);
>> +}
>
> thats bit too much code for a header file :( Lets move it to C file
> please. We can utilize local dmaengine.h and not expose all these and
> possible misuse by clients
OK, I have thought about that as well.
> Also I would like to see a use :-) before further comments.
You mean the DMA driver and at least one client?
I have the DMA driver in my public facing branch [1], but it is not an easy
read with it's close to 4k loc.
The client is not in my branch and it is actually using an older version of
the metadata support.
The problem is that I don't know when I will be able to send the driver for
review as all of this is targeting a brand new SoC (AM654) with completely new
data movement architecture. There are lots of dependencies still need to be
upstreamed before I can send something which at least compiles.
I can offer snippets from the client driver, if that is good enough or a link
to the public tree where it can be accessed, but it is not going to go
upstream before the DMA driver.
[1]
https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti/
https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti
k3-udma.c/h is the DMAengine driver, k3-navss-pricate.c k3-navss-udma.c is
something I try to get rid of (it is wrapper for the networking drivers to use
DMA)
--
Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 29-08-18, 19:14, Peter Ujfalusi wrote:
> Vinod,
>
> On 08/29/2018 06:52 PM, Vinod wrote:
> > On 23-08-18, 16:07, Peter Ujfalusi wrote:
> >> The metadata is best described as side band data or parameters traveling
> >> alongside the data DMAd by the DMA engine. It is data
> >> which is understood by the peripheral and the peripheral driver only, the
> >> DMA engine see it only as data block and it is not interpreting it in any
> >> way.
> >>
> >> The metadata can be different per descriptor as it is a parameter for the
> >> data being transferred.
> >>
> >> If the DMA supports per descriptor metadata it can implement the attach,
> >> get_ptr/set_len callbacks.
> >>
> >> Client drivers must only use either attach or get_ptr/set_len to avoid
> >> miss configuration.
> >
> > misconfiguration?
>
> Sorry for the typos, I'll got through them again.
>
> >> Client driver can check if a given metadata mode is supported by the
> >> channel during probe time with
> >> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_CLIENT);
> >> dmaengine_is_metadata_mode_supported(chan, DESC_METADATA_ENGINE);
> >>
> >> and based on this information can use either mode.
> >>
> >> Wrappers are also added for the metadata_ops.
> >>
> >> To be used in DESC_METADATA_CLIENT mode:
> >> dmaengine_desc_attach_metadata()
> >>
> >> To be used in DESC_METADATA_ENGINE mode:
> >> dmaengine_desc_get_metadata_ptr()
> >> dmaengine_desc_set_metadata_len()
> >>
> >> Signed-off-by: Peter Ujfalusi <[email protected]>
> >> ---
> >> Hi,
> >>
> >> Changes since rfc:
> >> - DESC_METADATA_EMBEDDED renamed to DESC_METADATA_ENGINE
> >> - Use flow is added for both CLIENT and ENGINE metadata modes
> >>
> >> Regards,
> >> Peter
> >>
> >> include/linux/dmaengine.h | 144 ++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 144 insertions(+)
> >>
> >> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> >> index 3db833a8c542..f809635cfeaa 100644
> >> --- a/include/linux/dmaengine.h
> >> +++ b/include/linux/dmaengine.h
> >> @@ -231,6 +231,57 @@ typedef struct { DECLARE_BITMAP(bits, DMA_TX_TYPE_END); } dma_cap_mask_t;
> >> * @bytes_transferred: byte counter
> >> */
> >>
> >> +/**
> >> + * enum dma_desc_metadata_mode - per descriptor metadata mode types supported
> >> + * @DESC_METADATA_CLIENT - the metadata buffer is allocated/provided by the
> >> + * client driver and it is attached (via the dmaengine_desc_attach_metadata()
> >> + * helper) to the descriptor.
> >> + *
> >> + * Client drivers interested to use this mode can follow:
> >> + * - DMA_MEM_TO_DEV:
> >> + * 1. prepare the descriptor (dmaengine_prep_*)
> >> + * construct the metadata in the clinet's buffer
> >
> > typo clinet
> >
> >> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> >> + * descriptor
> >> + * 3. submit the transfer
> >> + * - DMA_DEV_TO_MEM:
> >> + * 1. prepare the descriptor (dmaengine_prep_*)
> >> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> >> + * descriptor
> >> + * 3. submit the transfer
> >> + * 4. when the transfer is completed, the metadata should be available in the
> >> + * attached buffer
> >
> > I guess this is good to be moved into Documentation
>
> Should I create a new file for metadata? I guess it would make sense as the
> information is for both clients and engines.
Hmm not sure, lets see how it looks as entries in these files, detailing
roles of clients and providers
>
> >
> > also we dont allow this for memcpy txn?
>
> I have not thought about that, but if I think about it it should be along the
> same lines as MEM_TO_DEV.
> I'll add the MEM_TO_MEM as well to the documentation.
Okay and lets not implement it then..
>
> >> + *
> >> + * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
> >> + * driver. The client driver can ask for the pointer, maximum size and the
> >> + * currently used size of the metadata and can directly update or read it.
> >> + * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
> >> + * provided as helper functions.
> >> + *
> >> + * Client drivers interested to use this mode can follow:
> >> + * - DMA_MEM_TO_DEV:
> >> + * 1. prepare the descriptor (dmaengine_prep_*)
> >> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
> >> + * metadata area
> >> + * 3. update the metadata at the pointer
> >> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
> >> + * of data the client has placed into the metadata buffer
> >> + * 5. submit the transfer
> >> + * - DMA_DEV_TO_MEM:
> >> + * 1. prepare the descriptor (dmaengine_prep_*)
> >> + * 2. submit the transfer
> >> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
> >> + * pointer to the engine's metadata are
> >> + * 4. Read out the metadate from the pointer
> >> + *
> >> + * Note: the two mode is not compatible and clients must use one mode for a
> >> + * descriptor.
> >> + */
> >> +enum dma_desc_metadata_mode {
> >> + DESC_METADATA_CLIENT = (1 << 0),
> >> + DESC_METADATA_ENGINE = (1 << 1),
> >
> > BIT(x)
>
> OK, I followed what we have in the header to not mix (1 << x) and BIT(x)
yeah lets update :)
>
> >
> >> +};
> >> +
> >> struct dma_chan_percpu {
> >> /* stats */
> >> unsigned long memcpy_count;
> >> @@ -494,6 +545,18 @@ struct dmaengine_unmap_data {
> >> dma_addr_t addr[0];
> >> };
> >>
> >> +struct dma_async_tx_descriptor;
> >> +
> >> +struct dma_descriptor_metadata_ops {
> >> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data,
> >> + size_t len);
> >> +
> >> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc,
> >> + size_t *payload_len, size_t *max_len);
> >> + int (*set_len)(struct dma_async_tx_descriptor *desc,
> >> + size_t payload_len);
> >> +};
> >> +
> >> /**
> >> * struct dma_async_tx_descriptor - async transaction descriptor
> >> * ---dma generic offload fields---
> >> @@ -523,6 +586,8 @@ struct dma_async_tx_descriptor {
> >> dma_async_tx_callback_result callback_result;
> >> void *callback_param;
> >> struct dmaengine_unmap_data *unmap;
> >> + enum dma_desc_metadata_mode desc_metadata_mode;
> >> + struct dma_descriptor_metadata_ops *metadata_ops;
>
> I forgot to update the comment section for the dma_async_tx_descriptor, I'll
> address it in v2.
>
> >> #ifdef CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH
> >> struct dma_async_tx_descriptor *next;
> >> struct dma_async_tx_descriptor *parent;
> >> @@ -685,6 +750,7 @@ struct dma_filter {
> >> * @global_node: list_head for global dma_device_list
> >> * @filter: information for device/slave to filter function/param mapping
> >> * @cap_mask: one or more dma_capability flags
> >> + * @desc_metadata_modes: supported metadata modes by the DMA device
> >> * @max_xor: maximum number of xor sources, 0 if no capability
> >> * @max_pq: maximum number of PQ sources and PQ-continue capability
> >> * @copy_align: alignment shift for memcpy operations
> >> @@ -749,6 +815,7 @@ struct dma_device {
> >> struct list_head global_node;
> >> struct dma_filter filter;
> >> dma_cap_mask_t cap_mask;
> >> + enum dma_desc_metadata_mode desc_metadata_modes;
> >> unsigned short max_xor;
> >> unsigned short max_pq;
> >> enum dmaengine_alignment copy_align;
> >> @@ -935,6 +1002,83 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy(
> >> len, flags);
> >> }
> >>
> >> +static inline bool dmaengine_is_metadata_mode_supported(struct dma_chan *chan,
> >> + enum dma_desc_metadata_mode mode)
> >> +{
> >> + return !!(chan->device->desc_metadata_modes & mode);
> >> +}
> >> +
> >> +static inline int _desc_check_and_set_metadata_mode(
> >
> > why does this need to start with _ ?
>
> To scare people to use in client code ;)
Lets not expose to them :D
>
> >
> >> + struct dma_async_tx_descriptor *desc, enum dma_desc_metadata_mode mode)
> >> +{
> >> + /* Make sure that the metadata mode is not mixed */
> >> + if (!desc->desc_metadata_mode) {
> >> + if (dmaengine_is_metadata_mode_supported(desc->chan, mode))
> >> + desc->desc_metadata_mode = mode;
> >> + else
> >> + return -ENOTSUPP;
> >> + } else if (desc->desc_metadata_mode != mode) {
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static inline int dmaengine_desc_attach_metadata(
> >> + struct dma_async_tx_descriptor *desc, void *data, size_t len)
> >> +{
> >> + int ret;
> >> +
> >> + if (!desc)
> >> + return -EINVAL;
> >> +
> >> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_CLIENT);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!desc->metadata_ops || !desc->metadata_ops->attach)
> >> + return -ENOTSUPP;
> >> +
> >> + return desc->metadata_ops->attach(desc, data, len);
> >> +}
> >> +
> >> +static inline void *dmaengine_desc_get_metadata_ptr(
> >> + struct dma_async_tx_descriptor *desc, size_t *payload_len,
> >> + size_t *max_len)
> >> +{
> >> + int ret;
> >> +
> >> + if (!desc)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
> >> + if (ret)
> >> + return ERR_PTR(ret);
> >> +
> >> + if (!desc->metadata_ops || !desc->metadata_ops->get_ptr)
> >> + return ERR_PTR(-ENOTSUPP);
> >> +
> >> + return desc->metadata_ops->get_ptr(desc, payload_len, max_len);
> >> +}
> >> +
> >> +static inline int dmaengine_desc_set_metadata_len(
> >> + struct dma_async_tx_descriptor *desc, size_t payload_len)
> >> +{
> >> + int ret;
> >> +
> >> + if (!desc)
> >> + return -EINVAL;
> >> +
> >> + ret = _desc_check_and_set_metadata_mode(desc, DESC_METADATA_ENGINE);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!desc->metadata_ops || !desc->metadata_ops->set_len)
> >> + return -ENOTSUPP;
> >> +
> >> + return desc->metadata_ops->set_len(desc, payload_len);
> >> +}
> >
> > thats bit too much code for a header file :( Lets move it to C file
> > please. We can utilize local dmaengine.h and not expose all these and
> > possible misuse by clients
>
> OK, I have thought about that as well.
>
> > Also I would like to see a use :-) before further comments.
>
> You mean the DMA driver and at least one client?
DMA driver to _at_least_ start with. Client even better
> I have the DMA driver in my public facing branch [1], but it is not an easy
> read with it's close to 4k loc.
It doesnt exist :P
> The client is not in my branch and it is actually using an older version of
> the metadata support.
>
> The problem is that I don't know when I will be able to send the driver for
> review as all of this is targeting a brand new SoC (AM654) with completely new
> data movement architecture. There are lots of dependencies still need to be
> upstreamed before I can send something which at least compiles.
>
> I can offer snippets from the client driver, if that is good enough or a link
> to the public tree where it can be accessed, but it is not going to go
> upstream before the DMA driver.
TBH that's not going to help much, lets come back to it when you need
this upstream.
--
~Vinod
Vinod,
On 2018-08-29 19:22, Vinod wrote:
>>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
>>>> + * descriptor
>>>> + * 3. submit the transfer
>>>> + * - DMA_DEV_TO_MEM:
>>>> + * 1. prepare the descriptor (dmaengine_prep_*)
>>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
>>>> + * descriptor
>>>> + * 3. submit the transfer
>>>> + * 4. when the transfer is completed, the metadata should be available in the
>>>> + * attached buffer
>>>
>>> I guess this is good to be moved into Documentation
>>
>> Should I create a new file for metadata? I guess it would make sense as the
>> information is for both clients and engines.
>
> Hmm not sure, lets see how it looks as entries in these files, detailing
> roles of clients and providers
Update both client and provider documentation with tailoring the content
for the audience?
>>> also we dont allow this for memcpy txn?
>>
>> I have not thought about that, but if I think about it it should be along the
>> same lines as MEM_TO_DEV.
>> I'll add the MEM_TO_MEM as well to the documentation.
>
> Okay and lets not implement it then..
I'm not going to implement it, but the documentation could add that if
metadata is used for MEM_TO_MEM then it is the same use case as with
MEM_TO_DEV.
>
>>
>>>> + *
>>>> + * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
>>>> + * driver. The client driver can ask for the pointer, maximum size and the
>>>> + * currently used size of the metadata and can directly update or read it.
>>>> + * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
>>>> + * provided as helper functions.
>>>> + *
>>>> + * Client drivers interested to use this mode can follow:
>>>> + * - DMA_MEM_TO_DEV:
Here, add DMA_MEM_TO_MEM
>>>> + * 1. prepare the descriptor (dmaengine_prep_*)
>>>> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
>>>> + * metadata area
>>>> + * 3. update the metadata at the pointer
>>>> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
>>>> + * of data the client has placed into the metadata buffer
>>>> + * 5. submit the transfer
>>>> + * - DMA_DEV_TO_MEM:
>>>> + * 1. prepare the descriptor (dmaengine_prep_*)
>>>> + * 2. submit the transfer
>>>> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
>>>> + * pointer to the engine's metadata are
>>>> + * 4. Read out the metadate from the pointer
>>>> + *
>>>> + * Note: the two mode is not compatible and clients must use one mode for a
>>>> + * descriptor.
>>>> + */
>>>> +enum dma_desc_metadata_mode {
>>>> + DESC_METADATA_CLIENT = (1 << 0),
>>>> + DESC_METADATA_ENGINE = (1 << 1),
>>>
>>> BIT(x)
>>
>> OK, I followed what we have in the header to not mix (1 << x) and BIT(x)
>
> yeah lets update :)
OK.
>>>> +static inline int _desc_check_and_set_metadata_mode(
>>>
>>> why does this need to start with _ ?
>>
>> To scare people to use in client code ;)
>
> Lets not expose to them :D
Sure, if the code moves to dmaengine.c it is granted.
>>> Also I would like to see a use :-) before further comments.
>>
>> You mean the DMA driver and at least one client?
>
> DMA driver to _at_least_ start with. Client even better
Hrm, I can send the DMA driver as RFC (not to merge, will not compile)
but I need to do some excess cover letter and documentation since the
UDMA is 'just' a piece in the data movement architecture and need to
explain couple of things around it.
I will need couple of days for that for sure.
>> I have the DMA driver in my public facing branch [1], but it is not an easy
>> read with it's close to 4k loc.
>
> It doesnt exist :P
In this sense it does not, agree.
>> The client is not in my branch and it is actually using an older version of
>> the metadata support.
>>
>> The problem is that I don't know when I will be able to send the driver for
>> review as all of this is targeting a brand new SoC (AM654) with completely new
>> data movement architecture. There are lots of dependencies still need to be
>> upstreamed before I can send something which at least compiles.
>>
>> I can offer snippets from the client driver, if that is good enough or a link
>> to the public tree where it can be accessed, but it is not going to go
>> upstream before the DMA driver.
>
> TBH that's not going to help much, lets come back to it when you need
> this upstream.
One of the reason I have sent the metadata support early is because
Radhey was looking for similar thing for xilinx_dma and I already had
the generic implementation of it which suits his case.
I was planning to send the metadata support along with the DMA driver
(and other core changes, new features).
If not for me, then for Radhey's stake can the metadata support be
considered as stand alone for now?
I will send v2 as soon as I have it ready and I will send either v3 with
the k3-udma DMA driver (UDMA drivers as not for merge) or standalone
UDMA driver as RFC and for reference.
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
<snip>
> Vinod,
>
> On 2018-08-29 19:22, Vinod wrote:
> >>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to
> the
> >>>> + * descriptor
> >>>> + * 3. submit the transfer
> >>>> + * - DMA_DEV_TO_MEM:
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to
> the
> >>>> + * descriptor
> >>>> + * 3. submit the transfer
> >>>> + * 4. when the transfer is completed, the metadata should be available
> in the
> >>>> + * attached buffer
> >>>
> >>> I guess this is good to be moved into Documentation
> >>
> >> Should I create a new file for metadata? I guess it would make sense as the
> >> information is for both clients and engines.
> >
> > Hmm not sure, lets see how it looks as entries in these files, detailing
> > roles of clients and providers
>
> Update both client and provider documentation with tailoring the content
> for the audience?
>
> >>> also we dont allow this for memcpy txn?
> >>
> >> I have not thought about that, but if I think about it it should be along the
> >> same lines as MEM_TO_DEV.
> >> I'll add the MEM_TO_MEM as well to the documentation.
> >
> > Okay and lets not implement it then..
>
> I'm not going to implement it, but the documentation could add that if
> metadata is used for MEM_TO_MEM then it is the same use case as with
> MEM_TO_DEV.
>
> >
> >>
> >>>> + *
> >>>> + * @DESC_METADATA_ENGINE - the metadata buffer is
> allocated/managed by the DMA
> >>>> + * driver. The client driver can ask for the pointer, maximum size and
> the
> >>>> + * currently used size of the metadata and can directly update or read it.
> >>>> + * dmaengine_desc_get_metadata_ptr() and
> dmaengine_desc_set_metadata_len() is
> >>>> + * provided as helper functions.
> >>>> + *
> >>>> + * Client drivers interested to use this mode can follow:
> >>>> + * - DMA_MEM_TO_DEV:
>
> Here, add DMA_MEM_TO_MEM
>
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the
> engine's
> >>>> + * metadata area
> >>>> + * 3. update the metadata at the pointer
> >>>> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine
> the amount
> >>>> + * of data the client has placed into the metadata buffer
> >>>> + * 5. submit the transfer
> >>>> + * - DMA_DEV_TO_MEM:
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. submit the transfer
> >>>> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr()
> to get the
> >>>> + * pointer to the engine's metadata are
> >>>> + * 4. Read out the metadate from the pointer
> >>>> + *
> >>>> + * Note: the two mode is not compatible and clients must use one mode
> for a
> >>>> + * descriptor.
> >>>> + */
> >>>> +enum dma_desc_metadata_mode {
> >>>> + DESC_METADATA_CLIENT = (1 << 0),
> >>>> + DESC_METADATA_ENGINE = (1 << 1),
> >>>
> >>> BIT(x)
> >>
> >> OK, I followed what we have in the header to not mix (1 << x) and BIT(x)
> >
> > yeah lets update :)
>
> OK.
>
> >>>> +static inline int _desc_check_and_set_metadata_mode(
> >>>
> >>> why does this need to start with _ ?
> >>
> >> To scare people to use in client code ;)
> >
> > Lets not expose to them :D
>
> Sure, if the code moves to dmaengine.c it is granted.
>
> >>> Also I would like to see a use :-) before further comments.
> >>
> >> You mean the DMA driver and at least one client?
> >
> > DMA driver to _at_least_ start with. Client even better
>
> Hrm, I can send the DMA driver as RFC (not to merge, will not compile)
> but I need to do some excess cover letter and documentation since the
> UDMA is 'just' a piece in the data movement architecture and need to
> explain couple of things around it.
>
> I will need couple of days for that for sure.
>
> >> I have the DMA driver in my public facing branch [1], but it is not an easy
> >> read with it's close to 4k loc.
> >
> > It doesnt exist :P
>
> In this sense it does not, agree.
>
> >> The client is not in my branch and it is actually using an older version of
> >> the metadata support.
> >>
> >> The problem is that I don't know when I will be able to send the driver for
> >> review as all of this is targeting a brand new SoC (AM654) with completely
> new
> >> data movement architecture. There are lots of dependencies still need to be
> >> upstreamed before I can send something which at least compiles.
> >>
> >> I can offer snippets from the client driver, if that is good enough or a link
> >> to the public tree where it can be accessed, but it is not going to go
> >> upstream before the DMA driver.
> >
> > TBH that's not going to help much, lets come back to it when you need
> > this upstream.
>
> One of the reason I have sent the metadata support early is because
> Radhey was looking for similar thing for xilinx_dma and I already had
> the generic implementation of it which suits his case.
>
> I was planning to send the metadata support along with the DMA driver
> (and other core changes, new features).
>
> If not for me, then for Radhey's stake can the metadata support be
> considered as stand alone for now?
Thanks, Peter. I was thinking to put the same request. Based on metadata_ops
RFC, I can send v2 of my earlier patchset[1]. Once it is acked, I will next
send client axiethernet driver[2] RFC to networking mailing list.
Let's wait for Vinod's inputs.
[1] https://www.spinics.net/lists/dmaengine/msg15208.html
[2] https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/xilinx/xilinx_axienet_main.c
>
> I will send v2 as soon as I have it ready and I will send either v3 with
> the k3-udma DMA driver (UDMA drivers as not for merge) or standalone
> UDMA driver as RFC and for reference.
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hey Peter,
On 30-08-18, 11:57, Peter Ujfalusi wrote:
> On 2018-08-29 19:22, Vinod wrote:
> >>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> >>>> + * descriptor
> >>>> + * 3. submit the transfer
> >>>> + * - DMA_DEV_TO_MEM:
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. use dmaengine_desc_attach_metadata() to attach the buffer to the
> >>>> + * descriptor
> >>>> + * 3. submit the transfer
> >>>> + * 4. when the transfer is completed, the metadata should be available in the
> >>>> + * attached buffer
> >>>
> >>> I guess this is good to be moved into Documentation
> >>
> >> Should I create a new file for metadata? I guess it would make sense as the
> >> information is for both clients and engines.
> >
> > Hmm not sure, lets see how it looks as entries in these files, detailing
> > roles of clients and providers
>
> Update both client and provider documentation with tailoring the content
> for the audience?
Right :)
> >>> also we dont allow this for memcpy txn?
> >>
> >> I have not thought about that, but if I think about it it should be along the
> >> same lines as MEM_TO_DEV.
> >> I'll add the MEM_TO_MEM as well to the documentation.
> >
> > Okay and lets not implement it then..
>
> I'm not going to implement it, but the documentation could add that if
> metadata is used for MEM_TO_MEM then it is the same use case as with
> MEM_TO_DEV.
Yes that would be sensible
> >>>> + * @DESC_METADATA_ENGINE - the metadata buffer is allocated/managed by the DMA
> >>>> + * driver. The client driver can ask for the pointer, maximum size and the
> >>>> + * currently used size of the metadata and can directly update or read it.
> >>>> + * dmaengine_desc_get_metadata_ptr() and dmaengine_desc_set_metadata_len() is
> >>>> + * provided as helper functions.
> >>>> + *
> >>>> + * Client drivers interested to use this mode can follow:
> >>>> + * - DMA_MEM_TO_DEV:
>
> Here, add DMA_MEM_TO_MEM
>
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. use dmaengine_desc_get_metadata_ptr() to get the pointer to the engine's
> >>>> + * metadata area
> >>>> + * 3. update the metadata at the pointer
> >>>> + * 4. use dmaengine_desc_set_metadata_len() to tell the DMA engine the amount
> >>>> + * of data the client has placed into the metadata buffer
> >>>> + * 5. submit the transfer
> >>>> + * - DMA_DEV_TO_MEM:
> >>>> + * 1. prepare the descriptor (dmaengine_prep_*)
> >>>> + * 2. submit the transfer
> >>>> + * 3. on transfer completion, use dmaengine_desc_get_metadata_ptr() to get the
> >>>> + * pointer to the engine's metadata are
> >>>> + * 4. Read out the metadate from the pointer
> >>>> + *
> >>>> + * Note: the two mode is not compatible and clients must use one mode for a
> >>>> + * descriptor.
> >>>> + */
> >>>> +enum dma_desc_metadata_mode {
> >>>> + DESC_METADATA_CLIENT = (1 << 0),
> >>>> + DESC_METADATA_ENGINE = (1 << 1),
> >>>
> >>> BIT(x)
> >>
> >> OK, I followed what we have in the header to not mix (1 << x) and BIT(x)
> >
> > yeah lets update :)
>
> OK.
>
> >>>> +static inline int _desc_check_and_set_metadata_mode(
> >>>
> >>> why does this need to start with _ ?
> >>
> >> To scare people to use in client code ;)
> >
> > Lets not expose to them :D
>
> Sure, if the code moves to dmaengine.c it is granted.
>
> >>> Also I would like to see a use :-) before further comments.
> >>
> >> You mean the DMA driver and at least one client?
> >
> > DMA driver to _at_least_ start with. Client even better
>
> Hrm, I can send the DMA driver as RFC (not to merge, will not compile)
> but I need to do some excess cover letter and documentation since the
> UDMA is 'just' a piece in the data movement architecture and need to
> explain couple of things around it.
>
> I will need couple of days for that for sure.
If we are not merging, we can review it. I am not super excited about
merging w/o user of an API
> >> I have the DMA driver in my public facing branch [1], but it is not an easy
> >> read with it's close to 4k loc.
> >
> > It doesnt exist :P
>
> In this sense it does not, agree.
>
> >> The client is not in my branch and it is actually using an older version of
> >> the metadata support.
> >>
> >> The problem is that I don't know when I will be able to send the driver for
> >> review as all of this is targeting a brand new SoC (AM654) with completely new
> >> data movement architecture. There are lots of dependencies still need to be
> >> upstreamed before I can send something which at least compiles.
> >>
> >> I can offer snippets from the client driver, if that is good enough or a link
> >> to the public tree where it can be accessed, but it is not going to go
> >> upstream before the DMA driver.
> >
> > TBH that's not going to help much, lets come back to it when you need
> > this upstream.
>
> One of the reason I have sent the metadata support early is because
> Radhey was looking for similar thing for xilinx_dma and I already had
> the generic implementation of it which suits his case.
>
> I was planning to send the metadata support along with the DMA driver
> (and other core changes, new features).
>
> If not for me, then for Radhey's stake can the metadata support be
> considered as stand alone for now?
>
> I will send v2 as soon as I have it ready and I will send either v3 with
> the k3-udma DMA driver (UDMA drivers as not for merge) or standalone
> UDMA driver as RFC and for reference.
Okay, Radhey can take your patch and submit with his changes for merge.
--
~Vinod