Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Mon, Jan 28, 2013 at 10:06:03AM +, Vinod Koul wrote: > On Mon, Jan 21, 2013 at 01:19:23PM -0500, Matt Porter wrote: > > > b) Sg segment length and numbers: Well these are capabilities, so it tells > > > you what is the maximum I can do. IMO it doesn't make sense to tie it > > > down to > > > burst, width etc. For that configuration you are checking maximum. What > > > this > > > needs to return is what is the maximum length it supports and maximum > > > number of > > > sg list the h/w can use. Also if you return your burst and width > > > capablity, then > > > any client can easily find out what is the length byte value it can hold. > > > > Ok, so I could report the max burst size to the client, but on EDMA it's > > going to be SZ_64K-1, as that's the hw limit. Max addr width would also > > be the same or capped at the maximum enum the current API support of 8 > > bytes. > Yes IMO you should report the h/w limit and let client deduce what can be done > with that h/w limit given the other parameters (burst, length etc...) Hrm, in a driver that runs on two different DMACs, we don't know which one we are running on in order to determine if the information returned is relevant to do a calculation. If omap_hsmmc.c is running on SDMA the absolute max_seg_len may be fixed. On EDMA, that max values can be returned but is invalid for purposes of computing the actual length of sg segments that can be absorbed by the driver. The client driver does know the burst and length, and could compute this in an EDMA specific way, but we're talking now about in the client driver is: if (i_am_edma) max_seg_len = (SZ_64K-1) * max_burst * addr_width; else /* sdma */ max_seg_len = MY_FIXED_VALUE_FROM_CHANNEL_CAPS; There's no way to deduce what to do in the client driver without having the knowledge of being on an EDMA DMAC or not. The only thing to do with fixed h/w caps is to report back a safe value. That would be SZ_64K-1 but would be very inefficient when the slave config is set for a large FIFO and 32-bit address width and the actual size that can be transferred if much larger. > > > > This information is not useful to the client in my case. Only the client > > knows its FIFO size, and thus the actual burst size it needs to request > > as that is a value specific to the IP the client driver controls. So it > > knows actual burst size and the width of that fifo transfer, because we > > already support telling the dmaengine driver this info in the current > > API. > Your current API way is not very generic. We need to make sure it useful on > other systems too. That is why it would make sense to query the DMA H/W > capablities and then use it with above properties to get various parameters > used > for initiating a transfer, that way you dont need to set slave parameters Ok, you want something then that takes max_burst and addr_width as arguments and returns max_seg_len. Easy enough. That's the minimum information the driver needs to report the value and will avoid having to set the slave config first. -Matt ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Mon, Jan 21, 2013 at 01:19:23PM -0500, Matt Porter wrote: > > b) Sg segment length and numbers: Well these are capabilities, so it tells > > you what is the maximum I can do. IMO it doesn't make sense to tie it down > > to > > burst, width etc. For that configuration you are checking maximum. What this > > needs to return is what is the maximum length it supports and maximum > > number of > > sg list the h/w can use. Also if you return your burst and width capablity, > > then > > any client can easily find out what is the length byte value it can hold. > > Ok, so I could report the max burst size to the client, but on EDMA it's > going to be SZ_64K-1, as that's the hw limit. Max addr width would also > be the same or capped at the maximum enum the current API support of 8 > bytes. Yes IMO you should report the h/w limit and let client deduce what can be done with that h/w limit given the other parameters (burst, length etc...) > > This information is not useful to the client in my case. Only the client > knows its FIFO size, and thus the actual burst size it needs to request > as that is a value specific to the IP the client driver controls. So it > knows actual burst size and the width of that fifo transfer, because we > already support telling the dmaengine driver this info in the current > API. Your current API way is not very generic. We need to make sure it useful on other systems too. That is why it would make sense to query the DMA H/W capablities and then use it with above properties to get various parameters used for initiating a transfer, that way you dont need to set slave parameters -- ~Vinod ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Mon, Jan 21, 2013 at 03:15:23AM +, Vinod Koul wrote: > On Sun, Jan 20, 2013 at 11:37:35AM -0500, Matt Porter wrote: > > On Sun, Jan 20, 2013 at 12:37:34PM +, Vinod Koul wrote: > > > On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote: > > > > The call is implemented as follows: > > > > > > > > struct dmaengine_chan_caps > > > > *dma_get_channel_caps(struct dma_chan *chan, > > > > enum dma_transfer_direction dir); > > > > > > > > The dma transfer direction parameter may appear a bit out of place > > > > but it is necessary since the direction field in struct > > > > dma_slave_config was deprecated. In some cases, EDMA for one, it > > > > is necessary for the dmaengine driver to have the burst and address > > > > width slave configuration parameters available in order to compute > > > > the maximum segment size that can be handle. Due to this requirement, > > > > the calling order of this api is as follows: > > > Well you are passing direction as argument so even in EDMA it doesn't > > > seem to > > > help you as you seem to need burst and width!. So why do you even need the > > > direction to compute the capablities > > > > Yes, I need burst and width, but they are dependent on direction (dst vs > > src, as stored in the slave channel config). Ok, so I think I know where > > this is leading...the problem is probably that I made an implicit > > dependency on burst and width here. The expectation in this > And also due to wrong documentation. This is what you have put up the flow as: > Due to this requirement, > the calling order of this api is as follows: > > 1. Allocate a DMA slave channel > 1a. [Optionally] Get channel capabilities > 2. Set slave and controller specific parameters > 3. Get a descriptor for transaction > 4. Submit the transaction > 5. Issue pending requests and wait for callback notification > > Now when we query capablities, slave parameters _are_not_set_. > So seems like you have thought something and written something else! Agreed. My documentation is incorrect. My bad, it should have been ordered 1, 2, 1a, ... as you've noted. > Which brings me to the point on what are we trying to query: > a) API capability, dont need slave parameters for that agreed > b) Sg segment length and numbers: Well these are capabilities, so it tells > you what is the maximum I can do. IMO it doesn't make sense to tie it down to > burst, width etc. For that configuration you are checking maximum. What this > needs to return is what is the maximum length it supports and maximum number > of > sg list the h/w can use. Also if you return your burst and width capablity, > then > any client can easily find out what is the length byte value it can hold. Ok, so I could report the max burst size to the client, but on EDMA it's going to be SZ_64K-1, as that's the hw limit. Max addr width would also be the same or capped at the maximum enum the current API support of 8 bytes. This information is not useful to the client in my case. Only the client knows its FIFO size, and thus the actual burst size it needs to request as that is a value specific to the IP the client driver controls. So it knows actual burst size and the width of that fifo transfer, because we already support telling the dmaengine driver this info in the current API. But, the client driver has no idea what DMAC it's using under the API. So, whereas if the omap_hsmmc driver is running on omap-dma, it can handle any SG segment size, if the driver happens to be running on edma, it needs to know the actual allowed max segment size for the slave channel parameters that it has set. The theoretical max segment size on EDMA is far larger than the actual maximum for a configured slave channel. This is why (yes, I screwed up and the documentation had the ordering wrong) I had the intention of dma_get_channel_caps() only being valid *after* dma_slave_config() was called. This is, in fact, the only point at which the edma driver has enough information to be able to report a valid max number of segments back to a client driver. Consider that on another channel, with burst size configured by a client driver to a high value (e.g. twice as big of a FIFO), now the edma driver has to report a small max segment size that can be used. The client driver has no idea it's on an edma controller so it needs some help from the dmaengine API to choose the optimal constraints when it builds SG lists. Too large and the EDMA driver has to reject it, too small and we lose performance and also run into the constraint edma has on the total number of segments we can handle in one sg list. > If you feel this computaion if client specific, though looking at doesnt make > me > think so, you can add a callback for this computaion given the parameters. It's client-specific in the sense that the client peripheral is what provides the address width and burst size constraint (based on the FIFO integrated with that
Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Sun, Jan 20, 2013 at 11:37:35AM -0500, Matt Porter wrote: > On Sun, Jan 20, 2013 at 12:37:34PM +, Vinod Koul wrote: > > On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote: > > > The call is implemented as follows: > > > > > > struct dmaengine_chan_caps > > > *dma_get_channel_caps(struct dma_chan *chan, > > > enum dma_transfer_direction dir); > > > > > > The dma transfer direction parameter may appear a bit out of place > > > but it is necessary since the direction field in struct > > > dma_slave_config was deprecated. In some cases, EDMA for one, it > > > is necessary for the dmaengine driver to have the burst and address > > > width slave configuration parameters available in order to compute > > > the maximum segment size that can be handle. Due to this requirement, > > > the calling order of this api is as follows: > > Well you are passing direction as argument so even in EDMA it doesn't seem > > to > > help you as you seem to need burst and width!. So why do you even need the > > direction to compute the capablities > > Yes, I need burst and width, but they are dependent on direction (dst vs > src, as stored in the slave channel config). Ok, so I think I know where > this is leading...the problem is probably that I made an implicit > dependency on burst and width here. The expectation in this And also due to wrong documentation. This is what you have put up the flow as: Due to this requirement, the calling order of this api is as follows: 1. Allocate a DMA slave channel 1a. [Optionally] Get channel capabilities 2. Set slave and controller specific parameters 3. Get a descriptor for transaction 4. Submit the transaction 5. Issue pending requests and wait for callback notification Now when we query capablities, slave parameters _are_not_set_. So seems like you have thought something and written something else! Which brings me to the point on what are we trying to query: a) API capability, dont need slave parameters for that b) Sg segment length and numbers: Well these are capabilities, so it tells you what is the maximum I can do. IMO it doesn't make sense to tie it down to burst, width etc. For that configuration you are checking maximum. What this needs to return is what is the maximum length it supports and maximum number of sg list the h/w can use. Also if you return your burst and width capablity, then any client can easily find out what is the length byte value it can hold. If you feel this computaion if client specific, though looking at doesnt make me think so, you can add a callback for this computaion given the parameters. -- ~Vinod ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Sun, Jan 20, 2013 at 12:37:34PM +, Vinod Koul wrote: > On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote: > > The call is implemented as follows: > > > > struct dmaengine_chan_caps > > *dma_get_channel_caps(struct dma_chan *chan, > > enum dma_transfer_direction dir); > > > > The dma transfer direction parameter may appear a bit out of place > > but it is necessary since the direction field in struct > > dma_slave_config was deprecated. In some cases, EDMA for one, it > > is necessary for the dmaengine driver to have the burst and address > > width slave configuration parameters available in order to compute > > the maximum segment size that can be handle. Due to this requirement, > > the calling order of this api is as follows: > Well you are passing direction as argument so even in EDMA it doesn't seem to > help you as you seem to need burst and width!. So why do you even need the > direction to compute the capablities Yes, I need burst and width, but they are dependent on direction (dst vs src, as stored in the slave channel config). Ok, so I think I know where this is leading...the problem is probably that I made an implicit dependency on burst and width here. The expectation in this implementation is that dmaengine_slave_config() has already been called and as a result, the dmaengine driver has either src_* or dst_* attributes populated depending on the direction of the channel. Given that, the call to dma_get_channel_caps() needs to provide the direction in order for the driver to know which of src_*/dst_* attributes are valid in order to do the max segment size calculation. An alternative, since the slave driver is the one that provided the info in the first place is: struct dmaengine_chan_caps *dma_get_channel_caps(struct dma_chan *chan, enum dma_slave_buswidth addr_width, u32 maxburst); where the attributes required by the edma driver to find the max segment size are now explicitly provided. This approach also removes the ordering requirement of calling dmaengine_slave_config() first. Is this what you had in mind? -Matt ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
Re: [PATCH v2 0/3] dmaengine: add per channel capabilities api
On Thu, Jan 10, 2013 at 02:07:03PM -0500, Matt Porter wrote: > The call is implemented as follows: > > struct dmaengine_chan_caps > *dma_get_channel_caps(struct dma_chan *chan, > enum dma_transfer_direction dir); > > The dma transfer direction parameter may appear a bit out of place > but it is necessary since the direction field in struct > dma_slave_config was deprecated. In some cases, EDMA for one, it > is necessary for the dmaengine driver to have the burst and address > width slave configuration parameters available in order to compute > the maximum segment size that can be handle. Due to this requirement, > the calling order of this api is as follows: Well you are passing direction as argument so even in EDMA it doesn't seem to help you as you seem to need burst and width!. So why do you even need the direction to compute the capablities -- ~Vinod ___ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
