diff mbox series

[v4,5/5] drm/msm/dpu: move VBIF blocks handling to dpu_rm

Message ID 20220105231031.436020-6-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series [v4,1/5] drm/msm/dpu: drop unused lm_max_width from RM | expand

Commit Message

Dmitry Baryshkov Jan. 5, 2022, 11:10 p.m. UTC 
Move handling of VBIF blocks into dpu_rm. This serves the purpose of
unification of handling of all hardware blocks inside the DPU driver.
This removes hand-coded loops in dpu_vbif (which look for necessary VBIF
instance by looping through the dpu_kms->hw_vbif and comparing
vbif_idx).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 28 +--------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 21 ++++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
 6 files changed, 42 insertions(+), 47 deletions(-)

Comments

Stephen Boyd Jan. 7, 2022, 1:27 a.m. UTC | #1 
Quoting Dmitry Baryshkov (2022-01-05 15:10:31)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index bf4d72356a12..2301ac114920 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -78,6 +78,10 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>                 if (rm->hw_intf[i])
>                         dpu_hw_intf_destroy(rm->hw_intf[i]);
>         }
> +       for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++) {
> +               if (rm->hw_vbif[i])
> +                       dpu_hw_vbif_destroy(rm->hw_vbif[i]);

Maybe drop this check and pass NULL to dpu_hw_vbif_destroy() sometimes?
Then the check can be omitted and the braces dropped

> +       }
>
>         return 0;
>  }
> @@ -212,6 +216,23 @@ int dpu_rm_init(struct dpu_rm *rm,
>                 rm->dspp_blks[dspp->id - DSPP_0] = &hw->base;
>         }
>
> +       for (i = 0; i < cat->vbif_count; i++) {
> +               struct dpu_hw_vbif *hw;
> +               const struct dpu_vbif_cfg *vbif = &cat->vbif[i];
> +
> +               if (vbif->id < VBIF_0 || vbif->id >= VBIF_MAX) {
> +                       DPU_ERROR("skip vbif %d with invalid id\n", vbif->id);
> +                       continue;
> +               }
> +               hw = dpu_hw_vbif_init(vbif->id, mmio, cat);
> +               if (IS_ERR_OR_NULL(hw)) {
> +                       rc = PTR_ERR(hw);
> +                       DPU_ERROR("failed vbif object creation: err %d\n", rc);
> +                       goto fail;

If it's NULL then rc will be 0 and fail will return 0. Is that
intentional?

> +               }
> +               rm->hw_vbif[vbif->id - VBIF_0] = hw;
> +       }
> +
>         return 0;
>
>  fail:
Dmitry Baryshkov Jan. 21, 2022, 9:01 p.m. UTC | #2 
On 07/01/2022 04:27, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2022-01-05 15:10:31)
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index bf4d72356a12..2301ac114920 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -78,6 +78,10 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>                  if (rm->hw_intf[i])
>>                          dpu_hw_intf_destroy(rm->hw_intf[i]);
>>          }
>> +       for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++) {
>> +               if (rm->hw_vbif[i])
>> +                       dpu_hw_vbif_destroy(rm->hw_vbif[i]);
> 
> Maybe drop this check and pass NULL to dpu_hw_vbif_destroy() sometimes?
> Then the check can be omitted and the braces dropped

Nice idea. This also applies to dpu_hw_intf_destroy, so I'm going to 
apply it to the previous patch.

> 
>> +       }
>>
>>          return 0;
>>   }
>> @@ -212,6 +216,23 @@ int dpu_rm_init(struct dpu_rm *rm,
>>                  rm->dspp_blks[dspp->id - DSPP_0] = &hw->base;
>>          }
>>
>> +       for (i = 0; i < cat->vbif_count; i++) {
>> +               struct dpu_hw_vbif *hw;
>> +               const struct dpu_vbif_cfg *vbif = &cat->vbif[i];
>> +
>> +               if (vbif->id < VBIF_0 || vbif->id >= VBIF_MAX) {
>> +                       DPU_ERROR("skip vbif %d with invalid id\n", vbif->id);
>> +                       continue;
>> +               }
>> +               hw = dpu_hw_vbif_init(vbif->id, mmio, cat);
>> +               if (IS_ERR_OR_NULL(hw)) {
>> +                       rc = PTR_ERR(hw);
>> +                       DPU_ERROR("failed vbif object creation: err %d\n", rc);
>> +                       goto fail;
> 
> If it's NULL then rc will be 0 and fail will return 0. Is that
> intentional?

Actually no. And init functions can not return NULL. So let's fix it too.

> 
>> +               }
>> +               rm->hw_vbif[vbif->id - VBIF_0] = hw;
>> +       }
>> +
>>          return 0;
>>
>>   fail:
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
index 6417aa28d32c..895e86dabcb6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
@@ -8,6 +8,7 @@ 
 #include "dpu_hw_catalog.h"
 #include "dpu_hw_mdss.h"
 #include "dpu_hw_util.h"
+#include "dpu_hw_blk.h"
 
 struct dpu_hw_vbif;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 47fe11a84a77..4a1983d8561b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -782,8 +782,6 @@  static long dpu_kms_round_pixclk(struct msm_kms *kms, unsigned long rate,
 
 static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
 {
-	int i;
-
 	if (dpu_kms->hw_intr)
 		dpu_hw_intr_destroy(dpu_kms->hw_intr);
 	dpu_kms->hw_intr = NULL;
@@ -791,15 +789,6 @@  static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
 	/* safe to call these more than once during shutdown */
 	_dpu_kms_mmu_destroy(dpu_kms);
 
-	if (dpu_kms->catalog) {
-		for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
-			u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
-
-			if ((vbif_idx < VBIF_MAX) && dpu_kms->hw_vbif[vbif_idx])
-				dpu_hw_vbif_destroy(dpu_kms->hw_vbif[vbif_idx]);
-		}
-	}
-
 	if (dpu_kms->rm_init)
 		dpu_rm_destroy(&dpu_kms->rm);
 	dpu_kms->rm_init = false;
@@ -1027,7 +1016,7 @@  static int dpu_kms_hw_init(struct msm_kms *kms)
 {
 	struct dpu_kms *dpu_kms;
 	struct drm_device *dev;
-	int i, rc = -EINVAL;
+	int rc = -EINVAL;
 
 	if (!kms) {
 		DPU_ERROR("invalid kms\n");
@@ -1116,21 +1105,6 @@  static int dpu_kms_hw_init(struct msm_kms *kms)
 		goto power_error;
 	}
 
-	for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
-		u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
-
-		dpu_kms->hw_vbif[i] = dpu_hw_vbif_init(vbif_idx,
-				dpu_kms->vbif[vbif_idx], dpu_kms->catalog);
-		if (IS_ERR_OR_NULL(dpu_kms->hw_vbif[vbif_idx])) {
-			rc = PTR_ERR(dpu_kms->hw_vbif[vbif_idx]);
-			if (!dpu_kms->hw_vbif[vbif_idx])
-				rc = -EINVAL;
-			DPU_ERROR("failed to init vbif %d: %d\n", vbif_idx, rc);
-			dpu_kms->hw_vbif[vbif_idx] = NULL;
-			goto power_error;
-		}
-	}
-
 	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
 			_dpu_kms_get_clk(dpu_kms, "core"));
 	if (rc) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 3f518c809e33..b96c901483ae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -105,7 +105,6 @@  struct dpu_kms {
 	struct dpu_rm rm;
 	bool rm_init;
 
-	struct dpu_hw_vbif *hw_vbif[VBIF_MAX];
 	struct dpu_hw_mdp *hw_mdp;
 
 	bool has_danger_ctrl;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index bf4d72356a12..2301ac114920 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -78,6 +78,10 @@  int dpu_rm_destroy(struct dpu_rm *rm)
 		if (rm->hw_intf[i])
 			dpu_hw_intf_destroy(rm->hw_intf[i]);
 	}
+	for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++) {
+		if (rm->hw_vbif[i])
+			dpu_hw_vbif_destroy(rm->hw_vbif[i]);
+	}
 
 	return 0;
 }
@@ -212,6 +216,23 @@  int dpu_rm_init(struct dpu_rm *rm,
 		rm->dspp_blks[dspp->id - DSPP_0] = &hw->base;
 	}
 
+	for (i = 0; i < cat->vbif_count; i++) {
+		struct dpu_hw_vbif *hw;
+		const struct dpu_vbif_cfg *vbif = &cat->vbif[i];
+
+		if (vbif->id < VBIF_0 || vbif->id >= VBIF_MAX) {
+			DPU_ERROR("skip vbif %d with invalid id\n", vbif->id);
+			continue;
+		}
+		hw = dpu_hw_vbif_init(vbif->id, mmio, cat);
+		if (IS_ERR_OR_NULL(hw)) {
+			rc = PTR_ERR(hw);
+			DPU_ERROR("failed vbif object creation: err %d\n", rc);
+			goto fail;
+		}
+		rm->hw_vbif[vbif->id - VBIF_0] = hw;
+	}
+
 	return 0;
 
 fail:
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index 9b13200a050a..a15977bdceeb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -20,6 +20,7 @@  struct dpu_global_state;
  * @ctl_blks: array of ctl hardware resources
  * @hw_intf: array of intf hardware resources
  * @dspp_blks: array of dspp hardware resources
+ * @hw_vbif: array of vbif hardware resources
  */
 struct dpu_rm {
 	struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
@@ -28,6 +29,7 @@  struct dpu_rm {
 	struct dpu_hw_intf *hw_intf[INTF_MAX - INTF_0];
 	struct dpu_hw_blk *dspp_blks[DSPP_MAX - DSPP_0];
 	struct dpu_hw_blk *merge_3d_blks[MERGE_3D_MAX - MERGE_3D_0];
+	struct dpu_hw_vbif *hw_vbif[VBIF_MAX - VBIF_0];
 };
 
 /**
@@ -95,5 +97,15 @@  static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
 	return rm->hw_intf[intf_idx - INTF_0];
 }
 
+/**
+ * dpu_rm_get_vbif - Return a struct dpu_hw_vbif instance given it's index.
+ * @rm: DPU Resource Manager handle
+ * @vbif_idx: VBIF's index
+ */
+static inline struct dpu_hw_vbif *dpu_rm_get_vbif(struct dpu_rm *rm, enum dpu_vbif vbif_idx)
+{
+	return rm->hw_vbif[vbif_idx - VBIF_0];
+}
+
 #endif /* __DPU_RM_H__ */
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..00ac2aa81651 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -152,15 +152,10 @@  void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
 	struct dpu_hw_mdp *mdp;
 	bool forced_on = false;
 	u32 ot_lim;
-	int ret, i;
+	int ret;
 
 	mdp = dpu_kms->hw_mdp;
-
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
-		if (dpu_kms->hw_vbif[i] &&
-				dpu_kms->hw_vbif[i]->idx == params->vbif_idx)
-			vbif = dpu_kms->hw_vbif[i];
-	}
+	vbif = dpu_rm_get_vbif(&dpu_kms->rm, params->vbif_idx);
 
 	if (!vbif || !mdp) {
 		DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
@@ -216,14 +211,7 @@  void dpu_vbif_set_qos_remap(struct dpu_kms *dpu_kms,
 	}
 	mdp = dpu_kms->hw_mdp;
 
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
-		if (dpu_kms->hw_vbif[i] &&
-				dpu_kms->hw_vbif[i]->idx == params->vbif_idx) {
-			vbif = dpu_kms->hw_vbif[i];
-			break;
-		}
-	}
-
+	vbif = dpu_rm_get_vbif(&dpu_kms->rm, params->vbif_idx);
 	if (!vbif || !vbif->cap) {
 		DPU_ERROR("invalid vbif %d\n", params->vbif_idx);
 		return;
@@ -261,8 +249,8 @@  void dpu_vbif_clear_errors(struct dpu_kms *dpu_kms)
 	struct dpu_hw_vbif *vbif;
 	u32 i, pnd, src;
 
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
-		vbif = dpu_kms->hw_vbif[i];
+	for (i = VBIF_0; i < VBIF_MAX; i++) {
+		vbif = dpu_rm_get_vbif(&dpu_kms->rm, i);
 		if (vbif && vbif->ops.clear_errors) {
 			vbif->ops.clear_errors(vbif, &pnd, &src);
 			if (pnd || src) {
@@ -278,8 +266,8 @@  void dpu_vbif_init_memtypes(struct dpu_kms *dpu_kms)
 	struct dpu_hw_vbif *vbif;
 	int i, j;
 
-	for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
-		vbif = dpu_kms->hw_vbif[i];
+	for (i = VBIF_0; i < VBIF_MAX; i++) {
+		vbif = dpu_rm_get_vbif(&dpu_kms->rm, i);
 		if (vbif && vbif->cap && vbif->ops.set_mem_type) {
 			for (j = 0; j < vbif->cap->memtype_count; j++)
 				vbif->ops.set_mem_type(