* [PATCH] soc: mediatek: cmdq: fixup possible timeout issue
@ 2020-10-22 9:41 Houlong Wei
2020-10-31 1:49 ` Nicolas Boichat
2020-11-02 0:44 ` Chun-Kuang Hu
0 siblings, 2 replies; 3+ messages in thread
From: Houlong Wei @ 2020-10-22 9:41 UTC (permalink / raw)
To: Matthias Brugger, Jassi Brar
Cc: Daniel Kurtz, linux-kernel, linux-arm-kernel, linux-mediatek,
srv_heupstream, Nicolas Boichat, CK HU, Bibby Hsieh,
Daoyuan Huang, Dennis-YC Hsieh, Houlong Wei, ginny.chen,
yongqiang.niu
Fixes: 576f1b4bc802 ("soc: mediatek: Add Mediatek CMDQ helper")
There may be possible timeout issue when lots of cmdq packets are
flushed to the same cmdq client. The necessary modifications are as
below.
1.Adjust the timer timeout period as client->timeout_ms * client->pkt_cnt.
2.Optimize the time to start the timer.
Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
---
drivers/soc/mediatek/mtk-cmdq-helper.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index dc644cfb6419..31142c193527 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -350,7 +350,8 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
del_timer(&client->timer);
else
mod_timer(&client->timer, jiffies +
- msecs_to_jiffies(client->timeout_ms));
+ msecs_to_jiffies(client->timeout_ms *
+ client->pkt_cnt));
spin_unlock_irqrestore(&client->lock, flags);
}
@@ -379,9 +380,7 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
spin_lock_irqsave(&client->lock, flags);
- if (client->pkt_cnt++ == 0)
- mod_timer(&client->timer, jiffies +
- msecs_to_jiffies(client->timeout_ms));
+ client->pkt_cnt++;
spin_unlock_irqrestore(&client->lock, flags);
}
@@ -391,6 +390,21 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
/* We can send next packet immediately, so just call txdone. */
mbox_client_txdone(client->chan, 0);
+ if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
+ spin_lock_irqsave(&client->lock, flags);
+ /*
+ * GCE HW maybe execute too quickly and the callback function
+ * may be invoked earlier. If this happens, pkt_cnt is reduced
+ * by 1 in cmdq_pkt_flush_async_cb(). The timer is set only if
+ * pkt_cnt is greater than 0.
+ */
+ if (client->pkt_cnt > 0)
+ mod_timer(&client->timer, jiffies +
+ msecs_to_jiffies(client->timeout_ms *
+ client->pkt_cnt));
+ spin_unlock_irqrestore(&client->lock, flags);
+ }
+
return 0;
}
EXPORT_SYMBOL(cmdq_pkt_flush_async);
--
2.18.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] soc: mediatek: cmdq: fixup possible timeout issue
2020-10-22 9:41 [PATCH] soc: mediatek: cmdq: fixup possible timeout issue Houlong Wei
@ 2020-10-31 1:49 ` Nicolas Boichat
2020-11-02 0:44 ` Chun-Kuang Hu
1 sibling, 0 replies; 3+ messages in thread
From: Nicolas Boichat @ 2020-10-31 1:49 UTC (permalink / raw)
To: Houlong Wei
Cc: Matthias Brugger, Jassi Brar, Daniel Kurtz, lkml,
linux-arm Mailing List, moderated list:ARM/Mediatek SoC support,
srv_heupstream, CK HU, Bibby Hsieh, Daoyuan Huang,
Dennis-YC Hsieh, ginny.chen, Yongqiang Niu, Nicolas Boichat
Thanks for the patch.
On Thu, Oct 22, 2020 at 5:44 PM Houlong Wei <houlong.wei@mediatek.com> wrote:
>
> Fixes: 576f1b4bc802 ("soc: mediatek: Add Mediatek CMDQ helper")
nit: This belongs right next to the Sob line, but I guess/hope
Matthias can help you fix that ,-)
>
> There may be possible timeout issue when lots of cmdq packets are
> flushed to the same cmdq client. The necessary modifications are as
> below.
> 1.Adjust the timer timeout period as client->timeout_ms * client->pkt_cnt.
> 2.Optimize the time to start the timer.
>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index dc644cfb6419..31142c193527 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -350,7 +350,8 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> del_timer(&client->timer);
> else
> mod_timer(&client->timer, jiffies +
> - msecs_to_jiffies(client->timeout_ms));
> + msecs_to_jiffies(client->timeout_ms *
> + client->pkt_cnt));
> spin_unlock_irqrestore(&client->lock, flags);
> }
>
> @@ -379,9 +380,7 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>
> if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> spin_lock_irqsave(&client->lock, flags);
> - if (client->pkt_cnt++ == 0)
> - mod_timer(&client->timer, jiffies +
> - msecs_to_jiffies(client->timeout_ms));
> + client->pkt_cnt++;
> spin_unlock_irqrestore(&client->lock, flags);
> }
>
> @@ -391,6 +390,21 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> /* We can send next packet immediately, so just call txdone. */
> mbox_client_txdone(client->chan, 0);
>
> + if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> + spin_lock_irqsave(&client->lock, flags);
> + /*
> + * GCE HW maybe execute too quickly and the callback function
> + * may be invoked earlier. If this happens, pkt_cnt is reduced
> + * by 1 in cmdq_pkt_flush_async_cb(). The timer is set only if
> + * pkt_cnt is greater than 0.
> + */
> + if (client->pkt_cnt > 0)
> + mod_timer(&client->timer, jiffies +
> + msecs_to_jiffies(client->timeout_ms *
> + client->pkt_cnt));
> + spin_unlock_irqrestore(&client->lock, flags);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(cmdq_pkt_flush_async);
> --
> 2.18.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] soc: mediatek: cmdq: fixup possible timeout issue
2020-10-22 9:41 [PATCH] soc: mediatek: cmdq: fixup possible timeout issue Houlong Wei
2020-10-31 1:49 ` Nicolas Boichat
@ 2020-11-02 0:44 ` Chun-Kuang Hu
1 sibling, 0 replies; 3+ messages in thread
From: Chun-Kuang Hu @ 2020-11-02 0:44 UTC (permalink / raw)
To: Houlong Wei
Cc: Matthias Brugger, Jassi Brar, Nicolas Boichat, Yongqiang Niu,
srv_heupstream, Daoyuan Huang, linux-kernel, Dennis-YC Hsieh,
moderated list:ARM/Mediatek SoC support, Bibby Hsieh, CK HU,
Linux ARM, ginny.chen
Hi, Houlong:
Houlong Wei <houlong.wei@mediatek.com> 於 2020年10月22日 週四 下午5:55寫道：
>
> Fixes: 576f1b4bc802 ("soc: mediatek: Add Mediatek CMDQ helper")
>
> There may be possible timeout issue when lots of cmdq packets are
> flushed to the same cmdq client. The necessary modifications are as
> below.
> 1.Adjust the timer timeout period as client->timeout_ms * client->pkt_cnt.
> 2.Optimize the time to start the timer.
>
> Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
> drivers/soc/mediatek/mtk-cmdq-helper.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index dc644cfb6419..31142c193527 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -350,7 +350,8 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> del_timer(&client->timer);
> else
> mod_timer(&client->timer, jiffies +
> - msecs_to_jiffies(client->timeout_ms));
> + msecs_to_jiffies(client->timeout_ms *
> + client->pkt_cnt));
> spin_unlock_irqrestore(&client->lock, flags);
> }
>
> @@ -379,9 +380,7 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>
> if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> spin_lock_irqsave(&client->lock, flags);
> - if (client->pkt_cnt++ == 0)
> - mod_timer(&client->timer, jiffies +
> - msecs_to_jiffies(client->timeout_ms));
> + client->pkt_cnt++;
> spin_unlock_irqrestore(&client->lock, flags);
> }
>
> @@ -391,6 +390,21 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> /* We can send next packet immediately, so just call txdone. */
> mbox_client_txdone(client->chan, 0);
>
> + if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> + spin_lock_irqsave(&client->lock, flags);
> + /*
> + * GCE HW maybe execute too quickly and the callback function
> + * may be invoked earlier. If this happens, pkt_cnt is reduced
> + * by 1 in cmdq_pkt_flush_async_cb(). The timer is set only if
> + * pkt_cnt is greater than 0.
> + */
> + if (client->pkt_cnt > 0)
> + mod_timer(&client->timer, jiffies +
> + msecs_to_jiffies(client->timeout_ms *
> + client->pkt_cnt));
I think for some client, it care about execution time of one packet,
but some client care about total execution time of all packets, so we
should let client driver implement its own timeout handler and remove
handler in cmdq helper [1].
[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20201102000438.29225-1-chunkuang.hu@kernel.org/
Regards,
Chun-Kuang.
> + spin_unlock_irqrestore(&client->lock, flags);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(cmdq_pkt_flush_async);
> --
> 2.18.0
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-02 0:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-22 9:41 [PATCH] soc: mediatek: cmdq: fixup possible timeout issue Houlong Wei
2020-10-31 1:49 ` Nicolas Boichat
2020-11-02 0:44 ` Chun-Kuang Hu
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).