(Sorry for the resend, I made a mistake with the formatting and the recipient list)
On 9/15/26 11:41 AM, 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 > 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 > <[email protected]> Hello, I have been testing previous versions of this patch on an MT6858 device with a DSC-only panel. Since I spotted a few issues with the code, I decided to leave a few comments here even though I am not a maintainer. Some of them are already mentioned in the Sashiko report. [...] > diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c > b/drivers/gpu/drm/mediatek/mtk_crtc.c > index 97e3ff412e6e..c0a53c153b8d 100644 > --- a/drivers/gpu/drm/mediatek/mtk_crtc.c > +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c > @@ -22,6 +22,7 @@ > > #include "mtk_crtc.h" > #include "mtk_ddp_comp.h" > +#include "mtk_disp_drv.h" > #include "mtk_drm_drv.h" > #include "mtk_plane.h" > > @@ -344,6 +345,8 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc *mtk_crtc) > struct drm_connector *connector; > struct drm_encoder *encoder; > struct drm_connector_list_iter conn_iter; > + struct mtk_ddp_comp *comp_dsi = NULL, *comp_dsc = NULL; > + struct drm_dsc_config *dsc_cfg; > struct drm_device *dev = mtk_crtc->base.dev; > unsigned int width, height, vrefresh, bpc = MTK_MAX_BPC; > int ret; > @@ -398,6 +401,17 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc > *mtk_crtc) This is inside a for loop that skips the last item: for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) { > if (!mtk_ddp_comp_add(mtk_crtc->ddp_comp[i], mtk_crtc->mutex)) > mtk_mutex_add_comp(mtk_crtc->mutex, > mtk_crtc->ddp_comp[i]->id); > + > + /* For now, only single DSI is supported */ > + if (mtk_crtc->ddp_comp[i]->id >= DDP_COMPONENT_DSI0 && > + mtk_crtc->ddp_comp[i]->id <= DDP_COMPONENT_DSI3) > + if (!comp_dsi) > + comp_dsi = mtk_crtc->ddp_comp[i]; > + > + if (mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC0 || > + mtk_crtc->ddp_comp[i]->id == DDP_COMPONENT_DSC1) > + if (!comp_dsc) > + comp_dsc = mtk_crtc->ddp_comp[i]; If DSI is the last component, it would never be considered as a candidate for DSC setup. Shouldn't this be in a loop that covers all components? > } > if (!mtk_ddp_comp_add(mtk_crtc->ddp_comp[i], mtk_crtc->mutex)) > mtk_mutex_add_comp(mtk_crtc->mutex, mtk_crtc->ddp_comp[i]->id); > @@ -413,6 +427,13 @@ static int mtk_crtc_ddp_hw_init(struct mtk_crtc > *mtk_crtc) > mtk_ddp_comp_start(comp); > } > > + /* Setup the DSC if present, with the config coming from DSI */ > + if (comp_dsc && comp_dsi) { > + dsc_cfg = mtk_dsi_get_dsc_config(comp_dsi->dev); > + if (dsc_cfg) > + mtk_ddp_comp_dsc_setup(comp_dsc, dsc_cfg); > + } > + > /* Initially configure all planes */ > for (i = 0; i < mtk_crtc->layer_nr; i++) { > struct drm_plane *plane = &mtk_crtc->planes[i]; [...] > 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..090f0609a14c > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_disp_dsc.c [...] > + > +#define DISP_REG_DSC_DBG_CON 0x60 > +# define DSC_CKSM_CAL_EN BIT(9) > + > +#define DISP_REG_DSC_OUTBUF 0x70 > +# define DSC_OBUF_SIZE GENMASK(11, 0) > + > +#define DISP_REG_DSC_PPS(x) (0x80 + (x * 4)) /* 0..19 */ This causes an operator precedence bug when you use DISP_REG_DSC_PPS(8 + i) below. Wrap the argument in parentheses: #define DISP_REG_DSC_PPS(x) (0x80 + ((x) * 4))) > +# define DSC_P0_UP_LINE_BUF_DEPTH GENMASK(3, 0) > +# define DSC_P0_BPC GENMASK(7, 4) > +# define DSC_P0_BPP GENMASK(17, 8) > +# define DSC_P0_RCT_ON BIT(18) > +# define DSC_P0_BLOCK_PRED_EN BIT(19) [...] > +static void mtk_dsc_pps_setup(struct mtk_dsc *disp_dsc, struct > drm_dsc_config *dsc_cfg) > +{ > + struct drm_dsc_rc_range_parameters *rcrp = dsc_cfg->rc_range_params; > + u16 *rbt = dsc_cfg->rc_buf_thresh; > + u32 data; > + int i, j; > + > + /* PPS 0 - Note: Fractional BPP is not supported! */ > + data = FIELD_PREP(DSC_P0_UP_LINE_BUF_DEPTH, dsc_cfg->line_buf_depth); > + data |= FIELD_PREP(DSC_P0_BPC, dsc_cfg->bits_per_component); > + data |= FIELD_PREP(DSC_P0_BPP, dsc_cfg->bits_per_pixel); > + data |= DSC_P0_RCT_ON | DSC_P0_BLOCK_PRED_EN; > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(0)); > + > + /* PPS 1 */ > + data = FIELD_PREP(DSC_P1_INITIAL_XMIT_DELAY, > dsc_cfg->initial_xmit_delay); > + data |= FIELD_PREP(DSC_P1_INITIAL_DEC_DELAY, > dsc_cfg->initial_dec_delay); > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(1)); > + > + /* PPS 2 */ > + data = FIELD_PREP(DSC_P2_INITIAL_SCALE_VALUE, > dsc_cfg->initial_scale_value); > + data |= FIELD_PREP(DSC_P2_SCALE_INCR_INTERVAL, > dsc_cfg->scale_increment_interval); > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(2)); Is RC_MODEL_SIZE omitted here on purpose? > + > + /* PPS 3 */ > + data = FIELD_PREP(DSC_P3_SCALE_DECR_INTERVAL, > dsc_cfg->scale_decrement_interval); > + data |= FIELD_PREP(DSC_P3_FIRST_LINE_BPG_OFFSET, > dsc_cfg->first_line_bpg_offset); > + writel(data, disp_dsc->reg + DISP_REG_DSC_PPS(3)); [...] > + > +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) / 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); This register has a field called SLICE_MODE (bit 0) that should be set when there is more than one slice. Since the code above already configures the slice count, this should probably be enabled. (Maybe only some SoCs require it, but I wasn't able to get it working on MT6858 without this.) > + > + /* 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); [...] > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c > b/drivers/gpu/drm/mediatek/mtk_dsi.c > index f82506e10fd5..97b8a91874f5 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -18,6 +18,8 @@ > #include <video/mipi_display.h> > #include <video/videomode.h> > > +#include <drm/display/drm_dsc.h> > +#include <drm/display/drm_dsc_helper.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > #include <drm/drm_bridge_connector.h> > @@ -71,11 +73,12 @@ > > #define DSI_PSCTRL 0x1c > #define DSI_PS_WC GENMASK(13, 0) > -#define DSI_PS_SEL GENMASK(17, 16) > +#define DSI_PS_SEL GENMASK(19, 16) > #define PACKED_PS_16BIT_RGB565 0 > #define PACKED_PS_18BIT_RGB666 1 > #define LOOSELY_PS_24BIT_RGB666 2 > #define PACKED_PS_24BIT_RGB888 3 > +#define COMPRESSED_PS_DSC 5 > > #define DSI_VSA_NL 0x20 > #define DSI_VBP_NL 0x24 > @@ -203,6 +206,7 @@ struct mtk_dsi { > struct drm_bridge bridge; > struct drm_bridge *next_bridge; > struct drm_connector *connector; > + struct drm_dsc_config *dsc; > struct phy *phy; > > void __iomem *regs; > @@ -393,9 +397,35 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi) > writel(regval, dsi->regs + DSI_TXRX_CTRL); > } > > -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; > + const short dsi_buf_bpp = 3; > + u32 ps_wc; > + > + /* Word count */ > + ps_wc = FIELD_PREP(DSI_PS_WC, 2 * dsi->dsc->slice_chunk_size); This assumes that there are exactly two slices. That is often the case, but the number of slices may vary depending on the width. I think this would be a good place to use dsi->dsc->slice_count. > + > + 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); > + } > +} > + > +static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) > +{ > + if (dsi->dsc) > + mtk_dsi_ps_control_dsc(dsi, config_vact); > + else > + mtk_dsi_ps_control_uncompressed(dsi, config_vact); > } > > static void mtk_dsi_config_vdo_timing_per_frame_lp(struct mtk_dsi *dsi) > @@ -565,26 +610,68 @@ static void > mtk_dsi_config_vdo_timing_per_line_lp(struct mtk_dsi *dsi) > writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC); > } > > -static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > +static int mtk_dsi_set_dsc_params(struct mtk_dsi *dsi) > +{ > + struct drm_dsc_config *dsc = dsi->dsc; > + struct device *dev = dsi->host.dev; > + int ret; > + > + if (dsc->bits_per_pixel & GENMASK(3, 0)) { > + dev_err(dev, "Fractional bits_per_pixel not supported\n"); > + return -EINVAL; > + } > + > + if (dsc->bits_per_component != 8) { > + dev_err(dev, "%u bits per component is not supported\n", > + dsc->bits_per_component); > + return -EINVAL; > + } Any reason why this wouldn't support 10bpc? The rest of the code seems to handle it fine. > + > + dsc->simple_422 = false; > + dsc->convert_rgb = true; > + dsc->vbr_enable = false; > + > + drm_dsc_set_const_params(dsc); > + drm_dsc_set_rc_buf_thresh(dsc); > + > + ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR); > + if (ret) { > + dev_err(dev, "Cannot find DSC RC params\n"); > + return ret; > + } The defaults in MediaTek's downstream driver match DRM_DSC_1_2_444, not DRM_DSC_1_1_PRE_SCR, and the panel on my test device requires that even though it uses DSC v1.1. This may be a panel-specific problem, but have you tested this with any actual panels that need 1_1_PRE_SCR? > + > + dsc->initial_scale_value = drm_dsc_initial_scale_value(dsc); > + dsc->line_buf_depth = dsc->bits_per_component + 1; > + > + return drm_dsc_compute_rc_parameters(dsc); > +} > + > +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); > + } else { > + mtk_dsi_ps_control(dsi, false); > + } I don't quite understand what is going on here, even in the original driver before this patch. What is the point of setting VACT_NL twice? Does mtk_dsi_ps_control() really need to be called in mtk_dsi_poweron() if mtk_dsi_config_vdo_timing() ends up calling it a second time? If the order is not important, perhaps this would be cleaner: mtk_dsi_poweron() - mtk_dsi_config_vdo_timing() - set VSA_NL, VBP_NL, VFP_NL, VACT_NL, SIZE_CON - mtk_dsi_set_dsc_params() - mtk_dsi_ps_control_{dsc|uncompressed}() - mtk_dsi_config_vdo_timing_per_{frame|line}_lp() One problem is that mtk_dsi_config_vdo_timing_per_frame_lp() currently doesn't take DSC into account when computing ps_wc, resulting in wrong timings. Another problem is that the data rate calculation in mtk_dsi_poweron() is also unconditionally using the bits-per-pixel value from the color format. To get a correct frame rate with DSC, it would need to do something similar to dsi_adjust_pclk_for_compression() in drivers/gpu/drm/msm/dsi/dsi_host.c. Other than that, the DSC support seems to work well so far. Thank you for the useful contribution! Best regards, Nikolai P.S.: I can try to push my changes and workarounds to a git tree if needed.
