Message-ID: <7911aa8e4ed128872649a3724718e2b28c40c8ac.camel@mediatek.com>
Date:   Thu, 31 Aug 2023 01:45:22 +0000
From:   CK Hu (胡俊光) <ck.hu@...iatek.com>
To:     "chunkuang.hu@...nel.org" <chunkuang.hu@...nel.org>,
        Shawn Sung (宋孝謙) 
        <Shawn.Sung@...iatek.com>, "daniel@...ll.ch" <daniel@...ll.ch>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "airlied@...il.com" <airlied@...il.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "jassisinghbrar@...il.com" <jassisinghbrar@...il.com>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
CC:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        Jason-JH Lin (林睿祥) 
        <Jason-JH.Lin@...iatek.com>,
        Nancy Lin (林欣螢) <Nancy.Lin@...iatek.com>,
        Singo Chang (張興國) 
        <Singo.Chang@...iatek.com>,
        Project_Global_Chrome_Upstream_Group 
        <Project_Global_Chrome_Upstream_Group@...iatek.com>
Subject: Re: [PATCH 11/15] drm/mediatek: Support CRC in VDOSYS0

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@...iatek.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,

Powered by blists - more mailing lists