Re: [PATCH v6 02/11] drm/mediatek: Implement Display Stream Compression support

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

On Wed, 2026-07-15 at 15:56 +0200, AngeloGioacchino Del Regno wrote:
> Add a real driver for the Display Stream Compression (DSC) Display
> Controller IP, implementing support for DSC v1.1 to v1.2.
> 
> In order to do this, it was necessary to remove the basic DSC IP
> bypass setup from mtk_ddp_comp: this functionality is retained in
> the new mtk_disp_dsc driver, which checks if DSC was actually
> requested by other components (with the only one that currently
> supports this being DSI) and, if not, it will set BYPASS mode in
> the DSC IP.
> 
> Like before, the BYPASS mode is set before starting the DSC IP,
> but unlike before, this is being done in the component start
> callback instead of the config one.
> Notably, the config callback is called by mtk_crtc always
> immediately before the calling start callback, so the order of
> register writes is retained.
> The only real difference is that now this is being done through
> CPU writes instead of CMDQ, but since that's called only once
> and since it's just three registers, the performance impact will
> not be minimal and not even measurable.
> 
> As anticipated, DSC handling was also introduced in the mtk_dsi
> driver: when performing dsi_host_attach, the driver now checks
> if the DSI panel adds the DSC configuration structure to the
> mipi_dsi_device structure and, if it does, it will store a
> pointer in the driver-local mtk_dsi structure's `dsc` member.
> 
> The DSI driver will then check whether the DSC configuration
> that comes from the panel is valid (in regard to MediaTek DSI)
> and will call the DRM API's DSC helpers to calculate and set
> all of the const and RC parameters for the actual DSC setup.
> 
> For the time being, even though the latest MediaTek SoCs do
> support DSC v1.2, only DSC v1.1 pre-scr support is implemented
> as an initial contribution (which is rather big, and 1.2 would
> make it even bigger - but that can anyway be implemented later).
> 
> As a last step for validation of DSC parameters in DSI, a check
> for the hdisplay against DSC slice sidth and one for vdisplay

slice width

> against DSC slice height was added to the mode_valid callback,
> making sure that H/V are, as expected, multiples of slice W/H.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

[snip]

> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 679d413bf10b..5e2d8748120a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -12,6 +12,8 @@
>  #include "mtk_mdp_rdma.h"
>  #include "mtk_plane.h"
>  
> +struct drm_dsc_config;

#include <drm/display/drm_dsc.h>

> +
>  int mtk_aal_clk_enable(struct device *dev);
>  void mtk_aal_clk_disable(struct device *dev);
>  void mtk_aal_config(struct device *dev, unsigned int w,
> @@ -47,9 +49,16 @@ void mtk_dpi_start(struct device *dev);
>  void mtk_dpi_stop(struct device *dev);
>  unsigned int mtk_dpi_encoder_index(struct device *dev);
>  
> +int mtk_dsc_clk_enable(struct device *dev);
> +void mtk_dsc_clk_disable(struct device *dev);
> +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg);
> +void mtk_dsc_start(struct device *dev);
> +void mtk_dsc_stop(struct device *dev);
> +
>  void mtk_dsi_ddp_start(struct device *dev);
>  void mtk_dsi_ddp_stop(struct device *dev);
>  unsigned int mtk_dsi_encoder_index(struct device *dev);
> +struct drm_dsc_config *mtk_dsi_get_dsc_config(struct device *dev);
>  
>  int mtk_gamma_clk_enable(struct device *dev);
>  void mtk_gamma_clk_disable(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_dsc.c b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> new file mode 100644
> index 000000000000..bed6b77bf9a9
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021 MediaTek Inc.
> + * Copyright (c) 2025 Collabora Ltd
> + *                    AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +#include <drm/display/drm_dsc.h>
> +#include <drm/display/drm_dsc_helper.h>
> +
> +#include "mtk_crtc.h"
> +#include "mtk_ddp_comp.h"
> +#include "mtk_disp_drv.h"
> +
> +#define DISP_REG_DSC_CON		0x0
> +#  define DSC_EN			BIT(0)
> +#  define DSC_DUAL_INOUT		BIT(2)
> +#  define DSC_IN_SRC_SEL		BIT(3)
> +#  define DSC_BYPASS			BIT(4)
> +#  define DSC_RELAY			BIT(5)
> +#  define DSC_V1_1_EXT			BIT(6)
> +#  define DSC_PT_MEM_EN			BIT(7)
> +#  define DSC_SW_RESET			BIT(8)
> +#  define DSC_EMPTY_FLAG_SEL		GENMASK(15, 14)
> + #  define DSC_EMPTY_FLAG_NORMAL_DET	0
> + #  define DSC_EMPTY_FLAG_ALWAYS_HIGH	1
> + #  define DSC_EMPTY_FLAG_ALWAYS_LOW	2
> + #  define DSC_EMPTY_FLAG_DO_NOT_SEND	3
> +#  define DSC_UFOE_SEL			BIT(16)
> +#  define DSC_OUTPUT_SWAP		BIT(18)
> +#  define DSC_ZERO_FIFO_STALL_DISABLE	BIT(20)

Align register definition indent to OVL driver.

> +
> +#define DISP_REG_DSC_SPR		0x14
> +#define DISP_REG_DSC_PIC_W		0x18
> +#  define DSC_PIC_WIDTH			GENMASK(15, 0)
> +#  define DSC_PIC_GROUP_WIDTH_M1	GENMASK(31, 16)
> +

[snip]

> +
> +void mtk_dsc_setup(struct device *dev, struct drm_dsc_config *dsc_cfg)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +	u32 dsc_slice_w, dsc_slice_h, dsc_mode, dsc_cfg_rval, dsc_shadow;
> +	u32 dsc_dbg_con, dsc_con, dsc_enc_width, dsc_pic_w, dsc_pic_h;
> +	u32 pic_group_width, pic_height_ext_num, slice_group_width;
> +	u32 chunk_size, dsc_pad_num, dsc_pre_pad_sz;
> +	bool dsc_en_bit;
> +
> +	pic_height_ext_num = dsc_cfg->pic_height + dsc_cfg->slice_height - 1;
> +	pic_group_width = dsc_cfg->slice_width * 4;
> +	pic_group_width /= 3;

pic_group_width = dsc_cfg->slice_width * 4 / 3;

> +
> +	slice_group_width = dsc_cfg->slice_width + 2;
> +	slice_group_width /= 3;

slice_group_width = (dsc_cfg->slice_width + 2) / 3;

> +
> +	if (dsc_cfg->slice_chunk_size)
> +		chunk_size = dsc_cfg->slice_chunk_size;
> +	else
> +		chunk_size = dsc_cfg->slice_width * dsc_cfg->bits_per_pixel / 8 / 16;
> +
> +	dsc_enc_width = FIELD_PREP(DSC_ENC_WIDTH_PIC, dsc_cfg->pic_width) |
> +			FIELD_PREP(DSC_ENC_WIDTH_SLICE, dsc_cfg->slice_width);
> +
> +	dsc_pic_w = FIELD_PREP(DSC_PIC_GROUP_WIDTH_M1, pic_group_width - 1);
> +	dsc_pic_w |= FIELD_PREP(DSC_PIC_WIDTH, dsc_cfg->pic_width);
> +	dsc_pic_h = FIELD_PREP(DSC_PIC_HEIGHT_EXT_M1, pic_height_ext_num - 1);
> +	dsc_pic_h |= FIELD_PREP(DSC_PIC_HEIGHT, dsc_cfg->pic_height - 1);
> +
> +	dsc_slice_w = FIELD_PREP(DSC_SLICE_GROUP_WIDTH_M1, slice_group_width - 1);
> +	dsc_slice_w |= FIELD_PREP(DSC_SLICE_WIDTH, dsc_cfg->slice_width);
> +	dsc_slice_h = FIELD_PREP(DSC_SLICE_WIDTH_MOD3, dsc_cfg->slice_width % 3);
> +	dsc_slice_h |= FIELD_PREP(DSC_SLICE_NUM_M1,
> +				  (pic_height_ext_num / dsc_cfg->slice_height) - 1);
> +	dsc_slice_h |= FIELD_PREP(DSC_SLICE_HEIGHT_M1, dsc_cfg->slice_height - 1);
> +
> +	dsc_pad_num = (3 - ((chunk_size * 2) % 3)) % 3;
> +	dsc_pad_num = FIELD_PREP(DSC_PAD_NUMBER, dsc_pad_num);
> +
> +	dsc_pre_pad_sz = FIELD_PREP(DSC_PIC_PREPAD_HEIGHT, dsc_cfg->pic_height);
> +	dsc_pre_pad_sz |= FIELD_PREP(DSC_PIC_PREPAD_WIDTH, dsc_cfg->pic_width);
> +
> +	dsc_mode = FIELD_PREP(DSC_INIT_DELAY_HEIGHT, 4);
> +	dsc_mode |= FIELD_PREP(DSC_RGB_SWAP, 0);
> +
> +	/* Must enable checksum calc in DBG if enabling core checksum in CFG */
> +	dsc_cfg_rval = DSC_CFG_ICH_EN | DSC_CFG_CRC_EN | DSC_CFG_DSC12_BUGFIX |
> +		       DSC_CFG_CORE_CHECKSUM;
> +	dsc_dbg_con = DSC_CKSM_CAL_EN;
> +
> +	if (dsc_cfg->bits_per_component == 8)
> +		dsc_cfg_rval |= FIELD_PREP_CONST(DSC_CFG_FLATNESS_DET_THRES,
> +						 DSC_CFG_FLATNESS_8BITS);
> +	else
> +		dsc_cfg_rval |= FIELD_PREP_CONST(DSC_CFG_FLATNESS_DET_THRES,
> +						 DSC_CFG_FLATNESS_10BITS);
> +
> +	dsc_shadow = FIELD_PREP(DSC_SHADOW_DSC_VERSION_MINOR,
> +				dsc_cfg->dsc_version_minor);
> +	dsc_shadow |= DSC_FORCE_COMMIT | DSC_BYPASS_SHADOW;
> +
> +	/* If DSC is currently enabled, disable it before setup and re-enable after */
> +	dsc_con = readl(disp_dsc->reg + DISP_REG_DSC_CON);
> +	if (dsc_con & DSC_EN) {
> +		dsc_en_bit = true;
> +		writel(dsc_con & ~DSC_EN, disp_dsc->reg + DISP_REG_DSC_CON);
> +	} else {
> +		dsc_en_bit = false;
> +	}
> +
> +	writel(0, disp_dsc->reg + DISP_REG_DSC_SPR);
> +	writel(dsc_enc_width, disp_dsc->reg + DISP_REG_DSC_ENC_WIDTH);
> +	writel(dsc_pic_w, disp_dsc->reg + DISP_REG_DSC_PIC_W);
> +	writel(dsc_pic_h, disp_dsc->reg + DISP_REG_DSC_PIC_H);
> +	writel(dsc_slice_w, disp_dsc->reg + DISP_REG_DSC_SLICE_W);
> +	writel(dsc_slice_h, disp_dsc->reg + DISP_REG_DSC_SLICE_H);
> +	writel(((chunk_size * 4) / 3) << 16 | chunk_size,
> +	       disp_dsc->reg + DISP_REG_DSC_CHUNK_SIZE);
> +	writel(dsc_pre_pad_sz, disp_dsc->reg + DISP_REG_DSC_PIC_PRE_PAD_SIZE);
> +	writel(dsc_pad_num, disp_dsc->reg + DISP_REG_DSC_PAD);
> +	writel(FIELD_PREP(DISP_DSC_BUF_SIZE_MASK, chunk_size * dsc_cfg->slice_height),
> +	       disp_dsc->reg + DISP_REG_DSC_BUF_SIZE);
> +	writel(dsc_mode, disp_dsc->reg + DISP_REG_DSC_MODE);
> +	writel(dsc_cfg_rval, disp_dsc->reg + DISP_REG_DSC_CFG);
> +	writel(dsc_dbg_con, disp_dsc->reg + DISP_REG_DSC_DBG_CON);
> +	writel(FIELD_PREP_CONST(DSC_OBUF_SIZE, 1040), disp_dsc->reg + DISP_REG_DSC_OUTBUF);
> +	writel(dsc_shadow, disp_dsc->reg + DISP_REG_DSC_SHADOW);
> +
> +	/* Set PPS registers configuration */
> +	mtk_dsc_pps_setup(disp_dsc, dsc_cfg);
> +
> +	dsc_con = FIELD_PREP_CONST(DSC_EMPTY_FLAG_SEL, DSC_EMPTY_FLAG_ALWAYS_LOW);
> +	dsc_con |= DSC_V1_1_EXT | DSC_UFOE_SEL | DSC_PT_MEM_EN;
> +	dsc_con |= DSC_ZERO_FIFO_STALL_DISABLE;
> +
> +	if (dsc_en_bit)
> +		dsc_con |= DSC_EN;
> +
> +	writel(dsc_con, disp_dsc->reg + DISP_REG_DSC_CON);
> +
> +	disp_dsc->dsc_config_done = true;
> +}
> +
> +void mtk_dsc_start(struct device *dev)
> +{
> +	struct mtk_dsc *disp_dsc = dev_get_drvdata(dev);
> +
> +	/* If no DSC or config not done, set bypass mode */
> +	if (!disp_dsc->dsc_config_done) {
> +		mtk_ddp_write_mask(NULL, DSC_BYPASS, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_BYPASS);
> +		mtk_ddp_write_mask(NULL, DSC_UFOE_SEL, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_UFOE_SEL);
> +		mtk_ddp_write_mask(NULL, DSC_DUAL_INOUT, &disp_dsc->cmdq_reg,
> +				   disp_dsc->reg, DISP_REG_DSC_CON, DSC_DUAL_INOUT);
> +	}
> +
> +	mtk_ddp_write_mask(NULL, DSC_EN, &disp_dsc->cmdq_reg,
> +			   disp_dsc->reg, DISP_REG_DSC_CON, DSC_EN);

When cmdq_pkt is NULL, it's not necessary to use mtk_ddp_write_mask with parameter cmdq_reg.
The original mtk_dsc_start() should modify this.
Because this patch let DSC setting not in vblank, so this driver does not need cmdq.
Let's drop cmdq in this patch.

> +}
> +

[snip]

>  
> -static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
> +static void mtk_dsi_ps_control_dsc(struct mtk_dsi *dsi, bool config_vact)
> +{
> +	const struct mtk_dsi_driver_data *data = dsi->driver_data;
> +	const u16 *reg_main = dsi->driver_data->reg_main;

reg_main is defined in patch [6/11], so move this patch after that patch or not use reg_main in this patch.

> +	const short dsi_buf_bpp = 3;
> +	u32 ps_wc;
> +
> +	/* Word count */
> +	ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size);
> +
> +	if (config_vact) {
> +		writel(FIELD_PREP(VACT_NL, dsi->vm.vactive),
> +		       dsi->regs + reg_main[DSI_VACT_NL]);
> +		writel(ps_wc, dsi->regs + reg_main[DSI_HSTX_CKL_WC]);
> +	}
> +
> +	/* Always use DSC Pixel Stream type */
> +	writel(ps_wc | FIELD_PREP(DSI_PS_SEL, COMPRESSED_PS_DSC),
> +	       dsi->regs + reg_main[DSI_PSCTRL]);
> +
> +	if (data->has_size_ctl)
> +		writel(FIELD_PREP(DSI_HEIGHT, dsi->vm.vactive) |
> +		       FIELD_PREP(DSI_WIDTH, (ps_wc + dsi_buf_bpp - 1) / dsi_buf_bpp),
> +		       dsi->regs + reg_main[DSI_SIZE_CON]);
> +}
> +
> +static void mtk_dsi_ps_control_uncompressed(struct mtk_dsi *dsi, bool config_vact)
>  {
> -	u32 dsi_buf_bpp, ps_val, ps_wc, vact_nl;
> +	u32 dsi_buf_bpp, ps_val, ps_wc, size_val, vact_nl;
>  
>  	if (dsi->format == MIPI_DSI_FMT_RGB565)
>  		dsi_buf_bpp = 2;
> @@ -430,6 +460,21 @@ static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact)
>  		writel(ps_wc, dsi->regs + DSI_HSTX_CKL_WC);
>  	}
>  	writel(ps_val, dsi->regs + DSI_PSCTRL);
> +
> +	if (dsi->driver_data->has_size_ctl) {
> +		size_val = FIELD_PREP(DSI_HEIGHT, dsi->vm.vactive);
> +		size_val |= FIELD_PREP(DSI_WIDTH, dsi->vm.hactive);
> +
> +		writel(size_val, dsi->regs + DSI_SIZE_CON);
> +	}

You have move below code to here. Maybe keep the original statement and size_val could be drop.

	if (dsi->driver_data->has_size_ctl)
		writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
			FIELD_PREP(DSI_WIDTH, vm->hactive),
			dsi->regs + DSI_SIZE_CON);

> +}
> +

[snip]

> +
> +static int mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
>  {
>  	struct videomode *vm = &dsi->vm;
> +	int ret;
>  
>  	writel(vm->vsync_len, dsi->regs + DSI_VSA_NL);
>  	writel(vm->vback_porch, dsi->regs + DSI_VBP_NL);
>  	writel(vm->vfront_porch, dsi->regs + DSI_VFP_NL);
>  	writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>  
> -	if (dsi->driver_data->has_size_ctl)
> -		writel(FIELD_PREP(DSI_HEIGHT, vm->vactive) |
> -			FIELD_PREP(DSI_WIDTH, vm->hactive),
> -			dsi->regs + DSI_SIZE_CON);
> -
>  	if (dsi->driver_data->support_per_frame_lp)
>  		mtk_dsi_config_vdo_timing_per_frame_lp(dsi);
>  	else
>  		mtk_dsi_config_vdo_timing_per_line_lp(dsi);
>  
> -	mtk_dsi_ps_control(dsi, false);
> +	if (dsi->dsc) {
> +		ret = mtk_dsi_set_dsc_params(dsi);
> +		if (ret)
> +			return ret;
> +
> +		mtk_dsi_ps_control(dsi, true);

The original code use 'false' for second parameter. Why here use 'true'?

Regards,
CK

> +	} else {
> +		mtk_dsi_ps_control(dsi, false);
> +	}
> +
> +	return 0;
>  }
>  
>