Re: [PATCH RFC 4/5] drm/msm: dpu1: call wb & sspp clk_force_ctrl op if split clock control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/10/2023 19:07, Dmitry Baryshkov wrote:
On 09/10/2023 19:36, Neil Armstrong wrote:
Now clk_ctrl IDs can be optional and the clk_ctrl_reg can be specified
on the SSPP & WB caps directly, pass the SSPP & WB hw struct to the
qos & limit params then call the clk_force_ctrl() op accordingly.

Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
---
  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    |  4 +--
  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c          |  9 +++---
  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c           | 37 +++++++++++++++-------
  drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h           | 12 ++++---
  4 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 78037a697633..e4dfe0be7207 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -45,6 +45,7 @@ static void dpu_encoder_phys_wb_set_ot_limit(
      struct dpu_vbif_set_ot_params ot_params;
      memset(&ot_params, 0, sizeof(ot_params));
+    ot_params.wb = hw_wb;
      ot_params.xin_id = hw_wb->caps->xin_id;
      ot_params.num = hw_wb->idx - WB_0;
      ot_params.width = phys_enc->cached_mode.hdisplay;
@@ -52,7 +53,6 @@ static void dpu_encoder_phys_wb_set_ot_limit(
      ot_params.is_wfd = true;
      ot_params.frame_rate = drm_mode_vrefresh(&phys_enc->cached_mode);
      ot_params.vbif_idx = hw_wb->caps->vbif_idx;
-    ot_params.clk_ctrl = hw_wb->caps->clk_ctrl;
      ot_params.rd = false;
      dpu_vbif_set_ot_limit(phys_enc->dpu_kms, &ot_params);
@@ -81,9 +81,9 @@ static void dpu_encoder_phys_wb_set_qos_remap(
      hw_wb = phys_enc->hw_wb;
      memset(&qos_params, 0, sizeof(qos_params));
+    qos_params.wb = hw_wb;
      qos_params.vbif_idx = hw_wb->caps->vbif_idx;
      qos_params.xin_id = hw_wb->caps->xin_id;
-    qos_params.clk_ctrl = hw_wb->caps->clk_ctrl;
      qos_params.num = hw_wb->idx - WB_0;
      qos_params.is_rt = false;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index c2aaaded07ed..b0b662068377 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -350,6 +350,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
      struct dpu_kms *dpu_kms = _dpu_plane_get_kms(plane);
      memset(&ot_params, 0, sizeof(ot_params));
+    ot_params.sspp = pipe->sspp;
      ot_params.xin_id = pipe->sspp->cap->xin_id;
      ot_params.num = pipe->sspp->idx - SSPP_NONE;
      ot_params.width = drm_rect_width(&pipe_cfg->src_rect);
@@ -357,7 +358,6 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
      ot_params.is_wfd = !pdpu->is_rt_pipe;
      ot_params.frame_rate = frame_rate;
      ot_params.vbif_idx = VBIF_RT;
-    ot_params.clk_ctrl = pipe->sspp->cap->clk_ctrl;
      ot_params.rd = true;
      dpu_vbif_set_ot_limit(dpu_kms, &ot_params);
@@ -377,16 +377,15 @@ static void _dpu_plane_set_qos_remap(struct drm_plane *plane,
      memset(&qos_params, 0, sizeof(qos_params));
      qos_params.vbif_idx = VBIF_RT;
-    qos_params.clk_ctrl = pipe->sspp->cap->clk_ctrl;
+    qos_params.sspp = pipe->sspp;
      qos_params.xin_id = pipe->sspp->cap->xin_id;
      qos_params.num = pipe->sspp->idx - SSPP_VIG0;
      qos_params.is_rt = pdpu->is_rt_pipe;
-    DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d, clk_ctrl:%d\n",
+    DPU_DEBUG_PLANE(pdpu, "pipe:%d vbif:%d xin:%d rt:%d\n",
              qos_params.num,
              qos_params.vbif_idx,
-            qos_params.xin_id, qos_params.is_rt,
-            qos_params.clk_ctrl);
+            qos_params.xin_id, qos_params.is_rt);
      dpu_vbif_set_qos_remap(dpu_kms, &qos_params);
  }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 2ae5cba1848b..a79559084a91 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -158,11 +158,19 @@ static u32 _dpu_vbif_get_ot_limit(struct dpu_hw_vbif *vbif,
      return ot_lim;
  }
-static bool dpu_vbif_setup_clk_force_ctrl(struct dpu_hw_mdp *mdp,
-                      unsigned int clk_ctrl,
+static bool dpu_vbif_setup_clk_force_ctrl(struct dpu_hw_sspp *sspp,
+                      struct dpu_hw_wb *wb,
+                      struct dpu_hw_mdp *mdp,
                        bool enable)
  {
-    return mdp->ops.setup_clk_force_ctrl(mdp, clk_ctrl, enable);
+    if (sspp && sspp->cap->clk_ctrl_reg)
+        return sspp->ops.setup_clk_force_ctrl(sspp, enable);
+    else if (wb && wb->caps->clk_ctrl_reg)
+        return wb->ops.setup_clk_force_ctrl(wb, enable);
+    else

This is what I wanted to avoid.

If we move the caller function to the sspp / WB, we will not need this kind of wrapper.

I tried it, but it requires passing the mdp pointer to the setup_clk_force_ctrl op,
which is IMHO not super clean... or if you have a way to get dpu_hw_mdp from
within hw_sspp/hw_wb it would help.


+        return mdp->ops.setup_clk_force_ctrl(mdp,
+                sspp ? sspp->cap->clk_ctrl : wb->caps->clk_ctrl,
+                enable);
  }
  /**
@@ -190,9 +198,13 @@ void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
          return;
      }
-    if (!mdp->ops.setup_clk_force_ctrl ||
-            !vbif->ops.set_limit_conf ||
-            !vbif->ops.set_halt_ctrl)
+    if ((!params->sspp && !params->wb) ||
+        (params->sspp && !params->sspp->ops.setup_clk_force_ctrl) ||
+        (params->wb && !params->wb->ops.setup_clk_force_ctrl) ||
+        !mdp->ops.setup_clk_force_ctrl)
+        return;
+
+    if (!vbif->ops.set_limit_conf || !vbif->ops.set_halt_ctrl)
          return;
      /* set write_gather_en for all write clients */
@@ -207,7 +219,7 @@ void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
      trace_dpu_perf_set_ot(params->num, params->xin_id, ot_lim,
          params->vbif_idx);
-    forced_on = dpu_vbif_setup_clk_force_ctrl(mdp, params->clk_ctrl, true);
+    forced_on = dpu_vbif_setup_clk_force_ctrl(params->sspp, params->wb, mdp, true);

I'd suggest removing the setup_clk_force_ctrl from dpu_vbif_set_ot_limit() and dpu_vbif_set_qos_remap(). Instead make dpu_plane / dpu_encoder_phys_wb call into dpu_hw_sspp / dpu_hw_wb, which will enable the clock, call dpu_vbif then disable the clock.

In my opinion this is simpler than the condition in the previous chunk.

Indeed this is a nice option, but the hw_mdp pointer requirement into hw_sspp/hw_wb
still puzzles me.


      vbif->ops.set_limit_conf(vbif, params->xin_id, params->rd, ot_lim);
@@ -220,7 +232,7 @@ void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
      vbif->ops.set_halt_ctrl(vbif, params->xin_id, false);
      if (forced_on)
-        dpu_vbif_setup_clk_force_ctrl(mdp,  params->clk_ctrl, false);
+        dpu_vbif_setup_clk_force_ctrl(params->sspp, params->wb, mdp, false);
  }
  void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
@@ -245,7 +257,10 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
          return;
      }
-    if (!vbif->ops.set_qos_remap || !mdp->ops.setup_clk_force_ctrl) {
+    if ((!params->sspp && !params->wb) ||
+        (params->sspp && !params->sspp->ops.setup_clk_force_ctrl) ||
+        (params->wb && !params->wb->ops.setup_clk_force_ctrl) ||
+        !mdp->ops.setup_clk_force_ctrl || !vbif->ops.set_qos_remap) {
          DRM_DEBUG_ATOMIC("qos remap not supported\n");
          return;
      }
@@ -258,7 +273,7 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
          return;
      }
-    forced_on = dpu_vbif_setup_clk_force_ctrl(mdp, params->clk_ctrl, true);
+    forced_on = dpu_vbif_setup_clk_force_ctrl(params->sspp, params->wb, mdp, true);
      for (i = 0; i < qos_tbl->npriority_lvl; i++) {
          DRM_DEBUG_ATOMIC("%s xin:%d lvl:%d/%d\n",
@@ -269,7 +284,7 @@ void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
      }
      if (forced_on)
-        dpu_vbif_setup_clk_force_ctrl(mdp, params->clk_ctrl, false);
+        dpu_vbif_setup_clk_force_ctrl(params->sspp, params->wb, mdp, false);
  }
  void dpu_vbif_clear_errors(struct dpu_kms *dpu_kms)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h
index ab490177d886..a4fe76e390d9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.h
@@ -7,7 +7,12 @@
  #include "dpu_kms.h"
+struct dpu_hw_sspp;
+struct dpu_hw_wb;
+
  struct dpu_vbif_set_ot_params {
+    struct dpu_hw_sspp *sspp;
+    struct dpu_hw_wb *wb;
      u32 xin_id;
      u32 num;
      u32 width;
@@ -16,28 +21,27 @@ struct dpu_vbif_set_ot_params {
      bool rd;
      bool is_wfd;
      u32 vbif_idx;
-    u32 clk_ctrl;
  };
  struct dpu_vbif_set_memtype_params {
      u32 xin_id;
      u32 vbif_idx;
-    u32 clk_ctrl;
      bool is_cacheable;
  };
  /**
   * struct dpu_vbif_set_qos_params - QoS remapper parameter
+ * @sspp: backing SSPP
   * @vbif_idx: vbif identifier
   * @xin_id: client interface identifier
- * @clk_ctrl: clock control identifier of the xin
   * @num: pipe identifier (debug only)
   * @is_rt: true if pipe is used in real-time use case
   */
  struct dpu_vbif_set_qos_params {
+    struct dpu_hw_sspp *sspp;
+    struct dpu_hw_wb *wb;
      u32 vbif_idx;
      u32 xin_id;
-    u32 clk_ctrl;
      u32 num;
      bool is_rt;
  };



Thanks,
Neil



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux