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

Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> · Thu, 9 Feb 2023 01:26:16 +0200

On 09/02/2023 01:24, Jessica Zhang wrote:


On 2/8/2023 2:18 PM, Dmitry Baryshkov wrote:
On 08/02/2023 23:37, 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 279a0b7015ce..746250bce3d1 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
@@ -587,39 +587,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)
  {
@@ -645,8 +612,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);
@@ -704,6 +670,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)

Could you please move the function back to the place, so that we can 
see the actual difference?

Hi Dmitry,

This function was moved because prepare_commit() and is_ongoing_pptx() 
(which is called in prepare_commit()) were originally defined later in 
the file.


+{
+    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));
+}
+
+static void dpu_encoder_phys_cmd_prepare_commit(
+        struct dpu_encoder_phys *phys_enc)
+{
+}

There is no need to have the empty callback, you can skip it 
completely. Actually, if it is not needed anymore for the cmd 
encoders, you can drop the .prepare_commit from struct 
dpu_encoder_phys_ops. And then, by extension, 
dpu_encoder_prepare_commit(), dpu_kms_prepare_commit(). This sounds 
like a nice second patch for this rfc.

Got it.

FWIW I kept this as an empty method to match mdp4_prepare_commit() [1], 
but I can just add a NULL check in msm_atomic_commit_tail() and remove 
both instances of empty callbacks if that's preferable.

yes, please.


[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c#L87


--
With best wishes
Dmitry







