Re: [DPU PATCH 5/6] drm/msm: hook up DPU with upstream DSI

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

 



On 2018-04-19 08:38, Sean Paul wrote:
On Mon, Apr 16, 2018 at 11:22:20AM -0700, Jeykumar Sankaran wrote:
Switch DPU from dsi-staging to upstream dsi driver. To make
the switch atomic, this change includes:
- remove dpu connector layers
- clean up dpu connector dependencies in encoder/crtc
- compile out writeback and display port drivers
- compile out dsi-staging driver (separate patch submitted to
  remove the driver)
- adapt upstream device hierarchy

Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx>
---
 .../config/arm64/chromiumos-arm64.flavour.config   |    4 +-
 .../arm64/chromiumos-qualcomm.flavour.config       |    4 +-

These files aren't in upstream.

<snip />

+
+struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
+		int drm_enc_mode)
+{
+	struct dpu_encoder_virt *dpu_enc = NULL;
+
+	dpu_enc = kzalloc(sizeof(*dpu_enc), GFP_KERNEL);

You should probably use devm_kzalloc.

+	if (!dpu_enc)
+		return ERR_PTR(ENOMEM);
+
+	drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
+			drm_enc_mode, NULL);

Check the return value?
<snip />

-#ifdef CONFIG_DRM_MSM_DSI_STAGING
 static void _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
-				    struct dpu_kms *dpu_kms,
-				    unsigned max_encoders)
+				    struct dpu_kms *dpu_kms)
 {
-	static const struct dpu_connector_ops dsi_ops = {
-		.post_init =  dsi_conn_post_init,
-		.detect =     dsi_conn_detect,
-		.get_modes =  dsi_connector_get_modes,
-		.put_modes =  dsi_connector_put_modes,
-		.mode_valid = dsi_conn_mode_valid,
-		.get_info =   dsi_display_get_info,
-		.set_backlight = dsi_display_set_backlight,
-		.soft_reset   = dsi_display_soft_reset,
-		.pre_kickoff  = dsi_conn_pre_kickoff,
-		.clk_ctrl = dsi_display_clk_ctrl,
-		.set_power = dsi_display_set_power,
-		.get_mode_info = dsi_conn_get_mode_info,
-		.get_dst_format = dsi_display_get_dst_format,
-		.post_kickoff = dsi_conn_post_kickoff
-	};
-	struct msm_display_info info;
-	struct drm_encoder *encoder;
-	void *display, *connector;
+	struct drm_encoder *encoder = NULL;
 	int i, rc;

-	/* dsi */
-	for (i = 0; i < dpu_kms->dsi_display_count &&
-		priv->num_encoders < max_encoders; ++i) {
-		display = dpu_kms->dsi_displays[i];
-		encoder = NULL;
+	/*TODO: Support two independent DSI connectors */
+	encoder = dpu_encoder_init(dev, DRM_MODE_CONNECTOR_DSI);
+	if (IS_ERR_OR_NULL(encoder)) {
+		DPU_ERROR("encoder init failed for dsi %d\n", i);

Is i meaningful here? It seems like it's uninitialized...

+		return;
+	}

-		memset(&info, 0x0, sizeof(info));
-		rc = dsi_display_get_info(&info, display);
-		if (rc) {
-			DPU_ERROR("dsi get_info %d failed\n", i);
-			continue;
-		}
+	priv->encoders[priv->num_encoders++] = encoder;

-		encoder = dpu_encoder_init(dev, &info);
-		if (IS_ERR_OR_NULL(encoder)) {
-			DPU_ERROR("encoder init failed for dsi %d\n", i);
-			continue;
+	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+		if (!priv->dsi[i]) {
+			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
+			return;
 		}

-		rc = dsi_display_drm_bridge_init(display, encoder);
+		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
-			DPU_ERROR("dsi bridge %d init failed, %d\n", i,
rc);
-			dpu_encoder_destroy(encoder);
+			DPU_ERROR("modeset_init failed for dsi[%d]\n", i);

Printing rc would be nice here, same for above.

 			continue;
 		}
-
-		connector = dpu_connector_init(dev,
-					encoder,
-					0,
-					display,
-					&dsi_ops,
-					DRM_CONNECTOR_POLL_HPD,
-					DRM_MODE_CONNECTOR_DSI);
-		if (connector) {
-			priv->encoders[priv->num_encoders++] = encoder;
-		} else {
-			DPU_ERROR("dsi %d connector init failed\n", i);
-			dsi_display_drm_bridge_deinit(display);
-			dpu_encoder_destroy(encoder);
-		}
 	}
 }
-#endif
+

 #ifdef CONFIG_DRM_MSM_WRITEBACK
 static void _dpu_kms_initialize_wb(struct drm_device *dev,
 				   struct msm_drm_private *priv,
 				   struct dpu_kms *dpu_kms,
-				   unsigned max_encoders)
+				   unsigned int max_encoders)
 {
 	static const struct dpu_connector_ops wb_ops = {
 		.post_init =    dpu_wb_connector_post_init,
@@ -800,7 +710,7 @@ static void _dpu_kms_initialize_wb(struct drm_device
*dev,
 static void _dpu_kms_initialize_dp(struct drm_device *dev,
 				   struct msm_drm_private *priv,
 				   struct dpu_kms *dpu_kms,
-				   unsigned max_encoders)
+				   unsigned int max_encoders)

These 2 type changes are just diff noise, please remove.

 {
 	static const struct dpu_connector_ops dp_ops = {
 		.post_init  = dp_connector_post_init,
@@ -872,18 +782,7 @@ static void _dpu_kms_setup_displays(struct
drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
 {
-	unsigned max_encoders;
-
-	max_encoders = dpu_kms->dsi_display_count +
dpu_kms->wb_display_count +
-				dpu_kms->dp_display_count;
-	if (max_encoders > ARRAY_SIZE(priv->encoders)) {
-		max_encoders = ARRAY_SIZE(priv->encoders);
-		DPU_ERROR("capping number of displays to %d",
max_encoders);
-	}
-
-#ifdef CONFIG_DRM_MSM_DSI_STAGING
-	_dpu_kms_initialize_dsi(dev, priv, dpu_kms, max_encoders);
-#endif
+	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);

 #ifdef CONFIG_DRM_MSM_WRITEBACK
 	_dpu_kms_initialize_wb(dev, priv, dpu_kms, max_encoders);
@@ -1521,6 +1420,7 @@ static int dpu_kms_atomic_check(struct msm_kms
*kms,
 		dpu_kms->aspace[domain] : NULL;
 }

+#ifdef CONFIG_DRM_MSM_DISPLAYPORT

This seems suspicious. Why different behavior depending on DisplayPort?
The
function loops through all connectors, so this is something that should
work
regardless of whether DP is connected.

The reason why this kms_func hook was introduced is to notify poll enabled
connectors (DP or HDMI) to send the HPD event notification through
DPU connector hook. Since the DPU connector layer is removed, this call
is meaningless.

To address your comments w.r.t the difference in handling for
non-dsi displays, I am planning to remove their handling codes from DPU but
retaining respective driver files.

<snip />
@@ -2001,6 +1891,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 		goto power_error;
 	}

+	if (dpu_kms->aspace[0])
+		kms->aspace = dpu_kms->aspace[0];

In future, stuff like this should be in a separate patch, instead of
hidden in
this huge glob. It seems like it's needed for the dsi driver, but I almost
missed it. Splitting this isolated stuff out makes it much easier for
reviewers
since you can explain why it's needed in the commit message vs me having
to look
through the code to infer its usage.

Sean

Agreed. Let me try separating it out and move it down to the base.

<snip />

Will take care of rest of the comments in v2.
--
Jeykumar S
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[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