On 20-07-18, 16:42, Peter Ujfalusi wrote:

On 2018-07-19 12:22, Vinod wrote:

quoted

Hi Peter,

On 18-07-18, 13:06, Peter Ujfalusi wrote:

quoted

quoted

quoted

+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().

quoted

Should anyone do refcounting?

Need to think about that.

quoted

quoted

quoted

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

quoted

quoted

quoted

+	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..

quoted

quoted

quoted

+	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 ;)

quoted

quoted

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

quoted

quoted

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