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; };

  /**

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;

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__ */

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__ */

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__ */

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__ */

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;
  

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;

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(

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(

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(