* [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function
@ 2020-09-20 23:42 Chun-Kuang Hu
2020-09-21 8:52 ` Matthias Brugger
0 siblings, 1 reply; 4+ messages in thread
From: Chun-Kuang Hu @ 2020-09-20 23:42 UTC (permalink / raw)
To: Matthias Brugger; +Cc: linux-kernel, dri-devel, linux-mediatek, Chun-Kuang Hu
For each client driver, its timeout handler need to dump hardware register
or its state machine information, so remove timeout handler in helper
function and let client driver implement its own timeout handler.
Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 3 +-
drivers/soc/mediatek/mtk-cmdq-helper.c | 41 +------------------------
include/linux/soc/mediatek/mtk-cmdq.h | 11 +------
3 files changed, 3 insertions(+), 52 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 3fc5511330b9..cabeb7fea2be 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -824,8 +824,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
#if IS_REACHABLE(CONFIG_MTK_CMDQ)
mtk_crtc->cmdq_client =
cmdq_mbox_create(mtk_crtc->mmsys_dev,
- drm_crtc_index(&mtk_crtc->base),
- 2000);
+ drm_crtc_index(&mtk_crtc->base));
if (IS_ERR(mtk_crtc->cmdq_client)) {
dev_dbg(dev, "mtk_crtc %d failed to create mailbox client, writing register by CPU now\n",
drm_crtc_index(&mtk_crtc->base));
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index dc644cfb6419..4f311f035b59 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -65,14 +65,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
}
EXPORT_SYMBOL(cmdq_dev_get_client_reg);
-static void cmdq_client_timeout(struct timer_list *t)
-{
- struct cmdq_client *client = from_timer(client, t, timer);
-
- dev_err(client->client.dev, "cmdq timeout!\n");
-}
-
-struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
+struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
{
struct cmdq_client *client;
@@ -80,12 +73,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
if (!client)
return (struct cmdq_client *)-ENOMEM;
- client->timeout_ms = timeout;
- if (timeout != CMDQ_NO_TIMEOUT) {
- spin_lock_init(&client->lock);
- timer_setup(&client->timer, cmdq_client_timeout, 0);
- }
- client->pkt_cnt = 0;
client->client.dev = dev;
client->client.tx_block = false;
client->client.knows_txdone = true;
@@ -107,11 +94,6 @@ EXPORT_SYMBOL(cmdq_mbox_create);
void cmdq_mbox_destroy(struct cmdq_client *client)
{
- if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
- spin_lock(&client->lock);
- del_timer_sync(&client->timer);
- spin_unlock(&client->lock);
- }
mbox_free_channel(client->chan);
kfree(client);
}
@@ -342,18 +324,6 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
struct cmdq_task_cb *cb = &pkt->cb;
struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
- if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
- unsigned long flags = 0;
-
- spin_lock_irqsave(&client->lock, flags);
- if (--client->pkt_cnt == 0)
- del_timer(&client->timer);
- else
- mod_timer(&client->timer, jiffies +
- msecs_to_jiffies(client->timeout_ms));
- spin_unlock_irqrestore(&client->lock, flags);
- }
-
dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
pkt->cmd_buf_size, DMA_TO_DEVICE);
if (cb->cb) {
@@ -366,7 +336,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
void *data)
{
int err;
- unsigned long flags = 0;
struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
pkt->cb.cb = cb;
@@ -377,14 +346,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
pkt->cmd_buf_size, DMA_TO_DEVICE);
- 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));
- spin_unlock_irqrestore(&client->lock, flags);
- }
-
err = mbox_send_message(client->chan, pkt);
if (err < 0)
return err;
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 2249ecaf77e4..175bd89f46f8 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -11,8 +11,6 @@
#include <linux/mailbox/mtk-cmdq-mailbox.h>
#include <linux/timer.h>
-#define CMDQ_NO_TIMEOUT 0xffffffffu
-
struct cmdq_pkt;
struct cmdq_client_reg {
@@ -22,12 +20,8 @@ struct cmdq_client_reg {
};
struct cmdq_client {
- spinlock_t lock;
- u32 pkt_cnt;
struct mbox_client client;
struct mbox_chan *chan;
- struct timer_list timer;
- u32 timeout_ms; /* in unit of microsecond */
};
/**
@@ -49,13 +43,10 @@ int cmdq_dev_get_client_reg(struct device *dev,
* cmdq_mbox_create() - create CMDQ mailbox client and channel
* @dev: device of CMDQ mailbox client
* @index: index of CMDQ mailbox channel
- * @timeout: timeout of a pkt execution by GCE, in unit of microsecond, set
- * CMDQ_NO_TIMEOUT if a timer is not used.
*
* Return: CMDQ mailbox client pointer
*/
-struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
- u32 timeout);
+struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
/**
* cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function
2020-09-20 23:42 [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function Chun-Kuang Hu
@ 2020-09-21 8:52 ` Matthias Brugger
2020-09-21 15:32 ` Chun-Kuang Hu
0 siblings, 1 reply; 4+ messages in thread
From: Matthias Brugger @ 2020-09-21 8:52 UTC (permalink / raw)
To: Chun-Kuang Hu; +Cc: linux-kernel, dri-devel, linux-mediatek
On 21/09/2020 01:42, Chun-Kuang Hu wrote:
> For each client driver, its timeout handler need to dump hardware register
> or its state machine information, so remove timeout handler in helper
> function and let client driver implement its own timeout handler.
>
I don't see the implementation of a client side handler. Did I miss something?
Would it make sense to instead add a callback to the handler in cmdq_mbox_create()?
Regards,
Matthias
> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 3 +-
> drivers/soc/mediatek/mtk-cmdq-helper.c | 41 +------------------------
> include/linux/soc/mediatek/mtk-cmdq.h | 11 +------
> 3 files changed, 3 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 3fc5511330b9..cabeb7fea2be 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -824,8 +824,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
> #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> mtk_crtc->cmdq_client =
> cmdq_mbox_create(mtk_crtc->mmsys_dev,
> - drm_crtc_index(&mtk_crtc->base),
> - 2000);
> + drm_crtc_index(&mtk_crtc->base));
> if (IS_ERR(mtk_crtc->cmdq_client)) {
> dev_dbg(dev, "mtk_crtc %d failed to create mailbox client, writing register by CPU now\n",
> drm_crtc_index(&mtk_crtc->base));
> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> index dc644cfb6419..4f311f035b59 100644
> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> @@ -65,14 +65,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
> }
> EXPORT_SYMBOL(cmdq_dev_get_client_reg);
>
> -static void cmdq_client_timeout(struct timer_list *t)
> -{
> - struct cmdq_client *client = from_timer(client, t, timer);
> -
> - dev_err(client->client.dev, "cmdq timeout!\n");
> -}
> -
> -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> {
> struct cmdq_client *client;
>
> @@ -80,12 +73,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> if (!client)
> return (struct cmdq_client *)-ENOMEM;
>
> - client->timeout_ms = timeout;
> - if (timeout != CMDQ_NO_TIMEOUT) {
> - spin_lock_init(&client->lock);
> - timer_setup(&client->timer, cmdq_client_timeout, 0);
> - }
> - client->pkt_cnt = 0;
> client->client.dev = dev;
> client->client.tx_block = false;
> client->client.knows_txdone = true;
> @@ -107,11 +94,6 @@ EXPORT_SYMBOL(cmdq_mbox_create);
>
> void cmdq_mbox_destroy(struct cmdq_client *client)
> {
> - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> - spin_lock(&client->lock);
> - del_timer_sync(&client->timer);
> - spin_unlock(&client->lock);
> - }
> mbox_free_channel(client->chan);
> kfree(client);
> }
> @@ -342,18 +324,6 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> struct cmdq_task_cb *cb = &pkt->cb;
> struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>
> - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> - unsigned long flags = 0;
> -
> - spin_lock_irqsave(&client->lock, flags);
> - if (--client->pkt_cnt == 0)
> - del_timer(&client->timer);
> - else
> - mod_timer(&client->timer, jiffies +
> - msecs_to_jiffies(client->timeout_ms));
> - spin_unlock_irqrestore(&client->lock, flags);
> - }
> -
> dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
> pkt->cmd_buf_size, DMA_TO_DEVICE);
> if (cb->cb) {
> @@ -366,7 +336,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> void *data)
> {
> int err;
> - unsigned long flags = 0;
> struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>
> pkt->cb.cb = cb;
> @@ -377,14 +346,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
> pkt->cmd_buf_size, DMA_TO_DEVICE);
>
> - 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));
> - spin_unlock_irqrestore(&client->lock, flags);
> - }
> -
> err = mbox_send_message(client->chan, pkt);
> if (err < 0)
> return err;
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> index 2249ecaf77e4..175bd89f46f8 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -11,8 +11,6 @@
> #include <linux/mailbox/mtk-cmdq-mailbox.h>
> #include <linux/timer.h>
>
> -#define CMDQ_NO_TIMEOUT 0xffffffffu
> -
> struct cmdq_pkt;
>
> struct cmdq_client_reg {
> @@ -22,12 +20,8 @@ struct cmdq_client_reg {
> };
>
> struct cmdq_client {
> - spinlock_t lock;
> - u32 pkt_cnt;
> struct mbox_client client;
> struct mbox_chan *chan;
> - struct timer_list timer;
> - u32 timeout_ms; /* in unit of microsecond */
> };
>
> /**
> @@ -49,13 +43,10 @@ int cmdq_dev_get_client_reg(struct device *dev,
> * cmdq_mbox_create() - create CMDQ mailbox client and channel
> * @dev: device of CMDQ mailbox client
> * @index: index of CMDQ mailbox channel
> - * @timeout: timeout of a pkt execution by GCE, in unit of microsecond, set
> - * CMDQ_NO_TIMEOUT if a timer is not used.
> *
> * Return: CMDQ mailbox client pointer
> */
> -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
> - u32 timeout);
> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
>
> /**
> * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function
2020-09-21 8:52 ` Matthias Brugger
@ 2020-09-21 15:32 ` Chun-Kuang Hu
2020-09-21 15:57 ` Matthias Brugger
0 siblings, 1 reply; 4+ messages in thread
From: Chun-Kuang Hu @ 2020-09-21 15:32 UTC (permalink / raw)
To: Matthias Brugger
Cc: Chun-Kuang Hu, linux-kernel, DRI Development,
moderated list:ARM/Mediatek SoC support
Hi, Matthias:
Matthias Brugger <matthias.bgg@gmail.com> 於 2020年9月21日 週一 下午4:53寫道：
>
>
>
> On 21/09/2020 01:42, Chun-Kuang Hu wrote:
> > For each client driver, its timeout handler need to dump hardware register
> > or its state machine information, so remove timeout handler in helper
> > function and let client driver implement its own timeout handler.
> >
>
> I don't see the implementation of a client side handler. Did I miss something?
> Would it make sense to instead add a callback to the handler in cmdq_mbox_create()?
According to the commit message, it make sense to add a callback to
the handler in comq_mbox_create().
But for DRM, I would like to check timeout in vblank irq because the
register should be applied in vblank. (I have not implement this patch
yet)
What I want to say is that different client may have different way to
detect timeout and different way to handle it.
If you want, I would add DRM timeout handle patch with this patch, and
modify commit message to include different way to detect timeout.
Regards,
Chun-Kuang.
>
> Regards,
> Matthias
>
> > Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 3 +-
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 41 +------------------------
> > include/linux/soc/mediatek/mtk-cmdq.h | 11 +------
> > 3 files changed, 3 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 3fc5511330b9..cabeb7fea2be 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -824,8 +824,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
> > #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > mtk_crtc->cmdq_client =
> > cmdq_mbox_create(mtk_crtc->mmsys_dev,
> > - drm_crtc_index(&mtk_crtc->base),
> > - 2000);
> > + drm_crtc_index(&mtk_crtc->base));
> > if (IS_ERR(mtk_crtc->cmdq_client)) {
> > dev_dbg(dev, "mtk_crtc %d failed to create mailbox client, writing register by CPU now\n",
> > drm_crtc_index(&mtk_crtc->base));
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index dc644cfb6419..4f311f035b59 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -65,14 +65,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
> > }
> > EXPORT_SYMBOL(cmdq_dev_get_client_reg);
> >
> > -static void cmdq_client_timeout(struct timer_list *t)
> > -{
> > - struct cmdq_client *client = from_timer(client, t, timer);
> > -
> > - dev_err(client->client.dev, "cmdq timeout!\n");
> > -}
> > -
> > -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
> > {
> > struct cmdq_client *client;
> >
> > @@ -80,12 +73,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
> > if (!client)
> > return (struct cmdq_client *)-ENOMEM;
> >
> > - client->timeout_ms = timeout;
> > - if (timeout != CMDQ_NO_TIMEOUT) {
> > - spin_lock_init(&client->lock);
> > - timer_setup(&client->timer, cmdq_client_timeout, 0);
> > - }
> > - client->pkt_cnt = 0;
> > client->client.dev = dev;
> > client->client.tx_block = false;
> > client->client.knows_txdone = true;
> > @@ -107,11 +94,6 @@ EXPORT_SYMBOL(cmdq_mbox_create);
> >
> > void cmdq_mbox_destroy(struct cmdq_client *client)
> > {
> > - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> > - spin_lock(&client->lock);
> > - del_timer_sync(&client->timer);
> > - spin_unlock(&client->lock);
> > - }
> > mbox_free_channel(client->chan);
> > kfree(client);
> > }
> > @@ -342,18 +324,6 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> > struct cmdq_task_cb *cb = &pkt->cb;
> > struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >
> > - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
> > - unsigned long flags = 0;
> > -
> > - spin_lock_irqsave(&client->lock, flags);
> > - if (--client->pkt_cnt == 0)
> > - del_timer(&client->timer);
> > - else
> > - mod_timer(&client->timer, jiffies +
> > - msecs_to_jiffies(client->timeout_ms));
> > - spin_unlock_irqrestore(&client->lock, flags);
> > - }
> > -
> > dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
> > pkt->cmd_buf_size, DMA_TO_DEVICE);
> > if (cb->cb) {
> > @@ -366,7 +336,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> > void *data)
> > {
> > int err;
> > - unsigned long flags = 0;
> > struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >
> > pkt->cb.cb = cb;
> > @@ -377,14 +346,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> > dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
> > pkt->cmd_buf_size, DMA_TO_DEVICE);
> >
> > - 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));
> > - spin_unlock_irqrestore(&client->lock, flags);
> > - }
> > -
> > err = mbox_send_message(client->chan, pkt);
> > if (err < 0)
> > return err;
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 2249ecaf77e4..175bd89f46f8 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -11,8 +11,6 @@
> > #include <linux/mailbox/mtk-cmdq-mailbox.h>
> > #include <linux/timer.h>
> >
> > -#define CMDQ_NO_TIMEOUT 0xffffffffu
> > -
> > struct cmdq_pkt;
> >
> > struct cmdq_client_reg {
> > @@ -22,12 +20,8 @@ struct cmdq_client_reg {
> > };
> >
> > struct cmdq_client {
> > - spinlock_t lock;
> > - u32 pkt_cnt;
> > struct mbox_client client;
> > struct mbox_chan *chan;
> > - struct timer_list timer;
> > - u32 timeout_ms; /* in unit of microsecond */
> > };
> >
> > /**
> > @@ -49,13 +43,10 @@ int cmdq_dev_get_client_reg(struct device *dev,
> > * cmdq_mbox_create() - create CMDQ mailbox client and channel
> > * @dev: device of CMDQ mailbox client
> > * @index: index of CMDQ mailbox channel
> > - * @timeout: timeout of a pkt execution by GCE, in unit of microsecond, set
> > - * CMDQ_NO_TIMEOUT if a timer is not used.
> > *
> > * Return: CMDQ mailbox client pointer
> > */
> > -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
> > - u32 timeout);
> > +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
> >
> > /**
> > * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function
2020-09-21 15:32 ` Chun-Kuang Hu
@ 2020-09-21 15:57 ` Matthias Brugger
0 siblings, 0 replies; 4+ messages in thread
From: Matthias Brugger @ 2020-09-21 15:57 UTC (permalink / raw)
To: Chun-Kuang Hu
Cc: linux-kernel, DRI Development, moderated list:ARM/Mediatek SoC support
On 21/09/2020 17:32, Chun-Kuang Hu wrote:
> Hi, Matthias:
>
> Matthias Brugger <matthias.bgg@gmail.com> 於 2020年9月21日 週一 下午4:53寫道：
>>
>>
>>
>> On 21/09/2020 01:42, Chun-Kuang Hu wrote:
>>> For each client driver, its timeout handler need to dump hardware register
>>> or its state machine information, so remove timeout handler in helper
>>> function and let client driver implement its own timeout handler.
>>>
>>
>> I don't see the implementation of a client side handler. Did I miss something?
>> Would it make sense to instead add a callback to the handler in cmdq_mbox_create()?
>
> According to the commit message, it make sense to add a callback to
> the handler in comq_mbox_create().
> But for DRM, I would like to check timeout in vblank irq because the
> register should be applied in vblank. (I have not implement this patch
> yet)
> What I want to say is that different client may have different way to
> detect timeout and different way to handle it.
> If you want, I would add DRM timeout handle patch with this patch, and
> modify commit message to include different way to detect timeout.
>
I think that would help me to understand the whole picture.
Thanks a lot!
Matthias
> Regards,
> Chun-Kuang.
>
>>
>> Regards,
>> Matthias
>>
>>> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
>>> ---
>>> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 3 +-
>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 41 +------------------------
>>> include/linux/soc/mediatek/mtk-cmdq.h | 11 +------
>>> 3 files changed, 3 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> index 3fc5511330b9..cabeb7fea2be 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>> @@ -824,8 +824,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
>>> #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>> mtk_crtc->cmdq_client =
>>> cmdq_mbox_create(mtk_crtc->mmsys_dev,
>>> - drm_crtc_index(&mtk_crtc->base),
>>> - 2000);
>>> + drm_crtc_index(&mtk_crtc->base));
>>> if (IS_ERR(mtk_crtc->cmdq_client)) {
>>> dev_dbg(dev, "mtk_crtc %d failed to create mailbox client, writing register by CPU now\n",
>>> drm_crtc_index(&mtk_crtc->base));
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index dc644cfb6419..4f311f035b59 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -65,14 +65,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
>>> }
>>> EXPORT_SYMBOL(cmdq_dev_get_client_reg);
>>>
>>> -static void cmdq_client_timeout(struct timer_list *t)
>>> -{
>>> - struct cmdq_client *client = from_timer(client, t, timer);
>>> -
>>> - dev_err(client->client.dev, "cmdq timeout!\n");
>>> -}
>>> -
>>> -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
>>> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
>>> {
>>> struct cmdq_client *client;
>>>
>>> @@ -80,12 +73,6 @@ struct cmdq_client *cmdq_mbox_create(struct device *dev, int index, u32 timeout)
>>> if (!client)
>>> return (struct cmdq_client *)-ENOMEM;
>>>
>>> - client->timeout_ms = timeout;
>>> - if (timeout != CMDQ_NO_TIMEOUT) {
>>> - spin_lock_init(&client->lock);
>>> - timer_setup(&client->timer, cmdq_client_timeout, 0);
>>> - }
>>> - client->pkt_cnt = 0;
>>> client->client.dev = dev;
>>> client->client.tx_block = false;
>>> client->client.knows_txdone = true;
>>> @@ -107,11 +94,6 @@ EXPORT_SYMBOL(cmdq_mbox_create);
>>>
>>> void cmdq_mbox_destroy(struct cmdq_client *client)
>>> {
>>> - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
>>> - spin_lock(&client->lock);
>>> - del_timer_sync(&client->timer);
>>> - spin_unlock(&client->lock);
>>> - }
>>> mbox_free_channel(client->chan);
>>> kfree(client);
>>> }
>>> @@ -342,18 +324,6 @@ static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
>>> struct cmdq_task_cb *cb = &pkt->cb;
>>> struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>>>
>>> - if (client->timeout_ms != CMDQ_NO_TIMEOUT) {
>>> - unsigned long flags = 0;
>>> -
>>> - spin_lock_irqsave(&client->lock, flags);
>>> - if (--client->pkt_cnt == 0)
>>> - del_timer(&client->timer);
>>> - else
>>> - mod_timer(&client->timer, jiffies +
>>> - msecs_to_jiffies(client->timeout_ms));
>>> - spin_unlock_irqrestore(&client->lock, flags);
>>> - }
>>> -
>>> dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
>>> pkt->cmd_buf_size, DMA_TO_DEVICE);
>>> if (cb->cb) {
>>> @@ -366,7 +336,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>>> void *data)
>>> {
>>> int err;
>>> - unsigned long flags = 0;
>>> struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
>>>
>>> pkt->cb.cb = cb;
>>> @@ -377,14 +346,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
>>> dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
>>> pkt->cmd_buf_size, DMA_TO_DEVICE);
>>>
>>> - 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));
>>> - spin_unlock_irqrestore(&client->lock, flags);
>>> - }
>>> -
>>> err = mbox_send_message(client->chan, pkt);
>>> if (err < 0)
>>> return err;
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 2249ecaf77e4..175bd89f46f8 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -11,8 +11,6 @@
>>> #include <linux/mailbox/mtk-cmdq-mailbox.h>
>>> #include <linux/timer.h>
>>>
>>> -#define CMDQ_NO_TIMEOUT 0xffffffffu
>>> -
>>> struct cmdq_pkt;
>>>
>>> struct cmdq_client_reg {
>>> @@ -22,12 +20,8 @@ struct cmdq_client_reg {
>>> };
>>>
>>> struct cmdq_client {
>>> - spinlock_t lock;
>>> - u32 pkt_cnt;
>>> struct mbox_client client;
>>> struct mbox_chan *chan;
>>> - struct timer_list timer;
>>> - u32 timeout_ms; /* in unit of microsecond */
>>> };
>>>
>>> /**
>>> @@ -49,13 +43,10 @@ int cmdq_dev_get_client_reg(struct device *dev,
>>> * cmdq_mbox_create() - create CMDQ mailbox client and channel
>>> * @dev: device of CMDQ mailbox client
>>> * @index: index of CMDQ mailbox channel
>>> - * @timeout: timeout of a pkt execution by GCE, in unit of microsecond, set
>>> - * CMDQ_NO_TIMEOUT if a timer is not used.
>>> *
>>> * Return: CMDQ mailbox client pointer
>>> */
>>> -struct cmdq_client *cmdq_mbox_create(struct device *dev, int index,
>>> - u32 timeout);
>>> +struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
>>>
>>> /**
>>> * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
>>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-21 15:57 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-20 23:42 [PATCH] soc: mediatek: cmdq: Remove timeout handler in helper function Chun-Kuang Hu
2020-09-21 8:52 ` Matthias Brugger
2020-09-21 15:32 ` Chun-Kuang Hu
2020-09-21 15:57 ` Matthias Brugger
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).