Re: [PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions
On 6/13/2024 11:29 AM, Marijn Suijten wrote:
On 2024-06-13 20:05:11, Dmitry Baryshkov wrote:
Rename dpu_hw_setup_vsync_source functions to make the names match the
implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
and 4.x used MDP_VSYNC_SEL register to select TE source.
Yeah that was never really clear anymore after I split this function in two in
commit a2ff096803b3 ("drm/msm/dpu: Disable MDP vsync source selection on DPU
5.0.0 and above").
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
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 05e48cf4ec1d..6e2ac50b94a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -107,8 +107,8 @@ 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,
- struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_wd_timer(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;
@@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp
*mdp,
}
}
-static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
- struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,
Maybe keep setup_wd_timer_and_vsync_sel()? OTOH it doesn't always set wd_timer,
only when vsync_source calls for it.
Yeah, I think this name is fine.
+ struct dpu_vsync_source_cfg *cfg)
{
struct dpu_hw_blk_reg_map *c;
u32 reg, i;
@@ -187,7 +187,7 @@ static void dpu_hw_setup_vsync_source_and_vsync_sel(struct
dpu_hw_mdp *mdp,
}
DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
- dpu_hw_setup_vsync_source(mdp, cfg);
+ dpu_hw_setup_wd_timer(mdp, cfg);
}
static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
@@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
ops->get_danger_status = dpu_hw_get_danger_status;
if (cap & BIT(DPU_MDP_VSYNC_SEL))
- ops->setup_vsync_source =
dpu_hw_setup_vsync_source_and_vsync_sel;
+ ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
else
- ops->setup_vsync_source = dpu_hw_setup_vsync_source;
+ ops->setup_vsync_source = dpu_hw_setup_wd_timer;
Should the callback also be renamed - and the docs improved? Seems the
vsync_sel register is used to selsect a vsync_source (plus some other bits like
the pingpong block).
- Marijn
Its a good thought but then we will also have to change the callers of
setup_vsync_source to check we have ops->setup_vsync_source ||
ops->setup_watchdog_timer in dpu_encoder.c
This way, the caller does not know because whether to use TE or watchdog
as the setting the source will happen under the hood.
So I think this is okay actually, hence
Reviewed-by: Abhinav Kumar
ops->get_safe_status = dpu_hw_get_safe_status;
--
2.39.2
Re: [PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions
On 2024-06-13 20:05:11, Dmitry Baryshkov wrote:
> Rename dpu_hw_setup_vsync_source functions to make the names match the
> implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
> and 4.x used MDP_VSYNC_SEL register to select TE source.
Yeah that was never really clear anymore after I split this function in two in
commit a2ff096803b3 ("drm/msm/dpu: Disable MDP vsync source selection on DPU
5.0.0 and above").
>
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> 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 05e48cf4ec1d..6e2ac50b94a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
> @@ -107,8 +107,8 @@ 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,
> - struct dpu_vsync_source_cfg *cfg)
> +static void dpu_hw_setup_wd_timer(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;
> @@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp
> *mdp,
> }
> }
>
> -static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
> - struct dpu_vsync_source_cfg *cfg)
> +static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,
Maybe keep setup_wd_timer_and_vsync_sel()? OTOH it doesn't always set wd_timer,
only when vsync_source calls for it.
> +struct dpu_vsync_source_cfg *cfg)
> {
> struct dpu_hw_blk_reg_map *c;
> u32 reg, i;
> @@ -187,7 +187,7 @@ static void
> dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
> }
> DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
>
> - dpu_hw_setup_vsync_source(mdp, cfg);
> + dpu_hw_setup_wd_timer(mdp, cfg);
> }
>
> static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
> @@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
> ops->get_danger_status = dpu_hw_get_danger_status;
>
> if (cap & BIT(DPU_MDP_VSYNC_SEL))
> - ops->setup_vsync_source =
> dpu_hw_setup_vsync_source_and_vsync_sel;
> + ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
> else
> - ops->setup_vsync_source = dpu_hw_setup_vsync_source;
> + ops->setup_vsync_source = dpu_hw_setup_wd_timer;
Should the callback also be renamed - and the docs improved? Seems the
vsync_sel register is used to selsect a vsync_source (plus some other bits like
the pingpong block).
- Marijn
>
> ops->get_safe_status = dpu_hw_get_safe_status;
>
>
> --
> 2.39.2
>
[PATCH v2 8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions
Rename dpu_hw_setup_vsync_source functions to make the names match the
implementation: on DPU 5.x the TOP only contains timer setup, while 3.x
and 4.x used MDP_VSYNC_SEL register to select TE source.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
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 05e48cf4ec1d..6e2ac50b94a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c
@@ -107,8 +107,8 @@ 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,
- struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_wd_timer(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;
@@ -163,8 +163,8 @@ static void dpu_hw_setup_vsync_source(struct dpu_hw_mdp
*mdp,
}
}
-static void dpu_hw_setup_vsync_source_and_vsync_sel(struct dpu_hw_mdp *mdp,
- struct dpu_vsync_source_cfg *cfg)
+static void dpu_hw_setup_vsync_sel(struct dpu_hw_mdp *mdp,
+ struct dpu_vsync_source_cfg *cfg)
{
struct dpu_hw_blk_reg_map *c;
u32 reg, i;
@@ -187,7 +187,7 @@ static void dpu_hw_setup_vsync_source_and_vsync_sel(struct
dpu_hw_mdp *mdp,
}
DPU_REG_WRITE(c, MDP_VSYNC_SEL, reg);
- dpu_hw_setup_vsync_source(mdp, cfg);
+ dpu_hw_setup_wd_timer(mdp, cfg);
}
static void dpu_hw_get_safe_status(struct dpu_hw_mdp *mdp,
@@ -239,9 +239,9 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops,
ops->get_danger_status = dpu_hw_get_danger_status;
if (cap & BIT(DPU_MDP_VSYNC_SEL))
- ops->setup_vsync_source =
dpu_hw_setup_vsync_source_and_vsync_sel;
+ ops->setup_vsync_source = dpu_hw_setup_vsync_sel;
else
- ops->setup_vsync_source = dpu_hw_setup_vsync_source;
+ ops->setup_vsync_source = dpu_hw_setup_wd_timer;
ops->get_safe_status = dpu_hw_get_safe_status;
--
2.39.2
