| Message ID | 20180601102429.16429-1-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
> -----Original Message----- > From: Peter Ujfalusi [mailto:peter.ujfalusi@ti.com] > Sent: Friday, June 1, 2018 3:54 PM > To: Radhey Shyam Pandey <radheys@xilinx.com>; vinod.koul@intel.com > Cc: lars@metafoo.de; michal.simek@xilinx.com; linux- > kernel@vger.kernel.org; dmaengine@vger.kernel.org; > dan.j.williams@intel.com; Appana Durga Kedareswara Rao > <appanad@xilinx.com>; linux-arm-kernel@lists.infradead.org > Subject: [RFC] dmaengine: Add metadat_ops for dma_async_tx_descriptor > > 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. > > Wrappers are also added for the metadata_ops: > dmaengine_desc_attach_metadata() > dmaengine_desc_get_metadata_ptr() > dmaengine_desc_set_metadata_len() > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi, > > since attachments are bouncing back, I send the patch separately > > Regards, > Peter > > include/linux/dmaengine.h | 50 > +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 51fbb861e84b..ac42ace36aa3 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -491,6 +491,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--- > @@ -520,6 +532,7 @@ struct dma_async_tx_descriptor { > dma_async_tx_callback_result callback_result; > void *callback_param; > struct dmaengine_unmap_data *unmap; > + 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; > @@ -932,6 +945,43 @@ static inline struct dma_async_tx_descriptor > *dmaengine_prep_dma_memcpy( > len, flags); > } > > +static inline int dmaengine_desc_attach_metadata( > + struct dma_async_tx_descriptor *desc, void *data, size_t > len) > +{ > + if (!desc) > + return 0; > + > + 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) > +{ > + if (!desc) > + return NULL; > + > + 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) > +{ > + if (!desc) > + return 0; > + > + if (!desc->metadata_ops || !desc->metadata_ops->set_len) > + return -ENOTSUPP; > + > + return desc->metadata_ops->set_len(desc, payload_len); > +} > + Thanks for the RFC patchset. It looks fine to me. We also need to update documentation. Let's wait for Vinod/Lars feedback. > /** > * 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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Peter, Sorry for late response on this.. On 01-06-18, 13:24, Peter Ujfalusi wrote: > 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. > > Wrappers are also added for the metadata_ops: > dmaengine_desc_attach_metadata() > dmaengine_desc_get_metadata_ptr() > dmaengine_desc_set_metadata_len() > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > Hi, > > since attachments are bouncing back, I send the patch separately > > Regards, > Peter > > include/linux/dmaengine.h | 50 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 51fbb861e84b..ac42ace36aa3 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -491,6 +491,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); How does one detach? When should the client free up the memory, IOW when does dma driver drop ref to data. > + > + void *(*get_ptr)(struct dma_async_tx_descriptor *desc, > + size_t *payload_len, size_t *max_len); so what is this supposed to do..? > + int (*set_len)(struct dma_async_tx_descriptor *desc, > + size_t payload_len); attach already has length, so how does this help? > +}; > + > /** > * struct dma_async_tx_descriptor - async transaction descriptor > * ---dma generic offload fields--- > @@ -520,6 +532,7 @@ struct dma_async_tx_descriptor { > dma_async_tx_callback_result callback_result; > void *callback_param; > struct dmaengine_unmap_data *unmap; > + 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; > @@ -932,6 +945,43 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy( > len, flags); > } > > +static inline int dmaengine_desc_attach_metadata( > + struct dma_async_tx_descriptor *desc, void *data, size_t len) > +{ > + if (!desc) > + return 0; > + > + 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) > +{ > + if (!desc) > + return NULL; > + > + 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) > +{ > + if (!desc) > + return 0; > + > + 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 > > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Vinod, On 2018-07-10 08:52, Vinod wrote: > > Hey Peter, > > Sorry for late response on this.. No problem, I was away myself also... > On 01-06-18, 13:24, Peter Ujfalusi wrote: >> 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. >> >> Wrappers are also added for the metadata_ops: >> dmaengine_desc_attach_metadata() >> dmaengine_desc_get_metadata_ptr() >> dmaengine_desc_set_metadata_len() >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> --- >> Hi, >> >> since attachments are bouncing back, I send the patch separately >> >> Regards, >> Peter >> >> include/linux/dmaengine.h | 50 +++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h >> index 51fbb861e84b..ac42ace36aa3 100644 >> --- a/include/linux/dmaengine.h >> +++ b/include/linux/dmaengine.h >> @@ -491,6 +491,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); > > How does one detach? I have not thought about detach, but clients can just attach NULL I guess. > When should the client free up the memory, IOW when > does dma driver drop ref to data. The metadata is for the descriptor so the DMA driver might want to access to it while the descriptor is valid. Typically clients can free up their metadata storage after the dma completion callback. On DEV_TO_MEM the metadata is going to be placed in the provided buffer when the transfer is completed. > >> + >> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc, >> + size_t *payload_len, size_t *max_len); > > so what is this supposed to do..? My issue with the attach in general is that it will need additional memcpy to move the metadata from/to the client buffer to it's place. With get_ptr the client can get the pointer to the actual place where the metadata resides and modify/read it in place w/o memcpy. I know, I know... We need to trust the clients, but with high throughput peripherals the memcpy is taxing. > >> + int (*set_len)(struct dma_async_tx_descriptor *desc, >> + size_t payload_len); > > attach already has length, so how does this help? So, DMA drivers can implement either or both: 1. attach() 2. get_ptr() / set_len() Clients must not mix the two way of handling the metadata. The set_len() is intended to tell the DMA driver the client provided metadata size (in MEM_TO_DEV case mostly). MEM_TO_DEV flow on client side: get_ptr() fill in the metadata to the pointer (not exceeding max_len) set_len() to tell the DMA driver the amount of valid bytes written DEV_TO_MEM flow on client side: In the completion callback, get_ptr() the metadata is payload_len bytes and can be accessed in the return pointer. BTW: The driver which is going to need this is now accessible in public: https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti or in my wip tree: https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti prefixed with k3-* > >> +}; >> + >> /** >> * struct dma_async_tx_descriptor - async transaction descriptor >> * ---dma generic offload fields--- >> @@ -520,6 +532,7 @@ struct dma_async_tx_descriptor { >> dma_async_tx_callback_result callback_result; >> void *callback_param; >> struct dmaengine_unmap_data *unmap; >> + 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; >> @@ -932,6 +945,43 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy( >> len, flags); >> } >> >> +static inline int dmaengine_desc_attach_metadata( >> + struct dma_async_tx_descriptor *desc, void *data, size_t len) >> +{ >> + if (!desc) >> + return 0; >> + >> + 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) >> +{ >> + if (!desc) >> + return NULL; >> + >> + 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) >> +{ >> + if (!desc) >> + return 0; >> + >> + 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 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe dmaengine" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Peter, On 18-07-18, 13:06, Peter Ujfalusi wrote: > >> +struct dma_async_tx_descriptor; > >> + > >> +struct dma_descriptor_metadata_ops { > >> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data, > >> + size_t len); > > > > How does one detach? > > I have not thought about detach, but clients can just attach NULL I guess. So what are the implication of attach and detach here, should the data be deref by dmaengine driver and drop the ref. Should anyone do refcounting? > > > When should the client free up the memory, IOW when > > does dma driver drop ref to data. > > The metadata is for the descriptor so the DMA driver might want to > access to it while the descriptor is valid. > > Typically clients can free up their metadata storage after the dma > completion callback. On DEV_TO_MEM the metadata is going to be placed in > the provided buffer when the transfer is completed. That sounds okay to me > >> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc, > >> + size_t *payload_len, size_t *max_len); > > > > so what is this supposed to do..? > > My issue with the attach in general is that it will need additional > memcpy to move the metadata from/to the client buffer to it's place. > > With get_ptr the client can get the pointer to the actual place where > the metadata resides and modify/read it in place w/o memcpy. > > I know, I know... We need to trust the clients, but with high throughput > peripherals the memcpy is taxing. Okay I am not sure I have understood fully, so with attach you set a pointer (containing metdata?) so why do you need additional one.. > > > > >> + int (*set_len)(struct dma_async_tx_descriptor *desc, > >> + size_t payload_len); > > > > attach already has length, so how does this help? > > So, DMA drivers can implement either or both: > 1. attach() > 2. get_ptr() / set_len() Ah okay, what are the reasons for providing two methods and not a single one > > Clients must not mix the two way of handling the metadata. > The set_len() is intended to tell the DMA driver the client provided > metadata size (in MEM_TO_DEV case mostly). > > MEM_TO_DEV flow on client side: > get_ptr() > fill in the metadata to the pointer (not exceeding max_len) > set_len() to tell the DMA driver the amount of valid bytes written > > DEV_TO_MEM flow on client side: > In the completion callback, get_ptr() > the metadata is payload_len bytes and can be accessed in the return pointer. I would think to unify this.. > BTW: The driver which is going to need this is now accessible in public: > https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti > > or in my wip tree: > https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti > > prefixed with k3-* >
On 2018-07-19 12:22, Vinod wrote: > Hi Peter, > > On 18-07-18, 13:06, Peter Ujfalusi wrote: > >>>> +struct dma_async_tx_descriptor; >>>> + >>>> +struct dma_descriptor_metadata_ops { >>>> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data, >>>> + size_t len); >>> >>> How does one detach? >> >> I have not thought about detach, but clients can just attach NULL I guess. > > So what are the implication of attach and detach here, should the data > be deref by dmaengine driver and drop the ref. It largely depends on the DMA driver, but I think we must have clear definition on how clients (and thus DMA drivers) must handle the metadata. I think the simpler rule would be that clients _must_ attach the metadata buffer after _prepare() and before issue_pending() and they must make sure that the buffer is valid (not freed up) before the completion callback is called for the given descriptor. About the detach: If clients detaches the metadata buffer then on completion it is not going to receive back any metadata and I think the DMA drivers should clean and disable the metadata sending as well if the detach happens before issue_pending(). > Should anyone do refcounting? Need to think about that. >> >>> When should the client free up the memory, IOW when >>> does dma driver drop ref to data. >> >> The metadata is for the descriptor so the DMA driver might want to >> access to it while the descriptor is valid. >> >> Typically clients can free up their metadata storage after the dma >> completion callback. On DEV_TO_MEM the metadata is going to be placed in >> the provided buffer when the transfer is completed. > > That sounds okay to me > >>>> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc, >>>> + size_t *payload_len, size_t *max_len); >>> >>> so what is this supposed to do..? >> >> My issue with the attach in general is that it will need additional >> memcpy to move the metadata from/to the client buffer to it's place. >> >> With get_ptr the client can get the pointer to the actual place where >> the metadata resides and modify/read it in place w/o memcpy. >> >> I know, I know... We need to trust the clients, but with high throughput >> peripherals the memcpy is taxing. > > Okay I am not sure I have understood fully, so with attach you set > a pointer (containing metdata?) so why do you need additional one.. > >> >>> >>>> + int (*set_len)(struct dma_async_tx_descriptor *desc, >>>> + size_t payload_len); >>> >>> attach already has length, so how does this help? >> >> So, DMA drivers can implement either or both: >> 1. attach() >> 2. get_ptr() / set_len() > > Ah okay, what are the reasons for providing two methods and not a single > one For the HW I have it would be more efficient to grab pointer and do in-place modification to metadata section (the part of the CPPI5 descriptor which is owned by the client driver). Other vendors might have the metadata scattered, or in different way which does not fit with the ptr mode for security or sanity point of view - I don't want to give the whole descriptor to the client. I don't trust ;) >> >> Clients must not mix the two way of handling the metadata. >> The set_len() is intended to tell the DMA driver the client provided >> metadata size (in MEM_TO_DEV case mostly). >> >> MEM_TO_DEV flow on client side: >> get_ptr() >> fill in the metadata to the pointer (not exceeding max_len) >> set_len() to tell the DMA driver the amount of valid bytes written >> >> DEV_TO_MEM flow on client side: >> In the completion callback, get_ptr() >> the metadata is payload_len bytes and can be accessed in the return pointer. > > I would think to unify this.. I have tried it, but the attach mode and the pointer mode is hard to handle with a generic API. I will try to find a way to unify things in a sane way. I have moved the metadata_ops to dma_async_tx_descriptor to emphasize that it is per descriptor setting: https://github.com/omap-audio/linux-audio/commit/02e095d1320a4bb3ae281ddb208ce82ead746f00#diff-92c0a79f414dc3be9dfc67a969c0dd71 >> BTW: The driver which is going to need this is now accessible in public: >> https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti >> >> or in my wip tree: >> https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti >> >> prefixed with k3-* >> - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20-07-18, 16:42, Peter Ujfalusi wrote: > > > On 2018-07-19 12:22, Vinod wrote: > > Hi Peter, > > > > On 18-07-18, 13:06, Peter Ujfalusi wrote: > > > >>>> +struct dma_async_tx_descriptor; > >>>> + > >>>> +struct dma_descriptor_metadata_ops { > >>>> + int (*attach)(struct dma_async_tx_descriptor *desc, void *data, > >>>> + size_t len); > >>> > >>> How does one detach? > >> > >> I have not thought about detach, but clients can just attach NULL I guess. > > > > So what are the implication of attach and detach here, should the data > > be deref by dmaengine driver and drop the ref. > > It largely depends on the DMA driver, but I think we must have clear > definition on how clients (and thus DMA drivers) must handle the metadata. Correct, defining these will help out get clarity and avoid abuse. > I think the simpler rule would be that clients _must_ attach the > metadata buffer after _prepare() and before issue_pending() and they > must make sure that the buffer is valid (not freed up) before the > completion callback is called for the given descriptor. > > About the detach: If clients detaches the metadata buffer then on > completion it is not going to receive back any metadata and I think the > DMA drivers should clean and disable the metadata sending as well if the > detach happens before issue_pending(). > > > Should anyone do refcounting? > > Need to think about that. > > >> > >>> When should the client free up the memory, IOW when > >>> does dma driver drop ref to data. > >> > >> The metadata is for the descriptor so the DMA driver might want to > >> access to it while the descriptor is valid. > >> > >> Typically clients can free up their metadata storage after the dma > >> completion callback. On DEV_TO_MEM the metadata is going to be placed in > >> the provided buffer when the transfer is completed. > > > > That sounds okay to me > > > >>>> + void *(*get_ptr)(struct dma_async_tx_descriptor *desc, > >>>> + size_t *payload_len, size_t *max_len); > >>> > >>> so what is this supposed to do..? > >> > >> My issue with the attach in general is that it will need additional > >> memcpy to move the metadata from/to the client buffer to it's place. > >> > >> With get_ptr the client can get the pointer to the actual place where > >> the metadata resides and modify/read it in place w/o memcpy. > >> > >> I know, I know... We need to trust the clients, but with high throughput > >> peripherals the memcpy is taxing. > > > > Okay I am not sure I have understood fully, so with attach you set > > a pointer (containing metdata?) so why do you need additional one.. > > > >> > >>> > >>>> + int (*set_len)(struct dma_async_tx_descriptor *desc, > >>>> + size_t payload_len); > >>> > >>> attach already has length, so how does this help? > >> > >> So, DMA drivers can implement either or both: > >> 1. attach() > >> 2. get_ptr() / set_len() > > > > Ah okay, what are the reasons for providing two methods and not a single > > one > > For the HW I have it would be more efficient to grab pointer and do > in-place modification to metadata section (the part of the CPPI5 > descriptor which is owned by the client driver). > > Other vendors might have the metadata scattered, or in different way > which does not fit with the ptr mode for security or sanity point of > view - I don't want to give the whole descriptor to the client. I don't > trust ;) > > >> > >> Clients must not mix the two way of handling the metadata. > >> The set_len() is intended to tell the DMA driver the client provided > >> metadata size (in MEM_TO_DEV case mostly). > >> > >> MEM_TO_DEV flow on client side: > >> get_ptr() > >> fill in the metadata to the pointer (not exceeding max_len) > >> set_len() to tell the DMA driver the amount of valid bytes written > >> > >> DEV_TO_MEM flow on client side: > >> In the completion callback, get_ptr() > >> the metadata is payload_len bytes and can be accessed in the return pointer. > > > > I would think to unify this.. > > I have tried it, but the attach mode and the pointer mode is hard to > handle with a generic API. > I will try to find a way to unify things in a sane way. Hmmm, looking from the description they will be for different methods, so lets make them orthogonal and not allow driver to register both. > > I have moved the metadata_ops to dma_async_tx_descriptor to emphasize > that it is per descriptor setting: > https://github.com/omap-audio/linux-audio/commit/02e095d1320a4bb3ae281ddb208ce82ead746f00#diff-92c0a79f414dc3be9dfc67a969c0dd71 > > > >> BTW: The driver which is going to need this is now accessible in public: > >> https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti > >> > >> or in my wip tree: > >> https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti > >> > >> prefixed with k3-* > >> > > - Péter > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Vinod, On 2018-07-24 14:14, Vinod wrote: >>>> Clients must not mix the two way of handling the metadata. >>>> The set_len() is intended to tell the DMA driver the client provided >>>> metadata size (in MEM_TO_DEV case mostly). >>>> >>>> MEM_TO_DEV flow on client side: >>>> get_ptr() >>>> fill in the metadata to the pointer (not exceeding max_len) >>>> set_len() to tell the DMA driver the amount of valid bytes written >>>> >>>> DEV_TO_MEM flow on client side: >>>> In the completion callback, get_ptr() >>>> the metadata is payload_len bytes and can be accessed in the return pointer. >>> >>> I would think to unify this.. >> >> I have tried it, but the attach mode and the pointer mode is hard to >> handle with a generic API. >> I will try to find a way to unify things in a sane way. > > Hmmm, looking from the description they will be for different methods, > so lets make them orthogonal and not allow driver to register both. I would allow DMA drivers to register both, but somehow enforce that clients are not mixing the two distinct way of dealing with the metadata. The reason for that is for example the attach mode is the simplest (I implemented it first and I have a client using it), but if the pointer mode is found to be more efficient and feasible for the DMA then the DMA driver can implement that mode and the client can move as well w/o breaking anything. > >> >> I have moved the metadata_ops to dma_async_tx_descriptor to emphasize >> that it is per descriptor setting: >> https://github.com/omap-audio/linux-audio/commit/02e095d1320a4bb3ae281ddb208ce82ead746f00#diff-92c0a79f414dc3be9dfc67a969c0dd71 >> >> >>>> BTW: The driver which is going to need this is now accessible in public: >>>> https://git.ti.com/ti-linux-kernel/ti-linux-kernel/trees/ti-linux-4.14.y/drivers/dma/ti >>>> >>>> or in my wip tree: >>>> https://github.com/omap-audio/linux-audio/tree/peter/ti-linux-4.14.y/wip/drivers/dma/ti >>>> >>>> prefixed with k3-* >>>> >> >> - Péter >> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30-07-18, 12:46, Peter Ujfalusi wrote: > Vinod, > > On 2018-07-24 14:14, Vinod wrote: > >>>> Clients must not mix the two way of handling the metadata. > >>>> The set_len() is intended to tell the DMA driver the client provided > >>>> metadata size (in MEM_TO_DEV case mostly). > >>>> > >>>> MEM_TO_DEV flow on client side: > >>>> get_ptr() > >>>> fill in the metadata to the pointer (not exceeding max_len) > >>>> set_len() to tell the DMA driver the amount of valid bytes written > >>>> > >>>> DEV_TO_MEM flow on client side: > >>>> In the completion callback, get_ptr() > >>>> the metadata is payload_len bytes and can be accessed in the return pointer. > >>> > >>> I would think to unify this.. > >> > >> I have tried it, but the attach mode and the pointer mode is hard to > >> handle with a generic API. > >> I will try to find a way to unify things in a sane way. > > > > Hmmm, looking from the description they will be for different methods, > > so lets make them orthogonal and not allow driver to register both. > > I would allow DMA drivers to register both, but somehow enforce that > clients are not mixing the two distinct way of dealing with the metadata. > > The reason for that is for example the attach mode is the simplest (I > implemented it first and I have a client using it), but if the pointer > mode is found to be more efficient and feasible for the DMA then the DMA > driver can implement that mode and the client can move as well w/o > breaking anything. Sounds reasonable...
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 51fbb861e84b..ac42ace36aa3 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -491,6 +491,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--- @@ -520,6 +532,7 @@ struct dma_async_tx_descriptor { dma_async_tx_callback_result callback_result; void *callback_param; struct dmaengine_unmap_data *unmap; + 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; @@ -932,6 +945,43 @@ static inline struct dma_async_tx_descriptor *dmaengine_prep_dma_memcpy( len, flags); } +static inline int dmaengine_desc_attach_metadata( + struct dma_async_tx_descriptor *desc, void *data, size_t len) +{ + if (!desc) + return 0; + + 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) +{ + if (!desc) + return NULL; + + 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) +{ + if (!desc) + return 0; + + 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
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. Wrappers are also added for the metadata_ops: dmaengine_desc_attach_metadata() dmaengine_desc_get_metadata_ptr() dmaengine_desc_set_metadata_len() Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- Hi, since attachments are bouncing back, I send the patch separately Regards, Peter include/linux/dmaengine.h | 50 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)