From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 26C163DA5B4; Tue, 15 Sep 2026 15:05:41 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789484745; cv=none; b=kxT7b2DLX1J22HPLqo9sj71Tvto+Goevml9Oa2TeQ8k+da8xZeXLPoRv6LP4CgDi1YSdSyVN3Vfj7mzeXIOyCywr9Ja5bDentva7wgRvB3SVLCE/qRo7TtyTyBcoK7gEgRD3a1ziDrvEFlH3GqJsCYi5MQlbmwvw0+yHRO3LEB8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789484745; c=relaxed/simple; bh=pN+y+ua7LcQO4k08fzswufOynoNn3Gegmsif93HWJfw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=IPOpHW6tV6LyBjf5PP+E+XdVQDgI4/gTYEyIhWDDGSBlmjR3AKNINnFm4sDtdYtyOzZ42BoVnLpodv7IvGbESZMgTk43tWvquaIgVk0PoXvLCk/F553SjcUf01S5fnb7AuSdv40Ghs8f2n/RDmjQ06Ov6r7/oDCEUzwE7MoNQnw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=G/D51xBP; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="G/D51xBP" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1789484740; bh=pN+y+ua7LcQO4k08fzswufOynoNn3Gegmsif93HWJfw=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=G/D51xBPXsh/78g+ra+ytqPS9njN+2QjXQlko8dJ3FUYq32RQHTYCEd4pqbfbAPBR r5qbbj/lwRONctQ5LMiAlG3fG2pcP8OvNrPoHFvEiwBEnzU4oufy47+tqaUE/ZezoD owr41VHIE0Ry1zdqS48Ry2A23xFXypqfLnwH+Uvkfi5iOoR2CeH7fqxPLAmY/Nt9Sf LAeCx2myTG2grgKNZFiP/fEk7RvoA+cGWqhgHLWzJduQ5C7Gf8f2F7kRA10q2mk9aN Pqh+fnpjZffqewNoWNo7LDLok6Mc5m+lbBxA0qRS4Yq037IYSuhEP/JTKie3v0Kz2r wOzPAmiBHyzUg== Received: from [100.64.1.21] (unknown [100.64.1.21]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange x25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: kholk11) by bali.collaboradmins.com (Postfix) with ESMTPSA id 4514817E0082; Tue, 15 Sep 2026 17:05:39 +0200 (CEST) Message-ID: <14b99e0a-e08a-42af-b982-471cae869149@collabora.com> Date: Tue, 15 Sep 2026 17:05:38 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v8 02/13] drm/mediatek: Implement Display Stream Compression support To: Nikolai Burov , "chunkuang.hu@kernel.org" Cc: "p.zabel@pengutronix.de" , "maarten.lankhorst@linux.intel.com" , "mripard@kernel.org" , "tzimmermann@suse.de" , "airlied@gmail.com" , "simona@ffwll.ch" , "robh@kernel.org" , "krzk+dt@kernel.org" , "conor+dt@kernel.org" , "matthias.bgg@gmail.com" , "jitao.shi@mediatek.com" , "dri-devel@lists.freedesktop.org" , "linux-mediatek@lists.infradead.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "kernel@collabora.com" , "justin.yeh@mediatek.com" , "jason-jh.lin@mediatek.com" References: <20260915084148.11385-1-angelogioacchino.delregno@collabora.com> <20260915084148.11385-3-angelogioacchino.delregno@collabora.com> From: AngeloGioacchino Del Regno Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 9/15/26 15:55, Nikolai Burov wrote: > (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 > > 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. > Hey, thanks for your comments! > [...] >> 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? > That's my own mess during conflict resolution.... as that was supposed to be in the latter loop instead, ugh. Just 7-8 lines above... I guess that's what happens when carrying things and rebasing continuously... Though, that worked for me because I tested that on the restructured mediatek-drm code which, in turn, resolves this bug due to the refactoring. Oops again. >> } >> 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))) > Whoops. Yup, that's right. >> +# 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? > Nope, that was not on purpose. Added, but it's on PPS6, not on PPS2. >> + >> + /* 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.) > No idea, but mine worked without (MT8196 Chromebook), and works with SLICE_MODE set as you say as well, so that's nice. >> + >> + /* 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