From: Horng-Shyang Liao <hs.liao@mediatek.com>
To: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Daniel Kurtz <djkurtz@chromium.org>,
Sascha Hauer <s.hauer@pengutronix.de>,
<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-mediatek@lists.infradead.org>,
<srv_heupstream@mediatek.com>,
"Sascha Hauer" <kernel@pengutronix.de>,
Philipp Zabel <p.zabel@pengutronix.de>,
Nicolas Boichat <drinkcat@chromium.org>,
CK HU <ck.hu@mediatek.com>,
"cawa cheng" <cawa.cheng@mediatek.com>,
Bibby Hsieh <bibby.hsieh@mediatek.com>,
"YT Shen" <yt.shen@mediatek.com>,
Daoyuan Huang <daoyuan.huang@mediatek.com>,
Damon Chu <damon.chu@mediatek.com>,
Josh-YC Liu <josh-yc.liu@mediatek.com>,
Glory Hung <glory.hung@mediatek.com>,
Jiaguang Zhang <jiaguang.zhang@mediatek.com>,
Dennis-YC Hsieh <dennis-yc.hsieh@mediatek.com>,
Monica Wang <monica.wang@mediatek.com>,
<jassisinghbrar@gmail.com>, <jaswinder.singh@linaro.org>,
<hs.liao@mediatek.com>
Subject: Re: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver
Date: Tue, 14 Jun 2016 15:44:28 +0800 [thread overview]
Message-ID: <1465890268.7191.13.camel@mtksdaap41> (raw)
In-Reply-To: <57583B45.2080504@gmail.com>
Hi Matthias,
On Wed, 2016-06-08 at 17:35 +0200, Matthias Brugger wrote:
>
> On 08/06/16 14:25, Horng-Shyang Liao wrote:
> > Hi Matthias,
> >
> > On Wed, 2016-06-08 at 12:45 +0200, Matthias Brugger wrote:
> >>
> >> On 08/06/16 07:40, Horng-Shyang Liao wrote:
> >>> Hi Matthias,
> >>>
> >>> On Tue, 2016-06-07 at 18:59 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 03/06/16 15:11, Matthias Brugger wrote:
> >>>>>
> >>>>>
> >>>> [...]
> >>>>
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + smp_mb(); /* modify jump before enable thread */
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + cmdq_thread_writel(thread, task->pa_base +
> >>>>>>>>>>>>>> task->command_size,
> >>>>>>>>>>>>>> + CMDQ_THR_END_ADDR);
> >>>>>>>>>>>>>> + cmdq_thread_resume(thread);
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> + list_move_tail(&task->list_entry, &thread->task_busy_list);
> >>>>>>>>>>>>>> + spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +static void cmdq_handle_error_done(struct cmdq *cmdq,
> >>>>>>>>>>>>>> + struct cmdq_thread *thread, u32 irq_flag)
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> + struct cmdq_task *task, *tmp, *curr_task = NULL;
> >>>>>>>>>>>>>> + u32 curr_pa;
> >>>>>>>>>>>>>> + struct cmdq_cb_data cmdq_cb_data;
> >>>>>>>>>>>>>> + bool err;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + if (irq_flag & CMDQ_THR_IRQ_ERROR)
> >>>>>>>>>>>>>> + err = true;
> >>>>>>>>>>>>>> + else if (irq_flag & CMDQ_THR_IRQ_DONE)
> >>>>>>>>>>>>>> + err = false;
> >>>>>>>>>>>>>> + else
> >>>>>>>>>>>>>> + return;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + curr_pa = cmdq_thread_readl(thread, CMDQ_THR_CURR_ADDR);
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> >>>>>>>>>>>>>> + list_entry) {
> >>>>>>>>>>>>>> + if (curr_pa >= task->pa_base &&
> >>>>>>>>>>>>>> + curr_pa < (task->pa_base + task->command_size))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What are you checking here? It seems as if you make some implcit
> >>>>>>>>>>>>> assumptions about pa_base and the order of execution of
> >>>>>>>>>>>>> commands in the
> >>>>>>>>>>>>> thread. Is it save to do so? Does dma_alloc_coherent give any
> >>>>>>>>>>>>> guarantees
> >>>>>>>>>>>>> about dma_handle?
> >>>>>>>>>>>>
> >>>>>>>>>>>> 1. Check what is the current running task in this GCE thread.
> >>>>>>>>>>>> 2. Yes.
> >>>>>>>>>>>> 3. Yes, CMDQ doesn't use iommu, so physical address is continuous.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, physical addresses might be continous, but AFAIK there is no
> >>>>>>>>>>> guarantee that the dma_handle address is steadily growing, when
> >>>>>>>>>>> calling
> >>>>>>>>>>> dma_alloc_coherent. And if I understand the code correctly, you
> >>>>>>>>>>> use this
> >>>>>>>>>>> assumption to decide if the task picked from task_busy_list is
> >>>>>>>>>>> currently
> >>>>>>>>>>> executing. So I think this mecanism is not working.
> >>>>>>>>>>
> >>>>>>>>>> I don't use dma_handle address, and just use physical addresses.
> >>>>>>>>>> From CPU's point of view, tasks are linked by the busy list.
> >>>>>>>>>> From GCE's point of view, tasks are linked by the JUMP command.
> >>>>>>>>>>
> >>>>>>>>>>> In which cases does the HW thread raise an interrupt.
> >>>>>>>>>>> In case of error. When does CMDQ_THR_IRQ_DONE get raised?
> >>>>>>>>>>
> >>>>>>>>>> GCE will raise interrupt if any task is done or error.
> >>>>>>>>>> However, GCE is fast, so CPU may get multiple done tasks
> >>>>>>>>>> when it is running ISR.
> >>>>>>>>>>
> >>>>>>>>>> In case of error, that GCE thread will pause and raise interrupt.
> >>>>>>>>>> So, CPU may get multiple done tasks and one error task.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I think we should reimplement the ISR mechanism. Can't we just read
> >>>>>>>>> CURR_IRQ_STATUS and THR_IRQ_STATUS in the handler and leave
> >>>>>>>>> cmdq_handle_error_done to the thread_fn? You will need to pass
> >>>>>>>>> information from the handler to thread_fn, but that shouldn't be an
> >>>>>>>>> issue. AFAIK interrupts are disabled in the handler, so we should stay
> >>>>>>>>> there as short as possible. Traversing task_busy_list is expensive, so
> >>>>>>>>> we need to do it in a thread context.
> >>>>>>>>
> >>>>>>>> Actually, our initial implementation is similar to your suggestion,
> >>>>>>>> but display needs CMDQ to return callback function very precisely,
> >>>>>>>> else display will drop frame.
> >>>>>>>> For display, CMDQ interrupt will be raised every 16 ~ 17 ms,
> >>>>>>>> and CMDQ needs to call callback function in ISR.
> >>>>>>>> If we defer callback to workqueue, the time interval may be larger than
> >>>>>>>> 32 ms.sometimes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think the problem is, that you implemented the workqueue as a ordered
> >>>>>>> workqueue, so there is no parallel processing. I'm still not sure why
> >>>>>>> you need the workqueue to be ordered. Can you please explain.
> >>>>>>
> >>>>>> The order should be kept.
> >>>>>> Let me use mouse cursor as an example.
> >>>>>> If task 1 means move mouse cursor to point A, task 2 means point B,
> >>>>>> and task 3 means point C, our expected result is A -> B -> C.
> >>>>>> If the order is not kept, the result could become A -> C -> B.
> >>>>>>
> >>>>>
> >>>>> Got it, thanks for the clarification.
> >>>>>
> >>>>
> >>>> I think a way to get rid of the workqueue is to use a timer, which gets
> >>>> programmed to the time a timeout in the first task in the busy list
> >>>> would happen. Everytime we update the busy list (e.g. because of task
> >>>> got finished by the thread), we update the timer. When the timer
> >>>> triggers, which hopefully won't happen too often, we return timeout on
> >>>> the busy list elements, until the time is lower then the actual time.
> >>>>
> >>>> At least with this we can reduce the data structures in this driver and
> >>>> make it more lightweight.
> >>>
> >>> From my understanding, your proposed method can handle timeout case.
> >>>
> >>> However, the workqueue is also in charge of releasing tasks.
> >>> Do you take releasing tasks into consideration by using the proposed
> >>> timer method?
> >>> Furthermore, I think the code will become more complex if we also use
> >>> timer to implement releasing tasks.
> >>>
> >>
> >> Can't we call
> >> clk_disable_unprepare(cmdq->clock);
> >> cmdq_task_release(task);
> >> after invoking the callback?
> >
> > Do you mean just call these two functions in ISR?
> > My major concern is dma_free_coherent() and kfree() in
> > cmdq_task_release(task).
>
> Why do we need the dma calls at all? Can't we just calculate the
> physical address using __pa(x)?
I prefer to use dma_map_single/dma_unmap_single.
> > Therefore, your suggestion is to use GFP_ATOMIC for both
> > dma_alloc_coherent() and kzalloc(). Right?
>
> I don't think we need GFP_ATOMIC, the critical path will just free the
> memory.
I tested these two functions, and kfree was safe.
However, dma_free_coherent raised BUG.
BUG: failure at
/mnt/host/source/src/third_party/kernel/v3.18/mm/vmalloc.c:1514/vunmap()!
1512 void vunmap(const void *addr)
1513 {
1514 BUG_ON(in_interrupt()); // <-- here
1515 might_sleep();
1516 if (addr)
1517 __vunmap(addr, 0);
1518 }
1519 EXPORT_SYMBOL(vunmap);
Therefore, I plan to use kmalloc + dma_map_single instead of
dma_alloc_coherent, and dma_unmap_single + kfree instead of
dma_free_coherent.
What do you think about the function replacement?
> > If so, I can try to implement timeout by timer, and discuss with you
> > if I have further questions.
> >
>
> Sounds good :)
>
> Thanks,
> Matthias
Thanks,
HS
> >> Regrading the clock, wouldn't it be easier to handle the clock
> >> enable/disable depending on the state of task_busy_list? I suppose we
> >> can't as we would need to check the task_busy_list of all threads, right?
> >>
> >> Regards,
> >> Matthias
> >
> > Thanks,
> > HS
> >
WARNING: multiple messages have this Message-ID (diff)
From: Horng-Shyang Liao <hs.liao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
To: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
Daniel Kurtz <djkurtz-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org,
Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>,
Nicolas Boichat
<drinkcat-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
CK HU <ck.hu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
cawa cheng <cawa.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Bibby Hsieh <bibby.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
YT Shen <yt.shen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Daoyuan Huang
<daoyuan.huang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Damon Chu <damon.chu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Josh-YC Liu <josh-yc.liu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Glory Hung <glory.hung-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Jiaguang Zhang
<jiaguang.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Dennis-YC Hsieh
<dennis-yc.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
Monica Wang <monica.wang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>,
jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
jaswinder.
Subject: Re: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver
Date: Tue, 14 Jun 2016 15:44:28 +0800 [thread overview]
Message-ID: <1465890268.7191.13.camel@mtksdaap41> (raw)
In-Reply-To: <57583B45.2080504-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Matthias,
On Wed, 2016-06-08 at 17:35 +0200, Matthias Brugger wrote:
>
> On 08/06/16 14:25, Horng-Shyang Liao wrote:
> > Hi Matthias,
> >
> > On Wed, 2016-06-08 at 12:45 +0200, Matthias Brugger wrote:
> >>
> >> On 08/06/16 07:40, Horng-Shyang Liao wrote:
> >>> Hi Matthias,
> >>>
> >>> On Tue, 2016-06-07 at 18:59 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 03/06/16 15:11, Matthias Brugger wrote:
> >>>>>
> >>>>>
> >>>> [...]
> >>>>
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + smp_mb(); /* modify jump before enable thread */
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + cmdq_thread_writel(thread, task->pa_base +
> >>>>>>>>>>>>>> task->command_size,
> >>>>>>>>>>>>>> + CMDQ_THR_END_ADDR);
> >>>>>>>>>>>>>> + cmdq_thread_resume(thread);
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> + list_move_tail(&task->list_entry, &thread->task_busy_list);
> >>>>>>>>>>>>>> + spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +static void cmdq_handle_error_done(struct cmdq *cmdq,
> >>>>>>>>>>>>>> + struct cmdq_thread *thread, u32 irq_flag)
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> + struct cmdq_task *task, *tmp, *curr_task = NULL;
> >>>>>>>>>>>>>> + u32 curr_pa;
> >>>>>>>>>>>>>> + struct cmdq_cb_data cmdq_cb_data;
> >>>>>>>>>>>>>> + bool err;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + if (irq_flag & CMDQ_THR_IRQ_ERROR)
> >>>>>>>>>>>>>> + err = true;
> >>>>>>>>>>>>>> + else if (irq_flag & CMDQ_THR_IRQ_DONE)
> >>>>>>>>>>>>>> + err = false;
> >>>>>>>>>>>>>> + else
> >>>>>>>>>>>>>> + return;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + curr_pa = cmdq_thread_readl(thread, CMDQ_THR_CURR_ADDR);
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> >>>>>>>>>>>>>> + list_entry) {
> >>>>>>>>>>>>>> + if (curr_pa >= task->pa_base &&
> >>>>>>>>>>>>>> + curr_pa < (task->pa_base + task->command_size))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What are you checking here? It seems as if you make some implcit
> >>>>>>>>>>>>> assumptions about pa_base and the order of execution of
> >>>>>>>>>>>>> commands in the
> >>>>>>>>>>>>> thread. Is it save to do so? Does dma_alloc_coherent give any
> >>>>>>>>>>>>> guarantees
> >>>>>>>>>>>>> about dma_handle?
> >>>>>>>>>>>>
> >>>>>>>>>>>> 1. Check what is the current running task in this GCE thread.
> >>>>>>>>>>>> 2. Yes.
> >>>>>>>>>>>> 3. Yes, CMDQ doesn't use iommu, so physical address is continuous.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, physical addresses might be continous, but AFAIK there is no
> >>>>>>>>>>> guarantee that the dma_handle address is steadily growing, when
> >>>>>>>>>>> calling
> >>>>>>>>>>> dma_alloc_coherent. And if I understand the code correctly, you
> >>>>>>>>>>> use this
> >>>>>>>>>>> assumption to decide if the task picked from task_busy_list is
> >>>>>>>>>>> currently
> >>>>>>>>>>> executing. So I think this mecanism is not working.
> >>>>>>>>>>
> >>>>>>>>>> I don't use dma_handle address, and just use physical addresses.
> >>>>>>>>>> From CPU's point of view, tasks are linked by the busy list.
> >>>>>>>>>> From GCE's point of view, tasks are linked by the JUMP command.
> >>>>>>>>>>
> >>>>>>>>>>> In which cases does the HW thread raise an interrupt.
> >>>>>>>>>>> In case of error. When does CMDQ_THR_IRQ_DONE get raised?
> >>>>>>>>>>
> >>>>>>>>>> GCE will raise interrupt if any task is done or error.
> >>>>>>>>>> However, GCE is fast, so CPU may get multiple done tasks
> >>>>>>>>>> when it is running ISR.
> >>>>>>>>>>
> >>>>>>>>>> In case of error, that GCE thread will pause and raise interrupt.
> >>>>>>>>>> So, CPU may get multiple done tasks and one error task.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I think we should reimplement the ISR mechanism. Can't we just read
> >>>>>>>>> CURR_IRQ_STATUS and THR_IRQ_STATUS in the handler and leave
> >>>>>>>>> cmdq_handle_error_done to the thread_fn? You will need to pass
> >>>>>>>>> information from the handler to thread_fn, but that shouldn't be an
> >>>>>>>>> issue. AFAIK interrupts are disabled in the handler, so we should stay
> >>>>>>>>> there as short as possible. Traversing task_busy_list is expensive, so
> >>>>>>>>> we need to do it in a thread context.
> >>>>>>>>
> >>>>>>>> Actually, our initial implementation is similar to your suggestion,
> >>>>>>>> but display needs CMDQ to return callback function very precisely,
> >>>>>>>> else display will drop frame.
> >>>>>>>> For display, CMDQ interrupt will be raised every 16 ~ 17 ms,
> >>>>>>>> and CMDQ needs to call callback function in ISR.
> >>>>>>>> If we defer callback to workqueue, the time interval may be larger than
> >>>>>>>> 32 ms.sometimes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think the problem is, that you implemented the workqueue as a ordered
> >>>>>>> workqueue, so there is no parallel processing. I'm still not sure why
> >>>>>>> you need the workqueue to be ordered. Can you please explain.
> >>>>>>
> >>>>>> The order should be kept.
> >>>>>> Let me use mouse cursor as an example.
> >>>>>> If task 1 means move mouse cursor to point A, task 2 means point B,
> >>>>>> and task 3 means point C, our expected result is A -> B -> C.
> >>>>>> If the order is not kept, the result could become A -> C -> B.
> >>>>>>
> >>>>>
> >>>>> Got it, thanks for the clarification.
> >>>>>
> >>>>
> >>>> I think a way to get rid of the workqueue is to use a timer, which gets
> >>>> programmed to the time a timeout in the first task in the busy list
> >>>> would happen. Everytime we update the busy list (e.g. because of task
> >>>> got finished by the thread), we update the timer. When the timer
> >>>> triggers, which hopefully won't happen too often, we return timeout on
> >>>> the busy list elements, until the time is lower then the actual time.
> >>>>
> >>>> At least with this we can reduce the data structures in this driver and
> >>>> make it more lightweight.
> >>>
> >>> From my understanding, your proposed method can handle timeout case.
> >>>
> >>> However, the workqueue is also in charge of releasing tasks.
> >>> Do you take releasing tasks into consideration by using the proposed
> >>> timer method?
> >>> Furthermore, I think the code will become more complex if we also use
> >>> timer to implement releasing tasks.
> >>>
> >>
> >> Can't we call
> >> clk_disable_unprepare(cmdq->clock);
> >> cmdq_task_release(task);
> >> after invoking the callback?
> >
> > Do you mean just call these two functions in ISR?
> > My major concern is dma_free_coherent() and kfree() in
> > cmdq_task_release(task).
>
> Why do we need the dma calls at all? Can't we just calculate the
> physical address using __pa(x)?
I prefer to use dma_map_single/dma_unmap_single.
> > Therefore, your suggestion is to use GFP_ATOMIC for both
> > dma_alloc_coherent() and kzalloc(). Right?
>
> I don't think we need GFP_ATOMIC, the critical path will just free the
> memory.
I tested these two functions, and kfree was safe.
However, dma_free_coherent raised BUG.
BUG: failure at
/mnt/host/source/src/third_party/kernel/v3.18/mm/vmalloc.c:1514/vunmap()!
1512 void vunmap(const void *addr)
1513 {
1514 BUG_ON(in_interrupt()); // <-- here
1515 might_sleep();
1516 if (addr)
1517 __vunmap(addr, 0);
1518 }
1519 EXPORT_SYMBOL(vunmap);
Therefore, I plan to use kmalloc + dma_map_single instead of
dma_alloc_coherent, and dma_unmap_single + kfree instead of
dma_free_coherent.
What do you think about the function replacement?
> > If so, I can try to implement timeout by timer, and discuss with you
> > if I have further questions.
> >
>
> Sounds good :)
>
> Thanks,
> Matthias
Thanks,
HS
> >> Regrading the clock, wouldn't it be easier to handle the clock
> >> enable/disable depending on the state of task_busy_list? I suppose we
> >> can't as we would need to check the task_busy_list of all threads, right?
> >>
> >> Regards,
> >> Matthias
> >
> > Thanks,
> > HS
> >
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: hs.liao@mediatek.com (Horng-Shyang Liao)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver
Date: Tue, 14 Jun 2016 15:44:28 +0800 [thread overview]
Message-ID: <1465890268.7191.13.camel@mtksdaap41> (raw)
In-Reply-To: <57583B45.2080504@gmail.com>
Hi Matthias,
On Wed, 2016-06-08 at 17:35 +0200, Matthias Brugger wrote:
>
> On 08/06/16 14:25, Horng-Shyang Liao wrote:
> > Hi Matthias,
> >
> > On Wed, 2016-06-08 at 12:45 +0200, Matthias Brugger wrote:
> >>
> >> On 08/06/16 07:40, Horng-Shyang Liao wrote:
> >>> Hi Matthias,
> >>>
> >>> On Tue, 2016-06-07 at 18:59 +0200, Matthias Brugger wrote:
> >>>>
> >>>> On 03/06/16 15:11, Matthias Brugger wrote:
> >>>>>
> >>>>>
> >>>> [...]
> >>>>
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + smp_mb(); /* modify jump before enable thread */
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + cmdq_thread_writel(thread, task->pa_base +
> >>>>>>>>>>>>>> task->command_size,
> >>>>>>>>>>>>>> + CMDQ_THR_END_ADDR);
> >>>>>>>>>>>>>> + cmdq_thread_resume(thread);
> >>>>>>>>>>>>>> + }
> >>>>>>>>>>>>>> + list_move_tail(&task->list_entry, &thread->task_busy_list);
> >>>>>>>>>>>>>> + spin_unlock_irqrestore(&cmdq->exec_lock, flags);
> >>>>>>>>>>>>>> +}
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> +static void cmdq_handle_error_done(struct cmdq *cmdq,
> >>>>>>>>>>>>>> + struct cmdq_thread *thread, u32 irq_flag)
> >>>>>>>>>>>>>> +{
> >>>>>>>>>>>>>> + struct cmdq_task *task, *tmp, *curr_task = NULL;
> >>>>>>>>>>>>>> + u32 curr_pa;
> >>>>>>>>>>>>>> + struct cmdq_cb_data cmdq_cb_data;
> >>>>>>>>>>>>>> + bool err;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + if (irq_flag & CMDQ_THR_IRQ_ERROR)
> >>>>>>>>>>>>>> + err = true;
> >>>>>>>>>>>>>> + else if (irq_flag & CMDQ_THR_IRQ_DONE)
> >>>>>>>>>>>>>> + err = false;
> >>>>>>>>>>>>>> + else
> >>>>>>>>>>>>>> + return;
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + curr_pa = cmdq_thread_readl(thread, CMDQ_THR_CURR_ADDR);
> >>>>>>>>>>>>>> +
> >>>>>>>>>>>>>> + list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
> >>>>>>>>>>>>>> + list_entry) {
> >>>>>>>>>>>>>> + if (curr_pa >= task->pa_base &&
> >>>>>>>>>>>>>> + curr_pa < (task->pa_base + task->command_size))
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> What are you checking here? It seems as if you make some implcit
> >>>>>>>>>>>>> assumptions about pa_base and the order of execution of
> >>>>>>>>>>>>> commands in the
> >>>>>>>>>>>>> thread. Is it save to do so? Does dma_alloc_coherent give any
> >>>>>>>>>>>>> guarantees
> >>>>>>>>>>>>> about dma_handle?
> >>>>>>>>>>>>
> >>>>>>>>>>>> 1. Check what is the current running task in this GCE thread.
> >>>>>>>>>>>> 2. Yes.
> >>>>>>>>>>>> 3. Yes, CMDQ doesn't use iommu, so physical address is continuous.
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Yes, physical addresses might be continous, but AFAIK there is no
> >>>>>>>>>>> guarantee that the dma_handle address is steadily growing, when
> >>>>>>>>>>> calling
> >>>>>>>>>>> dma_alloc_coherent. And if I understand the code correctly, you
> >>>>>>>>>>> use this
> >>>>>>>>>>> assumption to decide if the task picked from task_busy_list is
> >>>>>>>>>>> currently
> >>>>>>>>>>> executing. So I think this mecanism is not working.
> >>>>>>>>>>
> >>>>>>>>>> I don't use dma_handle address, and just use physical addresses.
> >>>>>>>>>> From CPU's point of view, tasks are linked by the busy list.
> >>>>>>>>>> From GCE's point of view, tasks are linked by the JUMP command.
> >>>>>>>>>>
> >>>>>>>>>>> In which cases does the HW thread raise an interrupt.
> >>>>>>>>>>> In case of error. When does CMDQ_THR_IRQ_DONE get raised?
> >>>>>>>>>>
> >>>>>>>>>> GCE will raise interrupt if any task is done or error.
> >>>>>>>>>> However, GCE is fast, so CPU may get multiple done tasks
> >>>>>>>>>> when it is running ISR.
> >>>>>>>>>>
> >>>>>>>>>> In case of error, that GCE thread will pause and raise interrupt.
> >>>>>>>>>> So, CPU may get multiple done tasks and one error task.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I think we should reimplement the ISR mechanism. Can't we just read
> >>>>>>>>> CURR_IRQ_STATUS and THR_IRQ_STATUS in the handler and leave
> >>>>>>>>> cmdq_handle_error_done to the thread_fn? You will need to pass
> >>>>>>>>> information from the handler to thread_fn, but that shouldn't be an
> >>>>>>>>> issue. AFAIK interrupts are disabled in the handler, so we should stay
> >>>>>>>>> there as short as possible. Traversing task_busy_list is expensive, so
> >>>>>>>>> we need to do it in a thread context.
> >>>>>>>>
> >>>>>>>> Actually, our initial implementation is similar to your suggestion,
> >>>>>>>> but display needs CMDQ to return callback function very precisely,
> >>>>>>>> else display will drop frame.
> >>>>>>>> For display, CMDQ interrupt will be raised every 16 ~ 17 ms,
> >>>>>>>> and CMDQ needs to call callback function in ISR.
> >>>>>>>> If we defer callback to workqueue, the time interval may be larger than
> >>>>>>>> 32 ms.sometimes.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I think the problem is, that you implemented the workqueue as a ordered
> >>>>>>> workqueue, so there is no parallel processing. I'm still not sure why
> >>>>>>> you need the workqueue to be ordered. Can you please explain.
> >>>>>>
> >>>>>> The order should be kept.
> >>>>>> Let me use mouse cursor as an example.
> >>>>>> If task 1 means move mouse cursor to point A, task 2 means point B,
> >>>>>> and task 3 means point C, our expected result is A -> B -> C.
> >>>>>> If the order is not kept, the result could become A -> C -> B.
> >>>>>>
> >>>>>
> >>>>> Got it, thanks for the clarification.
> >>>>>
> >>>>
> >>>> I think a way to get rid of the workqueue is to use a timer, which gets
> >>>> programmed to the time a timeout in the first task in the busy list
> >>>> would happen. Everytime we update the busy list (e.g. because of task
> >>>> got finished by the thread), we update the timer. When the timer
> >>>> triggers, which hopefully won't happen too often, we return timeout on
> >>>> the busy list elements, until the time is lower then the actual time.
> >>>>
> >>>> At least with this we can reduce the data structures in this driver and
> >>>> make it more lightweight.
> >>>
> >>> From my understanding, your proposed method can handle timeout case.
> >>>
> >>> However, the workqueue is also in charge of releasing tasks.
> >>> Do you take releasing tasks into consideration by using the proposed
> >>> timer method?
> >>> Furthermore, I think the code will become more complex if we also use
> >>> timer to implement releasing tasks.
> >>>
> >>
> >> Can't we call
> >> clk_disable_unprepare(cmdq->clock);
> >> cmdq_task_release(task);
> >> after invoking the callback?
> >
> > Do you mean just call these two functions in ISR?
> > My major concern is dma_free_coherent() and kfree() in
> > cmdq_task_release(task).
>
> Why do we need the dma calls at all? Can't we just calculate the
> physical address using __pa(x)?
I prefer to use dma_map_single/dma_unmap_single.
> > Therefore, your suggestion is to use GFP_ATOMIC for both
> > dma_alloc_coherent() and kzalloc(). Right?
>
> I don't think we need GFP_ATOMIC, the critical path will just free the
> memory.
I tested these two functions, and kfree was safe.
However, dma_free_coherent raised BUG.
BUG: failure at
/mnt/host/source/src/third_party/kernel/v3.18/mm/vmalloc.c:1514/vunmap()!
1512 void vunmap(const void *addr)
1513 {
1514 BUG_ON(in_interrupt()); // <-- here
1515 might_sleep();
1516 if (addr)
1517 __vunmap(addr, 0);
1518 }
1519 EXPORT_SYMBOL(vunmap);
Therefore, I plan to use kmalloc + dma_map_single instead of
dma_alloc_coherent, and dma_unmap_single + kfree instead of
dma_free_coherent.
What do you think about the function replacement?
> > If so, I can try to implement timeout by timer, and discuss with you
> > if I have further questions.
> >
>
> Sounds good :)
>
> Thanks,
> Matthias
Thanks,
HS
> >> Regrading the clock, wouldn't it be easier to handle the clock
> >> enable/disable depending on the state of task_busy_list? I suppose we
> >> can't as we would need to check the task_busy_list of all threads, right?
> >>
> >> Regards,
> >> Matthias
> >
> > Thanks,
> > HS
> >
next prev parent reply other threads:[~2016-06-14 7:44 UTC|newest]
Thread overview: 125+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-30 3:19 [PATCH v8 0/3] Mediatek MT8173 CMDQ support HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` [PATCH v8 1/3] dt-bindings: soc: Add documentation for the MediaTek GCE unit HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 6:49 ` CK Hu
2016-05-30 6:49 ` CK Hu
2016-05-30 6:49 ` CK Hu
2016-05-30 9:38 ` Horng-Shyang Liao
2016-05-30 9:38 ` Horng-Shyang Liao
2016-05-30 9:38 ` Horng-Shyang Liao
2016-05-30 15:31 ` Matthias Brugger
2016-05-30 15:31 ` Matthias Brugger
2016-05-31 8:36 ` Horng-Shyang Liao
2016-05-31 8:36 ` Horng-Shyang Liao
2016-05-31 8:36 ` Horng-Shyang Liao
2016-05-31 20:04 ` Matthias Brugger
2016-05-31 20:04 ` Matthias Brugger
2016-06-01 9:57 ` Horng-Shyang Liao
2016-06-01 9:57 ` Horng-Shyang Liao
2016-06-01 9:57 ` Horng-Shyang Liao
2016-06-02 8:46 ` Matthias Brugger
2016-06-02 8:46 ` Matthias Brugger
2016-06-02 8:46 ` Matthias Brugger
2016-06-03 6:12 ` Horng-Shyang Liao
2016-06-03 6:12 ` Horng-Shyang Liao
2016-06-03 6:12 ` Horng-Shyang Liao
2016-06-03 11:18 ` Matthias Brugger
2016-06-03 11:18 ` Matthias Brugger
2016-06-03 12:13 ` Horng-Shyang Liao
2016-06-03 12:13 ` Horng-Shyang Liao
2016-06-03 12:13 ` Horng-Shyang Liao
2016-06-03 13:11 ` Matthias Brugger
2016-06-03 13:11 ` Matthias Brugger
2016-06-03 13:11 ` Matthias Brugger
2016-06-07 16:59 ` Matthias Brugger
2016-06-07 16:59 ` Matthias Brugger
2016-06-08 5:40 ` Horng-Shyang Liao
2016-06-08 5:40 ` Horng-Shyang Liao
2016-06-08 5:40 ` Horng-Shyang Liao
2016-06-08 10:45 ` Matthias Brugger
2016-06-08 10:45 ` Matthias Brugger
2016-06-08 10:45 ` Matthias Brugger
2016-06-08 12:25 ` Horng-Shyang Liao
2016-06-08 12:25 ` Horng-Shyang Liao
2016-06-08 12:25 ` Horng-Shyang Liao
2016-06-08 15:35 ` Matthias Brugger
2016-06-08 15:35 ` Matthias Brugger
2016-06-14 7:44 ` Horng-Shyang Liao [this message]
2016-06-14 7:44 ` Horng-Shyang Liao
2016-06-14 7:44 ` Horng-Shyang Liao
2016-06-14 10:17 ` Matthias Brugger
2016-06-14 10:17 ` Matthias Brugger
2016-06-14 12:07 ` Horng-Shyang Liao
2016-06-14 12:07 ` Horng-Shyang Liao
2016-06-14 12:07 ` Horng-Shyang Liao
2016-06-17 8:28 ` Horng-Shyang Liao
2016-06-17 8:28 ` Horng-Shyang Liao
2016-06-17 8:28 ` Horng-Shyang Liao
2016-06-17 15:57 ` Matthias Brugger
2016-06-17 15:57 ` Matthias Brugger
2016-06-17 15:57 ` Matthias Brugger
2016-06-21 5:52 ` Horng-Shyang Liao
2016-06-21 5:52 ` Horng-Shyang Liao
2016-06-21 5:52 ` Horng-Shyang Liao
2016-06-21 13:41 ` Matthias Brugger
2016-06-21 13:41 ` Matthias Brugger
2016-06-21 13:41 ` Matthias Brugger
2016-06-22 5:43 ` Horng-Shyang Liao
2016-06-22 5:43 ` Horng-Shyang Liao
2016-06-22 5:43 ` Horng-Shyang Liao
2016-06-22 9:58 ` Matthias Brugger
2016-06-22 9:58 ` Matthias Brugger
2016-06-22 9:58 ` Matthias Brugger
2016-06-17 16:14 ` Matthias Brugger
2016-06-17 16:14 ` Matthias Brugger
2016-06-03 13:11 ` Jassi Brar
2016-06-03 13:11 ` Jassi Brar
2016-06-03 13:11 ` Jassi Brar
2016-06-06 9:33 ` Horng-Shyang Liao
2016-06-06 9:33 ` Horng-Shyang Liao
2016-06-06 9:33 ` Horng-Shyang Liao
2016-06-06 13:17 ` Jassi Brar
2016-06-07 2:45 ` Horng-Shyang Liao
2016-06-07 2:45 ` Horng-Shyang Liao
2016-06-07 2:45 ` Horng-Shyang Liao
2016-06-07 17:04 ` Matthias Brugger
2016-06-07 17:04 ` Matthias Brugger
2016-06-08 5:09 ` Horng-Shyang Liao
2016-06-08 5:09 ` Horng-Shyang Liao
2016-06-08 5:09 ` Horng-Shyang Liao
2016-06-20 10:41 ` CK Hu
2016-06-20 10:41 ` CK Hu
2016-06-20 10:41 ` CK Hu
2016-06-20 11:22 ` Horng-Shyang Liao
2016-06-20 11:22 ` Horng-Shyang Liao
2016-06-20 11:22 ` Horng-Shyang Liao
2016-06-21 2:03 ` CK Hu
2016-06-21 2:03 ` CK Hu
2016-06-21 2:03 ` CK Hu
2016-06-21 7:46 ` Horng-Shyang Liao
2016-06-21 7:46 ` Horng-Shyang Liao
2016-06-21 7:46 ` Horng-Shyang Liao
2016-06-24 11:39 ` Horng-Shyang Liao
2016-06-24 11:39 ` Horng-Shyang Liao
2016-06-24 11:39 ` Horng-Shyang Liao
2016-06-27 2:00 ` CK Hu
2016-06-27 2:00 ` CK Hu
2016-06-27 2:00 ` CK Hu
2016-06-23 6:03 ` CK Hu
2016-06-23 6:03 ` CK Hu
2016-06-23 6:03 ` CK Hu
2016-06-23 7:54 ` Horng-Shyang Liao
2016-06-23 7:54 ` Horng-Shyang Liao
2016-06-23 7:54 ` Horng-Shyang Liao
2016-06-23 11:44 ` CK Hu
2016-06-23 11:44 ` CK Hu
2016-06-23 11:44 ` CK Hu
2016-05-30 3:19 ` [PATCH v8 3/3] arm64: dts: mt8173: Add GCE node HS Liao
2016-05-30 3:19 ` HS Liao
2016-05-30 3:19 ` HS Liao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1465890268.7191.13.camel@mtksdaap41 \
--to=hs.liao@mediatek.com \
--cc=bibby.hsieh@mediatek.com \
--cc=cawa.cheng@mediatek.com \
--cc=ck.hu@mediatek.com \
--cc=damon.chu@mediatek.com \
--cc=daoyuan.huang@mediatek.com \
--cc=dennis-yc.hsieh@mediatek.com \
--cc=devicetree@vger.kernel.org \
--cc=djkurtz@chromium.org \
--cc=drinkcat@chromium.org \
--cc=glory.hung@mediatek.com \
--cc=jassisinghbrar@gmail.com \
--cc=jaswinder.singh@linaro.org \
--cc=jiaguang.zhang@mediatek.com \
--cc=josh-yc.liu@mediatek.com \
--cc=kernel@pengutronix.de \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=monica.wang@mediatek.com \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=srv_heupstream@mediatek.com \
--cc=yt.shen@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.