[PATCH v2] drm/mediatek: dsi: Add dsi per-frame lp code for mt8188

CK Hu (胡俊光) ck.hu at mediatek.com
Tue Jul 9 00:04:34 PDT 2024


Hi, Shuijing:

On Tue, 2024-07-09 at 06:36 +0000, Shuijing Li (李水静) wrote:
> Dear CK,
> 
> 1.
> > 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.
> 
> 2.
> > ps_wc = dsi->vm.hactive * dsi_buf_bpp;
> 
> 3.
> > This register is never written. Which setting would influence this
> > value? Before dsi configuration finish, is this value stable?
> > 
> 
> ==>Answer 1.2.3:
> bllp_en/bllp_wc may be assigned values in the future.
> ps_wc may change in the future when features are added(e.g. dsc).
> Therefore, it is recommended to keep the original modifications to
> increase compatibility.

We should not consider anything in future. We don't know what happen in future so we could not review it.
If the modification is used only in future, add that modification in future patch.

Regards,
CK

> 
> 4.
> > Is this roundup the same as the one in mtk_dsi_config_vdo_timing()?
> > If the same, merge these two.
> 
> ==>Not same.
> 
> 
> Based on the feedback you provided, I have made the necessary changes
> and submitted version 3 for your consideration.
> 
> Thank you for your time and attention to this matter.
> 
> 
> 
> 
> 
> Best Regards,
> Shuijing
> 
> 
> On Wed, 2024-05-22 at 07:30 +0000, CK Hu (胡俊光) wrote:
> > 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 at 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_EDGE			BIT(10)
> > >  #define MAX_RTN_SIZE			GENMASK(15, 12)
> > >  #define HSTX_CKLP_EN			BIT(16)
> > > +#define HSTX_BLLP_EN_SHIFT 7
> > > +#define HSTX_BLLP_EN_MASK  0x1
> > >  
> > >  #define DSI_PSCTRL		0x1c
> > >  #define DSI_PS_WC			GENMASK(13, 0)
> > > @@ -76,6 +78,8 @@
> > >  #define PACKED_PS_18BIT_RGB666		1
> > >  #define LOOSELY_PS_24BIT_RGB666		2
> > >  #define PACKED_PS_24BIT_RGB888		3
> > > +#define PS_WC_SHIFT 0
> > > +#define PS_WC_MASK  0x7fff
> > >  
> > >  #define DSI_VSA_NL		0x20
> > >  #define DSI_VBP_NL		0x24
> > > @@ -88,12 +92,20 @@
> > >  #define DSI_HSA_WC		0x50
> > >  #define DSI_HBP_WC		0x54
> > >  #define DSI_HFP_WC		0x58
> > > +#define HFP_HS_EN		31
> > > +#define HFP_HS_VB_PS_WC_SHIFT 16
> > > +
> > > +#define DSI_BLLP_WC		0x5C
> > > +#define BLLP_WC_SHIFT 0
> > > +#define BLLP_WC_MASK  0xfff
> > >  
> > >  #define DSI_CMDQ_SIZE		0x60
> > >  #define CMDQ_SIZE			0x3f
> > >  #define CMDQ_SIZE_SEL		BIT(15)
> > >  
> > >  #define DSI_HSTX_CKL_WC		0x64
> > > +#define HSTX_CKL_WC_SHIFT 2
> > > +#define HSTX_CKL_WC_MASK  0x3fff
> > >  
> > >  #define DSI_RX_DATA0		0x74
> > >  #define DSI_RX_DATA1		0x78
> > > @@ -118,12 +130,22 @@
> > >  #define HS_PREP				GENMASK(15, 8)
> > >  #define HS_ZERO				GENMASK(23, 16)
> > >  #define HS_TRAIL			GENMASK(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_TIMECON1	0x114
> > >  #define TA_GO				GENMASK(7, 0)
> > >  #define TA_SURE				GENMASK(15, 8)
> > >  #define TA_GET				GENMASK(23, 16)
> > >  #define DA_HS_EXIT			GENMASK(31, 24)
> > > +#define DA_HS_EXIT_SHIFT 24
> > > +#define DA_HS_EXIT_MASK  0xff
> > >  
> > >  #define DSI_PHY_TIMECON2	0x118
> > >  #define CONT_DET			GENMASK(7, 0)
> > > @@ -154,6 +176,7 @@
> > >  #define DATA_1				GENMASK(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[] = {


More information about the Linux-mediatek mailing list