[PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ

[PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ

[Chun-Kuang Hu]

CMDQ helper provide timer to detect execution timeout, but DRM driver
could have a better way to detect execution timeout by vblank IRQ.
For DRM, CMDQ command should execute in vblank, so if it fail to
execute in next 2 vblank, timeout happen. Even though we could
calculate time between 2 vblank and use timer to delect, this would
make things more complicated.

This introduce a series refinement for CMDQ mailbox controller and CMDQ
helper. Remove timer handler in helper function because different
client have different way to detect timeout. Use standard mailbox
callback instead of proprietary one to get the necessary data
in callback function. Remove struct cmdq_client to access client
instance data by struct mbox_client.

Chun-Kuang Hu (4):
  soc / drm: mediatek: cmdq: Remove timeout handler in helper function
  mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of
    cmdq_task_cb
  mailbox / soc / drm: mediatek: Remove struct cmdq_client
  drm/mediatek: Detect CMDQ execution timeout

 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |  54 ++++++---
 drivers/mailbox/mtk-cmdq-mailbox.c       |  24 ++--
 drivers/soc/mediatek/mtk-cmdq-helper.c   | 146 ++---------------------
 include/linux/mailbox/mtk-cmdq-mailbox.h |  25 +---
 include/linux/soc/mediatek/mtk-cmdq.h    |  54 +--------
 5 files changed, 66 insertions(+), 237 deletions(-)

-- 
2.17.1

[PATCH 1/4] soc / drm: mediatek: cmdq: Remove timeout handler in helper function

[Chun-Kuang Hu]

For each client driver, its timeout handler need to dump hardware register
or its state machine information, and their way to detect timeout are
also different, 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

[PATCH 2/4] mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of cmdq_task_cb

[Chun-Kuang Hu]

rx_callback is a standard mailbox callback mechanism and could cover the
function of proprietary cmdq_task_cb, so use the standard one instead of
the proprietary one.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 17 +++++--
 drivers/mailbox/mtk-cmdq-mailbox.c       | 24 ++++------
 drivers/soc/mediatek/mtk-cmdq-helper.c   | 57 +-----------------------
 include/linux/mailbox/mtk-cmdq-mailbox.h | 24 +++-------
 include/linux/soc/mediatek/mtk-cmdq.h    | 17 +------
 5 files changed, 30 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index cabeb7fea2be..5df2d431418d 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -4,9 +4,11 @@
  */
 
 #include <linux/clk.h>
+#include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/soc/mediatek/mtk-cmdq.h>
 #include <linux/soc/mediatek/mtk-mmsys.h>
+#include <linux/mailbox_controller.h>
 
 #include <asm/barrier.h>
 #include <soc/mediatek/smi.h>
@@ -236,9 +238,11 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 }
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-static void ddp_cmdq_cb(struct cmdq_cb_data data)
+static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
-	cmdq_pkt_destroy(data.data);
+	struct cmdq_cb_data *data = mssg;
+
+	cmdq_pkt_destroy(data->pkt);
 }
 #endif
 
@@ -484,7 +488,11 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
 		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
 		mtk_crtc_ddp_config(crtc, cmdq_handle);
 		cmdq_pkt_finalize(cmdq_handle);
-		cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
+		dma_sync_single_for_device(mtk_crtc->cmdq_client->chan->mbox->dev,
+					   cmdq_handle->pa_base,
+					   cmdq_handle->cmd_buf_size,
+					   DMA_TO_DEVICE);
+		cmdq_pkt_flush_async(cmdq_handle);
 	}
 #endif
 	mutex_unlock(&mtk_crtc->hw_lock);
@@ -837,6 +845,9 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 	if (ret)
 		dev_dbg(dev, "mtk_crtc %d failed to get mediatek,gce-events property\n",
 			drm_crtc_index(&mtk_crtc->base));
+
+	if (mtk_crtc->cmdq_client)
+		mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
 #endif
 	return 0;
 }
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
index 484d4438cd83..bc11afdebefc 100644
--- a/drivers/mailbox/mtk-cmdq-mailbox.c
+++ b/drivers/mailbox/mtk-cmdq-mailbox.c
@@ -180,15 +180,13 @@ static bool cmdq_thread_is_in_wfe(struct cmdq_thread *thread)
 	return readl(thread->base + CMDQ_THR_WAIT_TOKEN) & CMDQ_THR_IS_WAITING;
 }
 
-static void cmdq_task_exec_done(struct cmdq_task *task, enum cmdq_cb_status sta)
+static void cmdq_task_exec_done(struct cmdq_task *task, int sta)
 {
-	struct cmdq_task_cb *cb = &task->pkt->async_cb;
 	struct cmdq_cb_data data;
 
-	WARN_ON(cb->cb == (cmdq_async_flush_cb)NULL);
 	data.sta = sta;
-	data.data = cb->data;
-	cb->cb(data);
+	data.pkt = task->pkt;
+	mbox_chan_received_data(task->thread->chan, &data);
 
 	list_del(&task->list_entry);
 }
@@ -244,10 +242,10 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq,
 			curr_task = task;
 
 		if (!curr_task || curr_pa == task_end_pa - CMDQ_INST_SIZE) {
-			cmdq_task_exec_done(task, CMDQ_CB_NORMAL);
+			cmdq_task_exec_done(task, 0);
 			kfree(task);
 		} else if (err) {
-			cmdq_task_exec_done(task, CMDQ_CB_ERROR);
+			cmdq_task_exec_done(task, -ENOEXEC);
 			cmdq_task_handle_error(curr_task);
 			kfree(task);
 		}
@@ -415,7 +413,7 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan)
 
 	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
 				 list_entry) {
-		cmdq_task_exec_done(task, CMDQ_CB_ERROR);
+		cmdq_task_exec_done(task, -ECONNABORTED);
 		kfree(task);
 	}
 
@@ -434,7 +432,6 @@ static void cmdq_mbox_shutdown(struct mbox_chan *chan)
 static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout)
 {
 	struct cmdq_thread *thread = (struct cmdq_thread *)chan->con_priv;
-	struct cmdq_task_cb *cb;
 	struct cmdq_cb_data data;
 	struct cmdq *cmdq = dev_get_drvdata(chan->mbox->dev);
 	struct cmdq_task *task, *tmp;
@@ -451,12 +448,9 @@ static int cmdq_mbox_flush(struct mbox_chan *chan, unsigned long timeout)
 
 	list_for_each_entry_safe(task, tmp, &thread->task_busy_list,
 				 list_entry) {
-		cb = &task->pkt->async_cb;
-		if (cb->cb) {
-			data.sta = CMDQ_CB_ERROR;
-			data.data = cb->data;
-			cb->cb(data);
-		}
+		data.sta = -ECONNABORTED;
+		data.pkt = task->pkt;
+		mbox_chan_received_data(task->thread->chan, &data);
 		list_del(&task->list_entry);
 		kfree(task);
 	}
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index 4f311f035b59..b6ee1b525084 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -318,34 +318,11 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 }
 EXPORT_SYMBOL(cmdq_pkt_finalize);
 
-static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
-{
-	struct cmdq_pkt *pkt = (struct cmdq_pkt *)data.data;
-	struct cmdq_task_cb *cb = &pkt->cb;
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
-	dma_sync_single_for_cpu(client->chan->mbox->dev, pkt->pa_base,
-				pkt->cmd_buf_size, DMA_TO_DEVICE);
-	if (cb->cb) {
-		data.data = cb->data;
-		cb->cb(data);
-	}
-}
-
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
-			 void *data)
+int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
 {
 	int err;
 	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
 
-	pkt->cb.cb = cb;
-	pkt->cb.data = data;
-	pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
-	pkt->async_cb.data = pkt;
-
-	dma_sync_single_for_device(client->chan->mbox->dev, pkt->pa_base,
-				   pkt->cmd_buf_size, DMA_TO_DEVICE);
-
 	err = mbox_send_message(client->chan, pkt);
 	if (err < 0)
 		return err;
@@ -356,36 +333,4 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
 }
 EXPORT_SYMBOL(cmdq_pkt_flush_async);
 
-struct cmdq_flush_completion {
-	struct completion cmplt;
-	bool err;
-};
-
-static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
-{
-	struct cmdq_flush_completion *cmplt;
-
-	cmplt = (struct cmdq_flush_completion *)data.data;
-	if (data.sta != CMDQ_CB_NORMAL)
-		cmplt->err = true;
-	else
-		cmplt->err = false;
-	complete(&cmplt->cmplt);
-}
-
-int cmdq_pkt_flush(struct cmdq_pkt *pkt)
-{
-	struct cmdq_flush_completion cmplt;
-	int err;
-
-	init_completion(&cmplt.cmplt);
-	err = cmdq_pkt_flush_async(pkt, cmdq_pkt_flush_cb, &cmplt);
-	if (err < 0)
-		return err;
-	wait_for_completion(&cmplt.cmplt);
-
-	return cmplt.err ? -EFAULT : 0;
-}
-EXPORT_SYMBOL(cmdq_pkt_flush);
-
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index 05eea1aef5aa..2cec4dbaa214 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -63,33 +63,19 @@ enum cmdq_code {
 	CMDQ_CODE_LOGIC = 0xa0,
 };
 
-enum cmdq_cb_status {
-	CMDQ_CB_NORMAL = 0,
-	CMDQ_CB_ERROR
-};
-
-struct cmdq_cb_data {
-	enum cmdq_cb_status	sta;
-	void			*data;
-};
-
-typedef void (*cmdq_async_flush_cb)(struct cmdq_cb_data data);
-
-struct cmdq_task_cb {
-	cmdq_async_flush_cb	cb;
-	void			*data;
-};
-
 struct cmdq_pkt {
 	void			*va_base;
 	dma_addr_t		pa_base;
 	size_t			cmd_buf_size; /* command occupied size */
 	size_t			buf_size; /* real buffer size */
-	struct cmdq_task_cb	cb;
-	struct cmdq_task_cb	async_cb;
 	void			*cl;
 };
 
+struct cmdq_cb_data {
+	int sta;
+	struct cmdq_pkt *pkt;
+};
+
 u8 cmdq_get_shift_pa(struct mbox_chan *chan);
 
 #endif /* __MTK_CMDQ_MAILBOX_H__ */
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 175bd89f46f8..a09b3f39b861 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -178,8 +178,6 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
  * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
  *                          packet and call back at the end of done packet
  * @pkt:	the CMDQ packet
- * @cb:		called at the end of done packet
- * @data:	this data will pass back to cb
  *
  * Return: 0 for success; else the error code is returned
  *
@@ -187,19 +185,6 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
  * at the end of done packet. Note that this is an ASYNC function. When the
  * function returned, it may or may not be finished.
  */
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
-			 void *data);
-
-/**
- * cmdq_pkt_flush() - trigger CMDQ to execute the CMDQ packet
- * @pkt:	the CMDQ packet
- *
- * Return: 0 for success; else the error code is returned
- *
- * Trigger CMDQ to execute the CMDQ packet. Note that this is a
- * synchronous flush function. When the function returned, the recorded
- * commands have been done.
- */
-int cmdq_pkt_flush(struct cmdq_pkt *pkt);
+int cmdq_pkt_flush_async(struct cmdq_pkt *pkt);
 
 #endif	/* __MTK_CMDQ_H__ */
-- 
2.17.1

[PATCH 3/4] mailbox / soc / drm: mediatek: Remove struct cmdq_client

[Chun-Kuang Hu]

In mailbox rx_callback, it pass struct mbox_client to callback
function, but it could not map back to client instance because
struct cmdq_client use a pointer to struct mbox_client:

struct cmdq_client {
	struct mbox_client client;
	struct mbox_chan *chan;
};

struct client_instance {
	/* client instance data */
	struct cmdq_client *cmdq_client;
};

so remove struct cmdq_client and let client instance define
mbox_client as:

struct client_instance {
	/* client instance data */
	struct mbox_client cl;
};

and in rx_callback function, use struct mbox_client to get
struct client_instance.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c  | 35 ++++++++--------
 drivers/soc/mediatek/mtk-cmdq-helper.c   | 52 ++++--------------------
 include/linux/mailbox/mtk-cmdq-mailbox.h |  1 -
 include/linux/soc/mediatek/mtk-cmdq.h    | 30 +++-----------
 4 files changed, 32 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 5df2d431418d..f99c9d2032b8 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -49,7 +49,8 @@ struct mtk_drm_crtc {
 	bool				pending_async_planes;
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	struct cmdq_client		*cmdq_client;
+	struct mbox_client		cmdq_cl;
+	struct mbox_chan		*cmdq_chan;
 	u32				cmdq_event;
 #endif
 
@@ -240,9 +241,10 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
+	struct mtk_drm_crtc *mtk_crtc = container_of(cl, struct mtk_drm_crtc, cmdq_cl);
 	struct cmdq_cb_data *data = mssg;
 
-	cmdq_pkt_destroy(data->pkt);
+	cmdq_pkt_destroy(mtk_crtc->cmdq_chan, data->pkt);
 }
 #endif
 
@@ -481,18 +483,18 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
 		mtk_disp_mutex_release(mtk_crtc->mutex);
 	}
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	if (mtk_crtc->cmdq_client) {
-		mbox_flush(mtk_crtc->cmdq_client->chan, 2000);
-		cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
+	if (mtk_crtc->cmdq_chan) {
+		mbox_flush(mtk_crtc->cmdq_chan, 2000);
+		cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_chan, PAGE_SIZE);
 		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
 		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
 		mtk_crtc_ddp_config(crtc, cmdq_handle);
 		cmdq_pkt_finalize(cmdq_handle);
-		dma_sync_single_for_device(mtk_crtc->cmdq_client->chan->mbox->dev,
+		dma_sync_single_for_device(mtk_crtc->cmdq_chan->mbox->dev,
 					   cmdq_handle->pa_base,
 					   cmdq_handle->cmd_buf_size,
 					   DMA_TO_DEVICE);
-		cmdq_pkt_flush_async(cmdq_handle);
+		cmdq_pkt_flush_async(mtk_crtc->cmdq_chan, cmdq_handle);
 	}
 #endif
 	mutex_unlock(&mtk_crtc->hw_lock);
@@ -671,7 +673,7 @@ void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct mtk_ddp_comp *comp)
 	struct mtk_drm_private *priv = crtc->dev->dev_private;
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client)
+	if (!priv->data->shadow_register && !mtk_crtc->cmdq_chan)
 #else
 	if (!priv->data->shadow_register)
 #endif
@@ -830,14 +832,18 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 	mutex_init(&mtk_crtc->hw_lock);
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	mtk_crtc->cmdq_client =
-			cmdq_mbox_create(mtk_crtc->mmsys_dev,
-					 drm_crtc_index(&mtk_crtc->base));
-	if (IS_ERR(mtk_crtc->cmdq_client)) {
+	mtk_crtc->cmdq_cl.dev = mtk_crtc->mmsys_dev;
+	mtk_crtc->cmdq_cl.tx_block = false;
+	mtk_crtc->cmdq_cl.knows_txdone = true;
+	mtk_crtc->cmdq_cl.rx_callback = ddp_cmdq_cb;
+	mtk_crtc->cmdq_chan = mbox_request_channel(&mtk_crtc->cmdq_cl,
+						   drm_crtc_index(&mtk_crtc->base));
+	if (IS_ERR(mtk_crtc->cmdq_chan)) {
 		dev_dbg(dev, "mtk_crtc %d failed to create mailbox client, writing register by CPU now\n",
 			drm_crtc_index(&mtk_crtc->base));
-		mtk_crtc->cmdq_client = NULL;
+		mtk_crtc->cmdq_chan = NULL;
 	}
+
 	ret = of_property_read_u32_index(priv->mutex_node,
 					 "mediatek,gce-events",
 					 drm_crtc_index(&mtk_crtc->base),
@@ -845,9 +851,6 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 	if (ret)
 		dev_dbg(dev, "mtk_crtc %d failed to get mediatek,gce-events property\n",
 			drm_crtc_index(&mtk_crtc->base));
-
-	if (mtk_crtc->cmdq_client)
-		mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
 #endif
 	return 0;
 }
diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
index b6ee1b525084..f5822adb79a8 100644
--- a/drivers/soc/mediatek/mtk-cmdq-helper.c
+++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
@@ -65,41 +65,7 @@ int cmdq_dev_get_client_reg(struct device *dev,
 }
 EXPORT_SYMBOL(cmdq_dev_get_client_reg);
 
-struct cmdq_client *cmdq_mbox_create(struct device *dev, int index)
-{
-	struct cmdq_client *client;
-
-	client = kzalloc(sizeof(*client), GFP_KERNEL);
-	if (!client)
-		return (struct cmdq_client *)-ENOMEM;
-
-	client->client.dev = dev;
-	client->client.tx_block = false;
-	client->client.knows_txdone = true;
-	client->chan = mbox_request_channel(&client->client, index);
-
-	if (IS_ERR(client->chan)) {
-		long err;
-
-		dev_err(dev, "failed to request channel\n");
-		err = PTR_ERR(client->chan);
-		kfree(client);
-
-		return ERR_PTR(err);
-	}
-
-	return client;
-}
-EXPORT_SYMBOL(cmdq_mbox_create);
-
-void cmdq_mbox_destroy(struct cmdq_client *client)
-{
-	mbox_free_channel(client->chan);
-	kfree(client);
-}
-EXPORT_SYMBOL(cmdq_mbox_destroy);
-
-struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
+struct cmdq_pkt *cmdq_pkt_create(struct mbox_chan *chan, size_t size)
 {
 	struct cmdq_pkt *pkt;
 	struct device *dev;
@@ -114,9 +80,8 @@ struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
 		return ERR_PTR(-ENOMEM);
 	}
 	pkt->buf_size = size;
-	pkt->cl = (void *)client;
 
-	dev = client->chan->mbox->dev;
+	dev = chan->mbox->dev;
 	dma_addr = dma_map_single(dev, pkt->va_base, pkt->buf_size,
 				  DMA_TO_DEVICE);
 	if (dma_mapping_error(dev, dma_addr)) {
@@ -132,11 +97,9 @@ struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size)
 }
 EXPORT_SYMBOL(cmdq_pkt_create);
 
-void cmdq_pkt_destroy(struct cmdq_pkt *pkt)
+void cmdq_pkt_destroy(struct mbox_chan *chan, struct cmdq_pkt *pkt)
 {
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
-
-	dma_unmap_single(client->chan->mbox->dev, pkt->pa_base, pkt->buf_size,
+	dma_unmap_single(chan->mbox->dev, pkt->pa_base, pkt->buf_size,
 			 DMA_TO_DEVICE);
 	kfree(pkt->va_base);
 	kfree(pkt);
@@ -318,16 +281,15 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
 }
 EXPORT_SYMBOL(cmdq_pkt_finalize);
 
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt)
+int cmdq_pkt_flush_async(struct mbox_chan *chan, struct cmdq_pkt *pkt)
 {
 	int err;
-	struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
 
-	err = mbox_send_message(client->chan, pkt);
+	err = mbox_send_message(chan, pkt);
 	if (err < 0)
 		return err;
 	/* We can send next packet immediately, so just call txdone. */
-	mbox_client_txdone(client->chan, 0);
+	mbox_client_txdone(chan, 0);
 
 	return 0;
 }
diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
index 2cec4dbaa214..ce2eaf7eaf34 100644
--- a/include/linux/mailbox/mtk-cmdq-mailbox.h
+++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
@@ -68,7 +68,6 @@ struct cmdq_pkt {
 	dma_addr_t		pa_base;
 	size_t			cmd_buf_size; /* command occupied size */
 	size_t			buf_size; /* real buffer size */
-	void			*cl;
 };
 
 struct cmdq_cb_data {
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index a09b3f39b861..fb35da22e00c 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -19,11 +19,6 @@ struct cmdq_client_reg {
 	u16 size;
 };
 
-struct cmdq_client {
-	struct mbox_client client;
-	struct mbox_chan *chan;
-};
-
 /**
  * cmdq_dev_get_client_reg() - parse cmdq client reg from the device
  *			       node of CMDQ client
@@ -39,35 +34,21 @@ struct cmdq_client {
 int cmdq_dev_get_client_reg(struct device *dev,
 			    struct cmdq_client_reg *client_reg, int idx);
 
-/**
- * cmdq_mbox_create() - create CMDQ mailbox client and channel
- * @dev:	device of CMDQ mailbox client
- * @index:	index of CMDQ mailbox channel
- *
- * Return: CMDQ mailbox client pointer
- */
-struct cmdq_client *cmdq_mbox_create(struct device *dev, int index);
-
-/**
- * cmdq_mbox_destroy() - destroy CMDQ mailbox client and channel
- * @client:	the CMDQ mailbox client
- */
-void cmdq_mbox_destroy(struct cmdq_client *client);
-
 /**
  * cmdq_pkt_create() - create a CMDQ packet
- * @client:	the CMDQ mailbox client
+ * @chan:	the mailbox channel
  * @size:	required CMDQ buffer size
  *
  * Return: CMDQ packet pointer
  */
-struct cmdq_pkt *cmdq_pkt_create(struct cmdq_client *client, size_t size);
+struct cmdq_pkt *cmdq_pkt_create(struct mbox_chan *chan, size_t size);
 
 /**
  * cmdq_pkt_destroy() - destroy the CMDQ packet
+ * @chan:	the mailbox channel
  * @pkt:	the CMDQ packet
  */
-void cmdq_pkt_destroy(struct cmdq_pkt *pkt);
+void cmdq_pkt_destroy(struct mbox_chan *chan, struct cmdq_pkt *pkt);
 
 /**
  * cmdq_pkt_write() - append write command to the CMDQ packet
@@ -177,6 +158,7 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
 /**
  * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
  *                          packet and call back at the end of done packet
+ * @chan:	the mailbox channel
  * @pkt:	the CMDQ packet
  *
  * Return: 0 for success; else the error code is returned
@@ -185,6 +167,6 @@ int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
  * at the end of done packet. Note that this is an ASYNC function. When the
  * function returned, it may or may not be finished.
  */
-int cmdq_pkt_flush_async(struct cmdq_pkt *pkt);
+int cmdq_pkt_flush_async(struct mbox_chan *chan, struct cmdq_pkt *pkt);
 
 #endif	/* __MTK_CMDQ_H__ */
-- 
2.17.1

[PATCH 4/4] drm/mediatek: Detect CMDQ execution timeout

[Chun-Kuang Hu]

CMDQ is used to update display register in vblank period, so
it should be execute in next vblank. If it fail to execute
in next 2 vblank, tiemout happen.

Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index f99c9d2032b8..71f770e25ad2 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -52,6 +52,7 @@ struct mtk_drm_crtc {
 	struct mbox_client		cmdq_cl;
 	struct mbox_chan		*cmdq_chan;
 	u32				cmdq_event;
+	u32				cmdq_vblank_cnt;
 #endif
 
 	struct device			*mmsys_dev;
@@ -244,6 +245,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 	struct mtk_drm_crtc *mtk_crtc = container_of(cl, struct mtk_drm_crtc, cmdq_cl);
 	struct cmdq_cb_data *data = mssg;
 
+	mtk_crtc->cmdq_vblank_cnt = 0;
 	cmdq_pkt_destroy(mtk_crtc->cmdq_chan, data->pkt);
 }
 #endif
@@ -494,6 +496,11 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
 					   cmdq_handle->pa_base,
 					   cmdq_handle->cmd_buf_size,
 					   DMA_TO_DEVICE);
+		/*
+		 * CMDQ command should execute in next vblank,
+		 * If it fail to execute in next 2 vblank, timeout happen.
+		 */
+		mtk_crtc->cmdq_vblank_cnt = 2;
 		cmdq_pkt_flush_async(mtk_crtc->cmdq_chan, cmdq_handle);
 	}
 #endif
@@ -674,10 +681,14 @@ void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct mtk_ddp_comp *comp)
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	if (!priv->data->shadow_register && !mtk_crtc->cmdq_chan)
+		mtk_crtc_ddp_config(crtc, NULL);
+	else if (mtk_crtc->cmdq_vblank_cnt && --mtk_crtc->cmdq_vblank_cnt == 0)
+		DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n",
+			  drm_crtc_index(&mtk_crtc->base));
 #else
 	if (!priv->data->shadow_register)
-#endif
 		mtk_crtc_ddp_config(crtc, NULL);
+#endif
 
 	mtk_drm_finish_page_flip(mtk_crtc);
 }
-- 
2.17.1

Re: [PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ

[Jassi Brar]

On Sun, Sep 27, 2020 at 6:04 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
>
> CMDQ helper provide timer to detect execution timeout, but DRM driver
> could have a better way to detect execution timeout by vblank IRQ.
> For DRM, CMDQ command should execute in vblank, so if it fail to
> execute in next 2 vblank, timeout happen. Even though we could
> calculate time between 2 vblank and use timer to delect, this would
> make things more complicated.
>
> This introduce a series refinement for CMDQ mailbox controller and CMDQ
> helper. Remove timer handler in helper function because different
> client have different way to detect timeout. Use standard mailbox
> callback instead of proprietary one to get the necessary data
> in callback function. Remove struct cmdq_client to access client
> instance data by struct mbox_client.
>
> Chun-Kuang Hu (4):
>   soc / drm: mediatek: cmdq: Remove timeout handler in helper function
>   mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of
>     cmdq_task_cb
>   mailbox / soc / drm: mediatek: Remove struct cmdq_client
>   drm/mediatek: Detect CMDQ execution timeout
>
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |  54 ++++++---
>  drivers/mailbox/mtk-cmdq-mailbox.c       |  24 ++--
>  drivers/soc/mediatek/mtk-cmdq-helper.c   | 146 ++---------------------
>  include/linux/mailbox/mtk-cmdq-mailbox.h |  25 +---
>  include/linux/soc/mediatek/mtk-cmdq.h    |  54 +--------
>  5 files changed, 66 insertions(+), 237 deletions(-)
>
Please break this into two patchsets - one for mailbox and one for its users.
Also, CC original author and recent major contributors to mtk-cmdq-mailbox.c

Thanks.

Re: [PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ

[Chun-Kuang Hu]

Hi, Jassi:

Jassi Brar <jassisinghbrar@gmail.com> 於 2020年10月3日 週六 上午4:30寫道：
>
> On Sun, Sep 27, 2020 at 6:04 PM Chun-Kuang Hu <chunkuang.hu@kernel.org> wrote:
> >
> > CMDQ helper provide timer to detect execution timeout, but DRM driver
> > could have a better way to detect execution timeout by vblank IRQ.
> > For DRM, CMDQ command should execute in vblank, so if it fail to
> > execute in next 2 vblank, timeout happen. Even though we could
> > calculate time between 2 vblank and use timer to delect, this would
> > make things more complicated.
> >
> > This introduce a series refinement for CMDQ mailbox controller and CMDQ
> > helper. Remove timer handler in helper function because different
> > client have different way to detect timeout. Use standard mailbox
> > callback instead of proprietary one to get the necessary data
> > in callback function. Remove struct cmdq_client to access client
> > instance data by struct mbox_client.
> >
> > Chun-Kuang Hu (4):
> >   soc / drm: mediatek: cmdq: Remove timeout handler in helper function
> >   mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of
> >     cmdq_task_cb
> >   mailbox / soc / drm: mediatek: Remove struct cmdq_client
> >   drm/mediatek: Detect CMDQ execution timeout
> >
> >  drivers/gpu/drm/mediatek/mtk_drm_crtc.c  |  54 ++++++---
> >  drivers/mailbox/mtk-cmdq-mailbox.c       |  24 ++--
> >  drivers/soc/mediatek/mtk-cmdq-helper.c   | 146 ++---------------------
> >  include/linux/mailbox/mtk-cmdq-mailbox.h |  25 +---
> >  include/linux/soc/mediatek/mtk-cmdq.h    |  54 +--------
> >  5 files changed, 66 insertions(+), 237 deletions(-)
> >
> Please break this into two patchsets - one for mailbox and one for its users.
> Also, CC original author and recent major contributors to mtk-cmdq-mailbox.c
>

Agree with you. But for patch [2/4] ("Use mailbox rx_callback instead
of cmdq_task_cb"), I think it would be a long term process.
I would break it into:

1. mtk-cmdq-mailbox.c: add rx_callback and keep  cmdq_task_cb because
client is using cmdq_task_cb.
2. client: change from cmdq_task_cb to rx_callback.
3. mtk-cmdq-mailbox.c: remove cmdq_task_cb.

The three step has dependency, but the 2nd should move to another
series, so I would go 1st step first.

Regards,
Chun-Kuang.

> Thanks.