[PATCH v5 0/6] drm/msm/dpu: simplify RM code

[PATCH v5 0/6] drm/msm/dpu: simplify RM code

[Dmitry Baryshkov]

INTF blocks are not really handled by resource manager, they are
assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
Then this allocation is passed to RM and then returned to then
dpu_encoder. So allocate them outside of RM and use them directly.

While we are at it, drop the lm_max_width from the RM and simplify VBIF
handling (move creation/destruction to the RM too). Once this and SSPP
handling patches are merged, the RM would handle lifetime of all
dpu_hw_* objects.

Changes since v4:
 - Changes IS_ERR_OR_NULL() to IS_ERR() in dpu_rm_init()
 - Removed if(!null) conditions when calling dpu_hw_intf_destroy() and
   dpu_hw_vbif_destroy()

Changes since v3:
 - Add missing DSPP blocks teardown
 - Remove dpu_hw_blk from dpu_hw_intf
 - move dpu_hw_vbif creation/destruction to the RM

Changes since v2:
 - Dropped DSPP, PP and MERGE_3D patches for now.

Changes since v1:
 - Split into separate patch series to ease review.

Dmitry Baryshkov (6):
  drm/msm/dpu: drop unused lm_max_width from RM
  drm/msm/dpu: add DSPP blocks teardown
  drm/msm/dpu: get INTF blocks directly rather than through RM
  drm/msm/dpu: stop embedding dpu_hw_blk into dpu_hw_intf
  drm/msm/dpu: fix error handling in dpu_rm_init
  drm/msm/dpu: move VBIF blocks handling to dpu_rm

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  36 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  16 ---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |   5 -
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |   8 --
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |   8 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h   |  11 --
 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       |   2 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 126 +++++-------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  31 ++++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c      |  26 +---
 12 files changed, 73 insertions(+), 225 deletions(-)

-- 
2.34.1

[PATCH v5 1/6] drm/msm/dpu: drop unused lm_max_width from RM

[Dmitry Baryshkov]

No code uses lm_max_width from resource manager, so drop it. Instead of
calculating the lm_max_width, code can use max_mixer_width field from
the hw catalog.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h |  4 ----
 2 files changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index f9c83d6e427a..b5b1ea1e4de6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -114,18 +114,6 @@ int dpu_rm_init(struct dpu_rm *rm,
 			goto fail;
 		}
 		rm->mixer_blks[lm->id - LM_0] = &hw->base;
-
-		if (!rm->lm_max_width) {
-			rm->lm_max_width = lm->sblk->maxwidth;
-		} else if (rm->lm_max_width != lm->sblk->maxwidth) {
-			/*
-			 * Don't expect to have hw where lm max widths differ.
-			 * If found, take the min.
-			 */
-			DPU_ERROR("unsupported: lm maxwidth differs\n");
-			if (rm->lm_max_width > lm->sblk->maxwidth)
-				rm->lm_max_width = lm->sblk->maxwidth;
-		}
 	}
 
 	for (i = 0; i < cat->merge_3d_count; i++) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index 1f12c8d5b8aa..0f27759211b5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -20,8 +20,6 @@ struct dpu_global_state;
  * @ctl_blks: array of ctl hardware resources
  * @intf_blks: array of intf hardware resources
  * @dspp_blks: array of dspp hardware resources
- * @lm_max_width: cached layer mixer maximum width
- * @rm_lock: resource manager mutex
  */
 struct dpu_rm {
 	struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
@@ -30,8 +28,6 @@ struct dpu_rm {
 	struct dpu_hw_blk *intf_blks[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];
-
-	uint32_t lm_max_width;
 };
 
 /**
-- 
2.34.1

Re: [Freedreno] [PATCH v5 1/6] drm/msm/dpu: drop unused lm_max_width from RM

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> No code uses lm_max_width from resource manager, so drop it. Instead of
> calculating the lm_max_width, code can use max_mixer_width field from
> the hw catalog.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h |  4 ----
>   2 files changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index f9c83d6e427a..b5b1ea1e4de6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -114,18 +114,6 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			goto fail;
>   		}
>   		rm->mixer_blks[lm->id - LM_0] = &hw->base;
> -
> -		if (!rm->lm_max_width) {
> -			rm->lm_max_width = lm->sblk->maxwidth;
> -		} else if (rm->lm_max_width != lm->sblk->maxwidth) {
> -			/*
> -			 * Don't expect to have hw where lm max widths differ.
> -			 * If found, take the min.
> -			 */
> -			DPU_ERROR("unsupported: lm maxwidth differs\n");
> -			if (rm->lm_max_width > lm->sblk->maxwidth)
> -				rm->lm_max_width = lm->sblk->maxwidth;
> -		}
>   	}
>   
>   	for (i = 0; i < cat->merge_3d_count; i++) {
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index 1f12c8d5b8aa..0f27759211b5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -20,8 +20,6 @@ struct dpu_global_state;
>    * @ctl_blks: array of ctl hardware resources
>    * @intf_blks: array of intf hardware resources
>    * @dspp_blks: array of dspp hardware resources
> - * @lm_max_width: cached layer mixer maximum width
> - * @rm_lock: resource manager mutex
>    */
>   struct dpu_rm {
>   	struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
> @@ -30,8 +28,6 @@ struct dpu_rm {
>   	struct dpu_hw_blk *intf_blks[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];
> -
> -	uint32_t lm_max_width;
>   };
>   
>   /**

[PATCH v5 2/6] drm/msm/dpu: add DSPP blocks teardown

[Dmitry Baryshkov]

Add missing calls to dpu_hw_dspp_destroy() to free resources allocated
for DSPP hardware blocks.

Fixes: e47616df008b ("drm/msm/dpu: add support for color processing blocks in dpu driver")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index b5b1ea1e4de6..63ed0d7df848 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -35,6 +35,14 @@ int dpu_rm_destroy(struct dpu_rm *rm)
 {
 	int i;
 
+	for (i = 0; i < ARRAY_SIZE(rm->dspp_blks); i++) {
+		struct dpu_hw_dspp *hw;
+
+		if (rm->dspp_blks[i]) {
+			hw = to_dpu_hw_dspp(rm->dspp_blks[i]);
+			dpu_hw_dspp_destroy(hw);
+		}
+	}
 	for (i = 0; i < ARRAY_SIZE(rm->pingpong_blks); i++) {
 		struct dpu_hw_pingpong *hw;
 
-- 
2.34.1

Re: [PATCH v5 2/6] drm/msm/dpu: add DSPP blocks teardown

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> Add missing calls to dpu_hw_dspp_destroy() to free resources allocated
> for DSPP hardware blocks.
> 
> Fixes: e47616df008b ("drm/msm/dpu: add support for color processing blocks in dpu driver")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index b5b1ea1e4de6..63ed0d7df848 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -35,6 +35,14 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>   {
>   	int i;
>   
> +	for (i = 0; i < ARRAY_SIZE(rm->dspp_blks); i++) {
> +		struct dpu_hw_dspp *hw;
> +
> +		if (rm->dspp_blks[i]) {
> +			hw = to_dpu_hw_dspp(rm->dspp_blks[i]);
> +			dpu_hw_dspp_destroy(hw);
> +		}
> +	}
>   	for (i = 0; i < ARRAY_SIZE(rm->pingpong_blks); i++) {
>   		struct dpu_hw_pingpong *hw;
>   

[PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Dmitry Baryshkov]

INTF blocks are not really handled by resource manager, they are
assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
Then this allocation is passed to RM and then returned to then
dpu_encoder.
So allocate them outside of RM and use them directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 ++-----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
 8 files changed, 16 insertions(+), 134 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..11f7126728db 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct drm_encoder *drm_enc)
 	return linecount;
 }
 
-void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
-				  struct dpu_encoder_hw_resources *hw_res)
-{
-	struct dpu_encoder_virt *dpu_enc = NULL;
-	int i = 0;
-
-	dpu_enc = to_dpu_encoder_virt(drm_enc);
-	DPU_DEBUG_ENC(dpu_enc, "\n");
-
-	/* Query resources used by phys encs, expected to be without overlap */
-	memset(hw_res, 0, sizeof(*hw_res));
-
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-		if (phys->ops.get_hw_resources)
-			phys->ops.get_hw_resources(phys, hw_res);
-	}
-}
-
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
 	struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
 	int num_lm, num_ctl, num_pp;
-	int i, j;
+	int i;
 
 	if (!drm_enc) {
 		DPU_ERROR("invalid encoder\n");
@@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 	cstate->num_mixers = num_lm;
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		int num_blk;
-		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
 		if (!dpu_enc->hw_pp[i]) {
@@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 		phys->hw_pp = dpu_enc->hw_pp[i];
 		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
-		num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
-			global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
-			hw_blk, ARRAY_SIZE(hw_blk));
-		for (j = 0; j < num_blk; j++) {
-			struct dpu_hw_intf *hw_intf;
-
-			hw_intf = to_dpu_hw_intf(hw_blk[i]);
-			if (hw_intf->idx == phys->intf_idx)
-				phys->hw_intf = hw_intf;
-		}
+		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
 
 		if (!phys->hw_intf) {
 			DPU_ERROR_ENC(dpu_enc,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index e241914a9677..722dd7db6bdf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -18,22 +18,6 @@
 
 #define IDLE_TIMEOUT	(66 - 16/2)
 
-/**
- * Encoder functions and data types
- * @intfs:	Interfaces this encoder is using, INTF_MODE_NONE if unused
- */
-struct dpu_encoder_hw_resources {
-	enum dpu_intf_mode intfs[INTF_MAX];
-};
-
-/**
- * dpu_encoder_get_hw_resources - Populate table of required hardware resources
- * @encoder:	encoder pointer
- * @hw_res:	resource table to populate with encoder required resources
- */
-void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
-				  struct dpu_encoder_hw_resources *hw_res);
-
 /**
  * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e7270eb6b84b..42febfce79c7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
  * @disable:			DRM Call. Disable mode.
  * @atomic_check:		DRM Call. Atomic check new DRM state.
  * @destroy:			DRM Call. Destroy and release resources.
- * @get_hw_resources:		Populate the structure with the hardware
- *				resources that this phys_enc is using.
- *				Expect no overlap between phys_encs.
  * @control_vblank_irq		Register/Deregister for VBLANK IRQ
  * @wait_for_commit_done:	Wait for hardware to have flushed the
  *				current pending frames to hardware
@@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
 			    struct drm_crtc_state *crtc_state,
 			    struct drm_connector_state *conn_state);
 	void (*destroy)(struct dpu_encoder_phys *encoder);
-	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
-				 struct dpu_encoder_hw_resources *hw_res);
 	int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
 	int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
 	int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
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 34a6940d12c5..7d2beea9cc4e 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
@@ -534,13 +534,6 @@ static void dpu_encoder_phys_cmd_destroy(struct dpu_encoder_phys *phys_enc)
 	kfree(cmd_enc);
 }
 
-static void dpu_encoder_phys_cmd_get_hw_resources(
-		struct dpu_encoder_phys *phys_enc,
-		struct dpu_encoder_hw_resources *hw_res)
-{
-	hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
-}
-
 static void dpu_encoder_phys_cmd_prepare_for_kickoff(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
-	ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
 	ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
 	ops->wait_for_commit_done = dpu_encoder_phys_cmd_wait_for_commit_done;
 	ops->prepare_for_kickoff = dpu_encoder_phys_cmd_prepare_for_kickoff;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89cd456..db6a9b896e42 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -465,13 +465,6 @@ static void dpu_encoder_phys_vid_destroy(struct dpu_encoder_phys *phys_enc)
 	kfree(phys_enc);
 }
 
-static void dpu_encoder_phys_vid_get_hw_resources(
-		struct dpu_encoder_phys *phys_enc,
-		struct dpu_encoder_hw_resources *hw_res)
-{
-	hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
-}
-
 static int dpu_encoder_phys_vid_wait_for_vblank(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -680,7 +673,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
-	ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
 	ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
 	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
 	ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 2d385b4b7f5e..3f518c809e33 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -144,7 +144,6 @@ struct dpu_global_state {
 	uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
 	uint32_t mixer_to_enc_id[LM_MAX - LM_0];
 	uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
-	uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
 	uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 63ed0d7df848..8df21a46308e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t *res_map, int idx,
  */
 struct dpu_rm_requirements {
 	struct msm_display_topology topology;
-	struct dpu_encoder_hw_resources hw_res;
 };
 
 int dpu_rm_destroy(struct dpu_rm *rm)
@@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
 	return 0;
 }
 
-static int _dpu_rm_reserve_intf(
-		struct dpu_rm *rm,
-		struct dpu_global_state *global_state,
-		uint32_t enc_id,
-		uint32_t id)
-{
-	int idx = id - INTF_0;
-
-	if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
-		DPU_ERROR("invalid intf id: %d", id);
-		return -EINVAL;
-	}
-
-	if (!rm->intf_blks[idx]) {
-		DPU_ERROR("couldn't find intf id %d\n", id);
-		return -EINVAL;
-	}
-
-	if (reserved_by_other(global_state->intf_to_enc_id, idx, enc_id)) {
-		DPU_ERROR("intf id %d already reserved\n", id);
-		return -ENAVAIL;
-	}
-
-	global_state->intf_to_enc_id[idx] = enc_id;
-	return 0;
-}
-
-static int _dpu_rm_reserve_intf_related_hw(
-		struct dpu_rm *rm,
-		struct dpu_global_state *global_state,
-		uint32_t enc_id,
-		struct dpu_encoder_hw_resources *hw_res)
-{
-	int i, ret = 0;
-	u32 id;
-
-	for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
-		if (hw_res->intfs[i] == INTF_MODE_NONE)
-			continue;
-		id = i + INTF_0;
-		ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
-		if (ret)
-			return ret;
-	}
-
-	return ret;
-}
-
 static int _dpu_rm_make_reservation(
 		struct dpu_rm *rm,
 		struct dpu_global_state *global_state,
@@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
 		return ret;
 	}
 
-	ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, enc->base.id,
-				&reqs->hw_res);
-	if (ret)
-		return ret;
-
 	return ret;
 }
 
@@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
 		struct dpu_rm_requirements *reqs,
 		struct msm_display_topology req_topology)
 {
-	dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
-
 	reqs->topology = req_topology;
 
 	DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
@@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state *global_state,
 		ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
 	_dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
 		ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
-	_dpu_rm_clear_mapping(global_state->intf_to_enc_id,
-		ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
 }
 
 int dpu_rm_reserve(
@@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
 		hw_to_enc_id = global_state->ctl_to_enc_id;
 		max_blks = ARRAY_SIZE(rm->ctl_blks);
 		break;
-	case DPU_HW_BLK_INTF:
-		hw_blks = rm->intf_blks;
-		hw_to_enc_id = global_state->intf_to_enc_id;
-		max_blks = ARRAY_SIZE(rm->intf_blks);
-		break;
 	case DPU_HW_BLK_DSPP:
 		hw_blks = rm->dspp_blks;
 		hw_to_enc_id = global_state->dspp_to_enc_id;
@@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
 
 	return num_blks;
 }
+
+struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
+{
+	return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
+}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index 0f27759211b5..ee50f6651b6e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state *global_state,
 int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
 	struct dpu_global_state *global_state, uint32_t enc_id,
 	enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int blks_size);
+
+/**
+ * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given it's index.
+ * @rm: DPU Resource Manager handle
+ * @intf_idx: INTF's index
+ */
+struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx);
+
 #endif /* __DPU_RM_H__ */
 
-- 
2.34.1

Re: [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> INTF blocks are not really handled by resource manager, they are
> assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
> Then this allocation is passed to RM and then returned to then
> dpu_encoder.
> So allocate them outside of RM and use them directly.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

I have some questions about this approach.

Agreed, that when there is one encoder tied to one interface, we dont 
need to go through RM because RM just gives what the encoder asks. 
Instead we can directly get the interface from encoder.

But what happens when multiple displays are requesting the same interface?

There are use-cases which we have handled especially in mid-tier chips 
which do not have external display where between mode-switches
OR between some user triggered events DSI0 was shared between two displays.

So  lets say display 1 (with encoder X) requests DSI0
and display 2 (with encoder Y) also requests DSI0,

with this change, the code will allow that as there is no interface to
encoder mapping.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 ++-----------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
>   8 files changed, 16 insertions(+), 134 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1e648db439f9..11f7126728db 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct drm_encoder *drm_enc)
>   	return linecount;
>   }
>   
> -void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
> -				  struct dpu_encoder_hw_resources *hw_res)
> -{
> -	struct dpu_encoder_virt *dpu_enc = NULL;
> -	int i = 0;
> -
> -	dpu_enc = to_dpu_encoder_virt(drm_enc);
> -	DPU_DEBUG_ENC(dpu_enc, "\n");
> -
> -	/* Query resources used by phys encs, expected to be without overlap */
> -	memset(hw_res, 0, sizeof(*hw_res));
> -
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -
> -		if (phys->ops.get_hw_resources)
> -			phys->ops.get_hw_resources(phys, hw_res);
> -	}
> -}
> -
>   static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
>   {
>   	struct dpu_encoder_virt *dpu_enc = NULL;
> @@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   	struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
>   	struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
>   	int num_lm, num_ctl, num_pp;
> -	int i, j;
> +	int i;
>   
>   	if (!drm_enc) {
>   		DPU_ERROR("invalid encoder\n");
> @@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   	cstate->num_mixers = num_lm;
>   
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		int num_blk;
> -		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
>   		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>   
>   		if (!dpu_enc->hw_pp[i]) {
> @@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   		phys->hw_pp = dpu_enc->hw_pp[i];
>   		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>   
> -		num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
> -			global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
> -			hw_blk, ARRAY_SIZE(hw_blk));
> -		for (j = 0; j < num_blk; j++) {
> -			struct dpu_hw_intf *hw_intf;
> -
> -			hw_intf = to_dpu_hw_intf(hw_blk[i]);
> -			if (hw_intf->idx == phys->intf_idx)
> -				phys->hw_intf = hw_intf;
> -		}
> +		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> +			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
>   
>   		if (!phys->hw_intf) {
>   			DPU_ERROR_ENC(dpu_enc,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index e241914a9677..722dd7db6bdf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -18,22 +18,6 @@
>   
>   #define IDLE_TIMEOUT	(66 - 16/2)
>   
> -/**
> - * Encoder functions and data types
> - * @intfs:	Interfaces this encoder is using, INTF_MODE_NONE if unused
> - */
> -struct dpu_encoder_hw_resources {
> -	enum dpu_intf_mode intfs[INTF_MAX];
> -};
> -
> -/**
> - * dpu_encoder_get_hw_resources - Populate table of required hardware resources
> - * @encoder:	encoder pointer
> - * @hw_res:	resource table to populate with encoder required resources
> - */
> -void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
> -				  struct dpu_encoder_hw_resources *hw_res);
> -
>   /**
>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>    * @encoder:	encoder pointer
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index e7270eb6b84b..42febfce79c7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
>    * @disable:			DRM Call. Disable mode.
>    * @atomic_check:		DRM Call. Atomic check new DRM state.
>    * @destroy:			DRM Call. Destroy and release resources.
> - * @get_hw_resources:		Populate the structure with the hardware
> - *				resources that this phys_enc is using.
> - *				Expect no overlap between phys_encs.
>    * @control_vblank_irq		Register/Deregister for VBLANK IRQ
>    * @wait_for_commit_done:	Wait for hardware to have flushed the
>    *				current pending frames to hardware
> @@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
>   			    struct drm_crtc_state *crtc_state,
>   			    struct drm_connector_state *conn_state);
>   	void (*destroy)(struct dpu_encoder_phys *encoder);
> -	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
> -				 struct dpu_encoder_hw_resources *hw_res);
>   	int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool enable);
>   	int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>   	int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
> 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 34a6940d12c5..7d2beea9cc4e 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
> @@ -534,13 +534,6 @@ static void dpu_encoder_phys_cmd_destroy(struct dpu_encoder_phys *phys_enc)
>   	kfree(cmd_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_get_hw_resources(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct dpu_encoder_hw_resources *hw_res)
> -{
> -	hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
> -}
> -
>   static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> -	ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
>   	ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
>   	ops->wait_for_commit_done = dpu_encoder_phys_cmd_wait_for_commit_done;
>   	ops->prepare_for_kickoff = dpu_encoder_phys_cmd_prepare_for_kickoff;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index ddd9d89cd456..db6a9b896e42 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -465,13 +465,6 @@ static void dpu_encoder_phys_vid_destroy(struct dpu_encoder_phys *phys_enc)
>   	kfree(phys_enc);
>   }
>   
> -static void dpu_encoder_phys_vid_get_hw_resources(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct dpu_encoder_hw_resources *hw_res)
> -{
> -	hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
> -}
> -
>   static int dpu_encoder_phys_vid_wait_for_vblank(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -680,7 +673,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;
> -	ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
>   	ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
>   	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
>   	ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index 2d385b4b7f5e..3f518c809e33 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -144,7 +144,6 @@ struct dpu_global_state {
>   	uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
>   	uint32_t mixer_to_enc_id[LM_MAX - LM_0];
>   	uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
> -	uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
>   	uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
>   };
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 63ed0d7df848..8df21a46308e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t *res_map, int idx,
>    */
>   struct dpu_rm_requirements {
>   	struct msm_display_topology topology;
> -	struct dpu_encoder_hw_resources hw_res;
>   };
>   
>   int dpu_rm_destroy(struct dpu_rm *rm)
> @@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
>   	return 0;
>   }
>   
> -static int _dpu_rm_reserve_intf(
> -		struct dpu_rm *rm,
> -		struct dpu_global_state *global_state,
> -		uint32_t enc_id,
> -		uint32_t id)
> -{
> -	int idx = id - INTF_0;
> -
> -	if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
> -		DPU_ERROR("invalid intf id: %d", id);
> -		return -EINVAL;
> -	}
> -
> -	if (!rm->intf_blks[idx]) {
> -		DPU_ERROR("couldn't find intf id %d\n", id);
> -		return -EINVAL;
> -	}
> -
> -	if (reserved_by_other(global_state->intf_to_enc_id, idx, enc_id)) {
> -		DPU_ERROR("intf id %d already reserved\n", id);
> -		return -ENAVAIL;
> -	}
> -
> -	global_state->intf_to_enc_id[idx] = enc_id;
> -	return 0;
> -}
> -
> -static int _dpu_rm_reserve_intf_related_hw(
> -		struct dpu_rm *rm,
> -		struct dpu_global_state *global_state,
> -		uint32_t enc_id,
> -		struct dpu_encoder_hw_resources *hw_res)
> -{
> -	int i, ret = 0;
> -	u32 id;
> -
> -	for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
> -		if (hw_res->intfs[i] == INTF_MODE_NONE)
> -			continue;
> -		id = i + INTF_0;
> -		ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return ret;
> -}
> -
>   static int _dpu_rm_make_reservation(
>   		struct dpu_rm *rm,
>   		struct dpu_global_state *global_state,
> @@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
>   		return ret;
>   	}
>   
> -	ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, enc->base.id,
> -				&reqs->hw_res);
> -	if (ret)
> -		return ret;
> -
>   	return ret;
>   }
>   
> @@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
>   		struct dpu_rm_requirements *reqs,
>   		struct msm_display_topology req_topology)
>   {
> -	dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
> -
>   	reqs->topology = req_topology;
>   
>   	DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
> @@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state *global_state,
>   		ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
>   	_dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
>   		ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
> -	_dpu_rm_clear_mapping(global_state->intf_to_enc_id,
> -		ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
>   }
>   
>   int dpu_rm_reserve(
> @@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>   		hw_to_enc_id = global_state->ctl_to_enc_id;
>   		max_blks = ARRAY_SIZE(rm->ctl_blks);
>   		break;
> -	case DPU_HW_BLK_INTF:
> -		hw_blks = rm->intf_blks;
> -		hw_to_enc_id = global_state->intf_to_enc_id;
> -		max_blks = ARRAY_SIZE(rm->intf_blks);
> -		break;
>   	case DPU_HW_BLK_DSPP:
>   		hw_blks = rm->dspp_blks;
>   		hw_to_enc_id = global_state->dspp_to_enc_id;
> @@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>   
>   	return num_blks;
>   }
> +
> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
> +{
> +	return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
> +}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index 0f27759211b5..ee50f6651b6e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state *global_state,
>   int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>   	struct dpu_global_state *global_state, uint32_t enc_id,
>   	enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int blks_size);
> +
> +/**
> + * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given it's index.
> + * @rm: DPU Resource Manager handle
> + * @intf_idx: INTF's index
> + */
> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx);
> +
>   #endif /* __DPU_RM_H__ */
>   

Re: [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Dmitry Baryshkov]

On 10/02/2022 03:25, Abhinav Kumar wrote:
> 
> 
> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>> INTF blocks are not really handled by resource manager, they are
>> assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
>> Then this allocation is passed to RM and then returned to then
>> dpu_encoder.
>> So allocate them outside of RM and use them directly.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> 
> I have some questions about this approach.
> 
> Agreed, that when there is one encoder tied to one interface, we dont 
> need to go through RM because RM just gives what the encoder asks. 
> Instead we can directly get the interface from encoder.
> 
> But what happens when multiple displays are requesting the same interface?
> 
> There are use-cases which we have handled especially in mid-tier chips 
> which do not have external display where between mode-switches
> OR between some user triggered events DSI0 was shared between two displays.
> 
> So  lets say display 1 (with encoder X) requests DSI0
> and display 2 (with encoder Y) also requests DSI0,

All the encoders are allocated in the loop over possible DSI/DP 
interfaces. Thus it's not possible to have two encoders being driven by 
the same DSI entity. Moreover, the MSM DSI manager would not be able to 
cope with such cases.

In my opinion, the proper way to handle such cases would be a bridge 
which would generate hotplug events and toggle display pipeline inbetween.

> with this change, the code will allow that as there is no interface to
> encoder mapping.
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 ++-----------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
>>   8 files changed, 16 insertions(+), 134 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1e648db439f9..11f7126728db 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct drm_encoder 
>> *drm_enc)
>>       return linecount;
>>   }
>> -void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>> -                  struct dpu_encoder_hw_resources *hw_res)
>> -{
>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>> -    int i = 0;
>> -
>> -    dpu_enc = to_dpu_encoder_virt(drm_enc);
>> -    DPU_DEBUG_ENC(dpu_enc, "\n");
>> -
>> -    /* Query resources used by phys encs, expected to be without 
>> overlap */
>> -    memset(hw_res, 0, sizeof(*hw_res));
>> -
>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> -
>> -        if (phys->ops.get_hw_resources)
>> -            phys->ops.get_hw_resources(phys, hw_res);
>> -    }
>> -}
>> -
>>   static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
>>   {
>>       struct dpu_encoder_virt *dpu_enc = NULL;
>> @@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct 
>> drm_encoder *drm_enc,
>>       struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
>>       struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
>>       int num_lm, num_ctl, num_pp;
>> -    int i, j;
>> +    int i;
>>       if (!drm_enc) {
>>           DPU_ERROR("invalid encoder\n");
>> @@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct 
>> drm_encoder *drm_enc,
>>       cstate->num_mixers = num_lm;
>>       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -        int num_blk;
>> -        struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>           if (!dpu_enc->hw_pp[i]) {
>> @@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct 
>> drm_encoder *drm_enc,
>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>> -        num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
>> -            global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
>> -            hw_blk, ARRAY_SIZE(hw_blk));
>> -        for (j = 0; j < num_blk; j++) {
>> -            struct dpu_hw_intf *hw_intf;
>> -
>> -            hw_intf = to_dpu_hw_intf(hw_blk[i]);
>> -            if (hw_intf->idx == phys->intf_idx)
>> -                phys->hw_intf = hw_intf;
>> -        }
>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> phys->intf_idx);
>>           if (!phys->hw_intf) {
>>               DPU_ERROR_ENC(dpu_enc,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index e241914a9677..722dd7db6bdf 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -18,22 +18,6 @@
>>   #define IDLE_TIMEOUT    (66 - 16/2)
>> -/**
>> - * Encoder functions and data types
>> - * @intfs:    Interfaces this encoder is using, INTF_MODE_NONE if unused
>> - */
>> -struct dpu_encoder_hw_resources {
>> -    enum dpu_intf_mode intfs[INTF_MAX];
>> -};
>> -
>> -/**
>> - * dpu_encoder_get_hw_resources - Populate table of required hardware 
>> resources
>> - * @encoder:    encoder pointer
>> - * @hw_res:    resource table to populate with encoder required 
>> resources
>> - */
>> -void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>> -                  struct dpu_encoder_hw_resources *hw_res);
>> -
>>   /**
>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's 
>> assigned to
>>    * @encoder:    encoder pointer
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index e7270eb6b84b..42febfce79c7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
>>    * @disable:            DRM Call. Disable mode.
>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>>    * @destroy:            DRM Call. Destroy and release resources.
>> - * @get_hw_resources:        Populate the structure with the hardware
>> - *                resources that this phys_enc is using.
>> - *                Expect no overlap between phys_encs.
>>    * @control_vblank_irq        Register/Deregister for VBLANK IRQ
>>    * @wait_for_commit_done:    Wait for hardware to have flushed the
>>    *                current pending frames to hardware
>> @@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
>>                   struct drm_crtc_state *crtc_state,
>>                   struct drm_connector_state *conn_state);
>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>> -    void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>> -                 struct dpu_encoder_hw_resources *hw_res);
>>       int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool 
>> enable);
>>       int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>>       int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
>> 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 34a6940d12c5..7d2beea9cc4e 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
>> @@ -534,13 +534,6 @@ static void dpu_encoder_phys_cmd_destroy(struct 
>> dpu_encoder_phys *phys_enc)
>>       kfree(cmd_enc);
>>   }
>> -static void dpu_encoder_phys_cmd_get_hw_resources(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        struct dpu_encoder_hw_resources *hw_res)
>> -{
>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
>> -}
>> -
>>   static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>> -    ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
>>       ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
>>       ops->wait_for_commit_done = 
>> dpu_encoder_phys_cmd_wait_for_commit_done;
>>       ops->prepare_for_kickoff = 
>> dpu_encoder_phys_cmd_prepare_for_kickoff;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index ddd9d89cd456..db6a9b896e42 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -465,13 +465,6 @@ static void dpu_encoder_phys_vid_destroy(struct 
>> dpu_encoder_phys *phys_enc)
>>       kfree(phys_enc);
>>   }
>> -static void dpu_encoder_phys_vid_get_hw_resources(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        struct dpu_encoder_hw_resources *hw_res)
>> -{
>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
>> -}
>> -
>>   static int dpu_encoder_phys_vid_wait_for_vblank(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -680,7 +673,6 @@ static void dpu_encoder_phys_vid_init_ops(struct 
>> dpu_encoder_phys_ops *ops)
>>       ops->enable = dpu_encoder_phys_vid_enable;
>>       ops->disable = dpu_encoder_phys_vid_disable;
>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>> -    ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
>>       ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
>>       ops->wait_for_commit_done = 
>> dpu_encoder_phys_vid_wait_for_commit_done;
>>       ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> index 2d385b4b7f5e..3f518c809e33 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>> @@ -144,7 +144,6 @@ struct dpu_global_state {
>>       uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
>>       uint32_t mixer_to_enc_id[LM_MAX - LM_0];
>>       uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
>> -    uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
>>       uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 63ed0d7df848..8df21a46308e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t 
>> *res_map, int idx,
>>    */
>>   struct dpu_rm_requirements {
>>       struct msm_display_topology topology;
>> -    struct dpu_encoder_hw_resources hw_res;
>>   };
>>   int dpu_rm_destroy(struct dpu_rm *rm)
>> @@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
>>       return 0;
>>   }
>> -static int _dpu_rm_reserve_intf(
>> -        struct dpu_rm *rm,
>> -        struct dpu_global_state *global_state,
>> -        uint32_t enc_id,
>> -        uint32_t id)
>> -{
>> -    int idx = id - INTF_0;
>> -
>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
>> -        DPU_ERROR("invalid intf id: %d", id);
>> -        return -EINVAL;
>> -    }
>> -
>> -    if (!rm->intf_blks[idx]) {
>> -        DPU_ERROR("couldn't find intf id %d\n", id);
>> -        return -EINVAL;
>> -    }
>> -
>> -    if (reserved_by_other(global_state->intf_to_enc_id, idx, enc_id)) {
>> -        DPU_ERROR("intf id %d already reserved\n", id);
>> -        return -ENAVAIL;
>> -    }
>> -
>> -    global_state->intf_to_enc_id[idx] = enc_id;
>> -    return 0;
>> -}
>> -
>> -static int _dpu_rm_reserve_intf_related_hw(
>> -        struct dpu_rm *rm,
>> -        struct dpu_global_state *global_state,
>> -        uint32_t enc_id,
>> -        struct dpu_encoder_hw_resources *hw_res)
>> -{
>> -    int i, ret = 0;
>> -    u32 id;
>> -
>> -    for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
>> -        if (hw_res->intfs[i] == INTF_MODE_NONE)
>> -            continue;
>> -        id = i + INTF_0;
>> -        ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
>> -        if (ret)
>> -            return ret;
>> -    }
>> -
>> -    return ret;
>> -}
>> -
>>   static int _dpu_rm_make_reservation(
>>           struct dpu_rm *rm,
>>           struct dpu_global_state *global_state,
>> @@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
>>           return ret;
>>       }
>> -    ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, 
>> enc->base.id,
>> -                &reqs->hw_res);
>> -    if (ret)
>> -        return ret;
>> -
>>       return ret;
>>   }
>> @@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
>>           struct dpu_rm_requirements *reqs,
>>           struct msm_display_topology req_topology)
>>   {
>> -    dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
>> -
>>       reqs->topology = req_topology;
>>       DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
>> @@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state 
>> *global_state,
>>           ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
>>       _dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
>>           ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>> -    _dpu_rm_clear_mapping(global_state->intf_to_enc_id,
>> -        ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
>>   }
>>   int dpu_rm_reserve(
>> @@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>           hw_to_enc_id = global_state->ctl_to_enc_id;
>>           max_blks = ARRAY_SIZE(rm->ctl_blks);
>>           break;
>> -    case DPU_HW_BLK_INTF:
>> -        hw_blks = rm->intf_blks;
>> -        hw_to_enc_id = global_state->intf_to_enc_id;
>> -        max_blks = ARRAY_SIZE(rm->intf_blks);
>> -        break;
>>       case DPU_HW_BLK_DSPP:
>>           hw_blks = rm->dspp_blks;
>>           hw_to_enc_id = global_state->dspp_to_enc_id;
>> @@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>       return num_blks;
>>   }
>> +
>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf 
>> intf_idx)
>> +{
>> +    return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
>> +}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> index 0f27759211b5..ee50f6651b6e 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> @@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state 
>> *global_state,
>>   int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>       struct dpu_global_state *global_state, uint32_t enc_id,
>>       enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int 
>> blks_size);
>> +
>> +/**
>> + * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given it's 
>> index.
>> + * @rm: DPU Resource Manager handle
>> + * @intf_idx: INTF's index
>> + */
>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf 
>> intf_idx);
>> +
>>   #endif /* __DPU_RM_H__ */


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Abhinav Kumar]

On 2/10/2022 1:32 AM, Dmitry Baryshkov wrote:
> On 10/02/2022 03:25, Abhinav Kumar wrote:
>>
>>
>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>> INTF blocks are not really handled by resource manager, they are
>>> assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
>>> Then this allocation is passed to RM and then returned to then
>>> dpu_encoder.
>>> So allocate them outside of RM and use them directly.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>
>> I have some questions about this approach.
>>
>> Agreed, that when there is one encoder tied to one interface, we dont 
>> need to go through RM because RM just gives what the encoder asks. 
>> Instead we can directly get the interface from encoder.
>>
>> But what happens when multiple displays are requesting the same 
>> interface?
>>
>> There are use-cases which we have handled especially in mid-tier chips 
>> which do not have external display where between mode-switches
>> OR between some user triggered events DSI0 was shared between two 
>> displays.
>>
>> So  lets say display 1 (with encoder X) requests DSI0
>> and display 2 (with encoder Y) also requests DSI0,
> 
> All the encoders are allocated in the loop over possible DSI/DP 
> interfaces. Thus it's not possible to have two encoders being driven by 
> the same DSI entity. Moreover, the MSM DSI manager would not be able to 
> cope with such cases.
> 
> In my opinion, the proper way to handle such cases would be a bridge 
> which would generate hotplug events and toggle display pipeline inbetween.

Like I wrote in the previous comment, today there is always one encoder 
requesting for one interface (or two for split DSI ) and there is no 
chance of a conflict ( the loop that you are referring to ).

And yes I am aware that DSI does not support this today.

Myself and a few others internally are looking ahead and thinking of 
what can come in the future and something which we already support 
downstream, which is to support sharing the interface across encoders.

So what happens downstream is the controller ID comes from the device tree:

6266 	info->num_of_h_tiles = display->ctrl_count;
6267 	for (i = 0; i < info->num_of_h_tiles; i++)
6268 		info->h_tile_instance[i] = display->ctrl[i].ctrl->cell_index;
6269

There is a concept of a dsi_display quite similar to the dsi_manager we 
have upstream. There are also implementations already in place of a 
shared display, where like I was describing in IRC, the DSI0 can be 
shared across two encoders between a mode_set().

Hotplug is not necessarily the only trigger which happens, it can just
be a simple mode_set() in between.

In that case, this encoder to intf mapping in the RM will protect 
against concurrent hardware use.

Ofcourse, all this is not present today but will someday :)

That time this will have to be brought back.

So I thought I must mention the use-cases which will get potentially 
affected with this change of removing INTF from RM.

If for the sake of code simplicity, if we want to ignore the possibility
of this coming back later, please let me know what you think and we can
take this further by acking it.


> 
>> with this change, the code will allow that as there is no interface to
>> encoder mapping.
>>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 ++-----------------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
>>>   8 files changed, 16 insertions(+), 134 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 1e648db439f9..11f7126728db 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct drm_encoder 
>>> *drm_enc)
>>>       return linecount;
>>>   }
>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>>> -                  struct dpu_encoder_hw_resources *hw_res)
>>> -{
>>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>>> -    int i = 0;
>>> -
>>> -    dpu_enc = to_dpu_encoder_virt(drm_enc);
>>> -    DPU_DEBUG_ENC(dpu_enc, "\n");
>>> -
>>> -    /* Query resources used by phys encs, expected to be without 
>>> overlap */
>>> -    memset(hw_res, 0, sizeof(*hw_res));
>>> -
>>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>> -
>>> -        if (phys->ops.get_hw_resources)
>>> -            phys->ops.get_hw_resources(phys, hw_res);
>>> -    }
>>> -}
>>> -
>>>   static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
>>>   {
>>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>> @@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct 
>>> drm_encoder *drm_enc,
>>>       struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
>>>       struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
>>>       int num_lm, num_ctl, num_pp;
>>> -    int i, j;
>>> +    int i;
>>>       if (!drm_enc) {
>>>           DPU_ERROR("invalid encoder\n");
>>> @@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct 
>>> drm_encoder *drm_enc,
>>>       cstate->num_mixers = num_lm;
>>>       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>> -        int num_blk;
>>> -        struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
>>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>           if (!dpu_enc->hw_pp[i]) {
>>> @@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct 
>>> drm_encoder *drm_enc,
>>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>> -        num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
>>> -            global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
>>> -            hw_blk, ARRAY_SIZE(hw_blk));
>>> -        for (j = 0; j < num_blk; j++) {
>>> -            struct dpu_hw_intf *hw_intf;
>>> -
>>> -            hw_intf = to_dpu_hw_intf(hw_blk[i]);
>>> -            if (hw_intf->idx == phys->intf_idx)
>>> -                phys->hw_intf = hw_intf;
>>> -        }
>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>> phys->intf_idx);
>>>           if (!phys->hw_intf) {
>>>               DPU_ERROR_ENC(dpu_enc,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index e241914a9677..722dd7db6bdf 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -18,22 +18,6 @@
>>>   #define IDLE_TIMEOUT    (66 - 16/2)
>>> -/**
>>> - * Encoder functions and data types
>>> - * @intfs:    Interfaces this encoder is using, INTF_MODE_NONE if 
>>> unused
>>> - */
>>> -struct dpu_encoder_hw_resources {
>>> -    enum dpu_intf_mode intfs[INTF_MAX];
>>> -};
>>> -
>>> -/**
>>> - * dpu_encoder_get_hw_resources - Populate table of required 
>>> hardware resources
>>> - * @encoder:    encoder pointer
>>> - * @hw_res:    resource table to populate with encoder required 
>>> resources
>>> - */
>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>>> -                  struct dpu_encoder_hw_resources *hw_res);
>>> -
>>>   /**
>>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's 
>>> assigned to
>>>    * @encoder:    encoder pointer
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> index e7270eb6b84b..42febfce79c7 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>> @@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
>>>    * @disable:            DRM Call. Disable mode.
>>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>>>    * @destroy:            DRM Call. Destroy and release resources.
>>> - * @get_hw_resources:        Populate the structure with the hardware
>>> - *                resources that this phys_enc is using.
>>> - *                Expect no overlap between phys_encs.
>>>    * @control_vblank_irq        Register/Deregister for VBLANK IRQ
>>>    * @wait_for_commit_done:    Wait for hardware to have flushed the
>>>    *                current pending frames to hardware
>>> @@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
>>>                   struct drm_crtc_state *crtc_state,
>>>                   struct drm_connector_state *conn_state);
>>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>>> -    void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>>> -                 struct dpu_encoder_hw_resources *hw_res);
>>>       int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool 
>>> enable);
>>>       int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>>>       int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
>>> 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 34a6940d12c5..7d2beea9cc4e 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
>>> @@ -534,13 +534,6 @@ static void dpu_encoder_phys_cmd_destroy(struct 
>>> dpu_encoder_phys *phys_enc)
>>>       kfree(cmd_enc);
>>>   }
>>> -static void dpu_encoder_phys_cmd_get_hw_resources(
>>> -        struct dpu_encoder_phys *phys_enc,
>>> -        struct dpu_encoder_hw_resources *hw_res)
>>> -{
>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
>>> -}
>>> -
>>>   static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>>           struct dpu_encoder_phys *phys_enc)
>>>   {
>>> @@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>>> -    ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
>>>       ops->control_vblank_irq = dpu_encoder_phys_cmd_control_vblank_irq;
>>>       ops->wait_for_commit_done = 
>>> dpu_encoder_phys_cmd_wait_for_commit_done;
>>>       ops->prepare_for_kickoff = 
>>> dpu_encoder_phys_cmd_prepare_for_kickoff;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> index ddd9d89cd456..db6a9b896e42 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>> @@ -465,13 +465,6 @@ static void dpu_encoder_phys_vid_destroy(struct 
>>> dpu_encoder_phys *phys_enc)
>>>       kfree(phys_enc);
>>>   }
>>> -static void dpu_encoder_phys_vid_get_hw_resources(
>>> -        struct dpu_encoder_phys *phys_enc,
>>> -        struct dpu_encoder_hw_resources *hw_res)
>>> -{
>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
>>> -}
>>> -
>>>   static int dpu_encoder_phys_vid_wait_for_vblank(
>>>           struct dpu_encoder_phys *phys_enc)
>>>   {
>>> @@ -680,7 +673,6 @@ static void dpu_encoder_phys_vid_init_ops(struct 
>>> dpu_encoder_phys_ops *ops)
>>>       ops->enable = dpu_encoder_phys_vid_enable;
>>>       ops->disable = dpu_encoder_phys_vid_disable;
>>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>>> -    ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
>>>       ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
>>>       ops->wait_for_commit_done = 
>>> dpu_encoder_phys_vid_wait_for_commit_done;
>>>       ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> index 2d385b4b7f5e..3f518c809e33 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>> @@ -144,7 +144,6 @@ struct dpu_global_state {
>>>       uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
>>>       uint32_t mixer_to_enc_id[LM_MAX - LM_0];
>>>       uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
>>> -    uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
>>>       uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
>>>   };
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> index 63ed0d7df848..8df21a46308e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t 
>>> *res_map, int idx,
>>>    */
>>>   struct dpu_rm_requirements {
>>>       struct msm_display_topology topology;
>>> -    struct dpu_encoder_hw_resources hw_res;
>>>   };
>>>   int dpu_rm_destroy(struct dpu_rm *rm)
>>> @@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
>>>       return 0;
>>>   }
>>> -static int _dpu_rm_reserve_intf(
>>> -        struct dpu_rm *rm,
>>> -        struct dpu_global_state *global_state,
>>> -        uint32_t enc_id,
>>> -        uint32_t id)
>>> -{
>>> -    int idx = id - INTF_0;
>>> -
>>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
>>> -        DPU_ERROR("invalid intf id: %d", id);
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (!rm->intf_blks[idx]) {
>>> -        DPU_ERROR("couldn't find intf id %d\n", id);
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    if (reserved_by_other(global_state->intf_to_enc_id, idx, enc_id)) {
>>> -        DPU_ERROR("intf id %d already reserved\n", id);
>>> -        return -ENAVAIL;
>>> -    }
>>> -
>>> -    global_state->intf_to_enc_id[idx] = enc_id;
>>> -    return 0;
>>> -}
>>> -
>>> -static int _dpu_rm_reserve_intf_related_hw(
>>> -        struct dpu_rm *rm,
>>> -        struct dpu_global_state *global_state,
>>> -        uint32_t enc_id,
>>> -        struct dpu_encoder_hw_resources *hw_res)
>>> -{
>>> -    int i, ret = 0;
>>> -    u32 id;
>>> -
>>> -    for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
>>> -        if (hw_res->intfs[i] == INTF_MODE_NONE)
>>> -            continue;
>>> -        id = i + INTF_0;
>>> -        ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
>>> -        if (ret)
>>> -            return ret;
>>> -    }
>>> -
>>> -    return ret;
>>> -}
>>> -
>>>   static int _dpu_rm_make_reservation(
>>>           struct dpu_rm *rm,
>>>           struct dpu_global_state *global_state,
>>> @@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
>>>           return ret;
>>>       }
>>> -    ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, 
>>> enc->base.id,
>>> -                &reqs->hw_res);
>>> -    if (ret)
>>> -        return ret;
>>> -
>>>       return ret;
>>>   }
>>> @@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
>>>           struct dpu_rm_requirements *reqs,
>>>           struct msm_display_topology req_topology)
>>>   {
>>> -    dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
>>> -
>>>       reqs->topology = req_topology;
>>>       DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
>>> @@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state 
>>> *global_state,
>>>           ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
>>>       _dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
>>>           ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>>> -    _dpu_rm_clear_mapping(global_state->intf_to_enc_id,
>>> -        ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
>>>   }
>>>   int dpu_rm_reserve(
>>> @@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct dpu_rm 
>>> *rm,
>>>           hw_to_enc_id = global_state->ctl_to_enc_id;
>>>           max_blks = ARRAY_SIZE(rm->ctl_blks);
>>>           break;
>>> -    case DPU_HW_BLK_INTF:
>>> -        hw_blks = rm->intf_blks;
>>> -        hw_to_enc_id = global_state->intf_to_enc_id;
>>> -        max_blks = ARRAY_SIZE(rm->intf_blks);
>>> -        break;
>>>       case DPU_HW_BLK_DSPP:
>>>           hw_blks = rm->dspp_blks;
>>>           hw_to_enc_id = global_state->dspp_to_enc_id;
>>> @@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>>       return num_blks;
>>>   }
>>> +
>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf 
>>> intf_idx)
>>> +{
>>> +    return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
>>> +}
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> index 0f27759211b5..ee50f6651b6e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>> @@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state 
>>> *global_state,
>>>   int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>>       struct dpu_global_state *global_state, uint32_t enc_id,
>>>       enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int 
>>> blks_size);
>>> +
>>> +/**
>>> + * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given it's 
>>> index.
>>> + * @rm: DPU Resource Manager handle
>>> + * @intf_idx: INTF's index
>>> + */
>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf 
>>> intf_idx);
>>> +
>>>   #endif /* __DPU_RM_H__ */
> 
> 

Re: [Freedreno] [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Dmitry Baryshkov]

On 11/02/2022 02:31, Abhinav Kumar wrote:
> 
> 
> On 2/10/2022 1:32 AM, Dmitry Baryshkov wrote:
>> On 10/02/2022 03:25, Abhinav Kumar wrote:
>>>
>>>
>>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>>> INTF blocks are not really handled by resource manager, they are
>>>> assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
>>>> Then this allocation is passed to RM and then returned to then
>>>> dpu_encoder.
>>>> So allocate them outside of RM and use them directly.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>
>>> I have some questions about this approach.
>>>
>>> Agreed, that when there is one encoder tied to one interface, we dont 
>>> need to go through RM because RM just gives what the encoder asks. 
>>> Instead we can directly get the interface from encoder.
>>>
>>> But what happens when multiple displays are requesting the same 
>>> interface?
>>>
>>> There are use-cases which we have handled especially in mid-tier 
>>> chips which do not have external display where between mode-switches
>>> OR between some user triggered events DSI0 was shared between two 
>>> displays.
>>>
>>> So  lets say display 1 (with encoder X) requests DSI0
>>> and display 2 (with encoder Y) also requests DSI0,
>>
>> All the encoders are allocated in the loop over possible DSI/DP 
>> interfaces. Thus it's not possible to have two encoders being driven 
>> by the same DSI entity. Moreover, the MSM DSI manager would not be 
>> able to cope with such cases.
>>
>> In my opinion, the proper way to handle such cases would be a bridge 
>> which would generate hotplug events and toggle display pipeline 
>> inbetween.
> 
> Like I wrote in the previous comment, today there is always one encoder 
> requesting for one interface (or two for split DSI ) and there is no 
> chance of a conflict ( the loop that you are referring to ).
> 
> And yes I am aware that DSI does not support this today.
> 
> Myself and a few others internally are looking ahead and thinking of 
> what can come in the future and something which we already support 
> downstream, which is to support sharing the interface across encoders.
> 
> So what happens downstream is the controller ID comes from the device tree:
> 
> 6266     info->num_of_h_tiles = display->ctrl_count;
> 6267     for (i = 0; i < info->num_of_h_tiles; i++)
> 6268         info->h_tile_instance[i] = display->ctrl[i].ctrl->cell_index;
> 6269
> 
> There is a concept of a dsi_display quite similar to the dsi_manager we 
> have upstream. There are also implementations already in place of a 
> shared display, where like I was describing in IRC, the DSI0 can be 
> shared across two encoders between a mode_set().
> 
> Hotplug is not necessarily the only trigger which happens, it can just
> be a simple mode_set() in between.

Even having a modeset, you will have to trigger the electrical switch 
between the DSI sinks. It should be a separate device sitting in both 
DSI pipelines, so you'll have a question of having it in sync.

> In that case, this encoder to intf mapping in the RM will protect 
> against concurrent hardware use.
> 
> Ofcourse, all this is not present today but will someday :)
> 
> That time this will have to be brought back.
> 
> So I thought I must mention the use-cases which will get potentially 
> affected with this change of removing INTF from RM.
> 
> If for the sake of code simplicity, if we want to ignore the possibility
> of this coming back later, please let me know what you think and we can
> take this further by acking it.

As I wrote on the IRC, I suppose that neither you nor me can imagine the 
way such usecases might be actually implemented when the need arises.

You have an idea of using multiple encoders. Fine idea, nothing 
particularly wrong with it.

I'd use single pipeline and multiple panels being switched. Or the panel 
driver providing modes for both small and larger panels (or internal and 
external) at the same time and guarding the switch on its own.

The engineer that would implement this feature might come with another 
approach (and might e.g. implement dynamic drm_bridge pipelines inside 
the core).

And if we bring in DSI split link into the equation, the situation might 
become even more complex (with one of encoders owning a half of the DSI 
and and enother encoder owning another half).

We should always think about future use cases, but we should not 
over-predict the future. Unnecessary abstractions complicate the code, 
making the driver harder to comprehend, harder to modify and more 
error-prone.

That said I'd suggest/ask to ack and accept this patch.

> 
> 
>>
>>> with this change, the code will allow that as there is no interface to
>>> encoder mapping.
>>>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
>>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
>>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
>>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 
>>>> ++-----------------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
>>>>   8 files changed, 16 insertions(+), 134 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 1e648db439f9..11f7126728db 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct 
>>>> drm_encoder *drm_enc)
>>>>       return linecount;
>>>>   }
>>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>>>> -                  struct dpu_encoder_hw_resources *hw_res)
>>>> -{
>>>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>>>> -    int i = 0;
>>>> -
>>>> -    dpu_enc = to_dpu_encoder_virt(drm_enc);
>>>> -    DPU_DEBUG_ENC(dpu_enc, "\n");
>>>> -
>>>> -    /* Query resources used by phys encs, expected to be without 
>>>> overlap */
>>>> -    memset(hw_res, 0, sizeof(*hw_res));
>>>> -
>>>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>> -
>>>> -        if (phys->ops.get_hw_resources)
>>>> -            phys->ops.get_hw_resources(phys, hw_res);
>>>> -    }
>>>> -}
>>>> -
>>>>   static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
>>>>   {
>>>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>>> @@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct 
>>>> drm_encoder *drm_enc,
>>>>       struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
>>>>       struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
>>>>       int num_lm, num_ctl, num_pp;
>>>> -    int i, j;
>>>> +    int i;
>>>>       if (!drm_enc) {
>>>>           DPU_ERROR("invalid encoder\n");
>>>> @@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct 
>>>> drm_encoder *drm_enc,
>>>>       cstate->num_mixers = num_lm;
>>>>       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>>> -        int num_blk;
>>>> -        struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
>>>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>>           if (!dpu_enc->hw_pp[i]) {
>>>> @@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct 
>>>> drm_encoder *drm_enc,
>>>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>> -        num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
>>>> -            global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
>>>> -            hw_blk, ARRAY_SIZE(hw_blk));
>>>> -        for (j = 0; j < num_blk; j++) {
>>>> -            struct dpu_hw_intf *hw_intf;
>>>> -
>>>> -            hw_intf = to_dpu_hw_intf(hw_blk[i]);
>>>> -            if (hw_intf->idx == phys->intf_idx)
>>>> -                phys->hw_intf = hw_intf;
>>>> -        }
>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>>> phys->intf_idx);
>>>>           if (!phys->hw_intf) {
>>>>               DPU_ERROR_ENC(dpu_enc,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index e241914a9677..722dd7db6bdf 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -18,22 +18,6 @@
>>>>   #define IDLE_TIMEOUT    (66 - 16/2)
>>>> -/**
>>>> - * Encoder functions and data types
>>>> - * @intfs:    Interfaces this encoder is using, INTF_MODE_NONE if 
>>>> unused
>>>> - */
>>>> -struct dpu_encoder_hw_resources {
>>>> -    enum dpu_intf_mode intfs[INTF_MAX];
>>>> -};
>>>> -
>>>> -/**
>>>> - * dpu_encoder_get_hw_resources - Populate table of required 
>>>> hardware resources
>>>> - * @encoder:    encoder pointer
>>>> - * @hw_res:    resource table to populate with encoder required 
>>>> resources
>>>> - */
>>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>>>> -                  struct dpu_encoder_hw_resources *hw_res);
>>>> -
>>>>   /**
>>>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's 
>>>> assigned to
>>>>    * @encoder:    encoder pointer
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> index e7270eb6b84b..42febfce79c7 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> @@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
>>>>    * @disable:            DRM Call. Disable mode.
>>>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>>>>    * @destroy:            DRM Call. Destroy and release resources.
>>>> - * @get_hw_resources:        Populate the structure with the hardware
>>>> - *                resources that this phys_enc is using.
>>>> - *                Expect no overlap between phys_encs.
>>>>    * @control_vblank_irq        Register/Deregister for VBLANK IRQ
>>>>    * @wait_for_commit_done:    Wait for hardware to have flushed the
>>>>    *                current pending frames to hardware
>>>> @@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
>>>>                   struct drm_crtc_state *crtc_state,
>>>>                   struct drm_connector_state *conn_state);
>>>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>>>> -    void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>>>> -                 struct dpu_encoder_hw_resources *hw_res);
>>>>       int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool 
>>>> enable);
>>>>       int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>>>>       int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
>>>> 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 34a6940d12c5..7d2beea9cc4e 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
>>>> @@ -534,13 +534,6 @@ static void dpu_encoder_phys_cmd_destroy(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>>       kfree(cmd_enc);
>>>>   }
>>>> -static void dpu_encoder_phys_cmd_get_hw_resources(
>>>> -        struct dpu_encoder_phys *phys_enc,
>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>> -{
>>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
>>>> -}
>>>> -
>>>>   static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>>>           struct dpu_encoder_phys *phys_enc)
>>>>   {
>>>> @@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>>>> -    ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
>>>>       ops->control_vblank_irq = 
>>>> dpu_encoder_phys_cmd_control_vblank_irq;
>>>>       ops->wait_for_commit_done = 
>>>> dpu_encoder_phys_cmd_wait_for_commit_done;
>>>>       ops->prepare_for_kickoff = 
>>>> dpu_encoder_phys_cmd_prepare_for_kickoff;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> index ddd9d89cd456..db6a9b896e42 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> @@ -465,13 +465,6 @@ static void dpu_encoder_phys_vid_destroy(struct 
>>>> dpu_encoder_phys *phys_enc)
>>>>       kfree(phys_enc);
>>>>   }
>>>> -static void dpu_encoder_phys_vid_get_hw_resources(
>>>> -        struct dpu_encoder_phys *phys_enc,
>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>> -{
>>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
>>>> -}
>>>> -
>>>>   static int dpu_encoder_phys_vid_wait_for_vblank(
>>>>           struct dpu_encoder_phys *phys_enc)
>>>>   {
>>>> @@ -680,7 +673,6 @@ static void dpu_encoder_phys_vid_init_ops(struct 
>>>> dpu_encoder_phys_ops *ops)
>>>>       ops->enable = dpu_encoder_phys_vid_enable;
>>>>       ops->disable = dpu_encoder_phys_vid_disable;
>>>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>>>> -    ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
>>>>       ops->control_vblank_irq = 
>>>> dpu_encoder_phys_vid_control_vblank_irq;
>>>>       ops->wait_for_commit_done = 
>>>> dpu_encoder_phys_vid_wait_for_commit_done;
>>>>       ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> index 2d385b4b7f5e..3f518c809e33 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> @@ -144,7 +144,6 @@ struct dpu_global_state {
>>>>       uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
>>>>       uint32_t mixer_to_enc_id[LM_MAX - LM_0];
>>>>       uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
>>>> -    uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
>>>>       uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
>>>>   };
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> index 63ed0d7df848..8df21a46308e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t 
>>>> *res_map, int idx,
>>>>    */
>>>>   struct dpu_rm_requirements {
>>>>       struct msm_display_topology topology;
>>>> -    struct dpu_encoder_hw_resources hw_res;
>>>>   };
>>>>   int dpu_rm_destroy(struct dpu_rm *rm)
>>>> @@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
>>>>       return 0;
>>>>   }
>>>> -static int _dpu_rm_reserve_intf(
>>>> -        struct dpu_rm *rm,
>>>> -        struct dpu_global_state *global_state,
>>>> -        uint32_t enc_id,
>>>> -        uint32_t id)
>>>> -{
>>>> -    int idx = id - INTF_0;
>>>> -
>>>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
>>>> -        DPU_ERROR("invalid intf id: %d", id);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    if (!rm->intf_blks[idx]) {
>>>> -        DPU_ERROR("couldn't find intf id %d\n", id);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    if (reserved_by_other(global_state->intf_to_enc_id, idx, 
>>>> enc_id)) {
>>>> -        DPU_ERROR("intf id %d already reserved\n", id);
>>>> -        return -ENAVAIL;
>>>> -    }
>>>> -
>>>> -    global_state->intf_to_enc_id[idx] = enc_id;
>>>> -    return 0;
>>>> -}
>>>> -
>>>> -static int _dpu_rm_reserve_intf_related_hw(
>>>> -        struct dpu_rm *rm,
>>>> -        struct dpu_global_state *global_state,
>>>> -        uint32_t enc_id,
>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>> -{
>>>> -    int i, ret = 0;
>>>> -    u32 id;
>>>> -
>>>> -    for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
>>>> -        if (hw_res->intfs[i] == INTF_MODE_NONE)
>>>> -            continue;
>>>> -        id = i + INTF_0;
>>>> -        ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
>>>> -        if (ret)
>>>> -            return ret;
>>>> -    }
>>>> -
>>>> -    return ret;
>>>> -}
>>>> -
>>>>   static int _dpu_rm_make_reservation(
>>>>           struct dpu_rm *rm,
>>>>           struct dpu_global_state *global_state,
>>>> @@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
>>>>           return ret;
>>>>       }
>>>> -    ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, 
>>>> enc->base.id,
>>>> -                &reqs->hw_res);
>>>> -    if (ret)
>>>> -        return ret;
>>>> -
>>>>       return ret;
>>>>   }
>>>> @@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
>>>>           struct dpu_rm_requirements *reqs,
>>>>           struct msm_display_topology req_topology)
>>>>   {
>>>> -    dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
>>>> -
>>>>       reqs->topology = req_topology;
>>>>       DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
>>>> @@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state 
>>>> *global_state,
>>>>           ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
>>>>       _dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
>>>>           ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>>>> -    _dpu_rm_clear_mapping(global_state->intf_to_enc_id,
>>>> -        ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
>>>>   }
>>>>   int dpu_rm_reserve(
>>>> @@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct dpu_rm 
>>>> *rm,
>>>>           hw_to_enc_id = global_state->ctl_to_enc_id;
>>>>           max_blks = ARRAY_SIZE(rm->ctl_blks);
>>>>           break;
>>>> -    case DPU_HW_BLK_INTF:
>>>> -        hw_blks = rm->intf_blks;
>>>> -        hw_to_enc_id = global_state->intf_to_enc_id;
>>>> -        max_blks = ARRAY_SIZE(rm->intf_blks);
>>>> -        break;
>>>>       case DPU_HW_BLK_DSPP:
>>>>           hw_blks = rm->dspp_blks;
>>>>           hw_to_enc_id = global_state->dspp_to_enc_id;
>>>> @@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm 
>>>> *rm,
>>>>       return num_blks;
>>>>   }
>>>> +
>>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum 
>>>> dpu_intf intf_idx)
>>>> +{
>>>> +    return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> index 0f27759211b5..ee50f6651b6e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> @@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state 
>>>> *global_state,
>>>>   int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>>>       struct dpu_global_state *global_state, uint32_t enc_id,
>>>>       enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int 
>>>> blks_size);
>>>> +
>>>> +/**
>>>> + * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given 
>>>> it's index.
>>>> + * @rm: DPU Resource Manager handle
>>>> + * @intf_idx: INTF's index
>>>> + */
>>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum 
>>>> dpu_intf intf_idx);
>>>> +
>>>>   #endif /* __DPU_RM_H__ */
>>
>>


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v5 3/6] drm/msm/dpu: get INTF blocks directly rather than through RM

[Abhinav Kumar]

On 2/11/2022 5:47 AM, Dmitry Baryshkov wrote:
> On 11/02/2022 02:31, Abhinav Kumar wrote:
>>
>>
>> On 2/10/2022 1:32 AM, Dmitry Baryshkov wrote:
>>> On 10/02/2022 03:25, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>>>> INTF blocks are not really handled by resource manager, they are
>>>>> assigned at dpu_encoder_setup_display using dpu_encoder_get_intf().
>>>>> Then this allocation is passed to RM and then returned to then
>>>>> dpu_encoder.
>>>>> So allocate them outside of RM and use them directly.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>
>>>> I have some questions about this approach.
>>>>
>>>> Agreed, that when there is one encoder tied to one interface, we 
>>>> dont need to go through RM because RM just gives what the encoder 
>>>> asks. Instead we can directly get the interface from encoder.
>>>>
>>>> But what happens when multiple displays are requesting the same 
>>>> interface?
>>>>
>>>> There are use-cases which we have handled especially in mid-tier 
>>>> chips which do not have external display where between mode-switches
>>>> OR between some user triggered events DSI0 was shared between two 
>>>> displays.
>>>>
>>>> So  lets say display 1 (with encoder X) requests DSI0
>>>> and display 2 (with encoder Y) also requests DSI0,
>>>
>>> All the encoders are allocated in the loop over possible DSI/DP 
>>> interfaces. Thus it's not possible to have two encoders being driven 
>>> by the same DSI entity. Moreover, the MSM DSI manager would not be 
>>> able to cope with such cases.
>>>
>>> In my opinion, the proper way to handle such cases would be a bridge 
>>> which would generate hotplug events and toggle display pipeline 
>>> inbetween.
>>
>> Like I wrote in the previous comment, today there is always one 
>> encoder requesting for one interface (or two for split DSI ) and there 
>> is no chance of a conflict ( the loop that you are referring to ).
>>
>> And yes I am aware that DSI does not support this today.
>>
>> Myself and a few others internally are looking ahead and thinking of 
>> what can come in the future and something which we already support 
>> downstream, which is to support sharing the interface across encoders.
>>
>> So what happens downstream is the controller ID comes from the device 
>> tree:
>>
>> 6266     info->num_of_h_tiles = display->ctrl_count;
>> 6267     for (i = 0; i < info->num_of_h_tiles; i++)
>> 6268         info->h_tile_instance[i] = 
>> display->ctrl[i].ctrl->cell_index;
>> 6269
>>
>> There is a concept of a dsi_display quite similar to the dsi_manager 
>> we have upstream. There are also implementations already in place of a 
>> shared display, where like I was describing in IRC, the DSI0 can be 
>> shared across two encoders between a mode_set().
>>
>> Hotplug is not necessarily the only trigger which happens, it can just
>> be a simple mode_set() in between.
> 
> Even having a modeset, you will have to trigger the electrical switch 
> between the DSI sinks. It should be a separate device sitting in both 
> DSI pipelines, so you'll have a question of having it in sync.
> 
>> In that case, this encoder to intf mapping in the RM will protect 
>> against concurrent hardware use.
>>
>> Ofcourse, all this is not present today but will someday :)
>>
>> That time this will have to be brought back.
>>
>> So I thought I must mention the use-cases which will get potentially 
>> affected with this change of removing INTF from RM.
>>
>> If for the sake of code simplicity, if we want to ignore the possibility
>> of this coming back later, please let me know what you think and we can
>> take this further by acking it.
> 
> As I wrote on the IRC, I suppose that neither you nor me can imagine the 
> way such usecases might be actually implemented when the need arises.
> 
> You have an idea of using multiple encoders. Fine idea, nothing 
> particularly wrong with it.
> 
> I'd use single pipeline and multiple panels being switched. Or the panel 
> driver providing modes for both small and larger panels (or internal and 
> external) at the same time and guarding the switch on its own.
> 
> The engineer that would implement this feature might come with another 
> approach (and might e.g. implement dynamic drm_bridge pipelines inside 
> the core).
> 
> And if we bring in DSI split link into the equation, the situation might 
> become even more complex (with one of encoders owning a half of the DSI 
> and and enother encoder owning another half).
> 
> We should always think about future use cases, but we should not 
> over-predict the future. Unnecessary abstractions complicate the code, 
> making the driver harder to comprehend, harder to modify and more 
> error-prone.
> 
> That said I'd suggest/ask to ack and accept this patch.

Alright, since we have discussed pros and cons of this approach, in the 
absence of any current code/features which will get impacted due to this,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

I will make an equivalent change in the WB series.


> 
>>
>>
>>>
>>>> with this change, the code will allow that as there is no interface to
>>>> encoder mapping.
>>>>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 36 +---------
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   | 16 -----
>>>>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 --
>>>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  8 ---
>>>>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  8 ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h       |  1 -
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c        | 68 
>>>>> ++-----------------
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h        |  8 +++
>>>>>   8 files changed, 16 insertions(+), 134 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> index 1e648db439f9..11f7126728db 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>> @@ -420,26 +420,6 @@ int dpu_encoder_get_linecount(struct 
>>>>> drm_encoder *drm_enc)
>>>>>       return linecount;
>>>>>   }
>>>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
>>>>> -                  struct dpu_encoder_hw_resources *hw_res)
>>>>> -{
>>>>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>>>>> -    int i = 0;
>>>>> -
>>>>> -    dpu_enc = to_dpu_encoder_virt(drm_enc);
>>>>> -    DPU_DEBUG_ENC(dpu_enc, "\n");
>>>>> -
>>>>> -    /* Query resources used by phys encs, expected to be without 
>>>>> overlap */
>>>>> -    memset(hw_res, 0, sizeof(*hw_res));
>>>>> -
>>>>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>>>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>>> -
>>>>> -        if (phys->ops.get_hw_resources)
>>>>> -            phys->ops.get_hw_resources(phys, hw_res);
>>>>> -    }
>>>>> -}
>>>>> -
>>>>>   static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
>>>>>   {
>>>>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>>>> @@ -973,7 +953,7 @@ static void dpu_encoder_virt_mode_set(struct 
>>>>> drm_encoder *drm_enc,
>>>>>       struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC];
>>>>>       struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL };
>>>>>       int num_lm, num_ctl, num_pp;
>>>>> -    int i, j;
>>>>> +    int i;
>>>>>       if (!drm_enc) {
>>>>>           DPU_ERROR("invalid encoder\n");
>>>>> @@ -1040,8 +1020,6 @@ static void dpu_encoder_virt_mode_set(struct 
>>>>> drm_encoder *drm_enc,
>>>>>       cstate->num_mixers = num_lm;
>>>>>       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>>>> -        int num_blk;
>>>>> -        struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
>>>>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>>>           if (!dpu_enc->hw_pp[i]) {
>>>>> @@ -1059,16 +1037,8 @@ static void dpu_encoder_virt_mode_set(struct 
>>>>> drm_encoder *drm_enc,
>>>>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>>>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>>> -        num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm,
>>>>> -            global_state, drm_enc->base.id, DPU_HW_BLK_INTF,
>>>>> -            hw_blk, ARRAY_SIZE(hw_blk));
>>>>> -        for (j = 0; j < num_blk; j++) {
>>>>> -            struct dpu_hw_intf *hw_intf;
>>>>> -
>>>>> -            hw_intf = to_dpu_hw_intf(hw_blk[i]);
>>>>> -            if (hw_intf->idx == phys->intf_idx)
>>>>> -                phys->hw_intf = hw_intf;
>>>>> -        }
>>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>>>> phys->intf_idx);
>>>>>           if (!phys->hw_intf) {
>>>>>               DPU_ERROR_ENC(dpu_enc,
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>> index e241914a9677..722dd7db6bdf 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>>> @@ -18,22 +18,6 @@
>>>>>   #define IDLE_TIMEOUT    (66 - 16/2)
>>>>> -/**
>>>>> - * Encoder functions and data types
>>>>> - * @intfs:    Interfaces this encoder is using, INTF_MODE_NONE if 
>>>>> unused
>>>>> - */
>>>>> -struct dpu_encoder_hw_resources {
>>>>> -    enum dpu_intf_mode intfs[INTF_MAX];
>>>>> -};
>>>>> -
>>>>> -/**
>>>>> - * dpu_encoder_get_hw_resources - Populate table of required 
>>>>> hardware resources
>>>>> - * @encoder:    encoder pointer
>>>>> - * @hw_res:    resource table to populate with encoder required 
>>>>> resources
>>>>> - */
>>>>> -void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>>>>> -                  struct dpu_encoder_hw_resources *hw_res);
>>>>> -
>>>>>   /**
>>>>>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's 
>>>>> assigned to
>>>>>    * @encoder:    encoder pointer
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> index e7270eb6b84b..42febfce79c7 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>> @@ -91,9 +91,6 @@ struct dpu_encoder_virt_ops {
>>>>>    * @disable:            DRM Call. Disable mode.
>>>>>    * @atomic_check:        DRM Call. Atomic check new DRM state.
>>>>>    * @destroy:            DRM Call. Destroy and release resources.
>>>>> - * @get_hw_resources:        Populate the structure with the hardware
>>>>> - *                resources that this phys_enc is using.
>>>>> - *                Expect no overlap between phys_encs.
>>>>>    * @control_vblank_irq        Register/Deregister for VBLANK IRQ
>>>>>    * @wait_for_commit_done:    Wait for hardware to have flushed the
>>>>>    *                current pending frames to hardware
>>>>> @@ -129,8 +126,6 @@ struct dpu_encoder_phys_ops {
>>>>>                   struct drm_crtc_state *crtc_state,
>>>>>                   struct drm_connector_state *conn_state);
>>>>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>>>>> -    void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>>>>> -                 struct dpu_encoder_hw_resources *hw_res);
>>>>>       int (*control_vblank_irq)(struct dpu_encoder_phys *enc, bool 
>>>>> enable);
>>>>>       int (*wait_for_commit_done)(struct dpu_encoder_phys *phys_enc);
>>>>>       int (*wait_for_tx_complete)(struct dpu_encoder_phys *phys_enc);
>>>>> 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 34a6940d12c5..7d2beea9cc4e 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
>>>>> @@ -534,13 +534,6 @@ static void 
>>>>> dpu_encoder_phys_cmd_destroy(struct dpu_encoder_phys *phys_enc)
>>>>>       kfree(cmd_enc);
>>>>>   }
>>>>> -static void dpu_encoder_phys_cmd_get_hw_resources(
>>>>> -        struct dpu_encoder_phys *phys_enc,
>>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>>> -{
>>>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_CMD;
>>>>> -}
>>>>> -
>>>>>   static void dpu_encoder_phys_cmd_prepare_for_kickoff(
>>>>>           struct dpu_encoder_phys *phys_enc)
>>>>>   {
>>>>> @@ -736,7 +729,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>>>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>>>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>>>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>>>>> -    ops->get_hw_resources = dpu_encoder_phys_cmd_get_hw_resources;
>>>>>       ops->control_vblank_irq = 
>>>>> dpu_encoder_phys_cmd_control_vblank_irq;
>>>>>       ops->wait_for_commit_done = 
>>>>> dpu_encoder_phys_cmd_wait_for_commit_done;
>>>>>       ops->prepare_for_kickoff = 
>>>>> dpu_encoder_phys_cmd_prepare_for_kickoff;
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>>> index ddd9d89cd456..db6a9b896e42 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>>> @@ -465,13 +465,6 @@ static void 
>>>>> dpu_encoder_phys_vid_destroy(struct dpu_encoder_phys *phys_enc)
>>>>>       kfree(phys_enc);
>>>>>   }
>>>>> -static void dpu_encoder_phys_vid_get_hw_resources(
>>>>> -        struct dpu_encoder_phys *phys_enc,
>>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>>> -{
>>>>> -    hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
>>>>> -}
>>>>> -
>>>>>   static int dpu_encoder_phys_vid_wait_for_vblank(
>>>>>           struct dpu_encoder_phys *phys_enc)
>>>>>   {
>>>>> @@ -680,7 +673,6 @@ static void 
>>>>> dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>>>>>       ops->enable = dpu_encoder_phys_vid_enable;
>>>>>       ops->disable = dpu_encoder_phys_vid_disable;
>>>>>       ops->destroy = dpu_encoder_phys_vid_destroy;
>>>>> -    ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
>>>>>       ops->control_vblank_irq = 
>>>>> dpu_encoder_phys_vid_control_vblank_irq;
>>>>>       ops->wait_for_commit_done = 
>>>>> dpu_encoder_phys_vid_wait_for_commit_done;
>>>>>       ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>>> index 2d385b4b7f5e..3f518c809e33 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>>> @@ -144,7 +144,6 @@ struct dpu_global_state {
>>>>>       uint32_t pingpong_to_enc_id[PINGPONG_MAX - PINGPONG_0];
>>>>>       uint32_t mixer_to_enc_id[LM_MAX - LM_0];
>>>>>       uint32_t ctl_to_enc_id[CTL_MAX - CTL_0];
>>>>> -    uint32_t intf_to_enc_id[INTF_MAX - INTF_0];
>>>>>       uint32_t dspp_to_enc_id[DSPP_MAX - DSPP_0];
>>>>>   };
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>> index 63ed0d7df848..8df21a46308e 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>> @@ -28,7 +28,6 @@ static inline bool reserved_by_other(uint32_t 
>>>>> *res_map, int idx,
>>>>>    */
>>>>>   struct dpu_rm_requirements {
>>>>>       struct msm_display_topology topology;
>>>>> -    struct dpu_encoder_hw_resources hw_res;
>>>>>   };
>>>>>   int dpu_rm_destroy(struct dpu_rm *rm)
>>>>> @@ -448,54 +447,6 @@ static int _dpu_rm_reserve_ctls(
>>>>>       return 0;
>>>>>   }
>>>>> -static int _dpu_rm_reserve_intf(
>>>>> -        struct dpu_rm *rm,
>>>>> -        struct dpu_global_state *global_state,
>>>>> -        uint32_t enc_id,
>>>>> -        uint32_t id)
>>>>> -{
>>>>> -    int idx = id - INTF_0;
>>>>> -
>>>>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->intf_blks)) {
>>>>> -        DPU_ERROR("invalid intf id: %d", id);
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -
>>>>> -    if (!rm->intf_blks[idx]) {
>>>>> -        DPU_ERROR("couldn't find intf id %d\n", id);
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -
>>>>> -    if (reserved_by_other(global_state->intf_to_enc_id, idx, 
>>>>> enc_id)) {
>>>>> -        DPU_ERROR("intf id %d already reserved\n", id);
>>>>> -        return -ENAVAIL;
>>>>> -    }
>>>>> -
>>>>> -    global_state->intf_to_enc_id[idx] = enc_id;
>>>>> -    return 0;
>>>>> -}
>>>>> -
>>>>> -static int _dpu_rm_reserve_intf_related_hw(
>>>>> -        struct dpu_rm *rm,
>>>>> -        struct dpu_global_state *global_state,
>>>>> -        uint32_t enc_id,
>>>>> -        struct dpu_encoder_hw_resources *hw_res)
>>>>> -{
>>>>> -    int i, ret = 0;
>>>>> -    u32 id;
>>>>> -
>>>>> -    for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) {
>>>>> -        if (hw_res->intfs[i] == INTF_MODE_NONE)
>>>>> -            continue;
>>>>> -        id = i + INTF_0;
>>>>> -        ret = _dpu_rm_reserve_intf(rm, global_state, enc_id, id);
>>>>> -        if (ret)
>>>>> -            return ret;
>>>>> -    }
>>>>> -
>>>>> -    return ret;
>>>>> -}
>>>>> -
>>>>>   static int _dpu_rm_make_reservation(
>>>>>           struct dpu_rm *rm,
>>>>>           struct dpu_global_state *global_state,
>>>>> @@ -517,11 +468,6 @@ static int _dpu_rm_make_reservation(
>>>>>           return ret;
>>>>>       }
>>>>> -    ret = _dpu_rm_reserve_intf_related_hw(rm, global_state, 
>>>>> enc->base.id,
>>>>> -                &reqs->hw_res);
>>>>> -    if (ret)
>>>>> -        return ret;
>>>>> -
>>>>>       return ret;
>>>>>   }
>>>>> @@ -530,8 +476,6 @@ static int _dpu_rm_populate_requirements(
>>>>>           struct dpu_rm_requirements *reqs,
>>>>>           struct msm_display_topology req_topology)
>>>>>   {
>>>>> -    dpu_encoder_get_hw_resources(enc, &reqs->hw_res);
>>>>> -
>>>>>       reqs->topology = req_topology;
>>>>>       DRM_DEBUG_KMS("num_lm: %d num_enc: %d num_intf: %d\n",
>>>>> @@ -561,8 +505,6 @@ void dpu_rm_release(struct dpu_global_state 
>>>>> *global_state,
>>>>>           ARRAY_SIZE(global_state->mixer_to_enc_id), enc->base.id);
>>>>>       _dpu_rm_clear_mapping(global_state->ctl_to_enc_id,
>>>>>           ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>>>>> -    _dpu_rm_clear_mapping(global_state->intf_to_enc_id,
>>>>> -        ARRAY_SIZE(global_state->intf_to_enc_id), enc->base.id);
>>>>>   }
>>>>>   int dpu_rm_reserve(
>>>>> @@ -626,11 +568,6 @@ int dpu_rm_get_assigned_resources(struct 
>>>>> dpu_rm *rm,
>>>>>           hw_to_enc_id = global_state->ctl_to_enc_id;
>>>>>           max_blks = ARRAY_SIZE(rm->ctl_blks);
>>>>>           break;
>>>>> -    case DPU_HW_BLK_INTF:
>>>>> -        hw_blks = rm->intf_blks;
>>>>> -        hw_to_enc_id = global_state->intf_to_enc_id;
>>>>> -        max_blks = ARRAY_SIZE(rm->intf_blks);
>>>>> -        break;
>>>>>       case DPU_HW_BLK_DSPP:
>>>>>           hw_blks = rm->dspp_blks;
>>>>>           hw_to_enc_id = global_state->dspp_to_enc_id;
>>>>> @@ -656,3 +593,8 @@ int dpu_rm_get_assigned_resources(struct dpu_rm 
>>>>> *rm,
>>>>>       return num_blks;
>>>>>   }
>>>>> +
>>>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum 
>>>>> dpu_intf intf_idx)
>>>>> +{
>>>>> +    return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>> index 0f27759211b5..ee50f6651b6e 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>> @@ -84,5 +84,13 @@ void dpu_rm_release(struct dpu_global_state 
>>>>> *global_state,
>>>>>   int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>>>>>       struct dpu_global_state *global_state, uint32_t enc_id,
>>>>>       enum dpu_hw_blk_type type, struct dpu_hw_blk **blks, int 
>>>>> blks_size);
>>>>> +
>>>>> +/**
>>>>> + * dpu_rm_get_intf - Return a struct dpu_hw_intf instance given 
>>>>> it's index.
>>>>> + * @rm: DPU Resource Manager handle
>>>>> + * @intf_idx: INTF's index
>>>>> + */
>>>>> +struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum 
>>>>> dpu_intf intf_idx);
>>>>> +
>>>>>   #endif /* __DPU_RM_H__ */
>>>
>>>
> 
> 

[PATCH v5 4/6] drm/msm/dpu: stop embedding dpu_hw_blk into dpu_hw_intf

[Dmitry Baryshkov]

Now as dpu_hw_intf is not hanled by dpu_rm_get_assigned_resources, there
is no point in embedding the (empty) struct dpu_hw_blk into dpu_hw_intf
(and using typecasts between dpu_hw_blk and dpu_hw_intf). Drop it and
use dpu_hw_intf directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 11 -----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 17 +++--------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  9 ++++++---
 3 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index 3568be80dab5..230d122fa43b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -78,7 +78,6 @@ struct dpu_hw_intf_ops {
 };
 
 struct dpu_hw_intf {
-	struct dpu_hw_blk base;
 	struct dpu_hw_blk_reg_map hw;
 
 	/* intf */
@@ -90,16 +89,6 @@ struct dpu_hw_intf {
 	struct dpu_hw_intf_ops ops;
 };
 
-/**
- * to_dpu_hw_intf - convert base object dpu_hw_base to container
- * @hw: Pointer to base hardware block
- * return: Pointer to hardware block container
- */
-static inline struct dpu_hw_intf *to_dpu_hw_intf(struct dpu_hw_blk *hw)
-{
-	return container_of(hw, struct dpu_hw_intf, base);
-}
-
 /**
  * dpu_hw_intf_init(): Initializes the intf driver for the passed
  * interface idx.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 8df21a46308e..96554e962e38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -74,14 +74,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
 			dpu_hw_ctl_destroy(hw);
 		}
 	}
-	for (i = 0; i < ARRAY_SIZE(rm->intf_blks); i++) {
-		struct dpu_hw_intf *hw;
-
-		if (rm->intf_blks[i]) {
-			hw = to_dpu_hw_intf(rm->intf_blks[i]);
-			dpu_hw_intf_destroy(hw);
-		}
-	}
+	for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
+		dpu_hw_intf_destroy(rm->hw_intf[i]);
 
 	return 0;
 }
@@ -179,7 +173,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			DPU_ERROR("failed intf object creation: err %d\n", rc);
 			goto fail;
 		}
-		rm->intf_blks[intf->id - INTF_0] = &hw->base;
+		rm->hw_intf[intf->id - INTF_0] = hw;
 	}
 
 	for (i = 0; i < cat->ctl_count; i++) {
@@ -593,8 +587,3 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
 
 	return num_blks;
 }
-
-struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
-{
-	return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
-}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index ee50f6651b6e..9b13200a050a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -18,14 +18,14 @@ struct dpu_global_state;
  * @pingpong_blks: array of pingpong hardware resources
  * @mixer_blks: array of layer mixer hardware resources
  * @ctl_blks: array of ctl hardware resources
- * @intf_blks: array of intf hardware resources
+ * @hw_intf: array of intf hardware resources
  * @dspp_blks: array of dspp hardware resources
  */
 struct dpu_rm {
 	struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
 	struct dpu_hw_blk *mixer_blks[LM_MAX - LM_0];
 	struct dpu_hw_blk *ctl_blks[CTL_MAX - CTL_0];
-	struct dpu_hw_blk *intf_blks[INTF_MAX - INTF_0];
+	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];
 };
@@ -90,7 +90,10 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
  * @rm: DPU Resource Manager handle
  * @intf_idx: INTF's index
  */
-struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx);
+static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
+{
+	return rm->hw_intf[intf_idx - INTF_0];
+}
 
 #endif /* __DPU_RM_H__ */
 
-- 
2.34.1

Re: [PATCH v5 4/6] drm/msm/dpu: stop embedding dpu_hw_blk into dpu_hw_intf

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> Now as dpu_hw_intf is not hanled by dpu_rm_get_assigned_resources, there
> is no point in embedding the (empty) struct dpu_hw_blk into dpu_hw_intf
> (and using typecasts between dpu_hw_blk and dpu_hw_intf). Drop it and
> use dpu_hw_intf directly.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 11 -----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 17 +++--------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  9 ++++++---
>   3 files changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> index 3568be80dab5..230d122fa43b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
> @@ -78,7 +78,6 @@ struct dpu_hw_intf_ops {
>   };
>   
>   struct dpu_hw_intf {
> -	struct dpu_hw_blk base;
>   	struct dpu_hw_blk_reg_map hw;
>   
>   	/* intf */
> @@ -90,16 +89,6 @@ struct dpu_hw_intf {
>   	struct dpu_hw_intf_ops ops;
>   };
>   
> -/**
> - * to_dpu_hw_intf - convert base object dpu_hw_base to container
> - * @hw: Pointer to base hardware block
> - * return: Pointer to hardware block container
> - */
> -static inline struct dpu_hw_intf *to_dpu_hw_intf(struct dpu_hw_blk *hw)
> -{
> -	return container_of(hw, struct dpu_hw_intf, base);
> -}
> -
>   /**
>    * dpu_hw_intf_init(): Initializes the intf driver for the passed
>    * interface idx.
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 8df21a46308e..96554e962e38 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -74,14 +74,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>   			dpu_hw_ctl_destroy(hw);
>   		}
>   	}
> -	for (i = 0; i < ARRAY_SIZE(rm->intf_blks); i++) {
> -		struct dpu_hw_intf *hw;
> -
> -		if (rm->intf_blks[i]) {
> -			hw = to_dpu_hw_intf(rm->intf_blks[i]);
> -			dpu_hw_intf_destroy(hw);
> -		}
> -	}
> +	for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
> +		dpu_hw_intf_destroy(rm->hw_intf[i]);
>   
>   	return 0;
>   }
> @@ -179,7 +173,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			DPU_ERROR("failed intf object creation: err %d\n", rc);
>   			goto fail;
>   		}
> -		rm->intf_blks[intf->id - INTF_0] = &hw->base;
> +		rm->hw_intf[intf->id - INTF_0] = hw;
>   	}
>   
>   	for (i = 0; i < cat->ctl_count; i++) {
> @@ -593,8 +587,3 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>   
>   	return num_blks;
>   }
> -
> -struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
> -{
> -	return to_dpu_hw_intf(rm->intf_blks[intf_idx - INTF_0]);
> -}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index ee50f6651b6e..9b13200a050a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -18,14 +18,14 @@ struct dpu_global_state;
>    * @pingpong_blks: array of pingpong hardware resources
>    * @mixer_blks: array of layer mixer hardware resources
>    * @ctl_blks: array of ctl hardware resources
> - * @intf_blks: array of intf hardware resources
> + * @hw_intf: array of intf hardware resources
>    * @dspp_blks: array of dspp hardware resources
>    */
>   struct dpu_rm {
>   	struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
>   	struct dpu_hw_blk *mixer_blks[LM_MAX - LM_0];
>   	struct dpu_hw_blk *ctl_blks[CTL_MAX - CTL_0];
> -	struct dpu_hw_blk *intf_blks[INTF_MAX - INTF_0];
> +	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];
>   };
> @@ -90,7 +90,10 @@ int dpu_rm_get_assigned_resources(struct dpu_rm *rm,
>    * @rm: DPU Resource Manager handle
>    * @intf_idx: INTF's index
>    */
> -struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx);
> +static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_intf intf_idx)
> +{
> +	return rm->hw_intf[intf_idx - INTF_0];
> +}
>   
>   #endif /* __DPU_RM_H__ */
>   

[PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init

[Dmitry Baryshkov]

Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If
the value is NULL, then the function will return 0 instead of a proper
return code. Moreover none of dpu_hw_*_init() functions can return NULL.
So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 96554e962e38..7497538adae1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_lm_init(lm->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed lm object creation: err %d\n", rc);
 			goto fail;
@@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed merge_3d object creation: err %d\n",
 				rc);
@@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_pingpong_init(pp->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed pingpong object creation: err %d\n",
 				rc);
@@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_intf_init(intf->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed intf object creation: err %d\n", rc);
 			goto fail;
@@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_ctl_init(ctl->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed ctl object creation: err %d\n", rc);
 			goto fail;
@@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm,
 			continue;
 		}
 		hw = dpu_hw_dspp_init(dspp->id, mmio, cat);
-		if (IS_ERR_OR_NULL(hw)) {
+		if (IS_ERR(hw)) {
 			rc = PTR_ERR(hw);
 			DPU_ERROR("failed dspp object creation: err %d\n", rc);
 			goto fail;
-- 
2.34.1

Re: [PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If
> the value is NULL, then the function will return 0 instead of a proper
> return code. Moreover none of dpu_hw_*_init() functions can return NULL.
> So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR().
> 
Can you please give an example of a case where dpu_hw_*_init() can 
return NULL?

All dpu_hw_*_init() functions are only called if the corresponding
hw*_counts are valid. So I would like to understand this.

Now, if NULL is treated as a non-error case, should we atleast print
a message indicating so?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 96554e962e38..7497538adae1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_lm_init(lm->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed lm object creation: err %d\n", rc);
>   			goto fail;
> @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed merge_3d object creation: err %d\n",
>   				rc);
> @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_pingpong_init(pp->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed pingpong object creation: err %d\n",
>   				rc);
> @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_intf_init(intf->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed intf object creation: err %d\n", rc);
>   			goto fail;
> @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_ctl_init(ctl->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed ctl object creation: err %d\n", rc);
>   			goto fail;
> @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>   			continue;
>   		}
>   		hw = dpu_hw_dspp_init(dspp->id, mmio, cat);
> -		if (IS_ERR_OR_NULL(hw)) {
> +		if (IS_ERR(hw)) {
>   			rc = PTR_ERR(hw);
>   			DPU_ERROR("failed dspp object creation: err %d\n", rc);
>   			goto fail;

Re: [PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init

[Dmitry Baryshkov]

On 14/02/2022 22:15, Abhinav Kumar wrote:
> 
> 
> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>> Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If
>> the value is NULL, then the function will return 0 instead of a proper
>> return code. Moreover none of dpu_hw_*_init() functions can return NULL.
>> So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR().
>>
> Can you please give an example of a case where dpu_hw_*_init() can 
> return NULL?
> 
> All dpu_hw_*_init() functions are only called if the corresponding
> hw*_counts are valid. So I would like to understand this.
> 
> Now, if NULL is treated as a non-error case, should we atleast print
> a message indicating so?

- No dpu_hw_*init() can return NULL

- If at some point any of these functions returns NULL, it will cause a 
premature dpu_rm_init() termination with the success (=0) status, 
leaving parts of RM uninitialized.

Thus I'm replacing IS_ERR_OR_NULL with IS_ERR()

> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 96554e962e38..7497538adae1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_lm_init(lm->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed lm object creation: err %d\n", rc);
>>               goto fail;
>> @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed merge_3d object creation: err %d\n",
>>                   rc);
>> @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_pingpong_init(pp->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed pingpong object creation: err %d\n",
>>                   rc);
>> @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_intf_init(intf->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed intf object creation: err %d\n", rc);
>>               goto fail;
>> @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_ctl_init(ctl->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed ctl object creation: err %d\n", rc);
>>               goto fail;
>> @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>               continue;
>>           }
>>           hw = dpu_hw_dspp_init(dspp->id, mmio, cat);
>> -        if (IS_ERR_OR_NULL(hw)) {
>> +        if (IS_ERR(hw)) {
>>               rc = PTR_ERR(hw);
>>               DPU_ERROR("failed dspp object creation: err %d\n", rc);
>>               goto fail;


-- 
With best wishes
Dmitry

Re: [PATCH v5 5/6] drm/msm/dpu: fix error handling in dpu_rm_init

[Abhinav Kumar]

On 2/14/2022 12:43 PM, Dmitry Baryshkov wrote:
> On 14/02/2022 22:15, Abhinav Kumar wrote:
>>
>>
>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>> Using IS_ERR_OR_NULL() together with PTR_ERR() is a typical mistake. If
>>> the value is NULL, then the function will return 0 instead of a proper
>>> return code. Moreover none of dpu_hw_*_init() functions can return NULL.
>>> So, replace all dpu_rm_init()'s IS_ERR_OR_NULL() calls with IS_ERR().
>>>
>> Can you please give an example of a case where dpu_hw_*_init() can 
>> return NULL?
>>
>> All dpu_hw_*_init() functions are only called if the corresponding
>> hw*_counts are valid. So I would like to understand this.
>>
>> Now, if NULL is treated as a non-error case, should we atleast print
>> a message indicating so?
> 
> - No dpu_hw_*init() can return NULL
> 
> - If at some point any of these functions returns NULL, it will cause a 
> premature dpu_rm_init() termination with the success (=0) status, 
> leaving parts of RM uninitialized.
> 
> Thus I'm replacing IS_ERR_OR_NULL with IS_ERR()

Ah okay, got it.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> index 96554e962e38..7497538adae1 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -109,7 +109,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_lm_init(lm->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed lm object creation: err %d\n", rc);
>>>               goto fail;
>>> @@ -126,7 +126,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_merge_3d_init(merge_3d->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed merge_3d object creation: err %d\n",
>>>                   rc);
>>> @@ -144,7 +144,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_pingpong_init(pp->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed pingpong object creation: err %d\n",
>>>                   rc);
>>> @@ -168,7 +168,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_intf_init(intf->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed intf object creation: err %d\n", rc);
>>>               goto fail;
>>> @@ -185,7 +185,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_ctl_init(ctl->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed ctl object creation: err %d\n", rc);
>>>               goto fail;
>>> @@ -202,7 +202,7 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>               continue;
>>>           }
>>>           hw = dpu_hw_dspp_init(dspp->id, mmio, cat);
>>> -        if (IS_ERR_OR_NULL(hw)) {
>>> +        if (IS_ERR(hw)) {
>>>               rc = PTR_ERR(hw);
>>>               DPU_ERROR("failed dspp object creation: err %d\n", rc);
>>>               goto fail;
> 
> 

[PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Dmitry Baryshkov]

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      | 19 ++++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
 6 files changed, 40 insertions(+), 47 deletions(-)

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 7497538adae1..6d49666c4e77 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
 	}
 	for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
 		dpu_hw_intf_destroy(rm->hw_intf[i]);
+	for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
+		dpu_hw_vbif_destroy(rm->hw_vbif[i]);
 
 	return 0;
 }
@@ -210,6 +212,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(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(
-- 
2.34.1

Re: [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Abhinav Kumar]

On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
> 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>

I have a slightly different idea about this. Let me know what you think.

VBIF is a bus interface for the dpu to fetch from. I am not sure if 
pulling it in the RM is right because its not a dedicated HW block like
the others in the RM.

But, I agree with your problem statement of hand-coded loops.

So instead, why dont you just have a helper in the dpu_vbif.c to get
you the vbif hw for the passed index like, maybe something like this?

--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -11,6 +11,19 @@
  #include "dpu_hw_vbif.h"
  #include "dpu_trace.h"

+static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 vbif_idx)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
+               if (dpu_kms->hw_vbif[i] &&
+                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
+                       vbif = dpu_kms->hw_vbif[i];
+       }
+
+       return vbif;
+}
+
  /**
   * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
   * @vbif:      Pointer to hardware vbif driver
@@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(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];
-       }
+       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);

         if (!vbif || !mdp) {
                 DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
@@ -216,13 +225,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_vbif_get_hw(params->vbif_idx);



> ---
>   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      | 19 ++++++++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>   6 files changed, 40 insertions(+), 47 deletions(-)
> 
> 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 7497538adae1..6d49666c4e77 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>   	}
>   	for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>   		dpu_hw_intf_destroy(rm->hw_intf[i]);
> +	for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
> +		dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>   
>   	return 0;
>   }
> @@ -210,6 +212,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(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(

Re: [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Dmitry Baryshkov]

On 14/02/2022 22:53, Abhinav Kumar wrote:
> 
> 
> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>> 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>
> 
> I have a slightly different idea about this. Let me know what you think.
>  
> VBIF is a bus interface for the dpu to fetch from. I am not sure if 
> pulling it in the RM is right because its not a dedicated HW block like
> the others in the RM.

It's not a hardware block, but a it's still a hardware resource 
(hardware instance). It is described in the hw catalog. Thus I suggested 
moving it to dpu_rm.

As you have seen, from my previous iterations of this patchset, I tried 
  to move things out of dpu_rm. After some hacking, I saw that having 
alloc/free loops in several places seems like a worse idea. So I moved 
dpu_hw_intf back to dpu_rm and then moved dpu_hw_vbif to dpu_rm too.

> 
> But, I agree with your problem statement of hand-coded loops.
> 
> So instead, why dont you just have a helper in the dpu_vbif.c to get
> you the vbif hw for the passed index like, maybe something like this?
> 
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> @@ -11,6 +11,19 @@
>   #include "dpu_hw_vbif.h"
>   #include "dpu_trace.h"
> 
> +static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 vbif_idx)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
> +               if (dpu_kms->hw_vbif[i] &&
> +                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
> +                       vbif = dpu_kms->hw_vbif[i];
> +       }
> +
> +       return vbif;
> +}
> +

You see, this code still bears an idea of looping through hw_vbif 
entries looking for the correct one (we can directly access hw_vbif[idx 
- VBIF_0] instead).

And also the alloc/destroy loops are very similar to rm ones, but are 
working against the array in dpu_kms.

One of the previous iterations had neearly the same idea as yours patch 
proposes, but I later abandoned this idea.

I'm trying to place common code nearby, so that there is a less chance 
of an error.

>   /**
>    * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
>    * @vbif:      Pointer to hardware vbif driver
> @@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(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];
> -       }
> +       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);
> 
>          if (!vbif || !mdp) {
>                  DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
> @@ -216,13 +225,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_vbif_get_hw(params->vbif_idx);
> 
> 
> 
>> ---
>>   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      | 19 ++++++++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>>   6 files changed, 40 insertions(+), 47 deletions(-)
>>
>> 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 7497538adae1..6d49666c4e77 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>       }
>>       for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>>           dpu_hw_intf_destroy(rm->hw_intf[i]);
>> +    for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
>> +        dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>>       return 0;
>>   }
>> @@ -210,6 +212,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(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(


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Abhinav Kumar]

On 2/14/2022 12:56 PM, Dmitry Baryshkov wrote:
> On 14/02/2022 22:53, Abhinav Kumar wrote:
>>
>>
>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>> 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>
>>
>> I have a slightly different idea about this. Let me know what you think.
>>
>> VBIF is a bus interface for the dpu to fetch from. I am not sure if 
>> pulling it in the RM is right because its not a dedicated HW block like
>> the others in the RM.
> 
> It's not a hardware block, but a it's still a hardware resource 
> (hardware instance). It is described in the hw catalog. Thus I suggested 
> moving it to dpu_rm.
> 
> As you have seen, from my previous iterations of this patchset, I tried 
>   to move things out of dpu_rm. After some hacking, I saw that having 
> alloc/free loops in several places seems like a worse idea. So I moved 
> dpu_hw_intf back to dpu_rm and then moved dpu_hw_vbif to dpu_rm too.
>
Actually for some reason, I only see two revs here:

https://patchwork.freedesktop.org/series/99175/#rev1

Hence, I didnt check the previous patchsets from patchwork to see the 
evolution.

>>
>> But, I agree with your problem statement of hand-coded loops.
>>
>> So instead, why dont you just have a helper in the dpu_vbif.c to get
>> you the vbif hw for the passed index like, maybe something like this?
>>
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>> @@ -11,6 +11,19 @@
>>   #include "dpu_hw_vbif.h"
>>   #include "dpu_trace.h"
>>
>> +static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 
>> vbif_idx)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>> +               if (dpu_kms->hw_vbif[i] &&
>> +                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
>> +                       vbif = dpu_kms->hw_vbif[i];
>> +       }
>> +
>> +       return vbif;
>> +}
>> +
> 
> You see, this code still bears an idea of looping through hw_vbif 
> entries looking for the correct one (we can directly access hw_vbif[idx 
> - VBIF_0] instead).
> 
> And also the alloc/destroy loops are very similar to rm ones, but are 
> working against the array in dpu_kms.
> 
> One of the previous iterations had neearly the same idea as yours patch 
> proposes, but I later abandoned this idea.
> 
> I'm trying to place common code nearby, so that there is a less chance 
> of an error.

This loop is being used to find the vbif matching the index only in two 
places today dpu_vbif_set_ot_limit and dpu_vbif_set_qos_remap.

Today and from whatever I see even in downstream (which has support for 
more newer chipsets), there is only one VBIF
instance in the catalog and always with the index 0.

So to be honest, even that loop is an overkill today because the index 
seems to be always 0 and there is only one instance so the loop seems to 
break out at the first occurrence.

Thats why I was wondering whether moving VBIF to RM is an overkill for 
this and just the simple cleanup i was suggesting was enough as that 
loop itself is an overkill today for one instance of vbif.

> 
>>   /**
>>    * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
>>    * @vbif:      Pointer to hardware vbif driver
>> @@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(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];
>> -       }
>> +       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);
>>
>>          if (!vbif || !mdp) {
>>                  DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
>> @@ -216,13 +225,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_vbif_get_hw(params->vbif_idx);
>>
>>
>>
>>> ---
>>>   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      | 19 ++++++++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>>>   6 files changed, 40 insertions(+), 47 deletions(-)
>>>
>>> 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 7497538adae1..6d49666c4e77 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>>       }
>>>       for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>>>           dpu_hw_intf_destroy(rm->hw_intf[i]);
>>> +    for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
>>> +        dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>>>       return 0;
>>>   }
>>> @@ -210,6 +212,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(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(
> 
> 

Re: [Freedreno] [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Dmitry Baryshkov]

On 15/02/2022 01:04, Abhinav Kumar wrote:
> 
> 
> On 2/14/2022 12:56 PM, Dmitry Baryshkov wrote:
>> On 14/02/2022 22:53, Abhinav Kumar wrote:
>>>
>>>
>>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>>> 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>
>>>
>>> I have a slightly different idea about this. Let me know what you think.
>>>
>>> VBIF is a bus interface for the dpu to fetch from. I am not sure if 
>>> pulling it in the RM is right because its not a dedicated HW block like
>>> the others in the RM.
>>
>> It's not a hardware block, but a it's still a hardware resource 
>> (hardware instance). It is described in the hw catalog. Thus I 
>> suggested moving it to dpu_rm.
>>
>> As you have seen, from my previous iterations of this patchset, I 
>> tried   to move things out of dpu_rm. After some hacking, I saw that 
>> having alloc/free loops in several places seems like a worse idea. So 
>> I moved dpu_hw_intf back to dpu_rm and then moved dpu_hw_vbif to 
>> dpu_rm too.
>>
> Actually for some reason, I only see two revs here:
> 
> https://patchwork.freedesktop.org/series/99175/#rev1

Yeah, I'm not sure why patchwork created new series rather than new 
revision.

> 
> Hence, I didnt check the previous patchsets from patchwork to see the 
> evolution.
> 

https://patchwork.freedesktop.org/patch/464353/


>>>
>>> But, I agree with your problem statement of hand-coded loops.
>>>
>>> So instead, why dont you just have a helper in the dpu_vbif.c to get
>>> you the vbif hw for the passed index like, maybe something like this?
>>>
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> @@ -11,6 +11,19 @@
>>>   #include "dpu_hw_vbif.h"
>>>   #include "dpu_trace.h"
>>>
>>> +static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 
>>> vbif_idx)
>>> +{
>>> +       int i;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>> +               if (dpu_kms->hw_vbif[i] &&
>>> +                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
>>> +                       vbif = dpu_kms->hw_vbif[i];
>>> +       }
>>> +
>>> +       return vbif;
>>> +}
>>> +
>>
>> You see, this code still bears an idea of looping through hw_vbif 
>> entries looking for the correct one (we can directly access 
>> hw_vbif[idx - VBIF_0] instead).
>>
>> And also the alloc/destroy loops are very similar to rm ones, but are 
>> working against the array in dpu_kms.
>>
>> One of the previous iterations had neearly the same idea as yours 
>> patch proposes, but I later abandoned this idea.
>>
>> I'm trying to place common code nearby, so that there is a less chance 
>> of an error.
> 
> This loop is being used to find the vbif matching the index only in two 
> places today dpu_vbif_set_ot_limit and dpu_vbif_set_qos_remap.

Here I was talking about the loops to allocate and destroy VBIFs.

> 
> Today and from whatever I see even in downstream (which has support for 
> more newer chipsets), there is only one VBIF
> instance in the catalog and always with the index 0.

When should we use VBIF_NRT? Judging from the _NRT suffix I thought that 
it's used for WB2, but it doesn't seem to be true.

> So to be honest, even that loop is an overkill today because the index 
> seems to be always 0 and there is only one instance so the loop seems to 
> break out at the first occurrence.

It's always better to remove a loop rather than to break from it.

> 
> Thats why I was wondering whether moving VBIF to RM is an overkill for 
> this and just the simple cleanup i was suggesting was enough as that 
> loop itself is an overkill today for one instance of vbif.

Then we might as well drop an array and just leave a single vbif_0.

Just checked downstream device trees. 8996 declares two VBIFs. It looks 
like it's the only user of VBIF_1. Any comments? What would we loose for 
(possible) 8996 support in DPU if we drop VBIF_1 / VBIF_NRT support?

> 
>>
>>>   /**
>>>    * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
>>>    * @vbif:      Pointer to hardware vbif driver
>>> @@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(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];
>>> -       }
>>> +       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);
>>>
>>>          if (!vbif || !mdp) {
>>>                  DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
>>> @@ -216,13 +225,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_vbif_get_hw(params->vbif_idx);
>>>
>>>
>>>
>>>> ---
>>>>   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      | 19 ++++++++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>>>>   6 files changed, 40 insertions(+), 47 deletions(-)
>>>>
>>>> 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 7497538adae1..6d49666c4e77 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>>>       }
>>>>       for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>>>>           dpu_hw_intf_destroy(rm->hw_intf[i]);
>>>> +    for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
>>>> +        dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>>>>       return 0;
>>>>   }
>>>> @@ -210,6 +212,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(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(
>>
>>


-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

[Abhinav Kumar]

On 2/14/2022 2:39 PM, Dmitry Baryshkov wrote:
> On 15/02/2022 01:04, Abhinav Kumar wrote:
>>
>>
>> On 2/14/2022 12:56 PM, Dmitry Baryshkov wrote:
>>> On 14/02/2022 22:53, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>>>> 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>
>>>>
>>>> I have a slightly different idea about this. Let me know what you 
>>>> think.
>>>>
>>>> VBIF is a bus interface for the dpu to fetch from. I am not sure if 
>>>> pulling it in the RM is right because its not a dedicated HW block like
>>>> the others in the RM.
>>>
>>> It's not a hardware block, but a it's still a hardware resource 
>>> (hardware instance). It is described in the hw catalog. Thus I 
>>> suggested moving it to dpu_rm.
>>>
>>> As you have seen, from my previous iterations of this patchset, I 
>>> tried   to move things out of dpu_rm. After some hacking, I saw that 
>>> having alloc/free loops in several places seems like a worse idea. So 
>>> I moved dpu_hw_intf back to dpu_rm and then moved dpu_hw_vbif to 
>>> dpu_rm too.
>>>
>> Actually for some reason, I only see two revs here:
>>
>> https://patchwork.freedesktop.org/series/99175/#rev1
> 
> Yeah, I'm not sure why patchwork created new series rather than new 
> revision.
> 
>>
>> Hence, I didnt check the previous patchsets from patchwork to see the 
>> evolution.
>>
> 
> https://patchwork.freedesktop.org/patch/464353/
> 
> 
>>>>
>>>> But, I agree with your problem statement of hand-coded loops.
>>>>
>>>> So instead, why dont you just have a helper in the dpu_vbif.c to get
>>>> you the vbif hw for the passed index like, maybe something like this?
>>>>
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>>> @@ -11,6 +11,19 @@
>>>>   #include "dpu_hw_vbif.h"
>>>>   #include "dpu_trace.h"
>>>>
>>>> +static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 
>>>> vbif_idx)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>>> +               if (dpu_kms->hw_vbif[i] &&
>>>> +                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
>>>> +                       vbif = dpu_kms->hw_vbif[i];
>>>> +       }
>>>> +
>>>> +       return vbif;
>>>> +}
>>>> +
>>>
>>> You see, this code still bears an idea of looping through hw_vbif 
>>> entries looking for the correct one (we can directly access 
>>> hw_vbif[idx - VBIF_0] instead).
>>>
>>> And also the alloc/destroy loops are very similar to rm ones, but are 
>>> working against the array in dpu_kms.
>>>
>>> One of the previous iterations had neearly the same idea as yours 
>>> patch proposes, but I later abandoned this idea.
>>>
>>> I'm trying to place common code nearby, so that there is a less 
>>> chance of an error.
>>
>> This loop is being used to find the vbif matching the index only in 
>> two places today dpu_vbif_set_ot_limit and dpu_vbif_set_qos_remap.
> 
> Here I was talking about the loops to allocate and destroy VBIFs.
> 
>>
>> Today and from whatever I see even in downstream (which has support 
>> for more newer chipsets), there is only one VBIF
>> instance in the catalog and always with the index 0.
> 
> When should we use VBIF_NRT? Judging from the _NRT suffix I thought that 
> it's used for WB2, but it doesn't seem to be true.
> 
>> So to be honest, even that loop is an overkill today because the index 
>> seems to be always 0 and there is only one instance so the loop seems 
>> to break out at the first occurrence.
> 
> It's always better to remove a loop rather than to break from it.
> 
>>
>> Thats why I was wondering whether moving VBIF to RM is an overkill for 
>> this and just the simple cleanup i was suggesting was enough as that 
>> loop itself is an overkill today for one instance of vbif.
> 
> Then we might as well drop an array and just leave a single vbif_0.
> 
> Just checked downstream device trees. 8996 declares two VBIFs. It looks 
> like it's the only user of VBIF_1. Any comments? What would we loose for 
> (possible) 8996 support in DPU if we drop VBIF_1 / VBIF_NRT support?

Yes it seems like 8996 writeback is the only client to use VBIF_NRT. I 
am unable to locate the old codebase to check its usage.

Here since we are talking about eventually using DPU for 8996, even if 
we consider VBIF_NRT into the mix, its only 2.

So I feel we can get rid of the loop and instead just come up with a 
simple if else?

So something like:

if (VBIF_RT)
	vbif = dpu_kms[VBIF_RT].vbif;
else
	vbif = dpu_kms[VBIF_NRT].vbif

I am not foreseeing more vbif clients so far.

We can revisit this in the future if it goes beyond 2.

> 
>>
>>>
>>>>   /**
>>>>    * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
>>>>    * @vbif:      Pointer to hardware vbif driver
>>>> @@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(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];
>>>> -       }
>>>> +       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);
>>>>
>>>>          if (!vbif || !mdp) {
>>>>                  DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
>>>> @@ -216,13 +225,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_vbif_get_hw(params->vbif_idx);
>>>>
>>>>
>>>>
>>>>> ---
>>>>>   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      | 19 ++++++++++++++
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>>>>>   6 files changed, 40 insertions(+), 47 deletions(-)
>>>>>
>>>>> 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 7497538adae1..6d49666c4e77 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>>> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>>>>       }
>>>>>       for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>>>>>           dpu_hw_intf_destroy(rm->hw_intf[i]);
>>>>> +    for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
>>>>> +        dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>>>>>       return 0;
>>>>>   }
>>>>> @@ -210,6 +212,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(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(
>>>
>>>
> 
>