We choose OVL as CRC generator from other hardware
components that are also capable of calculating CRCs,
since its frame done event triggers vblanks, it can be
used as a signal to know when is safe to retrieve CRC of
the frame.

Please note that position of the hardware component
that is chosen as CRC generator in the display path is
significant. For example, while OVL is the first module
in VDOSYS0, its CRC won't be affected by the modules
after it, which means effects applied by PQ, Gamma,
Dither or any other components after OVL won't be
calculated in CRC generation.

Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_disp_drv.h     |   1 +
 drivers/gpu/drm/mediatek/mtk_disp_ovl.c     | 121 +++++++++++++++++++-
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |   1 +
 3 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
index 2254038519e1..d2753360ae1e 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
@@ -100,6 +100,7 @@ void mtk_ovl_enable_vblank(struct device *dev);
 void mtk_ovl_disable_vblank(struct device *dev);
 const u32 *mtk_ovl_get_formats(struct device *dev);
 size_t mtk_ovl_get_num_formats(struct device *dev);
+u32 mtk_ovl_crc_cnt(struct device *dev);
 
 void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex *mutex);
 void mtk_ovl_adaptor_remove_comp(struct device *dev, struct mtk_mutex *mutex);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 824f81291293..453db2de3e83 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -25,6 +25,13 @@
 #define OVL_FME_CPL_INT					BIT(1)
 #define DISP_REG_OVL_INTSTA			0x0008
 #define DISP_REG_OVL_EN				0x000c
+#define OVL_EN						BIT(0)
+#define OVL_OP_8BIT_MODE				BIT(4)
+#define OVL_HG_FOVL_CK_ON				BIT(8)
+#define OVL_HF_FOVL_CK_ON				BIT(10)
+#define DISP_REG_OVL_TRIG			0x0010
+#define OVL_CRC_EN					BIT(8)
+#define OVL_CRC_CLR					BIT(9)
 #define DISP_REG_OVL_RST			0x0014
 #define DISP_REG_OVL_ROI_SIZE			0x0020
 #define DISP_REG_OVL_DATAPATH_CON		0x0024
@@ -44,6 +51,8 @@
 #define DISP_REG_OVL_RDMA_CTRL(n)		(0x00c0 + 0x20 * (n))
 #define DISP_REG_OVL_RDMA_GMC(n)		(0x00c8 + 0x20 * (n))
 #define DISP_REG_OVL_ADDR_MT2701		0x0040
+#define DISP_REG_OVL_CRC			0x0270
+#define OVL_CRC_OUT_MASK				GENMASK(30, 0)
 #define DISP_REG_OVL_CLRFMT_EXT			0x02D0
 #define DISP_REG_OVL_CLRFMT_EXT1		0x02D8
 #define OVL_CLRFMT_EXT1_CSC_EN(n)			(1 << ((n) * 4 + 1))
@@ -151,6 +160,10 @@ static const u32 mt8195_formats[] = {
 	DRM_FORMAT_YUYV,
 };
 
+static const u32 mt8195_ovl_crcs[] = {
+	DISP_REG_OVL_CRC,
+};
+
 struct mtk_disp_ovl_data {
 	unsigned int addr;
 	unsigned int gmc_bits;
@@ -161,6 +174,8 @@ struct mtk_disp_ovl_data {
 	const u32 *formats;
 	size_t num_formats;
 	bool supports_clrfmt_ext;
+	const u32 *crcs;
+	size_t crc_cnt;
 };
 
 /*
@@ -176,8 +191,82 @@ struct mtk_disp_ovl {
 	const struct mtk_disp_ovl_data	*data;
 	void				(*vblank_cb)(void *data);
 	void				*vblank_cb_data;
+	struct cmdq_client		*cmdq_client;
+	struct cmdq_pkt			*cmdq_pkt;
+	u32				cmdq_event;
 };
 
+u32 mtk_ovl_crc_cnt(struct device *dev)
+{
+	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+	return (u32)ovl->data->crc_cnt;
+}
+
+static void mtk_ovl_crc_loop_start(struct device *dev)
+{
+	int i;
+	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+	struct mtk_drm_crtc *mtk_crtc = container_of(ovl->crtc,
+						struct mtk_drm_crtc, base);
+
+	if (!ovl->cmdq_event || ovl->cmdq_client)
+		return;
+
+	ovl->cmdq_client = cmdq_mbox_create(dev, 0);
+	if (IS_ERR(ovl->cmdq_client)) {
+		pr_err("failed to create mailbox client\n");
+		return;
+	}
+
+	ovl->cmdq_pkt = cmdq_pkt_create(ovl->cmdq_client, PAGE_SIZE);
+	if (!ovl->cmdq_pkt) {
+		pr_err("failed to create cmdq packet\n");
+		return;
+	}
+
+	cmdq_pkt_wfe(ovl->cmdq_pkt, ovl->cmdq_event, true);
+
+	for (i = 0; i < ovl->data->crc_cnt; i++) {
+		/* put crc to spr1 register */
+		cmdq_pkt_read_s(ovl->cmdq_pkt, ovl->cmdq_reg.subsys,
+				ovl->data->crcs[i], CMDQ_THR_SPR_IDX1);
+		cmdq_pkt_assign(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
+				CMDQ_ADDR_HIGH(mtk_crtc->crc.pa + i * sizeof(u32)));
+
+		/* copy spr1 register to crc.pa */
+		cmdq_pkt_write_s(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
+				 CMDQ_ADDR_LOW(mtk_crtc->crc.pa + i * sizeof(u32)),
+				 CMDQ_THR_SPR_IDX1);
+	}
+
+	/* reset crc */
+	mtk_ddp_write_mask(ovl->cmdq_pkt, ~0, &ovl->cmdq_reg, ovl->regs,
+			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
+	/* clear reset bit */
+	mtk_ddp_write_mask(ovl->cmdq_pkt, 0, &ovl->cmdq_reg, ovl->regs,
+			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
+
+	cmdq_pkt_finalize_loop(ovl->cmdq_pkt);
+	cmdq_pkt_flush_async(ovl->cmdq_pkt);
+}
+
+static void mtk_ovl_crc_loop_stop(struct device *dev)
+{
+	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
+
+	if (ovl->cmdq_pkt) {
+		cmdq_pkt_destroy(ovl->cmdq_pkt);
+		ovl->cmdq_pkt = NULL;
+	}
+
+	if (ovl->cmdq_client) {
+		mbox_flush(ovl->cmdq_client->chan, 2000);
+		cmdq_mbox_destroy(ovl->cmdq_client);
+		ovl->cmdq_client = NULL;
+	}
+}
+
 static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
 {
 	struct mtk_disp_ovl *priv = dev_id;
@@ -201,6 +290,7 @@ void mtk_ovl_register_vblank_cb(struct device *dev,
 
 	ovl->vblank_cb = vblank_cb;
 	ovl->vblank_cb_data = vblank_cb_data;
+	ovl->crtc = (struct drm_crtc *)vblank_cb_data;
 }
 
 void mtk_ovl_unregister_vblank_cb(struct device *dev)
@@ -216,14 +306,14 @@ void mtk_ovl_enable_vblank(struct device *dev)
 	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
 
 	writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA);
-	writel_relaxed(OVL_FME_CPL_INT, ovl->regs + DISP_REG_OVL_INTEN);
+	writel(OVL_FME_CPL_INT, ovl->regs + DISP_REG_OVL_INTEN);
 }
 
 void mtk_ovl_disable_vblank(struct device *dev)
 {
 	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
 
-	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
+	writel(0x0, ovl->regs + DISP_REG_OVL_INTEN);
 }
 
 const u32 *mtk_ovl_get_formats(struct device *dev)
@@ -258,6 +348,7 @@ void mtk_ovl_start(struct device *dev)
 {
 	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
 	unsigned int reg = 0;
+	unsigned int val = OVL_EN;
 
 	if (ovl->data->smi_id_en) {
 		reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
@@ -265,13 +356,22 @@ void mtk_ovl_start(struct device *dev)
 	}
 	reg |= OVL_OUTPUT_CLAMP;
 	writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
-	writel_relaxed(0x1, ovl->regs + DISP_REG_OVL_EN);
+
+	if (ovl->data->crcs)
+		val |= OVL_OP_8BIT_MODE | OVL_HG_FOVL_CK_ON | OVL_HF_FOVL_CK_ON;
+
+	writel_relaxed(val, ovl->regs + DISP_REG_OVL_EN);
+	writel_relaxed(OVL_CRC_EN, ovl->regs + DISP_REG_OVL_TRIG);
+
+	mtk_ovl_crc_loop_start(dev);
 }
 
 void mtk_ovl_stop(struct device *dev)
 {
 	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
 
+	mtk_ovl_crc_loop_stop(dev);
+
 	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_EN);
 	if (ovl->data->smi_id_en) {
 		unsigned int reg;
@@ -321,7 +421,8 @@ void mtk_ovl_config(struct device *dev, unsigned int w,
 	if (w != 0 && h != 0)
 		mtk_ddp_write_relaxed(cmdq_pkt, h << 16 | w, &ovl->cmdq_reg, ovl->regs,
 				      DISP_REG_OVL_ROI_SIZE);
-	mtk_ddp_write_relaxed(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_ROI_BGCLR);
+	mtk_ddp_write_relaxed(cmdq_pkt, 0xff000000, &ovl->cmdq_reg, ovl->regs,
+			      DISP_REG_OVL_ROI_BGCLR);
 
 	mtk_ddp_write(cmdq_pkt, 0x1, &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_RST);
 	mtk_ddp_write(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs, DISP_REG_OVL_RST);
@@ -677,6 +778,16 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
 #endif
 
 	priv->data = of_device_get_match_data(dev);
+
+	if (priv->data->crcs) {
+		if (of_property_read_u32_index(dev->of_node,
+					       "mediatek,gce-events", 0,
+					       &priv->cmdq_event)) {
+			dev_err(dev, "failed to get gce-events\n");
+			return -ENOPARAM;
+		}
+	}
+
 	platform_set_drvdata(pdev, priv);
 
 	ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
@@ -771,6 +882,8 @@ static const struct mtk_disp_ovl_data mt8195_ovl_driver_data = {
 	.formats = mt8195_formats,
 	.num_formats = ARRAY_SIZE(mt8195_formats),
 	.supports_clrfmt_ext = true,
+	.crcs = mt8195_ovl_crcs,
+	.crc_cnt = ARRAY_SIZE(mt8195_ovl_crcs),
 };
 
 static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index f114da4d36a9..1b747a34a06b 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -347,6 +347,7 @@ static const struct mtk_ddp_comp_funcs ddp_ovl = {
 	.clk_enable = mtk_ovl_clk_enable,
 	.clk_disable = mtk_ovl_clk_disable,
 	.config = mtk_ovl_config,
+	.crc_cnt = mtk_ovl_crc_cnt,
 	.start = mtk_ovl_start,
 	.stop = mtk_ovl_stop,
 	.register_vblank_cb = mtk_ovl_register_vblank_cb,
-- 
2.18.0

Hi, Hsio-chien:

On Wed, 2023-08-23 at 23:13 +0800, Hsiao Chien Sung wrote:
> We choose OVL as CRC generator from other hardware
> components that are also capable of calculating CRCs,
> since its frame done event triggers vblanks, it can be
> used as a signal to know when is safe to retrieve CRC of
> the frame.
> 
> Please note that position of the hardware component
> that is chosen as CRC generator in the display path is
> significant. For example, while OVL is the first module
> in VDOSYS0, its CRC won't be affected by the modules
> after it, which means effects applied by PQ, Gamma,
> Dither or any other components after OVL won't be
> calculated in CRC generation.
> 
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |   1 +
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c     | 121
> +++++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |   1 +
>  3 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 2254038519e1..d2753360ae1e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -100,6 +100,7 @@ void mtk_ovl_enable_vblank(struct device *dev);
>  void mtk_ovl_disable_vblank(struct device *dev);
>  const u32 *mtk_ovl_get_formats(struct device *dev);
>  size_t mtk_ovl_get_num_formats(struct device *dev);
> +u32 mtk_ovl_crc_cnt(struct device *dev);
>  
>  void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex
> *mutex);
>  void mtk_ovl_adaptor_remove_comp(struct device *dev, struct
> mtk_mutex *mutex);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 824f81291293..453db2de3e83 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -25,6 +25,13 @@
>  #define OVL_FME_CPL_INT					BIT(1)
>  #define DISP_REG_OVL_INTSTA			0x0008
>  #define DISP_REG_OVL_EN				0x000c
> +#define OVL_EN						BIT(0)
> +#define OVL_OP_8BIT_MODE				BIT(4)
> +#define OVL_HG_FOVL_CK_ON				BIT(8)
> +#define OVL_HF_FOVL_CK_ON				BIT(10)
> +#define DISP_REG_OVL_TRIG			0x0010
> +#define OVL_CRC_EN					BIT(8)
> +#define OVL_CRC_CLR					BIT(9)
>  #define DISP_REG_OVL_RST			0x0014
>  #define DISP_REG_OVL_ROI_SIZE			0x0020
>  #define DISP_REG_OVL_DATAPATH_CON		0x0024
> @@ -44,6 +51,8 @@
>  #define DISP_REG_OVL_RDMA_CTRL(n)		(0x00c0 + 0x20 * (n))
>  #define DISP_REG_OVL_RDMA_GMC(n)		(0x00c8 + 0x20 * (n))
>  #define DISP_REG_OVL_ADDR_MT2701		0x0040
> +#define DISP_REG_OVL_CRC			0x0270
> +#define OVL_CRC_OUT_MASK				GENMASK(30, 0)
>  #define DISP_REG_OVL_CLRFMT_EXT			0x02D0
>  #define DISP_REG_OVL_CLRFMT_EXT1		0x02D8
>  #define OVL_CLRFMT_EXT1_CSC_EN(n)			(1 << ((n) * 4
> + 1))
> @@ -151,6 +160,10 @@ static const u32 mt8195_formats[] = {
>  	DRM_FORMAT_YUYV,
>  };
>  
> +static const u32 mt8195_ovl_crcs[] = {
> +	DISP_REG_OVL_CRC,
> +};
> +
>  struct mtk_disp_ovl_data {
>  	unsigned int addr;
>  	unsigned int gmc_bits;
> @@ -161,6 +174,8 @@ struct mtk_disp_ovl_data {
>  	const u32 *formats;
>  	size_t num_formats;
>  	bool supports_clrfmt_ext;
> +	const u32 *crcs;
> +	size_t crc_cnt;
> 
>  };
>  
>  /*
> @@ -176,8 +191,82 @@ struct mtk_disp_ovl {
>  	const struct mtk_disp_ovl_data	*data;
>  	void				(*vblank_cb)(void *data);
>  	void				*vblank_cb_data;
> +	struct cmdq_client		*cmdq_client;

struct cmdq_client cmdq_client;

Refer to mtk_drm_crtc.c

> +	struct cmdq_pkt			*cmdq_pkt;

struct cmdq_pkt cmdq_pkt;

Refer to mtk_drm_crtc.c

> +	u32				cmdq_event;
>  };
>  
> +u32 mtk_ovl_crc_cnt(struct device *dev)
> +{
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> +	return (u32)ovl->data->crc_cnt;

If CONFIG_MTK_CMDQ is not reachable, return 0.

> +}
> +
> +static void mtk_ovl_crc_loop_start(struct device *dev)
> +{
> +	int i;
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +	struct mtk_drm_crtc *mtk_crtc = container_of(ovl->crtc,
> +						struct mtk_drm_crtc,
> base);
> +
> +	if (!ovl->cmdq_event || ovl->cmdq_client)
> +		return;
> +
> +	ovl->cmdq_client = cmdq_mbox_create(dev, 0);
> +	if (IS_ERR(ovl->cmdq_client)) {
> +		pr_err("failed to create mailbox client\n");
> +		return;
> +	}
> +
> +	ovl->cmdq_pkt = cmdq_pkt_create(ovl->cmdq_client, PAGE_SIZE);
> +	if (!ovl->cmdq_pkt) {
> +		pr_err("failed to create cmdq packet\n");
> +		return;
> +	}
> +
> +	cmdq_pkt_wfe(ovl->cmdq_pkt, ovl->cmdq_event, true);
> +
> +	for (i = 0; i < ovl->data->crc_cnt; i++) {
> +		/* put crc to spr1 register */
> +		cmdq_pkt_read_s(ovl->cmdq_pkt, ovl->cmdq_reg.subsys,
> +				ovl->data->crcs[i], CMDQ_THR_SPR_IDX1);
> +		cmdq_pkt_assign(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
> +				CMDQ_ADDR_HIGH(mtk_crtc->crc.pa + i *
> sizeof(u32)));
> +
> +		/* copy spr1 register to crc.pa */
> +		cmdq_pkt_write_s(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
> +				 CMDQ_ADDR_LOW(mtk_crtc->crc.pa + i *
> sizeof(u32)),
> +				 CMDQ_THR_SPR_IDX1);

I would like copy crc to ovl variable instread of crtc variable, and
crtc could query crc value by a new interface.

> +	}
> +
> +	/* reset crc */
> +	mtk_ddp_write_mask(ovl->cmdq_pkt, ~0, &ovl->cmdq_reg, ovl-
> >regs,
> +			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> +	/* clear reset bit */
> +	mtk_ddp_write_mask(ovl->cmdq_pkt, 0, &ovl->cmdq_reg, ovl->regs,
> +			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> +
> +	cmdq_pkt_finalize_loop(ovl->cmdq_pkt);

Create packet in probe function and only flush packet when OVL start.

> +	cmdq_pkt_flush_async(ovl->cmdq_pkt);
> +}
> +
> +static void mtk_ovl_crc_loop_stop(struct device *dev)
> +{
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> +	if (ovl->cmdq_pkt) {
> +		cmdq_pkt_destroy(ovl->cmdq_pkt);
> +		ovl->cmdq_pkt = NULL;
> +	}
> +
> +	if (ovl->cmdq_client) {
> +		mbox_flush(ovl->cmdq_client->chan, 2000);
> +		cmdq_mbox_destroy(ovl->cmdq_client);
> +		ovl->cmdq_client = NULL;
> +	}
> +}
> +
>  static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
>  {
>  	struct mtk_disp_ovl *priv = dev_id;
> @@ -201,6 +290,7 @@ void mtk_ovl_register_vblank_cb(struct device
> *dev,
>  
>  	ovl->vblank_cb = vblank_cb;
>  	ovl->vblank_cb_data = vblank_cb_data;
> +	ovl->crtc = (struct drm_crtc *)vblank_cb_data;
>  }
>  
>  void mtk_ovl_unregister_vblank_cb(struct device *dev)
> @@ -216,14 +306,14 @@ void mtk_ovl_enable_vblank(struct device *dev)
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
>  	writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA);
> -	writel_relaxed(OVL_FME_CPL_INT, ovl->regs +
> DISP_REG_OVL_INTEN);
> +	writel(OVL_FME_CPL_INT, ovl->regs + DISP_REG_OVL_INTEN);

This is not related to CRC, so move to another patch.

>  }
>  
>  void mtk_ovl_disable_vblank(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
> -	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
> +	writel(0x0, ovl->regs + DISP_REG_OVL_INTEN);

Ditto.

>  }
>  
>  const u32 *mtk_ovl_get_formats(struct device *dev)
> @@ -258,6 +348,7 @@ void mtk_ovl_start(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  	unsigned int reg = 0;
> +	unsigned int val = OVL_EN;
>  
>  	if (ovl->data->smi_id_en) {
>  		reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> @@ -265,13 +356,22 @@ void mtk_ovl_start(struct device *dev)
>  	}
>  	reg |= OVL_OUTPUT_CLAMP;
>  	writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> -	writel_relaxed(0x1, ovl->regs + DISP_REG_OVL_EN);
> +
> +	if (ovl->data->crcs)
> +		val |= OVL_OP_8BIT_MODE | OVL_HG_FOVL_CK_ON |
> OVL_HF_FOVL_CK_ON;
> +
> +	writel_relaxed(val, ovl->regs + DISP_REG_OVL_EN);
> +	writel_relaxed(OVL_CRC_EN, ovl->regs + DISP_REG_OVL_TRIG);

Set this only when this SoC support crc.

> +
> +	mtk_ovl_crc_loop_start(dev);
>  }
>  
>  void mtk_ovl_stop(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
> +	mtk_ovl_crc_loop_stop(dev);
> +
>  	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_EN);
>  	if (ovl->data->smi_id_en) {
>  		unsigned int reg;
> @@ -321,7 +421,8 @@ void mtk_ovl_config(struct device *dev, unsigned
> int w,
>  	if (w != 0 && h != 0)
>  		mtk_ddp_write_relaxed(cmdq_pkt, h << 16 | w, &ovl-
> >cmdq_reg, ovl->regs,
>  				      DISP_REG_OVL_ROI_SIZE);
> -	mtk_ddp_write_relaxed(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs, 
> DISP_REG_OVL_ROI_BGCLR);
> +	mtk_ddp_write_relaxed(cmdq_pkt, 0xff000000, &ovl->cmdq_reg,
> ovl->regs,
> +			      DISP_REG_OVL_ROI_BGCLR);

If change background color is not related to CRC, move to another
patch. If it's related, add comment for why do this.

>  
>  	mtk_ddp_write(cmdq_pkt, 0x1, &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_RST);
>  	mtk_ddp_write(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_RST);
> @@ -677,6 +778,16 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
>  #endif
>  
>  	priv->data = of_device_get_match_data(dev);
> +
> +	if (priv->data->crcs) {
> +		if (of_property_read_u32_index(dev->of_node,
> +					       "mediatek,gce-events",
> 0,
> +					       &priv->cmdq_event)) {
> +			dev_err(dev, "failed to get gce-events\n");
> +			return -ENOPARAM;

If this SoC support crc but does not support gce, I think we could just
ignore crc function and keep drm driver work.

Regards,
CK

> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, priv);
>  
>  	ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
> @@ -771,6 +882,8 @@ static const struct mtk_disp_ovl_data
> mt8195_ovl_driver_data = {
>  	.formats = mt8195_formats,
>  	.num_formats = ARRAY_SIZE(mt8195_formats),
>  	.supports_clrfmt_ext = true,
> +	.crcs = mt8195_ovl_crcs,
> +	.crc_cnt = ARRAY_SIZE(mt8195_ovl_crcs),
>  };
>  
>  static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index f114da4d36a9..1b747a34a06b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -347,6 +347,7 @@ static const struct mtk_ddp_comp_funcs ddp_ovl =
> {
>  	.clk_enable = mtk_ovl_clk_enable,
>  	.clk_disable = mtk_ovl_clk_disable,
>  	.config = mtk_ovl_config,
> +	.crc_cnt = mtk_ovl_crc_cnt,
>  	.start = mtk_ovl_start,
>  	.stop = mtk_ovl_stop,
>  	.register_vblank_cb = mtk_ovl_register_vblank_cb,

Hi, Hsiao Chien:

On Wed, 2023-08-23 at 23:13 +0800, Hsiao Chien Sung wrote:
> We choose OVL as CRC generator from other hardware
> components that are also capable of calculating CRCs,
> since its frame done event triggers vblanks, it can be
> used as a signal to know when is safe to retrieve CRC of
> the frame.
> 
> Please note that position of the hardware component
> that is chosen as CRC generator in the display path is
> significant. For example, while OVL is the first module
> in VDOSYS0, its CRC won't be affected by the modules
> after it, which means effects applied by PQ, Gamma,
> Dither or any other components after OVL won't be
> calculated in CRC generation.
> 
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |   1 +
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c     | 121
> +++++++++++++++++++-
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c |   1 +
>  3 files changed, 119 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 2254038519e1..d2753360ae1e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -100,6 +100,7 @@ void mtk_ovl_enable_vblank(struct device *dev);
>  void mtk_ovl_disable_vblank(struct device *dev);
>  const u32 *mtk_ovl_get_formats(struct device *dev);
>  size_t mtk_ovl_get_num_formats(struct device *dev);
> +u32 mtk_ovl_crc_cnt(struct device *dev);
>  
>  void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex
> *mutex);
>  void mtk_ovl_adaptor_remove_comp(struct device *dev, struct
> mtk_mutex *mutex);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 824f81291293..453db2de3e83 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -25,6 +25,13 @@
>  #define OVL_FME_CPL_INT					BIT(1)
>  #define DISP_REG_OVL_INTSTA			0x0008
>  #define DISP_REG_OVL_EN				0x000c
> +#define OVL_EN						BIT(0)
> +#define OVL_OP_8BIT_MODE				BIT(4)
> +#define OVL_HG_FOVL_CK_ON				BIT(8)
> +#define OVL_HF_FOVL_CK_ON				BIT(10)
> +#define DISP_REG_OVL_TRIG			0x0010
> +#define OVL_CRC_EN					BIT(8)
> +#define OVL_CRC_CLR					BIT(9)
>  #define DISP_REG_OVL_RST			0x0014
>  #define DISP_REG_OVL_ROI_SIZE			0x0020
>  #define DISP_REG_OVL_DATAPATH_CON		0x0024
> @@ -44,6 +51,8 @@
>  #define DISP_REG_OVL_RDMA_CTRL(n)		(0x00c0 + 0x20 * (n))
>  #define DISP_REG_OVL_RDMA_GMC(n)		(0x00c8 + 0x20 * (n))
>  #define DISP_REG_OVL_ADDR_MT2701		0x0040
> +#define DISP_REG_OVL_CRC			0x0270
> +#define OVL_CRC_OUT_MASK				GENMASK(30, 0)
>  #define DISP_REG_OVL_CLRFMT_EXT			0x02D0
>  #define DISP_REG_OVL_CLRFMT_EXT1		0x02D8
>  #define OVL_CLRFMT_EXT1_CSC_EN(n)			(1 << ((n) * 4
> + 1))
> @@ -151,6 +160,10 @@ static const u32 mt8195_formats[] = {
>  	DRM_FORMAT_YUYV,
>  };
>  
> +static const u32 mt8195_ovl_crcs[] = {
> +	DISP_REG_OVL_CRC,
> +};
> +
>  struct mtk_disp_ovl_data {
>  	unsigned int addr;
>  	unsigned int gmc_bits;
> @@ -161,6 +174,8 @@ struct mtk_disp_ovl_data {
>  	const u32 *formats;
>  	size_t num_formats;
>  	bool supports_clrfmt_ext;
> +	const u32 *crcs;
> +	size_t crc_cnt;
>  };
>  
>  /*
> @@ -176,8 +191,82 @@ struct mtk_disp_ovl {
>  	const struct mtk_disp_ovl_data	*data;
>  	void				(*vblank_cb)(void *data);
>  	void				*vblank_cb_data;
> +	struct cmdq_client		*cmdq_client;
> +	struct cmdq_pkt			*cmdq_pkt;
> +	u32				cmdq_event;
>  };
>  
> +u32 mtk_ovl_crc_cnt(struct device *dev)
> +{
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> +	return (u32)ovl->data->crc_cnt;
> +}
> +
> +static void mtk_ovl_crc_loop_start(struct device *dev)
> +{
> +	int i;
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +	struct mtk_drm_crtc *mtk_crtc = container_of(ovl->crtc,
> +						struct mtk_drm_crtc,
> base);
> +
> +	if (!ovl->cmdq_event || ovl->cmdq_client)
> +		return;
> +
> +	ovl->cmdq_client = cmdq_mbox_create(dev, 0);
> +	if (IS_ERR(ovl->cmdq_client)) {
> +		pr_err("failed to create mailbox client\n");
> +		return;
> +	}
> +
> +	ovl->cmdq_pkt = cmdq_pkt_create(ovl->cmdq_client, PAGE_SIZE);
> +	if (!ovl->cmdq_pkt) {
> +		pr_err("failed to create cmdq packet\n");
> +		return;
> +	}
> +
> +	cmdq_pkt_wfe(ovl->cmdq_pkt, ovl->cmdq_event, true);
> +
> +	for (i = 0; i < ovl->data->crc_cnt; i++) {
> +		/* put crc to spr1 register */
> +		cmdq_pkt_read_s(ovl->cmdq_pkt, ovl->cmdq_reg.subsys,
> +				ovl->data->crcs[i], CMDQ_THR_SPR_IDX1);

Read CRC by CPU in ovl irq handler instead of using cmdq, so things
would be simpler.

Regards,
CK

> +		cmdq_pkt_assign(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
> +				CMDQ_ADDR_HIGH(mtk_crtc->crc.pa + i *
> sizeof(u32)));
> +
> +		/* copy spr1 register to crc.pa */
> +		cmdq_pkt_write_s(ovl->cmdq_pkt, CMDQ_THR_SPR_IDX0,
> +				 CMDQ_ADDR_LOW(mtk_crtc->crc.pa + i *
> sizeof(u32)),
> +				 CMDQ_THR_SPR_IDX1);
> +	}
> +
> +	/* reset crc */
> +	mtk_ddp_write_mask(ovl->cmdq_pkt, ~0, &ovl->cmdq_reg, ovl-
> >regs,
> +			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> +	/* clear reset bit */
> +	mtk_ddp_write_mask(ovl->cmdq_pkt, 0, &ovl->cmdq_reg, ovl->regs,
> +			   DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> +
> +	cmdq_pkt_finalize_loop(ovl->cmdq_pkt);
> +	cmdq_pkt_flush_async(ovl->cmdq_pkt);
> +}
> +
> +static void mtk_ovl_crc_loop_stop(struct device *dev)
> +{
> +	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> +	if (ovl->cmdq_pkt) {
> +		cmdq_pkt_destroy(ovl->cmdq_pkt);
> +		ovl->cmdq_pkt = NULL;
> +	}
> +
> +	if (ovl->cmdq_client) {
> +		mbox_flush(ovl->cmdq_client->chan, 2000);
> +		cmdq_mbox_destroy(ovl->cmdq_client);
> +		ovl->cmdq_client = NULL;
> +	}
> +}
> +
>  static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
>  {
>  	struct mtk_disp_ovl *priv = dev_id;
> @@ -201,6 +290,7 @@ void mtk_ovl_register_vblank_cb(struct device
> *dev,
>  
>  	ovl->vblank_cb = vblank_cb;
>  	ovl->vblank_cb_data = vblank_cb_data;
> +	ovl->crtc = (struct drm_crtc *)vblank_cb_data;
>  }
>  
>  void mtk_ovl_unregister_vblank_cb(struct device *dev)
> @@ -216,14 +306,14 @@ void mtk_ovl_enable_vblank(struct device *dev)
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
>  	writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA);
> -	writel_relaxed(OVL_FME_CPL_INT, ovl->regs +
> DISP_REG_OVL_INTEN);
> +	writel(OVL_FME_CPL_INT, ovl->regs + DISP_REG_OVL_INTEN);
>  }
>  
>  void mtk_ovl_disable_vblank(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
> -	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
> +	writel(0x0, ovl->regs + DISP_REG_OVL_INTEN);
>  }
>  
>  const u32 *mtk_ovl_get_formats(struct device *dev)
> @@ -258,6 +348,7 @@ void mtk_ovl_start(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  	unsigned int reg = 0;
> +	unsigned int val = OVL_EN;
>  
>  	if (ovl->data->smi_id_en) {
>  		reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> @@ -265,13 +356,22 @@ void mtk_ovl_start(struct device *dev)
>  	}
>  	reg |= OVL_OUTPUT_CLAMP;
>  	writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> -	writel_relaxed(0x1, ovl->regs + DISP_REG_OVL_EN);
> +
> +	if (ovl->data->crcs)
> +		val |= OVL_OP_8BIT_MODE | OVL_HG_FOVL_CK_ON |
> OVL_HF_FOVL_CK_ON;
> +
> +	writel_relaxed(val, ovl->regs + DISP_REG_OVL_EN);
> +	writel_relaxed(OVL_CRC_EN, ovl->regs + DISP_REG_OVL_TRIG);
> +
> +	mtk_ovl_crc_loop_start(dev);
>  }
>  
>  void mtk_ovl_stop(struct device *dev)
>  {
>  	struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>  
> +	mtk_ovl_crc_loop_stop(dev);
> +
>  	writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_EN);
>  	if (ovl->data->smi_id_en) {
>  		unsigned int reg;
> @@ -321,7 +421,8 @@ void mtk_ovl_config(struct device *dev, unsigned
> int w,
>  	if (w != 0 && h != 0)
>  		mtk_ddp_write_relaxed(cmdq_pkt, h << 16 | w, &ovl-
> >cmdq_reg, ovl->regs,
>  				      DISP_REG_OVL_ROI_SIZE);
> -	mtk_ddp_write_relaxed(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs, 
> DISP_REG_OVL_ROI_BGCLR);
> +	mtk_ddp_write_relaxed(cmdq_pkt, 0xff000000, &ovl->cmdq_reg,
> ovl->regs,
> +			      DISP_REG_OVL_ROI_BGCLR);
>  
>  	mtk_ddp_write(cmdq_pkt, 0x1, &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_RST);
>  	mtk_ddp_write(cmdq_pkt, 0x0, &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_RST);
> @@ -677,6 +778,16 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
>  #endif
>  
>  	priv->data = of_device_get_match_data(dev);
> +
> +	if (priv->data->crcs) {
> +		if (of_property_read_u32_index(dev->of_node,
> +					       "mediatek,gce-events",
> 0,
> +					       &priv->cmdq_event)) {
> +			dev_err(dev, "failed to get gce-events\n");
> +			return -ENOPARAM;
> +		}
> +	}
> +
>  	platform_set_drvdata(pdev, priv);
>  
>  	ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
> @@ -771,6 +882,8 @@ static const struct mtk_disp_ovl_data
> mt8195_ovl_driver_data = {
>  	.formats = mt8195_formats,
>  	.num_formats = ARRAY_SIZE(mt8195_formats),
>  	.supports_clrfmt_ext = true,
> +	.crcs = mt8195_ovl_crcs,
> +	.crc_cnt = ARRAY_SIZE(mt8195_ovl_crcs),
>  };
>  
>  static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index f114da4d36a9..1b747a34a06b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -347,6 +347,7 @@ static const struct mtk_ddp_comp_funcs ddp_ovl =
> {
>  	.clk_enable = mtk_ovl_clk_enable,
>  	.clk_disable = mtk_ovl_clk_disable,
>  	.config = mtk_ovl_config,
> +	.crc_cnt = mtk_ovl_crc_cnt,
>  	.start = mtk_ovl_start,
>  	.stop = mtk_ovl_stop,
>  	.register_vblank_cb = mtk_ovl_register_vblank_cb,

Hi CK,

On Thu, 2023-08-24 at 02:29 +0000, CK Hu (胡俊光) wrote:
> 
> Read CRC by CPU in ovl irq handler instead of using cmdq, so things
> would be simpler.
> 

CPU is less reliable than GCE in terms of CRC retrieval because:
1. Slower read/write register speed (1/15 of GCE)
2. The time ISR being executed is not controllable
3. CPU and bus could be busy, or the clock rate is low at the time

Although we do have a CPU version, there is about a 5% chance to
fail if the CPU clock rate is not set to the maximum,
therefore, we choose GCE (CMDQ) to do the job for better stability.

Will mention this in the cover letter in the next version.

Thanks,

Hsiao Chien Sung