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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
