[07/25] drm/msm/dpu: reserve using crtc state

Submitted by Jeykumar Sankaran on Oct. 9, 2018, 4:27 a.m.

Details

Message ID 1539059262-8326-8-git-send-email-jsanka@codeaurora.org
State New
Headers show
Series "reserve RM resources in CRTC state" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jeykumar Sankaran Oct. 9, 2018, 4:27 a.m.
DPU maintained reservation lists to cache assigned
HW blocks for the display and a retrieval mechanism for
the individual DRM components to query their respective
HW blocks.

This patch uses the sub-classed CRTC state to store
and track HW blocks assigned for different components
of the display pipeline. It helps the driver:
- to get rid of unwanted store and retrieval RM API's
- to preserve HW resources assigned in atomic_check
  through atomic swap/duplicate.

Separate patch is submitted to remove resource
reservation in atomic_commit path.

Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 65 +++-------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           | 14 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 28 +++-------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 20 ++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 58 ++++++++++++-------
 5 files changed, 72 insertions(+), 113 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4960641..0625f56 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -421,69 +421,20 @@  void dpu_crtc_complete_commit(struct drm_crtc *crtc,
 	trace_dpu_crtc_complete_commit(DRMID(crtc));
 }
 
-static void _dpu_crtc_setup_mixer_for_encoder(
-		struct drm_crtc *crtc,
-		struct drm_encoder *enc)
+static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
 {
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
-	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
-	struct dpu_rm *rm = &dpu_kms->rm;
 	struct dpu_crtc_mixer *mixer;
-	struct dpu_hw_ctl *last_valid_ctl = NULL;
-	int i;
-	struct dpu_rm_hw_iter lm_iter, ctl_iter;
-
-	dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM);
-	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
+	int i, ctl_index;
 
 	/* Set up all the mixers and ctls reserved by this encoder */
-	for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) {
+	for (i = 0; i < cstate->num_mixers; i++) {
 		mixer = &cstate->mixers[i];
 
-		if (!dpu_rm_get_hw(rm, &lm_iter))
-			break;
-		mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw;
-
 		/* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */
-		if (!dpu_rm_get_hw(rm, &ctl_iter)) {
-			DPU_DEBUG("no ctl assigned to lm %d, using previous\n",
-					mixer->hw_lm->idx - LM_0);
-			mixer->lm_ctl = last_valid_ctl;
-		} else {
-			mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
-			last_valid_ctl = mixer->lm_ctl;
-		}
-
-		/* Shouldn't happen, mixers are always >= ctls */
-		if (!mixer->lm_ctl) {
-			DPU_ERROR("no valid ctls found for lm %d\n",
-					mixer->hw_lm->idx - LM_0);
-			return;
-		}
-
-		cstate->num_mixers++;
-		DPU_DEBUG("setup mixer %d: lm %d\n",
-				i, mixer->hw_lm->idx - LM_0);
-		DPU_DEBUG("setup mixer %d: ctl %d\n",
-				i, mixer->lm_ctl->idx - CTL_0);
-	}
-}
-
-static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
-{
-	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
-	struct drm_encoder *enc;
-
-	mutex_lock(&dpu_crtc->crtc_lock);
-	/* Check for mixers on all encoders attached to this crtc */
-	list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) {
-		if (enc->crtc != crtc)
-			continue;
-
-		_dpu_crtc_setup_mixer_for_encoder(crtc, enc);
+		ctl_index = min(i, cstate->num_ctls - 1);
+		mixer->lm_ctl = cstate->hw_ctls[ctl_index];
 	}
-
-	mutex_unlock(&dpu_crtc->crtc_lock);
 }
 
 static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
@@ -536,10 +487,8 @@  static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
 	dev = crtc->dev;
 	smmu_state = &dpu_crtc->smmu_state;
 
-	if (!cstate->num_mixers) {
-		_dpu_crtc_setup_mixers(crtc);
-		_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
-	}
+	_dpu_crtc_setup_mixers(crtc);
+	_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
 
 	if (dpu_crtc->event) {
 		WARN_ON(dpu_crtc->event);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 75fdd3c..17aaad7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -84,12 +84,14 @@  struct dpu_crtc_smmu_state_data {
  * struct dpu_crtc_mixer: stores the map for each virtual pipeline in the CRTC
  * @hw_lm:	LM HW Driver context
  * @lm_ctl:	CTL Path HW driver context
+ * @hw_pp:	Pingpong HW driver context
  * @mixer_op_mode:	mixer blending operation mode
  * @flush_mask:	mixer flush mask for ctl, mixer and pipe
  */
 struct dpu_crtc_mixer {
 	struct dpu_hw_mixer *hw_lm;
 	struct dpu_hw_ctl *lm_ctl;
+	struct dpu_hw_pingpong *hw_pp;
 	u32 mixer_op_mode;
 	u32 flush_mask;
 };
@@ -230,6 +232,18 @@  struct dpu_crtc_state {
 
 	u32 num_ctls;
 	struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
+
+	/**
+	 * as drm encoders doesn't have dedicates state objects
+	 * and drm connectors are not owned by DPU, maintain
+	 * HW interface and other interface related blocks
+	 * in crtc state
+	 *
+	 * TODO: No support for clone mode yet where a crtc
+	 * can be attached with more than one encoder/connector.
+	 */
+	u32 num_intfs;
+	struct dpu_hw_intf *hw_intfs[CRTC_DUAL_MIXERS];
 };
 
 #define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d12f896..17dbbc3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1001,9 +1001,8 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 	struct dpu_kms *dpu_kms;
 	struct list_head *connector_list;
 	struct drm_connector *conn = NULL, *conn_iter;
-	struct dpu_rm_hw_iter pp_iter, ctl_iter;
+	struct dpu_crtc_state *dpu_cstate;
 	struct msm_display_topology topology;
-	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
 	int i = 0, ret;
 
 	if (!drm_enc) {
@@ -1043,27 +1042,14 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 		return;
 	}
 
-	dpu_rm_init_hw_iter(&pp_iter, drm_enc->base.id, DPU_HW_BLK_PINGPONG);
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
-		dpu_enc->hw_pp[i] = NULL;
-		if (!dpu_rm_get_hw(&dpu_kms->rm, &pp_iter))
-			break;
-		dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw;
-	}
-
-	dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
-	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
-		if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter))
-			break;
-		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
-	}
+	dpu_cstate = to_dpu_crtc_state(drm_enc->crtc->state);
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 		int ctl_index;
 
 		if (phys) {
-			if (!dpu_enc->hw_pp[i]) {
+			if (!dpu_cstate->mixers[i].hw_pp) {
 				DPU_ERROR_ENC(dpu_enc, "no pp block assigned"
 					     "at idx: %d\n", i);
 				return;
@@ -1071,14 +1057,16 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 			ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1 : 0;
 
-			if (!hw_ctl[ctl_index]) {
+			if (!dpu_cstate->hw_ctls[ctl_index]) {
 				DPU_ERROR_ENC(dpu_enc, "no ctl block assigned"
 					     "at idx: %d\n", ctl_index);
 				return;
 			}
 
-			phys->hw_pp = dpu_enc->hw_pp[i];
-			phys->hw_ctl = hw_ctl[ctl_index];
+			dpu_enc->hw_pp[i] = dpu_cstate->mixers[i].hw_pp;
+
+			phys->hw_pp = dpu_cstate->mixers[i].hw_pp;
+			phys->hw_ctl = dpu_cstate->hw_ctls[ctl_index];
 
 			phys->connector = conn->state->connector;
 			if (phys->ops.mode_set)
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 84de385..4563f8d 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
@@ -461,28 +461,20 @@  static int dpu_encoder_phys_vid_control_vblank_irq(
 
 static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
 {
-	struct msm_drm_private *priv;
 	struct dpu_encoder_phys_vid *vid_enc;
-	struct dpu_rm_hw_iter iter;
 	struct dpu_hw_ctl *ctl;
+	struct dpu_crtc_state *dpu_cstate;
+	int i;
 	u32 flush_mask = 0;
 
-	if (!phys_enc || !phys_enc->parent || !phys_enc->parent->dev ||
-			!phys_enc->parent->dev->dev_private) {
-		DPU_ERROR("invalid encoder/device\n");
-		return;
-	}
-	priv = phys_enc->parent->dev->dev_private;
+	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
 
 	vid_enc = to_dpu_encoder_phys_vid(phys_enc);
 	ctl = phys_enc->hw_ctl;
 
-	dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_INTF);
-	while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) {
-		struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf *)iter.hw;
-
-		if (hw_intf->idx == phys_enc->intf_idx) {
-			vid_enc->hw_intf = hw_intf;
+	for (i = 0; i < dpu_cstate->num_intfs; i++) {
+		if (dpu_cstate->hw_intfs[i]->idx == phys_enc->intf_idx) {
+			vid_enc->hw_intf = dpu_cstate->hw_intfs[i];
 			break;
 		}
 	}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 377def7..5703b11 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -388,6 +388,7 @@  static bool _dpu_rm_check_lm_and_get_connected_blks(
 }
 
 static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
+			       struct dpu_crtc_state *dpu_cstate,
 			       struct dpu_rm_requirements *reqs)
 
 {
@@ -449,16 +450,22 @@  static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
 		lm[i]->enc_id = enc_id;
 		pp[i]->enc_id = enc_id;
 
+		dpu_cstate->mixers[i].hw_lm = to_dpu_hw_mixer(lm[i]->hw);
+		dpu_cstate->mixers[i].hw_pp = to_dpu_hw_pingpong(pp[i]->hw);
+
 		trace_dpu_rm_reserve_lms(lm[i]->id, DPU_HW_BLK_LM, enc_id,
 					 pp[i]->id);
 	}
 
+	dpu_cstate->num_mixers = lm_count;
+
 	return rc;
 }
 
 static int _dpu_rm_reserve_ctls(
 		struct dpu_rm *rm,
 		uint32_t enc_id,
+		struct dpu_crtc_state *dpu_cstate,
 		const struct msm_display_topology *top)
 {
 	struct dpu_rm_hw_blk *ctls[MAX_BLOCKS];
@@ -501,21 +508,24 @@  static int _dpu_rm_reserve_ctls(
 
 	for (i = 0; i < ARRAY_SIZE(ctls) && i < num_ctls; i++) {
 		ctls[i]->enc_id = enc_id;
+		dpu_cstate->hw_ctls[i] = to_dpu_hw_ctl(ctls[i]->hw);
+
 		trace_dpu_rm_reserve_ctls(ctls[i]->id, DPU_HW_BLK_CTL,
 					  enc_id);
 	}
 
+	dpu_cstate->num_ctls = num_ctls;
+
 	return 0;
 }
 
-static int _dpu_rm_reserve_intf(
+static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf(
 		struct dpu_rm *rm,
 		uint32_t enc_id,
 		uint32_t id,
 		enum dpu_hw_blk_type type)
 {
 	struct dpu_rm_hw_iter iter;
-	int ret = 0;
 
 	/* Find the block entry in the rm, and note the reservation */
 	dpu_rm_init_hw_iter(&iter, 0, type);
@@ -525,7 +535,7 @@  static int _dpu_rm_reserve_intf(
 
 		if (RESERVED_BY_OTHER(iter.blk, enc_id)) {
 			DPU_ERROR("type %d id %d already reserved\n", type, id);
-			return -ENAVAIL;
+			return NULL;
 		}
 
 		iter.blk->enc_id = enc_id;
@@ -535,56 +545,63 @@  static int _dpu_rm_reserve_intf(
 	}
 
 	/* Shouldn't happen since intfs are fixed at probe */
-	if (!iter.hw) {
+	if (!iter.blk) {
 		DPU_ERROR("couldn't find type %d id %d\n", type, id);
-		return -EINVAL;
+		return NULL;
 	}
 
-	return ret;
+	return iter.blk;
 }
 
 static int _dpu_rm_reserve_intf_related_hw(
 		struct dpu_rm *rm,
 		uint32_t enc_id,
+		struct dpu_crtc_state *dpu_cstate,
 		struct dpu_encoder_hw_resources *hw_res)
 {
-	int i, ret = 0;
-	u32 id;
+	struct dpu_rm_hw_blk *blk;
+	int i, num_intfs = 0;
 
 	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, enc_id, id,
+
+		blk = _dpu_rm_reserve_intf(rm, enc_id, i + INTF_0,
 				DPU_HW_BLK_INTF);
-		if (ret)
-			return ret;
+		if (!blk)
+			return -ENAVAIL;
+
+		dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw);
 	}
 
-	return ret;
+	dpu_cstate->num_intfs = num_intfs;
+
+	return 0;
 }
 
 static int _dpu_rm_make_reservation(
 		struct dpu_rm *rm,
 		struct drm_encoder *enc,
-		struct drm_crtc_state *crtc_state,
+		struct dpu_crtc_state *dpu_cstate,
 		struct dpu_rm_requirements *reqs)
 {
 	int ret;
 
-	ret = _dpu_rm_reserve_lms(rm, enc->base.id, reqs);
+	ret = _dpu_rm_reserve_lms(rm, enc->base.id, dpu_cstate, reqs);
 	if (ret) {
 		DPU_ERROR("unable to find appropriate mixers\n");
 		return ret;
 	}
 
-	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, &reqs->topology);
+	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, dpu_cstate,
+				   &reqs->topology);
 	if (ret) {
 		DPU_ERROR("unable to find appropriate CTL\n");
 		return ret;
 	}
 
-	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id, &reqs->hw_res);
+	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id, dpu_cstate,
+					      &reqs->hw_res);
 	if (ret)
 		return ret;
 
@@ -594,7 +611,6 @@  static int _dpu_rm_make_reservation(
 static int _dpu_rm_populate_requirements(
 		struct dpu_rm *rm,
 		struct drm_encoder *enc,
-		struct drm_crtc_state *crtc_state,
 		struct dpu_rm_requirements *reqs,
 		struct msm_display_topology req_topology)
 {
@@ -642,6 +658,7 @@  int dpu_rm_reserve(
 		bool test_only)
 {
 	struct dpu_rm_requirements reqs;
+	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
 	int ret;
 
 	/* Check if this is just a page-flip */
@@ -653,14 +670,13 @@  int dpu_rm_reserve(
 
 	mutex_lock(&rm->rm_lock);
 
-	ret = _dpu_rm_populate_requirements(rm, enc, crtc_state, &reqs,
-					    topology);
+	ret = _dpu_rm_populate_requirements(rm, enc, &reqs, topology);
 	if (ret) {
 		DPU_ERROR("failed to populate hw requirements\n");
 		goto end;
 	}
 
-	ret = _dpu_rm_make_reservation(rm, enc, crtc_state, &reqs);
+	ret = _dpu_rm_make_reservation(rm, enc, dpu_cstate, &reqs);
 	if (ret) {
 		DPU_ERROR("failed to reserve hw resources: %d\n", ret);
 		_dpu_rm_release_reservation(rm, enc->base.id);

Comments

On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote:
> DPU maintained reservation lists to cache assigned
> HW blocks for the display and a retrieval mechanism for
> the individual DRM components to query their respective
> HW blocks.
> 
> This patch uses the sub-classed CRTC state to store
> and track HW blocks assigned for different components
> of the display pipeline. It helps the driver:
> - to get rid of unwanted store and retrieval RM API's
> - to preserve HW resources assigned in atomic_check
>   through atomic swap/duplicate.
> 
> Separate patch is submitted to remove resource
> reservation in atomic_commit path.
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 65 +++-------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           | 14 +++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 28 +++-------
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 20 ++-----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 58 ++++++++++++-------
>  5 files changed, 72 insertions(+), 113 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 4960641..0625f56 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc,
>  	trace_dpu_crtc_complete_commit(DRMID(crtc));
>  }
>  
> -static void _dpu_crtc_setup_mixer_for_encoder(
> -		struct drm_crtc *crtc,
> -		struct drm_encoder *enc)
> +static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>  {
>  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> -	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
> -	struct dpu_rm *rm = &dpu_kms->rm;
>  	struct dpu_crtc_mixer *mixer;
> -	struct dpu_hw_ctl *last_valid_ctl = NULL;
> -	int i;
> -	struct dpu_rm_hw_iter lm_iter, ctl_iter;
> -
> -	dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM);
> -	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
> +	int i, ctl_index;
>  
>  	/* Set up all the mixers and ctls reserved by this encoder */
> -	for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) {
> +	for (i = 0; i < cstate->num_mixers; i++) {
>  		mixer = &cstate->mixers[i];
>  
> -		if (!dpu_rm_get_hw(rm, &lm_iter))
> -			break;
> -		mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw;
> -
>  		/* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */
> -		if (!dpu_rm_get_hw(rm, &ctl_iter)) {
> -			DPU_DEBUG("no ctl assigned to lm %d, using previous\n",
> -					mixer->hw_lm->idx - LM_0);
> -			mixer->lm_ctl = last_valid_ctl;
> -		} else {
> -			mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
> -			last_valid_ctl = mixer->lm_ctl;
> -		}
> -
> -		/* Shouldn't happen, mixers are always >= ctls */
> -		if (!mixer->lm_ctl) {
> -			DPU_ERROR("no valid ctls found for lm %d\n",
> -					mixer->hw_lm->idx - LM_0);
> -			return;
> -		}
> -
> -		cstate->num_mixers++;
> -		DPU_DEBUG("setup mixer %d: lm %d\n",
> -				i, mixer->hw_lm->idx - LM_0);
> -		DPU_DEBUG("setup mixer %d: ctl %d\n",
> -				i, mixer->lm_ctl->idx - CTL_0);
> -	}
> -}
> -
> -static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
> -{
> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> -	struct drm_encoder *enc;
> -
> -	mutex_lock(&dpu_crtc->crtc_lock);
> -	/* Check for mixers on all encoders attached to this crtc */
> -	list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) {
> -		if (enc->crtc != crtc)
> -			continue;
> -
> -		_dpu_crtc_setup_mixer_for_encoder(crtc, enc);
> +		ctl_index = min(i, cstate->num_ctls - 1);

This is another one of those places I mentioned where we're just assuming a
value is going to be in a certain range. If num_ctls/num_intfs/num_phys_encs
(all the same value afaict) is 0, we end up in a bad place.

At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this can never
drop below 0.

> +		mixer->lm_ctl = cstate->hw_ctls[ctl_index];
>  	}
> -
> -	mutex_unlock(&dpu_crtc->crtc_lock);
>  }
>  
>  static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
> @@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc,
>  	dev = crtc->dev;
>  	smmu_state = &dpu_crtc->smmu_state;
>  
> -	if (!cstate->num_mixers) {
> -		_dpu_crtc_setup_mixers(crtc);
> -		_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
> -	}
> +	_dpu_crtc_setup_mixers(crtc);
> +	_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>  
>  	if (dpu_crtc->event) {
>  		WARN_ON(dpu_crtc->event);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 75fdd3c..17aaad7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -84,12 +84,14 @@ struct dpu_crtc_smmu_state_data {
>   * struct dpu_crtc_mixer: stores the map for each virtual pipeline in the CRTC
>   * @hw_lm:	LM HW Driver context
>   * @lm_ctl:	CTL Path HW driver context
> + * @hw_pp:	Pingpong HW driver context
>   * @mixer_op_mode:	mixer blending operation mode
>   * @flush_mask:	mixer flush mask for ctl, mixer and pipe
>   */
>  struct dpu_crtc_mixer {
>  	struct dpu_hw_mixer *hw_lm;
>  	struct dpu_hw_ctl *lm_ctl;
> +	struct dpu_hw_pingpong *hw_pp;
>  	u32 mixer_op_mode;
>  	u32 flush_mask;
>  };
> @@ -230,6 +232,18 @@ struct dpu_crtc_state {
>  
>  	u32 num_ctls;
>  	struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
> +
> +	/**
> +	 * as drm encoders doesn't have dedicates state objects
> +	 * and drm connectors are not owned by DPU, maintain
> +	 * HW interface and other interface related blocks
> +	 * in crtc state

Can you use private state instead?

> +	 *
> +	 * TODO: No support for clone mode yet where a crtc
> +	 * can be attached with more than one encoder/connector.
> +	 */
> +	u32 num_intfs;
> +	struct dpu_hw_intf *hw_intfs[CRTC_DUAL_MIXERS];
>  };
>  
>  #define to_dpu_crtc_state(x) \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index d12f896..17dbbc3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1001,9 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  	struct dpu_kms *dpu_kms;
>  	struct list_head *connector_list;
>  	struct drm_connector *conn = NULL, *conn_iter;
> -	struct dpu_rm_hw_iter pp_iter, ctl_iter;
> +	struct dpu_crtc_state *dpu_cstate;
>  	struct msm_display_topology topology;
> -	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
>  	int i = 0, ret;
>  
>  	if (!drm_enc) {
> @@ -1043,27 +1042,14 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  		return;
>  	}
>  
> -	dpu_rm_init_hw_iter(&pp_iter, drm_enc->base.id, DPU_HW_BLK_PINGPONG);
> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> -		dpu_enc->hw_pp[i] = NULL;
> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &pp_iter))
> -			break;
> -		dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw;
> -	}
> -
> -	dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter))
> -			break;
> -		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
> -	}
> +	dpu_cstate = to_dpu_crtc_state(drm_enc->crtc->state);
>  
>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>  		int ctl_index;
>  
>  		if (phys) {
> -			if (!dpu_enc->hw_pp[i]) {
> +			if (!dpu_cstate->mixers[i].hw_pp) {
>  				DPU_ERROR_ENC(dpu_enc, "no pp block assigned"
>  					     "at idx: %d\n", i);
>  				return;
> @@ -1071,14 +1057,16 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  
>  			ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1 : 0;
>  
> -			if (!hw_ctl[ctl_index]) {
> +			if (!dpu_cstate->hw_ctls[ctl_index]) {
>  				DPU_ERROR_ENC(dpu_enc, "no ctl block assigned"
>  					     "at idx: %d\n", ctl_index);
>  				return;
>  			}
>  
> -			phys->hw_pp = dpu_enc->hw_pp[i];
> -			phys->hw_ctl = hw_ctl[ctl_index];
> +			dpu_enc->hw_pp[i] = dpu_cstate->mixers[i].hw_pp;
> +
> +			phys->hw_pp = dpu_cstate->mixers[i].hw_pp;
> +			phys->hw_ctl = dpu_cstate->hw_ctls[ctl_index];
>  
>  			phys->connector = conn->state->connector;
>  			if (phys->ops.mode_set)
> 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 84de385..4563f8d 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
> @@ -461,28 +461,20 @@ static int dpu_encoder_phys_vid_control_vblank_irq(
>  
>  static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc)
>  {
> -	struct msm_drm_private *priv;
>  	struct dpu_encoder_phys_vid *vid_enc;
> -	struct dpu_rm_hw_iter iter;
>  	struct dpu_hw_ctl *ctl;
> +	struct dpu_crtc_state *dpu_cstate;
> +	int i;
>  	u32 flush_mask = 0;
>  
> -	if (!phys_enc || !phys_enc->parent || !phys_enc->parent->dev ||
> -			!phys_enc->parent->dev->dev_private) {
> -		DPU_ERROR("invalid encoder/device\n");
> -		return;
> -	}
> -	priv = phys_enc->parent->dev->dev_private;
> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>  
>  	vid_enc = to_dpu_encoder_phys_vid(phys_enc);
>  	ctl = phys_enc->hw_ctl;
>  
> -	dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_INTF);
> -	while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) {
> -		struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf *)iter.hw;
> -
> -		if (hw_intf->idx == phys_enc->intf_idx) {
> -			vid_enc->hw_intf = hw_intf;
> +	for (i = 0; i < dpu_cstate->num_intfs; i++) {
> +		if (dpu_cstate->hw_intfs[i]->idx == phys_enc->intf_idx) {
> +			vid_enc->hw_intf = dpu_cstate->hw_intfs[i];
>  			break;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 377def7..5703b11 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -388,6 +388,7 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(
>  }
>  
>  static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
> +			       struct dpu_crtc_state *dpu_cstate,
>  			       struct dpu_rm_requirements *reqs)
>  
>  {
> @@ -449,16 +450,22 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
>  		lm[i]->enc_id = enc_id;
>  		pp[i]->enc_id = enc_id;
>  
> +		dpu_cstate->mixers[i].hw_lm = to_dpu_hw_mixer(lm[i]->hw);
> +		dpu_cstate->mixers[i].hw_pp = to_dpu_hw_pingpong(pp[i]->hw);
> +
>  		trace_dpu_rm_reserve_lms(lm[i]->id, DPU_HW_BLK_LM, enc_id,
>  					 pp[i]->id);
>  	}
>  
> +	dpu_cstate->num_mixers = lm_count;
> +
>  	return rc;
>  }
>  
>  static int _dpu_rm_reserve_ctls(
>  		struct dpu_rm *rm,
>  		uint32_t enc_id,
> +		struct dpu_crtc_state *dpu_cstate,
>  		const struct msm_display_topology *top)
>  {
>  	struct dpu_rm_hw_blk *ctls[MAX_BLOCKS];
> @@ -501,21 +508,24 @@ static int _dpu_rm_reserve_ctls(
>  
>  	for (i = 0; i < ARRAY_SIZE(ctls) && i < num_ctls; i++) {
>  		ctls[i]->enc_id = enc_id;
> +		dpu_cstate->hw_ctls[i] = to_dpu_hw_ctl(ctls[i]->hw);
> +
>  		trace_dpu_rm_reserve_ctls(ctls[i]->id, DPU_HW_BLK_CTL,
>  					  enc_id);
>  	}
>  
> +	dpu_cstate->num_ctls = num_ctls;
> +
>  	return 0;
>  }
>  
> -static int _dpu_rm_reserve_intf(
> +static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf(
>  		struct dpu_rm *rm,
>  		uint32_t enc_id,
>  		uint32_t id,
>  		enum dpu_hw_blk_type type)
>  {
>  	struct dpu_rm_hw_iter iter;
> -	int ret = 0;
>  
>  	/* Find the block entry in the rm, and note the reservation */
>  	dpu_rm_init_hw_iter(&iter, 0, type);
> @@ -525,7 +535,7 @@ static int _dpu_rm_reserve_intf(
>  
>  		if (RESERVED_BY_OTHER(iter.blk, enc_id)) {
>  			DPU_ERROR("type %d id %d already reserved\n", type, id);
> -			return -ENAVAIL;
> +			return NULL;
>  		}
>  
>  		iter.blk->enc_id = enc_id;
> @@ -535,56 +545,63 @@ static int _dpu_rm_reserve_intf(
>  	}
>  
>  	/* Shouldn't happen since intfs are fixed at probe */
> -	if (!iter.hw) {
> +	if (!iter.blk) {
>  		DPU_ERROR("couldn't find type %d id %d\n", type, id);
> -		return -EINVAL;
> +		return NULL;
>  	}
>  
> -	return ret;
> +	return iter.blk;
>  }
>  
>  static int _dpu_rm_reserve_intf_related_hw(
>  		struct dpu_rm *rm,
>  		uint32_t enc_id,
> +		struct dpu_crtc_state *dpu_cstate,
>  		struct dpu_encoder_hw_resources *hw_res)
>  {
> -	int i, ret = 0;
> -	u32 id;
> +	struct dpu_rm_hw_blk *blk;
> +	int i, num_intfs = 0;
>  
>  	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, enc_id, id,
> +
> +		blk = _dpu_rm_reserve_intf(rm, enc_id, i + INTF_0,
>  				DPU_HW_BLK_INTF);
> -		if (ret)
> -			return ret;
> +		if (!blk)
> +			return -ENAVAIL;
> +
> +		dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw);
>  	}
>  
> -	return ret;
> +	dpu_cstate->num_intfs = num_intfs;
> +
> +	return 0;
>  }
>  
>  static int _dpu_rm_make_reservation(
>  		struct dpu_rm *rm,
>  		struct drm_encoder *enc,
> -		struct drm_crtc_state *crtc_state,
> +		struct dpu_crtc_state *dpu_cstate,
>  		struct dpu_rm_requirements *reqs)
>  {
>  	int ret;
>  
> -	ret = _dpu_rm_reserve_lms(rm, enc->base.id, reqs);
> +	ret = _dpu_rm_reserve_lms(rm, enc->base.id, dpu_cstate, reqs);
>  	if (ret) {
>  		DPU_ERROR("unable to find appropriate mixers\n");
>  		return ret;
>  	}
>  
> -	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, &reqs->topology);
> +	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, dpu_cstate,
> +				   &reqs->topology);
>  	if (ret) {
>  		DPU_ERROR("unable to find appropriate CTL\n");
>  		return ret;
>  	}
>  
> -	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id, &reqs->hw_res);
> +	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id, dpu_cstate,
> +					      &reqs->hw_res);
>  	if (ret)
>  		return ret;
>  
> @@ -594,7 +611,6 @@ static int _dpu_rm_make_reservation(
>  static int _dpu_rm_populate_requirements(
>  		struct dpu_rm *rm,
>  		struct drm_encoder *enc,
> -		struct drm_crtc_state *crtc_state,
>  		struct dpu_rm_requirements *reqs,
>  		struct msm_display_topology req_topology)
>  {
> @@ -642,6 +658,7 @@ int dpu_rm_reserve(
>  		bool test_only)
>  {
>  	struct dpu_rm_requirements reqs;
> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>  	int ret;
>  
>  	/* Check if this is just a page-flip */
> @@ -653,14 +670,13 @@ int dpu_rm_reserve(
>  
>  	mutex_lock(&rm->rm_lock);
>  
> -	ret = _dpu_rm_populate_requirements(rm, enc, crtc_state, &reqs,
> -					    topology);
> +	ret = _dpu_rm_populate_requirements(rm, enc, &reqs, topology);
>  	if (ret) {
>  		DPU_ERROR("failed to populate hw requirements\n");
>  		goto end;
>  	}
>  
> -	ret = _dpu_rm_make_reservation(rm, enc, crtc_state, &reqs);
> +	ret = _dpu_rm_make_reservation(rm, enc, dpu_cstate, &reqs);
>  	if (ret) {
>  		DPU_ERROR("failed to reserve hw resources: %d\n", ret);
>  		_dpu_rm_release_reservation(rm, enc->base.id);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
Hi Jeykumar,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on robclark/msm-next]
[also build test WARNING on next-20181009]
[cannot apply to v4.19-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jeykumar-Sankaran/reserve-RM-resources-in-CRTC-state/20181010-031051
base:   git://people.freedesktop.org/~robclark/linux msm-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:9:0,
                    from include/linux/wait.h:7,
                    from include/linux/wait_bit.h:8,
                    from include/linux/fs.h:6,
                    from include/linux/debugfs.h:15,
                    from drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:21:
   drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c: In function '_dpu_crtc_setup_mixers':
   include/linux/kernel.h:845:29: warning: comparison of distinct pointer types lacks a cast
      (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                ^
   include/linux/kernel.h:859:4: note: in expansion of macro '__typecheck'
      (__typecheck(x, y) && __no_side_effects(x, y))
       ^~~~~~~~~~~
   include/linux/kernel.h:869:24: note: in expansion of macro '__safe_cmp'
     __builtin_choose_expr(__safe_cmp(x, y), \
                           ^~~~~~~~~~
   include/linux/kernel.h:878:19: note: in expansion of macro '__careful_cmp'
    #define min(x, y) __careful_cmp(x, y, <)
                      ^~~~~~~~~~~~~
>> drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c:435:15: note: in expansion of macro 'min'
      ctl_index = min(i, cstate->num_ctls - 1);
                  ^~~

vim +/min +435 drivers/gpu//drm/msm/disp/dpu1/dpu_crtc.c

   423	
   424	static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
   425	{
   426		struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
   427		struct dpu_crtc_mixer *mixer;
   428		int i, ctl_index;
   429	
   430		/* Set up all the mixers and ctls reserved by this encoder */
   431		for (i = 0; i < cstate->num_mixers; i++) {
   432			mixer = &cstate->mixers[i];
   433	
   434			/* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */
 > 435			ctl_index = min(i, cstate->num_ctls - 1);
   436			mixer->lm_ctl = cstate->hw_ctls[ctl_index];
   437		}
   438	}
   439	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
On 2018-10-09 14:06, Sean Paul wrote:
> On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote:
>> DPU maintained reservation lists to cache assigned
>> HW blocks for the display and a retrieval mechanism for
>> the individual DRM components to query their respective
>> HW blocks.
>> 
>> This patch uses the sub-classed CRTC state to store
>> and track HW blocks assigned for different components
>> of the display pipeline. It helps the driver:
>> - to get rid of unwanted store and retrieval RM API's
>> - to preserve HW resources assigned in atomic_check
>>   through atomic swap/duplicate.
>> 
>> Separate patch is submitted to remove resource
>> reservation in atomic_commit path.
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 65
> +++-------------------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           | 14 +++++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 28 +++-------
>>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 20 ++-----
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 58
> ++++++++++++-------
>>  5 files changed, 72 insertions(+), 113 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 4960641..0625f56 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc
> *crtc,
>>  	trace_dpu_crtc_complete_commit(DRMID(crtc));
>>  }
>> 
>> -static void _dpu_crtc_setup_mixer_for_encoder(
>> -		struct drm_crtc *crtc,
>> -		struct drm_encoder *enc)
>> +static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>>  {
>>  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>> -	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>> -	struct dpu_rm *rm = &dpu_kms->rm;
>>  	struct dpu_crtc_mixer *mixer;
>> -	struct dpu_hw_ctl *last_valid_ctl = NULL;
>> -	int i;
>> -	struct dpu_rm_hw_iter lm_iter, ctl_iter;
>> -
>> -	dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM);
>> -	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
>> +	int i, ctl_index;
>> 
>>  	/* Set up all the mixers and ctls reserved by this encoder */
>> -	for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
> {
>> +	for (i = 0; i < cstate->num_mixers; i++) {
>>  		mixer = &cstate->mixers[i];
>> 
>> -		if (!dpu_rm_get_hw(rm, &lm_iter))
>> -			break;
>> -		mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw;
>> -
>>  		/* CTL may be <= LMs, if <, multiple LMs controlled by 1
> CTL */
>> -		if (!dpu_rm_get_hw(rm, &ctl_iter)) {
>> -			DPU_DEBUG("no ctl assigned to lm %d, using
> previous\n",
>> -					mixer->hw_lm->idx - LM_0);
>> -			mixer->lm_ctl = last_valid_ctl;
>> -		} else {
>> -			mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
>> -			last_valid_ctl = mixer->lm_ctl;
>> -		}
>> -
>> -		/* Shouldn't happen, mixers are always >= ctls */
>> -		if (!mixer->lm_ctl) {
>> -			DPU_ERROR("no valid ctls found for lm %d\n",
>> -					mixer->hw_lm->idx - LM_0);
>> -			return;
>> -		}
>> -
>> -		cstate->num_mixers++;
>> -		DPU_DEBUG("setup mixer %d: lm %d\n",
>> -				i, mixer->hw_lm->idx - LM_0);
>> -		DPU_DEBUG("setup mixer %d: ctl %d\n",
>> -				i, mixer->lm_ctl->idx - CTL_0);
>> -	}
>> -}
>> -
>> -static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>> -{
>> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> -	struct drm_encoder *enc;
>> -
>> -	mutex_lock(&dpu_crtc->crtc_lock);
>> -	/* Check for mixers on all encoders attached to this crtc */
>> -	list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list,
> head) {
>> -		if (enc->crtc != crtc)
>> -			continue;
>> -
>> -		_dpu_crtc_setup_mixer_for_encoder(crtc, enc);
>> +		ctl_index = min(i, cstate->num_ctls - 1);
> 
> This is another one of those places I mentioned where we're just 
> assuming
> a
> value is going to be in a certain range. If
> num_ctls/num_intfs/num_phys_encs
> (all the same value afaict) is 0, we end up in a bad place.
Even though all these variables have the same value, they are 
representing the
sizes of logically seperate components.

> 
> At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this 
> can
> never
> drop below 0.
Isn't RM guaranteeing that? I can add the WARN_ON checks on these
num_xxx when the HW blocks are allocated.

Thanks,
Jeykumar S.
> 
>> +		mixer->lm_ctl = cstate->hw_ctls[ctl_index];
>>  	}
>> -
>> -	mutex_unlock(&dpu_crtc->crtc_lock);
>>  }
>> 
>>  static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
>> @@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct drm_crtc
> *crtc,
>>  	dev = crtc->dev;
>>  	smmu_state = &dpu_crtc->smmu_state;
>> 
>> -	if (!cstate->num_mixers) {
>> -		_dpu_crtc_setup_mixers(crtc);
>> -		_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>> -	}
>> +	_dpu_crtc_setup_mixers(crtc);
>> +	_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>> 
>>  	if (dpu_crtc->event) {
>>  		WARN_ON(dpu_crtc->event);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index 75fdd3c..17aaad7 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -84,12 +84,14 @@ struct dpu_crtc_smmu_state_data {
>>   * struct dpu_crtc_mixer: stores the map for each virtual pipeline in
> the CRTC
>>   * @hw_lm:	LM HW Driver context
>>   * @lm_ctl:	CTL Path HW driver context
>> + * @hw_pp:	Pingpong HW driver context
>>   * @mixer_op_mode:	mixer blending operation mode
>>   * @flush_mask:	mixer flush mask for ctl, mixer and pipe
>>   */
>>  struct dpu_crtc_mixer {
>>  	struct dpu_hw_mixer *hw_lm;
>>  	struct dpu_hw_ctl *lm_ctl;
>> +	struct dpu_hw_pingpong *hw_pp;
>>  	u32 mixer_op_mode;
>>  	u32 flush_mask;
>>  };
>> @@ -230,6 +232,18 @@ struct dpu_crtc_state {
>> 
>>  	u32 num_ctls;
>>  	struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
>> +
>> +	/**
>> +	 * as drm encoders doesn't have dedicates state objects
>> +	 * and drm connectors are not owned by DPU, maintain
>> +	 * HW interface and other interface related blocks
>> +	 * in crtc state
> 
> Can you use private state instead?
hmm.. It's time for me to revisit the old patch:
https://patchwork.freedesktop.org/patch/230100/
> 
>> +	 *
>> +	 * TODO: No support for clone mode yet where a crtc
>> +	 * can be attached with more than one encoder/connector.
>> +	 */
>> +	u32 num_intfs;
>> +	struct dpu_hw_intf *hw_intfs[CRTC_DUAL_MIXERS];
>>  };
>> 
>>  #define to_dpu_crtc_state(x) \
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index d12f896..17dbbc3 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1001,9 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  	struct dpu_kms *dpu_kms;
>>  	struct list_head *connector_list;
>>  	struct drm_connector *conn = NULL, *conn_iter;
>> -	struct dpu_rm_hw_iter pp_iter, ctl_iter;
>> +	struct dpu_crtc_state *dpu_cstate;
>>  	struct msm_display_topology topology;
>> -	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
>>  	int i = 0, ret;
>> 
>>  	if (!drm_enc) {
>> @@ -1043,27 +1042,14 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  		return;
>>  	}
>> 
>> -	dpu_rm_init_hw_iter(&pp_iter, drm_enc->base.id,
> DPU_HW_BLK_PINGPONG);
>> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>> -		dpu_enc->hw_pp[i] = NULL;
>> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &pp_iter))
>> -			break;
>> -		dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw;
>> -	}
>> -
>> -	dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
>> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter))
>> -			break;
>> -		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
>> -	}
>> +	dpu_cstate = to_dpu_crtc_state(drm_enc->crtc->state);
>> 
>>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>  		int ctl_index;
>> 
>>  		if (phys) {
>> -			if (!dpu_enc->hw_pp[i]) {
>> +			if (!dpu_cstate->mixers[i].hw_pp) {
>>  				DPU_ERROR_ENC(dpu_enc, "no pp block
> assigned"
>>  					     "at idx: %d\n", i);
>>  				return;
>> @@ -1071,14 +1057,16 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>> 
>>  			ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1
> : 0;
>> 
>> -			if (!hw_ctl[ctl_index]) {
>> +			if (!dpu_cstate->hw_ctls[ctl_index]) {
>>  				DPU_ERROR_ENC(dpu_enc, "no ctl block
> assigned"
>>  					     "at idx: %d\n", ctl_index);
>>  				return;
>>  			}
>> 
>> -			phys->hw_pp = dpu_enc->hw_pp[i];
>> -			phys->hw_ctl = hw_ctl[ctl_index];
>> +			dpu_enc->hw_pp[i] = dpu_cstate->mixers[i].hw_pp;
>> +
>> +			phys->hw_pp = dpu_cstate->mixers[i].hw_pp;
>> +			phys->hw_ctl = dpu_cstate->hw_ctls[ctl_index];
>> 
>>  			phys->connector = conn->state->connector;
>>  			if (phys->ops.mode_set)
>> 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 84de385..4563f8d 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
>> @@ -461,28 +461,20 @@ static int
> dpu_encoder_phys_vid_control_vblank_irq(
>> 
>>  static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys
> *phys_enc)
>>  {
>> -	struct msm_drm_private *priv;
>>  	struct dpu_encoder_phys_vid *vid_enc;
>> -	struct dpu_rm_hw_iter iter;
>>  	struct dpu_hw_ctl *ctl;
>> +	struct dpu_crtc_state *dpu_cstate;
>> +	int i;
>>  	u32 flush_mask = 0;
>> 
>> -	if (!phys_enc || !phys_enc->parent || !phys_enc->parent->dev ||
>> -			!phys_enc->parent->dev->dev_private) {
>> -		DPU_ERROR("invalid encoder/device\n");
>> -		return;
>> -	}
>> -	priv = phys_enc->parent->dev->dev_private;
>> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>> 
>>  	vid_enc = to_dpu_encoder_phys_vid(phys_enc);
>>  	ctl = phys_enc->hw_ctl;
>> 
>> -	dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id,
> DPU_HW_BLK_INTF);
>> -	while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) {
>> -		struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf
> *)iter.hw;
>> -
>> -		if (hw_intf->idx == phys_enc->intf_idx) {
>> -			vid_enc->hw_intf = hw_intf;
>> +	for (i = 0; i < dpu_cstate->num_intfs; i++) {
>> +		if (dpu_cstate->hw_intfs[i]->idx == phys_enc->intf_idx) {
>> +			vid_enc->hw_intf = dpu_cstate->hw_intfs[i];
>>  			break;
>>  		}
>>  	}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 377def7..5703b11 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -388,6 +388,7 @@ static bool 
>> _dpu_rm_check_lm_and_get_connected_blks(
>>  }
>> 
>>  static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
>> +			       struct dpu_crtc_state *dpu_cstate,
>>  			       struct dpu_rm_requirements *reqs)
>> 
>>  {
>> @@ -449,16 +450,22 @@ static int _dpu_rm_reserve_lms(struct dpu_rm 
>> *rm,
> uint32_t enc_id,
>>  		lm[i]->enc_id = enc_id;
>>  		pp[i]->enc_id = enc_id;
>> 
>> +		dpu_cstate->mixers[i].hw_lm = to_dpu_hw_mixer(lm[i]->hw);
>> +		dpu_cstate->mixers[i].hw_pp =
> to_dpu_hw_pingpong(pp[i]->hw);
>> +
>>  		trace_dpu_rm_reserve_lms(lm[i]->id, DPU_HW_BLK_LM, enc_id,
>>  					 pp[i]->id);
>>  	}
>> 
>> +	dpu_cstate->num_mixers = lm_count;
>> +
>>  	return rc;
>>  }
>> 
>>  static int _dpu_rm_reserve_ctls(
>>  		struct dpu_rm *rm,
>>  		uint32_t enc_id,
>> +		struct dpu_crtc_state *dpu_cstate,
>>  		const struct msm_display_topology *top)
>>  {
>>  	struct dpu_rm_hw_blk *ctls[MAX_BLOCKS];
>> @@ -501,21 +508,24 @@ static int _dpu_rm_reserve_ctls(
>> 
>>  	for (i = 0; i < ARRAY_SIZE(ctls) && i < num_ctls; i++) {
>>  		ctls[i]->enc_id = enc_id;
>> +		dpu_cstate->hw_ctls[i] = to_dpu_hw_ctl(ctls[i]->hw);
>> +
>>  		trace_dpu_rm_reserve_ctls(ctls[i]->id, DPU_HW_BLK_CTL,
>>  					  enc_id);
>>  	}
>> 
>> +	dpu_cstate->num_ctls = num_ctls;
>> +
>>  	return 0;
>>  }
>> 
>> -static int _dpu_rm_reserve_intf(
>> +static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf(
>>  		struct dpu_rm *rm,
>>  		uint32_t enc_id,
>>  		uint32_t id,
>>  		enum dpu_hw_blk_type type)
>>  {
>>  	struct dpu_rm_hw_iter iter;
>> -	int ret = 0;
>> 
>>  	/* Find the block entry in the rm, and note the reservation */
>>  	dpu_rm_init_hw_iter(&iter, 0, type);
>> @@ -525,7 +535,7 @@ static int _dpu_rm_reserve_intf(
>> 
>>  		if (RESERVED_BY_OTHER(iter.blk, enc_id)) {
>>  			DPU_ERROR("type %d id %d already reserved\n",
> type, id);
>> -			return -ENAVAIL;
>> +			return NULL;
>>  		}
>> 
>>  		iter.blk->enc_id = enc_id;
>> @@ -535,56 +545,63 @@ static int _dpu_rm_reserve_intf(
>>  	}
>> 
>>  	/* Shouldn't happen since intfs are fixed at probe */
>> -	if (!iter.hw) {
>> +	if (!iter.blk) {
>>  		DPU_ERROR("couldn't find type %d id %d\n", type, id);
>> -		return -EINVAL;
>> +		return NULL;
>>  	}
>> 
>> -	return ret;
>> +	return iter.blk;
>>  }
>> 
>>  static int _dpu_rm_reserve_intf_related_hw(
>>  		struct dpu_rm *rm,
>>  		uint32_t enc_id,
>> +		struct dpu_crtc_state *dpu_cstate,
>>  		struct dpu_encoder_hw_resources *hw_res)
>>  {
>> -	int i, ret = 0;
>> -	u32 id;
>> +	struct dpu_rm_hw_blk *blk;
>> +	int i, num_intfs = 0;
>> 
>>  	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, enc_id, id,
>> +
>> +		blk = _dpu_rm_reserve_intf(rm, enc_id, i + INTF_0,
>>  				DPU_HW_BLK_INTF);
>> -		if (ret)
>> -			return ret;
>> +		if (!blk)
>> +			return -ENAVAIL;
>> +
>> +		dpu_cstate->hw_intfs[num_intfs++] =
> to_dpu_hw_intf(blk->hw);
>>  	}
>> 
>> -	return ret;
>> +	dpu_cstate->num_intfs = num_intfs;
>> +
>> +	return 0;
>>  }
>> 
>>  static int _dpu_rm_make_reservation(
>>  		struct dpu_rm *rm,
>>  		struct drm_encoder *enc,
>> -		struct drm_crtc_state *crtc_state,
>> +		struct dpu_crtc_state *dpu_cstate,
>>  		struct dpu_rm_requirements *reqs)
>>  {
>>  	int ret;
>> 
>> -	ret = _dpu_rm_reserve_lms(rm, enc->base.id, reqs);
>> +	ret = _dpu_rm_reserve_lms(rm, enc->base.id, dpu_cstate, reqs);
>>  	if (ret) {
>>  		DPU_ERROR("unable to find appropriate mixers\n");
>>  		return ret;
>>  	}
>> 
>> -	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, &reqs->topology);
>> +	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, dpu_cstate,
>> +				   &reqs->topology);
>>  	if (ret) {
>>  		DPU_ERROR("unable to find appropriate CTL\n");
>>  		return ret;
>>  	}
>> 
>> -	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id,
> &reqs->hw_res);
>> +	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id,
> dpu_cstate,
>> +					      &reqs->hw_res);
>>  	if (ret)
>>  		return ret;
>> 
>> @@ -594,7 +611,6 @@ static int _dpu_rm_make_reservation(
>>  static int _dpu_rm_populate_requirements(
>>  		struct dpu_rm *rm,
>>  		struct drm_encoder *enc,
>> -		struct drm_crtc_state *crtc_state,
>>  		struct dpu_rm_requirements *reqs,
>>  		struct msm_display_topology req_topology)
>>  {
>> @@ -642,6 +658,7 @@ int dpu_rm_reserve(
>>  		bool test_only)
>>  {
>>  	struct dpu_rm_requirements reqs;
>> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>>  	int ret;
>> 
>>  	/* Check if this is just a page-flip */
>> @@ -653,14 +670,13 @@ int dpu_rm_reserve(
>> 
>>  	mutex_lock(&rm->rm_lock);
>> 
>> -	ret = _dpu_rm_populate_requirements(rm, enc, crtc_state, &reqs,
>> -					    topology);
>> +	ret = _dpu_rm_populate_requirements(rm, enc, &reqs, topology);
>>  	if (ret) {
>>  		DPU_ERROR("failed to populate hw requirements\n");
>>  		goto end;
>>  	}
>> 
>> -	ret = _dpu_rm_make_reservation(rm, enc, crtc_state, &reqs);
>> +	ret = _dpu_rm_make_reservation(rm, enc, dpu_cstate, &reqs);
>>  	if (ret) {
>>  		DPU_ERROR("failed to reserve hw resources: %d\n", ret);
>>  		_dpu_rm_release_reservation(rm, enc->base.id);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>>
On 2018-10-09 23:28, Jeykumar Sankaran wrote:
> On 2018-10-09 14:06, Sean Paul wrote:
>> On Mon, Oct 08, 2018 at 09:27:24PM -0700, Jeykumar Sankaran wrote:
>>> DPU maintained reservation lists to cache assigned
>>> HW blocks for the display and a retrieval mechanism for
>>> the individual DRM components to query their respective
>>> HW blocks.
>>> 
>>> This patch uses the sub-classed CRTC state to store
>>> and track HW blocks assigned for different components
>>> of the display pipeline. It helps the driver:
>>> - to get rid of unwanted store and retrieval RM API's
>>> - to preserve HW resources assigned in atomic_check
>>>   through atomic swap/duplicate.
>>> 
>>> Separate patch is submitted to remove resource
>>> reservation in atomic_commit path.
>>> 
>>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>>> ---
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c           | 65
>> +++-------------------
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h           | 14 +++++
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 28 +++-------
>>>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   | 20 ++-----
>>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 58
>> ++++++++++++-------
>>>  5 files changed, 72 insertions(+), 113 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 4960641..0625f56 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -421,69 +421,20 @@ void dpu_crtc_complete_commit(struct drm_crtc
>> *crtc,
>>>  	trace_dpu_crtc_complete_commit(DRMID(crtc));
>>>  }
>>> 
>>> -static void _dpu_crtc_setup_mixer_for_encoder(
>>> -		struct drm_crtc *crtc,
>>> -		struct drm_encoder *enc)
>>> +static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>>>  {
>>>  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
>>> -	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>>> -	struct dpu_rm *rm = &dpu_kms->rm;
>>>  	struct dpu_crtc_mixer *mixer;
>>> -	struct dpu_hw_ctl *last_valid_ctl = NULL;
>>> -	int i;
>>> -	struct dpu_rm_hw_iter lm_iter, ctl_iter;
>>> -
>>> -	dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM);
>>> -	dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);
>>> +	int i, ctl_index;
>>> 
>>>  	/* Set up all the mixers and ctls reserved by this encoder */
>>> -	for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++)
>> {
>>> +	for (i = 0; i < cstate->num_mixers; i++) {
>>>  		mixer = &cstate->mixers[i];
>>> 
>>> -		if (!dpu_rm_get_hw(rm, &lm_iter))
>>> -			break;
>>> -		mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw;
>>> -
>>>  		/* CTL may be <= LMs, if <, multiple LMs controlled by 1
>> CTL */
>>> -		if (!dpu_rm_get_hw(rm, &ctl_iter)) {
>>> -			DPU_DEBUG("no ctl assigned to lm %d, using
>> previous\n",
>>> -					mixer->hw_lm->idx - LM_0);
>>> -			mixer->lm_ctl = last_valid_ctl;
>>> -		} else {
>>> -			mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
>>> -			last_valid_ctl = mixer->lm_ctl;
>>> -		}
>>> -
>>> -		/* Shouldn't happen, mixers are always >= ctls */
>>> -		if (!mixer->lm_ctl) {
>>> -			DPU_ERROR("no valid ctls found for lm %d\n",
>>> -					mixer->hw_lm->idx - LM_0);
>>> -			return;
>>> -		}
>>> -
>>> -		cstate->num_mixers++;
>>> -		DPU_DEBUG("setup mixer %d: lm %d\n",
>>> -				i, mixer->hw_lm->idx - LM_0);
>>> -		DPU_DEBUG("setup mixer %d: ctl %d\n",
>>> -				i, mixer->lm_ctl->idx - CTL_0);
>>> -	}
>>> -}
>>> -
>>> -static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc)
>>> -{
>>> -	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>>> -	struct drm_encoder *enc;
>>> -
>>> -	mutex_lock(&dpu_crtc->crtc_lock);
>>> -	/* Check for mixers on all encoders attached to this crtc */
>>> -	list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list,
>> head) {
>>> -		if (enc->crtc != crtc)
>>> -			continue;
>>> -
>>> -		_dpu_crtc_setup_mixer_for_encoder(crtc, enc);
>>> +		ctl_index = min(i, cstate->num_ctls - 1);
>> 
>> This is another one of those places I mentioned where we're just 
>> assuming
>> a
>> value is going to be in a certain range. If
>> num_ctls/num_intfs/num_phys_encs
>> (all the same value afaict) is 0, we end up in a bad place.
> Even though all these variables have the same value, they are 
> representing the
> sizes of logically seperate components.
> 
>> 
>> At a minimum, there should be a WARN_ON/BUG_ON somewhere ensuring this 
>> can
>> never
>> drop below 0.
> Isn't RM guaranteeing that? I can add the WARN_ON checks on these
> num_xxx when the HW blocks are allocated.
> 
> Thanks,
> Jeykumar S.
>> 
>>> +		mixer->lm_ctl = cstate->hw_ctls[ctl_index];
>>>  	}
>>> -
>>> -	mutex_unlock(&dpu_crtc->crtc_lock);
>>>  }
>>> 
>>>  static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
>>> @@ -536,10 +487,8 @@ static void dpu_crtc_atomic_begin(struct 
>>> drm_crtc
>> *crtc,
>>>  	dev = crtc->dev;
>>>  	smmu_state = &dpu_crtc->smmu_state;
>>> 
>>> -	if (!cstate->num_mixers) {
>>> -		_dpu_crtc_setup_mixers(crtc);
>>> -		_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>>> -	}
>>> +	_dpu_crtc_setup_mixers(crtc);
>>> +	_dpu_crtc_setup_lm_bounds(crtc, crtc->state);
>>> 
>>>  	if (dpu_crtc->event) {
>>>  		WARN_ON(dpu_crtc->event);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> index 75fdd3c..17aaad7 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>>> @@ -84,12 +84,14 @@ struct dpu_crtc_smmu_state_data {
>>>   * struct dpu_crtc_mixer: stores the map for each virtual pipeline 
>>> in
>> the CRTC
>>>   * @hw_lm:	LM HW Driver context
>>>   * @lm_ctl:	CTL Path HW driver context
>>> + * @hw_pp:	Pingpong HW driver context
>>>   * @mixer_op_mode:	mixer blending operation mode
>>>   * @flush_mask:	mixer flush mask for ctl, mixer and pipe
>>>   */
>>>  struct dpu_crtc_mixer {
>>>  	struct dpu_hw_mixer *hw_lm;
>>>  	struct dpu_hw_ctl *lm_ctl;
>>> +	struct dpu_hw_pingpong *hw_pp;
>>>  	u32 mixer_op_mode;
>>>  	u32 flush_mask;
>>>  };
>>> @@ -230,6 +232,18 @@ struct dpu_crtc_state {
>>> 
>>>  	u32 num_ctls;
>>>  	struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];
>>> +
>>> +	/**
>>> +	 * as drm encoders doesn't have dedicates state objects
>>> +	 * and drm connectors are not owned by DPU, maintain
>>> +	 * HW interface and other interface related blocks
>>> +	 * in crtc state
>> 
>> Can you use private state instead?
> hmm.. It's time for me to revisit the old patch:
> https://patchwork.freedesktop.org/patch/230100/
>> 
Please note that using drm_priv_obj_state to track the per
display hw reservation, I need to maintain MAX_CRTCS drm_priv_obj's
in dpu_kms.

The design would not be the same as mdp5 model since mdp5 uses the
same priv_obj to track the resources of all CRTC's and also maintains
the per crtc hw information in mdp5_crtc_state.

Thanks,
Jeykumar S.

>>> +	 *
>>> +	 * TODO: No support for clone mode yet where a crtc
>>> +	 * can be attached with more than one encoder/connector.
>>> +	 */
>>> +	u32 num_intfs;
>>> +	struct dpu_hw_intf *hw_intfs[CRTC_DUAL_MIXERS];
>>>  };
>>> 
>>>  #define to_dpu_crtc_state(x) \
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index d12f896..17dbbc3 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -1001,9 +1001,8 @@ static void dpu_encoder_virt_mode_set(struct
>> drm_encoder *drm_enc,
>>>  	struct dpu_kms *dpu_kms;
>>>  	struct list_head *connector_list;
>>>  	struct drm_connector *conn = NULL, *conn_iter;
>>> -	struct dpu_rm_hw_iter pp_iter, ctl_iter;
>>> +	struct dpu_crtc_state *dpu_cstate;
>>>  	struct msm_display_topology topology;
>>> -	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
>>>  	int i = 0, ret;
>>> 
>>>  	if (!drm_enc) {
>>> @@ -1043,27 +1042,14 @@ static void dpu_encoder_virt_mode_set(struct
>> drm_encoder *drm_enc,
>>>  		return;
>>>  	}
>>> 
>>> -	dpu_rm_init_hw_iter(&pp_iter, drm_enc->base.id,
>> DPU_HW_BLK_PINGPONG);
>>> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>> -		dpu_enc->hw_pp[i] = NULL;
>>> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &pp_iter))
>>> -			break;
>>> -		dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw;
>>> -	}
>>> -
>>> -	dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
>>> -	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>> -		if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter))
>>> -			break;
>>> -		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
>>> -	}
>>> +	dpu_cstate = to_dpu_crtc_state(drm_enc->crtc->state);
>>> 
>>>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>>>  		int ctl_index;
>>> 
>>>  		if (phys) {
>>> -			if (!dpu_enc->hw_pp[i]) {
>>> +			if (!dpu_cstate->mixers[i].hw_pp) {
>>>  				DPU_ERROR_ENC(dpu_enc, "no pp block
>> assigned"
>>>  					     "at idx: %d\n", i);
>>>  				return;
>>> @@ -1071,14 +1057,16 @@ static void dpu_encoder_virt_mode_set(struct
>> drm_encoder *drm_enc,
>>> 
>>>  			ctl_index = phys->split_role == ENC_ROLE_SLAVE ? 1
>> : 0;
>>> 
>>> -			if (!hw_ctl[ctl_index]) {
>>> +			if (!dpu_cstate->hw_ctls[ctl_index]) {
>>>  				DPU_ERROR_ENC(dpu_enc, "no ctl block
>> assigned"
>>>  					     "at idx: %d\n", ctl_index);
>>>  				return;
>>>  			}
>>> 
>>> -			phys->hw_pp = dpu_enc->hw_pp[i];
>>> -			phys->hw_ctl = hw_ctl[ctl_index];
>>> +			dpu_enc->hw_pp[i] = dpu_cstate->mixers[i].hw_pp;
>>> +
>>> +			phys->hw_pp = dpu_cstate->mixers[i].hw_pp;
>>> +			phys->hw_ctl = dpu_cstate->hw_ctls[ctl_index];
>>> 
>>>  			phys->connector = conn->state->connector;
>>>  			if (phys->ops.mode_set)
>>> 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 84de385..4563f8d 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
>>> @@ -461,28 +461,20 @@ static int
>> dpu_encoder_phys_vid_control_vblank_irq(
>>> 
>>>  static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys
>> *phys_enc)
>>>  {
>>> -	struct msm_drm_private *priv;
>>>  	struct dpu_encoder_phys_vid *vid_enc;
>>> -	struct dpu_rm_hw_iter iter;
>>>  	struct dpu_hw_ctl *ctl;
>>> +	struct dpu_crtc_state *dpu_cstate;
>>> +	int i;
>>>  	u32 flush_mask = 0;
>>> 
>>> -	if (!phys_enc || !phys_enc->parent || !phys_enc->parent->dev ||
>>> -			!phys_enc->parent->dev->dev_private) {
>>> -		DPU_ERROR("invalid encoder/device\n");
>>> -		return;
>>> -	}
>>> -	priv = phys_enc->parent->dev->dev_private;
>>> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>>> 
>>>  	vid_enc = to_dpu_encoder_phys_vid(phys_enc);
>>>  	ctl = phys_enc->hw_ctl;
>>> 
>>> -	dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id,
>> DPU_HW_BLK_INTF);
>>> -	while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) {
>>> -		struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf
>> *)iter.hw;
>>> -
>>> -		if (hw_intf->idx == phys_enc->intf_idx) {
>>> -			vid_enc->hw_intf = hw_intf;
>>> +	for (i = 0; i < dpu_cstate->num_intfs; i++) {
>>> +		if (dpu_cstate->hw_intfs[i]->idx == phys_enc->intf_idx) {
>>> +			vid_enc->hw_intf = dpu_cstate->hw_intfs[i];
>>>  			break;
>>>  		}
>>>  	}
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> index 377def7..5703b11 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -388,6 +388,7 @@ static bool 
>>> _dpu_rm_check_lm_and_get_connected_blks(
>>>  }
>>> 
>>>  static int _dpu_rm_reserve_lms(struct dpu_rm *rm, uint32_t enc_id,
>>> +			       struct dpu_crtc_state *dpu_cstate,
>>>  			       struct dpu_rm_requirements *reqs)
>>> 
>>>  {
>>> @@ -449,16 +450,22 @@ static int _dpu_rm_reserve_lms(struct dpu_rm 
>>> *rm,
>> uint32_t enc_id,
>>>  		lm[i]->enc_id = enc_id;
>>>  		pp[i]->enc_id = enc_id;
>>> 
>>> +		dpu_cstate->mixers[i].hw_lm = to_dpu_hw_mixer(lm[i]->hw);
>>> +		dpu_cstate->mixers[i].hw_pp =
>> to_dpu_hw_pingpong(pp[i]->hw);
>>> +
>>>  		trace_dpu_rm_reserve_lms(lm[i]->id, DPU_HW_BLK_LM, enc_id,
>>>  					 pp[i]->id);
>>>  	}
>>> 
>>> +	dpu_cstate->num_mixers = lm_count;
>>> +
>>>  	return rc;
>>>  }
>>> 
>>>  static int _dpu_rm_reserve_ctls(
>>>  		struct dpu_rm *rm,
>>>  		uint32_t enc_id,
>>> +		struct dpu_crtc_state *dpu_cstate,
>>>  		const struct msm_display_topology *top)
>>>  {
>>>  	struct dpu_rm_hw_blk *ctls[MAX_BLOCKS];
>>> @@ -501,21 +508,24 @@ static int _dpu_rm_reserve_ctls(
>>> 
>>>  	for (i = 0; i < ARRAY_SIZE(ctls) && i < num_ctls; i++) {
>>>  		ctls[i]->enc_id = enc_id;
>>> +		dpu_cstate->hw_ctls[i] = to_dpu_hw_ctl(ctls[i]->hw);
>>> +
>>>  		trace_dpu_rm_reserve_ctls(ctls[i]->id, DPU_HW_BLK_CTL,
>>>  					  enc_id);
>>>  	}
>>> 
>>> +	dpu_cstate->num_ctls = num_ctls;
>>> +
>>>  	return 0;
>>>  }
>>> 
>>> -static int _dpu_rm_reserve_intf(
>>> +static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf(
>>>  		struct dpu_rm *rm,
>>>  		uint32_t enc_id,
>>>  		uint32_t id,
>>>  		enum dpu_hw_blk_type type)
>>>  {
>>>  	struct dpu_rm_hw_iter iter;
>>> -	int ret = 0;
>>> 
>>>  	/* Find the block entry in the rm, and note the reservation */
>>>  	dpu_rm_init_hw_iter(&iter, 0, type);
>>> @@ -525,7 +535,7 @@ static int _dpu_rm_reserve_intf(
>>> 
>>>  		if (RESERVED_BY_OTHER(iter.blk, enc_id)) {
>>>  			DPU_ERROR("type %d id %d already reserved\n",
>> type, id);
>>> -			return -ENAVAIL;
>>> +			return NULL;
>>>  		}
>>> 
>>>  		iter.blk->enc_id = enc_id;
>>> @@ -535,56 +545,63 @@ static int _dpu_rm_reserve_intf(
>>>  	}
>>> 
>>>  	/* Shouldn't happen since intfs are fixed at probe */
>>> -	if (!iter.hw) {
>>> +	if (!iter.blk) {
>>>  		DPU_ERROR("couldn't find type %d id %d\n", type, id);
>>> -		return -EINVAL;
>>> +		return NULL;
>>>  	}
>>> 
>>> -	return ret;
>>> +	return iter.blk;
>>>  }
>>> 
>>>  static int _dpu_rm_reserve_intf_related_hw(
>>>  		struct dpu_rm *rm,
>>>  		uint32_t enc_id,
>>> +		struct dpu_crtc_state *dpu_cstate,
>>>  		struct dpu_encoder_hw_resources *hw_res)
>>>  {
>>> -	int i, ret = 0;
>>> -	u32 id;
>>> +	struct dpu_rm_hw_blk *blk;
>>> +	int i, num_intfs = 0;
>>> 
>>>  	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, enc_id, id,
>>> +
>>> +		blk = _dpu_rm_reserve_intf(rm, enc_id, i + INTF_0,
>>>  				DPU_HW_BLK_INTF);
>>> -		if (ret)
>>> -			return ret;
>>> +		if (!blk)
>>> +			return -ENAVAIL;
>>> +
>>> +		dpu_cstate->hw_intfs[num_intfs++] =
>> to_dpu_hw_intf(blk->hw);
>>>  	}
>>> 
>>> -	return ret;
>>> +	dpu_cstate->num_intfs = num_intfs;
>>> +
>>> +	return 0;
>>>  }
>>> 
>>>  static int _dpu_rm_make_reservation(
>>>  		struct dpu_rm *rm,
>>>  		struct drm_encoder *enc,
>>> -		struct drm_crtc_state *crtc_state,
>>> +		struct dpu_crtc_state *dpu_cstate,
>>>  		struct dpu_rm_requirements *reqs)
>>>  {
>>>  	int ret;
>>> 
>>> -	ret = _dpu_rm_reserve_lms(rm, enc->base.id, reqs);
>>> +	ret = _dpu_rm_reserve_lms(rm, enc->base.id, dpu_cstate, reqs);
>>>  	if (ret) {
>>>  		DPU_ERROR("unable to find appropriate mixers\n");
>>>  		return ret;
>>>  	}
>>> 
>>> -	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, &reqs->topology);
>>> +	ret = _dpu_rm_reserve_ctls(rm, enc->base.id, dpu_cstate,
>>> +				   &reqs->topology);
>>>  	if (ret) {
>>>  		DPU_ERROR("unable to find appropriate CTL\n");
>>>  		return ret;
>>>  	}
>>> 
>>> -	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id,
>> &reqs->hw_res);
>>> +	ret = _dpu_rm_reserve_intf_related_hw(rm, enc->base.id,
>> dpu_cstate,
>>> +					      &reqs->hw_res);
>>>  	if (ret)
>>>  		return ret;
>>> 
>>> @@ -594,7 +611,6 @@ static int _dpu_rm_make_reservation(
>>>  static int _dpu_rm_populate_requirements(
>>>  		struct dpu_rm *rm,
>>>  		struct drm_encoder *enc,
>>> -		struct drm_crtc_state *crtc_state,
>>>  		struct dpu_rm_requirements *reqs,
>>>  		struct msm_display_topology req_topology)
>>>  {
>>> @@ -642,6 +658,7 @@ int dpu_rm_reserve(
>>>  		bool test_only)
>>>  {
>>>  	struct dpu_rm_requirements reqs;
>>> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>>>  	int ret;
>>> 
>>>  	/* Check if this is just a page-flip */
>>> @@ -653,14 +670,13 @@ int dpu_rm_reserve(
>>> 
>>>  	mutex_lock(&rm->rm_lock);
>>> 
>>> -	ret = _dpu_rm_populate_requirements(rm, enc, crtc_state, &reqs,
>>> -					    topology);
>>> +	ret = _dpu_rm_populate_requirements(rm, enc, &reqs, topology);
>>>  	if (ret) {
>>>  		DPU_ERROR("failed to populate hw requirements\n");
>>>  		goto end;
>>>  	}
>>> 
>>> -	ret = _dpu_rm_make_reservation(rm, enc, crtc_state, &reqs);
>>> +	ret = _dpu_rm_make_reservation(rm, enc, dpu_cstate, &reqs);
>>>  	if (ret) {
>>>  		DPU_ERROR("failed to reserve hw resources: %d\n", ret);
>>>  		_dpu_rm_release_reservation(rm, enc->base.id);
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>>> a Linux Foundation Collaborative Project
>>>