[RFC PATCH v3 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()

[Dmitry Baryshkov]

On 13/02/2023 21:48, Jessica Zhang wrote:
> Currently, DPU will enable TE during prepare_commit(). However, this
> will cause issues when trying to read/write to register in

Nit: what issues? SError? reboot to the sahara? board reset?

> get_autorefresh_config(), because the core clock rates aren't set at
> that time.
> 
> This used to work because phys_enc->hw_pp is only initialized in mode
> set [1], so the first prepare_commit() will return before any register
> read/write as hw_pp would be NULL.
> 
> However, when we try to implement support for INTF TE, we will run into
> the clock issue described above as hw_intf will *not* be NULL on the
> first prepare_commit(). This is because the initialization of
> dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2].
> 
> To avoid this issue, let's enable TE during prepare_for_kickoff()
> instead as the core clock rates are guaranteed to be set then.
> 
> Depends on: "Implement tearcheck support on INTF block" [3]
> 
> Changes in V3:
> - Added function prototypes
> - Reordered function definitions to make change more legible
> - Removed prepare_commit() function from dpu_encoder_phys_cmd
> 
> [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109
> [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339
> [3] https://patchwork.freedesktop.org/series/112332/
> 
> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index cb05036f2916..c6feffafa13f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -40,6 +40,10 @@
>   
>   #define DPU_ENC_MAX_POLL_TIMEOUT_US	2000
>   
> +static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
> +		struct dpu_encoder_phys *phys_enc);

This should not be necessary.


> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc);
> +
>   static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
>   {
>   	return (phys_enc->split_role != ENC_ROLE_SLAVE);
> @@ -611,6 +615,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>   			  phys_enc->hw_pp->idx - PINGPONG_0);
>   	}
>   
> +	dpu_encoder_phys_cmd_enable_te(phys_enc);
> +

And this is much cleaner and easier to spot the difference than it was 
in the previous patch. Thank you!

With the dpu_encoder_phys_cmd_is_ongoing_pptx() prototype removed it LGTM.

>   	DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n",
>   			phys_enc->hw_pp->idx - PINGPONG_0,
>   			atomic_read(&phys_enc->pending_kickoff_cnt));
> @@ -641,8 +647,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx(
>   	return false;
>   }
>   
> -static void dpu_encoder_phys_cmd_prepare_commit(
> -		struct dpu_encoder_phys *phys_enc)
> +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc)
>   {
>   	struct dpu_encoder_phys_cmd *cmd_enc =
>   		to_dpu_encoder_phys_cmd(phys_enc);
> @@ -799,7 +804,6 @@ static void dpu_encoder_phys_cmd_trigger_start(
>   static void dpu_encoder_phys_cmd_init_ops(
>   		struct dpu_encoder_phys_ops *ops)
>   {
> -	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
>   	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_cmd_enable;

-- 
With best wishes
Dmitry