Skip to contentSkip to conversation

drm/msm/dpu: add dpu encoder uninit

PATCH In review Jeykumar Sankaran 4 participants 7 messages Nov 16, 2018 – Nov 19, 2018 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/2 drm/msm/dpu: add dpu encoder uninit

Add encoder interface to release dpu encoder
on mode_init failures in kms.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +10 -2
index dd7ab85..b253165 100644
@@ -422,6 +422,15 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
 	}
 }
 
+void dpu_encoder_uninit(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	drm_encoder_cleanup(drm_enc);
+
+	kfree(dpu_enc);
+}
+
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -453,10 +462,9 @@ static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 	dpu_enc->num_phys_encs = 0;
 	mutex_unlock(&dpu_enc->enc_lock);
 
-	drm_encoder_cleanup(drm_enc);
 	mutex_destroy(&dpu_enc->enc_lock);
 
-	kfree(dpu_enc);
+	dpu_encoder_uninit(drm_enc);
 }
 
 void dpu_encoder_helper_split_config(
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +6 -0
index 9dbf38f..60b88bd 100644
@@ -142,6 +142,12 @@ struct drm_encoder *dpu_encoder_init(
 		int drm_enc_mode);
 
 /**
+ * dpu_encoder_uninit - uninitialize virtual encoder object
+ * @drm_enc:  Pointer to drm encoder
+ */
+void dpu_encoder_uninit(struct drm_encoder *drm_enc);
+
+/**
  * dpu_encoder_setup - setup dpu_encoder for the display probed
  * @dev:		Pointer to drm device structure
  * @enc:		Pointer to the drm_encoder
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

PATCH 2/2 drm/msm/dpu: add display port support in DPU

Add display port support in DPU by creating hooks
for DP encoder enumeration and encoder mode
initialization.

This change is based on the SDM845 Display port
driver changes[1].

[1] https://lwn.net/Articles/768265/

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  3 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 49 +++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 6 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +3 -0
index b253165..e9c7edc6 100644
@@ -2048,6 +2048,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 	case DRM_MODE_ENCODER_DSI:
 		intf_type = INTF_DSI;
 		break;
+	case DRM_MODE_ENCODER_TMDS:
+		intf_type = INTF_DP;
+		break;
 	default:
 		DPU_ERROR_ENC(dpu_enc, "unsupported display interface type\n");
 		return -EINVAL;
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +43 -6

68 lines not on this page · raw message

Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw

Re: [PATCH 2/2] drm/msm/dpu: add display port support in DPU

Quoted 39 lines · Jeykumar Sankaran

Add display port support in DPU by creating hooks
for DP encoder enumeration and encoder mode
initialization.

This change is based on the SDM845 Display port
driver changes[1].

[1] https://lwn.net/Articles/768265/

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  3 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 49 +++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 6 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +3 -0
index b253165..e9c7edc6 100644
@@ -2048,6 +2048,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 	case DRM_MODE_ENCODER_DSI:
 		intf_type = INTF_DSI;
 		break;
+	case DRM_MODE_ENCODER_TMDS:
+		intf_type = INTF_DP;
+		break;
 	default:
 		DPU_ERROR_ENC(dpu_enc, "unsupported display interface type\n");
 		return -EINVAL;
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +2 -0
index 985c855..b823a37 100644
@@ -473,6 +473,31 @@ static void _dpu_kms_initialize_dsi(struct drm_device *dev,
 	}
 }
 
+static void _dpu_kms_initialize_displayport(struct drm_device *dev,
+					    struct msm_drm_private *priv,

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void _dpu_kms_initialize_dsi(struct drm_device *dev, · line 487

Quoted 10 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +10 -0
+					    struct dpu_kms *dpu_kms)
+{
+	struct drm_encoder *encoder = NULL;
+	int rc;
+
+	if (!priv->dp)
+		return;
+
+	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
+	if (IS_ERR_OR_NULL(encoder)) {

dpu_encoder_init() only returns valid pointer or ERR_PTR() so a Only IS_ERR() is needed.

Quoted 42 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +23 -5
+		DPU_ERROR("encoder init failed for dsi display\n");
+		return;
+	}
+
+	priv->encoders[priv->num_encoders++] = encoder;
+
+	rc = msm_dp_modeset_init(priv->dp, dev, encoder);
+	if (rc) {
+		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
+		dpu_encoder_uninit(encoder);
+	}
+}
+
 /**
  * _dpu_kms_setup_displays - create encoders, bridges and connectors
  *                           for underlying displays
@@ -487,6 +512,8 @@ static void _dpu_kms_setup_displays(struct drm_device *dev,
 {
 	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
 
+	_dpu_kms_initialize_displayport(dev, priv, dpu_kms);
+
 	/**
 	 * Extend this function to initialize other
 	 * types of displays
@@ -723,13 +750,23 @@ static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
 	info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE :
 			MSM_DISPLAY_CAP_VID_MODE;
 
-	/* TODO: No support for DSI swap */
-	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
-		if (priv->dsi[i]) {
-			info.h_tile_instance[info.num_of_h_tiles] = i;
-			info.num_of_h_tiles++;
+	switch (info.intf_type) {
+	case DRM_MODE_ENCODER_DSI:
+		/* TODO: No support for DSI swap */
+		for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
+			if (priv->dsi[i]) {
+				info.h_tile_instance[info.num_of_h_tiles] = i;
+				info.num_of_h_tiles++;
+			}

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void _dpu_kms_set_encoder_mode(struct msm_kms *kms, · line 769

Quoted 10 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +8 -1
 		}
-	}
+		break;
+	case DRM_MODE_ENCODER_TMDS:
+		info.num_of_h_tiles = 1;
+		break;
+	default:
+		DPU_ERROR("Invalid connector type\n");
+		return;
+	};

This is still going to be one of two options, a simple if/else can suffice here and you don't need a log message and a default path.

drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c static void _dpu_kms_set_encoder_mode(struct msm_kms *kms, · line 772

Quoted 3 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +0 -0
 	rc = dpu_encoder_setup(encoder->dev, encoder, &info);
 	if (rc)

Jordan

Signature · 6 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

A message that is not in the archive (sent off-list or not yet received): 1542396142-19534-1-git-send-email-jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

#4 lore raw
Quoted 46 lines · Jeykumar Sankaran

Add encoder interface to release dpu encoder
on mode_init failures in kms.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +10 -2
index dd7ab85..b253165 100644
@@ -422,6 +422,15 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
 	}
 }
 
+void dpu_encoder_uninit(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	drm_encoder_cleanup(drm_enc);
+
+	kfree(dpu_enc);
+}
+
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -453,10 +462,9 @@ static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 	dpu_enc->num_phys_encs = 0;
 	mutex_unlock(&dpu_enc->enc_lock);
 
-	drm_encoder_cleanup(drm_enc);
 	mutex_destroy(&dpu_enc->enc_lock);
 
-	kfree(dpu_enc);
+	dpu_encoder_uninit(drm_enc);
 }
 
 void dpu_encoder_helper_split_config(
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +0 -0
index 9dbf38f..60b88bd 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h struct drm_encoder *dpu_encoder_init( · line 148

Quoted 7 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +4 -0
@@ -142,6 +142,12 @@ struct drm_encoder *dpu_encoder_init(
 		int drm_enc_mode);
 
 /**
+ * dpu_encoder_uninit - uninitialize virtual encoder object
+ * @drm_enc:  Pointer to drm encoder
+ */
+void dpu_encoder_uninit(struct drm_encoder *drm_enc);

Just make it static?

Quoted 9 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +2 -0
+
+/**
  * dpu_encoder_setup - setup dpu_encoder for the display probed
  * @dev:		Pointer to drm device structure
  * @enc:		Pointer to the drm_encoder
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#5 lore raw
Quoted 49 lines · Sean Paul
Quoted 48 lines · Jeykumar Sankaran

Add encoder interface to release dpu encoder
on mode_init failures in kms.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +10 -2
index dd7ab85..b253165 100644
@@ -422,6 +422,15 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,
 	}
 }
 
+void dpu_encoder_uninit(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	drm_encoder_cleanup(drm_enc);
+
+	kfree(dpu_enc);
+}
+
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -453,10 +462,9 @@ static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 	dpu_enc->num_phys_encs = 0;
 	mutex_unlock(&dpu_enc->enc_lock);
 
-	drm_encoder_cleanup(drm_enc);
 	mutex_destroy(&dpu_enc->enc_lock);
 
-	kfree(dpu_enc);
+	dpu_encoder_uninit(drm_enc);
 }
 
 void dpu_encoder_helper_split_config(
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +0 -0
index 9dbf38f..60b88bd 100644
@@ -142,6 +142,12 @@ struct drm_encoder *dpu_encoder_init(
 		int drm_enc_mode);
 
Quoted 7 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h struct drm_encoder *dpu_encoder_init( · line 148

Quoted 5 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +4 -0
 /**
+ * dpu_encoder_uninit - uninitialize virtual encoder object
+ * @drm_enc:  Pointer to drm encoder
+ */
+void dpu_encoder_uninit(struct drm_encoder *drm_enc);

Just make it static?

I just saw YueHaibing's patch to remove the kfree entirely since dpu_enc is devm_* managed. IMO, that'd be a better solution than this.

Sean

Quoted 13 lines
Quoted 10 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +2 -0
+
+/**
  * dpu_encoder_setup - setup dpu_encoder for the display probed
  * @dev:		Pointer to drm device structure
  * @enc:		Pointer to the drm_encoder
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Signature · 1 line
Sean Paul, Software Engineer, Google / Chromium OS
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#6 lore raw
Quoted 62 lines · Sean Paul
Quoted 11 lines · Sean Paul

Review

Quoted 10 lines · Jeykumar Sankaran

Add encoder interface to release dpu encoder
on mode_init failures in kms.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

Quoted 4 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c void dpu_encoder_get_hw_resources(struct

Quoted 4 lines

index dd7ab85..b253165 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0
@@ -422,6 +422,15 @@ void dpu_encoder_get_hw_resources(struct

drm_encoder *drm_enc,

Quoted 16 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static void dpu_encoder_destroy(struct

Quoted 16 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +9 -0
 	}
 }
 
+void dpu_encoder_uninit(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	drm_encoder_cleanup(drm_enc);
+
+	kfree(dpu_enc);
+}
+
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -453,10 +462,9 @@ static void dpu_encoder_destroy(struct

drm_encoder *drm_enc)

Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -1
 	dpu_enc->num_phys_encs = 0;
 	mutex_unlock(&dpu_enc->enc_lock);
 
-	drm_encoder_cleanup(drm_enc);
 	mutex_destroy(&dpu_enc->enc_lock);
 
Quoted 6 lines

Review

Quoted 6 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -1
-	kfree(dpu_enc);
+	dpu_encoder_uninit(drm_enc);
 }
 
 void dpu_encoder_helper_split_config(
(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h

Quoted 6 lines
Quoted 6 lines

index 9dbf38f..60b88bd 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +0 -0
@@ -142,6 +142,12 @@ struct drm_encoder *dpu_encoder_init(
 		int drm_enc_mode);
 
Quoted 7 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h struct drm_encoder *dpu_encoder_init( · line 148

Quoted 5 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +4 -0
 /**
+ * dpu_encoder_uninit - uninitialize virtual encoder object
+ * @drm_enc:  Pointer to drm encoder
+ */
+void dpu_encoder_uninit(struct drm_encoder *drm_enc);

Just make it static?

Quoted 6 lines

I just saw YueHaibing's patch to remove the kfree entirely since dpu_enc is
devm_* managed. IMO, that'd be a better solution than this.

Sean

I still need the unint here to call drm_encoder_cleanup if the display mode_init fails.

I will rebase the patch on top of https://lkml.org/lkml/2018/11/17/87

Thanks,
Jeykumar S.

Quoted 15 lines
Quoted 9 lines
Quoted 9 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +2 -1
+
+/**
  * dpu_encoder_setup - setup dpu_encoder for the display probed
  * @dev:		Pointer to drm device structure
  * @enc:		Pointer to the drm_encoder
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 5 lines
Quoted 2 lines

a Linux Foundation Collaborative Project

--
Sean Paul, Software Engineer, Google / Chromium OS

Signature · 5 lines
Jeykumar S
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#7 lore raw
In reply to Sean Paul
Quoted 54 lines · Sean Paul

Review

Quoted 10 lines · Jeykumar Sankaran

Add encoder interface to release dpu encoder
on mode_init failures in kms.

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++++++
 2 files changed, 16 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c void dpu_encoder_get_hw_resources(struct

Quoted 4 lines

index dd7ab85..b253165 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0
@@ -422,6 +422,15 @@ void dpu_encoder_get_hw_resources(struct

drm_encoder *drm_enc,

Quoted 16 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +9 -0
 	}
 }
 
+void dpu_encoder_uninit(struct drm_encoder *drm_enc)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+	drm_encoder_cleanup(drm_enc);
+
+	kfree(dpu_enc);
+}
+
 static void dpu_encoder_destroy(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc = NULL;
@@ -453,10 +462,9 @@ static void dpu_encoder_destroy(struct 
Quoted 1 line

drm_encoder

*drm_enc)

Review

Quoted 12 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -2
 	dpu_enc->num_phys_encs = 0;
 	mutex_unlock(&dpu_enc->enc_lock);
 
-	drm_encoder_cleanup(drm_enc);
 	mutex_destroy(&dpu_enc->enc_lock);
 
-	kfree(dpu_enc);
+	dpu_encoder_uninit(drm_enc);
 }
 
 void dpu_encoder_helper_split_config(
(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h

Quoted 6 lines

index 9dbf38f..60b88bd 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +0 -0
@@ -142,6 +142,12 @@ struct drm_encoder *dpu_encoder_init(
 		int drm_enc_mode);
 
Quoted 7 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h struct drm_encoder *dpu_encoder_init( · line 148

Quoted 5 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +4 -0
 /**
+ * dpu_encoder_uninit - uninitialize virtual encoder object
+ * @drm_enc:  Pointer to drm encoder
+ */
+void dpu_encoder_uninit(struct drm_encoder *drm_enc);

Just make it static?

encoder_uninit will be called by dpu_kms in patch 2/2 in case of mode_init failures.

Thanks and Regards,
Jeykumar S.

Quoted 11 lines
Quoted 8 lines
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +2 -1
+
+/**
  * dpu_encoder_setup - setup dpu_encoder for the display probed
  * @dev:		Pointer to drm device structure
  * @enc:		Pointer to the drm_encoder
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

Signature · 5 lines
Jeykumar S
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno