<html><body><p> <pre> Hi, Shuijing: On Wed, 2024-04-24 at 17:16 +0800, Shuijing Li wrote: > Adding the per-frame lp function of mt8188, which can keep HFP in HS and > reduce the time required for each line to enter and exit low power. > Per Frame LP: > |<----------One Active Frame-------->| > --______________________________________----___________________ > ^HSA+HBP^^RGB^^HFP^^HSA+HBP^^RGB^^HFP^ ^HSA+HBP^^RGB^^HFP^ > > Per Line LP: > |<---------------One Active Frame----------->| > --______________--______________--______________----______________ > ^HSA+HBP^^RGB^ ^HSA+HBP^^RGB^ ^HSA+HBP^^RGB^ ^HSA+HBP^^RGB^ > > Signed-off-by: Shuijing Li <shuijing.li@mediatek.com> > --- > Changes in v2: > Use bitfield macros and add new function for per prame lp and improve > the format, per suggestion frome previous thread: > https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20240314094238.3315-1-shuijing.li@mediatek.com/__;!!CTRNKA9wMg0ARbw!iJrqnLwBi80AYshU8-sTvs0LiPH_URRrjTLsXFQLyUyAWZW0x5B0lCgIKTXl-wS9NmkTBcIdyncDMxDio1jk8w$ > --- > drivers/gpu/drm/mediatek/mtk_dsi.c | 143 +++++++++++++++++++++++++++++ > 1 file changed, 143 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c > index 9501f4019199..75719b0535f7 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c > @@ -68,6 +68,8 @@ > #define EXT_TE_EDGEBIT(10) > #define MAX_RTN_SIZEGENMASK(15, 12) > #define HSTX_CKLP_ENBIT(16) > +#define HSTX_BLLP_EN_SHIFT 7 > +#define HSTX_BLLP_EN_MASK 0x1 > > #define DSI_PSCTRL0x1c > #define DSI_PS_WCGENMASK(13, 0) > @@ -76,6 +78,8 @@ > #define PACKED_PS_18BIT_RGB6661 > #define LOOSELY_PS_24BIT_RGB6662 > #define PACKED_PS_24BIT_RGB8883 > +#define PS_WC_SHIFT 0 > +#define PS_WC_MASK 0x7fff > > #define DSI_VSA_NL0x20 > #define DSI_VBP_NL0x24 > @@ -88,12 +92,20 @@ > #define DSI_HSA_WC0x50 > #define DSI_HBP_WC0x54 > #define DSI_HFP_WC0x58 > +#define HFP_HS_EN31 > +#define HFP_HS_VB_PS_WC_SHIFT 16 > + > +#define DSI_BLLP_WC0x5C > +#define BLLP_WC_SHIFT 0 > +#define BLLP_WC_MASK 0xfff > > #define DSI_CMDQ_SIZE0x60 > #define CMDQ_SIZE0x3f > #define CMDQ_SIZE_SELBIT(15) > > #define DSI_HSTX_CKL_WC0x64 > +#define HSTX_CKL_WC_SHIFT 2 > +#define HSTX_CKL_WC_MASK 0x3fff > > #define DSI_RX_DATA00x74 > #define DSI_RX_DATA10x78 > @@ -118,12 +130,22 @@ > #define HS_PREPGENMASK(15, 8) > #define HS_ZEROGENMASK(23, 16) > #define HS_TRAILGENMASK(31, 24) > +#define LPX_SHIFT 0 > +#define LPX_MASK 0xff > +#define DA_HS_PREP_SHIFT 8 > +#define DA_HS_PREP_MASK 0xff > +#define DA_HS_ZERO_SHIFT 16 > +#define DA_HS_ZERO_MASK 0xff > +#define DA_HS_TRAIL_SHIFT 24 > +#define DA_HS_TRAIL_MASK 0xff > > #define DSI_PHY_TIMECON10x114 > #define TA_GOGENMASK(7, 0) > #define TA_SUREGENMASK(15, 8) > #define TA_GETGENMASK(23, 16) > #define DA_HS_EXITGENMASK(31, 24) > +#define DA_HS_EXIT_SHIFT 24 > +#define DA_HS_EXIT_MASK 0xff > > #define DSI_PHY_TIMECON20x118 > #define CONT_DETGENMASK(7, 0) > @@ -154,6 +176,7 @@ > #define DATA_1GENMASK(31, 24) > > #define NS_TO_CYCLE(n, c) ((n) / (c) + (((n) % (c)) ? 1 : 0)) > +#define REG_FIELD_VALUE(reg, field) (((reg) >> (field##_SHIFT)) & (field##_MASK)) Use function in bitfield.h instead of re-inventing it. > > #define MTK_DSI_HOST_IS_READ(type) \ > ((type == MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM) || \ > @@ -187,6 +210,7 @@ struct mtk_dsi_driver_data { > bool has_shadow_ctl; > bool has_size_ctl; > bool cmdq_long_packet_ctl; > +bool support_per_frame_lp; > }; > > struct mtk_dsi { > @@ -425,6 +449,121 @@ static void mtk_dsi_ps_control(struct mtk_dsi *dsi, bool config_vact) > writel(ps_val, dsi->regs + DSI_PSCTRL); > } > > +static void mtk_dsi_config_vdo_timing_per_frame_lp(struct mtk_dsi *dsi) > +{ > +u32 horizontal_sync_active_byte; > +u32 horizontal_backporch_byte; > +u32 horizontal_frontporch_byte; > +u32 dsi_tmp_buf_bpp; > +unsigned int lpx, da_hs_exit, da_hs_prep, da_hs_trail; > +unsigned int da_hs_zero, ps_wc, hs_vb_ps_wc; > +u32 bllp_wc, bllp_en, v_active_roundup, hstx_cklp_wc; > +u32 hstx_cklp_wc_max, hstx_cklp_wc_min; > +struct videomode *vm = &dsi->vm; > + > +if (dsi->format == MIPI_DSI_FMT_RGB565) > +dsi_tmp_buf_bpp = 2; > +else > +dsi_tmp_buf_bpp = 3; > + > +da_hs_trail = REG_FIELD_VALUE(readl(dsi->regs + DSI_PHY_TIMECON0), > +DA_HS_TRAIL); da_hs_trail = dsi->phy_timing.clk_hs_trail; > +bllp_en = REG_FIELD_VALUE(readl(dsi->regs + DSI_TXRX_CTRL), > +HSTX_BLLP_EN); Where do you define HSTX_BLLP_EN? DSI_TXRX_CTRL is written in mtk_dsi_txrx_control() and I guess that bllp_en is zero, right? If bllp_en is zero, drop the code related to bllp_en. > +if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE) { > +horizontal_sync_active_byte = > +vm->hsync_len * dsi_tmp_buf_bpp - 10; > +horizontal_backporch_byte = > +vm->hback_porch * dsi_tmp_buf_bpp - 10; > +horizontal_frontporch_byte = > +vm->hfront_porch * dsi_tmp_buf_bpp - 12; > + > +ps_wc = REG_FIELD_VALUE(readl(dsi->regs + DSI_PSCTRL), PS_WC); ps_wc = dsi->vm.hactive * dsi_buf_bpp; > +v_active_roundup = (32 + horizontal_sync_active_byte + > +horizontal_backporch_byte + ps_wc + > +horizontal_frontporch_byte) % dsi->lanes; > +if (v_active_roundup) > +horizontal_backporch_byte = horizontal_backporch_byte + > +dsi->lanes - v_active_roundup; Is this roundup the same as the one in mtk_dsi_config_vdo_timing()? If the same, merge these two. > +hstx_cklp_wc_min = (DIV_ROUND_UP((12 + 2 + 4 + > +horizontal_sync_active_byte), dsi->lanes) + da_hs_trail + 1) > +* dsi->lanes / 6 - 1; > +hstx_cklp_wc_max = (DIV_ROUND_UP((20 + 6 + 4 + > +horizontal_sync_active_byte + horizontal_backporch_byte + > +ps_wc), dsi->lanes) + da_hs_trail + 1) * dsi->lanes / 6 - 1; > +} else { > +horizontal_sync_active_byte = vm->hsync_len * dsi_tmp_buf_bpp - 4; > + > +horizontal_backporch_byte = (vm->hback_porch + vm->hsync_len) * > +dsi_tmp_buf_bpp - 10; > +hstx_cklp_wc_min = (DIV_ROUND_UP(4, dsi->lanes) + da_hs_trail + 1) > +* dsi->lanes / 6 - 1; > + > +if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) { > +ps_wc = REG_FIELD_VALUE(readl(dsi->regs + DSI_PSCTRL), PS_WC); > +bllp_wc = REG_FIELD_VALUE(readl(dsi->regs + DSI_BLLP_WC), BLLP_WC); This register is never written. Which setting would influence this value? Before dsi configuration finish, is this value stable? > +horizontal_frontporch_byte = (vm->hfront_porch * > +dsi_tmp_buf_bpp - 18); > + > +v_active_roundup = (28 + horizontal_backporch_byte + ps_wc + > +horizontal_frontporch_byte + bllp_wc) % dsi->lanes; > +if (v_active_roundup) > +horizontal_backporch_byte = horizontal_backporch_byte + > +dsi->lanes - v_active_roundup; > +if (bllp_en) { > +hstx_cklp_wc_max = (DIV_ROUND_UP((16 + 6 + 4 + > +horizontal_backporch_byte + bllp_wc + ps_wc), > +dsi->lanes) + da_hs_trail + 1) * dsi->lanes / 6 - 1; > +} else { > +hstx_cklp_wc_max = (DIV_ROUND_UP((12 + 4 + 4 + > +horizontal_backporch_byte + bllp_wc + ps_wc), > +dsi->lanes) + da_hs_trail + 1) * dsi->lanes / 6 - 1; > +} > +} else { > +ps_wc = REG_FIELD_VALUE(readl(dsi->regs + DSI_PSCTRL), PS_WC); > +horizontal_frontporch_byte = (vm->hfront_porch * > +dsi_tmp_buf_bpp - 12); > + > +v_active_roundup = (22 + horizontal_backporch_byte + ps_wc + > +horizontal_frontporch_byte) % dsi->lanes; > +if (v_active_roundup) > +horizontal_backporch_byte = horizontal_backporch_byte + > +dsi->lanes - v_active_roundup; > + > +hstx_cklp_wc_max = (DIV_ROUND_UP((12 + 4 + 4 + > +horizontal_backporch_byte + ps_wc), > +dsi->lanes) + da_hs_trail + 1) * dsi->lanes / 6 - 1; > +} > +} > +hstx_cklp_wc = REG_FIELD_VALUE(ps_wc, HSTX_CKL_WC); hstx_cklp_wc = REG_FIELD_VALUE(dsi->vm.hactive * dsi_buf_bpp, HSTX_CKL_WC); But this looks a little weird. > +if (hstx_cklp_wc <= hstx_cklp_wc_min || > +hstx_cklp_wc >= hstx_cklp_wc_max) { > +hstx_cklp_wc = (hstx_cklp_wc_max / 2) << HSTX_CKL_WC_SHIFT; You choose the new setting with a risk that has a warning, why not let hstx_cklp_wc = (hstx_cklp_wc_min + hstx_cklp_wc_max) / 2 ? > +writel(hstx_cklp_wc, dsi->regs + DSI_HSTX_CKL_WC); > +} > +hstx_cklp_wc = hstx_cklp_wc >> HSTX_CKL_WC_SHIFT; > +if (hstx_cklp_wc <= hstx_cklp_wc_min || > +hstx_cklp_wc >= hstx_cklp_wc_max) { > +DRM_WARN("Wrong setting of hstx_ckl_wc\n"); > +} > + > +lpx = REG_FIELD_VALUE(readl(dsi->regs + DSI_PHY_TIMECON0), LPX); lpx = dsi->phy_timing.lpx; > +da_hs_exit = REG_FIELD_VALUE(readl(dsi->regs + DSI_PHY_TIMECON1), DA_HS_EXIT); da_hs_exit = dsi->phy_timing.da_hs_exit; > +da_hs_prep = REG_FIELD_VALUE(readl(dsi->regs + DSI_PHY_TIMECON0), DA_HS_PREP); da_hs_prep = dsi->phy_timing.da_hs_prepare; > +da_hs_zero = REG_FIELD_VALUE(readl(dsi->regs + DSI_PHY_TIMECON0), DA_HS_ZERO); da_hs_zero = dsi->phy_timing.da_hs_zero; > +ps_wc = REG_FIELD_VALUE(readl(dsi->regs + DSI_PSCTRL), PS_WC); This value is assigned? > +hs_vb_ps_wc = ps_wc - > +(lpx + da_hs_exit + da_hs_prep + da_hs_zero + 2) > +* dsi->lanes; > +horizontal_frontporch_byte = (1 << HFP_HS_EN) > +| (hs_vb_ps_wc << HFP_HS_VB_PS_WC_SHIFT) > +| (horizontal_frontporch_byte); > + > +writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC); > +writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC); > +writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC); > +} > + > static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > { > u32 horizontal_sync_active_byte; > @@ -499,6 +638,9 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi) > writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC); > writel(horizontal_frontporch_byte, dsi->regs + DSI_HFP_WC); > If dsi->driver_data->support_per_frame_lp is false, the calculation before this is redundant. It's worth to move previous calculation to a function and call that function when dsi->driver_data- >support_per_frame_lp is false. > +if (dsi->driver_data->support_per_frame_lp) > +mtk_dsi_config_vdo_timing_per_frame_lp(dsi); > + > mtk_dsi_ps_control(dsi, false); > } > > @@ -1193,6 +1335,7 @@ static const struct mtk_dsi_driver_data mt8188_dsi_driver_data = { > .has_shadow_ctl = true, > .has_size_ctl = true, > .cmdq_long_packet_ctl = true, > +.support_per_frame_lp = true, > }; > > static const struct of_device_id mtk_dsi_of_match[] = { </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>