| Message ID | 1396357575-30585-6-git-send-email-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/01/2014 08:06 AM, Peter Ujfalusi wrote: > Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high > priority channels, like audio. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> This looks good, though another way to do it would be to leave default to Queue 0. Put audio in Queue 1, and change QUEPRI to make Queue 1 as higher priority. This is fine, Acked-by: Joel Fernandes <joelf@ti.com> Regards, -Joel
On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: > Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high > priority channels, like audio. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> Acked-by: Sekhar Nori <nsekhar@ti.com> > --- > arch/arm/common/edma.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index 86a8b263278f..19520e2519d9 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, > > pdata->queue_priority_mapping = queue_priority_map; > > - pdata->default_queue = 0; > + /* select queue 1 as default */ It will be nice to expand the comment with explanation of why this is being chosen as default (lower priority queue by default for typical bulk data transfer). Thanks, Sekhar
On 04/11/2014 11:17 AM, Sekhar Nori wrote: > On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >> priority channels, like audio. >> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > > Acked-by: Sekhar Nori <nsekhar@ti.com> > >> --- >> arch/arm/common/edma.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >> index 86a8b263278f..19520e2519d9 100644 >> --- a/arch/arm/common/edma.c >> +++ b/arch/arm/common/edma.c >> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >> >> pdata->queue_priority_mapping = queue_priority_map; >> >> - pdata->default_queue = 0; >> + /* select queue 1 as default */ > > It will be nice to expand the comment with explanation of why this is > being chosen as default (lower priority queue by default for typical > bulk data transfer). Yes, extended comment is a good idea. For the next version I think I'm going to change the code around default TC/Queue and the non default queue selection, mostly based on Joel's comment: EVENTQ_1 as default queue. Set the EVENTQ_1 priority to 7 EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 Add new member to struct edma, like high_pri_queue. When we set the queue priorities in edma_probe() we look for the highest priority queue and save the number in high_pri_queue. I will rename the edma_request_non_default_queue() to edma_request_high_pri_queue() and it will assign the channel to the high priority queue. I think this way it is going to be more explicit and IMHO a bit more safer in a sense the we are going to get high priority when we ask for it.
On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: > On 04/11/2014 11:17 AM, Sekhar Nori wrote: >> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >>> priority channels, like audio. >>> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >> >> Acked-by: Sekhar Nori <nsekhar@ti.com> >> >>> --- >>> arch/arm/common/edma.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>> index 86a8b263278f..19520e2519d9 100644 >>> --- a/arch/arm/common/edma.c >>> +++ b/arch/arm/common/edma.c >>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >>> >>> pdata->queue_priority_mapping = queue_priority_map; >>> >>> - pdata->default_queue = 0; >>> + /* select queue 1 as default */ >> >> It will be nice to expand the comment with explanation of why this is >> being chosen as default (lower priority queue by default for typical >> bulk data transfer). > > Yes, extended comment is a good idea. > > For the next version I think I'm going to change the code around default > TC/Queue and the non default queue selection, mostly based on Joel's comment: > > EVENTQ_1 as default queue. > Set the EVENTQ_1 priority to 7 > EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 > > Add new member to struct edma, like high_pri_queue. > When we set the queue priorities in edma_probe() we look for the highest > priority queue and save the number in high_pri_queue. > > I will rename the edma_request_non_default_queue() to > edma_request_high_pri_queue() and it will assign the channel to the high > priority queue. > > I think this way it is going to be more explicit and IMHO a bit more safer in > a sense the we are going to get high priority when we ask for it. Sounds much better. I had posted some ideas about adding support for channel priority in the core code but we can leave that for Vinod and Dan to say if they really see a need for that. Thanks, Sekhar
On 04/11/2014 11:56 AM, Sekhar Nori wrote: > On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: >> On 04/11/2014 11:17 AM, Sekhar Nori wrote: >>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >>>> priority channels, like audio. >>>> >>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> >>> Acked-by: Sekhar Nori <nsekhar@ti.com> >>> >>>> --- >>>> arch/arm/common/edma.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>>> index 86a8b263278f..19520e2519d9 100644 >>>> --- a/arch/arm/common/edma.c >>>> +++ b/arch/arm/common/edma.c >>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >>>> >>>> pdata->queue_priority_mapping = queue_priority_map; >>>> >>>> - pdata->default_queue = 0; >>>> + /* select queue 1 as default */ >>> >>> It will be nice to expand the comment with explanation of why this is >>> being chosen as default (lower priority queue by default for typical >>> bulk data transfer). >> >> Yes, extended comment is a good idea. >> >> For the next version I think I'm going to change the code around default >> TC/Queue and the non default queue selection, mostly based on Joel's comment: >> >> EVENTQ_1 as default queue. >> Set the EVENTQ_1 priority to 7 >> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 >> >> Add new member to struct edma, like high_pri_queue. >> When we set the queue priorities in edma_probe() we look for the highest >> priority queue and save the number in high_pri_queue. >> >> I will rename the edma_request_non_default_queue() to >> edma_request_high_pri_queue() and it will assign the channel to the high >> priority queue. >> >> I think this way it is going to be more explicit and IMHO a bit more safer in >> a sense the we are going to get high priority when we ask for it. > > Sounds much better. I had posted some ideas about adding support for > channel priority in the core code but we can leave that for Vinod and > Dan to say if they really see a need for that. If we do it via the dmaengine core I think it would be better to have a new flag to be passed to dmaengine_prep_dma_*(). We could have for example: DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is possible. We can watch for this flag in the edma driver and act accordingly. ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally since audio should be treated in this way if the DMA IP can do this. Not sure what to do with eDMA's 8 priority level. With that we could have high priority; low priority; low, but not the lowest priority; about in the middle priority; etc. We could have also new callback in the dma_device struct with needed wrappers to set priority level, but where to draw the range? High, Mid and Low? Range of 0 - 10?
On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: > On 04/11/2014 11:56 AM, Sekhar Nori wrote: > > On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: > >> On 04/11/2014 11:17 AM, Sekhar Nori wrote: > >>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: > >>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high > >>>> priority channels, like audio. > >>>> > >>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>> > >>> Acked-by: Sekhar Nori <nsekhar@ti.com> > >>> > >>>> --- > >>>> arch/arm/common/edma.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > >>>> index 86a8b263278f..19520e2519d9 100644 > >>>> --- a/arch/arm/common/edma.c > >>>> +++ b/arch/arm/common/edma.c > >>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, > >>>> > >>>> pdata->queue_priority_mapping = queue_priority_map; > >>>> > >>>> - pdata->default_queue = 0; > >>>> + /* select queue 1 as default */ > >>> > >>> It will be nice to expand the comment with explanation of why this is > >>> being chosen as default (lower priority queue by default for typical > >>> bulk data transfer). > >> > >> Yes, extended comment is a good idea. > >> > >> For the next version I think I'm going to change the code around default > >> TC/Queue and the non default queue selection, mostly based on Joel's comment: > >> > >> EVENTQ_1 as default queue. > >> Set the EVENTQ_1 priority to 7 > >> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 > >> > >> Add new member to struct edma, like high_pri_queue. > >> When we set the queue priorities in edma_probe() we look for the highest > >> priority queue and save the number in high_pri_queue. > >> > >> I will rename the edma_request_non_default_queue() to > >> edma_request_high_pri_queue() and it will assign the channel to the high > >> priority queue. > >> > >> I think this way it is going to be more explicit and IMHO a bit more safer in > >> a sense the we are going to get high priority when we ask for it. > > > > Sounds much better. I had posted some ideas about adding support for > > channel priority in the core code but we can leave that for Vinod and > > Dan to say if they really see a need for that. Is it part of this series? > If we do it via the dmaengine core I think it would be better to have a new > flag to be passed to dmaengine_prep_dma_*(). > We could have for example: > DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is > possible. > We can watch for this flag in the edma driver and act accordingly. > ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally > since audio should be treated in this way if the DMA IP can do this. Will the priority be different for each descriptor or would be based on channel usage. If not then we can add this in dma_slave_config ? I can forsee its usage on slave controllers, so yes its useful :)
On Friday 11 April 2014 03:12 PM, Vinod Koul wrote: > On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: >> On 04/11/2014 11:56 AM, Sekhar Nori wrote: >>> On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: >>>> On 04/11/2014 11:17 AM, Sekhar Nori wrote: >>>>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >>>>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >>>>>> priority channels, like audio. >>>>>> >>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>>> >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com> >>>>> >>>>>> --- >>>>>> arch/arm/common/edma.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>>>>> index 86a8b263278f..19520e2519d9 100644 >>>>>> --- a/arch/arm/common/edma.c >>>>>> +++ b/arch/arm/common/edma.c >>>>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >>>>>> >>>>>> pdata->queue_priority_mapping = queue_priority_map; >>>>>> >>>>>> - pdata->default_queue = 0; >>>>>> + /* select queue 1 as default */ >>>>> >>>>> It will be nice to expand the comment with explanation of why this is >>>>> being chosen as default (lower priority queue by default for typical >>>>> bulk data transfer). >>>> >>>> Yes, extended comment is a good idea. >>>> >>>> For the next version I think I'm going to change the code around default >>>> TC/Queue and the non default queue selection, mostly based on Joel's comment: >>>> >>>> EVENTQ_1 as default queue. >>>> Set the EVENTQ_1 priority to 7 >>>> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 >>>> >>>> Add new member to struct edma, like high_pri_queue. >>>> When we set the queue priorities in edma_probe() we look for the highest >>>> priority queue and save the number in high_pri_queue. >>>> >>>> I will rename the edma_request_non_default_queue() to >>>> edma_request_high_pri_queue() and it will assign the channel to the high >>>> priority queue. >>>> >>>> I think this way it is going to be more explicit and IMHO a bit more safer in >>>> a sense the we are going to get high priority when we ask for it. >>> >>> Sounds much better. I had posted some ideas about adding support for >>> channel priority in the core code but we can leave that for Vinod and >>> Dan to say if they really see a need for that. > Is it part of this series? No, the current series has an EDMA specific way of managing priority. > >> If we do it via the dmaengine core I think it would be better to have a new >> flag to be passed to dmaengine_prep_dma_*(). >> We could have for example: >> DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is >> possible. >> We can watch for this flag in the edma driver and act accordingly. >> ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally >> since audio should be treated in this way if the DMA IP can do this. > Will the priority be different for each descriptor or would be based on channel > usage. If not then we can add this in dma_slave_config ? The priority will be per-channel not per-transaction (at least for the use case we are talking about here). Thanks, Sekhar
On Fri, Apr 11, 2014 at 02:32:28PM +0300, Peter Ujfalusi wrote: > Hi Vinod, > > On 04/11/2014 12:42 PM, Vinod Koul wrote: > > On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: > >> On 04/11/2014 11:56 AM, Sekhar Nori wrote: > >>> On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: > >>>> On 04/11/2014 11:17 AM, Sekhar Nori wrote: > >>>>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: > >>>>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high > >>>>>> priority channels, like audio. > >>>>>> > >>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>>>> > >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com> > >>>>> > >>>>>> --- > >>>>>> arch/arm/common/edma.c | 3 ++- > >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > >>>>>> index 86a8b263278f..19520e2519d9 100644 > >>>>>> --- a/arch/arm/common/edma.c > >>>>>> +++ b/arch/arm/common/edma.c > >>>>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, > >>>>>> > >>>>>> pdata->queue_priority_mapping = queue_priority_map; > >>>>>> > >>>>>> - pdata->default_queue = 0; > >>>>>> + /* select queue 1 as default */ > >>>>> > >>>>> It will be nice to expand the comment with explanation of why this is > >>>>> being chosen as default (lower priority queue by default for typical > >>>>> bulk data transfer). > >>>> > >>>> Yes, extended comment is a good idea. > >>>> > >>>> For the next version I think I'm going to change the code around default > >>>> TC/Queue and the non default queue selection, mostly based on Joel's comment: > >>>> > >>>> EVENTQ_1 as default queue. > >>>> Set the EVENTQ_1 priority to 7 > >>>> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 > >>>> > >>>> Add new member to struct edma, like high_pri_queue. > >>>> When we set the queue priorities in edma_probe() we look for the highest > >>>> priority queue and save the number in high_pri_queue. > >>>> > >>>> I will rename the edma_request_non_default_queue() to > >>>> edma_request_high_pri_queue() and it will assign the channel to the high > >>>> priority queue. > >>>> > >>>> I think this way it is going to be more explicit and IMHO a bit more safer in > >>>> a sense the we are going to get high priority when we ask for it. > >>> > >>> Sounds much better. I had posted some ideas about adding support for > >>> channel priority in the core code but we can leave that for Vinod and > >>> Dan to say if they really see a need for that. > > Is it part of this series? > > No, it is not. The original series tackled the DMA queue selection within the > edma driver stack. It was selecting high priority channel for cyclic (audio) > use only, all other DMA channels were executed on a lower priority queue. > > >> If we do it via the dmaengine core I think it would be better to have a new > >> flag to be passed to dmaengine_prep_dma_*(). > >> We could have for example: > >> DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is > >> possible. > >> We can watch for this flag in the edma driver and act accordingly. > >> ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally > >> since audio should be treated in this way if the DMA IP can do this. > > > > Will the priority be different for each descriptor or would be based on channel > > usage. > > I would say that it is channel based config. I don't see the reason why would > one mix different priorities on a configured channel between descriptors. > > > If not then we can add this in dma_slave_config ? > > So adding to the struct for example: > bool high_priority; In designware controller, we can have channel priorties from 0 to 7 which IIRC 7 being highest. So bool wont work. unsigned int/u8 would be good. How about your controller, is it binary?
Hi Vinod, On 04/11/2014 12:42 PM, Vinod Koul wrote: > On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: >> On 04/11/2014 11:56 AM, Sekhar Nori wrote: >>> On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: >>>> On 04/11/2014 11:17 AM, Sekhar Nori wrote: >>>>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >>>>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >>>>>> priority channels, like audio. >>>>>> >>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>>> >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com> >>>>> >>>>>> --- >>>>>> arch/arm/common/edma.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>>>>> index 86a8b263278f..19520e2519d9 100644 >>>>>> --- a/arch/arm/common/edma.c >>>>>> +++ b/arch/arm/common/edma.c >>>>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >>>>>> >>>>>> pdata->queue_priority_mapping = queue_priority_map; >>>>>> >>>>>> - pdata->default_queue = 0; >>>>>> + /* select queue 1 as default */ >>>>> >>>>> It will be nice to expand the comment with explanation of why this is >>>>> being chosen as default (lower priority queue by default for typical >>>>> bulk data transfer). >>>> >>>> Yes, extended comment is a good idea. >>>> >>>> For the next version I think I'm going to change the code around default >>>> TC/Queue and the non default queue selection, mostly based on Joel's comment: >>>> >>>> EVENTQ_1 as default queue. >>>> Set the EVENTQ_1 priority to 7 >>>> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 >>>> >>>> Add new member to struct edma, like high_pri_queue. >>>> When we set the queue priorities in edma_probe() we look for the highest >>>> priority queue and save the number in high_pri_queue. >>>> >>>> I will rename the edma_request_non_default_queue() to >>>> edma_request_high_pri_queue() and it will assign the channel to the high >>>> priority queue. >>>> >>>> I think this way it is going to be more explicit and IMHO a bit more safer in >>>> a sense the we are going to get high priority when we ask for it. >>> >>> Sounds much better. I had posted some ideas about adding support for >>> channel priority in the core code but we can leave that for Vinod and >>> Dan to say if they really see a need for that. > Is it part of this series? No, it is not. The original series tackled the DMA queue selection within the edma driver stack. It was selecting high priority channel for cyclic (audio) use only, all other DMA channels were executed on a lower priority queue. >> If we do it via the dmaengine core I think it would be better to have a new >> flag to be passed to dmaengine_prep_dma_*(). >> We could have for example: >> DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is >> possible. >> We can watch for this flag in the edma driver and act accordingly. >> ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally >> since audio should be treated in this way if the DMA IP can do this. > > Will the priority be different for each descriptor or would be based on channel > usage. I would say that it is channel based config. I don't see the reason why would one mix different priorities on a configured channel between descriptors. > If not then we can add this in dma_slave_config ? So adding to the struct for example: bool high_priority; I'm not sure if it helps if we have let's say three priority level like, low, normal and high. I don't think that any client would ask for low priority instead using the normal priority. > I can forsee its usage on slave controllers, so yes its useful :) True I'm sure it is going to be used as soon as it is available if the HW supports priorities.
On 04/11/2014 02:31 PM, Vinod Koul wrote: >> I would say that it is channel based config. I don't see the reason why would >> one mix different priorities on a configured channel between descriptors. >> >>> If not then we can add this in dma_slave_config ? >> >> So adding to the struct for example: >> bool high_priority; > > In designware controller, we can have channel priorties from 0 to 7 which IIRC 7 > being highest. So bool wont work. unsigned int/u8 would be good. I see. But from a generic code what number should one pass if we want to get the highest priority? With eDMA3 we essentially have three levels (see later) so if we receive 7 as priority what shall we do? Just treat as highest? But if we receive 4, which is somewhere in the middle for designware it is still means highest for us. I see this too small step partitioning in common code a bit problematic when it comes to how to interpret the 'magic numbers'. Also how all the driver in the system will decide the priority number? I'm sure they will pick something conveniently average for themselves and I imagine audio would try to pick something which is bigger than the numbers others have taken... > How about your controller, is it binary? We also have priority from 0 to 7, 0 being the highest priority. We have 3 Transfer Controllers/Event Queues and we can set the priority for the TC/EQ and assign the given channel to the desired TC/EQ. So in reality we have 3 priorities to choose from in my view since we only have 3 TC/EQ in eDMA3 (of AM335x/AM447x) on other SoCs we can have different number of TC/EQ.
On Fri, Apr 11, 2014 at 03:23:54PM +0300, Peter Ujfalusi wrote: > On 04/11/2014 02:31 PM, Vinod Koul wrote: > > >> I would say that it is channel based config. I don't see the reason why would > >> one mix different priorities on a configured channel between descriptors. > >> > >>> If not then we can add this in dma_slave_config ? > >> > >> So adding to the struct for example: > >> bool high_priority; > > > > In designware controller, we can have channel priorties from 0 to 7 which IIRC 7 > > being highest. So bool wont work. unsigned int/u8 would be good. > > I see. But from a generic code what number should one pass if we want to get > the highest priority? With eDMA3 we essentially have three levels (see later) > so if we receive 7 as priority what shall we do? Just treat as highest? But if > we receive 4, which is somewhere in the middle for designware it is still > means highest for us. > > I see this too small step partitioning in common code a bit problematic when > it comes to how to interpret the 'magic numbers'. > Also how all the driver in the system will decide the priority number? I'm > sure they will pick something conveniently average for themselves and I > imagine audio would try to pick something which is bigger than the numbers > others have taken... > > > How about your controller, is it binary? > > We also have priority from 0 to 7, 0 being the highest priority. We have 3 > Transfer Controllers/Event Queues and we can set the priority for the TC/EQ > and assign the given channel to the desired TC/EQ. > So in reality we have 3 priorities to choose from in my view since we only > have 3 TC/EQ in eDMA3 (of AM335x/AM447x) on other SoCs we can have different > number of TC/EQ. I think the number shouldn't be viewed in absolute terms. If we decide that (lets say) 0-7, then any controller should map 0 to lowest and 7 to highest. For your case you can do this and then intermediate numbers would be medium priority. Such a system might work well... Also how would a client driver know which priority to use? Would it come from DT?
On 04/11/2014 04:42 AM, Vinod Koul wrote:> On Fri, Apr 11, 2014 at 12:38:00PM +0300, Peter Ujfalusi wrote: >> On 04/11/2014 11:56 AM, Sekhar Nori wrote: >>> On Friday 11 April 2014 02:20 PM, Peter Ujfalusi wrote: >>>> On 04/11/2014 11:17 AM, Sekhar Nori wrote: >>>>> On Tuesday 01 April 2014 06:36 PM, Peter Ujfalusi wrote: >>>>>> Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high >>>>>> priority channels, like audio. >>>>>> >>>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>>> >>>>> Acked-by: Sekhar Nori <nsekhar@ti.com> >>>>> >>>>>> --- >>>>>> arch/arm/common/edma.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c >>>>>> index 86a8b263278f..19520e2519d9 100644 >>>>>> --- a/arch/arm/common/edma.c >>>>>> +++ b/arch/arm/common/edma.c >>>>>> @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, >>>>>> >>>>>> pdata->queue_priority_mapping = queue_priority_map; >>>>>> >>>>>> - pdata->default_queue = 0; >>>>>> + /* select queue 1 as default */ >>>>> >>>>> It will be nice to expand the comment with explanation of why this is >>>>> being chosen as default (lower priority queue by default for typical >>>>> bulk data transfer). >>>> >>>> Yes, extended comment is a good idea. >>>> >>>> For the next version I think I'm going to change the code around default >>>> TC/Queue and the non default queue selection, mostly based on Joel's comment: >>>> >>>> EVENTQ_1 as default queue. >>>> Set the EVENTQ_1 priority to 7 >>>> EVENTQ_0 priority is going to stay 0 and EVENTQ_2 as 2 >>>> >>>> Add new member to struct edma, like high_pri_queue. >>>> When we set the queue priorities in edma_probe() we look for the highest >>>> priority queue and save the number in high_pri_queue. >>>> >>>> I will rename the edma_request_non_default_queue() to >>>> edma_request_high_pri_queue() and it will assign the channel to the high >>>> priority queue. >>>> >>>> I think this way it is going to be more explicit and IMHO a bit more safer in >>>> a sense the we are going to get high priority when we ask for it. >>> >>> Sounds much better. I had posted some ideas about adding support for >>> channel priority in the core code but we can leave that for Vinod and >>> Dan to say if they really see a need for that. > Is it part of this series? > >> If we do it via the dmaengine core I think it would be better to have a new >> flag to be passed to dmaengine_prep_dma_*(). >> We could have for example: >> DMA_PREP_HIGH_PRI as flag to indicate that we need high priority DMA if it is >> possible. >> We can watch for this flag in the edma driver and act accordingly. >> ALSA's dmaengine_pcm_prepare_and_submit() could set this flag unconditionally >> since audio should be treated in this way if the DMA IP can do this. > Will the priority be different for each descriptor or would be based on channel > usage. If not then we can add this in dma_slave_config ? > > I can forsee its usage on slave controllers, so yes its useful :) > I agree, a better place to do this would be in the config stage, since we can set in stone (for EDMA atleast) that the channel has a certain priority. I can see where per-desc priority may help, such as a case where 2 users of the same channel want to submit different priority desc. For EDMA hardware though, there is no such support and the priority mapping is from channel->queue. One way to add per-desc support would be to change the priority of the channel itself for every desc issued, based on the priority stored in the desc itself which would be stored in the prep stage. But currently we don't have such a usecase of changing priorities based on desc. thanks, -Joel
Hi Vinod, On 04/11/2014 03:46 PM, Vinod Koul wrote: > I think the number shouldn't be viewed in absolute terms. If we decide that (lets > say) 0-7, then any controller should map 0 to lowest and 7 to highest. > > For your case you can do this and then intermediate numbers would be medium > priority. Such a system might work well... > > Also how would a client driver know which priority to use? Would it come from > DT? I think DT would be the best place. Not sure if we should set the range for this either. What I was thinking is to add an optional new property to be set by the client nodes, using DMA: mcasp0: mcasp@48038000 { compatible = "ti,am33xx-mcasp-audio"; ... dmas = <&edma 8>, <&edma 9>; dma-names = "tx", "rx"; dma-priorities = <2>, <2>; }; We could agree that lower number means lower priority, higher is - well - higher priority. If the dma-priority is missing we should assume lowest priority (0). The highest priority depends on the platform. For eDMA3 in AM335x it is three level. For designware controller you might have the range 0-8 as valid. The question is how to get this information into use? We can take the priority number in the core when the dma channel is requested and add field to "struct dma_chan" in order to store it and the DMA drivers could have access to it. In this way we only need to update the nodes which needs non default priority for DMA. What do you think?
On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote: > Hi Vinod, > > On 04/11/2014 03:46 PM, Vinod Koul wrote: >> I think the number shouldn't be viewed in absolute terms. If we decide that (lets >> say) 0-7, then any controller should map 0 to lowest and 7 to highest. >> >> For your case you can do this and then intermediate numbers would be medium >> priority. Such a system might work well... >> >> Also how would a client driver know which priority to use? Would it come from >> DT? > > I think DT would be the best place. In the strict sense of what DT is supposed to represent, DT is not the best place for this. Priority is usecase driven rather that hardware description driven. So on a reasonably less loaded system, I am sure you could run audio even with a lower priority DMA queue. Moreover, IMHO, encoding it in DT now will make it an ABI. Without a wide variety of example use cases, I think it is too early to commit to an ABI. > Not sure if we should set the range for this either. What I was thinking is to > add an optional new property to be set by the client nodes, using DMA: > > mcasp0: mcasp@48038000 { > compatible = "ti,am33xx-mcasp-audio"; > ... > dmas = <&edma 8>, > <&edma 9>; > dma-names = "tx", "rx"; > dma-priorities = <2>, <2>; > }; > > We could agree that lower number means lower priority, higher is - well - > higher priority. > If the dma-priority is missing we should assume lowest priority (0). > The highest priority depends on the platform. For eDMA3 in AM335x it is three > level. For designware controller you might have the range 0-8 as valid. > > The question is how to get this information into use? > We can take the priority number in the core when the dma channel is requested > and add field to "struct dma_chan" in order to store it and the DMA drivers > could have access to it. > In this way we only need to update the nodes which needs non default priority > for DMA. > > What do you think? If we are using dma_slave_config, I think it will be easiest to define two levels of priority (HIGH and LOW, as thats all we see a need for right now), and have the audio driver select the HIGH priority. If nothing is set, EDMA can default to LOW. Thanks, Sekhar
On 04/14/2014 03:12 PM, Sekhar Nori wrote: > On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote: >> Hi Vinod, >> >> On 04/11/2014 03:46 PM, Vinod Koul wrote: >>> I think the number shouldn't be viewed in absolute terms. If we decide that (lets >>> say) 0-7, then any controller should map 0 to lowest and 7 to highest. >>> >>> For your case you can do this and then intermediate numbers would be medium >>> priority. Such a system might work well... >>> >>> Also how would a client driver know which priority to use? Would it come from >>> DT? >> >> I think DT would be the best place. > > In the strict sense of what DT is supposed to represent, DT is not the > best place for this. Priority is usecase driven rather that hardware > description driven. While this is true, the DMA priority - if supported by the DMA engine - is a HW feature as well. If it is supported by the HW it can be used to tune and partition the system for the anticipated load or purpose. > So on a reasonably less loaded system, I am sure you > could run audio even with a lower priority DMA queue. Yes, you can. But as soon as you have other devices using the same priority (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio. During audio playback/capture you execute a long MMC read for example can introduce a glitch. > Moreover, IMHO, encoding it in DT now will make it an ABI. Without a > wide variety of example use cases, I think it is too early to commit to > an ABI. True, but we need to start from somewhere? I'm fine if we handle this right now as I did in this series (to put only cyclic on high priority) for now. With some forward looking changes in the implementation of course. >> Not sure if we should set the range for this either. What I was thinking is to >> add an optional new property to be set by the client nodes, using DMA: >> >> mcasp0: mcasp@48038000 { >> compatible = "ti,am33xx-mcasp-audio"; >> ... >> dmas = <&edma 8>, >> <&edma 9>; >> dma-names = "tx", "rx"; >> dma-priorities = <2>, <2>; >> }; >> >> We could agree that lower number means lower priority, higher is - well - >> higher priority. >> If the dma-priority is missing we should assume lowest priority (0). >> The highest priority depends on the platform. For eDMA3 in AM335x it is three >> level. For designware controller you might have the range 0-8 as valid. >> >> The question is how to get this information into use? >> We can take the priority number in the core when the dma channel is requested >> and add field to "struct dma_chan" in order to store it and the DMA drivers >> could have access to it. >> In this way we only need to update the nodes which needs non default priority >> for DMA. >> >> What do you think? > > If we are using dma_slave_config, I think it will be easiest to define > two levels of priority (HIGH and LOW, as thats all we see a need for > right now), and have the audio driver select the HIGH priority. If > nothing is set, EDMA can default to LOW. But the information on which channel should be high priority should be coming form somewhere... We can wire it in the audio stack, but I don't think it is a good idea to start with. And if we have high/low priority we could as well have an integer to specify the desired level.
On Monday 14 April 2014 06:11 PM, Peter Ujfalusi wrote: > On 04/14/2014 03:12 PM, Sekhar Nori wrote: >> On Monday 14 April 2014 05:26 PM, Peter Ujfalusi wrote: >>> Hi Vinod, >>> >>> On 04/11/2014 03:46 PM, Vinod Koul wrote: >>>> I think the number shouldn't be viewed in absolute terms. If we decide that (lets >>>> say) 0-7, then any controller should map 0 to lowest and 7 to highest. >>>> >>>> For your case you can do this and then intermediate numbers would be medium >>>> priority. Such a system might work well... >>>> >>>> Also how would a client driver know which priority to use? Would it come from >>>> DT? >>> >>> I think DT would be the best place. >> >> In the strict sense of what DT is supposed to represent, DT is not the >> best place for this. Priority is usecase driven rather that hardware >> description driven. > > While this is true, the DMA priority - if supported by the DMA engine - is a > HW feature as well. If it is supported by the HW it can be used to tune and > partition the system for the anticipated load or purpose. > >> So on a reasonably less loaded system, I am sure you >> could run audio even with a lower priority DMA queue. > > Yes, you can. But as soon as you have other devices using the same priority > (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio. > During audio playback/capture you execute a long MMC read for example can > introduce a glitch. > >> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a >> wide variety of example use cases, I think it is too early to commit to >> an ABI. > > True, but we need to start from somewhere? Right, and based on our IRC discussion, we are not really fixing up the priority value space. That makes me much more comfortable with the idea. >>> Not sure if we should set the range for this either. What I was thinking is to >>> add an optional new property to be set by the client nodes, using DMA: >>> >>> mcasp0: mcasp@48038000 { >>> compatible = "ti,am33xx-mcasp-audio"; >>> ... >>> dmas = <&edma 8>, >>> <&edma 9>; >>> dma-names = "tx", "rx"; >>> dma-priorities = <2>, <2>; >>> }; >>> >>> We could agree that lower number means lower priority, higher is - well - >>> higher priority. Even this does not have to be uniform across, right? The numbers could be left to interpretation per-SoC. >>> If the dma-priority is missing we should assume lowest priority (0). >>> The highest priority depends on the platform. For eDMA3 in AM335x it is three >>> level. For designware controller you might have the range 0-8 as valid. >>> >>> The question is how to get this information into use? >>> We can take the priority number in the core when the dma channel is requested >>> and add field to "struct dma_chan" in order to store it and the DMA drivers >>> could have access to it. How about Vinod's idea of extending dma_slave_config? Priority is similar to rest of the runtime setup dmaengine_slave_config() is meant to do. Thanks, Sekhar
On 04/14/2014 05:32 PM, Sekhar Nori wrote: >> Yes, you can. But as soon as you have other devices using the same priority >> (with eDMA3 at least) and asks for a 'long' transfer it can ruin the audio. >> During audio playback/capture you execute a long MMC read for example can >> introduce a glitch. >> >>> Moreover, IMHO, encoding it in DT now will make it an ABI. Without a >>> wide variety of example use cases, I think it is too early to commit to >>> an ABI. >> >> True, but we need to start from somewhere? > > Right, and based on our IRC discussion, we are not really fixing up the > priority value space. That makes me much more comfortable with the idea. The only thing we should agree on is the 0 means lowest priority. I think this will help in case when a new kernel is fed with an older dt blob where the property does not exist. > >>>> Not sure if we should set the range for this either. What I was thinking is to >>>> add an optional new property to be set by the client nodes, using DMA: >>>> >>>> mcasp0: mcasp@48038000 { >>>> compatible = "ti,am33xx-mcasp-audio"; >>>> ... >>>> dmas = <&edma 8>, >>>> <&edma 9>; >>>> dma-names = "tx", "rx"; >>>> dma-priorities = <2>, <2>; >>>> }; >>>> > >>>> We could agree that lower number means lower priority, higher is - well - >>>> higher priority. > > Even this does not have to be uniform across, right? The numbers could > be left to interpretation per-SoC. > >>>> If the dma-priority is missing we should assume lowest priority (0). >>>> The highest priority depends on the platform. For eDMA3 in AM335x it is three >>>> level. For designware controller you might have the range 0-8 as valid. >>>> >>>> The question is how to get this information into use? >>>> We can take the priority number in the core when the dma channel is requested >>>> and add field to "struct dma_chan" in order to store it and the DMA drivers >>>> could have access to it. > > How about Vinod's idea of extending dma_slave_config? Priority is > similar to rest of the runtime setup dmaengine_slave_config() is meant > to do. The dma_slave_config is prepared by the client drivers, so they would need to be updated to handle the priority for the DMA. This would lead to duplicated code - unless we have a small function in dmaengine core to fetch this information, but still all dmaengine clients would need to call that. IMHO it would be better to let the dmaengine core to take the priority for the channel when it has been asked so client drivers does not need to know about it.
On 04/16/2014 07:59 AM, Peter Ujfalusi wrote: [..] >>>>> If the dma-priority is missing we should assume lowest priority (0). >>>>> The highest priority depends on the platform. For eDMA3 in AM335x it is three >>>>> level. For designware controller you might have the range 0-8 as valid. >>>>> >>>>> The question is how to get this information into use? >>>>> We can take the priority number in the core when the dma channel is requested >>>>> and add field to "struct dma_chan" in order to store it and the DMA drivers >>>>> could have access to it. >> >> How about Vinod's idea of extending dma_slave_config? Priority is >> similar to rest of the runtime setup dmaengine_slave_config() is meant >> to do. > > The dma_slave_config is prepared by the client drivers, so they would need to > be updated to handle the priority for the DMA. This would lead to duplicated > code - unless we have a small function in dmaengine core to fetch this > information, but still all dmaengine clients would need to call that. > IMHO it would be better to let the dmaengine core to take the priority for the > channel when it has been asked so client drivers does not need to know about it. > I still feel it is much cleaner to keep this out of DT and have audio driver configure the channel manually for higher priority. Because, AFAIK audio is the only device that uses slave DMA and needs higher priority. I'd imagine everyone else who needs high priority, have their own dedicated DMAC, so from that sense I don't see the priority mechanism being used a lot anywhere else but audio, so atleast we can rule out things like code duplication in other drivers. Priority can be set to a default of low for those drivers that don't configure the channel for priority. I'm also OK with EDMA driver configuring channel for higher priority manually for the Cyclic case like you did initially. Thanks, -Joel
On 04/16/2014 07:05 PM, Joel Fernandes wrote: > On 04/16/2014 07:59 AM, Peter Ujfalusi wrote: > [..] >>>>>> If the dma-priority is missing we should assume lowest priority (0). >>>>>> The highest priority depends on the platform. For eDMA3 in AM335x it is three >>>>>> level. For designware controller you might have the range 0-8 as valid. >>>>>> >>>>>> The question is how to get this information into use? >>>>>> We can take the priority number in the core when the dma channel is requested >>>>>> and add field to "struct dma_chan" in order to store it and the DMA drivers >>>>>> could have access to it. >>> >>> How about Vinod's idea of extending dma_slave_config? Priority is >>> similar to rest of the runtime setup dmaengine_slave_config() is meant >>> to do. >> >> The dma_slave_config is prepared by the client drivers, so they would need to >> be updated to handle the priority for the DMA. This would lead to duplicated >> code - unless we have a small function in dmaengine core to fetch this >> information, but still all dmaengine clients would need to call that. >> IMHO it would be better to let the dmaengine core to take the priority for the >> channel when it has been asked so client drivers does not need to know about it. >> > > I still feel it is much cleaner to keep this out of DT and have audio > driver configure the channel manually for higher priority. Because, > AFAIK audio is the only device that uses slave DMA and needs higher > priority. I'd imagine everyone else who needs high priority, have their > own dedicated DMAC, so from that sense I don't see the priority > mechanism being used a lot anywhere else but audio, so atleast we can > rule out things like code duplication in other drivers. Priority can be > set to a default of low for those drivers that don't configure the > channel for priority. I'm also OK with EDMA driver configuring channel > for higher priority manually for the Cyclic case like you did initially. So how should we go about this? I'm fine to select higher priority in the eDMA driver for now when a cyclic channel is configured and later when we have (if we ever have) generic way to handle DMA channel/transaction priority we can switch eDMA as well to use it.
diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c index 86a8b263278f..19520e2519d9 100644 --- a/arch/arm/common/edma.c +++ b/arch/arm/common/edma.c @@ -1546,7 +1546,8 @@ static int edma_of_parse_dt(struct device *dev, pdata->queue_priority_mapping = queue_priority_map; - pdata->default_queue = 0; + /* select queue 1 as default */ + pdata->default_queue = EVENTQ_1; prop = of_find_property(node, "ti,edma-xbar-event-map", &sz); if (prop)
Use the EVENTQ_1 for default and leave the EVENTQ_0 to be used by high priority channels, like audio. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- arch/arm/common/edma.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)