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
