[12/14] drm/msm/dpu: remove topology name

Submitted by Jeykumar Sankaran on Aug. 29, 2018, 12:40 a.m.

Details

Message ID 1535503203-22054-13-git-send-email-jsanka@codeaurora.org
State New
Headers show
Series "clean up DPU for RM refactor" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jeykumar Sankaran Aug. 29, 2018, 12:40 a.m.
Strip down the support for topology enums. It
can be replaced with simple hw count checks.

Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  3 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  1 -
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  9 ++++--
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  7 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 36 ++++++----------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 24 ---------------
 6 files changed, 19 insertions(+), 61 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index dbf669e..56ef349 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1013,7 +1013,6 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 	struct drm_connector *conn = NULL, *conn_iter;
 	struct dpu_rm_hw_iter pp_iter, ctl_iter;
 	struct msm_display_topology topology;
-	enum dpu_rm_topology_name topology_name;
 	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
 	int i = 0, ret;
 
@@ -1069,7 +1068,6 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
 	}
 
-	topology_name = dpu_rm_get_topology_name(topology);
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
@@ -1089,7 +1087,6 @@  static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 			phys->hw_ctl = hw_ctl[i];
 
 			phys->connector = conn->state->connector;
-			phys->topology_name = topology_name;
 			if (phys->ops.mode_set)
 				phys->ops.mode_set(phys, mode, adj_mode);
 		}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 60f809f..c5600e6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -35,7 +35,6 @@ 
  * @needs_cdm:	Encoder requests a CDM based on pixel format conversion needs
  * @display_num_of_h_tiles: Number of horizontal tiles in case of split
  *                          interface
- * @topology:   Topology of the display
  */
 struct dpu_encoder_hw_resources {
 	enum dpu_intf_mode intfs[INTF_MAX];
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index b3917e0..b5fc65c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -24,6 +24,7 @@ 
 #include "dpu_hw_top.h"
 #include "dpu_hw_cdm.h"
 #include "dpu_encoder.h"
+#include "dpu_crtc.h"
 
 #define DPU_ENCODER_NAME_MAX	16
 
@@ -213,7 +214,6 @@  struct dpu_encoder_irq {
  * @split_role:		Role to play in a split-panel configuration
  * @intf_mode:		Interface mode
  * @intf_idx:		Interface index on dpu hardware
- * @topology_name:	topology selected for the display
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  * @enable_state:	Enable state tracking
  * @vblank_refcount:	Reference count of vblank request
@@ -243,7 +243,6 @@  struct dpu_encoder_phys {
 	enum dpu_enc_split_role split_role;
 	enum dpu_intf_mode intf_mode;
 	enum dpu_intf intf_idx;
-	enum dpu_rm_topology_name topology_name;
 	spinlock_t *enc_spinlock;
 	enum dpu_enc_enable_state enable_state;
 	atomic_t vblank_refcount;
@@ -361,11 +360,15 @@  struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
 		struct dpu_encoder_phys *phys_enc)
 {
+	struct dpu_crtc_state *dpu_cstate;
+
 	if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
 		return BLEND_3D_NONE;
 
+	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
+
 	if (phys_enc->split_role == ENC_ROLE_SOLO &&
-	    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
+	    (dpu_cstate->num_mixers == 2))
 		return BLEND_3D_H_ROW_INT;
 
 	return BLEND_3D_NONE;
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 6de13f4..1643e2f 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
@@ -355,13 +355,14 @@  static void dpu_encoder_phys_vid_underrun_irq(void *arg, int irq_idx)
 
 static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys *phys_enc)
 {
+	struct dpu_crtc_state *dpu_cstate;
+
 	if (!phys_enc)
 		return false;
 
-	if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)
-		return true;
+	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
 
-	return false;
+	return (dpu_cstate->num_ctls > 1);
 }
 
 static bool dpu_encoder_phys_vid_needs_single_flush(
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 13c0a36..fbd489f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -33,7 +33,6 @@ 
 				(t).num_intf == (r).num_intf)
 
 struct dpu_rm_topology_def {
-	enum dpu_rm_topology_name top_name;
 	int num_lm;
 	int num_comp_enc;
 	int num_intf;
@@ -42,10 +41,10 @@  struct dpu_rm_topology_def {
 };
 
 static const struct dpu_rm_topology_def g_top_table[] = {
-	{   DPU_RM_TOPOLOGY_NONE,                 0, 0, 0, 0, false },
-	{   DPU_RM_TOPOLOGY_SINGLEPIPE,           1, 0, 1, 1, false },
-	{   DPU_RM_TOPOLOGY_DUALPIPE,             2, 0, 2, 2, true  },
-	{   DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,     2, 0, 1, 1, false },
+	{   0, 0, 0, 0, false },
+	{   1, 0, 1, 1, false },
+	{   2, 0, 2, 2, true  },
+	{   2, 0, 1, 1, false },
 };
 
 /**
@@ -72,13 +71,11 @@  struct dpu_rm_requirements {
  * @enc_id:	Reservations are tracked by Encoder DRM object ID.
  *		CRTCs may be connected to multiple Encoders.
  *		An encoder or connector id identifies the display path.
- * @topology	DRM<->HW topology use case
  */
 struct dpu_rm_rsvp {
 	struct list_head list;
 	uint32_t seq;
 	uint32_t enc_id;
-	enum dpu_rm_topology_name topology;
 };
 
 /**
@@ -122,8 +119,8 @@  static void _dpu_rm_print_rsvps(
 	DPU_DEBUG("%d\n", stage);
 
 	list_for_each_entry(rsvp, &rm->rsvps, list) {
-		DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage, rsvp->seq,
-			      rsvp->enc_id, rsvp->topology);
+		DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
+			      rsvp->enc_id);
 	}
 
 	for (type = 0; type < DPU_HW_BLK_MAX; type++) {
@@ -146,18 +143,6 @@  struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm)
 	return rm->hw_mdp;
 }
 
-enum dpu_rm_topology_name
-dpu_rm_get_topology_name(struct msm_display_topology topology)
-{
-	int i;
-
-	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)
-		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
-			return g_top_table[i].top_name;
-
-	return DPU_RM_TOPOLOGY_NONE;
-}
-
 void dpu_rm_init_hw_iter(
 		struct dpu_rm_hw_iter *iter,
 		uint32_t enc_id,
@@ -760,7 +745,6 @@  static int _dpu_rm_make_next_rsvp(
 	/* Create reservation info, tag reserved blocks with it as we go */
 	rsvp->seq = ++rm->rsvp_next_seq;
 	rsvp->enc_id = enc->base.id;
-	rsvp->topology = reqs->topology->top_name;
 	list_add_tail(&rsvp->list, &rm->rsvps);
 
 	ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);
@@ -807,7 +791,7 @@  static int _dpu_rm_populate_requirements(
 
 	dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
 
-	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
+	for (i = 0; i < ARRAY_SIZE(g_top_table); i++) {
 		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
 					req_topology)) {
 			reqs->topology = &g_top_table[i];
@@ -831,9 +815,8 @@  static int _dpu_rm_populate_requirements(
 
 	DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n", reqs->top_ctrl,
 		      reqs->hw_res.display_num_of_h_tiles);
-	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display: %d\n",
+	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
 		      reqs->topology->num_lm, reqs->topology->num_ctl,
-		      reqs->topology->top_name,
 		      reqs->topology->needs_split_display);
 
 	return 0;
@@ -969,8 +952,7 @@  static int _dpu_rm_commit_rsvp(
 	}
 
 	if (!ret)
-		DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
-			      rsvp->topology);
+		DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index ffd1841..6fd9e7b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -21,21 +21,6 @@ 
 #include "dpu_hw_top.h"
 
 /**
- * enum dpu_rm_topology_name - HW resource use case in use by connector
- * @DPU_RM_TOPOLOGY_NONE:                 No topology in use currently
- * @DPU_RM_TOPOLOGY_SINGLEPIPE:           1 LM, 1 PP, 1 INTF/WB
- * @DPU_RM_TOPOLOGY_DUALPIPE:             2 LM, 2 PP, 2 INTF/WB
- * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE:     2 LM, 2 PP, 3DMux, 1 INTF/WB
- */
-enum dpu_rm_topology_name {
-	DPU_RM_TOPOLOGY_NONE = 0,
-	DPU_RM_TOPOLOGY_SINGLEPIPE,
-	DPU_RM_TOPOLOGY_DUALPIPE,
-	DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
-	DPU_RM_TOPOLOGY_MAX,
-};
-
-/**
  * enum dpu_rm_topology_control - HW resource use case in use by connector
  * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a successful
  *                              test, reserve the resources for this display.
@@ -187,13 +172,4 @@  void dpu_rm_init_hw_iter(
  */
 int dpu_rm_check_property_topctl(uint64_t val);
 
-/**
- * dpu_rm_get_topology_name - returns the name of the the given topology
- *                            definition
- * @topology: topology definition
- * @Return: name of the topology
- */
-enum dpu_rm_topology_name
-dpu_rm_get_topology_name(struct msm_display_topology topology);
-
 #endif /* __DPU_RM_H__ */

Comments

On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:
> Strip down the support for topology enums. It
> can be replaced with simple hw count checks.
> 

Changes in v4:
...

> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  3 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  1 -
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  9 ++++--
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  7 +++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 36 ++++++----------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 24 ---------------
>  6 files changed, 19 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dbf669e..56ef349 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  	struct drm_connector *conn = NULL, *conn_iter;
>  	struct dpu_rm_hw_iter pp_iter, ctl_iter;
>  	struct msm_display_topology topology;
> -	enum dpu_rm_topology_name topology_name;
>  	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
>  	int i = 0, ret;
>  
> @@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
>  	}
>  
> -	topology_name = dpu_rm_get_topology_name(topology);
>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>  
> @@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>  			phys->hw_ctl = hw_ctl[i];
>  
>  			phys->connector = conn->state->connector;
> -			phys->topology_name = topology_name;
>  			if (phys->ops.mode_set)
>  				phys->ops.mode_set(phys, mode, adj_mode);
>  		}
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 60f809f..c5600e6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -35,7 +35,6 @@
>   * @needs_cdm:	Encoder requests a CDM based on pixel format conversion needs
>   * @display_num_of_h_tiles: Number of horizontal tiles in case of split
>   *                          interface
> - * @topology:   Topology of the display
>   */
>  struct dpu_encoder_hw_resources {
>  	enum dpu_intf_mode intfs[INTF_MAX];
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index b3917e0..b5fc65c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -24,6 +24,7 @@
>  #include "dpu_hw_top.h"
>  #include "dpu_hw_cdm.h"
>  #include "dpu_encoder.h"
> +#include "dpu_crtc.h"
>  
>  #define DPU_ENCODER_NAME_MAX	16
>  
> @@ -213,7 +214,6 @@ struct dpu_encoder_irq {
>   * @split_role:		Role to play in a split-panel configuration
>   * @intf_mode:		Interface mode
>   * @intf_idx:		Interface index on dpu hardware
> - * @topology_name:	topology selected for the display
>   * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
>   * @enable_state:	Enable state tracking
>   * @vblank_refcount:	Reference count of vblank request
> @@ -243,7 +243,6 @@ struct dpu_encoder_phys {
>  	enum dpu_enc_split_role split_role;
>  	enum dpu_intf_mode intf_mode;
>  	enum dpu_intf intf_idx;
> -	enum dpu_rm_topology_name topology_name;
>  	spinlock_t *enc_spinlock;
>  	enum dpu_enc_enable_state enable_state;
>  	atomic_t vblank_refcount;
> @@ -361,11 +360,15 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>  static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode(
>  		struct dpu_encoder_phys *phys_enc)
>  {
> +	struct dpu_crtc_state *dpu_cstate;
> +
>  	if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
>  		return BLEND_3D_NONE;
>  
> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
> +
>  	if (phys_enc->split_role == ENC_ROLE_SOLO &&
> -	    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
> +	    (dpu_cstate->num_mixers == 2))

I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a helper called
dpu_crtc_state_is_stereo() instead of littering these checks everywhere.

>  		return BLEND_3D_H_ROW_INT;
>  
>  	return BLEND_3D_NONE;
> 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 6de13f4..1643e2f 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
> @@ -355,13 +355,14 @@ static void dpu_encoder_phys_vid_underrun_irq(void *arg, int irq_idx)
>  
>  static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys *phys_enc)
>  {
> +	struct dpu_crtc_state *dpu_cstate;
> +
>  	if (!phys_enc)
>  		return false;
>  
> -	if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)
> -		return true;
> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>  
> -	return false;
> +	return (dpu_cstate->num_ctls > 1);

Unnecessary parens

>  }
>  
>  static bool dpu_encoder_phys_vid_needs_single_flush(
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 13c0a36..fbd489f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -33,7 +33,6 @@
>  				(t).num_intf == (r).num_intf)
>  
>  struct dpu_rm_topology_def {
> -	enum dpu_rm_topology_name top_name;
>  	int num_lm;
>  	int num_comp_enc;
>  	int num_intf;
> @@ -42,10 +41,10 @@ struct dpu_rm_topology_def {
>  };
>  
>  static const struct dpu_rm_topology_def g_top_table[] = {
> -	{   DPU_RM_TOPOLOGY_NONE,                 0, 0, 0, 0, false },
> -	{   DPU_RM_TOPOLOGY_SINGLEPIPE,           1, 0, 1, 1, false },
> -	{   DPU_RM_TOPOLOGY_DUALPIPE,             2, 0, 2, 2, true  },
> -	{   DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,     2, 0, 1, 1, false },
> +	{   0, 0, 0, 0, false },
> +	{   1, 0, 1, 1, false },
> +	{   2, 0, 2, 2, true  },
> +	{   2, 0, 1, 1, false },

You don't need this. The data it provides can be deduced already:
num_ctl == num_intf
needs_split_display == (num_intf > 1)

>  };
>  
>  /**
> @@ -72,13 +71,11 @@ struct dpu_rm_requirements {
>   * @enc_id:	Reservations are tracked by Encoder DRM object ID.
>   *		CRTCs may be connected to multiple Encoders.
>   *		An encoder or connector id identifies the display path.
> - * @topology	DRM<->HW topology use case
>   */
>  struct dpu_rm_rsvp {
>  	struct list_head list;
>  	uint32_t seq;
>  	uint32_t enc_id;
> -	enum dpu_rm_topology_name topology;
>  };
>  
>  /**
> @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps(
>  	DPU_DEBUG("%d\n", stage);
>  
>  	list_for_each_entry(rsvp, &rm->rsvps, list) {
> -		DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage, rsvp->seq,
> -			      rsvp->enc_id, rsvp->topology);
> +		DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
> +			      rsvp->enc_id);
>  	}
>  
>  	for (type = 0; type < DPU_HW_BLK_MAX; type++) {
> @@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm)
>  	return rm->hw_mdp;
>  }
>  
> -enum dpu_rm_topology_name
> -dpu_rm_get_topology_name(struct msm_display_topology topology)
> -{
> -	int i;
> -
> -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)
> -		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
> -			return g_top_table[i].top_name;
> -
> -	return DPU_RM_TOPOLOGY_NONE;
> -}
> -
>  void dpu_rm_init_hw_iter(
>  		struct dpu_rm_hw_iter *iter,
>  		uint32_t enc_id,
> @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp(
>  	/* Create reservation info, tag reserved blocks with it as we go */
>  	rsvp->seq = ++rm->rsvp_next_seq;
>  	rsvp->enc_id = enc->base.id;
> -	rsvp->topology = reqs->topology->top_name;
>  	list_add_tail(&rsvp->list, &rm->rsvps);
>  
>  	ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);
> @@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(
>  
>  	dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
>  
> -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
> +	for (i = 0; i < ARRAY_SIZE(g_top_table); i++) {
>  		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
>  					req_topology)) {
>  			reqs->topology = &g_top_table[i];

Since you can get rid of g_top_table, you can get rid of reqs->topology too.

> @@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(
>  
>  	DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n", reqs->top_ctrl,
>  		      reqs->hw_res.display_num_of_h_tiles);
> -	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display: %d\n",
> +	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
>  		      reqs->topology->num_lm, reqs->topology->num_ctl,
> -		      reqs->topology->top_name,
>  		      reqs->topology->needs_split_display);
>  
>  	return 0;
> @@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp(
>  	}
>  
>  	if (!ret)
> -		DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
> -			      rsvp->topology);
> +		DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);

This debug message doesn't provide any value any longer.

>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index ffd1841..6fd9e7b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -21,21 +21,6 @@
>  #include "dpu_hw_top.h"
>  
>  /**
> - * enum dpu_rm_topology_name - HW resource use case in use by connector
> - * @DPU_RM_TOPOLOGY_NONE:                 No topology in use currently
> - * @DPU_RM_TOPOLOGY_SINGLEPIPE:           1 LM, 1 PP, 1 INTF/WB
> - * @DPU_RM_TOPOLOGY_DUALPIPE:             2 LM, 2 PP, 2 INTF/WB
> - * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE:     2 LM, 2 PP, 3DMux, 1 INTF/WB
> - */
> -enum dpu_rm_topology_name {
> -	DPU_RM_TOPOLOGY_NONE = 0,
> -	DPU_RM_TOPOLOGY_SINGLEPIPE,
> -	DPU_RM_TOPOLOGY_DUALPIPE,
> -	DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
> -	DPU_RM_TOPOLOGY_MAX,
> -};
> -
> -/**
>   * enum dpu_rm_topology_control - HW resource use case in use by connector
>   * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a successful
>   *                              test, reserve the resources for this display.
> @@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter(
>   */
>  int dpu_rm_check_property_topctl(uint64_t val);
>  
> -/**
> - * dpu_rm_get_topology_name - returns the name of the the given topology
> - *                            definition
> - * @topology: topology definition
> - * @Return: name of the topology
> - */
> -enum dpu_rm_topology_name
> -dpu_rm_get_topology_name(struct msm_display_topology topology);
> -
>  #endif /* __DPU_RM_H__ */
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
On 2018-08-31 09:08, Sean Paul wrote:
> On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:
>> Strip down the support for topology enums. It
>> can be replaced with simple hw count checks.
>> 
> 
> Changes in v4:
> ...
> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  3 --
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  1 -
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  9 ++++--
>>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  7 +++--
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 36
> ++++++----------------
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 24 
>> ---------------
>>  6 files changed, 19 insertions(+), 61 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index dbf669e..56ef349 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  	struct drm_connector *conn = NULL, *conn_iter;
>>  	struct dpu_rm_hw_iter pp_iter, ctl_iter;
>>  	struct msm_display_topology topology;
>> -	enum dpu_rm_topology_name topology_name;
>>  	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
>>  	int i = 0, ret;
>> 
>> @@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
>>  	}
>> 
>> -	topology_name = dpu_rm_get_topology_name(topology);
>>  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> 
>> @@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct
> drm_encoder *drm_enc,
>>  			phys->hw_ctl = hw_ctl[i];
>> 
>>  			phys->connector = conn->state->connector;
>> -			phys->topology_name = topology_name;
>>  			if (phys->ops.mode_set)
>>  				phys->ops.mode_set(phys, mode, adj_mode);
>>  		}
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 60f809f..c5600e6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -35,7 +35,6 @@
>>   * @needs_cdm:	Encoder requests a CDM based on pixel format
> conversion needs
>>   * @display_num_of_h_tiles: Number of horizontal tiles in case of 
>> split
>>   *                          interface
>> - * @topology:   Topology of the display
>>   */
>>  struct dpu_encoder_hw_resources {
>>  	enum dpu_intf_mode intfs[INTF_MAX];
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index b3917e0..b5fc65c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -24,6 +24,7 @@
>>  #include "dpu_hw_top.h"
>>  #include "dpu_hw_cdm.h"
>>  #include "dpu_encoder.h"
>> +#include "dpu_crtc.h"
>> 
>>  #define DPU_ENCODER_NAME_MAX	16
>> 
>> @@ -213,7 +214,6 @@ struct dpu_encoder_irq {
>>   * @split_role:		Role to play in a split-panel
> configuration
>>   * @intf_mode:		Interface mode
>>   * @intf_idx:		Interface index on dpu hardware
>> - * @topology_name:	topology selected for the display
>>   * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>   * @enable_state:	Enable state tracking
>>   * @vblank_refcount:	Reference count of vblank request
>> @@ -243,7 +243,6 @@ struct dpu_encoder_phys {
>>  	enum dpu_enc_split_role split_role;
>>  	enum dpu_intf_mode intf_mode;
>>  	enum dpu_intf intf_idx;
>> -	enum dpu_rm_topology_name topology_name;
>>  	spinlock_t *enc_spinlock;
>>  	enum dpu_enc_enable_state enable_state;
>>  	atomic_t vblank_refcount;
>> @@ -361,11 +360,15 @@ struct dpu_encoder_phys
> *dpu_encoder_phys_cmd_init(
>>  static inline enum dpu_3d_blend_mode
> dpu_encoder_helper_get_3d_blend_mode(
>>  		struct dpu_encoder_phys *phys_enc)
>>  {
>> +	struct dpu_crtc_state *dpu_cstate;
>> +
>>  	if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
>>  		return BLEND_3D_NONE;
>> 
>> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>> +
>>  	if (phys_enc->split_role == ENC_ROLE_SOLO &&
>> -	    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
>> +	    (dpu_cstate->num_mixers == 2))
> 
> I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a 
> helper
> called
> dpu_crtc_state_is_stereo() instead of littering these checks 
> everywhere.
> 
>>  		return BLEND_3D_H_ROW_INT;
>> 
>>  	return BLEND_3D_NONE;
>> 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 6de13f4..1643e2f 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
>> @@ -355,13 +355,14 @@ static void 
>> dpu_encoder_phys_vid_underrun_irq(void
> *arg, int irq_idx)
>> 
>>  static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys
> *phys_enc)
>>  {
>> +	struct dpu_crtc_state *dpu_cstate;
>> +
>>  	if (!phys_enc)
>>  		return false;
>> 
>> -	if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)
>> -		return true;
>> +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
>> 
>> -	return false;
>> +	return (dpu_cstate->num_ctls > 1);
> 
> Unnecessary parens
> 
>>  }
>> 
>>  static bool dpu_encoder_phys_vid_needs_single_flush(
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 13c0a36..fbd489f 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -33,7 +33,6 @@
>>  				(t).num_intf == (r).num_intf)
>> 
>>  struct dpu_rm_topology_def {
>> -	enum dpu_rm_topology_name top_name;
>>  	int num_lm;
>>  	int num_comp_enc;
>>  	int num_intf;
>> @@ -42,10 +41,10 @@ struct dpu_rm_topology_def {
>>  };
>> 
>>  static const struct dpu_rm_topology_def g_top_table[] = {
>> -	{   DPU_RM_TOPOLOGY_NONE,                 0, 0, 0, 0, false },
>> -	{   DPU_RM_TOPOLOGY_SINGLEPIPE,           1, 0, 1, 1, false },
>> -	{   DPU_RM_TOPOLOGY_DUALPIPE,             2, 0, 2, 2, true  },
>> -	{   DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,     2, 0, 1, 1, false },
>> +	{   0, 0, 0, 0, false },
>> +	{   1, 0, 1, 1, false },
>> +	{   2, 0, 2, 2, true  },
>> +	{   2, 0, 1, 1, false },
> 
> You don't need this. The data it provides can be deduced already:
> num_ctl == num_intf
> needs_split_display == (num_intf > 1)
> 
>>  };
>> 
>>  /**
>> @@ -72,13 +71,11 @@ struct dpu_rm_requirements {
>>   * @enc_id:	Reservations are tracked by Encoder DRM object ID.
>>   *		CRTCs may be connected to multiple Encoders.
>>   *		An encoder or connector id identifies the display path.
>> - * @topology	DRM<->HW topology use case
>>   */
>>  struct dpu_rm_rsvp {
>>  	struct list_head list;
>>  	uint32_t seq;
>>  	uint32_t enc_id;
>> -	enum dpu_rm_topology_name topology;
>>  };
>> 
>>  /**
>> @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps(
>>  	DPU_DEBUG("%d\n", stage);
>> 
>>  	list_for_each_entry(rsvp, &rm->rsvps, list) {
>> -		DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage,
> rsvp->seq,
>> -			      rsvp->enc_id, rsvp->topology);
>> +		DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
>> +			      rsvp->enc_id);
>>  	}
>> 
>>  	for (type = 0; type < DPU_HW_BLK_MAX; type++) {
>> @@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm
> *rm)
>>  	return rm->hw_mdp;
>>  }
>> 
>> -enum dpu_rm_topology_name
>> -dpu_rm_get_topology_name(struct msm_display_topology topology)
>> -{
>> -	int i;
>> -
>> -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)
>> -		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
>> -			return g_top_table[i].top_name;
>> -
>> -	return DPU_RM_TOPOLOGY_NONE;
>> -}
>> -
>>  void dpu_rm_init_hw_iter(
>>  		struct dpu_rm_hw_iter *iter,
>>  		uint32_t enc_id,
>> @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp(
>>  	/* Create reservation info, tag reserved blocks with it as we go
> */
>>  	rsvp->seq = ++rm->rsvp_next_seq;
>>  	rsvp->enc_id = enc->base.id;
>> -	rsvp->topology = reqs->topology->top_name;
>>  	list_add_tail(&rsvp->list, &rm->rsvps);
>> 
>>  	ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);
>> @@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(
>> 
>>  	dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
>> 
>> -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
>> +	for (i = 0; i < ARRAY_SIZE(g_top_table); i++) {
>>  		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
>>  					req_topology)) {
>>  			reqs->topology = &g_top_table[i];
> 
> Since you can get rid of g_top_table, you can get rid of reqs->topology
> too.
> 
DPU RM is using reqs->topology (ie. struct dpu_rm_topology_def) to 
represent
the hw_block requirements for a display. Removing the structure
might require changes in rest of RM alloc functions.

If you can hold off on this, I am planning to address your comment in 
the
RM refactor series.

Jeykumar S.

>> @@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(
>> 
>>  	DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n",
> reqs->top_ctrl,
>>  		      reqs->hw_res.display_num_of_h_tiles);
>> -	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display:
> %d\n",
>> +	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
>>  		      reqs->topology->num_lm, reqs->topology->num_ctl,
>> -		      reqs->topology->top_name,
>>  		      reqs->topology->needs_split_display);
>> 
>>  	return 0;
>> @@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp(
>>  	}
>> 
>>  	if (!ret)
>> -		DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
>> -			      rsvp->topology);
>> +		DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
> 
> This debug message doesn't provide any value any longer.
> 
>> 
>>  	return ret;
>>  }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> index ffd1841..6fd9e7b 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> @@ -21,21 +21,6 @@
>>  #include "dpu_hw_top.h"
>> 
>>  /**
>> - * enum dpu_rm_topology_name - HW resource use case in use by 
>> connector
>> - * @DPU_RM_TOPOLOGY_NONE:                 No topology in use 
>> currently
>> - * @DPU_RM_TOPOLOGY_SINGLEPIPE:           1 LM, 1 PP, 1 INTF/WB
>> - * @DPU_RM_TOPOLOGY_DUALPIPE:             2 LM, 2 PP, 2 INTF/WB
>> - * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE:     2 LM, 2 PP, 3DMux, 1 
>> INTF/WB
>> - */
>> -enum dpu_rm_topology_name {
>> -	DPU_RM_TOPOLOGY_NONE = 0,
>> -	DPU_RM_TOPOLOGY_SINGLEPIPE,
>> -	DPU_RM_TOPOLOGY_DUALPIPE,
>> -	DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
>> -	DPU_RM_TOPOLOGY_MAX,
>> -};
>> -
>> -/**
>>   * enum dpu_rm_topology_control - HW resource use case in use by
> connector
>>   * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a
> successful
>>   *                              test, reserve the resources for this
> display.
>> @@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter(
>>   */
>>  int dpu_rm_check_property_topctl(uint64_t val);
>> 
>> -/**
>> - * dpu_rm_get_topology_name - returns the name of the the given
> topology
>> - *                            definition
>> - * @topology: topology definition
>> - * @Return: name of the topology
>> - */
>> -enum dpu_rm_topology_name
>> -dpu_rm_get_topology_name(struct msm_display_topology topology);
>> -
>>  #endif /* __DPU_RM_H__ */
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>>
On Tue, Sep 04, 2018 at 04:03:25PM -0700, Jeykumar Sankaran wrote:
> On 2018-08-31 09:08, Sean Paul wrote:
> > On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:
> > > Strip down the support for topology enums. It
> > > can be replaced with simple hw count checks.
> > > 
> > 
> > Changes in v4:
> > ...
> > 
> > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        |  3 --
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h        |  1 -
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |  9 ++++--
> > >  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  7 +++--
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c             | 36
> > ++++++----------------
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             | 24
> > > ---------------
> > >  6 files changed, 19 insertions(+), 61 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index dbf669e..56ef349 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct
> > drm_encoder *drm_enc,
> > >  	struct drm_connector *conn = NULL, *conn_iter;
> > >  	struct dpu_rm_hw_iter pp_iter, ctl_iter;
> > >  	struct msm_display_topology topology;
> > > -	enum dpu_rm_topology_name topology_name;
> > >  	struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };
> > >  	int i = 0, ret;
> > > 
> > > @@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct
> > drm_encoder *drm_enc,
> > >  		hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
> > >  	}
> > > 
> > > -	topology_name = dpu_rm_get_topology_name(topology);
> > >  	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > >  		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > 
> > > @@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct
> > drm_encoder *drm_enc,
> > >  			phys->hw_ctl = hw_ctl[i];
> > > 
> > >  			phys->connector = conn->state->connector;
> > > -			phys->topology_name = topology_name;
> > >  			if (phys->ops.mode_set)
> > >  				phys->ops.mode_set(phys, mode, adj_mode);
> > >  		}
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > index 60f809f..c5600e6 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > @@ -35,7 +35,6 @@
> > >   * @needs_cdm:	Encoder requests a CDM based on pixel format
> > conversion needs
> > >   * @display_num_of_h_tiles: Number of horizontal tiles in case of
> > > split
> > >   *                          interface
> > > - * @topology:   Topology of the display
> > >   */
> > >  struct dpu_encoder_hw_resources {
> > >  	enum dpu_intf_mode intfs[INTF_MAX];
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > index b3917e0..b5fc65c 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> > > @@ -24,6 +24,7 @@
> > >  #include "dpu_hw_top.h"
> > >  #include "dpu_hw_cdm.h"
> > >  #include "dpu_encoder.h"
> > > +#include "dpu_crtc.h"
> > > 
> > >  #define DPU_ENCODER_NAME_MAX	16
> > > 
> > > @@ -213,7 +214,6 @@ struct dpu_encoder_irq {
> > >   * @split_role:		Role to play in a split-panel
> > configuration
> > >   * @intf_mode:		Interface mode
> > >   * @intf_idx:		Interface index on dpu hardware
> > > - * @topology_name:	topology selected for the display
> > >   * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
> > >   * @enable_state:	Enable state tracking
> > >   * @vblank_refcount:	Reference count of vblank request
> > > @@ -243,7 +243,6 @@ struct dpu_encoder_phys {
> > >  	enum dpu_enc_split_role split_role;
> > >  	enum dpu_intf_mode intf_mode;
> > >  	enum dpu_intf intf_idx;
> > > -	enum dpu_rm_topology_name topology_name;
> > >  	spinlock_t *enc_spinlock;
> > >  	enum dpu_enc_enable_state enable_state;
> > >  	atomic_t vblank_refcount;
> > > @@ -361,11 +360,15 @@ struct dpu_encoder_phys
> > *dpu_encoder_phys_cmd_init(
> > >  static inline enum dpu_3d_blend_mode
> > dpu_encoder_helper_get_3d_blend_mode(
> > >  		struct dpu_encoder_phys *phys_enc)
> > >  {
> > > +	struct dpu_crtc_state *dpu_cstate;
> > > +
> > >  	if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING)
> > >  		return BLEND_3D_NONE;
> > > 
> > > +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
> > > +
> > >  	if (phys_enc->split_role == ENC_ROLE_SOLO &&
> > > -	    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
> > > +	    (dpu_cstate->num_mixers == 2))
> > 
> > I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a helper
> > called
> > dpu_crtc_state_is_stereo() instead of littering these checks everywhere.
> > 
> > >  		return BLEND_3D_H_ROW_INT;
> > > 
> > >  	return BLEND_3D_NONE;
> > > 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 6de13f4..1643e2f 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
> > > @@ -355,13 +355,14 @@ static void
> > > dpu_encoder_phys_vid_underrun_irq(void
> > *arg, int irq_idx)
> > > 
> > >  static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys
> > *phys_enc)
> > >  {
> > > +	struct dpu_crtc_state *dpu_cstate;
> > > +
> > >  	if (!phys_enc)
> > >  		return false;
> > > 
> > > -	if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)
> > > -		return true;
> > > +	dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);
> > > 
> > > -	return false;
> > > +	return (dpu_cstate->num_ctls > 1);
> > 
> > Unnecessary parens
> > 
> > >  }
> > > 
> > >  static bool dpu_encoder_phys_vid_needs_single_flush(
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > index 13c0a36..fbd489f 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > @@ -33,7 +33,6 @@
> > >  				(t).num_intf == (r).num_intf)
> > > 
> > >  struct dpu_rm_topology_def {
> > > -	enum dpu_rm_topology_name top_name;
> > >  	int num_lm;
> > >  	int num_comp_enc;
> > >  	int num_intf;
> > > @@ -42,10 +41,10 @@ struct dpu_rm_topology_def {
> > >  };
> > > 
> > >  static const struct dpu_rm_topology_def g_top_table[] = {
> > > -	{   DPU_RM_TOPOLOGY_NONE,                 0, 0, 0, 0, false },
> > > -	{   DPU_RM_TOPOLOGY_SINGLEPIPE,           1, 0, 1, 1, false },
> > > -	{   DPU_RM_TOPOLOGY_DUALPIPE,             2, 0, 2, 2, true  },
> > > -	{   DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,     2, 0, 1, 1, false },
> > > +	{   0, 0, 0, 0, false },
> > > +	{   1, 0, 1, 1, false },
> > > +	{   2, 0, 2, 2, true  },
> > > +	{   2, 0, 1, 1, false },
> > 
> > You don't need this. The data it provides can be deduced already:
> > num_ctl == num_intf
> > needs_split_display == (num_intf > 1)
> > 
> > >  };
> > > 
> > >  /**
> > > @@ -72,13 +71,11 @@ struct dpu_rm_requirements {
> > >   * @enc_id:	Reservations are tracked by Encoder DRM object ID.
> > >   *		CRTCs may be connected to multiple Encoders.
> > >   *		An encoder or connector id identifies the display path.
> > > - * @topology	DRM<->HW topology use case
> > >   */
> > >  struct dpu_rm_rsvp {
> > >  	struct list_head list;
> > >  	uint32_t seq;
> > >  	uint32_t enc_id;
> > > -	enum dpu_rm_topology_name topology;
> > >  };
> > > 
> > >  /**
> > > @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps(
> > >  	DPU_DEBUG("%d\n", stage);
> > > 
> > >  	list_for_each_entry(rsvp, &rm->rsvps, list) {
> > > -		DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage,
> > rsvp->seq,
> > > -			      rsvp->enc_id, rsvp->topology);
> > > +		DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
> > > +			      rsvp->enc_id);
> > >  	}
> > > 
> > >  	for (type = 0; type < DPU_HW_BLK_MAX; type++) {
> > > @@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm
> > *rm)
> > >  	return rm->hw_mdp;
> > >  }
> > > 
> > > -enum dpu_rm_topology_name
> > > -dpu_rm_get_topology_name(struct msm_display_topology topology)
> > > -{
> > > -	int i;
> > > -
> > > -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)
> > > -		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
> > > -			return g_top_table[i].top_name;
> > > -
> > > -	return DPU_RM_TOPOLOGY_NONE;
> > > -}
> > > -
> > >  void dpu_rm_init_hw_iter(
> > >  		struct dpu_rm_hw_iter *iter,
> > >  		uint32_t enc_id,
> > > @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp(
> > >  	/* Create reservation info, tag reserved blocks with it as we go
> > */
> > >  	rsvp->seq = ++rm->rsvp_next_seq;
> > >  	rsvp->enc_id = enc->base.id;
> > > -	rsvp->topology = reqs->topology->top_name;
> > >  	list_add_tail(&rsvp->list, &rm->rsvps);
> > > 
> > >  	ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);
> > > @@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(
> > > 
> > >  	dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);
> > > 
> > > -	for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {
> > > +	for (i = 0; i < ARRAY_SIZE(g_top_table); i++) {
> > >  		if (RM_IS_TOPOLOGY_MATCH(g_top_table[i],
> > >  					req_topology)) {
> > >  			reqs->topology = &g_top_table[i];
> > 
> > Since you can get rid of g_top_table, you can get rid of reqs->topology
> > too.
> > 
> DPU RM is using reqs->topology (ie. struct dpu_rm_topology_def) to represent
> the hw_block requirements for a display. Removing the structure
> might require changes in rest of RM alloc functions.
> 
> If you can hold off on this, I am planning to address your comment in the
> RM refactor series.

I'd rather it be introduced when it's used. That way it can evalutated on its
own merits when we have the full picture. There's a bunch of unused or
duplicated code surrounding the RM, and I think it'd be best to clean it up
while we all have full context in our heads.

Sean

> 
> Jeykumar S.
> 
> > > @@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(
> > > 
> > >  	DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n",
> > reqs->top_ctrl,
> > >  		      reqs->hw_res.display_num_of_h_tiles);
> > > -	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display:
> > %d\n",
> > > +	DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n",
> > >  		      reqs->topology->num_lm, reqs->topology->num_ctl,
> > > -		      reqs->topology->top_name,
> > >  		      reqs->topology->needs_split_display);
> > > 
> > >  	return 0;
> > > @@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp(
> > >  	}
> > > 
> > >  	if (!ret)
> > > -		DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
> > > -			      rsvp->topology);
> > > +		DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
> > 
> > This debug message doesn't provide any value any longer.
> > 
> > > 
> > >  	return ret;
> > >  }
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > index ffd1841..6fd9e7b 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > @@ -21,21 +21,6 @@
> > >  #include "dpu_hw_top.h"
> > > 
> > >  /**
> > > - * enum dpu_rm_topology_name - HW resource use case in use by
> > > connector
> > > - * @DPU_RM_TOPOLOGY_NONE:                 No topology in use
> > > currently
> > > - * @DPU_RM_TOPOLOGY_SINGLEPIPE:           1 LM, 1 PP, 1 INTF/WB
> > > - * @DPU_RM_TOPOLOGY_DUALPIPE:             2 LM, 2 PP, 2 INTF/WB
> > > - * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE:     2 LM, 2 PP, 3DMux, 1
> > > INTF/WB
> > > - */
> > > -enum dpu_rm_topology_name {
> > > -	DPU_RM_TOPOLOGY_NONE = 0,
> > > -	DPU_RM_TOPOLOGY_SINGLEPIPE,
> > > -	DPU_RM_TOPOLOGY_DUALPIPE,
> > > -	DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
> > > -	DPU_RM_TOPOLOGY_MAX,
> > > -};
> > > -
> > > -/**
> > >   * enum dpu_rm_topology_control - HW resource use case in use by
> > connector
> > >   * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a
> > successful
> > >   *                              test, reserve the resources for this
> > display.
> > > @@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter(
> > >   */
> > >  int dpu_rm_check_property_topctl(uint64_t val);
> > > 
> > > -/**
> > > - * dpu_rm_get_topology_name - returns the name of the the given
> > topology
> > > - *                            definition
> > > - * @topology: topology definition
> > > - * @Return: name of the topology
> > > - */
> > > -enum dpu_rm_topology_name
> > > -dpu_rm_get_topology_name(struct msm_display_topology topology);
> > > -
> > >  #endif /* __DPU_RM_H__ */
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > Forum,
> > > a Linux Foundation Collaborative Project
> > > 
> 
> -- 
> Jeykumar S