[22/25] drm/msm/dpu: make crtc and encoder specific HW reservation

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

Details

Message ID 1539059262-8326-23-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.
Instead of letting encoder make a centralized reservation for
all of its display DRM components, this path splits the
responsibility between CRTC and Encoder, each requesting
RM for the HW mapping of its own domain.

Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 31 +++++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69 ++++++++++++++++++++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 36 +++++++++++----
 4 files changed, 119 insertions(+), 31 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 0625f56..0536b8a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -47,6 +47,8 @@ 
 #define LEFT_MIXER 0
 #define RIGHT_MIXER 1
 
+#define MAX_VDISPLAY_SPLIT 1080
+
 static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
 					    struct drm_display_mode *mode)
 {
@@ -448,6 +450,7 @@  static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
 
 	for (i = 0; i < cstate->num_mixers; i++) {
 		struct drm_rect *r = &cstate->lm_bounds[i];
+
 		r->x1 = crtc_split_width * i;
 		r->y1 = 0;
 		r->x2 = r->x1 + crtc_split_width;
@@ -885,6 +888,7 @@  static void dpu_crtc_disable(struct drm_crtc *crtc)
 	struct drm_display_mode *mode;
 	struct drm_encoder *encoder;
 	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
 	unsigned long flags;
 
 	if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
@@ -895,6 +899,7 @@  static void dpu_crtc_disable(struct drm_crtc *crtc)
 	cstate = to_dpu_crtc_state(crtc->state);
 	mode = &cstate->base.adjusted_mode;
 	priv = crtc->dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
 
 	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
 
@@ -953,6 +958,8 @@  static void dpu_crtc_disable(struct drm_crtc *crtc)
 		crtc->state->event = NULL;
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 	}
+
+	dpu_rm_crtc_release(&dpu_kms->rm, crtc->state);
 }
 
 static void dpu_crtc_enable(struct drm_crtc *crtc,
@@ -1004,6 +1011,21 @@  struct plane_state {
 	u32 pipe_id;
 };
 
+static void _dpu_crtc_get_topology(
+			struct drm_crtc_state *crtc_state,
+			struct drm_display_mode *mode)
+{
+	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
+
+	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
+
+	/**
+	 * encoder->atomic_check is invoked before crtc->atomic_check.
+	 * so dpu_cstate->num_intfs should have a non-zero value.
+	 */
+	dpu_cstate->num_ctls = dpu_cstate->num_intfs;
+}
+
 static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 		struct drm_crtc_state *state)
 {
@@ -1014,6 +1036,8 @@  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 	const struct drm_plane_state *pstate;
 	struct drm_plane *plane;
 	struct drm_display_mode *mode;
+	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms;
 
 	int cnt = 0, rc = 0, mixer_width, i, z_pos;
 
@@ -1039,6 +1063,9 @@  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 		goto end;
 	}
 
+	priv = crtc->dev->dev_private;
+	dpu_kms = to_dpu_kms(priv->kms);
+
 	mode = &state->adjusted_mode;
 	DPU_DEBUG("%s: check", dpu_crtc->name);
 
@@ -1229,6 +1256,10 @@  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	_dpu_crtc_get_topology(state, mode);
+	if (drm_atomic_crtc_needs_modeset(state))
+		rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state);
+
 end:
 	kfree(pstates);
 	return rc;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 5d501c8..ce66309 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -67,8 +67,6 @@ 
 
 #define IDLE_SHORT_TIMEOUT	1
 
-#define MAX_VDISPLAY_SPLIT 1080
-
 /**
  * enum dpu_enc_rc_events - events for resource control state machine
  * @DPU_ENC_RC_EVENT_KICKOFF:
@@ -557,14 +555,10 @@  static void _dpu_encoder_adjust_mode(struct drm_connector *connector,
 
 static void _dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
-			struct drm_crtc_state *crtc_state,
-			struct drm_display_mode *mode)
+			struct drm_crtc_state *crtc_state)
 {
 	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
 
-	/* User split topology for width > 1080 */
-	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
-	dpu_cstate->num_ctls = dpu_enc->num_phys_encs;
 	dpu_cstate->num_intfs = dpu_enc->num_phys_encs;
 }
 
@@ -623,9 +617,9 @@  static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	_dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode);
+	_dpu_encoder_get_topology(dpu_enc, crtc_state);
 	if (!ret && drm_atomic_crtc_needs_modeset(crtc_state))
-		ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state);
+		ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, crtc_state);
 
 	if (!ret)
 		drm_mode_set_crtcinfo(adj_mode, 0);
@@ -1186,7 +1180,7 @@  static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 
 	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
 
-	dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state);
+	dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state);
 }
 
 static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 5304597..901b1fc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -436,8 +436,8 @@  static int _dpu_rm_release_hw(struct dpu_rm *rm, enum dpu_hw_blk_type type,
 	return -EINVAL;
 }
 
-static void _dpu_rm_release_reservation(struct dpu_rm *rm,
-					struct dpu_crtc_state *dpu_cstate)
+static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm,
+					     struct dpu_crtc_state *dpu_cstate)
 {
 	int i;
 
@@ -464,6 +464,12 @@  static void _dpu_rm_release_reservation(struct dpu_rm *rm,
 					dpu_cstate->hw_ctls[i]->base.id))
 			dpu_cstate->hw_ctls[i] = NULL;
 	}
+}
+
+static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm,
+					      struct dpu_crtc_state *dpu_cstate)
+{
+	int i;
 
 	for (i = 0; i < dpu_cstate->num_intfs; i++) {
 		if (!dpu_cstate->hw_intfs[i])
@@ -475,23 +481,33 @@  static void _dpu_rm_release_reservation(struct dpu_rm *rm,
 	}
 }
 
-void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state)
+void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state)
 {
 	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
 
 	mutex_lock(&rm->rm_lock);
 
-	_dpu_rm_release_reservation(rm, dpu_cstate);
+	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
 
 	mutex_unlock(&rm->rm_lock);
 }
 
-int dpu_rm_reserve(
+void dpu_rm_encoder_release(struct dpu_rm *rm,
+			    struct drm_crtc_state *crtc_state)
+{
+	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
+
+	mutex_lock(&rm->rm_lock);
+
+	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
+
+	mutex_unlock(&rm->rm_lock);
+}
+
+int dpu_rm_crtc_reserve(
 		struct dpu_rm *rm,
-		struct drm_encoder *enc,
 		struct drm_crtc_state *crtc_state)
 {
-	struct dpu_encoder_hw_resources hw_res;
 	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
 	int ret;
 
@@ -499,12 +515,10 @@  int dpu_rm_reserve(
 	if (!drm_atomic_crtc_needs_modeset(crtc_state))
 		return 0;
 
-	DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
-		      enc->base.id, crtc_state->crtc->base.id);
 
-	mutex_lock(&rm->rm_lock);
+	DRM_DEBUG_KMS("reserving hw for crtc %d\n", crtc_state->crtc->base.id);
 
-	dpu_encoder_get_hw_resources(enc, &hw_res);
+	mutex_lock(&rm->rm_lock);
 
 	ret = _dpu_rm_reserve_lms(rm, dpu_cstate);
 	if (ret) {
@@ -518,6 +532,37 @@  int dpu_rm_reserve(
 		goto cleanup_on_fail;
 	}
 
+	mutex_unlock(&rm->rm_lock);
+
+	return ret;
+
+cleanup_on_fail:
+	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
+
+	mutex_unlock(&rm->rm_lock);
+
+	return ret;
+}
+
+int dpu_rm_encoder_reserve(
+		struct dpu_rm *rm,
+		struct drm_encoder *enc,
+		struct drm_crtc_state *crtc_state)
+{
+	struct dpu_encoder_hw_resources hw_res;
+	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
+	int ret;
+
+	/* Check if this is just a page-flip */
+	if (!drm_atomic_crtc_needs_modeset(crtc_state))
+		return 0;
+
+	DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id);
+
+	mutex_lock(&rm->rm_lock);
+
+	dpu_encoder_get_hw_resources(enc, &hw_res);
+
 	ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res);
 	if (ret) {
 		DPU_ERROR("unable to find appropriate INTF\n");
@@ -529,7 +574,7 @@  int dpu_rm_reserve(
 	return ret;
 
 cleanup_on_fail:
-	_dpu_rm_release_reservation(rm, dpu_cstate);
+	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
 
 	mutex_unlock(&rm->rm_lock);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index 1626cef..0b1deb0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -53,27 +53,45 @@  int dpu_rm_init(struct dpu_rm *rm,
 int dpu_rm_destroy(struct dpu_rm *rm);
 
 /**
- * dpu_rm_reserve - Given a CRTC->Encoder->Connector display chain, analyze
- *	the use connections and user requirements, specified through related
+ * dpu_rm_encoder_reserve - Given an Encoder, analyze the use connections
+ *      and user requirements, specified through related
  *	topology control properties, and reserve hardware blocks to that
  *	display chain.
- *	HW blocks can then be accessed through dpu_rm_get_* functions.
- *	HW Reservations should be released via dpu_rm_release_hw.
  * @rm: DPU Resource Manager handle
  * @drm_enc: DRM Encoder handle
  * @crtc_state: Proposed Atomic DRM CRTC State handle
  * @Return: 0 on Success otherwise -ERROR
  */
-int dpu_rm_reserve(struct dpu_rm *rm,
+int dpu_rm_encoder_reserve(struct dpu_rm *rm,
 		struct drm_encoder *drm_enc,
 		struct drm_crtc_state *crtc_state);
 
 /**
- * dpu_rm_release - Given the encoder for the display chain, release any
- *	HW blocks previously reserved for that use case.
+ * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections
+ *      and user requirements, specified through related
+ *	topology control properties, and reserve hardware blocks to that
+ *	display chain.
  * @rm: DPU Resource Manager handle
- * @crtc_state: atomic DRM state handle
+ * @crtc_state: Proposed Atomic DRM CRTC State handle
  * @Return: 0 on Success otherwise -ERROR
  */
-void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state);
+int dpu_rm_crtc_reserve(struct dpu_rm *rm,
+		struct drm_crtc_state *crtc_state);
+
+/**
+ * dpu_rm_encoder_release - Given the encoder, release any
+ *	HW blocks previously reserved for that use case.
+ * @rm: DPU Resource Manager handle
+ * @crtc_state: Proposed Atomic DRM CRTC State handle
+ */
+void dpu_rm_encoder_release(struct dpu_rm *rm,
+			    struct drm_crtc_state *crtc_state);
+
+/**
+ * dpu_rm_crtc_release - Given the crtc, release any
+ *	HW blocks previously reserved for that use case.
+ * @rm: DPU Resource Manager handle
+ * @crtc_state: Proposed Atomic DRM CRTC State handle
+ */
+void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state);
 #endif /* __DPU_RM_H__ */

Comments

On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote:
> Instead of letting encoder make a centralized reservation for
> all of its display DRM components, this path splits the
> responsibility between CRTC and Encoder, each requesting
> RM for the HW mapping of its own domain.
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 31 +++++++++++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69 ++++++++++++++++++++++++-----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 36 +++++++++++----
>  4 files changed, 119 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 0625f56..0536b8a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -47,6 +47,8 @@
>  #define LEFT_MIXER 0
>  #define RIGHT_MIXER 1
>  
> +#define MAX_VDISPLAY_SPLIT 1080
> +
>  static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate,
>  					    struct drm_display_mode *mode)
>  {
> @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc,
>  
>  	for (i = 0; i < cstate->num_mixers; i++) {
>  		struct drm_rect *r = &cstate->lm_bounds[i];
> +
>  		r->x1 = crtc_split_width * i;
>  		r->y1 = 0;
>  		r->x2 = r->x1 + crtc_split_width;
> @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  	struct drm_display_mode *mode;
>  	struct drm_encoder *encoder;
>  	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
>  	unsigned long flags;
>  
>  	if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) {
> @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  	cstate = to_dpu_crtc_state(crtc->state);
>  	mode = &cstate->base.adjusted_mode;
>  	priv = crtc->dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
>  
>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>  
> @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc)
>  		crtc->state->event = NULL;
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  	}
> +
> +	dpu_rm_crtc_release(&dpu_kms->rm, crtc->state);
>  }
>  
>  static void dpu_crtc_enable(struct drm_crtc *crtc,
> @@ -1004,6 +1011,21 @@ struct plane_state {
>  	u32 pipe_id;
>  };
>  
> +static void _dpu_crtc_get_topology(
> +			struct drm_crtc_state *crtc_state,
> +			struct drm_display_mode *mode)
> +{
> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> +
> +	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
> +
> +	/**
> +	 * encoder->atomic_check is invoked before crtc->atomic_check.
> +	 * so dpu_cstate->num_intfs should have a non-zero value.
> +	 */
> +	dpu_cstate->num_ctls = dpu_cstate->num_intfs;

Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs directly? Also,
you don't really need these in their own function, especially if num_ctls goes
away.

> +}
> +
>  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  		struct drm_crtc_state *state)
>  {
> @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  	const struct drm_plane_state *pstate;
>  	struct drm_plane *plane;
>  	struct drm_display_mode *mode;
> +	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms;
>  
>  	int cnt = 0, rc = 0, mixer_width, i, z_pos;
>  
> @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  		goto end;
>  	}
>  
> +	priv = crtc->dev->dev_private;
> +	dpu_kms = to_dpu_kms(priv->kms);
> +
>  	mode = &state->adjusted_mode;
>  	DPU_DEBUG("%s: check", dpu_crtc->name);
>  
> @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  		}
>  	}
>  
> +	_dpu_crtc_get_topology(state, mode);
> +	if (drm_atomic_crtc_needs_modeset(state))
> +		rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state);
> +
>  end:
>  	kfree(pstates);
>  	return rc;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 5d501c8..ce66309 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -67,8 +67,6 @@
>  
>  #define IDLE_SHORT_TIMEOUT	1
>  
> -#define MAX_VDISPLAY_SPLIT 1080
> -
>  /**
>   * enum dpu_enc_rc_events - events for resource control state machine
>   * @DPU_ENC_RC_EVENT_KICKOFF:
> @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector,
>  
>  static void _dpu_encoder_get_topology(
>  			struct dpu_encoder_virt *dpu_enc,
> -			struct drm_crtc_state *crtc_state,
> -			struct drm_display_mode *mode)
> +			struct drm_crtc_state *crtc_state)
>  {
>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>  
> -	/* User split topology for width > 1080 */
> -	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1;
> -	dpu_cstate->num_ctls = dpu_enc->num_phys_encs;
>  	dpu_cstate->num_intfs = dpu_enc->num_phys_encs;
>  }
>  
> @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check(
>  		}
>  	}
>  
> -	_dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode);
> +	_dpu_encoder_get_topology(dpu_enc, crtc_state);
>  	if (!ret && drm_atomic_crtc_needs_modeset(crtc_state))
> -		ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state);
> +		ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, crtc_state);
>  
>  	if (!ret)
>  		drm_mode_set_crtcinfo(adj_mode, 0);
> @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>  
>  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>  
> -	dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state);
> +	dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state);
>  }
>  
>  static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 5304597..901b1fc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm, enum dpu_hw_blk_type type,
>  	return -EINVAL;
>  }
>  
> -static void _dpu_rm_release_reservation(struct dpu_rm *rm,
> -					struct dpu_crtc_state *dpu_cstate)
> +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm,
> +					     struct dpu_crtc_state *dpu_cstate)
>  {
>  	int i;
>  
> @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm,
>  					dpu_cstate->hw_ctls[i]->base.id))
>  			dpu_cstate->hw_ctls[i] = NULL;
>  	}
> +}
> +
> +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm,
> +					      struct dpu_crtc_state *dpu_cstate)
> +{
> +	int i;
>  
>  	for (i = 0; i < dpu_cstate->num_intfs; i++) {
>  		if (!dpu_cstate->hw_intfs[i])
> @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm,
>  	}
>  }
>  
> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state)
> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state)
>  {
>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>  
>  	mutex_lock(&rm->rm_lock);
>  
> -	_dpu_rm_release_reservation(rm, dpu_cstate);
> +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
>  
>  	mutex_unlock(&rm->rm_lock);
>  }
>  
> -int dpu_rm_reserve(
> +void dpu_rm_encoder_release(struct dpu_rm *rm,
> +			    struct drm_crtc_state *crtc_state)
> +{
> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> +
> +	mutex_lock(&rm->rm_lock);
> +
> +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
> +
> +	mutex_unlock(&rm->rm_lock);
> +}
> +
> +int dpu_rm_crtc_reserve(
>  		struct dpu_rm *rm,
> -		struct drm_encoder *enc,
>  		struct drm_crtc_state *crtc_state)
>  {
> -	struct dpu_encoder_hw_resources hw_res;
>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>  	int ret;
>  
> @@ -499,12 +515,10 @@ int dpu_rm_reserve(
>  	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>  		return 0;
>  
> -	DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
> -		      enc->base.id, crtc_state->crtc->base.id);
>  
> -	mutex_lock(&rm->rm_lock);
> +	DRM_DEBUG_KMS("reserving hw for crtc %d\n", crtc_state->crtc->base.id);
>  
> -	dpu_encoder_get_hw_resources(enc, &hw_res);
> +	mutex_lock(&rm->rm_lock);
>  
>  	ret = _dpu_rm_reserve_lms(rm, dpu_cstate);
>  	if (ret) {
> @@ -518,6 +532,37 @@ int dpu_rm_reserve(
>  		goto cleanup_on_fail;
>  	}
>  
> +	mutex_unlock(&rm->rm_lock);
> +
> +	return ret;
> +
> +cleanup_on_fail:
> +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
> +
> +	mutex_unlock(&rm->rm_lock);
> +
> +	return ret;
> +}
> +
> +int dpu_rm_encoder_reserve(
> +		struct dpu_rm *rm,
> +		struct drm_encoder *enc,
> +		struct drm_crtc_state *crtc_state)
> +{
> +	struct dpu_encoder_hw_resources hw_res;
> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> +	int ret;
> +
> +	/* Check if this is just a page-flip */
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state))
> +		return 0;
> +
> +	DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id);
> +
> +	mutex_lock(&rm->rm_lock);
> +
> +	dpu_encoder_get_hw_resources(enc, &hw_res);
> +
>  	ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res);
>  	if (ret) {
>  		DPU_ERROR("unable to find appropriate INTF\n");
> @@ -529,7 +574,7 @@ int dpu_rm_reserve(
>  	return ret;
>  
>  cleanup_on_fail:
> -	_dpu_rm_release_reservation(rm, dpu_cstate);
> +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
>  
>  	mutex_unlock(&rm->rm_lock);
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index 1626cef..0b1deb0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -53,27 +53,45 @@ int dpu_rm_init(struct dpu_rm *rm,
>  int dpu_rm_destroy(struct dpu_rm *rm);
>  
>  /**
> - * dpu_rm_reserve - Given a CRTC->Encoder->Connector display chain, analyze
> - *	the use connections and user requirements, specified through related
> + * dpu_rm_encoder_reserve - Given an Encoder, analyze the use connections
> + *      and user requirements, specified through related
>   *	topology control properties, and reserve hardware blocks to that
>   *	display chain.
> - *	HW blocks can then be accessed through dpu_rm_get_* functions.
> - *	HW Reservations should be released via dpu_rm_release_hw.
>   * @rm: DPU Resource Manager handle
>   * @drm_enc: DRM Encoder handle
>   * @crtc_state: Proposed Atomic DRM CRTC State handle
>   * @Return: 0 on Success otherwise -ERROR
>   */
> -int dpu_rm_reserve(struct dpu_rm *rm,
> +int dpu_rm_encoder_reserve(struct dpu_rm *rm,
>  		struct drm_encoder *drm_enc,
>  		struct drm_crtc_state *crtc_state);
>  
>  /**
> - * dpu_rm_release - Given the encoder for the display chain, release any
> - *	HW blocks previously reserved for that use case.
> + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections
> + *      and user requirements, specified through related
> + *	topology control properties, and reserve hardware blocks to that
> + *	display chain.
>   * @rm: DPU Resource Manager handle
> - * @crtc_state: atomic DRM state handle
> + * @crtc_state: Proposed Atomic DRM CRTC State handle
>   * @Return: 0 on Success otherwise -ERROR
>   */
> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state);
> +int dpu_rm_crtc_reserve(struct dpu_rm *rm,
> +		struct drm_crtc_state *crtc_state);
> +
> +/**
> + * dpu_rm_encoder_release - Given the encoder, release any
> + *	HW blocks previously reserved for that use case.
> + * @rm: DPU Resource Manager handle
> + * @crtc_state: Proposed Atomic DRM CRTC State handle
> + */
> +void dpu_rm_encoder_release(struct dpu_rm *rm,
> +			    struct drm_crtc_state *crtc_state);
> +
> +/**
> + * dpu_rm_crtc_release - Given the crtc, release any
> + *	HW blocks previously reserved for that use case.
> + * @rm: DPU Resource Manager handle
> + * @crtc_state: Proposed Atomic DRM CRTC State handle
> + */
> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state);
>  #endif /* __DPU_RM_H__ */
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
On 2018-10-09 13:41, Sean Paul wrote:
> On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote:
>> Instead of letting encoder make a centralized reservation for
>> all of its display DRM components, this path splits the
>> responsibility between CRTC and Encoder, each requesting
>> RM for the HW mapping of its own domain.
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 31 +++++++++++++
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++----
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69
> ++++++++++++++++++++++++-----
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 36 +++++++++++----
>>  4 files changed, 119 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 0625f56..0536b8a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -47,6 +47,8 @@
>>  #define LEFT_MIXER 0
>>  #define RIGHT_MIXER 1
>> 
>> +#define MAX_VDISPLAY_SPLIT 1080
>> +
>>  static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state
> *cstate,
>>  					    struct drm_display_mode *mode)
>>  {
>> @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
> drm_crtc *crtc,
>> 
>>  	for (i = 0; i < cstate->num_mixers; i++) {
>>  		struct drm_rect *r = &cstate->lm_bounds[i];
>> +
>>  		r->x1 = crtc_split_width * i;
>>  		r->y1 = 0;
>>  		r->x2 = r->x1 + crtc_split_width;
>> @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc 
>> *crtc)
>>  	struct drm_display_mode *mode;
>>  	struct drm_encoder *encoder;
>>  	struct msm_drm_private *priv;
>> +	struct dpu_kms *dpu_kms;
>>  	unsigned long flags;
>> 
>>  	if (!crtc || !crtc->dev || !crtc->dev->dev_private ||
> !crtc->state) {
>> @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc 
>> *crtc)
>>  	cstate = to_dpu_crtc_state(crtc->state);
>>  	mode = &cstate->base.adjusted_mode;
>>  	priv = crtc->dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> 
>>  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>> 
>> @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc 
>> *crtc)
>>  		crtc->state->event = NULL;
>>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>>  	}
>> +
>> +	dpu_rm_crtc_release(&dpu_kms->rm, crtc->state);
>>  }
>> 
>>  static void dpu_crtc_enable(struct drm_crtc *crtc,
>> @@ -1004,6 +1011,21 @@ struct plane_state {
>>  	u32 pipe_id;
>>  };
>> 
>> +static void _dpu_crtc_get_topology(
>> +			struct drm_crtc_state *crtc_state,
>> +			struct drm_display_mode *mode)
>> +{
>> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>> +
>> +	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
> : 1;
>> +
>> +	/**
>> +	 * encoder->atomic_check is invoked before crtc->atomic_check.
>> +	 * so dpu_cstate->num_intfs should have a non-zero value.
>> +	 */
>> +	dpu_cstate->num_ctls = dpu_cstate->num_intfs;
> 
> Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs 
> directly?
> Also,
> you don't really need these in their own function, especially if 
> num_ctls
> goes
> away.
> 
Yes. I can live with just that. But since dpu_cstate maintains HW arrays
for each type, I thought it would be more readable if I could use
separate variables to track their counts instead of iterating over
ctl arrays over dpu_cstate->num_intfs and leaving comments that both
will be same for this version of hardware.

Also, the counts need not be the same for all the Snapdragon variants.

Thanks,
Jeykumar S.
>> +}
>> +
>>  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>>  		struct drm_crtc_state *state)
>>  {
>> @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> *crtc,
>>  	const struct drm_plane_state *pstate;
>>  	struct drm_plane *plane;
>>  	struct drm_display_mode *mode;
>> +	struct msm_drm_private *priv;
>> +	struct dpu_kms *dpu_kms;
>> 
>>  	int cnt = 0, rc = 0, mixer_width, i, z_pos;
>> 
>> @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> *crtc,
>>  		goto end;
>>  	}
>> 
>> +	priv = crtc->dev->dev_private;
>> +	dpu_kms = to_dpu_kms(priv->kms);
>> +
>>  	mode = &state->adjusted_mode;
>>  	DPU_DEBUG("%s: check", dpu_crtc->name);
>> 
>> @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct 
>> drm_crtc
> *crtc,
>>  		}
>>  	}
>> 
>> +	_dpu_crtc_get_topology(state, mode);
>> +	if (drm_atomic_crtc_needs_modeset(state))
>> +		rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state);
>> +
>>  end:
>>  	kfree(pstates);
>>  	return rc;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 5d501c8..ce66309 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -67,8 +67,6 @@
>> 
>>  #define IDLE_SHORT_TIMEOUT	1
>> 
>> -#define MAX_VDISPLAY_SPLIT 1080
>> -
>>  /**
>>   * enum dpu_enc_rc_events - events for resource control state machine
>>   * @DPU_ENC_RC_EVENT_KICKOFF:
>> @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct
> drm_connector *connector,
>> 
>>  static void _dpu_encoder_get_topology(
>>  			struct dpu_encoder_virt *dpu_enc,
>> -			struct drm_crtc_state *crtc_state,
>> -			struct drm_display_mode *mode)
>> +			struct drm_crtc_state *crtc_state)
>>  {
>>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>> 
>> -	/* User split topology for width > 1080 */
>> -	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
> : 1;
>> -	dpu_cstate->num_ctls = dpu_enc->num_phys_encs;
>>  	dpu_cstate->num_intfs = dpu_enc->num_phys_encs;
>>  }
>> 
>> @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check(
>>  		}
>>  	}
>> 
>> -	_dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode);
>> +	_dpu_encoder_get_topology(dpu_enc, crtc_state);
>>  	if (!ret && drm_atomic_crtc_needs_modeset(crtc_state))
>> -		ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state);
>> +		ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc,
> crtc_state);
>> 
>>  	if (!ret)
>>  		drm_mode_set_crtcinfo(adj_mode, 0);
>> @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>> 
>>  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>> 
>> -	dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state);
>> +	dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state);
>>  }
>> 
>>  static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg 
>> *catalog,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 5304597..901b1fc 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm,
> enum dpu_hw_blk_type type,
>>  	return -EINVAL;
>>  }
>> 
>> -static void _dpu_rm_release_reservation(struct dpu_rm *rm,
>> -					struct dpu_crtc_state *dpu_cstate)
>> +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm,
>> +					     struct dpu_crtc_state
> *dpu_cstate)
>>  {
>>  	int i;
>> 
>> @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct
> dpu_rm *rm,
>>  					dpu_cstate->hw_ctls[i]->base.id))
>>  			dpu_cstate->hw_ctls[i] = NULL;
>>  	}
>> +}
>> +
>> +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm,
>> +					      struct dpu_crtc_state
> *dpu_cstate)
>> +{
>> +	int i;
>> 
>>  	for (i = 0; i < dpu_cstate->num_intfs; i++) {
>>  		if (!dpu_cstate->hw_intfs[i])
>> @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct
> dpu_rm *rm,
>>  	}
>>  }
>> 
>> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state
> *crtc_state)
>> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state
> *crtc_state)
>>  {
>>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>> 
>>  	mutex_lock(&rm->rm_lock);
>> 
>> -	_dpu_rm_release_reservation(rm, dpu_cstate);
>> +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
>> 
>>  	mutex_unlock(&rm->rm_lock);
>>  }
>> 
>> -int dpu_rm_reserve(
>> +void dpu_rm_encoder_release(struct dpu_rm *rm,
>> +			    struct drm_crtc_state *crtc_state)
>> +{
>> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>> +
>> +	mutex_lock(&rm->rm_lock);
>> +
>> +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
>> +
>> +	mutex_unlock(&rm->rm_lock);
>> +}
>> +
>> +int dpu_rm_crtc_reserve(
>>  		struct dpu_rm *rm,
>> -		struct drm_encoder *enc,
>>  		struct drm_crtc_state *crtc_state)
>>  {
>> -	struct dpu_encoder_hw_resources hw_res;
>>  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>>  	int ret;
>> 
>> @@ -499,12 +515,10 @@ int dpu_rm_reserve(
>>  	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>>  		return 0;
>> 
>> -	DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
>> -		      enc->base.id, crtc_state->crtc->base.id);
>> 
>> -	mutex_lock(&rm->rm_lock);
>> +	DRM_DEBUG_KMS("reserving hw for crtc %d\n",
> crtc_state->crtc->base.id);
>> 
>> -	dpu_encoder_get_hw_resources(enc, &hw_res);
>> +	mutex_lock(&rm->rm_lock);
>> 
>>  	ret = _dpu_rm_reserve_lms(rm, dpu_cstate);
>>  	if (ret) {
>> @@ -518,6 +532,37 @@ int dpu_rm_reserve(
>>  		goto cleanup_on_fail;
>>  	}
>> 
>> +	mutex_unlock(&rm->rm_lock);
>> +
>> +	return ret;
>> +
>> +cleanup_on_fail:
>> +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
>> +
>> +	mutex_unlock(&rm->rm_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +int dpu_rm_encoder_reserve(
>> +		struct dpu_rm *rm,
>> +		struct drm_encoder *enc,
>> +		struct drm_crtc_state *crtc_state)
>> +{
>> +	struct dpu_encoder_hw_resources hw_res;
>> +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
>> +	int ret;
>> +
>> +	/* Check if this is just a page-flip */
>> +	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>> +		return 0;
>> +
>> +	DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id);
>> +
>> +	mutex_lock(&rm->rm_lock);
>> +
>> +	dpu_encoder_get_hw_resources(enc, &hw_res);
>> +
>>  	ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res);
>>  	if (ret) {
>>  		DPU_ERROR("unable to find appropriate INTF\n");
>> @@ -529,7 +574,7 @@ int dpu_rm_reserve(
>>  	return ret;
>> 
>>  cleanup_on_fail:
>> -	_dpu_rm_release_reservation(rm, dpu_cstate);
>> +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
>> 
>>  	mutex_unlock(&rm->rm_lock);
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> index 1626cef..0b1deb0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> @@ -53,27 +53,45 @@ int dpu_rm_init(struct dpu_rm *rm,
>>  int dpu_rm_destroy(struct dpu_rm *rm);
>> 
>>  /**
>> - * dpu_rm_reserve - Given a CRTC->Encoder->Connector display chain,
> analyze
>> - *	the use connections and user requirements, specified through
> related
>> + * dpu_rm_encoder_reserve - Given an Encoder, analyze the use
> connections
>> + *      and user requirements, specified through related
>>   *	topology control properties, and reserve hardware blocks to that
>>   *	display chain.
>> - *	HW blocks can then be accessed through dpu_rm_get_* functions.
>> - *	HW Reservations should be released via dpu_rm_release_hw.
>>   * @rm: DPU Resource Manager handle
>>   * @drm_enc: DRM Encoder handle
>>   * @crtc_state: Proposed Atomic DRM CRTC State handle
>>   * @Return: 0 on Success otherwise -ERROR
>>   */
>> -int dpu_rm_reserve(struct dpu_rm *rm,
>> +int dpu_rm_encoder_reserve(struct dpu_rm *rm,
>>  		struct drm_encoder *drm_enc,
>>  		struct drm_crtc_state *crtc_state);
>> 
>>  /**
>> - * dpu_rm_release - Given the encoder for the display chain, release
> any
>> - *	HW blocks previously reserved for that use case.
>> + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections
>> + *      and user requirements, specified through related
>> + *	topology control properties, and reserve hardware blocks to that
>> + *	display chain.
>>   * @rm: DPU Resource Manager handle
>> - * @crtc_state: atomic DRM state handle
>> + * @crtc_state: Proposed Atomic DRM CRTC State handle
>>   * @Return: 0 on Success otherwise -ERROR
>>   */
>> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state
> *crtc_state);
>> +int dpu_rm_crtc_reserve(struct dpu_rm *rm,
>> +		struct drm_crtc_state *crtc_state);
>> +
>> +/**
>> + * dpu_rm_encoder_release - Given the encoder, release any
>> + *	HW blocks previously reserved for that use case.
>> + * @rm: DPU Resource Manager handle
>> + * @crtc_state: Proposed Atomic DRM CRTC State handle
>> + */
>> +void dpu_rm_encoder_release(struct dpu_rm *rm,
>> +			    struct drm_crtc_state *crtc_state);
>> +
>> +/**
>> + * dpu_rm_crtc_release - Given the crtc, release any
>> + *	HW blocks previously reserved for that use case.
>> + * @rm: DPU Resource Manager handle
>> + * @crtc_state: Proposed Atomic DRM CRTC State handle
>> + */
>> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state
> *crtc_state);
>>  #endif /* __DPU_RM_H__ */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
On Tue, Oct 09, 2018 at 11:15:02PM -0700, Jeykumar Sankaran wrote:
> On 2018-10-09 13:41, Sean Paul wrote:
> > On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote:
> > > Instead of letting encoder make a centralized reservation for
> > > all of its display DRM components, this path splits the
> > > responsibility between CRTC and Encoder, each requesting
> > > RM for the HW mapping of its own domain.
> > > 
> > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 31 +++++++++++++
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++----
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69
> > ++++++++++++++++++++++++-----
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 36 +++++++++++----
> > >  4 files changed, 119 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > index 0625f56..0536b8a 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > > @@ -47,6 +47,8 @@
> > >  #define LEFT_MIXER 0
> > >  #define RIGHT_MIXER 1
> > > 
> > > +#define MAX_VDISPLAY_SPLIT 1080
> > > +
> > >  static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state
> > *cstate,
> > >  					    struct drm_display_mode *mode)
> > >  {
> > > @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct
> > drm_crtc *crtc,
> > > 
> > >  	for (i = 0; i < cstate->num_mixers; i++) {
> > >  		struct drm_rect *r = &cstate->lm_bounds[i];
> > > +
> > >  		r->x1 = crtc_split_width * i;
> > >  		r->y1 = 0;
> > >  		r->x2 = r->x1 + crtc_split_width;
> > > @@ -885,6 +888,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > >  	struct drm_display_mode *mode;
> > >  	struct drm_encoder *encoder;
> > >  	struct msm_drm_private *priv;
> > > +	struct dpu_kms *dpu_kms;
> > >  	unsigned long flags;
> > > 
> > >  	if (!crtc || !crtc->dev || !crtc->dev->dev_private ||
> > !crtc->state) {
> > > @@ -895,6 +899,7 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > >  	cstate = to_dpu_crtc_state(crtc->state);
> > >  	mode = &cstate->base.adjusted_mode;
> > >  	priv = crtc->dev->dev_private;
> > > +	dpu_kms = to_dpu_kms(priv->kms);
> > > 
> > >  	DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> > > 
> > > @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc
> > > *crtc)
> > >  		crtc->state->event = NULL;
> > >  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > >  	}
> > > +
> > > +	dpu_rm_crtc_release(&dpu_kms->rm, crtc->state);
> > >  }
> > > 
> > >  static void dpu_crtc_enable(struct drm_crtc *crtc,
> > > @@ -1004,6 +1011,21 @@ struct plane_state {
> > >  	u32 pipe_id;
> > >  };
> > > 
> > > +static void _dpu_crtc_get_topology(
> > > +			struct drm_crtc_state *crtc_state,
> > > +			struct drm_display_mode *mode)
> > > +{
> > > +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > +
> > > +	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
> > : 1;
> > > +
> > > +	/**
> > > +	 * encoder->atomic_check is invoked before crtc->atomic_check.
> > > +	 * so dpu_cstate->num_intfs should have a non-zero value.
> > > +	 */
> > > +	dpu_cstate->num_ctls = dpu_cstate->num_intfs;
> > 
> > Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs
> > directly?
> > Also,
> > you don't really need these in their own function, especially if
> > num_ctls
> > goes
> > away.
> > 
> Yes. I can live with just that. But since dpu_cstate maintains HW arrays
> for each type, I thought it would be more readable if I could use
> separate variables to track their counts instead of iterating over
> ctl arrays over dpu_cstate->num_intfs and leaving comments that both
> will be same for this version of hardware.

You could change the name to make it more generic. AFAICT,

num_h_tiles == num_phys_encs == num_intfs == num_ctls

So storing it as num_h_tiles might make more sense.

> 
> Also, the counts need not be the same for all the Snapdragon variants.

This is probably a good thing. It doesn't seem like the current driver would
work if these values were different, so making it explicit is a good signal that
more invasive changes are needed.

Sean

> 
> Thanks,
> Jeykumar S.
> > > +}
> > > +
> > >  static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> > >  		struct drm_crtc_state *state)
> > >  {
> > > @@ -1014,6 +1036,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > >  	const struct drm_plane_state *pstate;
> > >  	struct drm_plane *plane;
> > >  	struct drm_display_mode *mode;
> > > +	struct msm_drm_private *priv;
> > > +	struct dpu_kms *dpu_kms;
> > > 
> > >  	int cnt = 0, rc = 0, mixer_width, i, z_pos;
> > > 
> > > @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > >  		goto end;
> > >  	}
> > > 
> > > +	priv = crtc->dev->dev_private;
> > > +	dpu_kms = to_dpu_kms(priv->kms);
> > > +
> > >  	mode = &state->adjusted_mode;
> > >  	DPU_DEBUG("%s: check", dpu_crtc->name);
> > > 
> > > @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct
> > > drm_crtc
> > *crtc,
> > >  		}
> > >  	}
> > > 
> > > +	_dpu_crtc_get_topology(state, mode);
> > > +	if (drm_atomic_crtc_needs_modeset(state))
> > > +		rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state);
> > > +
> > >  end:
> > >  	kfree(pstates);
> > >  	return rc;
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 5d501c8..ce66309 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -67,8 +67,6 @@
> > > 
> > >  #define IDLE_SHORT_TIMEOUT	1
> > > 
> > > -#define MAX_VDISPLAY_SPLIT 1080
> > > -
> > >  /**
> > >   * enum dpu_enc_rc_events - events for resource control state machine
> > >   * @DPU_ENC_RC_EVENT_KICKOFF:
> > > @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct
> > drm_connector *connector,
> > > 
> > >  static void _dpu_encoder_get_topology(
> > >  			struct dpu_encoder_virt *dpu_enc,
> > > -			struct drm_crtc_state *crtc_state,
> > > -			struct drm_display_mode *mode)
> > > +			struct drm_crtc_state *crtc_state)
> > >  {
> > >  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > 
> > > -	/* User split topology for width > 1080 */
> > > -	dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2
> > : 1;
> > > -	dpu_cstate->num_ctls = dpu_enc->num_phys_encs;
> > >  	dpu_cstate->num_intfs = dpu_enc->num_phys_encs;
> > >  }
> > > 
> > > @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check(
> > >  		}
> > >  	}
> > > 
> > > -	_dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode);
> > > +	_dpu_encoder_get_topology(dpu_enc, crtc_state);
> > >  	if (!ret && drm_atomic_crtc_needs_modeset(crtc_state))
> > > -		ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state);
> > > +		ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc,
> > crtc_state);
> > > 
> > >  	if (!ret)
> > >  		drm_mode_set_crtcinfo(adj_mode, 0);
> > > @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct
> > drm_encoder *drm_enc)
> > > 
> > >  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
> > > 
> > > -	dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state);
> > > +	dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state);
> > >  }
> > > 
> > >  static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg
> > > *catalog,
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > index 5304597..901b1fc 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm,
> > enum dpu_hw_blk_type type,
> > >  	return -EINVAL;
> > >  }
> > > 
> > > -static void _dpu_rm_release_reservation(struct dpu_rm *rm,
> > > -					struct dpu_crtc_state *dpu_cstate)
> > > +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm,
> > > +					     struct dpu_crtc_state
> > *dpu_cstate)
> > >  {
> > >  	int i;
> > > 
> > > @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct
> > dpu_rm *rm,
> > >  					dpu_cstate->hw_ctls[i]->base.id))
> > >  			dpu_cstate->hw_ctls[i] = NULL;
> > >  	}
> > > +}
> > > +
> > > +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm,
> > > +					      struct dpu_crtc_state
> > *dpu_cstate)
> > > +{
> > > +	int i;
> > > 
> > >  	for (i = 0; i < dpu_cstate->num_intfs; i++) {
> > >  		if (!dpu_cstate->hw_intfs[i])
> > > @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct
> > dpu_rm *rm,
> > >  	}
> > >  }
> > > 
> > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state
> > *crtc_state)
> > > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state
> > *crtc_state)
> > >  {
> > >  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > 
> > >  	mutex_lock(&rm->rm_lock);
> > > 
> > > -	_dpu_rm_release_reservation(rm, dpu_cstate);
> > > +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
> > > 
> > >  	mutex_unlock(&rm->rm_lock);
> > >  }
> > > 
> > > -int dpu_rm_reserve(
> > > +void dpu_rm_encoder_release(struct dpu_rm *rm,
> > > +			    struct drm_crtc_state *crtc_state)
> > > +{
> > > +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > +
> > > +	mutex_lock(&rm->rm_lock);
> > > +
> > > +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
> > > +
> > > +	mutex_unlock(&rm->rm_lock);
> > > +}
> > > +
> > > +int dpu_rm_crtc_reserve(
> > >  		struct dpu_rm *rm,
> > > -		struct drm_encoder *enc,
> > >  		struct drm_crtc_state *crtc_state)
> > >  {
> > > -	struct dpu_encoder_hw_resources hw_res;
> > >  	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > >  	int ret;
> > > 
> > > @@ -499,12 +515,10 @@ int dpu_rm_reserve(
> > >  	if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > >  		return 0;
> > > 
> > > -	DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
> > > -		      enc->base.id, crtc_state->crtc->base.id);
> > > 
> > > -	mutex_lock(&rm->rm_lock);
> > > +	DRM_DEBUG_KMS("reserving hw for crtc %d\n",
> > crtc_state->crtc->base.id);
> > > 
> > > -	dpu_encoder_get_hw_resources(enc, &hw_res);
> > > +	mutex_lock(&rm->rm_lock);
> > > 
> > >  	ret = _dpu_rm_reserve_lms(rm, dpu_cstate);
> > >  	if (ret) {
> > > @@ -518,6 +532,37 @@ int dpu_rm_reserve(
> > >  		goto cleanup_on_fail;
> > >  	}
> > > 
> > > +	mutex_unlock(&rm->rm_lock);
> > > +
> > > +	return ret;
> > > +
> > > +cleanup_on_fail:
> > > +	_dpu_rm_crtc_release_reservation(rm, dpu_cstate);
> > > +
> > > +	mutex_unlock(&rm->rm_lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +int dpu_rm_encoder_reserve(
> > > +		struct dpu_rm *rm,
> > > +		struct drm_encoder *enc,
> > > +		struct drm_crtc_state *crtc_state)
> > > +{
> > > +	struct dpu_encoder_hw_resources hw_res;
> > > +	struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state);
> > > +	int ret;
> > > +
> > > +	/* Check if this is just a page-flip */
> > > +	if (!drm_atomic_crtc_needs_modeset(crtc_state))
> > > +		return 0;
> > > +
> > > +	DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id);
> > > +
> > > +	mutex_lock(&rm->rm_lock);
> > > +
> > > +	dpu_encoder_get_hw_resources(enc, &hw_res);
> > > +
> > >  	ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res);
> > >  	if (ret) {
> > >  		DPU_ERROR("unable to find appropriate INTF\n");
> > > @@ -529,7 +574,7 @@ int dpu_rm_reserve(
> > >  	return ret;
> > > 
> > >  cleanup_on_fail:
> > > -	_dpu_rm_release_reservation(rm, dpu_cstate);
> > > +	_dpu_rm_encoder_release_reservation(rm, dpu_cstate);
> > > 
> > >  	mutex_unlock(&rm->rm_lock);
> > > 
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > index 1626cef..0b1deb0 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > @@ -53,27 +53,45 @@ int dpu_rm_init(struct dpu_rm *rm,
> > >  int dpu_rm_destroy(struct dpu_rm *rm);
> > > 
> > >  /**
> > > - * dpu_rm_reserve - Given a CRTC->Encoder->Connector display chain,
> > analyze
> > > - *	the use connections and user requirements, specified through
> > related
> > > + * dpu_rm_encoder_reserve - Given an Encoder, analyze the use
> > connections
> > > + *      and user requirements, specified through related
> > >   *	topology control properties, and reserve hardware blocks to that
> > >   *	display chain.
> > > - *	HW blocks can then be accessed through dpu_rm_get_* functions.
> > > - *	HW Reservations should be released via dpu_rm_release_hw.
> > >   * @rm: DPU Resource Manager handle
> > >   * @drm_enc: DRM Encoder handle
> > >   * @crtc_state: Proposed Atomic DRM CRTC State handle
> > >   * @Return: 0 on Success otherwise -ERROR
> > >   */
> > > -int dpu_rm_reserve(struct dpu_rm *rm,
> > > +int dpu_rm_encoder_reserve(struct dpu_rm *rm,
> > >  		struct drm_encoder *drm_enc,
> > >  		struct drm_crtc_state *crtc_state);
> > > 
> > >  /**
> > > - * dpu_rm_release - Given the encoder for the display chain, release
> > any
> > > - *	HW blocks previously reserved for that use case.
> > > + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections
> > > + *      and user requirements, specified through related
> > > + *	topology control properties, and reserve hardware blocks to that
> > > + *	display chain.
> > >   * @rm: DPU Resource Manager handle
> > > - * @crtc_state: atomic DRM state handle
> > > + * @crtc_state: Proposed Atomic DRM CRTC State handle
> > >   * @Return: 0 on Success otherwise -ERROR
> > >   */
> > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state
> > *crtc_state);
> > > +int dpu_rm_crtc_reserve(struct dpu_rm *rm,
> > > +		struct drm_crtc_state *crtc_state);
> > > +
> > > +/**
> > > + * dpu_rm_encoder_release - Given the encoder, release any
> > > + *	HW blocks previously reserved for that use case.
> > > + * @rm: DPU Resource Manager handle
> > > + * @crtc_state: Proposed Atomic DRM CRTC State handle
> > > + */
> > > +void dpu_rm_encoder_release(struct dpu_rm *rm,
> > > +			    struct drm_crtc_state *crtc_state);
> > > +
> > > +/**
> > > + * dpu_rm_crtc_release - Given the crtc, release any
> > > + *	HW blocks previously reserved for that use case.
> > > + * @rm: DPU Resource Manager handle
> > > + * @crtc_state: Proposed Atomic DRM CRTC State handle
> > > + */
> > > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state
> > *crtc_state);
> > >  #endif /* __DPU_RM_H__ */
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > Forum,
> > > a Linux Foundation Collaborative Project
> > > 
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
> 
> -- 
> Jeykumar S