[PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[AngeloGioacchino Del Regno]

This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
functions, removing duplicated cmdq setup code in mtk-drm and also
removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.

This applies on top of [1] and [2].

[1]: https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
[2]: https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com

AngeloGioacchino Del Regno (3):
  drm/mediatek: Dynamically allocate CMDQ and use helper functions
  drm/mediatek: Remove all preprocessor ifs for CONFIG_MTK_CMDQ
  drm/mediatek: Move CMDQ setup to mtk_drm_cmdq_init() function

 drivers/gpu/drm/mediatek/mtk_disp_aal.c     |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_ccorr.c   |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_color.c   |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c   |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_merge.c   |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c     |   2 -
 drivers/gpu/drm/mediatek/mtk_disp_rdma.c    |   2 -
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     | 178 +++++++-------------
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |  12 --
 drivers/gpu/drm/mediatek/mtk_ethdr.c        |   6 +-
 drivers/gpu/drm/mediatek/mtk_mdp_rdma.c     |   3 +-
 include/linux/soc/mediatek/mtk-cmdq.h       |   1 +
 12 files changed, 69 insertions(+), 145 deletions(-)

-- 
2.40.1

[PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Instead of stack allocating the cmdq_client and cmdq_handle structures
switch them to pointers, allowing us to migrate this driver to use the
common functions provided by mtk-cmdq-helper.
In order to do this, it was also necessary to add a `priv` pointer to
struct cmdq_client, as that's used to pass (in this case) a mtk_crtc
handle to the ddp_cmdq_cb() mailbox RX callback function.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++-----------------
 include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
 2 files changed, 32 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 0df62b076f49..b63289ab6787 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -50,8 +50,8 @@ struct mtk_drm_crtc {
 	bool				pending_async_planes;
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	struct cmdq_client		cmdq_client;
-	struct cmdq_pkt			cmdq_handle;
+	struct cmdq_client		*cmdq_client;
+	struct cmdq_pkt			*cmdq_handle;
 	u32				cmdq_event;
 	u32				cmdq_vblank_cnt;
 	wait_queue_head_t		cb_blocking_queue;
@@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct mtk_drm_crtc *mtk_crtc)
 	}
 }
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
-static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client, struct cmdq_pkt *pkt,
-				   size_t size)
-{
-	struct device *dev;
-	dma_addr_t dma_addr;
-
-	pkt->va_base = kzalloc(size, GFP_KERNEL);
-	if (!pkt->va_base) {
-		kfree(pkt);
-		return -ENOMEM;
-	}
-	pkt->buf_size = size;
-	pkt->cl = (void *)client;
-
-	dev = client->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)) {
-		dev_err(dev, "dma map failed, size=%u\n", (u32)(u64)size);
-		kfree(pkt->va_base);
-		kfree(pkt);
-		return -ENOMEM;
-	}
-
-	pkt->pa_base = dma_addr;
-
-	return 0;
-}
-
-static void mtk_drm_cmdq_pkt_destroy(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_TO_DEVICE);
-	kfree(pkt->va_base);
-	kfree(pkt);
-}
-#endif
-
 static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
@@ -156,12 +115,9 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
 
 	mtk_mutex_put(mtk_crtc->mutex);
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle);
-
-	if (mtk_crtc->cmdq_client.chan) {
-		mbox_free_channel(mtk_crtc->cmdq_client.chan);
-		mtk_crtc->cmdq_client.chan = NULL;
-	}
+	cmdq_pkt_destroy(mtk_crtc->cmdq_handle);
+	cmdq_mbox_destroy(mtk_crtc->cmdq_client);
+	mtk_crtc->cmdq_client = NULL;
 #endif
 
 	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
@@ -288,7 +244,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
 	struct cmdq_cb_data *data = mssg;
 	struct cmdq_client *cmdq_cl = container_of(cl, struct cmdq_client, client);
-	struct mtk_drm_crtc *mtk_crtc = container_of(cmdq_cl, struct mtk_drm_crtc, cmdq_client);
+	struct mtk_drm_crtc *mtk_crtc = (struct mtk_drm_crtc *)cmdq_cl->priv;
 	struct mtk_crtc_state *state;
 	unsigned int i;
 
@@ -546,7 +502,7 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 				       bool needs_vblank)
 {
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	struct cmdq_pkt *cmdq_handle = &mtk_crtc->cmdq_handle;
+	struct cmdq_pkt *cmdq_handle = mtk_crtc->cmdq_handle;
 #endif
 	struct drm_crtc *crtc = &mtk_crtc->base;
 	struct mtk_drm_private *priv = crtc->dev->dev_private;
@@ -584,14 +540,14 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 		mtk_mutex_release(mtk_crtc->mutex);
 	}
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	if (mtk_crtc->cmdq_client.chan) {
-		mbox_flush(mtk_crtc->cmdq_client.chan, 2000);
+	if (mtk_crtc->cmdq_client) {
+		mbox_flush(mtk_crtc->cmdq_client->chan, 2000);
 		cmdq_handle->cmd_buf_size = 0;
 		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
 		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
 		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_client->chan->mbox->dev,
 					   cmdq_handle->pa_base,
 					   cmdq_handle->cmd_buf_size,
 					   DMA_TO_DEVICE);
@@ -604,8 +560,8 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 		 */
 		mtk_crtc->cmdq_vblank_cnt = 3;
 
-		mbox_send_message(mtk_crtc->cmdq_client.chan, cmdq_handle);
-		mbox_client_txdone(mtk_crtc->cmdq_client.chan, 0);
+		mbox_send_message(mtk_crtc->cmdq_client->chan, cmdq_handle);
+		mbox_client_txdone(mtk_crtc->cmdq_client->chan, 0);
 	}
 #endif
 	mtk_crtc->config_updating = false;
@@ -619,7 +575,7 @@ static void mtk_crtc_ddp_irq(void *data)
 	struct mtk_drm_private *priv = crtc->dev->dev_private;
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
-	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client.chan)
+	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client)
 		mtk_crtc_ddp_config(crtc, NULL);
 	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc->cmdq_vblank_cnt == 0)
 		DRM_ERROR("mtk_crtc %d CMDQ execute command timeout!\n",
@@ -722,7 +678,7 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
 	mtk_drm_crtc_update_config(mtk_crtc, false);
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	/* Wait for planes to be disabled by cmdq */
-	if (mtk_crtc->cmdq_client.chan)
+	if (mtk_crtc->cmdq_client)
 		wait_event_timeout(mtk_crtc->cb_blocking_queue,
 				   mtk_crtc->cmdq_vblank_cnt == 0,
 				   msecs_to_jiffies(500));
@@ -1002,19 +958,20 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 
 #if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	i = priv->mbox_index++;
-	mtk_crtc->cmdq_client.client.dev = mtk_crtc->mmsys_dev;
-	mtk_crtc->cmdq_client.client.tx_block = false;
-	mtk_crtc->cmdq_client.client.knows_txdone = true;
-	mtk_crtc->cmdq_client.client.rx_callback = ddp_cmdq_cb;
-	mtk_crtc->cmdq_client.chan =
-			mbox_request_channel(&mtk_crtc->cmdq_client.client, i);
-	if (IS_ERR(mtk_crtc->cmdq_client.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.chan = NULL;
+
+	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev, i);
+	if (IS_ERR(mtk_crtc->cmdq_client)) {
+		ret = PTR_ERR(mtk_crtc->cmdq_client);
+		dev_dbg(dev, "Failed to create CMDQ client: %d\n", ret);
+		mtk_crtc->cmdq_client = NULL;
+		return 0;
 	}
 
-	if (mtk_crtc->cmdq_client.chan) {
+	/* Setup the CMDQ handler callback */
+	mtk_crtc->cmdq_client->priv = mtk_crtc;
+	mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
+
+	if (mtk_crtc->cmdq_client) {
 		ret = of_property_read_u32_index(priv->mutex_node,
 						 "mediatek,gce-events",
 						 i,
@@ -1022,17 +979,15 @@ 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));
-			mbox_free_channel(mtk_crtc->cmdq_client.chan);
-			mtk_crtc->cmdq_client.chan = NULL;
+			cmdq_mbox_destroy(mtk_crtc->cmdq_client);
+			mtk_crtc->cmdq_client = NULL;
 		} else {
-			ret = mtk_drm_cmdq_pkt_create(&mtk_crtc->cmdq_client,
-						      &mtk_crtc->cmdq_handle,
-						      PAGE_SIZE);
+			mtk_crtc->cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
 			if (ret) {
 				dev_dbg(dev, "mtk_crtc %d failed to create cmdq packet\n",
 					drm_crtc_index(&mtk_crtc->base));
-				mbox_free_channel(mtk_crtc->cmdq_client.chan);
-				mtk_crtc->cmdq_client.chan = NULL;
+				cmdq_mbox_destroy(mtk_crtc->cmdq_client);
+				mtk_crtc->cmdq_client = NULL;
 			}
 		}
 
diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
index 649955d2cf5c..2a1dc8b12db3 100644
--- a/include/linux/soc/mediatek/mtk-cmdq.h
+++ b/include/linux/soc/mediatek/mtk-cmdq.h
@@ -25,6 +25,7 @@ struct cmdq_client_reg {
 struct cmdq_client {
 	struct mbox_client client;
 	struct mbox_chan *chan;
+	void *priv;
 };
 
 #if IS_ENABLED(CONFIG_MTK_CMDQ)
-- 
2.40.1

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Instead of stack allocating the cmdq_client and cmdq_handle
> structures
> switch them to pointers, allowing us to migrate this driver to use
> the
> common functions provided by mtk-cmdq-helper.
> In order to do this, it was also necessary to add a `priv` pointer to
> struct cmdq_client, as that's used to pass (in this case) a mtk_crtc
> handle to the ddp_cmdq_cb() mailbox RX callback function.
> 
> Signed-off-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++---------------
> --
>  include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>  2 files changed, 32 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 0df62b076f49..b63289ab6787 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>  	bool				pending_async_planes;
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	struct cmdq_client		cmdq_client;
> -	struct cmdq_pkt			cmdq_handle;
> +	struct cmdq_client		*cmdq_client;
> +	struct cmdq_pkt			*cmdq_handle;
>  	u32				cmdq_event;
>  	u32				cmdq_vblank_cnt;
>  	wait_queue_head_t		cb_blocking_queue;
> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
> mtk_drm_crtc *mtk_crtc)
>  	}
>  }
>  
> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
> struct cmdq_pkt *pkt,
> -				   size_t size)
> -{
> -	struct device *dev;
> -	dma_addr_t dma_addr;
> -
> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> -	if (!pkt->va_base) {
> -		kfree(pkt);
> -		return -ENOMEM;
> -	}
> -	pkt->buf_size = size;
> -	pkt->cl = (void *)client;

I have a plan to remove cl in struct cmdq_pkt. But this modification
would make this plan more difficult. So I would pending this patch
until cl is removed from struct cmdq_pkt.

Regards,
CK

> -
> -	dev = client->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)) {
> -		dev_err(dev, "dma map failed, size=%u\n",
> (u32)(u64)size);
> -		kfree(pkt->va_base);
> -		kfree(pkt);
> -		return -ENOMEM;
> -	}
> -
> -	pkt->pa_base = dma_addr;
> -
> -	return 0;
> -}
> -
> -static void mtk_drm_cmdq_pkt_destroy(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_TO_DEVICE);
> -	kfree(pkt->va_base);
> -	kfree(pkt);
> -}
> -#endif
> -
>  static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> @@ -156,12 +115,9 @@ static void mtk_drm_crtc_destroy(struct drm_crtc
> *crtc)
>  
>  	mtk_mutex_put(mtk_crtc->mutex);
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	mtk_drm_cmdq_pkt_destroy(&mtk_crtc->cmdq_handle);
> -
> -	if (mtk_crtc->cmdq_client.chan) {
> -		mbox_free_channel(mtk_crtc->cmdq_client.chan);
> -		mtk_crtc->cmdq_client.chan = NULL;
> -	}
> +	cmdq_pkt_destroy(mtk_crtc->cmdq_handle);
> +	cmdq_mbox_destroy(mtk_crtc->cmdq_client);
> +	mtk_crtc->cmdq_client = NULL;
>  #endif
>  
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> @@ -288,7 +244,7 @@ static void ddp_cmdq_cb(struct mbox_client *cl,
> void *mssg)
>  {
>  	struct cmdq_cb_data *data = mssg;
>  	struct cmdq_client *cmdq_cl = container_of(cl, struct
> cmdq_client, client);
> -	struct mtk_drm_crtc *mtk_crtc = container_of(cmdq_cl, struct
> mtk_drm_crtc, cmdq_client);
> +	struct mtk_drm_crtc *mtk_crtc = (struct mtk_drm_crtc *)cmdq_cl-
> >priv;
>  	struct mtk_crtc_state *state;
>  	unsigned int i;
>  
> @@ -546,7 +502,7 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  				       bool needs_vblank)
>  {
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	struct cmdq_pkt *cmdq_handle = &mtk_crtc->cmdq_handle;
> +	struct cmdq_pkt *cmdq_handle = mtk_crtc->cmdq_handle;
>  #endif
>  	struct drm_crtc *crtc = &mtk_crtc->base;
>  	struct mtk_drm_private *priv = crtc->dev->dev_private;
> @@ -584,14 +540,14 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  		mtk_mutex_release(mtk_crtc->mutex);
>  	}
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	if (mtk_crtc->cmdq_client.chan) {
> -		mbox_flush(mtk_crtc->cmdq_client.chan, 2000);
> +	if (mtk_crtc->cmdq_client) {
> +		mbox_flush(mtk_crtc->cmdq_client->chan, 2000);
>  		cmdq_handle->cmd_buf_size = 0;
>  		cmdq_pkt_clear_event(cmdq_handle, mtk_crtc-
> >cmdq_event);
>  		cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
>  		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_client->chan-
> >mbox->dev,
>  					   cmdq_handle->pa_base,
>  					   cmdq_handle->cmd_buf_size,
>  					   DMA_TO_DEVICE);
> @@ -604,8 +560,8 @@ static void mtk_drm_crtc_update_config(struct
> mtk_drm_crtc *mtk_crtc,
>  		 */
>  		mtk_crtc->cmdq_vblank_cnt = 3;
>  
> -		mbox_send_message(mtk_crtc->cmdq_client.chan,
> cmdq_handle);
> -		mbox_client_txdone(mtk_crtc->cmdq_client.chan, 0);
> +		mbox_send_message(mtk_crtc->cmdq_client->chan,
> cmdq_handle);
> +		mbox_client_txdone(mtk_crtc->cmdq_client->chan, 0);
>  	}
>  #endif
>  	mtk_crtc->config_updating = false;
> @@ -619,7 +575,7 @@ static void mtk_crtc_ddp_irq(void *data)
>  	struct mtk_drm_private *priv = crtc->dev->dev_private;
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> -	if (!priv->data->shadow_register && !mtk_crtc-
> >cmdq_client.chan)
> +	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client)
>  		mtk_crtc_ddp_config(crtc, NULL);
>  	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --mtk_crtc-
> >cmdq_vblank_cnt == 0)
>  		DRM_ERROR("mtk_crtc %d CMDQ execute command
> timeout!\n",
> @@ -722,7 +678,7 @@ static void mtk_drm_crtc_atomic_disable(struct
> drm_crtc *crtc,
>  	mtk_drm_crtc_update_config(mtk_crtc, false);
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>  	/* Wait for planes to be disabled by cmdq */
> -	if (mtk_crtc->cmdq_client.chan)
> +	if (mtk_crtc->cmdq_client)
>  		wait_event_timeout(mtk_crtc->cb_blocking_queue,
>  				   mtk_crtc->cmdq_vblank_cnt == 0,
>  				   msecs_to_jiffies(500));
> @@ -1002,19 +958,20 @@ int mtk_drm_crtc_create(struct drm_device
> *drm_dev,
>  
>  #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>  	i = priv->mbox_index++;
> -	mtk_crtc->cmdq_client.client.dev = mtk_crtc->mmsys_dev;
> -	mtk_crtc->cmdq_client.client.tx_block = false;
> -	mtk_crtc->cmdq_client.client.knows_txdone = true;
> -	mtk_crtc->cmdq_client.client.rx_callback = ddp_cmdq_cb;
> -	mtk_crtc->cmdq_client.chan =
> -			mbox_request_channel(&mtk_crtc-
> >cmdq_client.client, i);
> -	if (IS_ERR(mtk_crtc->cmdq_client.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.chan = NULL;
> +
> +	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev,
> i);
> +	if (IS_ERR(mtk_crtc->cmdq_client)) {
> +		ret = PTR_ERR(mtk_crtc->cmdq_client);
> +		dev_dbg(dev, "Failed to create CMDQ client: %d\n",
> ret);
> +		mtk_crtc->cmdq_client = NULL;
> +		return 0;
>  	}
>  
> -	if (mtk_crtc->cmdq_client.chan) {
> +	/* Setup the CMDQ handler callback */
> +	mtk_crtc->cmdq_client->priv = mtk_crtc;
> +	mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
> +
> +	if (mtk_crtc->cmdq_client) {
>  		ret = of_property_read_u32_index(priv->mutex_node,
>  						 "mediatek,gce-events",
>  						 i,
> @@ -1022,17 +979,15 @@ 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));
> -			mbox_free_channel(mtk_crtc->cmdq_client.chan);
> -			mtk_crtc->cmdq_client.chan = NULL;
> +			cmdq_mbox_destroy(mtk_crtc->cmdq_client);
> +			mtk_crtc->cmdq_client = NULL;
>  		} else {
> -			ret = mtk_drm_cmdq_pkt_create(&mtk_crtc-
> >cmdq_client,
> -						      &mtk_crtc-
> >cmdq_handle,
> -						      PAGE_SIZE);
> +			mtk_crtc->cmdq_handle =
> cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
>  			if (ret) {
>  				dev_dbg(dev, "mtk_crtc %d failed to
> create cmdq packet\n",
>  					drm_crtc_index(&mtk_crtc-
> >base));
> -				mbox_free_channel(mtk_crtc-
> >cmdq_client.chan);
> -				mtk_crtc->cmdq_client.chan = NULL;
> +				cmdq_mbox_destroy(mtk_crtc-
> >cmdq_client);
> +				mtk_crtc->cmdq_client = NULL;
>  			}
>  		}
>  
> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h
> b/include/linux/soc/mediatek/mtk-cmdq.h
> index 649955d2cf5c..2a1dc8b12db3 100644
> --- a/include/linux/soc/mediatek/mtk-cmdq.h
> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> @@ -25,6 +25,7 @@ struct cmdq_client_reg {
>  struct cmdq_client {
>  	struct mbox_client client;
>  	struct mbox_chan *chan;
> +	void *priv;
>  };
>  
>  #if IS_ENABLED(CONFIG_MTK_CMDQ)
> -- 
> 2.40.1

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno wrote:
>>   	
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>   Instead of stack allocating the cmdq_client and cmdq_handle
>> structures
>> switch them to pointers, allowing us to migrate this driver to use
>> the
>> common functions provided by mtk-cmdq-helper.
>> In order to do this, it was also necessary to add a `priv` pointer to
>> struct cmdq_client, as that's used to pass (in this case) a mtk_crtc
>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <
>> angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++---------------
>> --
>>   include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>   2 files changed, 32 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> index 0df62b076f49..b63289ab6787 100644
>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>   	bool				pending_async_planes;
>>   
>>   #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>> -	struct cmdq_client		cmdq_client;
>> -	struct cmdq_pkt			cmdq_handle;
>> +	struct cmdq_client		*cmdq_client;
>> +	struct cmdq_pkt			*cmdq_handle;
>>   	u32				cmdq_event;
>>   	u32				cmdq_vblank_cnt;
>>   	wait_queue_head_t		cb_blocking_queue;
>> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
>> mtk_drm_crtc *mtk_crtc)
>>   	}
>>   }
>>   
>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
>> struct cmdq_pkt *pkt,
>> -				   size_t size)
>> -{
>> -	struct device *dev;
>> -	dma_addr_t dma_addr;
>> -
>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>> -	if (!pkt->va_base) {
>> -		kfree(pkt);
>> -		return -ENOMEM;
>> -	}
>> -	pkt->buf_size = size;
>> -	pkt->cl = (void *)client;
> 
> I have a plan to remove cl in struct cmdq_pkt. But this modification
> would make this plan more difficult. So I would pending this patch
> until cl is removed from struct cmdq_pkt.
> 

I think that this ifdef cleanup is more urgent than the removal of `cl` from
struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in the first
place, don't you agree?

Regards,
Angelo

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno wrote:
> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
> > wrote:
> > >   	
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >   Instead of stack allocating the cmdq_client and cmdq_handle
> > > structures
> > > switch them to pointers, allowing us to migrate this driver to
> > > use
> > > the
> > > common functions provided by mtk-cmdq-helper.
> > > In order to do this, it was also necessary to add a `priv`
> > > pointer to
> > > struct cmdq_client, as that's used to pass (in this case) a
> > > mtk_crtc
> > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > ---
> > >   drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++--------
> > > -------
> > > --
> > >   include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > >   2 files changed, 32 insertions(+), 76 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > index 0df62b076f49..b63289ab6787 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > >   	bool				pending_async_planes;
> > >   
> > >   #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > -	struct cmdq_client		cmdq_client;
> > > -	struct cmdq_pkt			cmdq_handle;
> > > +	struct cmdq_client		*cmdq_client;
> > > +	struct cmdq_pkt			*cmdq_handle;
> > >   	u32				cmdq_event;
> > >   	u32				cmdq_vblank_cnt;
> > >   	wait_queue_head_t		cb_blocking_queue;
> > > @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
> > > mtk_drm_crtc *mtk_crtc)
> > >   	}
> > >   }
> > >   
> > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
> > > struct cmdq_pkt *pkt,
> > > -				   size_t size)
> > > -{
> > > -	struct device *dev;
> > > -	dma_addr_t dma_addr;
> > > -
> > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > -	if (!pkt->va_base) {
> > > -		kfree(pkt);
> > > -		return -ENOMEM;
> > > -	}
> > > -	pkt->buf_size = size;
> > > -	pkt->cl = (void *)client;
> > 
> > I have a plan to remove cl in struct cmdq_pkt. But this
> > modification
> > would make this plan more difficult. So I would pending this patch
> > until cl is removed from struct cmdq_pkt.
> > 
> 
> I think that this ifdef cleanup is more urgent than the removal of
> `cl` from
> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in
> the first
> place, don't you agree?

I think removing ifdefs and using helper function are different things.
You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().

Regards,
CK

> 
> Regards,
> Angelo
> 

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno wrote:
>> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>>    	
>>>> External email : Please do not click links or open attachments
>>>> until
>>>> you have verified the sender or the content.
>>>>    Instead of stack allocating the cmdq_client and cmdq_handle
>>>> structures
>>>> switch them to pointers, allowing us to migrate this driver to
>>>> use
>>>> the
>>>> common functions provided by mtk-cmdq-helper.
>>>> In order to do this, it was also necessary to add a `priv`
>>>> pointer to
>>>> struct cmdq_client, as that's used to pass (in this case) a
>>>> mtk_crtc
>>>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>> angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++--------
>>>> -------
>>>> --
>>>>    include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>>>    2 files changed, 32 insertions(+), 76 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> index 0df62b076f49..b63289ab6787 100644
>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>>>    	bool				pending_async_planes;
>>>>    
>>>>    #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>> -	struct cmdq_client		cmdq_client;
>>>> -	struct cmdq_pkt			cmdq_handle;
>>>> +	struct cmdq_client		*cmdq_client;
>>>> +	struct cmdq_pkt			*cmdq_handle;
>>>>    	u32				cmdq_event;
>>>>    	u32				cmdq_vblank_cnt;
>>>>    	wait_queue_head_t		cb_blocking_queue;
>>>> @@ -108,47 +108,6 @@ static void mtk_drm_finish_page_flip(struct
>>>> mtk_drm_crtc *mtk_crtc)
>>>>    	}
>>>>    }
>>>>    
>>>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client *client,
>>>> struct cmdq_pkt *pkt,
>>>> -				   size_t size)
>>>> -{
>>>> -	struct device *dev;
>>>> -	dma_addr_t dma_addr;
>>>> -
>>>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>>>> -	if (!pkt->va_base) {
>>>> -		kfree(pkt);
>>>> -		return -ENOMEM;
>>>> -	}
>>>> -	pkt->buf_size = size;
>>>> -	pkt->cl = (void *)client;
>>>
>>> I have a plan to remove cl in struct cmdq_pkt. But this
>>> modification
>>> would make this plan more difficult. So I would pending this patch
>>> until cl is removed from struct cmdq_pkt.
>>>
>>
>> I think that this ifdef cleanup is more urgent than the removal of
>> `cl` from
>> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream in
>> the first
>> place, don't you agree?
> 
> I think removing ifdefs and using helper function are different things.
> You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> 

I chose to do it like that because this function would otherwise be a
100% duplicate of the related cmdq helper :-)


> Regards,
> CK
> 
>>
>> Regards,
>> Angelo
>>

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
> Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > > > Hi, Angelo:
> > > > 
> > > > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > >    	
> > > > > External email : Please do not click links or open
> > > > > attachments
> > > > > until
> > > > > you have verified the sender or the content.
> > > > >    Instead of stack allocating the cmdq_client and
> > > > > cmdq_handle
> > > > > structures
> > > > > switch them to pointers, allowing us to migrate this driver
> > > > > to
> > > > > use
> > > > > the
> > > > > common functions provided by mtk-cmdq-helper.
> > > > > In order to do this, it was also necessary to add a `priv`
> > > > > pointer to
> > > > > struct cmdq_client, as that's used to pass (in this case) a
> > > > > mtk_crtc
> > > > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > > > 
> > > > > Signed-off-by: AngeloGioacchino Del Regno <
> > > > > angelogioacchino.delregno@collabora.com>
> > > > > ---
> > > > >    drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++-----
> > > > > ---
> > > > > -------
> > > > > --
> > > > >    include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > > > >    2 files changed, 32 insertions(+), 76 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > index 0df62b076f49..b63289ab6787 100644
> > > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > > > >    	bool				pending_async_planes;
> > > > >    
> > > > >    #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > -	struct cmdq_client		cmdq_client;
> > > > > -	struct cmdq_pkt			cmdq_handle;
> > > > > +	struct cmdq_client		*cmdq_client;
> > > > > +	struct cmdq_pkt			*cmdq_handle;
> > > > >    	u32				cmdq_event;
> > > > >    	u32				cmdq_vblank_cnt;
> > > > >    	wait_queue_head_t		cb_blocking_queue;
> > > > > @@ -108,47 +108,6 @@ static void
> > > > > mtk_drm_finish_page_flip(struct
> > > > > mtk_drm_crtc *mtk_crtc)
> > > > >    	}
> > > > >    }
> > > > >    
> > > > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
> > > > > *client,
> > > > > struct cmdq_pkt *pkt,
> > > > > -				   size_t size)
> > > > > -{
> > > > > -	struct device *dev;
> > > > > -	dma_addr_t dma_addr;
> > > > > -
> > > > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > > > -	if (!pkt->va_base) {
> > > > > -		kfree(pkt);
> > > > > -		return -ENOMEM;
> > > > > -	}
> > > > > -	pkt->buf_size = size;
> > > > > -	pkt->cl = (void *)client;
> > > > 
> > > > I have a plan to remove cl in struct cmdq_pkt. But this
> > > > modification
> > > > would make this plan more difficult. So I would pending this
> > > > patch
> > > > until cl is removed from struct cmdq_pkt.
> > > > 
> > > 
> > > I think that this ifdef cleanup is more urgent than the removal
> > > of
> > > `cl` from
> > > struct cmdq_pkt, as those ifdefs shouldn't have reached upstream
> > > in
> > > the first
> > > place, don't you agree?
> > 
> > I think removing ifdefs and using helper function are different
> > things.
> > You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> > 
> 
> I chose to do it like that because this function would otherwise be a
> 100% duplicate of the related cmdq helper :-)

Removing cl would change the interface of cmdq_pkt_create(). And this
is related to different maintainer's tree. So it would be a long time
to process. For you, only removing ifdes is urgent, so use
cmdq_pkt_create() is not urgent. So let's keep
mtk_drm_cmdq_pkt_create() and you could remove ifdefs.

Regards,
CK

> 
> 
> > Regards,
> > CK
> > 
> > > 
> > > Regards,
> > > Angelo
> > > 
> 
> 

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[AngeloGioacchino Del Regno]

Il 03/08/23 10:37, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno wrote:
>> Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
>>> wrote:
>>>> Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
>>>>> Hi, Angelo:
>>>>>
>>>>> On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del Regno
>>>>> wrote:
>>>>>>     	
>>>>>> External email : Please do not click links or open
>>>>>> attachments
>>>>>> until
>>>>>> you have verified the sender or the content.
>>>>>>     Instead of stack allocating the cmdq_client and
>>>>>> cmdq_handle
>>>>>> structures
>>>>>> switch them to pointers, allowing us to migrate this driver
>>>>>> to
>>>>>> use
>>>>>> the
>>>>>> common functions provided by mtk-cmdq-helper.
>>>>>> In order to do this, it was also necessary to add a `priv`
>>>>>> pointer to
>>>>>> struct cmdq_client, as that's used to pass (in this case) a
>>>>>> mtk_crtc
>>>>>> handle to the ddp_cmdq_cb() mailbox RX callback function.
>>>>>>
>>>>>> Signed-off-by: AngeloGioacchino Del Regno <
>>>>>> angelogioacchino.delregno@collabora.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107 +++++++-----
>>>>>> ---
>>>>>> -------
>>>>>> --
>>>>>>     include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
>>>>>>     2 files changed, 32 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> index 0df62b076f49..b63289ab6787 100644
>>>>>> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
>>>>>> @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
>>>>>>     	bool				pending_async_planes;
>>>>>>     
>>>>>>     #if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>>>> -	struct cmdq_client		cmdq_client;
>>>>>> -	struct cmdq_pkt			cmdq_handle;
>>>>>> +	struct cmdq_client		*cmdq_client;
>>>>>> +	struct cmdq_pkt			*cmdq_handle;
>>>>>>     	u32				cmdq_event;
>>>>>>     	u32				cmdq_vblank_cnt;
>>>>>>     	wait_queue_head_t		cb_blocking_queue;
>>>>>> @@ -108,47 +108,6 @@ static void
>>>>>> mtk_drm_finish_page_flip(struct
>>>>>> mtk_drm_crtc *mtk_crtc)
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
>>>>>> -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
>>>>>> *client,
>>>>>> struct cmdq_pkt *pkt,
>>>>>> -				   size_t size)
>>>>>> -{
>>>>>> -	struct device *dev;
>>>>>> -	dma_addr_t dma_addr;
>>>>>> -
>>>>>> -	pkt->va_base = kzalloc(size, GFP_KERNEL);
>>>>>> -	if (!pkt->va_base) {
>>>>>> -		kfree(pkt);
>>>>>> -		return -ENOMEM;
>>>>>> -	}
>>>>>> -	pkt->buf_size = size;
>>>>>> -	pkt->cl = (void *)client;
>>>>>
>>>>> I have a plan to remove cl in struct cmdq_pkt. But this
>>>>> modification
>>>>> would make this plan more difficult. So I would pending this
>>>>> patch
>>>>> until cl is removed from struct cmdq_pkt.
>>>>>
>>>>
>>>> I think that this ifdef cleanup is more urgent than the removal
>>>> of
>>>> `cl` from
>>>> struct cmdq_pkt, as those ifdefs shouldn't have reached upstream
>>>> in
>>>> the first
>>>> place, don't you agree?
>>>
>>> I think removing ifdefs and using helper function are different
>>> things.
>>> You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
>>>
>>
>> I chose to do it like that because this function would otherwise be a
>> 100% duplicate of the related cmdq helper :-)
> 
> Removing cl would change the interface of cmdq_pkt_create(). And this
> is related to different maintainer's tree. So it would be a long time
> to process. For you, only removing ifdes is urgent, so use
> cmdq_pkt_create() is not urgent. So let's keep
> mtk_drm_cmdq_pkt_create() and you could remove ifdefs.
> 

Hello CK,

my CMDQ cleanup has been stuck on your intention to remove `cl` from the CMDQ
helpers for ** six months ** now.

Are you performing that removal, or can we just get this cleanup finally done?

Regards,
Angelo

Re: [PATCH 1/3] drm/mediatek: Dynamically allocate CMDQ and use helper functions

[CK Hu (胡俊光)]

Hi, Angelo:

On Tue, 2024-02-06 at 14:33 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/08/23 10:37, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Thu, 2023-08-03 at 10:25 +0200, AngeloGioacchino Del Regno
> > wrote:
> > > Il 03/08/23 08:28, CK Hu (胡俊光) ha scritto:
> > > > Hi, Angelo:
> > > > 
> > > > On Wed, 2023-08-02 at 12:41 +0200, AngeloGioacchino Del Regno
> > > > wrote:
> > > > > Il 02/08/23 08:24, CK Hu (胡俊光) ha scritto:
> > > > > > Hi, Angelo:
> > > > > > 
> > > > > > On Fri, 2023-06-23 at 11:49 +0200, AngeloGioacchino Del
> > > > > > Regno
> > > > > > wrote:
> > > > > > >     	
> > > > > > > External email : Please do not click links or open
> > > > > > > attachments
> > > > > > > until
> > > > > > > you have verified the sender or the content.
> > > > > > >     Instead of stack allocating the cmdq_client and
> > > > > > > cmdq_handle
> > > > > > > structures
> > > > > > > switch them to pointers, allowing us to migrate this
> > > > > > > driver
> > > > > > > to
> > > > > > > use
> > > > > > > the
> > > > > > > common functions provided by mtk-cmdq-helper.
> > > > > > > In order to do this, it was also necessary to add a
> > > > > > > `priv`
> > > > > > > pointer to
> > > > > > > struct cmdq_client, as that's used to pass (in this case)
> > > > > > > a
> > > > > > > mtk_crtc
> > > > > > > handle to the ddp_cmdq_cb() mailbox RX callback function.
> > > > > > > 
> > > > > > > Signed-off-by: AngeloGioacchino Del Regno <
> > > > > > > angelogioacchino.delregno@collabora.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 107
> > > > > > > +++++++-----
> > > > > > > ---
> > > > > > > -------
> > > > > > > --
> > > > > > >     include/linux/soc/mediatek/mtk-cmdq.h   |   1 +
> > > > > > >     2 files changed, 32 insertions(+), 76 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > index 0df62b076f49..b63289ab6787 100644
> > > > > > > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > > > > > > @@ -50,8 +50,8 @@ struct mtk_drm_crtc {
> > > > > > >     	bool				pending_async_pla
> > > > > > > nes;
> > > > > > >     
> > > > > > >     #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > > > -	struct cmdq_client		cmdq_client;
> > > > > > > -	struct cmdq_pkt			cmdq_handle;
> > > > > > > +	struct cmdq_client		*cmdq_client;
> > > > > > > +	struct cmdq_pkt			*cmdq_handle;
> > > > > > >     	u32				cmdq_event;
> > > > > > >     	u32				cmdq_vblank_cnt;
> > > > > > >     	wait_queue_head_t		cb_blocking_queue
> > > > > > > ;
> > > > > > > @@ -108,47 +108,6 @@ static void
> > > > > > > mtk_drm_finish_page_flip(struct
> > > > > > > mtk_drm_crtc *mtk_crtc)
> > > > > > >     	}
> > > > > > >     }
> > > > > > >     
> > > > > > > -#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > > > > > > -static int mtk_drm_cmdq_pkt_create(struct cmdq_client
> > > > > > > *client,
> > > > > > > struct cmdq_pkt *pkt,
> > > > > > > -				   size_t size)
> > > > > > > -{
> > > > > > > -	struct device *dev;
> > > > > > > -	dma_addr_t dma_addr;
> > > > > > > -
> > > > > > > -	pkt->va_base = kzalloc(size, GFP_KERNEL);
> > > > > > > -	if (!pkt->va_base) {
> > > > > > > -		kfree(pkt);
> > > > > > > -		return -ENOMEM;
> > > > > > > -	}
> > > > > > > -	pkt->buf_size = size;
> > > > > > > -	pkt->cl = (void *)client;
> > > > > > 
> > > > > > I have a plan to remove cl in struct cmdq_pkt. But this
> > > > > > modification
> > > > > > would make this plan more difficult. So I would pending
> > > > > > this
> > > > > > patch
> > > > > > until cl is removed from struct cmdq_pkt.
> > > > > > 
> > > > > 
> > > > > I think that this ifdef cleanup is more urgent than the
> > > > > removal
> > > > > of
> > > > > `cl` from
> > > > > struct cmdq_pkt, as those ifdefs shouldn't have reached
> > > > > upstream
> > > > > in
> > > > > the first
> > > > > place, don't you agree?
> > > > 
> > > > I think removing ifdefs and using helper function are different
> > > > things.
> > > > You could remove ifdefs and keep mtk_drm_cmdq_pkt_create().
> > > > 
> > > 
> > > I chose to do it like that because this function would otherwise
> > > be a
> > > 100% duplicate of the related cmdq helper :-)
> > 
> > Removing cl would change the interface of cmdq_pkt_create(). And
> > this
> > is related to different maintainer's tree. So it would be a long
> > time
> > to process. For you, only removing ifdes is urgent, so use
> > cmdq_pkt_create() is not urgent. So let's keep
> > mtk_drm_cmdq_pkt_create() and you could remove ifdefs.
> > 
> 
> Hello CK,
> 
> my CMDQ cleanup has been stuck on your intention to remove `cl` from
> the CMDQ
> helpers for ** six months ** now.
> 
> Are you performing that removal, or can we just get this cleanup
> finally done?

It's busy last year and I almost forget this. Thank you to notice me. I
would send patch after Chinese new year (Feb 8th ~ Feb 14th).

Regards,
CK


> 
> Regards,
> Angelo
> 
> 
> 

[PATCH 2/3] drm/mediatek: Remove all preprocessor ifs for CONFIG_MTK_CMDQ

[AngeloGioacchino Del Regno]

Since this driver was migrated to use the MediaTek CMDQ helpers,
it's not anymore necessary to exclude CMDQ related code with
preprocessor if branches, as CMDQ is optional and the helpers
are providing the necessary inline functions to manage the case
in which CONFIG_MTK_CMDQ is not set.

Clean up all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)`
from all drivers in drm/mediatek.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_aal.c     |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_ccorr.c   |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_color.c   |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_gamma.c   |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_merge.c   |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c     |  2 --
 drivers/gpu/drm/mediatek/mtk_disp_rdma.c    |  2 --
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c     | 22 +++------------------
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 12 -----------
 drivers/gpu/drm/mediatek/mtk_ethdr.c        |  6 ++----
 drivers/gpu/drm/mediatek/mtk_mdp_rdma.c     |  3 +--
 11 files changed, 6 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 17a4d4a3b040..3faed081ea10 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -119,11 +119,9 @@ static int mtk_disp_aal_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap aal\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, priv);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
index 3f4bf7319f17..5a901ded8086 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
@@ -174,11 +174,9 @@ static int mtk_disp_ccorr_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap ccorr\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, priv);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c
index b188d3393f99..af866039bc38 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
@@ -112,11 +112,9 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap color\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, priv);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index 1bf709bac0cf..64ba5c5b631f 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -278,11 +278,9 @@ static int mtk_disp_gamma_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap gamma\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, priv);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
index 60e0b4e70978..69b6f7229339 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
@@ -269,11 +269,9 @@ static int mtk_disp_merge_probe(struct platform_device *pdev)
 			return PTR_ERR(priv->reset_ctl);
 	}
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->fifo_en = of_property_read_bool(dev->of_node,
 					      "mediatek,merge-fifo-en");
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 4c00e42ef0a8..854c7545d67d 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -531,11 +531,9 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap ovl\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	priv->data = of_device_get_match_data(dev);
 	platform_set_drvdata(pdev, priv);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index 5e90b6d593f5..c9bce38986c8 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -334,11 +334,9 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to ioremap rdma\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	if (of_find_property(dev->of_node, "mediatek,rdma-fifo-size", &ret)) {
 		ret = of_property_read_u32(dev->of_node,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index b63289ab6787..88c63330a421 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -49,13 +49,11 @@ struct mtk_drm_crtc {
 	bool				pending_planes;
 	bool				pending_async_planes;
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	struct cmdq_client		*cmdq_client;
 	struct cmdq_pkt			*cmdq_handle;
 	u32				cmdq_event;
 	u32				cmdq_vblank_cnt;
 	wait_queue_head_t		cb_blocking_queue;
-#endif
 
 	struct device			*mmsys_dev;
 	struct device			*dma_dev;
@@ -114,11 +112,9 @@ static void mtk_drm_crtc_destroy(struct drm_crtc *crtc)
 	int i;
 
 	mtk_mutex_put(mtk_crtc->mutex);
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	cmdq_pkt_destroy(mtk_crtc->cmdq_handle);
 	cmdq_mbox_destroy(mtk_crtc->cmdq_client);
 	mtk_crtc->cmdq_client = NULL;
-#endif
 
 	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
 		struct mtk_ddp_comp *comp;
@@ -239,7 +235,6 @@ struct mtk_ddp_comp *mtk_drm_ddp_comp_for_plane(struct drm_crtc *crtc,
 	return NULL;
 }
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 {
 	struct cmdq_cb_data *data = mssg;
@@ -282,7 +277,6 @@ static void ddp_cmdq_cb(struct mbox_client *cl, void *mssg)
 	mtk_crtc->cmdq_vblank_cnt = 0;
 	wake_up(&mtk_crtc->cb_blocking_queue);
 }
-#endif
 
 static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
 {
@@ -501,9 +495,7 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc,
 static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 				       bool needs_vblank)
 {
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	struct cmdq_pkt *cmdq_handle = mtk_crtc->cmdq_handle;
-#endif
 	struct drm_crtc *crtc = &mtk_crtc->base;
 	struct mtk_drm_private *priv = crtc->dev->dev_private;
 	unsigned int pending_planes = 0, pending_async_planes = 0;
@@ -539,7 +531,6 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 		mtk_crtc_ddp_config(crtc, NULL);
 		mtk_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->cmd_buf_size = 0;
@@ -563,7 +554,6 @@ static void mtk_drm_crtc_update_config(struct mtk_drm_crtc *mtk_crtc,
 		mbox_send_message(mtk_crtc->cmdq_client->chan, cmdq_handle);
 		mbox_client_txdone(mtk_crtc->cmdq_client->chan, 0);
 	}
-#endif
 	mtk_crtc->config_updating = false;
 	mutex_unlock(&mtk_crtc->hw_lock);
 }
@@ -574,16 +564,12 @@ static void mtk_crtc_ddp_irq(void *data)
 	struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
 	struct mtk_drm_private *priv = crtc->dev->dev_private;
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	if (!priv->data->shadow_register && !mtk_crtc->cmdq_client)
 		mtk_crtc_ddp_config(crtc, NULL);
 	else if (mtk_crtc->cmdq_vblank_cnt > 0 && --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)
-		mtk_crtc_ddp_config(crtc, NULL);
-#endif
+
 	mtk_drm_finish_page_flip(mtk_crtc);
 }
 
@@ -676,13 +662,13 @@ static void mtk_drm_crtc_atomic_disable(struct drm_crtc *crtc,
 	mtk_crtc->pending_planes = true;
 
 	mtk_drm_crtc_update_config(mtk_crtc, false);
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
+
 	/* Wait for planes to be disabled by cmdq */
 	if (mtk_crtc->cmdq_client)
 		wait_event_timeout(mtk_crtc->cb_blocking_queue,
 				   mtk_crtc->cmdq_vblank_cnt == 0,
 				   msecs_to_jiffies(500));
-#endif
+
 	/* Wait for planes to be disabled */
 	drm_crtc_wait_one_vblank(crtc);
 
@@ -956,7 +942,6 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 	drm_crtc_enable_color_mgmt(&mtk_crtc->base, 0, has_ctm, gamma_lut_size);
 	mutex_init(&mtk_crtc->hw_lock);
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	i = priv->mbox_index++;
 
 	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev, i);
@@ -994,6 +979,5 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 		/* for sending blocking cmd in crtc disable */
 		init_waitqueue_head(&mtk_crtc->cb_blocking_queue);
 	}
-#endif
 	return 0;
 }
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index c77af2e4000f..3fd6cc3670ab 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -70,12 +70,10 @@ void mtk_ddp_write(struct cmdq_pkt *cmdq_pkt, unsigned int value,
 		   struct cmdq_client_reg *cmdq_reg, void __iomem *regs,
 		   unsigned int offset)
 {
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	if (cmdq_pkt)
 		cmdq_pkt_write(cmdq_pkt, cmdq_reg->subsys,
 			       cmdq_reg->offset + offset, value);
 	else
-#endif
 		writel(value, regs + offset);
 }
 
@@ -83,12 +81,10 @@ void mtk_ddp_write_relaxed(struct cmdq_pkt *cmdq_pkt, unsigned int value,
 			   struct cmdq_client_reg *cmdq_reg, void __iomem *regs,
 			   unsigned int offset)
 {
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	if (cmdq_pkt)
 		cmdq_pkt_write(cmdq_pkt, cmdq_reg->subsys,
 			       cmdq_reg->offset + offset, value);
 	else
-#endif
 		writel_relaxed(value, regs + offset);
 }
 
@@ -96,19 +92,15 @@ void mtk_ddp_write_mask(struct cmdq_pkt *cmdq_pkt, unsigned int value,
 			struct cmdq_client_reg *cmdq_reg, void __iomem *regs,
 			unsigned int offset, unsigned int mask)
 {
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	if (cmdq_pkt) {
 		cmdq_pkt_write_mask(cmdq_pkt, cmdq_reg->subsys,
 				    cmdq_reg->offset + offset, value, mask);
 	} else {
-#endif
 		u32 tmp = readl(regs + offset);
 
 		tmp = (tmp & ~mask) | (value & mask);
 		writel(tmp, regs + offset);
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	}
-#endif
 }
 
 static int mtk_ddp_clk_enable(struct device *dev)
@@ -550,9 +542,7 @@ int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
 	struct platform_device *comp_pdev;
 	enum mtk_ddp_comp_type type;
 	struct mtk_ddp_comp_dev *priv;
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	int ret;
-#endif
 
 	if (comp_id < 0 || comp_id >= DDP_COMPONENT_DRM_ID_MAX)
 		return -EINVAL;
@@ -598,11 +588,9 @@ int mtk_ddp_comp_init(struct device_node *node, struct mtk_ddp_comp *comp,
 	if (IS_ERR(priv->clk))
 		return PTR_ERR(priv->clk);
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(comp->dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(comp->dev, "get mediatek,gce-client-reg fail!\n");
-#endif
 
 	platform_set_drvdata(comp_pdev, priv);
 
diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c
index 4a5bd5bb9d6e..929479fa22f3 100644
--- a/drivers/gpu/drm/mediatek/mtk_ethdr.c
+++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c
@@ -303,12 +303,10 @@ static int mtk_ethdr_probe(struct platform_device *pdev)
 	for (i = 0; i < ETHDR_ID_MAX; i++) {
 		priv->ethdr_comp[i].dev = dev;
 		priv->ethdr_comp[i].regs = of_iomap(dev->of_node, i);
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
-		ret = cmdq_dev_get_client_reg(dev,
-					      &priv->ethdr_comp[i].cmdq_base, i);
+		ret = cmdq_dev_get_client_reg(dev, &priv->ethdr_comp[i].cmdq_base, i);
 		if (ret)
 			dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
+
 		dev_dbg(dev, "[DRM]regs:0x%p, node:%d\n", priv->ethdr_comp[i].regs, i);
 	}
 
diff --git a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
index 93ef05ec9720..897e8e2a1d05 100644
--- a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
@@ -293,11 +293,10 @@ static int mtk_mdp_rdma_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->clk))
 		return dev_err_probe(dev, PTR_ERR(priv->clk), "failed to get rdma clk\n");
 
-#if IS_REACHABLE(CONFIG_MTK_CMDQ)
 	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
 	if (ret)
 		dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
-#endif
+
 	platform_set_drvdata(pdev, priv);
 
 	ret = devm_pm_runtime_enable(dev);
-- 
2.40.1

[PATCH 3/3] drm/mediatek: Move CMDQ setup to mtk_drm_cmdq_init() function

[AngeloGioacchino Del Regno]

In order to enhance human readability, separating the optional
CMDQ mailbox initialization from the rest of the CRTC creation
machinery, move it to a new mtk_drm_cmdq_init() function.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 83 ++++++++++++++-----------
 1 file changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 88c63330a421..bff65c8c6a0e 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -826,6 +826,48 @@ struct device *mtk_drm_crtc_dma_dev_get(struct drm_crtc *crtc)
 	return mtk_crtc->dma_dev;
 }
 
+static int mtk_drm_cmdq_init(struct device *dev, struct mtk_drm_private *priv,
+			     struct mtk_drm_crtc *mtk_crtc)
+{
+	int ret;
+
+	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev, priv->mbox_index);
+	if (IS_ERR(mtk_crtc->cmdq_client)) {
+		ret = PTR_ERR(mtk_crtc->cmdq_client);
+		dev_dbg(dev, "Failed to create CMDQ client: %d\n", ret);
+		goto error;
+	}
+
+	/* Setup the CMDQ handler callback */
+	mtk_crtc->cmdq_client->priv = mtk_crtc;
+	mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
+
+	ret = of_property_read_u32_index(priv->mutex_node, "mediatek,gce-events",
+					 priv->mbox_index, &mtk_crtc->cmdq_event);
+	if (ret) {
+		dev_dbg(dev, "Failed to get mediatek,gce-events: %d\n", ret);
+		goto free_mbox;
+	}
+
+	mtk_crtc->cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
+	if (IS_ERR(mtk_crtc->cmdq_handle)) {
+		ret = PTR_ERR(mtk_crtc->cmdq_handle);
+		dev_err(dev, "Failed to create cmdq packet: %d\n", ret);
+		goto free_mbox;
+	}
+
+	/* for sending blocking cmd in crtc disable */
+	init_waitqueue_head(&mtk_crtc->cb_blocking_queue);
+
+	return 0;
+
+free_mbox:
+	cmdq_mbox_destroy(mtk_crtc->cmdq_client);
+error:
+	mtk_crtc->cmdq_client = NULL;
+	return ret;
+}
+
 int mtk_drm_crtc_create(struct drm_device *drm_dev,
 			const unsigned int *path, unsigned int path_len,
 			int priv_data_index)
@@ -942,42 +984,13 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
 	drm_crtc_enable_color_mgmt(&mtk_crtc->base, 0, has_ctm, gamma_lut_size);
 	mutex_init(&mtk_crtc->hw_lock);
 
-	i = priv->mbox_index++;
-
-	mtk_crtc->cmdq_client = cmdq_mbox_create(mtk_crtc->mmsys_dev, i);
-	if (IS_ERR(mtk_crtc->cmdq_client)) {
-		ret = PTR_ERR(mtk_crtc->cmdq_client);
-		dev_dbg(dev, "Failed to create CMDQ client: %d\n", ret);
-		mtk_crtc->cmdq_client = NULL;
-		return 0;
-	}
-
-	/* Setup the CMDQ handler callback */
-	mtk_crtc->cmdq_client->priv = mtk_crtc;
-	mtk_crtc->cmdq_client->client.rx_callback = ddp_cmdq_cb;
+	ret = mtk_drm_cmdq_init(dev, priv, mtk_crtc);
+	if (ret)
+		dev_info(dev, "No CMDQ support for CRTC%d: using CPU writes\n",
+			 drm_crtc_index(&mtk_crtc->base));
 
-	if (mtk_crtc->cmdq_client) {
-		ret = of_property_read_u32_index(priv->mutex_node,
-						 "mediatek,gce-events",
-						 i,
-						 &mtk_crtc->cmdq_event);
-		if (ret) {
-			dev_dbg(dev, "mtk_crtc %d failed to get mediatek,gce-events property\n",
-				drm_crtc_index(&mtk_crtc->base));
-			cmdq_mbox_destroy(mtk_crtc->cmdq_client);
-			mtk_crtc->cmdq_client = NULL;
-		} else {
-			mtk_crtc->cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
-			if (ret) {
-				dev_dbg(dev, "mtk_crtc %d failed to create cmdq packet\n",
-					drm_crtc_index(&mtk_crtc->base));
-				cmdq_mbox_destroy(mtk_crtc->cmdq_client);
-				mtk_crtc->cmdq_client = NULL;
-			}
-		}
+	/* Unconditionally increment mbox_index */
+	priv->mbox_index++;
 
-		/* for sending blocking cmd in crtc disable */
-		init_waitqueue_head(&mtk_crtc->cb_blocking_queue);
-	}
 	return 0;
 }
-- 
2.40.1

Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[Alexandre Mergnat]

On 23/06/2023 11:49, AngeloGioacchino Del Regno wrote:
> This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
> functions, removing duplicated cmdq setup code in mtk-drm and also
> removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
> keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.
> 
> This applies on top of [1] and [2].
> 
> [1]:https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
> [2]:https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com

Hi Angelo,

Can you provide a public branch to test it please ?
I tried to apply the dependencies but still have an issue with the 3rd one:

https://lore.kernel.org/lkml/20230523104234.7849-1-angelogioacchino.delregno@collabora.com
OK

https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
OK

https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
KO

Thanks

-- 
Regards,
Alexandre

Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[AngeloGioacchino Del Regno]

Il 23/06/23 14:49, Alexandre Mergnat ha scritto:
> 
> 
> On 23/06/2023 11:49, AngeloGioacchino Del Regno wrote:
>> This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
>> functions, removing duplicated cmdq setup code in mtk-drm and also
>> removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
>> keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.
>>
>> This applies on top of [1] and [2].
>>
>> [1]:https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>> [2]:https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
> 
> Hi Angelo,
> 
> Can you provide a public branch to test it please ?
> I tried to apply the dependencies but still have an issue with the 3rd one:
> 
> https://lore.kernel.org/lkml/20230523104234.7849-1-angelogioacchino.delregno@collabora.com
> OK
> 
> https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
> OK
> 
> https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
> KO
> 
> Thanks
> 

Sorry for the very late reply; I've somehow lost this email in the haystack...

There you go:
https://gitlab.collabora.com/google/chromeos-kernel/-/commits/for-kernelci/

Cheers,
Angelo

Re: [PATCH 0/3] MediaTek DRM: Clean up CMDQ support and ifdefs

[Alexandre Mergnat]

On 19/07/2023 09:41, AngeloGioacchino Del Regno wrote:
> Il 23/06/23 14:49, Alexandre Mergnat ha scritto:
>>
>>
>> On 23/06/2023 11:49, AngeloGioacchino Del Regno wrote:
>>> This series changes MediaTek CMDQ support to use the mtk-cmdq-helper
>>> functions, removing duplicated cmdq setup code in mtk-drm and also
>>> removing all instances of `#if IS_REACHABLE(CONFIG_MTK_CMDQ)` while
>>> keeping compatibility with both CONFIG_MTK_CMDQ=n and =m/=y.
>>>
>>> This applies on top of [1] and [2].
>>>
>>> [1]:https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>>> [2]:https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
>>
>> Hi Angelo,
>>
>> Can you provide a public branch to test it please ?
>> I tried to apply the dependencies but still have an issue with the 3rd 
>> one:
>>
>> https://lore.kernel.org/lkml/20230523104234.7849-1-angelogioacchino.delregno@collabora.com
>> OK
>>
>> https://lore.kernel.org/lkml/20230524093412.92211-1-angelogioacchino.delregno@collabora.com
>> OK
>>
>> https://lore.kernel.org/lkml/20230608084727.74403-1-angelogioacchino.delregno@collabora.com
>> KO
>>
>> Thanks
>>
> 
> Sorry for the very late reply; I've somehow lost this email in the 
> haystack...
> 
> There you go:
> https://gitlab.collabora.com/google/chromeos-kernel/-/commits/for-kernelci/
> 

Thanks Angelo,

I build/boot your branch for the i350-evk board.

I had to revert this commit from your branch:
"15f12798e218 TODO: soc: mediatek: mtk-pm-domains: Support CPU Power 
Domains"
Because it conflict with the "soc: mediatek: MT8365 power support" serie 
[3].

Also, I change back the defconfig to be aligned with the v6.5-rc1

After that, the board boot and drm/display seems working like a charm.

[3]: 
https://lore.kernel.org/linux-arm-kernel/20230713150414.891893-1-msp@baylibre.com/

-- 
Regards,
Alexandre