diff mbox series

[v2,8/8] drm/msm/dpu: rename dpu_hw_setup_vsync_source functions

Message ID 20240613-dpu-handle-te-signal-v2-8-67a0116b5366@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: handle non-default TE source pins | expand

Commit Message

Dmitry Baryshkov June 13, 2024, 5:05 p.m. UTC 
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 <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Marijn Suijten June 13, 2024, 6:29 p.m. UTC | #1 
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 <dmitry.baryshkov@linaro.org>
> ---
>  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
>
Abhinav Kumar June 19, 2024, 7:11 p.m. UTC | #2 
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 <dmitry.baryshkov@linaro.org>
>> ---
>>   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 <quic_abhinavk@quicinc.com>


>>   
>>   	ops->get_safe_status = dpu_hw_get_safe_status;
>>   
>>
>> -- 
>> 2.39.2
>>
diff mbox series

Patch

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;