Re: [PATCH 3/7] drm/mediatek: Add TDSHP component support for MT8196

CK Hu (胡俊光)
2025-02-26T07:40:23Z

On Wed, 2025-02-19 at 17:20 +0800, Jay Liu wrote:
> Add TDSHP component support for MT8196.
> TDSHP is a hardware module designed to enhance the sharpness and
> clarity of displayed images by analyzing and improving edges and
> fine details in frames.
>
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_ddp_comp.c | 58 +++++++++++++++++++++++++
> drivers/gpu/drm/mediatek/mtk_ddp_comp.h | 1 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +
> 3 files changed, 61 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> index d7e230bac53e..b87fde64ee49 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.c
> @@ -57,6 +57,16 @@
> #define POSTMASK_RELAY_MODE BIT(0)
> #define DISP_REG_POSTMASK_SIZE 0x0030
>
> +#define DISP_REG_TDSHP_EN 0x0000
> +#define DISP_TDSHP_TDS_EN BIT(31)
> +#define DISP_REG_TDSHP_CTRL 0x0100
> +#define DISP_TDSHP_CTRL_EN BIT(0)
> +#define DISP_TDSHP_PWR_SCL_EN BIT(2)
> +#define DISP_REG_TDSHP_CFG 0x0110
> +#define DISP_REG_TDSHP_INPUT_SIZE 0x0120
> +#define DISP_REG_TDSHP_OUTPUT_OFFSET 0x0124
> +#define DISP_REG_TDSHP_OUTPUT_SIZE 0x0128
> +
> #define DISP_REG_UFO_START 0x0000
> #define UFO_BYPASS BIT(2)
>
> @@ -261,6 +271,44 @@ static void mtk_postmask_stop(struct device *dev)
> writel_relaxed(0x0, priv->regs + DISP_REG_POSTMASK_EN);
> }
>
> +static void mtk_disp_tdshp_config(struct device *dev, unsigned int w,
> + unsigned int h, unsigned int vrefresh,
> + unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
> +{
> + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> + u32 tdshp_ctrl = (bpc == 8) ? DISP_TDSHP_PWR_SCL_EN | DISP_TDSHP_CTRL_EN : 0;
> +
> + mtk_ddp_write(cmdq_pkt, tdshp_ctrl, &priv->cmdq_reg, priv->regs,
> + DISP_REG_TDSHP_CTRL);
> +
> + mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> + DISP_REG_TDSHP_INPUT_SIZE);
> + mtk_ddp_write(cmdq_pkt, w << 16 | h, &priv->cmdq_reg, priv->regs,
> + DISP_REG_TDSHP_OUTPUT_SIZE);
> + mtk_ddp_write(cmdq_pkt, 0x0, &priv->cmdq_reg, priv->regs,
> + DISP_REG_TDSHP_OUTPUT_OFFSET);
> +
> + mtk_ddp_write(cmdq_pkt, 0x1, &priv->cmdq_reg,
> + priv->regs, DISP_REG_TDSHP_CFG);

Symbolize 0x1.

> +
> + mtk_ddp_write_mask(cmdq_pkt, DISP_TDSHP_TDS_EN, &priv->cmdq_reg, priv->regs,
> + DISP_REG_TDSHP_EN, DISP_TDSHP_TDS_EN);

What's the function of DISP_TDSHP_TDS_EN?
I think default would should not enhance sharpness because it may cause some side effect for some image.

> +}
> +
> +static void mtk_disp_tdshp_start(struct device *dev)
> +{
> + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> + writel(DISP_TDSHP_CTRL_EN, priv->regs + DISP_REG_TDSHP_CTRL);

I don't know why you set DISP_TDSHP_CTRL_EN both here and in mtk_disp_tdshp_config().
If DISP_TDSHP_CTRL_EN is used to enable tdshp, why mtk_disp_tdshp_config() may disable tdshp in some case?

Regards,
CK

> +}
> +
> +static void mtk_disp_tdshp_stop(struct device *dev)
> +{
> + struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> +
> + writel(0x0, priv->regs + DISP_REG_TDSHP_CTRL);
> +}
> +
> static void mtk_ufoe_start(struct device *dev)
> {
> struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
> @@ -268,6 +316,14 @@ static void mtk_ufoe_start(struct device *dev)
> writel(UFO_BYPASS, priv->regs + DISP_REG_UFO_START);
> }
>
> +static const struct mtk_ddp_comp_funcs ddp_tdshp = {
> + .clk_enable = mtk_ddp_clk_enable,
> + .clk_disable = mtk_ddp_clk_disable,
> + .config = mtk_disp_tdshp_config,
> + .start = mtk_disp_tdshp_start,
> + .stop = mtk_disp_tdshp_stop,
> +};
> +
> static const struct mtk_ddp_comp_funcs ddp_aal = {
> .clk_enable = mtk_aal_clk_enable,
> .clk_disable = mtk_aal_clk_disable,
> @@ -440,6 +496,7 @@ static const char * const mtk_ddp_comp_stem[MTK_DDP_COMP_TYPE_MAX] = {
> [MTK_DISP_POSTMASK] = "postmask",
> [MTK_DISP_PWM] = "pwm",
> [MTK_DISP_RDMA] = "rdma",
> + [MTK_DISP_TDSHP] = "tdshp",
> [MTK_DISP_UFOE] = "ufoe",
> [MTK_DISP_WDMA] = "wdma",
> [MTK_DP_INTF] = "dp-intf",
> @@ -495,6 +552,7 @@ static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_DRM_ID_MAX]
> [DDP_COMPONENT_RDMA1] = { MTK_DISP_RDMA, 1, &ddp_rdma },
> [DDP_COMPONENT_RDMA2] = { MTK_DISP_RDMA, 2, &ddp_rdma },
> [DDP_COMPONENT_RDMA4] = { MTK_DISP_RDMA, 4, &ddp_rdma },
> + [DDP_COMPONENT_TDSHP0] = { MTK_DISP_TDSHP, 0, &ddp_tdshp },
> [DDP_COMPONENT_UFOE] = { MTK_DISP_UFOE, 0, &ddp_ufoe },
> [DDP_COMPONENT_WDMA0] = { MTK_DISP_WDMA, 0, NULL },
> [DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL },
> diff --git a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> index 39720b27f4e9..fc90d32b8c2e 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_ddp_comp.h
> @@ -38,6 +38,7 @@ enum mtk_ddp_comp_type {
> MTK_DISP_POSTMASK,
> MTK_DISP_PWM,
> MTK_DISP_RDMA,
> + MTK_DISP_TDSHP,
> MTK_DISP_UFOE,
> MTK_DISP_WDMA,
> MTK_DPI,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index f22ad2882697..4ec09864bc8c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -790,6 +790,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
> .data = (void *)MTK_DISP_RDMA },
> { .compatible = "mediatek,mt8195-disp-rdma",
> .data = (void *)MTK_DISP_RDMA },
> + { .compatible = "mediatek,mt8196-disp-tdshp",
> + .data = (void *)MTK_DISP_TDSHP },
> { .compatible = "mediatek,mt8173-disp-ufoe",
> .data = (void *)MTK_DISP_UFOE },
> { .compatible = "mediatek,mt8173-disp-wdma",