On Fri, 10 Feb 2023 at 00:31, Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote: > > > > On 2/9/2023 10:51 AM, Dmitry Baryshkov wrote: > > On 09/02/2023 20:44, Jessica Zhang wrote: > >> Currently, DPU will enable TE during prepare_commit(). However, this > >> will cause issues when trying to read/write to register in > >> 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] > >> > >> [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@xxxxxxxxxxx> > >> --- > >> .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 78 ++++++++++--------- > >> 1 file changed, 43 insertions(+), 35 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..561406d92a1a 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 > >> @@ -583,39 +583,6 @@ static void dpu_encoder_phys_cmd_destroy(struct > >> dpu_encoder_phys *phys_enc) > >> kfree(cmd_enc); > >> } > >> -static void dpu_encoder_phys_cmd_prepare_for_kickoff( > >> - struct dpu_encoder_phys *phys_enc) > >> -{ > >> - struct dpu_encoder_phys_cmd *cmd_enc = > >> - to_dpu_encoder_phys_cmd(phys_enc); > >> - int ret; > >> - > >> - if (!phys_enc->hw_pp) { > >> - DPU_ERROR("invalid encoder\n"); > >> - return; > >> - } > >> - DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", > >> DRMID(phys_enc->parent), > >> - phys_enc->hw_pp->idx - PINGPONG_0, > >> - atomic_read(&phys_enc->pending_kickoff_cnt)); > >> - > >> - /* > >> - * Mark kickoff request as outstanding. If there are more than one, > >> - * outstanding, then we have to wait for the previous one to > >> complete > >> - */ > >> - ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc); > >> - if (ret) { > >> - /* force pending_kickoff_cnt 0 to discard failed kickoff */ > >> - atomic_set(&phys_enc->pending_kickoff_cnt, 0); > >> - DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n", > >> - DRMID(phys_enc->parent), ret, > >> - phys_enc->hw_pp->idx - PINGPONG_0); > >> - } > >> - > >> - 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)); > >> -} > >> - > >> static bool dpu_encoder_phys_cmd_is_ongoing_pptx( > >> struct dpu_encoder_phys *phys_enc) > >> { > >> @@ -641,8 +608,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); > >> @@ -700,6 +666,48 @@ static void dpu_encoder_phys_cmd_prepare_commit( > >> "disabled autorefresh\n"); > >> } > >> +static void dpu_encoder_phys_cmd_prepare_for_kickoff( > >> + struct dpu_encoder_phys *phys_enc) > >> +{ > >> + struct dpu_encoder_phys_cmd *cmd_enc = > >> + to_dpu_encoder_phys_cmd(phys_enc); > >> + int ret; > >> + > >> + if (!phys_enc->hw_pp) { > >> + DPU_ERROR("invalid encoder\n"); > >> + return; > >> + } > >> + > >> + > >> + DRM_DEBUG_KMS("id:%u pp:%d pending_cnt:%d\n", > >> DRMID(phys_enc->parent), > >> + phys_enc->hw_pp->idx - PINGPONG_0, > >> + atomic_read(&phys_enc->pending_kickoff_cnt)); > >> + > >> + /* > >> + * Mark kickoff request as outstanding. If there are more than one, > >> + * outstanding, then we have to wait for the previous one to > >> complete > >> + */ > >> + ret = _dpu_encoder_phys_cmd_wait_for_idle(phys_enc); > >> + if (ret) { > >> + /* force pending_kickoff_cnt 0 to discard failed kickoff */ > >> + atomic_set(&phys_enc->pending_kickoff_cnt, 0); > >> + DRM_ERROR("failed wait_for_idle: id:%u ret:%d pp:%d\n", > >> + DRMID(phys_enc->parent), ret, > >> + phys_enc->hw_pp->idx - PINGPONG_0); > >> + } > >> + > >> + dpu_encoder_phys_cmd_enable_te(phys_enc); > >> + > >> + 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)); > >> +} > > > > Quoting v1: > > > > Could you please move the function back to the place, so that we can see > > the actual difference? > > Hi Dmitry, > > Sorry if I missed your reply to my reply in V1, but as stated in the V1 > patch: the reason the diff looks like this is because > prepare_for_kickoff() is defined above where the prepare_commit() and > is_ongoing_pptx() were originally defined. So I had to move both > function definitions to above the prepare_for_kickoff() function for the > patch to compile. You can add a function prototype in front of dpu_encoder_phys_cmd_prepare_for_kickoff(). > > That being said, I'm open to any suggestions for making this patch more > legible. > > > > >> + > >> +static void dpu_encoder_phys_cmd_prepare_commit( > >> + struct dpu_encoder_phys *phys_enc) > >> +{ > >> +} > > > > This is not necessary and can be dropped. There is a safety check in > > dpu_encoder_prepare_commit(). > > Acked. > > Thanks, > > Jessica Zhang > > > > >> + > >> static int _dpu_encoder_phys_cmd_wait_for_ctl_start( > >> struct dpu_encoder_phys *phys_enc) > >> { > > > > -- > > With best wishes > > Dmitry > > -- With best wishes Dmitry
