[PATCH 0/7] drm/msm/dpu: simplify DPU encoder init

[PATCH 0/7] drm/msm/dpu: simplify DPU encoder init

[Dmitry Baryshkov]

Rework dpu_encoder initialization code, simplifying calling sequences
and separating common init parts.

Dmitry Baryshkov (7):
  drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
  drm/msm/dpu: drop dpu_encoder_early_unregister
  drm/msm/dpu: separate common function to init physical encoder
  drm/msm/dpu: drop duplicated intf/wb indices from encoder structs
  drm/msm/dpu: inline dpu_encoder_get_wb()
  drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()
  drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 190 ++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  14 +-
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  20 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  55 ++---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  35 +---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  38 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  87 +++-----
 7 files changed, 155 insertions(+), 284 deletions(-)

-- 
2.39.2

[PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Dmitry Baryshkov]

There is no reason to split the dpu_encoder interface into separate
_init() and _setup() phases. Merge them into a single function.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 ++++++++-------------
 3 files changed, 56 insertions(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b34416cbd0f5..32785cb1b079 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs dpu_encoder_funcs = {
 		.early_unregister = dpu_encoder_early_unregister,
 };
 
-int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
+struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
+		int drm_enc_mode,
 		struct msm_display_info *disp_info)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	struct dpu_encoder_virt *dpu_enc = NULL;
 	int ret = 0;
 
-	dpu_enc = to_dpu_encoder_virt(enc);
+	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
+	if (!dpu_enc)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
+			       drm_enc_mode, NULL);
+	if (ret) {
+		devm_kfree(dev->dev, dpu_enc);
+		return ERR_PTR(ret);
+	}
+
+	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
+
+	spin_lock_init(&dpu_enc->enc_spinlock);
+	dpu_enc->enabled = false;
+	mutex_init(&dpu_enc->enc_lock);
+	mutex_init(&dpu_enc->rc_lock);
 
 	ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
 	if (ret)
@@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 
 	DPU_DEBUG_ENC(dpu_enc, "created\n");
 
-	return ret;
+	return &dpu_enc->base;
 
 fail:
 	DPU_ERROR("failed to create encoder\n");
 	if (drm_enc)
 		dpu_encoder_destroy(drm_enc);
 
-	return ret;
-
-
-}
-
-struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
-		int drm_enc_mode)
-{
-	struct dpu_encoder_virt *dpu_enc = NULL;
-	int rc = 0;
-
-	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
-	if (!dpu_enc)
-		return ERR_PTR(-ENOMEM);
-
-
-	rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
-							  drm_enc_mode, NULL);
-	if (rc) {
-		devm_kfree(dev->dev, dpu_enc);
-		return ERR_PTR(rc);
-	}
-
-	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
-
-	spin_lock_init(&dpu_enc->enc_spinlock);
-	dpu_enc->enabled = false;
-	mutex_init(&dpu_enc->enc_lock);
-	mutex_init(&dpu_enc->rc_lock);
-
-	return &dpu_enc->base;
+	return ERR_PTR(ret);
 }
 
 int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 6d14f84dd43f..90e1925d7770 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
 /**
  * dpu_encoder_init - initialize virtual encoder object
  * @dev:        Pointer to drm device structure
+ * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
  * @disp_info:  Pointer to display information structure
  * Returns:     Pointer to newly created drm encoder
  */
-struct drm_encoder *dpu_encoder_init(
-		struct drm_device *dev,
-		int drm_enc_mode);
-
-/**
- * dpu_encoder_setup - setup dpu_encoder for the display probed
- * @dev:		Pointer to drm device structure
- * @enc:		Pointer to the drm_encoder
- * @disp_info:	Pointer to the display info
- */
-int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
+struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
+		int drm_enc_mode,
 		struct msm_display_info *disp_info);
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index e1fd7b0f39cd..10bd0fd4ff48 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		    !msm_dsi_is_master_dsi(priv->dsi[i]))
 			continue;
 
-		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
+		memset(&info, 0, sizeof(info));
+		info.intf_type = INTF_DSI;
+
+		info.h_tile_instance[info.num_of_h_tiles++] = i;
+		if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
+			info.h_tile_instance[info.num_of_h_tiles++] = other;
+
+		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
+
+		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
+
+		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {
 			DPU_ERROR("encoder init failed for dsi display\n");
 			return PTR_ERR(encoder);
 		}
 
-		memset(&info, 0, sizeof(info));
-		info.intf_type = INTF_DSI;
-
 		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
@@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 			break;
 		}
 
-		info.h_tile_instance[info.num_of_h_tiles++] = i;
-		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
-
-		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
-
 		if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
 			rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
 			if (rc) {
@@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 					other, rc);
 				break;
 			}
-
-			info.h_tile_instance[info.num_of_h_tiles++] = other;
 		}
-
-		rc = dpu_encoder_setup(dev, encoder, &info);
-		if (rc)
-			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-				  encoder->base.id, rc);
 	}
 
 	return rc;
@@ -589,29 +585,23 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 		if (!priv->dp[i])
 			continue;
 
-		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+		memset(&info, 0, sizeof(info));
+		info.num_of_h_tiles = 1;
+		info.h_tile_instance[0] = i;
+		info.intf_type = INTF_DP;
+
+		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
 		if (IS_ERR(encoder)) {
 			DPU_ERROR("encoder init failed for dsi display\n");
 			return PTR_ERR(encoder);
 		}
 
-		memset(&info, 0, sizeof(info));
 		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
 			drm_encoder_cleanup(encoder);
 			return rc;
 		}
-
-		info.num_of_h_tiles = 1;
-		info.h_tile_instance[0] = i;
-		info.intf_type = INTF_DP;
-		rc = dpu_encoder_setup(dev, encoder, &info);
-		if (rc) {
-			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-				  encoder->base.id, rc);
-			return rc;
-		}
 	}
 
 	return 0;
@@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
 	if (!priv->hdmi)
 		return 0;
 
-	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+	memset(&info, 0, sizeof(info));
+	info.num_of_h_tiles = 1;
+	info.h_tile_instance[0] = i;
+	info.intf_type = INTF_HDMI;
+
+	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
 	if (IS_ERR(encoder)) {
 		DPU_ERROR("encoder init failed for HDMI display\n");
 		return PTR_ERR(encoder);
 	}
 
-	memset(&info, 0, sizeof(info));
 	rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
 	if (rc) {
 		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
@@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
 		return rc;
 	}
 
-	info.num_of_h_tiles = 1;
-	info.h_tile_instance[0] = i;
-	info.intf_type = INTF_HDMI;
-	rc = dpu_encoder_setup(dev, encoder, &info);
-	if (rc) {
-		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-			  encoder->base.id, rc);
-		return rc;
-	}
-
 	return 0;
 }
 
@@ -664,14 +648,19 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
 	struct msm_display_info info;
 	int rc;
 
-	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
+	memset(&info, 0, sizeof(info));
+
+	info.num_of_h_tiles = 1;
+	/* use only WB idx 2 instance for DPU */
+	info.h_tile_instance[0] = WB_2;
+	info.intf_type = INTF_WB;
+
+	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
 	if (IS_ERR(encoder)) {
 		DPU_ERROR("encoder init failed for dsi display\n");
 		return PTR_ERR(encoder);
 	}
 
-	memset(&info, 0, sizeof(info));
-
 	rc = dpu_writeback_init(dev, encoder, wb_formats,
 			n_formats);
 	if (rc) {
@@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
 		return rc;
 	}
 
-	info.num_of_h_tiles = 1;
-	/* use only WB idx 2 instance for DPU */
-	info.h_tile_instance[0] = WB_2;
-	info.intf_type = INTF_WB;
-
-	rc = dpu_encoder_setup(dev, encoder, &info);
-	if (rc) {
-		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-				  encoder->base.id, rc);
-		return rc;
-	}
-
 	return 0;
 }
 
-- 
2.39.2

[PATCH 2/7] drm/msm/dpu: drop dpu_encoder_early_unregister

[Dmitry Baryshkov]

There is no need to clean up debugfs manually, it will be done by the
DRM core on device deregistration.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 32785cb1b079..8c45c949ec39 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder)
 	return _dpu_encoder_init_debugfs(encoder);
 }
 
-static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
-{
-	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
-
-	debugfs_remove_recursive(dpu_enc->debugfs_root);
-}
-
 static int dpu_encoder_virt_add_phys_encs(
 		struct msm_display_info *disp_info,
 		struct dpu_encoder_virt *dpu_enc,
@@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
 static const struct drm_encoder_funcs dpu_encoder_funcs = {
 		.destroy = dpu_encoder_destroy,
 		.late_register = dpu_encoder_late_register,
-		.early_unregister = dpu_encoder_early_unregister,
 };
 
 struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
-- 
2.39.2

[PATCH 3/7] drm/msm/dpu: separate common function to init physical encoder

[Dmitry Baryshkov]

Move common DPU physical encoder initialization code to the new function
dpu_encoder_phys_init().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 31 +++++++++++++++++--
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  3 ++
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 19 +++---------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 20 +++---------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 19 +++---------
 5 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8c45c949ec39..c60dce5861e2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2303,8 +2303,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-		atomic_set(&phys->vsync_cnt, 0);
-		atomic_set(&phys->underrun_cnt, 0);
 
 		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
 			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
@@ -2505,3 +2503,32 @@ unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc)
 
 	return dpu_enc->dsc_mask;
 }
+
+int dpu_encoder_phys_init(struct dpu_encoder_phys *phys_enc,
+			  struct dpu_enc_phys_init_params *p)
+{
+	int i;
+
+	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
+	phys_enc->intf_idx = p->intf_idx;
+	phys_enc->wb_idx = p->wb_idx;
+	phys_enc->parent = p->parent;
+	phys_enc->dpu_kms = p->dpu_kms;
+	phys_enc->split_role = p->split_role;
+	phys_enc->enc_spinlock = p->enc_spinlock;
+	phys_enc->enable_state = DPU_ENC_DISABLED;
+
+	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
+		phys_enc->irq[i] = -EINVAL;
+
+	atomic_set(&phys_enc->vblank_refcount, 0);
+	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
+	atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
+
+	atomic_set(&phys_enc->vsync_cnt, 0);
+	atomic_set(&phys_enc->underrun_cnt, 0);
+
+	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
+
+	return 0;
+}
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 1d434b22180d..7019870215c0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -405,4 +405,7 @@ void dpu_encoder_frame_done_callback(
 		struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *ready_phys, u32 event);
 
+int dpu_encoder_phys_init(struct dpu_encoder_phys *phys,
+			  struct dpu_enc_phys_init_params *p);
+
 #endif /* __dpu_encoder_phys_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 74470d068622..ce86b9ef6bf1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -759,7 +759,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 {
 	struct dpu_encoder_phys *phys_enc = NULL;
 	struct dpu_encoder_phys_cmd *cmd_enc = NULL;
-	int i, ret = 0;
+	int ret = 0;
 
 	DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
 
@@ -770,25 +770,16 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 		return ERR_PTR(ret);
 	}
 	phys_enc = &cmd_enc->base;
-	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
-	phys_enc->intf_idx = p->intf_idx;
+
+	ret = dpu_encoder_phys_init(phys_enc, p);
+	if (ret)
+		return ERR_PTR(ret);
 
 	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
-	phys_enc->parent = p->parent;
-	phys_enc->dpu_kms = p->dpu_kms;
-	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_CMD;
-	phys_enc->enc_spinlock = p->enc_spinlock;
 	cmd_enc->stream_sel = 0;
-	phys_enc->enable_state = DPU_ENC_DISABLED;
-	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
-		phys_enc->irq[i] = -EINVAL;
 
-	atomic_set(&phys_enc->vblank_refcount, 0);
-	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
-	atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
 	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
-	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
 	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
 
 	DPU_DEBUG_CMDENC(cmd_enc, "created\n");
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 3a374292f311..aca3849621e2 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
@@ -699,7 +699,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 		struct dpu_enc_phys_init_params *p)
 {
 	struct dpu_encoder_phys *phys_enc = NULL;
-	int i;
+	int ret;
 
 	if (!p) {
 		DPU_ERROR("failed to create encoder due to invalid parameter\n");
@@ -712,24 +712,14 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
-	phys_enc->intf_idx = p->intf_idx;
-
 	DPU_DEBUG_VIDENC(phys_enc, "\n");
 
+	ret = dpu_encoder_phys_init(phys_enc, p);
+	if (ret)
+		return ERR_PTR(ret);
+
 	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
-	phys_enc->parent = p->parent;
-	phys_enc->dpu_kms = p->dpu_kms;
-	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_VIDEO;
-	phys_enc->enc_spinlock = p->enc_spinlock;
-	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
-		phys_enc->irq[i] = -EINVAL;
-
-	atomic_set(&phys_enc->vblank_refcount, 0);
-	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
-	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
-	phys_enc->enable_state = DPU_ENC_DISABLED;
 
 	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->intf_idx);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index f879d006de21..c252127552c6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -683,7 +683,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 	struct dpu_encoder_phys *phys_enc = NULL;
 	struct dpu_encoder_phys_wb *wb_enc = NULL;
 	int ret = 0;
-	int i;
 
 	DPU_DEBUG("\n");
 
@@ -701,28 +700,18 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 	}
 
 	phys_enc = &wb_enc->base;
-	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
-	phys_enc->wb_idx = p->wb_idx;
+
+	ret = dpu_encoder_phys_init(phys_enc, p);
+	if (ret)
+		return ERR_PTR(ret);
 
 	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
-	phys_enc->parent = p->parent;
-	phys_enc->dpu_kms = p->dpu_kms;
-	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_WB_LINE;
-	phys_enc->wb_idx = p->wb_idx;
-	phys_enc->enc_spinlock = p->enc_spinlock;
 
 	atomic_set(&wb_enc->wbirq_refcount, 0);
 
-	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
-		phys_enc->irq[i] = -EINVAL;
-
-	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
-	atomic_set(&phys_enc->vblank_refcount, 0);
 	wb_enc->wb_done_timeout_cnt = 0;
 
-	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
-	phys_enc->enable_state = DPU_ENC_DISABLED;
 
 	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
 			phys_enc->wb_idx);
-- 
2.39.2

[PATCH 4/7] drm/msm/dpu: drop duplicated intf/wb indices from encoder structs

[Dmitry Baryshkov]

Remove intf_idx and wb_idx fields from struct dpu_encoder_phys and
struct dpu_enc_phys_init_params. Set the hw_intf and hw_wb directly and
use them to get the instance index.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 72 ++++++++-----------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  | 12 ++--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 16 ++---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  2 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  8 +--
 5 files changed, 46 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index c60dce5861e2..4c85cbb030e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -339,7 +339,8 @@ void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
 	DRM_ERROR("irq timeout id=%u, intf_mode=%s intf=%d wb=%d, pp=%d, intr=%d\n",
 			DRMID(phys_enc->parent),
 			dpu_encoder_helper_get_intf_type(phys_enc->intf_mode),
-			phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
+			phys_enc->hw_intf ? phys_enc->hw_intf->idx - INTF_0 : -1,
+			phys_enc->hw_wb ? phys_enc->hw_wb->idx - WB_0 : -1,
 			phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
 
 	dpu_encoder_frame_done_callback(phys_enc->parent, phys_enc,
@@ -1408,7 +1409,8 @@ void dpu_encoder_frame_done_callback(
 			 */
 			trace_dpu_enc_frame_done_cb_not_busy(DRMID(drm_enc), event,
 					dpu_encoder_helper_get_intf_type(ready_phys->intf_mode),
-					ready_phys->intf_idx, ready_phys->wb_idx);
+					ready_phys->hw_intf ? ready_phys->hw_intf->idx : -1,
+					ready_phys->hw_wb ? ready_phys->hw_wb->idx : -1);
 			return;
 		}
 
@@ -1488,7 +1490,8 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_trigger_flush(DRMID(drm_enc),
 			dpu_encoder_helper_get_intf_type(phys->intf_mode),
-			phys->intf_idx, phys->wb_idx,
+			phys->hw_intf ? phys->hw_intf->idx : -1,
+			phys->hw_wb ? phys->hw_wb->idx : -1,
 			pending_kickoff_cnt, ctl->idx,
 			extra_flush_bits, ret);
 }
@@ -2099,7 +2102,8 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
 		seq_printf(s, "intf:%d  wb:%d  vsync:%8d     underrun:%8d    ",
-				phys->intf_idx - INTF_0, phys->wb_idx - WB_0,
+				phys->hw_intf ? phys->hw_intf->idx - INTF_0 : -1,
+				phys->hw_wb ? phys->hw_wb->idx - WB_0 : -1,
 				atomic_read(&phys->vsync_cnt),
 				atomic_read(&phys->underrun_cnt));
 
@@ -2256,6 +2260,8 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		 * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
 		 */
 		u32 controller_id = disp_info->h_tile_instance[i];
+		enum dpu_intf intf_idx;
+		enum dpu_wb wb_idx;
 
 		if (disp_info->num_of_h_tiles > 1) {
 			if (i == 0)
@@ -2269,57 +2275,39 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
 				i, controller_id, phys_params.split_role);
 
-		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
+		intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
 							    disp_info->intf_type,
 							    controller_id);
 
-		phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
+		wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
 				disp_info->intf_type, controller_id);
-		/*
-		 * The phys_params might represent either an INTF or a WB unit, but not
-		 * both of them at the same time.
-		 */
-		if ((phys_params.intf_idx == INTF_MAX) &&
-				(phys_params.wb_idx == WB_MAX)) {
-			DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n",
-						  disp_info->intf_type, controller_id);
-			ret = -EINVAL;
-		}
 
-		if ((phys_params.intf_idx != INTF_MAX) &&
-				(phys_params.wb_idx != WB_MAX)) {
-			DPU_ERROR_ENC(dpu_enc, "both intf and wb present: type %d, id %d\n",
-						  disp_info->intf_type, controller_id);
-			ret = -EINVAL;
-		}
+		if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
+			phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx);
 
-		if (!ret) {
-			ret = dpu_encoder_virt_add_phys_encs(disp_info,
-					dpu_enc, &phys_params);
-			if (ret)
-				DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
-		}
-	}
+		if (wb_idx >= WB_0 && wb_idx < WB_MAX)
+			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
-			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
-
-		if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
-			phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
-
-		if (!phys->hw_intf && !phys->hw_wb) {
+		if (!phys_params.hw_intf && !phys_params.hw_wb) {
 			DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at idx: %d\n", i);
 			ret = -EINVAL;
+			break;
 		}
 
-		if (phys->hw_intf && phys->hw_wb) {
+		if (phys_params.hw_intf && phys_params.hw_wb) {
 			DPU_ERROR_ENC(dpu_enc,
 					"invalid phys both intf and wb block at idx: %d\n", i);
 			ret = -EINVAL;
+			break;
 		}
+
+		ret = dpu_encoder_virt_add_phys_encs(disp_info,
+				dpu_enc, &phys_params);
+		if (ret) {
+			DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
+			break;
+		}
+
 	}
 
 	mutex_unlock(&dpu_enc->enc_lock);
@@ -2510,8 +2498,8 @@ int dpu_encoder_phys_init(struct dpu_encoder_phys *phys_enc,
 	int i;
 
 	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
-	phys_enc->intf_idx = p->intf_idx;
-	phys_enc->wb_idx = p->wb_idx;
+	phys_enc->hw_intf = p->hw_intf;
+	phys_enc->hw_wb = p->hw_wb;
 	phys_enc->parent = p->parent;
 	phys_enc->dpu_kms = p->dpu_kms;
 	phys_enc->split_role = p->split_role;
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 7019870215c0..1c096d9390d0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -161,8 +161,6 @@ enum dpu_intr_idx {
  * @enabled:		Whether the encoder has enabled and running a mode
  * @split_role:		Role to play in a split-panel configuration
  * @intf_mode:		Interface mode
- * @intf_idx:		Interface index on dpu hardware
- * @wb_idx:			Writeback index on dpu hardware
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  * @enable_state:	Enable state tracking
  * @vblank_refcount:	Reference count of vblank request
@@ -189,8 +187,6 @@ struct dpu_encoder_phys {
 	struct drm_display_mode cached_mode;
 	enum dpu_enc_split_role split_role;
 	enum dpu_intf_mode intf_mode;
-	enum dpu_intf intf_idx;
-	enum dpu_wb wb_idx;
 	spinlock_t *enc_spinlock;
 	enum dpu_enc_enable_state enable_state;
 	atomic_t vblank_refcount;
@@ -256,16 +252,16 @@ struct dpu_encoder_phys_cmd {
  * @parent:		Pointer to the containing virtual encoder
  * @parent_ops:		Callbacks exposed by the parent to the phys_enc
  * @split_role:		Role to play in a split-panel configuration
- * @intf_idx:		Interface index this phys_enc will control
- * @wb_idx:			Writeback index this phys_enc will control
+ * @hw_intf:		Hardware interface to the intf registers
+ * @hw_wb:		Hardware interface to the wb registers
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  */
 struct dpu_enc_phys_init_params {
 	struct dpu_kms *dpu_kms;
 	struct drm_encoder *parent;
 	enum dpu_enc_split_role split_role;
-	enum dpu_intf intf_idx;
-	enum dpu_wb wb_idx;
+	struct dpu_hw_intf *hw_intf;
+	struct dpu_hw_wb *hw_wb;
 	spinlock_t *enc_spinlock;
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index ce86b9ef6bf1..781290f17714 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -16,12 +16,12 @@
 #define DPU_DEBUG_CMDENC(e, fmt, ...) DPU_DEBUG("enc%d intf%d " fmt, \
 		(e) && (e)->base.parent ? \
 		(e)->base.parent->base.id : -1, \
-		(e) ? (e)->base.intf_idx - INTF_0 : -1, ##__VA_ARGS__)
+		(e) ? (e)->base.hw_intf->idx - INTF_0 : -1, ##__VA_ARGS__)
 
 #define DPU_ERROR_CMDENC(e, fmt, ...) DPU_ERROR("enc%d intf%d " fmt, \
 		(e) && (e)->base.parent ? \
 		(e)->base.parent->base.id : -1, \
-		(e) ? (e)->base.intf_idx - INTF_0 : -1, ##__VA_ARGS__)
+		(e) ? (e)->base.hw_intf->idx - INTF_0 : -1, ##__VA_ARGS__)
 
 #define to_dpu_encoder_phys_cmd(x) \
 	container_of(x, struct dpu_encoder_phys_cmd, base)
@@ -59,7 +59,7 @@ static void _dpu_encoder_phys_cmd_update_intf_cfg(
 	if (!ctl->ops.setup_intf_cfg)
 		return;
 
-	intf_cfg.intf = phys_enc->intf_idx;
+	intf_cfg.intf = phys_enc->hw_intf->idx;
 	intf_cfg.intf_mode_sel = DPU_CTL_MODE_SEL_CMD;
 	intf_cfg.stream_sel = cmd_enc->stream_sel;
 	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
@@ -430,7 +430,7 @@ static void dpu_encoder_phys_cmd_enable_helper(
 		return;
 	}
 
-	dpu_encoder_helper_split_config(phys_enc, phys_enc->intf_idx);
+	dpu_encoder_helper_split_config(phys_enc, phys_enc->hw_intf->idx);
 
 	_dpu_encoder_phys_cmd_pingpong_config(phys_enc);
 
@@ -438,7 +438,7 @@ static void dpu_encoder_phys_cmd_enable_helper(
 		return;
 
 	ctl = phys_enc->hw_ctl;
-	ctl->ops.update_pending_flush_intf(ctl, phys_enc->intf_idx);
+	ctl->ops.update_pending_flush_intf(ctl, phys_enc->hw_intf->idx);
 }
 
 static void dpu_encoder_phys_cmd_enable(struct dpu_encoder_phys *phys_enc)
@@ -525,7 +525,7 @@ static void dpu_encoder_phys_cmd_disable(struct dpu_encoder_phys *phys_enc)
 				phys_enc->hw_pp->idx);
 
 		ctl = phys_enc->hw_ctl;
-		ctl->ops.update_pending_flush_intf(ctl, phys_enc->intf_idx);
+		ctl->ops.update_pending_flush_intf(ctl, phys_enc->hw_intf->idx);
 	}
 
 	phys_enc->enable_state = DPU_ENC_DISABLED;
@@ -670,7 +670,7 @@ static int dpu_encoder_phys_cmd_wait_for_tx_complete(
 	if (rc) {
 		DRM_ERROR("failed wait_for_idle: id:%u ret:%d intf:%d\n",
 			  DRMID(phys_enc->parent), rc,
-			  phys_enc->intf_idx - INTF_0);
+			  phys_enc->hw_intf->idx - INTF_0);
 	}
 
 	return rc;
@@ -761,7 +761,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 	struct dpu_encoder_phys_cmd *cmd_enc = NULL;
 	int ret = 0;
 
-	DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
+	DPU_DEBUG("intf\n");
 
 	cmd_enc = kzalloc(sizeof(*cmd_enc), GFP_KERNEL);
 	if (!cmd_enc) {
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 aca3849621e2..f02ff8f43f47 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
@@ -721,7 +721,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_VIDEO;
 
-	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->intf_idx);
+	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
 
 	return phys_enc;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index c252127552c6..b058c69e8778 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -238,7 +238,7 @@ static int dpu_encoder_phys_wb_atomic_check(
 	const struct drm_display_mode *mode = &crtc_state->mode;
 
 	DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
-			phys_enc->wb_idx, mode->name, mode->hdisplay, mode->vdisplay);
+			phys_enc->hw_wb->idx, mode->name, mode->hdisplay, mode->vdisplay);
 
 	if (!conn_state || !conn_state->connector) {
 		DPU_ERROR("invalid connector state\n");
@@ -559,7 +559,7 @@ static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys *phys_enc)
 	if (!phys_enc)
 		return;
 
-	DPU_DEBUG("[wb:%d]\n", phys_enc->wb_idx - WB_0);
+	DPU_DEBUG("[wb:%d]\n", phys_enc->hw_wb->idx - WB_0);
 
 	kfree(phys_enc);
 }
@@ -712,9 +712,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 
 	wb_enc->wb_done_timeout_cnt = 0;
 
-
-	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
-			phys_enc->wb_idx);
+	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n", phys_enc->hw_wb->idx);
 
 	return phys_enc;
 
-- 
2.39.2

[PATCH 5/7] drm/msm/dpu: inline dpu_encoder_get_wb()

[Dmitry Baryshkov]

The function dpu_encoder_get_wb() returns controller_id if the
corresponding WB is present in the catalog. We can inline this function
and rely on dpu_rm_get_wb() returning NULL for indices for which the
WB is not present on the device.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4c85cbb030e4..507ff3f88c67 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
 	return INTF_MAX;
 }
 
-static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog,
-		enum dpu_intf_type type, u32 controller_id)
-{
-	int i = 0;
-
-	if (type != INTF_WB)
-		return WB_MAX;
-
-	for (i = 0; i < catalog->wb_count; i++) {
-		if (catalog->wb[i].id == controller_id)
-			return catalog->wb[i].id;
-	}
-
-	return WB_MAX;
-}
-
 void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *phy_enc)
 {
@@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		 */
 		u32 controller_id = disp_info->h_tile_instance[i];
 		enum dpu_intf intf_idx;
-		enum dpu_wb wb_idx;
 
 		if (disp_info->num_of_h_tiles > 1) {
 			if (i == 0)
@@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 							    disp_info->intf_type,
 							    controller_id);
 
-		wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
-				disp_info->intf_type, controller_id);
-
 		if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
 			phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx);
 
-		if (wb_idx >= WB_0 && wb_idx < WB_MAX)
-			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
+		if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
+			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id);
 
 		if (!phys_params.hw_intf && !phys_params.hw_wb) {
 			DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at idx: %d\n", i);
-- 
2.39.2

[PATCH 6/7] drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()

[Dmitry Baryshkov]

There is little sense to get intf index just to call dpu_rm_get_intf()
on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 507ff3f88c67..b35e92c658ad 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1259,22 +1259,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
 	mutex_unlock(&dpu_enc->enc_lock);
 }
 
-static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
+static struct dpu_hw_intf *dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
+		struct dpu_rm *dpu_rm,
 		enum dpu_intf_type type, u32 controller_id)
 {
 	int i = 0;
 
 	if (type == INTF_WB)
-		return INTF_MAX;
+		return NULL;
 
 	for (i = 0; i < catalog->intf_count; i++) {
 		if (catalog->intf[i].type == type
 		    && catalog->intf[i].controller_id == controller_id) {
-			return catalog->intf[i].id;
+			return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id);
 		}
 	}
 
-	return INTF_MAX;
+	return NULL;
 }
 
 void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
@@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		 * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
 		 */
 		u32 controller_id = disp_info->h_tile_instance[i];
-		enum dpu_intf intf_idx;
 
 		if (disp_info->num_of_h_tiles > 1) {
 			if (i == 0)
@@ -2258,12 +2258,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
 				i, controller_id, phys_params.split_role);
 
-		intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
-							    disp_info->intf_type,
-							    controller_id);
-
-		if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
-			phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx);
+		phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, &dpu_kms->rm,
+							   disp_info->intf_type,
+							   controller_id);
 
 		if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
 			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id);
@@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 			DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
 			break;
 		}
-
 	}
 
 	mutex_unlock(&dpu_enc->enc_lock);
-- 
2.39.2

[PATCH 7/7] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Dmitry Baryshkov]

The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
INTF and WB are statically allocated to each encoder/phys_enc, drop the
atomic_mode_set callback and set the IRQs during encoder init.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 --
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 -----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 20 +++++--------------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 13 ++----------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 11 +---------
 5 files changed, 8 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index b35e92c658ad..509b4fc7dbc5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1106,8 +1106,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
 		phys->cached_mode = crtc_state->adjusted_mode;
-		if (phys->ops.atomic_mode_set)
-			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
 	}
 }
 
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 1c096d9390d0..67c4b4e0975d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -68,8 +68,6 @@ struct dpu_encoder_phys;
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @atomic_mode_set:		DRM Call. Set a DRM mode.
- *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
  * @atomic_check:		DRM Call. Atomic check new DRM state.
@@ -97,9 +95,6 @@ struct dpu_encoder_phys_ops {
 			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
-			struct drm_crtc_state *crtc_state,
-			struct drm_connector_state *conn_state);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
 	int (*atomic_check)(struct dpu_encoder_phys *encoder,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 781290f17714..3ad03465acfe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -139,20 +139,6 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
 	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
 }
 
-static void dpu_encoder_phys_cmd_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
-
-	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
-
-	phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
-
-	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
 static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -736,7 +722,6 @@ static void dpu_encoder_phys_cmd_init_ops(
 		struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
@@ -777,6 +762,11 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
 
 	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_CMD;
+	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
+	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
+	phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
+	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
+
 	cmd_enc->stream_sel = 0;
 
 	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
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 f02ff8f43f47..cf9a0128ff71 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
@@ -348,16 +348,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
 	return phys_enc->split_role != ENC_ROLE_SOLO;
 }
 
-static void dpu_encoder_phys_vid_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
-
-	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
-}
-
 static int dpu_encoder_phys_vid_control_vblank_irq(
 		struct dpu_encoder_phys *phys_enc,
 		bool enable)
@@ -679,7 +669,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
 static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
@@ -720,6 +709,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
 
 	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_VIDEO;
+	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
+	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
 
 	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index b058c69e8778..27479334176b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -398,15 +398,6 @@ static void dpu_encoder_phys_wb_irq_ctrl(
 		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
 }
 
-static void dpu_encoder_phys_wb_atomic_mode_set(
-		struct dpu_encoder_phys *phys_enc,
-		struct drm_crtc_state *crtc_state,
-		struct drm_connector_state *conn_state)
-{
-
-	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
-}
-
 static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -656,7 +647,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
 static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_wb_is_master;
-	ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_wb_enable;
 	ops->disable = dpu_encoder_phys_wb_disable;
 	ops->destroy = dpu_encoder_phys_wb_destroy;
@@ -707,6 +697,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 
 	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
 	phys_enc->intf_mode = INTF_MODE_WB_LINE;
+	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
 
 	atomic_set(&wb_enc->wbirq_refcount, 0);
 
-- 
2.39.2

Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> There is no reason to split the dpu_encoder interface into separate
> _init() and _setup() phases. Merge them into a single function.
> 

I think the reason for having this split was to pass a valid encoder to 
the interface_modeset_init() and then do the rest of encoder 
initialization after modeset_init().

Looking at the current code, one issue i am seeing is that you will now 
initialize the dpu_encoder's msm_display_info along with dpu_encoder_init().

Most of it is fine but in the case of bonded_dsi(), I see an issue.

The info.num_of_h_tiles++ happens after the modeset_init() of the second 
dsi but now it has been moved earlier.

If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
num_of_h_tiles will still be 2 now.

Even for multi-DP case, the idea originally was the encoder gets setup 
for that display after the modeset_init() of that display.

This might become more relevant for DSC perhaps. So lets say first 
encoder needs DSC and second doesnt, we would know that only post 
msm_dp_modeset_init().

So I think if you move the memcpy(&dpu_enc->disp_info, disp_info, 
sizeof(*disp_info)); line out to be done after modeset_init(), this 
change would look okay.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 ++++++++-------------
>   3 files changed, 56 insertions(+), 100 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index b34416cbd0f5..32785cb1b079 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs dpu_encoder_funcs = {
>   		.early_unregister = dpu_encoder_early_unregister,
>   };
>   
> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> +		int drm_enc_mode,
>   		struct msm_display_info *disp_info)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
> @@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	struct dpu_encoder_virt *dpu_enc = NULL;
>   	int ret = 0;
>   
> -	dpu_enc = to_dpu_encoder_virt(enc);
> +	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
> +	if (!dpu_enc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
> +			       drm_enc_mode, NULL);
> +	if (ret) {
> +		devm_kfree(dev->dev, dpu_enc);
> +		return ERR_PTR(ret);
> +	}
> +
> +	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> +
> +	spin_lock_init(&dpu_enc->enc_spinlock);
> +	dpu_enc->enabled = false;
> +	mutex_init(&dpu_enc->enc_lock);
> +	mutex_init(&dpu_enc->rc_lock);
>   
>   	ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
>   	if (ret)
> @@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   
>   	DPU_DEBUG_ENC(dpu_enc, "created\n");
>   
> -	return ret;
> +	return &dpu_enc->base;
>   
>   fail:
>   	DPU_ERROR("failed to create encoder\n");
>   	if (drm_enc)
>   		dpu_encoder_destroy(drm_enc);
>   
> -	return ret;
> -
> -
> -}
> -
> -struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> -		int drm_enc_mode)
> -{
> -	struct dpu_encoder_virt *dpu_enc = NULL;
> -	int rc = 0;
> -
> -	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
> -	if (!dpu_enc)
> -		return ERR_PTR(-ENOMEM);
> -
> -
> -	rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
> -							  drm_enc_mode, NULL);
> -	if (rc) {
> -		devm_kfree(dev->dev, dpu_enc);
> -		return ERR_PTR(rc);
> -	}
> -
> -	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> -
> -	spin_lock_init(&dpu_enc->enc_spinlock);
> -	dpu_enc->enabled = false;
> -	mutex_init(&dpu_enc->enc_lock);
> -	mutex_init(&dpu_enc->rc_lock);
> -
> -	return &dpu_enc->base;
> +	return ERR_PTR(ret);
>   }
>   
>   int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 6d14f84dd43f..90e1925d7770 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
>   /**
>    * dpu_encoder_init - initialize virtual encoder object
>    * @dev:        Pointer to drm device structure
> + * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
>    * @disp_info:  Pointer to display information structure
>    * Returns:     Pointer to newly created drm encoder
>    */
> -struct drm_encoder *dpu_encoder_init(
> -		struct drm_device *dev,
> -		int drm_enc_mode);
> -
> -/**
> - * dpu_encoder_setup - setup dpu_encoder for the display probed
> - * @dev:		Pointer to drm device structure
> - * @enc:		Pointer to the drm_encoder
> - * @disp_info:	Pointer to the display info
> - */
> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> +		int drm_enc_mode,
>   		struct msm_display_info *disp_info);
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e1fd7b0f39cd..10bd0fd4ff48 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   		    !msm_dsi_is_master_dsi(priv->dsi[i]))
>   			continue;
>   
> -		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> +		memset(&info, 0, sizeof(info));
> +		info.intf_type = INTF_DSI;
> +
> +		info.h_tile_instance[info.num_of_h_tiles++] = i;
> +		if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
> +			info.h_tile_instance[info.num_of_h_tiles++] = other;
> +
> +		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
> +
> +		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
> +
> +		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>   		if (IS_ERR(encoder)) {
>   			DPU_ERROR("encoder init failed for dsi display\n");
>   			return PTR_ERR(encoder);
>   		}
>   
> -		memset(&info, 0, sizeof(info));
> -		info.intf_type = INTF_DSI;
> -
>   		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>   		if (rc) {
>   			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
> @@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   			break;
>   		}
>   
> -		info.h_tile_instance[info.num_of_h_tiles++] = i;
> -		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
> -
> -		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
> -
>   		if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
>   			rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
>   			if (rc) {
> @@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   					other, rc);
>   				break;
>   			}
> -
> -			info.h_tile_instance[info.num_of_h_tiles++] = other;
>   		}
> -
> -		rc = dpu_encoder_setup(dev, encoder, &info);
> -		if (rc)
> -			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
>   	}
>   
>   	return rc;
> @@ -589,29 +585,23 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>   		if (!priv->dp[i])
>   			continue;
>   
> -		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> +		memset(&info, 0, sizeof(info));
> +		info.num_of_h_tiles = 1;
> +		info.h_tile_instance[0] = i;
> +		info.intf_type = INTF_DP;
> +
> +		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>   		if (IS_ERR(encoder)) {
>   			DPU_ERROR("encoder init failed for dsi display\n");
>   			return PTR_ERR(encoder);
>   		}
>   
> -		memset(&info, 0, sizeof(info));
>   		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>   		if (rc) {
>   			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>   			drm_encoder_cleanup(encoder);
>   			return rc;
>   		}
> -
> -		info.num_of_h_tiles = 1;
> -		info.h_tile_instance[0] = i;
> -		info.intf_type = INTF_DP;
> -		rc = dpu_encoder_setup(dev, encoder, &info);
> -		if (rc) {
> -			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
> -			return rc;
> -		}
>   	}
>   
>   	return 0;
> @@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
>   	if (!priv->hdmi)
>   		return 0;
>   
> -	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> +	memset(&info, 0, sizeof(info));
> +	info.num_of_h_tiles = 1;
> +	info.h_tile_instance[0] = i;
> +	info.intf_type = INTF_HDMI;
> +
> +	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>   	if (IS_ERR(encoder)) {
>   		DPU_ERROR("encoder init failed for HDMI display\n");
>   		return PTR_ERR(encoder);
>   	}
>   
> -	memset(&info, 0, sizeof(info));
>   	rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
>   	if (rc) {
>   		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> @@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
>   		return rc;
>   	}
>   
> -	info.num_of_h_tiles = 1;
> -	info.h_tile_instance[0] = i;
> -	info.intf_type = INTF_HDMI;
> -	rc = dpu_encoder_setup(dev, encoder, &info);
> -	if (rc) {
> -		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -			  encoder->base.id, rc);
> -		return rc;
> -	}
> -
>   	return 0;
>   }
>   
> @@ -664,14 +648,19 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
>   	struct msm_display_info info;
>   	int rc;
>   
> -	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
> +	memset(&info, 0, sizeof(info));
> +
> +	info.num_of_h_tiles = 1;
> +	/* use only WB idx 2 instance for DPU */
> +	info.h_tile_instance[0] = WB_2;
> +	info.intf_type = INTF_WB;
> +
> +	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
>   	if (IS_ERR(encoder)) {
>   		DPU_ERROR("encoder init failed for dsi display\n");
>   		return PTR_ERR(encoder);
>   	}
>   
> -	memset(&info, 0, sizeof(info));
> -
>   	rc = dpu_writeback_init(dev, encoder, wb_formats,
>   			n_formats);
>   	if (rc) {
> @@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
>   		return rc;
>   	}
>   
> -	info.num_of_h_tiles = 1;
> -	/* use only WB idx 2 instance for DPU */
> -	info.h_tile_instance[0] = WB_2;
> -	info.intf_type = INTF_WB;
> -
> -	rc = dpu_encoder_setup(dev, encoder, &info);
> -	if (rc) {
> -		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
> -		return rc;
> -	}
> -
>   	return 0;
>   }
>   

Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Dmitry Baryshkov]

On 01/05/2023 22:58, Abhinav Kumar wrote:
> 
> 
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>> There is no reason to split the dpu_encoder interface into separate
>> _init() and _setup() phases. Merge them into a single function.
>>
> 
> I think the reason for having this split was to pass a valid encoder to 
> the interface_modeset_init() and then do the rest of encoder 
> initialization after modeset_init().
> 
> Looking at the current code, one issue i am seeing is that you will now 
> initialize the dpu_encoder's msm_display_info along with 
> dpu_encoder_init().
> 
> Most of it is fine but in the case of bonded_dsi(), I see an issue.
> 
> The info.num_of_h_tiles++ happens after the modeset_init() of the second 
> dsi but now it has been moved earlier.
> 
> If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
> num_of_h_tiles will still be 2 now.

If msm_dsi_modeset_init() fails, the function will err out and fail 
dpu_kms initialization. So it's not important, what is the value of 
num_h_tiles in this case.

> 
> Even for multi-DP case, the idea originally was the encoder gets setup 
> for that display after the modeset_init() of that display.
> 
> This might become more relevant for DSC perhaps. So lets say first 
> encoder needs DSC and second doesnt, we would know that only post 
> msm_dp_modeset_init().
> 
> So I think if you move the memcpy(&dpu_enc->disp_info, disp_info, 
> sizeof(*disp_info)); line out to be done after modeset_init(), this 
> change would look okay.
> 
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 ++++++++-------------
>>   3 files changed, 56 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index b34416cbd0f5..32785cb1b079 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs 
>> dpu_encoder_funcs = {
>>           .early_unregister = dpu_encoder_early_unregister,
>>   };
>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>> +        int drm_enc_mode,
>>           struct msm_display_info *disp_info)
>>   {
>>       struct msm_drm_private *priv = dev->dev_private;
>> @@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct drm_encoder *enc,
>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>       int ret = 0;
>> -    dpu_enc = to_dpu_encoder_virt(enc);
>> +    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>> +    if (!dpu_enc)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>> +                   drm_enc_mode, NULL);
>> +    if (ret) {
>> +        devm_kfree(dev->dev, dpu_enc);
>> +        return ERR_PTR(ret);
>> +    }
>> +
>> +    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>> +
>> +    spin_lock_init(&dpu_enc->enc_spinlock);
>> +    dpu_enc->enabled = false;
>> +    mutex_init(&dpu_enc->enc_lock);
>> +    mutex_init(&dpu_enc->rc_lock);
>>       ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
>>       if (ret)
>> @@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct drm_encoder *enc,
>>       DPU_DEBUG_ENC(dpu_enc, "created\n");
>> -    return ret;
>> +    return &dpu_enc->base;
>>   fail:
>>       DPU_ERROR("failed to create encoder\n");
>>       if (drm_enc)
>>           dpu_encoder_destroy(drm_enc);
>> -    return ret;
>> -
>> -
>> -}
>> -
>> -struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>> -        int drm_enc_mode)
>> -{
>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>> -    int rc = 0;
>> -
>> -    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>> -    if (!dpu_enc)
>> -        return ERR_PTR(-ENOMEM);
>> -
>> -
>> -    rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>> -                              drm_enc_mode, NULL);
>> -    if (rc) {
>> -        devm_kfree(dev->dev, dpu_enc);
>> -        return ERR_PTR(rc);
>> -    }
>> -
>> -    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>> -
>> -    spin_lock_init(&dpu_enc->enc_spinlock);
>> -    dpu_enc->enabled = false;
>> -    mutex_init(&dpu_enc->enc_lock);
>> -    mutex_init(&dpu_enc->rc_lock);
>> -
>> -    return &dpu_enc->base;
>> +    return ERR_PTR(ret);
>>   }
>>   int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 6d14f84dd43f..90e1925d7770 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct 
>> drm_encoder *encoder);
>>   /**
>>    * dpu_encoder_init - initialize virtual encoder object
>>    * @dev:        Pointer to drm device structure
>> + * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
>>    * @disp_info:  Pointer to display information structure
>>    * Returns:     Pointer to newly created drm encoder
>>    */
>> -struct drm_encoder *dpu_encoder_init(
>> -        struct drm_device *dev,
>> -        int drm_enc_mode);
>> -
>> -/**
>> - * dpu_encoder_setup - setup dpu_encoder for the display probed
>> - * @dev:        Pointer to drm device structure
>> - * @enc:        Pointer to the drm_encoder
>> - * @disp_info:    Pointer to the display info
>> - */
>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>> +        int drm_enc_mode,
>>           struct msm_display_info *disp_info);
>>   /**
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index e1fd7b0f39cd..10bd0fd4ff48 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>               !msm_dsi_is_master_dsi(priv->dsi[i]))
>>               continue;
>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>> +        memset(&info, 0, sizeof(info));
>> +        info.intf_type = INTF_DSI;
>> +
>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>> +        if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
>> +            info.h_tile_instance[info.num_of_h_tiles++] = other;
>> +
>> +        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>> +
>> +        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>> +
>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>>           if (IS_ERR(encoder)) {
>>               DPU_ERROR("encoder init failed for dsi display\n");
>>               return PTR_ERR(encoder);
>>           }
>> -        memset(&info, 0, sizeof(info));
>> -        info.intf_type = INTF_DSI;
>> -
>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>           if (rc) {
>>               DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>> @@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>               break;
>>           }
>> -        info.h_tile_instance[info.num_of_h_tiles++] = i;
>> -        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>> -
>> -        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>> -
>>           if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
>>               rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
>>               if (rc) {
>> @@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>                       other, rc);
>>                   break;
>>               }
>> -
>> -            info.h_tile_instance[info.num_of_h_tiles++] = other;
>>           }
>> -
>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>> -        if (rc)
>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> -                  encoder->base.id, rc);
>>       }
>>       return rc;
>> @@ -589,29 +585,23 @@ static int 
>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>           if (!priv->dp[i])
>>               continue;
>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>> +        memset(&info, 0, sizeof(info));
>> +        info.num_of_h_tiles = 1;
>> +        info.h_tile_instance[0] = i;
>> +        info.intf_type = INTF_DP;
>> +
>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>           if (IS_ERR(encoder)) {
>>               DPU_ERROR("encoder init failed for dsi display\n");
>>               return PTR_ERR(encoder);
>>           }
>> -        memset(&info, 0, sizeof(info));
>>           rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>>           if (rc) {
>>               DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>               drm_encoder_cleanup(encoder);
>>               return rc;
>>           }
>> -
>> -        info.num_of_h_tiles = 1;
>> -        info.h_tile_instance[0] = i;
>> -        info.intf_type = INTF_DP;
>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>> -        if (rc) {
>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> -                  encoder->base.id, rc);
>> -            return rc;
>> -        }
>>       }
>>       return 0;
>> @@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct 
>> drm_device *dev,
>>       if (!priv->hdmi)
>>           return 0;
>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>> +    memset(&info, 0, sizeof(info));
>> +    info.num_of_h_tiles = 1;
>> +    info.h_tile_instance[0] = i;
>> +    info.intf_type = INTF_HDMI;
>> +
>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>       if (IS_ERR(encoder)) {
>>           DPU_ERROR("encoder init failed for HDMI display\n");
>>           return PTR_ERR(encoder);
>>       }
>> -    memset(&info, 0, sizeof(info));
>>       rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
>>       if (rc) {
>>           DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>> @@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct 
>> drm_device *dev,
>>           return rc;
>>       }
>> -    info.num_of_h_tiles = 1;
>> -    info.h_tile_instance[0] = i;
>> -    info.intf_type = INTF_HDMI;
>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>> -    if (rc) {
>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> -              encoder->base.id, rc);
>> -        return rc;
>> -    }
>> -
>>       return 0;
>>   }
>> @@ -664,14 +648,19 @@ static int _dpu_kms_initialize_writeback(struct 
>> drm_device *dev,
>>       struct msm_display_info info;
>>       int rc;
>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
>> +    memset(&info, 0, sizeof(info));
>> +
>> +    info.num_of_h_tiles = 1;
>> +    /* use only WB idx 2 instance for DPU */
>> +    info.h_tile_instance[0] = WB_2;
>> +    info.intf_type = INTF_WB;
>> +
>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
>>       if (IS_ERR(encoder)) {
>>           DPU_ERROR("encoder init failed for dsi display\n");
>>           return PTR_ERR(encoder);
>>       }
>> -    memset(&info, 0, sizeof(info));
>> -
>>       rc = dpu_writeback_init(dev, encoder, wb_formats,
>>               n_formats);
>>       if (rc) {
>> @@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct 
>> drm_device *dev,
>>           return rc;
>>       }
>> -    info.num_of_h_tiles = 1;
>> -    /* use only WB idx 2 instance for DPU */
>> -    info.h_tile_instance[0] = WB_2;
>> -    info.intf_type = INTF_WB;
>> -
>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>> -    if (rc) {
>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>> -                  encoder->base.id, rc);
>> -        return rc;
>> -    }
>> -
>>       return 0;
>>   }

-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Abhinav Kumar]

On 5/1/2023 1:45 PM, Dmitry Baryshkov wrote:
> On 01/05/2023 22:58, Abhinav Kumar wrote:
>>
>>
>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>> There is no reason to split the dpu_encoder interface into separate
>>> _init() and _setup() phases. Merge them into a single function.
>>>
>>
>> I think the reason for having this split was to pass a valid encoder 
>> to the interface_modeset_init() and then do the rest of encoder 
>> initialization after modeset_init().
>>
>> Looking at the current code, one issue i am seeing is that you will 
>> now initialize the dpu_encoder's msm_display_info along with 
>> dpu_encoder_init().
>>
>> Most of it is fine but in the case of bonded_dsi(), I see an issue.
>>
>> The info.num_of_h_tiles++ happens after the modeset_init() of the 
>> second dsi but now it has been moved earlier.
>>
>> If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
>> num_of_h_tiles will still be 2 now.
> 
> If msm_dsi_modeset_init() fails, the function will err out and fail 
> dpu_kms initialization. So it's not important, what is the value of 
> num_h_tiles in this case.
> 

But I still feel the msm_display_info should be saved in the dpu encoder 
after the modeset_init() and not before. That way if some display 
interface specific init is done in the modeset_init(), we save the info 
after that.

>>
>> Even for multi-DP case, the idea originally was the encoder gets setup 
>> for that display after the modeset_init() of that display.
>>
>> This might become more relevant for DSC perhaps. So lets say first 
>> encoder needs DSC and second doesnt, we would know that only post 
>> msm_dp_modeset_init().
>>
>> So I think if you move the memcpy(&dpu_enc->disp_info, disp_info, 
>> sizeof(*disp_info)); line out to be done after modeset_init(), this 
>> change would look okay.
>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 ++++++++-------------
>>>   3 files changed, 56 insertions(+), 100 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index b34416cbd0f5..32785cb1b079 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs 
>>> dpu_encoder_funcs = {
>>>           .early_unregister = dpu_encoder_early_unregister,
>>>   };
>>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>> +        int drm_enc_mode,
>>>           struct msm_display_info *disp_info)
>>>   {
>>>       struct msm_drm_private *priv = dev->dev_private;
>>> @@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>> struct drm_encoder *enc,
>>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>>       int ret = 0;
>>> -    dpu_enc = to_dpu_encoder_virt(enc);
>>> +    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>>> +    if (!dpu_enc)
>>> +        return ERR_PTR(-ENOMEM);
>>> +
>>> +    ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>>> +                   drm_enc_mode, NULL);
>>> +    if (ret) {
>>> +        devm_kfree(dev->dev, dpu_enc);
>>> +        return ERR_PTR(ret);
>>> +    }
>>> +
>>> +    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>> +
>>> +    spin_lock_init(&dpu_enc->enc_spinlock);
>>> +    dpu_enc->enabled = false;
>>> +    mutex_init(&dpu_enc->enc_lock);
>>> +    mutex_init(&dpu_enc->rc_lock);
>>>       ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
>>>       if (ret)
>>> @@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>> struct drm_encoder *enc,
>>>       DPU_DEBUG_ENC(dpu_enc, "created\n");
>>> -    return ret;
>>> +    return &dpu_enc->base;
>>>   fail:
>>>       DPU_ERROR("failed to create encoder\n");
>>>       if (drm_enc)
>>>           dpu_encoder_destroy(drm_enc);
>>> -    return ret;
>>> -
>>> -
>>> -}
>>> -
>>> -struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>> -        int drm_enc_mode)
>>> -{
>>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>>> -    int rc = 0;
>>> -
>>> -    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>>> -    if (!dpu_enc)
>>> -        return ERR_PTR(-ENOMEM);
>>> -
>>> -
>>> -    rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>>> -                              drm_enc_mode, NULL);
>>> -    if (rc) {
>>> -        devm_kfree(dev->dev, dpu_enc);
>>> -        return ERR_PTR(rc);
>>> -    }
>>> -
>>> -    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>> -
>>> -    spin_lock_init(&dpu_enc->enc_spinlock);
>>> -    dpu_enc->enabled = false;
>>> -    mutex_init(&dpu_enc->enc_lock);
>>> -    mutex_init(&dpu_enc->rc_lock);
>>> -
>>> -    return &dpu_enc->base;
>>> +    return ERR_PTR(ret);
>>>   }
>>>   int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index 6d14f84dd43f..90e1925d7770 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct 
>>> drm_encoder *encoder);
>>>   /**
>>>    * dpu_encoder_init - initialize virtual encoder object
>>>    * @dev:        Pointer to drm device structure
>>> + * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
>>>    * @disp_info:  Pointer to display information structure
>>>    * Returns:     Pointer to newly created drm encoder
>>>    */
>>> -struct drm_encoder *dpu_encoder_init(
>>> -        struct drm_device *dev,
>>> -        int drm_enc_mode);
>>> -
>>> -/**
>>> - * dpu_encoder_setup - setup dpu_encoder for the display probed
>>> - * @dev:        Pointer to drm device structure
>>> - * @enc:        Pointer to the drm_encoder
>>> - * @disp_info:    Pointer to the display info
>>> - */
>>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>> +        int drm_enc_mode,
>>>           struct msm_display_info *disp_info);
>>>   /**
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index e1fd7b0f39cd..10bd0fd4ff48 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>               !msm_dsi_is_master_dsi(priv->dsi[i]))
>>>               continue;
>>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>> +        memset(&info, 0, sizeof(info));
>>> +        info.intf_type = INTF_DSI;
>>> +
>>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>> +        if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
>>> +            info.h_tile_instance[info.num_of_h_tiles++] = other;
>>> +
>>> +        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>>> +
>>> +        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>>> +
>>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>>>           if (IS_ERR(encoder)) {
>>>               DPU_ERROR("encoder init failed for dsi display\n");
>>>               return PTR_ERR(encoder);
>>>           }
>>> -        memset(&info, 0, sizeof(info));
>>> -        info.intf_type = INTF_DSI;
>>> -
>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>           if (rc) {
>>>               DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>>> @@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>               break;
>>>           }
>>> -        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>> -        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>>> -
>>> -        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>>> -
>>>           if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
>>>               rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
>>>               if (rc) {
>>> @@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>                       other, rc);
>>>                   break;
>>>               }
>>> -
>>> -            info.h_tile_instance[info.num_of_h_tiles++] = other;
>>>           }
>>> -
>>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>>> -        if (rc)
>>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> -                  encoder->base.id, rc);
>>>       }
>>>       return rc;
>>> @@ -589,29 +585,23 @@ static int 
>>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>           if (!priv->dp[i])
>>>               continue;
>>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>>> +        memset(&info, 0, sizeof(info));
>>> +        info.num_of_h_tiles = 1;
>>> +        info.h_tile_instance[0] = i;
>>> +        info.intf_type = INTF_DP;
>>> +
>>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>>           if (IS_ERR(encoder)) {
>>>               DPU_ERROR("encoder init failed for dsi display\n");
>>>               return PTR_ERR(encoder);
>>>           }
>>> -        memset(&info, 0, sizeof(info));
>>>           rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>>>           if (rc) {
>>>               DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>               drm_encoder_cleanup(encoder);
>>>               return rc;
>>>           }
>>> -
>>> -        info.num_of_h_tiles = 1;
>>> -        info.h_tile_instance[0] = i;
>>> -        info.intf_type = INTF_DP;
>>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>>> -        if (rc) {
>>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> -                  encoder->base.id, rc);
>>> -            return rc;
>>> -        }
>>>       }
>>>       return 0;
>>> @@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct 
>>> drm_device *dev,
>>>       if (!priv->hdmi)
>>>           return 0;
>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>>> +    memset(&info, 0, sizeof(info));
>>> +    info.num_of_h_tiles = 1;
>>> +    info.h_tile_instance[0] = i;
>>> +    info.intf_type = INTF_HDMI;
>>> +
>>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>>       if (IS_ERR(encoder)) {
>>>           DPU_ERROR("encoder init failed for HDMI display\n");
>>>           return PTR_ERR(encoder);
>>>       }
>>> -    memset(&info, 0, sizeof(info));
>>>       rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
>>>       if (rc) {
>>>           DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>> @@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct 
>>> drm_device *dev,
>>>           return rc;
>>>       }
>>> -    info.num_of_h_tiles = 1;
>>> -    info.h_tile_instance[0] = i;
>>> -    info.intf_type = INTF_HDMI;
>>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>>> -    if (rc) {
>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> -              encoder->base.id, rc);
>>> -        return rc;
>>> -    }
>>> -
>>>       return 0;
>>>   }
>>> @@ -664,14 +648,19 @@ static int _dpu_kms_initialize_writeback(struct 
>>> drm_device *dev,
>>>       struct msm_display_info info;
>>>       int rc;
>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
>>> +    memset(&info, 0, sizeof(info));
>>> +
>>> +    info.num_of_h_tiles = 1;
>>> +    /* use only WB idx 2 instance for DPU */
>>> +    info.h_tile_instance[0] = WB_2;
>>> +    info.intf_type = INTF_WB;
>>> +
>>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
>>>       if (IS_ERR(encoder)) {
>>>           DPU_ERROR("encoder init failed for dsi display\n");
>>>           return PTR_ERR(encoder);
>>>       }
>>> -    memset(&info, 0, sizeof(info));
>>> -
>>>       rc = dpu_writeback_init(dev, encoder, wb_formats,
>>>               n_formats);
>>>       if (rc) {
>>> @@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct 
>>> drm_device *dev,
>>>           return rc;
>>>       }
>>> -    info.num_of_h_tiles = 1;
>>> -    /* use only WB idx 2 instance for DPU */
>>> -    info.h_tile_instance[0] = WB_2;
>>> -    info.intf_type = INTF_WB;
>>> -
>>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>>> -    if (rc) {
>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>> -                  encoder->base.id, rc);
>>> -        return rc;
>>> -    }
>>> -
>>>       return 0;
>>>   }
> 

Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Dmitry Baryshkov]

On 02/05/2023 00:22, Abhinav Kumar wrote:
> 
> 
> On 5/1/2023 1:45 PM, Dmitry Baryshkov wrote:
>> On 01/05/2023 22:58, Abhinav Kumar wrote:
>>>
>>>
>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>> There is no reason to split the dpu_encoder interface into separate
>>>> _init() and _setup() phases. Merge them into a single function.
>>>>
>>>
>>> I think the reason for having this split was to pass a valid encoder 
>>> to the interface_modeset_init() and then do the rest of encoder 
>>> initialization after modeset_init().
>>>
>>> Looking at the current code, one issue i am seeing is that you will 
>>> now initialize the dpu_encoder's msm_display_info along with 
>>> dpu_encoder_init().
>>>
>>> Most of it is fine but in the case of bonded_dsi(), I see an issue.
>>>
>>> The info.num_of_h_tiles++ happens after the modeset_init() of the 
>>> second dsi but now it has been moved earlier.
>>>
>>> If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
>>> num_of_h_tiles will still be 2 now.
>>
>> If msm_dsi_modeset_init() fails, the function will err out and fail 
>> dpu_kms initialization. So it's not important, what is the value of 
>> num_h_tiles in this case.
>>
> 
> But I still feel the msm_display_info should be saved in the dpu encoder 
> after the modeset_init() and not before. That way if some display 
> interface specific init is done in the modeset_init(), we save the info 
> after that.

Up to now we have been using 'poll' model, e.g. we specifically asked 
for the DSC info from the DSI host rather than making msm_dsi set it. So 
far I don't see a good reason why this should be changed.

> 
>>>
>>> Even for multi-DP case, the idea originally was the encoder gets 
>>> setup for that display after the modeset_init() of that display.
>>>
>>> This might become more relevant for DSC perhaps. So lets say first 
>>> encoder needs DSC and second doesnt, we would know that only post 
>>> msm_dp_modeset_init().
>>>
>>> So I think if you move the memcpy(&dpu_enc->disp_info, disp_info, 
>>> sizeof(*disp_info)); line out to be done after modeset_init(), this 
>>> change would look okay.
>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 
>>>> ++++++++-------------
>>>>   3 files changed, 56 insertions(+), 100 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index b34416cbd0f5..32785cb1b079 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs 
>>>> dpu_encoder_funcs = {
>>>>           .early_unregister = dpu_encoder_early_unregister,
>>>>   };
>>>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>>> +        int drm_enc_mode,
>>>>           struct msm_display_info *disp_info)
>>>>   {
>>>>       struct msm_drm_private *priv = dev->dev_private;
>>>> @@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>>> struct drm_encoder *enc,
>>>>       struct dpu_encoder_virt *dpu_enc = NULL;
>>>>       int ret = 0;
>>>> -    dpu_enc = to_dpu_encoder_virt(enc);
>>>> +    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>>>> +    if (!dpu_enc)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>>>> +                   drm_enc_mode, NULL);
>>>> +    if (ret) {
>>>> +        devm_kfree(dev->dev, dpu_enc);
>>>> +        return ERR_PTR(ret);
>>>> +    }
>>>> +
>>>> +    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>>> +
>>>> +    spin_lock_init(&dpu_enc->enc_spinlock);
>>>> +    dpu_enc->enabled = false;
>>>> +    mutex_init(&dpu_enc->enc_lock);
>>>> +    mutex_init(&dpu_enc->rc_lock);
>>>>       ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
>>>>       if (ret)
>>>> @@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device 
>>>> *dev, struct drm_encoder *enc,
>>>>       DPU_DEBUG_ENC(dpu_enc, "created\n");
>>>> -    return ret;
>>>> +    return &dpu_enc->base;
>>>>   fail:
>>>>       DPU_ERROR("failed to create encoder\n");
>>>>       if (drm_enc)
>>>>           dpu_encoder_destroy(drm_enc);
>>>> -    return ret;
>>>> -
>>>> -
>>>> -}
>>>> -
>>>> -struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>>> -        int drm_enc_mode)
>>>> -{
>>>> -    struct dpu_encoder_virt *dpu_enc = NULL;
>>>> -    int rc = 0;
>>>> -
>>>> -    dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
>>>> -    if (!dpu_enc)
>>>> -        return ERR_PTR(-ENOMEM);
>>>> -
>>>> -
>>>> -    rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>>>> -                              drm_enc_mode, NULL);
>>>> -    if (rc) {
>>>> -        devm_kfree(dev->dev, dpu_enc);
>>>> -        return ERR_PTR(rc);
>>>> -    }
>>>> -
>>>> -    drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
>>>> -
>>>> -    spin_lock_init(&dpu_enc->enc_spinlock);
>>>> -    dpu_enc->enabled = false;
>>>> -    mutex_init(&dpu_enc->enc_lock);
>>>> -    mutex_init(&dpu_enc->rc_lock);
>>>> -
>>>> -    return &dpu_enc->base;
>>>> +    return ERR_PTR(ret);
>>>>   }
>>>>   int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index 6d14f84dd43f..90e1925d7770 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct 
>>>> drm_encoder *encoder);
>>>>   /**
>>>>    * dpu_encoder_init - initialize virtual encoder object
>>>>    * @dev:        Pointer to drm device structure
>>>> + * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
>>>>    * @disp_info:  Pointer to display information structure
>>>>    * Returns:     Pointer to newly created drm encoder
>>>>    */
>>>> -struct drm_encoder *dpu_encoder_init(
>>>> -        struct drm_device *dev,
>>>> -        int drm_enc_mode);
>>>> -
>>>> -/**
>>>> - * dpu_encoder_setup - setup dpu_encoder for the display probed
>>>> - * @dev:        Pointer to drm device structure
>>>> - * @enc:        Pointer to the drm_encoder
>>>> - * @disp_info:    Pointer to the display info
>>>> - */
>>>> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>>>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>>> +        int drm_enc_mode,
>>>>           struct msm_display_info *disp_info);
>>>>   /**
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index e1fd7b0f39cd..10bd0fd4ff48 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>               !msm_dsi_is_master_dsi(priv->dsi[i]))
>>>>               continue;
>>>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>>>> +        memset(&info, 0, sizeof(info));
>>>> +        info.intf_type = INTF_DSI;
>>>> +
>>>> +        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>>> +        if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
>>>> +            info.h_tile_instance[info.num_of_h_tiles++] = other;
>>>> +
>>>> +        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>>>> +
>>>> +        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>>>> +
>>>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>>>>           if (IS_ERR(encoder)) {
>>>>               DPU_ERROR("encoder init failed for dsi display\n");
>>>>               return PTR_ERR(encoder);
>>>>           }
>>>> -        memset(&info, 0, sizeof(info));
>>>> -        info.intf_type = INTF_DSI;
>>>> -
>>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>>           if (rc) {
>>>>               DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>>>> @@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>               break;
>>>>           }
>>>> -        info.h_tile_instance[info.num_of_h_tiles++] = i;
>>>> -        info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
>>>> -
>>>> -        info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
>>>> -
>>>>           if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && 
>>>> priv->dsi[other]) {
>>>>               rc = msm_dsi_modeset_init(priv->dsi[other], dev, 
>>>> encoder);
>>>>               if (rc) {
>>>> @@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>                       other, rc);
>>>>                   break;
>>>>               }
>>>> -
>>>> -            info.h_tile_instance[info.num_of_h_tiles++] = other;
>>>>           }
>>>> -
>>>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>>>> -        if (rc)
>>>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> -                  encoder->base.id, rc);
>>>>       }
>>>>       return rc;
>>>> @@ -589,29 +585,23 @@ static int 
>>>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>           if (!priv->dp[i])
>>>>               continue;
>>>> -        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>>>> +        memset(&info, 0, sizeof(info));
>>>> +        info.num_of_h_tiles = 1;
>>>> +        info.h_tile_instance[0] = i;
>>>> +        info.intf_type = INTF_DP;
>>>> +
>>>> +        encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>>>           if (IS_ERR(encoder)) {
>>>>               DPU_ERROR("encoder init failed for dsi display\n");
>>>>               return PTR_ERR(encoder);
>>>>           }
>>>> -        memset(&info, 0, sizeof(info));
>>>>           rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>>>>           if (rc) {
>>>>               DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>>               drm_encoder_cleanup(encoder);
>>>>               return rc;
>>>>           }
>>>> -
>>>> -        info.num_of_h_tiles = 1;
>>>> -        info.h_tile_instance[0] = i;
>>>> -        info.intf_type = INTF_DP;
>>>> -        rc = dpu_encoder_setup(dev, encoder, &info);
>>>> -        if (rc) {
>>>> -            DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> -                  encoder->base.id, rc);
>>>> -            return rc;
>>>> -        }
>>>>       }
>>>>       return 0;
>>>> @@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct 
>>>> drm_device *dev,
>>>>       if (!priv->hdmi)
>>>>           return 0;
>>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
>>>> +    memset(&info, 0, sizeof(info));
>>>> +    info.num_of_h_tiles = 1;
>>>> +    info.h_tile_instance[0] = i;
>>>> +    info.intf_type = INTF_HDMI;
>>>> +
>>>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>>>>       if (IS_ERR(encoder)) {
>>>>           DPU_ERROR("encoder init failed for HDMI display\n");
>>>>           return PTR_ERR(encoder);
>>>>       }
>>>> -    memset(&info, 0, sizeof(info));
>>>>       rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
>>>>       if (rc) {
>>>>           DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>>>> @@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct 
>>>> drm_device *dev,
>>>>           return rc;
>>>>       }
>>>> -    info.num_of_h_tiles = 1;
>>>> -    info.h_tile_instance[0] = i;
>>>> -    info.intf_type = INTF_HDMI;
>>>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>>>> -    if (rc) {
>>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> -              encoder->base.id, rc);
>>>> -        return rc;
>>>> -    }
>>>> -
>>>>       return 0;
>>>>   }
>>>> @@ -664,14 +648,19 @@ static int 
>>>> _dpu_kms_initialize_writeback(struct drm_device *dev,
>>>>       struct msm_display_info info;
>>>>       int rc;
>>>> -    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
>>>> +    memset(&info, 0, sizeof(info));
>>>> +
>>>> +    info.num_of_h_tiles = 1;
>>>> +    /* use only WB idx 2 instance for DPU */
>>>> +    info.h_tile_instance[0] = WB_2;
>>>> +    info.intf_type = INTF_WB;
>>>> +
>>>> +    encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
>>>>       if (IS_ERR(encoder)) {
>>>>           DPU_ERROR("encoder init failed for dsi display\n");
>>>>           return PTR_ERR(encoder);
>>>>       }
>>>> -    memset(&info, 0, sizeof(info));
>>>> -
>>>>       rc = dpu_writeback_init(dev, encoder, wb_formats,
>>>>               n_formats);
>>>>       if (rc) {
>>>> @@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct 
>>>> drm_device *dev,
>>>>           return rc;
>>>>       }
>>>> -    info.num_of_h_tiles = 1;
>>>> -    /* use only WB idx 2 instance for DPU */
>>>> -    info.h_tile_instance[0] = WB_2;
>>>> -    info.intf_type = INTF_WB;
>>>> -
>>>> -    rc = dpu_encoder_setup(dev, encoder, &info);
>>>> -    if (rc) {
>>>> -        DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>>> -                  encoder->base.id, rc);
>>>> -        return rc;
>>>> -    }
>>>> -
>>>>       return 0;
>>>>   }
>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Abhinav Kumar]

On 5/1/2023 2:27 PM, Dmitry Baryshkov wrote:
> On 02/05/2023 00:22, Abhinav Kumar wrote:
>>
>>
>> On 5/1/2023 1:45 PM, Dmitry Baryshkov wrote:
>>> On 01/05/2023 22:58, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>>> There is no reason to split the dpu_encoder interface into separate
>>>>> _init() and _setup() phases. Merge them into a single function.
>>>>>
>>>>
>>>> I think the reason for having this split was to pass a valid encoder 
>>>> to the interface_modeset_init() and then do the rest of encoder 
>>>> initialization after modeset_init().
>>>>
>>>> Looking at the current code, one issue i am seeing is that you will 
>>>> now initialize the dpu_encoder's msm_display_info along with 
>>>> dpu_encoder_init().
>>>>
>>>> Most of it is fine but in the case of bonded_dsi(), I see an issue.
>>>>
>>>> The info.num_of_h_tiles++ happens after the modeset_init() of the 
>>>> second dsi but now it has been moved earlier.
>>>>
>>>> If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
>>>> num_of_h_tiles will still be 2 now.
>>>
>>> If msm_dsi_modeset_init() fails, the function will err out and fail 
>>> dpu_kms initialization. So it's not important, what is the value of 
>>> num_h_tiles in this case.
>>>
>>
>> But I still feel the msm_display_info should be saved in the dpu 
>> encoder after the modeset_init() and not before. That way if some 
>> display interface specific init is done in the modeset_init(), we save 
>> the info after that.
> 
> Up to now we have been using 'poll' model, e.g. we specifically asked 
> for the DSC info from the DSI host rather than making msm_dsi set it. So 
> far I don't see a good reason why this should be changed.
> 

Ok got it, so my concern came from the fact that we individually poll 
each feature today but lets say the number of features keeps growing we 
will have to combine them all into xxx_xxx_get_disp_info() which fills 
up all the fields of the display_info in one go.

But yes, as long as we do that before calling dpu_encoder_init() it 
should be fine.

Hence,

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

Re: [PATCH 2/7] drm/msm/dpu: drop dpu_encoder_early_unregister

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> There is no need to clean up debugfs manually, it will be done by the
> DRM core on device deregistration.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

There are two reasons to have the debugfs removed in the early_unregister:

1) Today, registration happens in late_register(), hence to balance the 
the call in _dpu_encoder_init_debugfs(), this one is present.

2) In drm_modeset_register_all(), if drm_connector_register_all() fails, 
it calls drm_encoder_unregister_all() first which calls early_unregister().

So to balance these out, dont we need to keep it?

>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 32785cb1b079..8c45c949ec39 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder)
>   	return _dpu_encoder_init_debugfs(encoder);
>   }
>   
> -static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
> -{
> -	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
> -
> -	debugfs_remove_recursive(dpu_enc->debugfs_root);
> -}
> -
>   static int dpu_encoder_virt_add_phys_encs(
>   		struct msm_display_info *disp_info,
>   		struct dpu_encoder_virt *dpu_enc,
> @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
>   static const struct drm_encoder_funcs dpu_encoder_funcs = {
>   		.destroy = dpu_encoder_destroy,
>   		.late_register = dpu_encoder_late_register,
> -		.early_unregister = dpu_encoder_early_unregister,
>   };
>   
>   struct drm_encoder *dpu_encoder_init(struct drm_device *dev,

Re: [PATCH 2/7] drm/msm/dpu: drop dpu_encoder_early_unregister

[Dmitry Baryshkov]

On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> > There is no need to clean up debugfs manually, it will be done by the
> > DRM core on device deregistration.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
>
> There are two reasons to have the debugfs removed in the early_unregister:
>
> 1) Today, registration happens in late_register(), hence to balance the
> the call in _dpu_encoder_init_debugfs(), this one is present.
>
> 2) In drm_modeset_register_all(), if drm_connector_register_all() fails,
> it calls drm_encoder_unregister_all() first which calls early_unregister().
>
> So to balance these out, dont we need to keep it?

Please correct me if I'm wrong, drm_debugfs_cleanup() should take care of that.

> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 --------
> >   1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 32785cb1b079..8c45c949ec39 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder)
> >       return _dpu_encoder_init_debugfs(encoder);
> >   }
> >
> > -static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
> > -{
> > -     struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
> > -
> > -     debugfs_remove_recursive(dpu_enc->debugfs_root);
> > -}
> > -
> >   static int dpu_encoder_virt_add_phys_encs(
> >               struct msm_display_info *disp_info,
> >               struct dpu_encoder_virt *dpu_enc,
> > @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> >   static const struct drm_encoder_funcs dpu_encoder_funcs = {
> >               .destroy = dpu_encoder_destroy,
> >               .late_register = dpu_encoder_late_register,
> > -             .early_unregister = dpu_encoder_early_unregister,
> >   };
> >
> >   struct drm_encoder *dpu_encoder_init(struct drm_device *dev,

-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dpu: drop dpu_encoder_early_unregister

[Abhinav Kumar]

On 5/2/2023 1:54 PM, Dmitry Baryshkov wrote:
> On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>> There is no need to clean up debugfs manually, it will be done by the
>>> DRM core on device deregistration.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>
>> There are two reasons to have the debugfs removed in the early_unregister:
>>
>> 1) Today, registration happens in late_register(), hence to balance the
>> the call in _dpu_encoder_init_debugfs(), this one is present.
>>
>> 2) In drm_modeset_register_all(), if drm_connector_register_all() fails,
>> it calls drm_encoder_unregister_all() first which calls early_unregister().
>>
>> So to balance these out, dont we need to keep it?
> 
> Please correct me if I'm wrong, drm_debugfs_cleanup() should take care of that.
> 

Not from what I am seeing, drm_debugfs_cleanup() is getting called from 
the error handling path of drm_dev_register() and separately from 
drm_dev_unregister() but not from the error handling path of 
drm_modeset_register_all().

So that case will be unbalanced with this change.

>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 --------
>>>    1 file changed, 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 32785cb1b079..8c45c949ec39 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct drm_encoder *encoder)
>>>        return _dpu_encoder_init_debugfs(encoder);
>>>    }
>>>
>>> -static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
>>> -{
>>> -     struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
>>> -
>>> -     debugfs_remove_recursive(dpu_enc->debugfs_root);
>>> -}
>>> -
>>>    static int dpu_encoder_virt_add_phys_encs(
>>>                struct msm_display_info *disp_info,
>>>                struct dpu_encoder_virt *dpu_enc,
>>> @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
>>>    static const struct drm_encoder_funcs dpu_encoder_funcs = {
>>>                .destroy = dpu_encoder_destroy,
>>>                .late_register = dpu_encoder_late_register,
>>> -             .early_unregister = dpu_encoder_early_unregister,
>>>    };
>>>
>>>    struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> 

Re: [PATCH 2/7] drm/msm/dpu: drop dpu_encoder_early_unregister

[Dmitry Baryshkov]

On 02/05/2023 23:59, Abhinav Kumar wrote:
> 
> 
> On 5/2/2023 1:54 PM, Dmitry Baryshkov wrote:
>> On Tue, 2 May 2023 at 23:45, Abhinav Kumar <quic_abhinavk@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>> There is no need to clean up debugfs manually, it will be done by the
>>>> DRM core on device deregistration.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>
>>> There are two reasons to have the debugfs removed in the 
>>> early_unregister:
>>>
>>> 1) Today, registration happens in late_register(), hence to balance the
>>> the call in _dpu_encoder_init_debugfs(), this one is present.
>>>
>>> 2) In drm_modeset_register_all(), if drm_connector_register_all() fails,
>>> it calls drm_encoder_unregister_all() first which calls 
>>> early_unregister().
>>>
>>> So to balance these out, dont we need to keep it?
>>
>> Please correct me if I'm wrong, drm_debugfs_cleanup() should take care 
>> of that.
>>
> 
> Not from what I am seeing, drm_debugfs_cleanup() is getting called from 
> the error handling path of drm_dev_register() and separately from 
> drm_dev_unregister() but not from the error handling path of 
> drm_modeset_register_all().
> 
> So that case will be unbalanced with this change.

But for the practical reasons the drm_modeset_register_all() can not 
fail. But I see your point. I'll check why drm_dev_register() doesn't 
respect an error return from drm_modeset_register_all(). Until that 
point we can drop this patch.

> 
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 8 --------
>>>>    1 file changed, 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 32785cb1b079..8c45c949ec39 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -2154,13 +2154,6 @@ static int dpu_encoder_late_register(struct 
>>>> drm_encoder *encoder)
>>>>        return _dpu_encoder_init_debugfs(encoder);
>>>>    }
>>>>
>>>> -static void dpu_encoder_early_unregister(struct drm_encoder *encoder)
>>>> -{
>>>> -     struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(encoder);
>>>> -
>>>> -     debugfs_remove_recursive(dpu_enc->debugfs_root);
>>>> -}
>>>> -
>>>>    static int dpu_encoder_virt_add_phys_encs(
>>>>                struct msm_display_info *disp_info,
>>>>                struct dpu_encoder_virt *dpu_enc,
>>>> @@ -2374,7 +2367,6 @@ static const struct drm_encoder_helper_funcs 
>>>> dpu_encoder_helper_funcs = {
>>>>    static const struct drm_encoder_funcs dpu_encoder_funcs = {
>>>>                .destroy = dpu_encoder_destroy,
>>>>                .late_register = dpu_encoder_late_register,
>>>> -             .early_unregister = dpu_encoder_early_unregister,
>>>>    };
>>>>
>>>>    struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>>

-- 
With best wishes
Dmitry

Re: [PATCH 3/7] drm/msm/dpu: separate common function to init physical encoder

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> Move common DPU physical encoder initialization code to the new function
> dpu_encoder_phys_init().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 31 +++++++++++++++++--
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  3 ++
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 19 +++---------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 20 +++---------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 19 +++---------
>   5 files changed, 46 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 8c45c949ec39..c60dce5861e2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2303,8 +2303,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>   		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -		atomic_set(&phys->vsync_cnt, 0);
> -		atomic_set(&phys->underrun_cnt, 0);
>   
>   		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>   			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
> @@ -2505,3 +2503,32 @@ unsigned int dpu_encoder_helper_get_dsc(struct dpu_encoder_phys *phys_enc)
>   
>   	return dpu_enc->dsc_mask;
>   }
> +
> +int dpu_encoder_phys_init(struct dpu_encoder_phys *phys_enc,
> +			  struct dpu_enc_phys_init_params *p)
> +{
> +	int i;
> +
> +	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
> +	phys_enc->intf_idx = p->intf_idx;
> +	phys_enc->wb_idx = p->wb_idx;
> +	phys_enc->parent = p->parent;
> +	phys_enc->dpu_kms = p->dpu_kms;
> +	phys_enc->split_role = p->split_role;
> +	phys_enc->enc_spinlock = p->enc_spinlock;
> +	phys_enc->enable_state = DPU_ENC_DISABLED;
> +
> +	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
> +		phys_enc->irq[i] = -EINVAL;
> +
> +	atomic_set(&phys_enc->vblank_refcount, 0);
> +	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> +	atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
> +
> +	atomic_set(&phys_enc->vsync_cnt, 0);
> +	atomic_set(&phys_enc->underrun_cnt, 0);
> +
> +	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
> +
> +	return 0;
> +}
> 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 1d434b22180d..7019870215c0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -405,4 +405,7 @@ void dpu_encoder_frame_done_callback(
>   		struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *ready_phys, u32 event);
>   
> +int dpu_encoder_phys_init(struct dpu_encoder_phys *phys,
> +			  struct dpu_enc_phys_init_params *p);
> +
>   #endif /* __dpu_encoder_phys_H__ */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 74470d068622..ce86b9ef6bf1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -759,7 +759,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   {
>   	struct dpu_encoder_phys *phys_enc = NULL;
>   	struct dpu_encoder_phys_cmd *cmd_enc = NULL;
> -	int i, ret = 0;
> +	int ret = 0;
>   
>   	DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
>   
> @@ -770,25 +770,16 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   		return ERR_PTR(ret);
>   	}
>   	phys_enc = &cmd_enc->base;
> -	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
> -	phys_enc->intf_idx = p->intf_idx;
> +
> +	ret = dpu_encoder_phys_init(phys_enc, p);
> +	if (ret)
> +		return ERR_PTR(ret);

dpu_encoder_phys_init() seems to always return 0, so we can make that 
void and drop ret and return here?

>   
>   	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
> -	phys_enc->parent = p->parent;
> -	phys_enc->dpu_kms = p->dpu_kms;
> -	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_CMD;
> -	phys_enc->enc_spinlock = p->enc_spinlock;
>   	cmd_enc->stream_sel = 0;
> -	phys_enc->enable_state = DPU_ENC_DISABLED;
> -	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
> -		phys_enc->irq[i] = -EINVAL;
>   
> -	atomic_set(&phys_enc->vblank_refcount, 0);
> -	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> -	atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
>   	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
> -	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>   	init_waitqueue_head(&cmd_enc->pending_vblank_wq);
>   
>   	DPU_DEBUG_CMDENC(cmd_enc, "created\n");
> 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 3a374292f311..aca3849621e2 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
> @@ -699,7 +699,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>   		struct dpu_enc_phys_init_params *p)
>   {
>   	struct dpu_encoder_phys *phys_enc = NULL;
> -	int i;
> +	int ret;
>   
>   	if (!p) {
>   		DPU_ERROR("failed to create encoder due to invalid parameter\n");
> @@ -712,24 +712,14 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>   		return ERR_PTR(-ENOMEM);
>   	}
>   
> -	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
> -	phys_enc->intf_idx = p->intf_idx;
> -
>   	DPU_DEBUG_VIDENC(phys_enc, "\n");
>   
> +	ret = dpu_encoder_phys_init(phys_enc, p);
> +	if (ret)
> +		return ERR_PTR(ret);

same here.

> +
>   	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
> -	phys_enc->parent = p->parent;
> -	phys_enc->dpu_kms = p->dpu_kms;
> -	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_VIDEO;
> -	phys_enc->enc_spinlock = p->enc_spinlock;
> -	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
> -		phys_enc->irq[i] = -EINVAL;
> -
> -	atomic_set(&phys_enc->vblank_refcount, 0);
> -	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> -	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
> -	phys_enc->enable_state = DPU_ENC_DISABLED;
>   
>   	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->intf_idx);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index f879d006de21..c252127552c6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -683,7 +683,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>   	struct dpu_encoder_phys *phys_enc = NULL;
>   	struct dpu_encoder_phys_wb *wb_enc = NULL;
>   	int ret = 0;
> -	int i;
>   
>   	DPU_DEBUG("\n");
>   
> @@ -701,28 +700,18 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>   	}
>   
>   	phys_enc = &wb_enc->base;
> -	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
> -	phys_enc->wb_idx = p->wb_idx;
> +
> +	ret = dpu_encoder_phys_init(phys_enc, p);
> +	if (ret)
> +		return ERR_PTR(ret);

same here

>   
>   	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
> -	phys_enc->parent = p->parent;
> -	phys_enc->dpu_kms = p->dpu_kms;
> -	phys_enc->split_role = p->split_role;
>   	phys_enc->intf_mode = INTF_MODE_WB_LINE;
> -	phys_enc->wb_idx = p->wb_idx;
> -	phys_enc->enc_spinlock = p->enc_spinlock;
>   
>   	atomic_set(&wb_enc->wbirq_refcount, 0);
>   
> -	for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
> -		phys_enc->irq[i] = -EINVAL;
> -
> -	atomic_set(&phys_enc->pending_kickoff_cnt, 0);
> -	atomic_set(&phys_enc->vblank_refcount, 0);
>   	wb_enc->wb_done_timeout_cnt = 0;
>   
> -	init_waitqueue_head(&phys_enc->pending_kickoff_wq);
> -	phys_enc->enable_state = DPU_ENC_DISABLED;
>   
>   	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
>   			phys_enc->wb_idx);

Re: [PATCH 3/7] drm/msm/dpu: separate common function to init physical encoder

[Dmitry Baryshkov]

On 03/05/2023 00:33, Abhinav Kumar wrote:
> 
> 
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>> Move common DPU physical encoder initialization code to the new function
>> dpu_encoder_phys_init().
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 31 +++++++++++++++++--
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  3 ++
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 19 +++---------
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 20 +++---------
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 19 +++---------
>>   5 files changed, 46 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 8c45c949ec39..c60dce5861e2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -2303,8 +2303,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>       for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>>           struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> -        atomic_set(&phys->vsync_cnt, 0);
>> -        atomic_set(&phys->underrun_cnt, 0);
>>           if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>               phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> phys->intf_idx);
>> @@ -2505,3 +2503,32 @@ unsigned int dpu_encoder_helper_get_dsc(struct 
>> dpu_encoder_phys *phys_enc)
>>       return dpu_enc->dsc_mask;
>>   }
>> +
>> +int dpu_encoder_phys_init(struct dpu_encoder_phys *phys_enc,
>> +              struct dpu_enc_phys_init_params *p)
>> +{
>> +    int i;
>> +
>> +    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>> +    phys_enc->intf_idx = p->intf_idx;
>> +    phys_enc->wb_idx = p->wb_idx;
>> +    phys_enc->parent = p->parent;
>> +    phys_enc->dpu_kms = p->dpu_kms;
>> +    phys_enc->split_role = p->split_role;
>> +    phys_enc->enc_spinlock = p->enc_spinlock;
>> +    phys_enc->enable_state = DPU_ENC_DISABLED;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
>> +        phys_enc->irq[i] = -EINVAL;
>> +
>> +    atomic_set(&phys_enc->vblank_refcount, 0);
>> +    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> +    atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
>> +
>> +    atomic_set(&phys_enc->vsync_cnt, 0);
>> +    atomic_set(&phys_enc->underrun_cnt, 0);
>> +
>> +    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>> +
>> +    return 0;
>> +}
>> 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 1d434b22180d..7019870215c0 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -405,4 +405,7 @@ void dpu_encoder_frame_done_callback(
>>           struct drm_encoder *drm_enc,
>>           struct dpu_encoder_phys *ready_phys, u32 event);
>> +int dpu_encoder_phys_init(struct dpu_encoder_phys *phys,
>> +              struct dpu_enc_phys_init_params *p);
>> +
>>   #endif /* __dpu_encoder_phys_H__ */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 74470d068622..ce86b9ef6bf1 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -759,7 +759,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>   {
>>       struct dpu_encoder_phys *phys_enc = NULL;
>>       struct dpu_encoder_phys_cmd *cmd_enc = NULL;
>> -    int i, ret = 0;
>> +    int ret = 0;
>>       DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
>> @@ -770,25 +770,16 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>           return ERR_PTR(ret);
>>       }
>>       phys_enc = &cmd_enc->base;
>> -    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>> -    phys_enc->intf_idx = p->intf_idx;
>> +
>> +    ret = dpu_encoder_phys_init(phys_enc, p);
>> +    if (ret)
>> +        return ERR_PTR(ret);
> 
> dpu_encoder_phys_init() seems to always return 0, so we can make that 
> void and drop ret and return here?

I had in mind a possible error from INTF_n/WB_n -> hw_intf/hw_wb lookup, 
but at the end I got rid of that. So, yes, why not.

> 
>>       dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>> -    phys_enc->parent = p->parent;
>> -    phys_enc->dpu_kms = p->dpu_kms;
>> -    phys_enc->split_role = p->split_role;
>>       phys_enc->intf_mode = INTF_MODE_CMD;
>> -    phys_enc->enc_spinlock = p->enc_spinlock;
>>       cmd_enc->stream_sel = 0;
>> -    phys_enc->enable_state = DPU_ENC_DISABLED;
>> -    for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
>> -        phys_enc->irq[i] = -EINVAL;
>> -    atomic_set(&phys_enc->vblank_refcount, 0);
>> -    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> -    atomic_set(&phys_enc->pending_ctlstart_cnt, 0);
>>       atomic_set(&cmd_enc->pending_vblank_cnt, 0);
>> -    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>>       init_waitqueue_head(&cmd_enc->pending_vblank_wq);
>>       DPU_DEBUG_CMDENC(cmd_enc, "created\n");
>> 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 3a374292f311..aca3849621e2 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
>> @@ -699,7 +699,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>>           struct dpu_enc_phys_init_params *p)
>>   {
>>       struct dpu_encoder_phys *phys_enc = NULL;
>> -    int i;
>> +    int ret;
>>       if (!p) {
>>           DPU_ERROR("failed to create encoder due to invalid 
>> parameter\n");
>> @@ -712,24 +712,14 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>>           return ERR_PTR(-ENOMEM);
>>       }
>> -    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>> -    phys_enc->intf_idx = p->intf_idx;
>> -
>>       DPU_DEBUG_VIDENC(phys_enc, "\n");
>> +    ret = dpu_encoder_phys_init(phys_enc, p);
>> +    if (ret)
>> +        return ERR_PTR(ret);
> 
> same here.
> 
>> +
>>       dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>> -    phys_enc->parent = p->parent;
>> -    phys_enc->dpu_kms = p->dpu_kms;
>> -    phys_enc->split_role = p->split_role;
>>       phys_enc->intf_mode = INTF_MODE_VIDEO;
>> -    phys_enc->enc_spinlock = p->enc_spinlock;
>> -    for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
>> -        phys_enc->irq[i] = -EINVAL;
>> -
>> -    atomic_set(&phys_enc->vblank_refcount, 0);
>> -    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> -    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>> -    phys_enc->enable_state = DPU_ENC_DISABLED;
>>       DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->intf_idx);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> index f879d006de21..c252127552c6 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
>> @@ -683,7 +683,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>       struct dpu_encoder_phys *phys_enc = NULL;
>>       struct dpu_encoder_phys_wb *wb_enc = NULL;
>>       int ret = 0;
>> -    int i;
>>       DPU_DEBUG("\n");
>> @@ -701,28 +700,18 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>>       }
>>       phys_enc = &wb_enc->base;
>> -    phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
>> -    phys_enc->wb_idx = p->wb_idx;
>> +
>> +    ret = dpu_encoder_phys_init(phys_enc, p);
>> +    if (ret)
>> +        return ERR_PTR(ret);
> 
> same here
> 
>>       dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>> -    phys_enc->parent = p->parent;
>> -    phys_enc->dpu_kms = p->dpu_kms;
>> -    phys_enc->split_role = p->split_role;
>>       phys_enc->intf_mode = INTF_MODE_WB_LINE;
>> -    phys_enc->wb_idx = p->wb_idx;
>> -    phys_enc->enc_spinlock = p->enc_spinlock;
>>       atomic_set(&wb_enc->wbirq_refcount, 0);
>> -    for (i = 0; i < ARRAY_SIZE(phys_enc->irq); i++)
>> -        phys_enc->irq[i] = -EINVAL;
>> -
>> -    atomic_set(&phys_enc->pending_kickoff_cnt, 0);
>> -    atomic_set(&phys_enc->vblank_refcount, 0);
>>       wb_enc->wb_done_timeout_cnt = 0;
>> -    init_waitqueue_head(&phys_enc->pending_kickoff_wq);
>> -    phys_enc->enable_state = DPU_ENC_DISABLED;
>>       DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
>>               phys_enc->wb_idx);

-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dpu: drop duplicated intf/wb indices from encoder structs

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> Remove intf_idx and wb_idx fields from struct dpu_encoder_phys and
> struct dpu_enc_phys_init_params. Set the hw_intf and hw_wb directly and
> use them to get the instance index.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

 From whatever I can see, this will not affect functionality of intf or 
wb and cleans it up well , so I am fine with this. Hence,

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

One minor comment/question.

<snipped other parts>

> @@ -761,7 +761,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   	struct dpu_encoder_phys_cmd *cmd_enc = NULL;
>   	int ret = 0;
>   
> -	DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);

Was it intentional to drop the index in this log?

> +	DPU_DEBUG("intf\n");
>   
>   	cmd_enc = kzalloc(sizeof(*cmd_enc), GFP_KERNEL);

Re: [PATCH 4/7] drm/msm/dpu: drop duplicated intf/wb indices from encoder structs

[Dmitry Baryshkov]

On Wed, 3 May 2023 at 02:04, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> > Remove intf_idx and wb_idx fields from struct dpu_encoder_phys and
> > struct dpu_enc_phys_init_params. Set the hw_intf and hw_wb directly and
> > use them to get the instance index.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
>
>  From whatever I can see, this will not affect functionality of intf or
> wb and cleans it up well , so I am fine with this. Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> One minor comment/question.
>
> <snipped other parts>
>
> > @@ -761,7 +761,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
> >       struct dpu_encoder_phys_cmd *cmd_enc = NULL;
> >       int ret = 0;
> >
> > -     DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
>
> Was it intentional to drop the index in this log?

We don't have p->intf_idx at this point. I think we can use
p->hw_intf->idx instead, I'll fix that for v2.

>
> > +     DPU_DEBUG("intf\n");
> >
> >       cmd_enc = kzalloc(sizeof(*cmd_enc), GFP_KERNEL);



-- 
With best wishes
Dmitry

Re: [PATCH 4/7] drm/msm/dpu: drop duplicated intf/wb indices from encoder structs

[Abhinav Kumar]

On 5/2/2023 4:15 PM, Dmitry Baryshkov wrote:
> On Wed, 3 May 2023 at 02:04, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>> Remove intf_idx and wb_idx fields from struct dpu_encoder_phys and
>>> struct dpu_enc_phys_init_params. Set the hw_intf and hw_wb directly and
>>> use them to get the instance index.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>
>>   From whatever I can see, this will not affect functionality of intf or
>> wb and cleans it up well , so I am fine with this. Hence,
>>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>> One minor comment/question.
>>
>> <snipped other parts>
>>
>>> @@ -761,7 +761,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>>        struct dpu_encoder_phys_cmd *cmd_enc = NULL;
>>>        int ret = 0;
>>>
>>> -     DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
>>
>> Was it intentional to drop the index in this log?
> 
> We don't have p->intf_idx at this point. I think we can use
> p->hw_intf->idx instead, I'll fix that for v2.
> 

Yes, I was aware that. In all other logs, intf_idx was replaced with 
hw_intf->idx except this one. So I was not sure if it was intentional or 
just removed accidentally.

>>
>>> +     DPU_DEBUG("intf\n");
>>>
>>>        cmd_enc = kzalloc(sizeof(*cmd_enc), GFP_KERNEL);
> 
> 
> 

Re: [PATCH 5/7] drm/msm/dpu: inline dpu_encoder_get_wb()

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> The function dpu_encoder_get_wb() returns controller_id if the
> corresponding WB is present in the catalog. We can inline this function
> and rely on dpu_rm_get_wb() returning NULL for indices for which the
> WB is not present on the device.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++-------------------
>   1 file changed, 2 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4c85cbb030e4..507ff3f88c67 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
>   	return INTF_MAX;
>   }
>   
> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg *catalog,
> -		enum dpu_intf_type type, u32 controller_id)
> -{
> -	int i = 0;
> -
> -	if (type != INTF_WB)
> -		return WB_MAX;
> -
> -	for (i = 0; i < catalog->wb_count; i++) {
> -		if (catalog->wb[i].id == controller_id)
> -			return catalog->wb[i].id;
> -	}
> -
> -	return WB_MAX;
> -}
> -
>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *phy_enc)
>   {
> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   		 */
>   		u32 controller_id = disp_info->h_tile_instance[i];
>   		enum dpu_intf intf_idx;
> -		enum dpu_wb wb_idx;
>   
>   		if (disp_info->num_of_h_tiles > 1) {
>   			if (i == 0)
> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   							    disp_info->intf_type,
>   							    controller_id);
>   
> -		wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
> -				disp_info->intf_type, controller_id);
> -
>   		if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>   			phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx);
>   
> -		if (wb_idx >= WB_0 && wb_idx < WB_MAX)
> -			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
> +		if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
> +			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id);


 From what I see, with 
https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we 
are dropping those checks from the RM too, so we are going to rely 
totally on entering the values correctly in catalog from now on?

>   
>   		if (!phys_params.hw_intf && !phys_params.hw_wb) {
>   			DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at idx: %d\n", i);

Re: [PATCH 4/7] drm/msm/dpu: drop duplicated intf/wb indices from encoder structs

[Dmitry Baryshkov]

On 03/05/2023 02:19, Abhinav Kumar wrote:
> 
> 
> On 5/2/2023 4:15 PM, Dmitry Baryshkov wrote:
>> On Wed, 3 May 2023 at 02:04, Abhinav Kumar <quic_abhinavk@quicinc.com> 
>> wrote:
>>>
>>>
>>>
>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>> Remove intf_idx and wb_idx fields from struct dpu_encoder_phys and
>>>> struct dpu_enc_phys_init_params. Set the hw_intf and hw_wb directly and
>>>> use them to get the instance index.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>
>>>   From whatever I can see, this will not affect functionality of intf or
>>> wb and cleans it up well , so I am fine with this. Hence,
>>>
>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>> One minor comment/question.
>>>
>>> <snipped other parts>
>>>
>>>> @@ -761,7 +761,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>>>>        struct dpu_encoder_phys_cmd *cmd_enc = NULL;
>>>>        int ret = 0;
>>>>
>>>> -     DPU_DEBUG("intf %d\n", p->intf_idx - INTF_0);
>>>
>>> Was it intentional to drop the index in this log?
>>
>> We don't have p->intf_idx at this point. I think we can use
>> p->hw_intf->idx instead, I'll fix that for v2.
>>
> 
> Yes, I was aware that. In all other logs, intf_idx was replaced with 
> hw_intf->idx except this one. So I was not sure if it was intentional or 
> just removed accidentally.

Most likely it was a leftover from the interim patch where I didn't have 
p->hw_intf. Initially I kept the code which manually set the 
phys->hw_intf (and hw_wb), but then it was just easier to pass that 
through the params and let the caller set it.

> 
>>>
>>>> +     DPU_DEBUG("intf\n");
>>>>
>>>>        cmd_enc = kzalloc(sizeof(*cmd_enc), GFP_KERNEL);
>>
>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 5/7] drm/msm/dpu: inline dpu_encoder_get_wb()

[Dmitry Baryshkov]

On 03/05/2023 02:51, Abhinav Kumar wrote:
> 
> 
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>> The function dpu_encoder_get_wb() returns controller_id if the
>> corresponding WB is present in the catalog. We can inline this function
>> and rely on dpu_rm_get_wb() returning NULL for indices for which the
>> WB is not present on the device.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++-------------------
>>   1 file changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4c85cbb030e4..507ff3f88c67 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1277,22 +1277,6 @@ static enum dpu_intf dpu_encoder_get_intf(const 
>> struct dpu_mdss_cfg *catalog,
>>       return INTF_MAX;
>>   }
>> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg 
>> *catalog,
>> -        enum dpu_intf_type type, u32 controller_id)
>> -{
>> -    int i = 0;
>> -
>> -    if (type != INTF_WB)
>> -        return WB_MAX;
>> -
>> -    for (i = 0; i < catalog->wb_count; i++) {
>> -        if (catalog->wb[i].id == controller_id)
>> -            return catalog->wb[i].id;
>> -    }
>> -
>> -    return WB_MAX;
>> -}
>> -
>>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>           struct dpu_encoder_phys *phy_enc)
>>   {
>> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>            */
>>           u32 controller_id = disp_info->h_tile_instance[i];
>>           enum dpu_intf intf_idx;
>> -        enum dpu_wb wb_idx;
>>           if (disp_info->num_of_h_tiles > 1) {
>>               if (i == 0)
>> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>                                   disp_info->intf_type,
>>                                   controller_id);
>> -        wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>> -                disp_info->intf_type, controller_id);
>> -
>>           if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>>               phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> intf_idx);
>> -        if (wb_idx >= WB_0 && wb_idx < WB_MAX)
>> -            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
>> +        if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>> +            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>> controller_id);
> 
> 
>  From what I see, with 
> https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we 
> are dropping those checks from the RM too, so we are going to rely 
> totally on entering the values correctly in catalog from now on?

Yes. I see no reason to mistrust the kernel data itself.

> 
>>           if (!phys_params.hw_intf && !phys_params.hw_wb) {
>>               DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at 
>> idx: %d\n", i);

-- 
With best wishes
Dmitry

Re: [PATCH 6/7] drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> There is little sense to get intf index just to call dpu_rm_get_intf()
> on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------
>   1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 507ff3f88c67..b35e92c658ad 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1259,22 +1259,23 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
>   	mutex_unlock(&dpu_enc->enc_lock);
>   }
>   
> -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
> +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
> +		struct dpu_rm *dpu_rm,
>   		enum dpu_intf_type type, u32 controller_id)
>   {
>   	int i = 0;
>   
>   	if (type == INTF_WB)
> -		return INTF_MAX;
> +		return NULL;
>   
>   	for (i = 0; i < catalog->intf_count; i++) {
>   		if (catalog->intf[i].type == type
>   		    && catalog->intf[i].controller_id == controller_id) {
> -			return catalog->intf[i].id;
> +			return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id);
>   		}

Why has the for loop been retained in this function but not for 
writeback? is there any difference? Doesnt looks like there needs to be.

>   	}
>   
> -	return INTF_MAX;
> +	return NULL;
>   }
>   
>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   		 * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
>   		 */
>   		u32 controller_id = disp_info->h_tile_instance[i];
> -		enum dpu_intf intf_idx;
>   
>   		if (disp_info->num_of_h_tiles > 1) {
>   			if (i == 0)
> @@ -2258,12 +2258,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   		DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>   				i, controller_id, phys_params.split_role);
>   
> -		intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> -							    disp_info->intf_type,
> -							    controller_id);
> -
> -		if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
> -			phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, intf_idx);
> +		phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, &dpu_kms->rm,
> +							   disp_info->intf_type,
> +							   controller_id);
>   
>   		if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>   			phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, controller_id);
> @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   			DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
>   			break;
>   		}
> -
unnecessary change?
>   	}
>   
>   	mutex_unlock(&dpu_enc->enc_lock);

Re: [PATCH 5/7] drm/msm/dpu: inline dpu_encoder_get_wb()

[Abhinav Kumar]

On 5/2/2023 4:54 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 02:51, Abhinav Kumar wrote:
>>
>>
>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>> The function dpu_encoder_get_wb() returns controller_id if the
>>> corresponding WB is present in the catalog. We can inline this function
>>> and rely on dpu_rm_get_wb() returning NULL for indices for which the
>>> WB is not present on the device.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 ++-------------------
>>>   1 file changed, 2 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 4c85cbb030e4..507ff3f88c67 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -1277,22 +1277,6 @@ static enum dpu_intf 
>>> dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
>>>       return INTF_MAX;
>>>   }
>>> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg 
>>> *catalog,
>>> -        enum dpu_intf_type type, u32 controller_id)
>>> -{
>>> -    int i = 0;
>>> -
>>> -    if (type != INTF_WB)
>>> -        return WB_MAX;
>>> -
>>> -    for (i = 0; i < catalog->wb_count; i++) {
>>> -        if (catalog->wb[i].id == controller_id)
>>> -            return catalog->wb[i].id;
>>> -    }
>>> -
>>> -    return WB_MAX;
>>> -}
>>> -
>>>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>>           struct dpu_encoder_phys *phy_enc)
>>>   {
>>> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>            */
>>>           u32 controller_id = disp_info->h_tile_instance[i];
>>>           enum dpu_intf intf_idx;
>>> -        enum dpu_wb wb_idx;
>>>           if (disp_info->num_of_h_tiles > 1) {
>>>               if (i == 0)
>>> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>                                   disp_info->intf_type,
>>>                                   controller_id);
>>> -        wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>>> -                disp_info->intf_type, controller_id);
>>> -
>>>           if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>>>               phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>> intf_idx);
>>> -        if (wb_idx >= WB_0 && wb_idx < WB_MAX)
>>> -            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
>>> +        if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>>> +            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>>> controller_id);
>>
>>
>>  From what I see, with 
>> https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 we 
>> are dropping those checks from the RM too, so we are going to rely 
>> totally on entering the values correctly in catalog from now on?
> 
> Yes. I see no reason to mistrust the kernel data itself.

Alright, if thats the overall plan, this change itself is fine.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
>>
>>>           if (!phys_params.hw_intf && !phys_params.hw_wb) {
>>>               DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned at 
>>> idx: %d\n", i);
> 

Re: [PATCH 6/7] drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()

[Dmitry Baryshkov]

On 03/05/2023 02:57, Abhinav Kumar wrote:
> 
> 
> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>> There is little sense to get intf index just to call dpu_rm_get_intf()
>> on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------
>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 507ff3f88c67..b35e92c658ad 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -1259,22 +1259,23 @@ static void 
>> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
>>       mutex_unlock(&dpu_enc->enc_lock);
>>   }
>> -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg 
>> *catalog,
>> +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct 
>> dpu_mdss_cfg *catalog,
>> +        struct dpu_rm *dpu_rm,
>>           enum dpu_intf_type type, u32 controller_id)
>>   {
>>       int i = 0;
>>       if (type == INTF_WB)
>> -        return INTF_MAX;
>> +        return NULL;
>>       for (i = 0; i < catalog->intf_count; i++) {
>>           if (catalog->intf[i].type == type
>>               && catalog->intf[i].controller_id == controller_id) {
>> -            return catalog->intf[i].id;
>> +            return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id);
>>           }
> 
> Why has the for loop been retained in this function but not for 
> writeback? is there any difference? Doesnt looks like there needs to be.

For writeback we always return controller_id (WB_2). For interfaces we 
have to map type+controller_id to the INTF instance.

> 
>>       }
>> -    return INTF_MAX;
>> +    return NULL;
>>   }
>>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>> @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>            * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
>>            */
>>           u32 controller_id = disp_info->h_tile_instance[i];
>> -        enum dpu_intf intf_idx;
>>           if (disp_info->num_of_h_tiles > 1) {
>>               if (i == 0)
>> @@ -2258,12 +2258,9 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>           DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>                   i, controller_id, phys_params.split_role);
>> -        intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>> -                                disp_info->intf_type,
>> -                                controller_id);
>> -
>> -        if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>> -            phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> intf_idx);
>> +        phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, 
>> &dpu_kms->rm,
>> +                               disp_info->intf_type,
>> +                               controller_id);
>>           if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>>               phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>> controller_id);
>> @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>               DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
>>               break;
>>           }
>> -
> unnecessary change?


ack, it sneaked in. I'll drop it for v2.

>>       }
>>       mutex_unlock(&dpu_enc->enc_lock);

-- 
With best wishes
Dmitry

Re: [PATCH 5/7] drm/msm/dpu: inline dpu_encoder_get_wb()

[Dmitry Baryshkov]

On 03/05/2023 02:58, Abhinav Kumar wrote:
> 
> 
> On 5/2/2023 4:54 PM, Dmitry Baryshkov wrote:
>> On 03/05/2023 02:51, Abhinav Kumar wrote:
>>>
>>>
>>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>>> The function dpu_encoder_get_wb() returns controller_id if the
>>>> corresponding WB is present in the catalog. We can inline this function
>>>> and rely on dpu_rm_get_wb() returning NULL for indices for which the
>>>> WB is not present on the device.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 24 
>>>> ++-------------------
>>>>   1 file changed, 2 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 4c85cbb030e4..507ff3f88c67 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -1277,22 +1277,6 @@ static enum dpu_intf 
>>>> dpu_encoder_get_intf(const struct dpu_mdss_cfg *catalog,
>>>>       return INTF_MAX;
>>>>   }
>>>> -static enum dpu_wb dpu_encoder_get_wb(const struct dpu_mdss_cfg 
>>>> *catalog,
>>>> -        enum dpu_intf_type type, u32 controller_id)
>>>> -{
>>>> -    int i = 0;
>>>> -
>>>> -    if (type != INTF_WB)
>>>> -        return WB_MAX;
>>>> -
>>>> -    for (i = 0; i < catalog->wb_count; i++) {
>>>> -        if (catalog->wb[i].id == controller_id)
>>>> -            return catalog->wb[i].id;
>>>> -    }
>>>> -
>>>> -    return WB_MAX;
>>>> -}
>>>> -
>>>>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>>>           struct dpu_encoder_phys *phy_enc)
>>>>   {
>>>> @@ -2261,7 +2245,6 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>            */
>>>>           u32 controller_id = disp_info->h_tile_instance[i];
>>>>           enum dpu_intf intf_idx;
>>>> -        enum dpu_wb wb_idx;
>>>>           if (disp_info->num_of_h_tiles > 1) {
>>>>               if (i == 0)
>>>> @@ -2279,14 +2262,11 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>                                   disp_info->intf_type,
>>>>                                   controller_id);
>>>> -        wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>>>> -                disp_info->intf_type, controller_id);
>>>> -
>>>>           if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>>>>               phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>>> intf_idx);
>>>> -        if (wb_idx >= WB_0 && wb_idx < WB_MAX)
>>>> -            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, wb_idx);
>>>> +        if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>>>> +            phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>>>> controller_id);
>>>
>>>
>>>  From what I see, with 
>>> https://patchwork.freedesktop.org/patch/534776/?series=117146&rev=1 
>>> we are dropping those checks from the RM too, so we are going to rely 
>>> totally on entering the values correctly in catalog from now on?
>>
>> Yes. I see no reason to mistrust the kernel data itself.
> 
> Alright, if thats the overall plan, this change itself is fine.

Yes, I think this is what we discussed some time ago for UBWC and QSEED 
programming.

> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>>>
>>>>           if (!phys_params.hw_intf && !phys_params.hw_wb) {
>>>>               DPU_ERROR_ENC(dpu_enc, "no intf or wb block assigned 
>>>> at idx: %d\n", i);
>>

-- 
With best wishes
Dmitry

Re: [PATCH 7/7] drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set

[Dmitry Baryshkov]

On 01/05/2023 02:57, Dmitry Baryshkov wrote:
> The atomic_mode_set() callback only sets the phys_enc's IRQ data. As the
> INTF and WB are statically allocated to each encoder/phys_enc, drop the
> atomic_mode_set callback and set the IRQs during encoder init.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Please ignore this for now, I'd like to take another look on my own. I 
didn't test the CMD panels and they are going to break with this change.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  2 --
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  5 -----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 20 +++++--------------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 13 ++----------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   | 11 +---------
>   5 files changed, 8 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index b35e92c658ad..509b4fc7dbc5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1106,8 +1106,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>   
>   		phys->cached_mode = crtc_state->adjusted_mode;
> -		if (phys->ops.atomic_mode_set)
> -			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>   	}
>   }
>   
> 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 1c096d9390d0..67c4b4e0975d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -68,8 +68,6 @@ struct dpu_encoder_phys;
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @atomic_mode_set:		DRM Call. Set a DRM mode.
> - *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
>    * @atomic_check:		DRM Call. Atomic check new DRM state.
> @@ -97,9 +95,6 @@ struct dpu_encoder_phys_ops {
>   			struct dentry *debugfs_root);
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
> -			struct drm_crtc_state *crtc_state,
> -			struct drm_connector_state *conn_state);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
>   	int (*atomic_check)(struct dpu_encoder_phys *encoder,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 781290f17714..3ad03465acfe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -139,20 +139,6 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>   	dpu_encoder_underrun_callback(phys_enc->parent, phys_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
> -
> -	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
> -
> -	phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> -
> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> -}
> -
>   static int _dpu_encoder_phys_cmd_handle_ppdone_timeout(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -736,7 +722,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>   		struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> @@ -777,6 +762,11 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(
>   
>   	dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_CMD;
> +	phys_enc->irq[INTR_IDX_CTL_START] = phys_enc->hw_ctl->caps->intr_start;
> +	phys_enc->irq[INTR_IDX_PINGPONG] = phys_enc->hw_pp->caps->intr_done;
> +	phys_enc->irq[INTR_IDX_RDPTR] = phys_enc->hw_pp->caps->intr_rdptr;
> +	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> +
>   	cmd_enc->stream_sel = 0;
>   
>   	atomic_set(&cmd_enc->pending_vblank_cnt, 0);
> 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 f02ff8f43f47..cf9a0128ff71 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
> @@ -348,16 +348,6 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>   	return phys_enc->split_role != ENC_ROLE_SOLO;
>   }
>   
> -static void dpu_encoder_phys_vid_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
> -
> -	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
> -}
> -
>   static int dpu_encoder_phys_vid_control_vblank_irq(
>   		struct dpu_encoder_phys *phys_enc,
>   		bool enable)
> @@ -679,7 +669,6 @@ static int dpu_encoder_phys_vid_get_frame_count(
>   static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;
> @@ -720,6 +709,8 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(
>   
>   	dpu_encoder_phys_vid_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_VIDEO;
> +	phys_enc->irq[INTR_IDX_VSYNC] = phys_enc->hw_intf->cap->intr_vsync;
> +	phys_enc->irq[INTR_IDX_UNDERRUN] = phys_enc->hw_intf->cap->intr_underrun;
>   
>   	DPU_DEBUG_VIDENC(phys_enc, "created intf idx:%d\n", p->hw_intf->idx);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> index b058c69e8778..27479334176b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
> @@ -398,15 +398,6 @@ static void dpu_encoder_phys_wb_irq_ctrl(
>   		dpu_core_irq_unregister_callback(phys->dpu_kms, phys->irq[INTR_IDX_WB_DONE]);
>   }
>   
> -static void dpu_encoder_phys_wb_atomic_mode_set(
> -		struct dpu_encoder_phys *phys_enc,
> -		struct drm_crtc_state *crtc_state,
> -		struct drm_connector_state *conn_state)
> -{
> -
> -	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
> -}
> -
>   static void _dpu_encoder_phys_wb_handle_wbdone_timeout(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -656,7 +647,6 @@ static bool dpu_encoder_phys_wb_is_valid_for_commit(struct dpu_encoder_phys *phy
>   static void dpu_encoder_phys_wb_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_wb_is_master;
> -	ops->atomic_mode_set = dpu_encoder_phys_wb_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_wb_enable;
>   	ops->disable = dpu_encoder_phys_wb_disable;
>   	ops->destroy = dpu_encoder_phys_wb_destroy;
> @@ -707,6 +697,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
>   
>   	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
>   	phys_enc->intf_mode = INTF_MODE_WB_LINE;
> +	phys_enc->irq[INTR_IDX_WB_DONE] = phys_enc->hw_wb->caps->intr_wb_done;
>   
>   	atomic_set(&wb_enc->wbirq_refcount, 0);
>   

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH 6/7] drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()

[Abhinav Kumar]

On 5/2/2023 4:58 PM, Dmitry Baryshkov wrote:
> On 03/05/2023 02:57, Abhinav Kumar wrote:
>>
>>
>> On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
>>> There is little sense to get intf index just to call dpu_rm_get_intf()
>>> on it. Move dpu_rm_get_intf() call to dpu_encoder_get_intf() function.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 20 ++++++++------------
>>>   1 file changed, 8 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 507ff3f88c67..b35e92c658ad 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -1259,22 +1259,23 @@ static void 
>>> dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
>>>       mutex_unlock(&dpu_enc->enc_lock);
>>>   }
>>> -static enum dpu_intf dpu_encoder_get_intf(const struct dpu_mdss_cfg 
>>> *catalog,
>>> +static struct dpu_hw_intf *dpu_encoder_get_intf(const struct 
>>> dpu_mdss_cfg *catalog,
>>> +        struct dpu_rm *dpu_rm,
>>>           enum dpu_intf_type type, u32 controller_id)
>>>   {
>>>       int i = 0;
>>>       if (type == INTF_WB)
>>> -        return INTF_MAX;
>>> +        return NULL;
>>>       for (i = 0; i < catalog->intf_count; i++) {
>>>           if (catalog->intf[i].type == type
>>>               && catalog->intf[i].controller_id == controller_id) {
>>> -            return catalog->intf[i].id;
>>> +            return dpu_rm_get_intf(dpu_rm, catalog->intf[i].id);
>>>           }
>>
>> Why has the for loop been retained in this function but not for 
>> writeback? is there any difference? Doesnt looks like there needs to be.
> 
> For writeback we always return controller_id (WB_2). For interfaces we 
> have to map type+controller_id to the INTF instance.

Ah correct, got it now. With that minor comment fixed from below,

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

> 
>>
>>>       }
>>> -    return INTF_MAX;
>>> +    return NULL;
>>>   }
>>>   void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>> @@ -2244,7 +2245,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>            * h_tile_instance_ids[2] = {1, 0}; DSI1 = left, DSI0 = right
>>>            */
>>>           u32 controller_id = disp_info->h_tile_instance[i];
>>> -        enum dpu_intf intf_idx;
>>>           if (disp_info->num_of_h_tiles > 1) {
>>>               if (i == 0)
>>> @@ -2258,12 +2258,9 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>           DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>                   i, controller_id, phys_params.split_role);
>>> -        intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>> -                                disp_info->intf_type,
>>> -                                controller_id);
>>> -
>>> -        if (intf_idx >= INTF_0 && intf_idx < INTF_MAX)
>>> -            phys_params.hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>>> intf_idx);
>>> +        phys_params.hw_intf = dpu_encoder_get_intf(dpu_kms->catalog, 
>>> &dpu_kms->rm,
>>> +                               disp_info->intf_type,
>>> +                               controller_id);
>>>           if (disp_info->intf_type == INTF_WB && controller_id < WB_MAX)
>>>               phys_params.hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>>> controller_id);
>>> @@ -2287,7 +2284,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>               DPU_ERROR_ENC(dpu_enc, "failed to add phys encs\n");
>>>               break;
>>>           }
>>> -
>> unnecessary change?
> 
> 
> ack, it sneaked in. I'll drop it for v2.
> 
>>>       }
>>>       mutex_unlock(&dpu_enc->enc_lock);
> 

Re: [PATCH 0/7] drm/msm/dpu: simplify DPU encoder init

[Abhinav Kumar]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> Rework dpu_encoder initialization code, simplifying calling sequences
> and separating common init parts.

Please mention that your series was made on top of 
https://patchwork.freedesktop.org/series/116530/.

Figured it out when I tried to apply it to my branch to test.

Validated writeback just in case with this, hence please use

Tested-by: Abhinav Kumar <quic_abhinavk@quicinc.com> # sc7280

> 
> Dmitry Baryshkov (7):
>    drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
>    drm/msm/dpu: drop dpu_encoder_early_unregister
>    drm/msm/dpu: separate common function to init physical encoder
>    drm/msm/dpu: drop duplicated intf/wb indices from encoder structs
>    drm/msm/dpu: inline dpu_encoder_get_wb()
>    drm/msm/dpu: call dpu_rm_get_intf() from dpu_encoder_get_intf()
>    drm/msm/dpu: drop dpu_encoder_phys_ops.atomic_mode_set
> 
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 190 ++++++++----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  14 +-
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  20 +-
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  55 ++---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  35 +---
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_wb.c   |  38 +---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  87 +++-----
>   7 files changed, 155 insertions(+), 284 deletions(-)
>