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

From: CK Hu (胡俊光)
Date: Tue Jul 09 2024 - 03:05:02 EST

Next message: Stephen Rothwell: "linux-next: manual merge of the vhost tree with the net-next tree"
Previous message: Zqiang: "[PATCH v2] rcu-tasks: Fix access non-existent percpu rtpcp variable in rcu_tasks_need_gpcb()"
In reply to: Shuijing Li (李水静): "Re: [PATCH v2] drm/mediatek: dsi: Add dsi per-frame lp code for mt8188"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxx/__;!!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[] = {

Next message: Stephen Rothwell: "linux-next: manual merge of the vhost tree with the net-next tree"
Previous message: Zqiang: "[PATCH v2] rcu-tasks: Fix access non-existent percpu rtpcp variable in rcu_tasks_need_gpcb()"
In reply to: Shuijing Li (李水静): "Re: [PATCH v2] drm/mediatek: dsi: Add dsi per-frame lp code for mt8188"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]