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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
---
  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;
  }
[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux