Re: [PATCH v6 13/14] drm/mediatek: Support CRC in OVL

From: CK Hu (胡俊光)
Date: Tue Mar 26 2024 - 02:12:26 EST

Next message: Su Hui: "[PATCH net v2 1/2] octeontx2-pf: check negative error code in otx2_open()"
Previous message: Varadarajan Narayanan: "Re: [PATCH v2 3/4] clk: qcom: add IPQ9574 interconnect clocks support"
Next in thread: Shawn Sung (宋孝謙): "Re: [PATCH v6 13/14] drm/mediatek: Support CRC in OVL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Shawn:

On Fri, 2024-03-22 at 13:28 +0800, Shawn Sung wrote:
> From: Hsiao Chien Sung <shawn.sung@xxxxxxxxxxxx>
>
> We choose OVL as the 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@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 3 +
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 198
> ++++++++++++++++++++++--
> 3 files changed, 194 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> index 17b0364112922..cb71effda9c2a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> @@ -351,6 +351,9 @@ 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,
> + .crc_entry = mtk_ovl_crc_entry,
> + .crc_read = mtk_ovl_crc_read,
> .start = mtk_ovl_start,
> .stop = mtk_ovl_stop,
> .register_vblank_cb = mtk_ovl_register_vblank_cb,
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 082ac18fe04aa..c476056a5cbb5 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -105,6 +105,9 @@ 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);
> +size_t mtk_ovl_crc_cnt(struct device *dev);
> +u32 *mtk_ovl_crc_entry(struct device *dev);
> +void mtk_ovl_crc_read(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 a936f338ab79d..279e6193e7975 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -24,12 +24,20 @@
> #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
> #define OVL_LAYER_SMI_ID_EN BIT(0)
> #define OVL_BGCLR_SEL_IN BIT(2)
> #define OVL_LAYER_AFBC_EN(n) BIT(4+n)
> +#define OVL_OUTPUT_CLAMP BIT(26)
> #define DISP_REG_OVL_ROI_BGCLR 0x0028
> #define DISP_REG_OVL_SRC_CON 0x002c
> #define DISP_REG_OVL_CON(n) (0x0030 + 0x20 * (n))
> @@ -42,7 +50,26 @@
> #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))
> +#define DISP_REG_OVL_Y2R_PARA_R0(n) (0x0134 + 0x28 * (n))
> +#define OVL_Y2R_PARA_C_CF_RMY (GENMAS
> K(14, 0))
> +#define DISP_REG_OVL_Y2R_PARA_G0(n) (0x013c + 0x28 * (n))
> +#define OVL_Y2R_PARA_C_CF_GMU (GENMAS
> K(30, 16))
> +#define DISP_REG_OVL_Y2R_PARA_B1(n) (0x0148 + 0x28 * (n))
> +#define OVL_Y2R_PARA_C_CF_BMV (GENMAS
> K(14, 0))
> +#define DISP_REG_OVL_Y2R_PARA_YUV_A_0(n) (0x014c + 0x28 * (n))
> +#define OVL_Y2R_PARA_C_CF_YA (GENMASK(10,
> 0))
> +#define OVL_Y2R_PARA_C_CF_UA (GENMASK(26,
> 16))
> +#define DISP_REG_OVL_Y2R_PARA_YUV_A_1(n) (0x0150 + 0x28 * (n))
> +#define OVL_Y2R_PARA_C_CF_VA (GENMASK(10,
> 0))
> +#define DISP_REG_OVL_Y2R_PRE_ADD2(n) (0x0154 + 0x28 * (n))
> +#define DISP_REG_OVL_R2R_R0(n) (0x0500 + 0x40
> * (n))
> +#define DISP_REG_OVL_R2R_G1(n) (0x0510 + 0x40
> * (n))
> +#define DISP_REG_OVL_R2R_B2(n) (0x0520 + 0x40
> * (n))
> #define DISP_REG_OVL_ADDR_MT8173 0x0f40
> #define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->addr +
> 0x20 * (n))
> #define DISP_REG_OVL_HDR_ADDR(ovl, n) ((ovl)->data-
> >addr + 0x20 * (n) + 0x04)
> @@ -55,6 +82,8 @@
> #define OVL_CON_CLRFMT_MAN BIT(23)
> #define OVL_CON_BYTE_SWAP BIT(24)
> #define OVL_CON_RGB_SWAP BIT(25)
> +#define OVL_CON_MTX_AUTO_DIS BIT(26)
> +#define OVL_CON_MTX_EN BIT(27)
> #define OVL_CON_CLRFMT_RGB (1 << 12)
> #define OVL_CON_CLRFMT_RGBA8888 (2 << 12)
> #define OVL_CON_CLRFMT_ARGB8888 (3 << 12)
> @@ -62,6 +91,7 @@
> #define OVL_CON_CLRFMT_YUYV (5 << 12)
> #define OVL_CON_MTX_YUV_TO_RGB (6 << 16)
> #define OVL_CON_CLRFMT_PARGB8888 (OVL_CON_CLRFMT_ARGB8888 |
> OVL_CON_CLRFMT_MAN)
> +#define OVL_CON_MTX_PROGRAMMABLE (8 << 16)
> #define OVL_CON_CLRFMT_RGB565(ovl) ((ovl)->data->fmt_rgb565_is_0 ?
> \
> 0 : OVL_CON_CLRFMT_RGB)
> #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ?
> \
> @@ -131,6 +161,10 @@ static const u32 mt8195_formats[] = {
> DRM_FORMAT_YUYV,
> };
>
> +static const u32 mt8195_ovl_crc_ofs[] = {
> + DISP_REG_OVL_CRC,
> +};
> +
> struct mtk_disp_ovl_data {
> unsigned int addr;
> unsigned int gmc_bits;
> @@ -141,12 +175,15 @@ struct mtk_disp_ovl_data {
> const u32 *formats;
> size_t num_formats;
> bool supports_clrfmt_ext;
> + const u32 *crc_ofs;
> + size_t crc_cnt;
> };
>
> /*
> * struct mtk_disp_ovl - DISP_OVL driver structure
> * @crtc: associated crtc to report vblank events to
> * @data: platform data
> + * @crc: crc related information
> */
> struct mtk_disp_ovl {
> struct drm_crtc *crtc;
> @@ -156,8 +193,31 @@ struct mtk_disp_ovl {
> const struct mtk_disp_ovl_data *data;
> void (*vblank_cb)(void *data);
> void *vblank_cb_data;
> + resource_size_t regs_pa;
> + struct mtk_crtc_crc crc;
> };
>
> +size_t mtk_ovl_crc_cnt(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->crc.cnt;
> +}
> +
> +u32 *mtk_ovl_crc_entry(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->crc.va;
> +}
> +
> +void mtk_ovl_crc_read(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + mtk_crtc_read_crc(&ovl->crc, ovl->regs);
> +}
> +
> static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
> {
> struct mtk_disp_ovl *priv = dev_id;
> @@ -237,21 +297,40 @@ void mtk_ovl_clk_disable(struct device *dev)
> void mtk_ovl_start(struct device *dev)
> {
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> + unsigned int reg = readl(ovl->regs +
> DISP_REG_OVL_DATAPATH_CON);
>
> - if (ovl->data->smi_id_en) {
> - unsigned int reg;
> + if (ovl->data->smi_id_en)
> + reg |= OVL_LAYER_SMI_ID_EN;
>
> - reg = readl(ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> - reg = reg | OVL_LAYER_SMI_ID_EN;
> - writel_relaxed(reg, ovl->regs +
> DISP_REG_OVL_DATAPATH_CON);
> + /*
> + * When doing Y2R conversion, it's common to get an output
> + * that is larger than 10 bits (negative numbers).
> + * Enable this bit to clamp the output to 10 bits per channel
> + * (should always be enabled)
> + */
> + reg |= OVL_OUTPUT_CLAMP;
> + writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> +
> + reg = OVL_EN;
> + if (ovl->data->crc_cnt) {
> + /* enable crc and its related clocks */
> + writel_relaxed(OVL_CRC_EN, ovl->regs +
> DISP_REG_OVL_TRIG);
> + reg |= OVL_OP_8BIT_MODE | OVL_HG_FOVL_CK_ON |
> OVL_HF_FOVL_CK_ON;
> }
> - writel_relaxed(0x1, ovl->regs + DISP_REG_OVL_EN);
> + writel_relaxed(reg, ovl->regs + DISP_REG_OVL_EN);
> +
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> + mtk_crtc_start_crc_cmdq(&ovl->crc);
> +#endif
> }
>
> void mtk_ovl_stop(struct device *dev)
> {
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> + mtk_crtc_stop_crc_cmdq(&ovl->crc);
> +#endif
> writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_EN);
> if (ovl->data->smi_id_en) {
> unsigned int reg;
> @@ -488,6 +567,83 @@ void mtk_ovl_layer_config(struct device *dev,
> unsigned int idx,
> (state->base.fb && !state->base.fb->format->has_alpha))
> ignore_pixel_alpha = OVL_CONST_BLEND;
>
> + /*
> + * OVL only supports 8 bits data in CRC calculation, transform
> 10-bit
> + * RGB to 8-bit RGB by leveraging the ability of the Y2R (YUV-
> to-RGB)
> + * hardware to multiply coefficients, although there is nothing
> to do
> + * with the YUV format.
> + */
> + if (ovl->data->supports_clrfmt_ext) {
> + u32 y2r_coef = 0, y2r_offset = 0, r2r_coef = 0, csc_en
> = 0;
> +
> + if (is_10bit_rgb(fmt)) {
> + con |= OVL_CON_MTX_AUTO_DIS | OVL_CON_MTX_EN |
> OVL_CON_MTX_PROGRAMMABLE;
> +
> + /*
> + * Y2R coefficient setting
> + * bit 13 is 2^1, bit 12 is 2^0, bit 11 is 2^-
> 1,
> + * bit 10 is 2^-2 = 0.25
> + */
> + y2r_coef = BIT(10);
> +
> + /* -1 in 10bit */
> + y2r_offset = GENMASK(10, 0) - 1;

I don't know why do this? If an input value is 0x100, then

0x100 right shit 2 bit become 0x40.
0x40 - 1 = 0x3f.
0x3f left shift 2 bit become 0xfc.

So input 0x100 and output 0xfc. Why?

> +
> + /*
> + * R2R coefficient setting
> + * bit 19 is 2^1, bit 18 is 2^0, bit 17 is 2^-
> 1,
> + * bit 20 is 2^2 = 4
> + */
> + r2r_coef = BIT(20);
> +
> + /* CSC_EN is for R2R */
> + csc_en = OVL_CLRFMT_EXT1_CSC_EN(idx);
> +
> + /*
> + * 1. YUV input data - 1 and shift right for 2
> bits to remove it
> + * [R'] [0.25 0 0] [Y in - 1]
> + * [G'] = [ 0 0.25 0] * [U in - 1]
> + * [B'] [ 0 0 0.25] [V in - 1]
> + *
> + * 2. shift left for 2 bit letting the last 2
> bits become 0

You truncate the last two bit, so some quality lost. I think the
quality is main function and CRC is just for debug. So it's better that
in normal case we keep quality and only for debug to lost the quality.

I have another question. You just truncate the last two bit but it is
still 10 bit value, so CRC could calculate this 10 bit value? I don't
know why you say CRC just for 8 bit?

Regards,
CK

> + * [R out] [ 4 0 0] [R']
> + * [G out] = [ 0 4 0] * [G']
> + * [B out] [ 0 0 4] [B']
> + */
> + }
> +
> + mtk_ddp_write_mask(cmdq_pkt, y2r_coef,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_R0(idx),
> + OVL_Y2R_PARA_C_CF_RMY);
> + mtk_ddp_write_mask(cmdq_pkt, (y2r_coef << 16),
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_G0(idx),
> + OVL_Y2R_PARA_C_CF_GMU);
> + mtk_ddp_write_mask(cmdq_pkt, y2r_coef,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_B1(idx),
> + OVL_Y2R_PARA_C_CF_BMV);
> +
> + mtk_ddp_write_mask(cmdq_pkt, y2r_offset,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_YUV_A_0(idx),
> + OVL_Y2R_PARA_C_CF_YA);
> + mtk_ddp_write_mask(cmdq_pkt, (y2r_offset << 16),
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_YUV_A_0(idx),
> + OVL_Y2R_PARA_C_CF_UA);
> + mtk_ddp_write_mask(cmdq_pkt, y2r_offset,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_Y2R_PARA_YUV_A_1(idx),
> + OVL_Y2R_PARA_C_CF_VA);
> +
> + mtk_ddp_write_relaxed(cmdq_pkt, r2r_coef,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_R2R_R0(idx));
> + mtk_ddp_write_relaxed(cmdq_pkt, r2r_coef,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_R2R_G1(idx));
> + mtk_ddp_write_relaxed(cmdq_pkt, r2r_coef,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_R2R_B2(idx));
> +
> + mtk_ddp_write_mask(cmdq_pkt, csc_en,
> + &ovl->cmdq_reg, ovl->regs,
> DISP_REG_OVL_CLRFMT_EXT1,
> + OVL_CLRFMT_EXT1_CSC_EN(idx));
> + }
> +
> if (pending->rotation & DRM_MODE_REFLECT_Y) {
> con |= OVL_CON_VIRT_FLIP;
> addr += (pending->height - 1) * pending->pitch;
> @@ -594,15 +750,31 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
> dev_err(dev, "failed to ioremap ovl\n");
> return PTR_ERR(priv->regs);
> }
> +
> + priv->data = of_device_get_match_data(dev);
> + platform_set_drvdata(pdev, priv);
> +
> + if (priv->data->crc_cnt) {
> + mtk_crtc_init_crc(&priv->crc,
> + priv->data->crc_ofs, priv->data-
> >crc_cnt,
> + DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> + }
> +
> #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);
>
> + if (priv->data->crc_cnt) {
> + if (of_property_read_u32_index(dev->of_node,
> + "mediatek,gce-events",
> 0,
> + &priv->crc.cmdq_event))
> {
> + dev_warn(dev, "failed to get gce-events for
> crc\n");
> + }
> + priv->crc.cmdq_reg = &priv->cmdq_reg;
> + mtk_crtc_create_crc_cmdq(dev, &priv->crc);
> + }
> +#endif
> ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
> IRQF_TRIGGER_NONE, dev_name(dev), priv);
> if (ret < 0) {
> @@ -623,6 +795,10 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
>
> static void mtk_disp_ovl_remove(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + mtk_crtc_destroy_crc(&ovl->crc);
> component_del(&pdev->dev, &mtk_disp_ovl_component_ops);
> pm_runtime_disable(&pdev->dev);
> }
> @@ -693,6 +869,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,
> + .crc_ofs = mt8195_ovl_crc_ofs,
> + .crc_cnt = ARRAY_SIZE(mt8195_ovl_crc_ofs),
> };
>
> static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {

Next message: Su Hui: "[PATCH net v2 1/2] octeontx2-pf: check negative error code in otx2_open()"
Previous message: Varadarajan Narayanan: "Re: [PATCH v2 3/4] clk: qcom: add IPQ9574 interconnect clocks support"
Next in thread: Shawn Sung (宋孝謙): "Re: [PATCH v6 13/14] drm/mediatek: Support CRC in OVL"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]