From: Matthias Brugger <matthias.bgg@gmail.com>
To: Horng-Shyang Liao <hs.liao@mediatek.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
Subject: Re: [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver
Date: Wed, 8 Jun 2016 17:35:33 +0200 [thread overview]
Message-ID: <57583B45.2080504@gmail.com> (raw)
In-Reply-To: <1465388727.21326.8.camel@mtksdaap41>
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)?
> 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.
> If so, I can try to implement timeout by timer, and discuss with you
> if I have further questions.
>
Sounds good :)
Thanks,
Matthias
>> 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-08 15:35 UTC|newest]
Thread overview: 44+ 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 ` [PATCH v8 1/3] dt-bindings: soc: Add documentation for the MediaTek GCE unit HS Liao
2016-05-30 3:19 ` [PATCH v8 2/3] CMDQ: Mediatek CMDQ driver HS Liao
2016-05-30 6:49 ` CK Hu
2016-05-30 9:38 ` Horng-Shyang Liao
2016-05-30 15:31 ` Matthias Brugger
2016-05-31 8:36 ` Horng-Shyang Liao
2016-05-31 20:04 ` Matthias Brugger
2016-06-01 9:57 ` Horng-Shyang Liao
2016-06-02 8:46 ` Matthias Brugger
2016-06-03 6:12 ` Horng-Shyang Liao
2016-06-03 11:18 ` Matthias Brugger
2016-06-03 12:13 ` Horng-Shyang Liao
2016-06-03 13:11 ` Matthias Brugger
2016-06-07 16:59 ` Matthias Brugger
2016-06-08 5:40 ` Horng-Shyang Liao
2016-06-08 10:45 ` Matthias Brugger
2016-06-08 12:25 ` Horng-Shyang Liao
2016-06-08 15:35 ` Matthias Brugger [this message]
2016-06-14 7:44 ` Horng-Shyang Liao
2016-06-14 10:17 ` Matthias Brugger
2016-06-14 12:07 ` Horng-Shyang Liao
2016-06-17 8:28 ` Horng-Shyang Liao
2016-06-17 15:57 ` Matthias Brugger
2016-06-21 5:52 ` Horng-Shyang Liao
2016-06-21 13:41 ` Matthias Brugger
2016-06-22 5:43 ` Horng-Shyang Liao
2016-06-22 9:58 ` Matthias Brugger
2016-06-17 16:14 ` Matthias Brugger
2016-06-03 13:11 ` Jassi Brar
2016-06-06 9:33 ` Horng-Shyang Liao
2016-06-07 2:45 ` Horng-Shyang Liao
2016-06-07 17:04 ` Matthias Brugger
2016-06-08 5:09 ` Horng-Shyang Liao
2016-06-20 10:41 ` CK Hu
2016-06-20 11:22 ` Horng-Shyang Liao
2016-06-21 2:03 ` CK Hu
2016-06-21 7:46 ` Horng-Shyang Liao
2016-06-24 11:39 ` Horng-Shyang Liao
2016-06-27 2:00 ` CK Hu
2016-06-23 6:03 ` CK Hu
2016-06-23 7:54 ` Horng-Shyang Liao
2016-06-23 11:44 ` CK Hu
2016-05-30 3:19 ` [PATCH v8 3/3] arm64: dts: mt8173: Add GCE node 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=57583B45.2080504@gmail.com \
--to=matthias.bgg@gmail.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=hs.liao@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=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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).