| Message ID | 20230411-dpu-intf-te-v2-11-ef76c877eb97@somainline.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: Implement tearcheck support on INTF block | expand |
On 17/04/2023 23:21, Marijn Suijten wrote: > Since hardware revision 5.0.0 the TE configuration moved out of the > PINGPONG block into the INTF block, including vsync source selection > that was previously part of MDP top. Writing to the MDP_VSYNC_SEL > register has no effect anymore and is omitted downstream via the > DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF > blocks used by hardware prior to 5.0.0. > > The code that writes to these registers in the INTF block will follow in > subsequent patches. > > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 +- > .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 3 ++ > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 52 +++++++++++++++------- > 4 files changed, 41 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h > index b7845591c384..6906f8046b9e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h > @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = { > { > .name = "top_0", .id = MDP_TOP, > .base = 0x0, .len = 0x458, > - .features = 0, > + .features = BIT(DPU_MDP_VSYNC_SEL), > .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, > .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, > .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, > diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h > index 5b9b3b99f1b5..14ce397800d5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h > @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = { > { > .name = "top_0", .id = MDP_TOP, > .base = 0x0, .len = 0x45c, > - .features = BIT(DPU_MDP_AUDIO_SELECT), > + .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL), > .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, > .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, > .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > index 71584cd56fd7..599e177b89dd 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h > @@ -48,6 +48,8 @@ enum { > * @DPU_MDP_UBWC_1_5, Universal Bandwidth compression version 1.5 > * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 block results > * in a failure > + * @DPU_MDP_VSYNC_SEL Enables vsync source selection via MDP_VSYNC_SEL register > + * (moved into INTF block since DPU 5.0.0) > * @DPU_MDP_MAX Maximum value > > */ > @@ -59,6 +61,7 @@ enum { > DPU_MDP_UBWC_1_5, > DPU_MDP_AUDIO_SELECT, > DPU_MDP_PERIPH_0_REMOVED, > + DPU_MDP_VSYNC_SEL, > DPU_MDP_MAX > }; > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > index 2bb02e17ee52..9ea15a647a66 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c > @@ -126,28 +126,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp, > status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3; > } > > -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, > +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, > struct dpu_vsync_source_cfg *cfg) In my opinion _v1 is not really descriptive here. Could you please rename it to dpu_hw_setup_vsync_source_no_vsync_sel() ? Or maybe rename dpu_hw_setup_vsync_source() to dpu_hw_setup_vsync_source_vsync_sel() and drop _v1 from this function. Up to you. > { > struct dpu_hw_blk_reg_map *c; > - u32 reg, wd_load_value, wd_ctl, wd_ctl2, i; > - static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; > + u32 reg, wd_load_value, wd_ctl, wd_ctl2; > > - if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) > + if (!mdp || !cfg) > return; > > c = &mdp->hw; > - reg = DPU_REG_READ(c, MDP_VSYNC_SEL); > - for (i = 0; i < cfg->pp_count; i++) { > - int pp_idx = cfg->ppnumber[i] - PINGPONG_0; > - > - if (pp_idx >= ARRAY_SIZE(pp_offset)) > - continue; > - > - reg &= ~(0xf << pp_offset[pp_idx]); > - reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; > - } > - DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); > > if (cfg->vsync_source >= DPU_VSYNC_SOURCE_WD_TIMER_4 && > cfg->vsync_source <= DPU_VSYNC_SOURCE_WD_TIMER_0) { > @@ -194,6 +182,33 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, > } > } > > +static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, > + struct dpu_vsync_source_cfg *cfg) > +{ > + struct dpu_hw_blk_reg_map *c; > + u32 reg, i; > + static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; > + > + if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) > + return; > + > + c = &mdp->hw; > + > + reg = DPU_REG_READ(c, MDP_VSYNC_SEL); > + for (i = 0; i < cfg->pp_count; i++) { > + int pp_idx = cfg->ppnumber[i] - PINGPONG_0; > + > + if (pp_idx >= ARRAY_SIZE(pp_offset)) > + continue; > + > + reg &= ~(0xf << pp_offset[pp_idx]); > + reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; > + } > + DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); > + > + dpu_hw_setup_vsync_source_v1(mdp, cfg); > +} > + > static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp, > struct dpu_danger_safe_status *status) > { > @@ -241,7 +256,12 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, > ops->setup_split_pipe = dpu_hw_setup_split_pipe; > ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; > ops->get_danger_status = dpu_hw_get_danger_status; > - ops->setup_vsync_source = dpu_hw_setup_vsync_source; > + > + if (cap & BIT(DPU_MDP_VSYNC_SEL)) > + ops->setup_vsync_source = dpu_hw_setup_vsync_source; > + else > + ops->setup_vsync_source = dpu_hw_setup_vsync_source_v1; > + > ops->get_safe_status = dpu_hw_get_safe_status; > > if (cap & BIT(DPU_MDP_AUDIO_SELECT)) >
On 20.04.2023 03:00, Dmitry Baryshkov wrote: > On 17/04/2023 23:21, Marijn Suijten wrote: >> Since hardware revision 5.0.0 the TE configuration moved out of the >> PINGPONG block into the INTF block, including vsync source selection >> that was previously part of MDP top. Writing to the MDP_VSYNC_SEL >> register has no effect anymore and is omitted downstream via the >> DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF >> blocks used by hardware prior to 5.0.0. >> >> The code that writes to these registers in the INTF block will follow in >> subsequent patches. >> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 +- >> .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 +- >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 3 ++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 52 +++++++++++++++------- >> 4 files changed, 41 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >> index b7845591c384..6906f8046b9e 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >> @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = { >> { >> .name = "top_0", .id = MDP_TOP, >> .base = 0x0, .len = 0x458, >> - .features = 0, >> + .features = BIT(DPU_MDP_VSYNC_SEL), >> .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, >> .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, >> .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >> index 5b9b3b99f1b5..14ce397800d5 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >> @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = { >> { >> .name = "top_0", .id = MDP_TOP, >> .base = 0x0, .len = 0x45c, >> - .features = BIT(DPU_MDP_AUDIO_SELECT), >> + .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL), >> .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, >> .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, >> .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >> index 71584cd56fd7..599e177b89dd 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >> @@ -48,6 +48,8 @@ enum { >> * @DPU_MDP_UBWC_1_5, Universal Bandwidth compression version 1.5 >> * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 block results >> * in a failure >> + * @DPU_MDP_VSYNC_SEL Enables vsync source selection via MDP_VSYNC_SEL register >> + * (moved into INTF block since DPU 5.0.0) >> * @DPU_MDP_MAX Maximum value >> */ >> @@ -59,6 +61,7 @@ enum { >> DPU_MDP_UBWC_1_5, >> DPU_MDP_AUDIO_SELECT, >> DPU_MDP_PERIPH_0_REMOVED, >> + DPU_MDP_VSYNC_SEL, >> DPU_MDP_MAX >> }; >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >> index 2bb02e17ee52..9ea15a647a66 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >> @@ -126,28 +126,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp, >> status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3; >> } >> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, >> struct dpu_vsync_source_cfg *cfg) > > In my opinion _v1 is not really descriptive here. Could you please rename it to dpu_hw_setup_vsync_source_no_vsync_sel() ? v1 refers to the CTL rev 100 a.k.a 1.0.0 a.k.a 1, but that's not yet very well formulated upstream.. if we even need it.. Konrad > > Or maybe rename dpu_hw_setup_vsync_source() to dpu_hw_setup_vsync_source_vsync_sel() and drop _v1 from this function. > > Up to you. > > >> { >> struct dpu_hw_blk_reg_map *c; >> - u32 reg, wd_load_value, wd_ctl, wd_ctl2, i; >> - static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; >> + u32 reg, wd_load_value, wd_ctl, wd_ctl2; >> - if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) >> + if (!mdp || !cfg) >> return; >> c = &mdp->hw; >> - reg = DPU_REG_READ(c, MDP_VSYNC_SEL); >> - for (i = 0; i < cfg->pp_count; i++) { >> - int pp_idx = cfg->ppnumber[i] - PINGPONG_0; >> - >> - if (pp_idx >= ARRAY_SIZE(pp_offset)) >> - continue; >> - >> - reg &= ~(0xf << pp_offset[pp_idx]); >> - reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; >> - } >> - DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); >> if (cfg->vsync_source >= DPU_VSYNC_SOURCE_WD_TIMER_4 && >> cfg->vsync_source <= DPU_VSYNC_SOURCE_WD_TIMER_0) { >> @@ -194,6 +182,33 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >> } >> } >> +static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >> + struct dpu_vsync_source_cfg *cfg) >> +{ >> + struct dpu_hw_blk_reg_map *c; >> + u32 reg, i; >> + static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; >> + >> + if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) >> + return; >> + >> + c = &mdp->hw; >> + >> + reg = DPU_REG_READ(c, MDP_VSYNC_SEL); >> + for (i = 0; i < cfg->pp_count; i++) { >> + int pp_idx = cfg->ppnumber[i] - PINGPONG_0; >> + >> + if (pp_idx >= ARRAY_SIZE(pp_offset)) >> + continue; >> + >> + reg &= ~(0xf << pp_offset[pp_idx]); >> + reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; >> + } >> + DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); >> + >> + dpu_hw_setup_vsync_source_v1(mdp, cfg); >> +} >> + >> static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp, >> struct dpu_danger_safe_status *status) >> { >> @@ -241,7 +256,12 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, >> ops->setup_split_pipe = dpu_hw_setup_split_pipe; >> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; >> ops->get_danger_status = dpu_hw_get_danger_status; >> - ops->setup_vsync_source = dpu_hw_setup_vsync_source; >> + >> + if (cap & BIT(DPU_MDP_VSYNC_SEL)) >> + ops->setup_vsync_source = dpu_hw_setup_vsync_source; >> + else >> + ops->setup_vsync_source = dpu_hw_setup_vsync_source_v1; >> + >> ops->get_safe_status = dpu_hw_get_safe_status; >> if (cap & BIT(DPU_MDP_AUDIO_SELECT)) >> >
On 20/04/2023 04:01, Konrad Dybcio wrote: > > > On 20.04.2023 03:00, Dmitry Baryshkov wrote: >> On 17/04/2023 23:21, Marijn Suijten wrote: >>> Since hardware revision 5.0.0 the TE configuration moved out of the >>> PINGPONG block into the INTF block, including vsync source selection >>> that was previously part of MDP top. Writing to the MDP_VSYNC_SEL >>> register has no effect anymore and is omitted downstream via the >>> DPU/SDE_MDP_VSYNC_SEL feature flag. This flag is only added to INTF >>> blocks used by hardware prior to 5.0.0. >>> >>> The code that writes to these registers in the INTF block will follow in >>> subsequent patches. >>> >>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> .../drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h | 2 +- >>> .../gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h | 2 +- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 3 ++ >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 52 +++++++++++++++------- >>> 4 files changed, 41 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >>> index b7845591c384..6906f8046b9e 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h >>> @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = { >>> { >>> .name = "top_0", .id = MDP_TOP, >>> .base = 0x0, .len = 0x458, >>> - .features = 0, >>> + .features = BIT(DPU_MDP_VSYNC_SEL), >>> .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, >>> .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, >>> .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >>> index 5b9b3b99f1b5..14ce397800d5 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h >>> @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = { >>> { >>> .name = "top_0", .id = MDP_TOP, >>> .base = 0x0, .len = 0x45c, >>> - .features = BIT(DPU_MDP_AUDIO_SELECT), >>> + .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL), >>> .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, >>> .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, >>> .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>> index 71584cd56fd7..599e177b89dd 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h >>> @@ -48,6 +48,8 @@ enum { >>> * @DPU_MDP_UBWC_1_5, Universal Bandwidth compression version 1.5 >>> * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 block results >>> * in a failure >>> + * @DPU_MDP_VSYNC_SEL Enables vsync source selection via MDP_VSYNC_SEL register >>> + * (moved into INTF block since DPU 5.0.0) >>> * @DPU_MDP_MAX Maximum value >>> */ >>> @@ -59,6 +61,7 @@ enum { >>> DPU_MDP_UBWC_1_5, >>> DPU_MDP_AUDIO_SELECT, >>> DPU_MDP_PERIPH_0_REMOVED, >>> + DPU_MDP_VSYNC_SEL, >>> DPU_MDP_MAX >>> }; >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >>> index 2bb02e17ee52..9ea15a647a66 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c >>> @@ -126,28 +126,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp, >>> status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3; >>> } >>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >>> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, >>> struct dpu_vsync_source_cfg *cfg) >> >> In my opinion _v1 is not really descriptive here. Could you please rename it to dpu_hw_setup_vsync_source_no_vsync_sel() ? > v1 refers to the CTL rev 100 a.k.a 1.0.0 a.k.a 1, but that's not > yet very well formulated upstream.. if we even need it.. Yeah, but this mdp_top, not the ctl. And for CTL I'd probably rename _v1 to _active to follow actual feature name. > > Konrad >> >> Or maybe rename dpu_hw_setup_vsync_source() to dpu_hw_setup_vsync_source_vsync_sel() and drop _v1 from this function. >> >> Up to you. >> >> >>> { >>> struct dpu_hw_blk_reg_map *c; >>> - u32 reg, wd_load_value, wd_ctl, wd_ctl2, i; >>> - static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; >>> + u32 reg, wd_load_value, wd_ctl, wd_ctl2; >>> - if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) >>> + if (!mdp || !cfg) >>> return; >>> c = &mdp->hw; >>> - reg = DPU_REG_READ(c, MDP_VSYNC_SEL); >>> - for (i = 0; i < cfg->pp_count; i++) { >>> - int pp_idx = cfg->ppnumber[i] - PINGPONG_0; >>> - >>> - if (pp_idx >= ARRAY_SIZE(pp_offset)) >>> - continue; >>> - >>> - reg &= ~(0xf << pp_offset[pp_idx]); >>> - reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; >>> - } >>> - DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); >>> if (cfg->vsync_source >= DPU_VSYNC_SOURCE_WD_TIMER_4 && >>> cfg->vsync_source <= DPU_VSYNC_SOURCE_WD_TIMER_0) { >>> @@ -194,6 +182,33 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >>> } >>> } >>> +static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >>> + struct dpu_vsync_source_cfg *cfg) >>> +{ >>> + struct dpu_hw_blk_reg_map *c; >>> + u32 reg, i; >>> + static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; >>> + >>> + if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) >>> + return; >>> + >>> + c = &mdp->hw; >>> + >>> + reg = DPU_REG_READ(c, MDP_VSYNC_SEL); >>> + for (i = 0; i < cfg->pp_count; i++) { >>> + int pp_idx = cfg->ppnumber[i] - PINGPONG_0; >>> + >>> + if (pp_idx >= ARRAY_SIZE(pp_offset)) >>> + continue; >>> + >>> + reg &= ~(0xf << pp_offset[pp_idx]); >>> + reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; >>> + } >>> + DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); >>> + >>> + dpu_hw_setup_vsync_source_v1(mdp, cfg); >>> +} >>> + >>> static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp, >>> struct dpu_danger_safe_status *status) >>> { >>> @@ -241,7 +256,12 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, >>> ops->setup_split_pipe = dpu_hw_setup_split_pipe; >>> ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; >>> ops->get_danger_status = dpu_hw_get_danger_status; >>> - ops->setup_vsync_source = dpu_hw_setup_vsync_source; >>> + >>> + if (cap & BIT(DPU_MDP_VSYNC_SEL)) >>> + ops->setup_vsync_source = dpu_hw_setup_vsync_source; >>> + else >>> + ops->setup_vsync_source = dpu_hw_setup_vsync_source_v1; >>> + >>> ops->get_safe_status = dpu_hw_get_safe_status; >>> if (cap & BIT(DPU_MDP_AUDIO_SELECT)) >>> >>
On 2023-04-20 04:03:31, Dmitry Baryshkov wrote: [..] > >>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, > >>> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, > >>> struct dpu_vsync_source_cfg *cfg) > >> > >> In my opinion _v1 is not really descriptive here. Could you please rename it to dpu_hw_setup_vsync_source_no_vsync_sel() ? > > v1 refers to the CTL rev 100 a.k.a 1.0.0 a.k.a 1, but that's not > > yet very well formulated upstream.. if we even need it.. I think v1 just refers to "the first next variant of this function", similar to how for example Microsoft COM APIs start without a suffix, then get 1, 2, 3 etc appended as new variants "of the same" trickle in. > Yeah, but this mdp_top, not the ctl. And for CTL I'd probably rename _v1 > to _active to follow actual feature name. Correct, I just got lazily inspired by downstream here. There it switches on SDE_MDP_VSYNC_SEL which is based on DPU >= 5.0.0 as explained in the patch. > >> Or maybe rename dpu_hw_setup_vsync_source() to dpu_hw_setup_vsync_source_vsync_sel() and drop _v1 from this function. Maybe add _and_ in there? > >> > >> Up to you. - Marijn
On 21/04/2023 00:51, Marijn Suijten wrote: > On 2023-04-20 04:03:31, Dmitry Baryshkov wrote: > [..] >>>>> -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, >>>>> +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, >>>>> struct dpu_vsync_source_cfg *cfg) >>>> >>>> In my opinion _v1 is not really descriptive here. Could you please rename it to dpu_hw_setup_vsync_source_no_vsync_sel() ? >>> v1 refers to the CTL rev 100 a.k.a 1.0.0 a.k.a 1, but that's not >>> yet very well formulated upstream.. if we even need it.. > > I think v1 just refers to "the first next variant of this function", > similar to how for example Microsoft COM APIs start without a suffix, > then get 1, 2, 3 etc appended as new variants "of the same" trickle in. > >> Yeah, but this mdp_top, not the ctl. And for CTL I'd probably rename _v1 >> to _active to follow actual feature name. > > Correct, I just got lazily inspired by downstream here. There it > switches on SDE_MDP_VSYNC_SEL which is based on DPU >= 5.0.0 as > explained in the patch. > >>>> Or maybe rename dpu_hw_setup_vsync_source() to dpu_hw_setup_vsync_source_vsync_sel() and drop _v1 from this function. > > Maybe add _and_ in there? Either way will work. >>>> Up to you. > > - Marijn
diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h index b7845591c384..6906f8046b9e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_3_0_msm8998.h @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg msm8998_mdp[] = { { .name = "top_0", .id = MDP_TOP, .base = 0x0, .len = 0x458, - .features = 0, + .features = BIT(DPU_MDP_VSYNC_SEL), .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, diff --git a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h index 5b9b3b99f1b5..14ce397800d5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h +++ b/drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_4_0_sdm845.h @@ -30,7 +30,7 @@ static const struct dpu_mdp_cfg sdm845_mdp[] = { { .name = "top_0", .id = MDP_TOP, .base = 0x0, .len = 0x45c, - .features = BIT(DPU_MDP_AUDIO_SELECT), + .features = BIT(DPU_MDP_AUDIO_SELECT) | BIT(DPU_MDP_VSYNC_SEL), .clk_ctrls[DPU_CLK_CTRL_VIG0] = { .reg_off = 0x2ac, .bit_off = 0 }, .clk_ctrls[DPU_CLK_CTRL_VIG1] = { .reg_off = 0x2b4, .bit_off = 0 }, .clk_ctrls[DPU_CLK_CTRL_VIG2] = { .reg_off = 0x2bc, .bit_off = 0 }, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index 71584cd56fd7..599e177b89dd 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -48,6 +48,8 @@ enum { * @DPU_MDP_UBWC_1_5, Universal Bandwidth compression version 1.5 * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 block results * in a failure + * @DPU_MDP_VSYNC_SEL Enables vsync source selection via MDP_VSYNC_SEL register + * (moved into INTF block since DPU 5.0.0) * @DPU_MDP_MAX Maximum value */ @@ -59,6 +61,7 @@ enum { DPU_MDP_UBWC_1_5, DPU_MDP_AUDIO_SELECT, DPU_MDP_PERIPH_0_REMOVED, + DPU_MDP_VSYNC_SEL, DPU_MDP_MAX }; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c index 2bb02e17ee52..9ea15a647a66 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c @@ -126,28 +126,16 @@ static void dpu_hw_get_danger_status(struct dpu_hw_mdp *mdp, status->sspp[SSPP_CURSOR1] = (value >> 26) & 0x3; } -static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, +static void dpu_hw_setup_vsync_source_v1(struct dpu_hw_mdp *mdp, struct dpu_vsync_source_cfg *cfg) { struct dpu_hw_blk_reg_map *c; - u32 reg, wd_load_value, wd_ctl, wd_ctl2, i; - static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; + u32 reg, wd_load_value, wd_ctl, wd_ctl2; - if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) + if (!mdp || !cfg) return; c = &mdp->hw; - reg = DPU_REG_READ(c, MDP_VSYNC_SEL); - for (i = 0; i < cfg->pp_count; i++) { - int pp_idx = cfg->ppnumber[i] - PINGPONG_0; - - if (pp_idx >= ARRAY_SIZE(pp_offset)) - continue; - - reg &= ~(0xf << pp_offset[pp_idx]); - reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; - } - DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); if (cfg->vsync_source >= DPU_VSYNC_SOURCE_WD_TIMER_4 && cfg->vsync_source <= DPU_VSYNC_SOURCE_WD_TIMER_0) { @@ -194,6 +182,33 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, } } +static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp *mdp, + struct dpu_vsync_source_cfg *cfg) +{ + struct dpu_hw_blk_reg_map *c; + u32 reg, i; + static const u32 pp_offset[PINGPONG_MAX] = {0xC, 0x8, 0x4, 0x13, 0x18}; + + if (!mdp || !cfg || (cfg->pp_count > ARRAY_SIZE(cfg->ppnumber))) + return; + + c = &mdp->hw; + + reg = DPU_REG_READ(c, MDP_VSYNC_SEL); + for (i = 0; i < cfg->pp_count; i++) { + int pp_idx = cfg->ppnumber[i] - PINGPONG_0; + + if (pp_idx >= ARRAY_SIZE(pp_offset)) + continue; + + reg &= ~(0xf << pp_offset[pp_idx]); + reg |= (cfg->vsync_source & 0xf) << pp_offset[pp_idx]; + } + DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg); + + dpu_hw_setup_vsync_source_v1(mdp, cfg); +} + static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp, struct dpu_danger_safe_status *status) { @@ -241,7 +256,12 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, ops->setup_split_pipe = dpu_hw_setup_split_pipe; ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; ops->get_danger_status = dpu_hw_get_danger_status; - ops->setup_vsync_source = dpu_hw_setup_vsync_source; + + if (cap & BIT(DPU_MDP_VSYNC_SEL)) + ops->setup_vsync_source = dpu_hw_setup_vsync_source; + else + ops->setup_vsync_source = dpu_hw_setup_vsync_source_v1; + ops->get_safe_status = dpu_hw_get_safe_status; if (cap & BIT(DPU_MDP_AUDIO_SELECT))