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

[Joel Fernandes]

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@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?
>
>> 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

--
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/