Message-ID: <MWHPR0201MB3628F1B65ECC5CD366B89370C7080@MWHPR0201MB3628.namprd02.prod.outlook.com>
Date:   Thu, 30 Aug 2018 09:32:15 +0000
From:   Radhey Shyam Pandey <radheys@...inx.com>
To:     Peter Ujfalusi <peter.ujfalusi@...com>, Vinod <vkoul@...nel.org>
CC:     "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lars@...afoo.de" <lars@...afoo.de>
Subject: RE: [PATCH] dmaengine: Add metadata_ops for dma_async_tx_descriptor

<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

Powered by blists - more mailing lists