Re: [PATCH v2 05/14] arm: common: edma: Select event queue 1 as default when booted with DT

[Peter Ujfalusi]

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@xxxxxx>
>>>>>
>>>>> Acked-by: Sekhar Nori <nsekhar@xxxxxx>
>>>>>
>>>>>> ---
>>>>>>  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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/