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

[Vinod Koul <vinod.koul@intel.com>]

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?

-- 
~Vinod

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

--