[PATCH v2 0/7] drm/exynos: add pm runtime support

[PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

This patch series adds pm runtime support for Exynos drm.

Originally, this patch was posted by Gustavo but there was no any
answer about some comments. So I rebased this patch series on top of
exynos-drm-next, removed unnecessary patches and modified wrong macro.

Changelog v2:
- Remove patch 5 and 6.
  . commit callback are already removed so isn't required anymore.
- Remove patch 8 which makes dp clock enabled directly from FIMD.
  . Really not mendatory for FIMD uses DP, and it could be different
    according to Board.
- Modified CONFIG_PM_SLEEP to CONFIG_PM.
  . In case of runtime pm, CONFIG_PM macro should be used instead of
    CONFIG_PM_SLEEP.

Gustavo Padovan (7):
  drm/exynos: do not start enabling DP at bind() phase
  drm/exynos: add pm_runtime to DP
  drm/exynos: add pm_runtime to HDMI
  drm/exynos: add pm_runtime to Mixer
  drm/exynos: add pm_runtime to FIMD
  drm/exynos: add pm_runtime to DECON 5433
  drm/exynos: add pm_runtime to DECON 7

 drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
 drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
 drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
 drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
 drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
 7 files changed, 352 insertions(+), 265 deletions(-)

-- 
1.9.1

[PATCH v2 1/7] drm/exynos: do not start enabling DP at bind() phase

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

The DP device will be properly enabled at the enable() call just
after the bind call finishes.

Changelog v2:
- no change

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c | 107 +++++++++++++++++++++++---------
 drivers/gpu/drm/exynos/exynos_dp_core.h |   1 +
 2 files changed, 78 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 124fb9a..e4d32a1 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1009,9 +1009,9 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 {
 	int ret;
 
-	encoder->bridge = dp->bridge;
-	dp->bridge->encoder = encoder;
-	ret = drm_bridge_attach(encoder->dev, dp->bridge);
+	encoder->bridge->next = dp->ptn_bridge;
+	dp->ptn_bridge->encoder = encoder;
+	ret = drm_bridge_attach(encoder->dev, dp->ptn_bridge);
 	if (ret) {
 		DRM_ERROR("Failed to attach bridge to drm\n");
 		return ret;
@@ -1020,14 +1020,15 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
 	return 0;
 }
 
-static int exynos_dp_create_connector(struct drm_encoder *encoder)
+static int exynos_dp_bridge_attach(struct drm_bridge *bridge)
 {
-	struct exynos_dp_device *dp = encoder_to_dp(encoder);
+	struct exynos_dp_device *dp = bridge->driver_private;
+	struct drm_encoder *encoder = &dp->encoder;
 	struct drm_connector *connector = &dp->connector;
 	int ret;
 
 	/* Pre-empt DP connector creation if there's a bridge */
-	if (dp->bridge) {
+	if (dp->ptn_bridge) {
 		ret = exynos_drm_attach_lcd_bridge(dp, encoder);
 		if (!ret)
 			return 0;
@@ -1052,22 +1053,9 @@ static int exynos_dp_create_connector(struct drm_encoder *encoder)
 	return ret;
 }
 
-static bool exynos_dp_mode_fixup(struct drm_encoder *encoder,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
-{
-	return true;
-}
-
-static void exynos_dp_mode_set(struct drm_encoder *encoder,
-			       struct drm_display_mode *mode,
-			       struct drm_display_mode *adjusted_mode)
-{
-}
-
-static void exynos_dp_enable(struct drm_encoder *encoder)
+static void exynos_dp_bridge_enable(struct drm_bridge *bridge)
 {
-	struct exynos_dp_device *dp = encoder_to_dp(encoder);
+	struct exynos_dp_device *dp = bridge->driver_private;
 	struct exynos_drm_crtc *crtc = dp_to_crtc(dp);
 
 	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
@@ -1092,9 +1080,9 @@ static void exynos_dp_enable(struct drm_encoder *encoder)
 	dp->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
-static void exynos_dp_disable(struct drm_encoder *encoder)
+static void exynos_dp_bridge_disable(struct drm_bridge *bridge)
 {
-	struct exynos_dp_device *dp = encoder_to_dp(encoder);
+	struct exynos_dp_device *dp = bridge->driver_private;
 	struct exynos_drm_crtc *crtc = dp_to_crtc(dp);
 
 	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
@@ -1123,6 +1111,69 @@ static void exynos_dp_disable(struct drm_encoder *encoder)
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
+static void exynos_dp_bridge_nop(struct drm_bridge *bridge)
+{
+	/* do nothing */
+}
+
+static const struct drm_bridge_funcs exynos_dp_bridge_funcs = {
+	.enable = exynos_dp_bridge_enable,
+	.disable = exynos_dp_bridge_disable,
+	.pre_enable = exynos_dp_bridge_nop,
+	.post_disable = exynos_dp_bridge_nop,
+	.attach = exynos_dp_bridge_attach,
+};
+
+static int exynos_dp_create_connector(struct drm_encoder *encoder)
+{
+	struct exynos_dp_device *dp = encoder_to_dp(encoder);
+	struct drm_device *drm_dev = dp->drm_dev;
+	struct drm_bridge *bridge;
+	int ret;
+
+	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
+	if (!bridge) {
+		DRM_ERROR("failed to allocate for drm bridge\n");
+		return -ENOMEM;
+	}
+
+	dp->bridge = bridge;
+
+	encoder->bridge = bridge;
+	bridge->driver_private = dp;
+	bridge->encoder = encoder;
+	bridge->funcs = &exynos_dp_bridge_funcs;
+
+	ret = drm_bridge_attach(drm_dev, bridge);
+	if (ret) {
+		DRM_ERROR("failed to attach drm bridge\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static bool exynos_dp_mode_fixup(struct drm_encoder *encoder,
+				 const struct drm_display_mode *mode,
+				 struct drm_display_mode *adjusted_mode)
+{
+	return true;
+}
+
+static void exynos_dp_mode_set(struct drm_encoder *encoder,
+			       struct drm_display_mode *mode,
+			       struct drm_display_mode *adjusted_mode)
+{
+}
+
+static void exynos_dp_enable(struct drm_encoder *encoder)
+{
+}
+
+static void exynos_dp_disable(struct drm_encoder *encoder)
+{
+}
+
 static struct drm_encoder_helper_funcs exynos_dp_encoder_helper_funcs = {
 	.mode_fixup = exynos_dp_mode_fixup,
 	.mode_set = exynos_dp_mode_set,
@@ -1238,7 +1289,7 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 		}
 	}
 
-	if (!dp->panel && !dp->bridge) {
+	if (!dp->panel && !dp->ptn_bridge) {
 		ret = exynos_dp_dt_parse_panel(dp);
 		if (ret)
 			return ret;
@@ -1289,10 +1340,6 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 
 	INIT_WORK(&dp->hotplug_work, exynos_dp_hotplug);
 
-	phy_power_on(dp->phy);
-
-	exynos_dp_init_dp(dp);
-
 	ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler,
 			irq_flags, "exynos-dp", dp);
 	if (ret) {
@@ -1365,9 +1412,9 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	if (endpoint) {
 		bridge_node = of_graph_get_remote_port_parent(endpoint);
 		if (bridge_node) {
-			dp->bridge = of_drm_find_bridge(bridge_node);
+			dp->ptn_bridge = of_drm_find_bridge(bridge_node);
 			of_node_put(bridge_node);
-			if (!dp->bridge)
+			if (!dp->ptn_bridge)
 				return -EPROBE_DEFER;
 		} else
 			return -EPROBE_DEFER;
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.h b/drivers/gpu/drm/exynos/exynos_dp_core.h
index e413b6f..66eec4b 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.h
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.h
@@ -153,6 +153,7 @@ struct exynos_dp_device {
 	struct drm_connector	connector;
 	struct drm_panel	*panel;
 	struct drm_bridge	*bridge;
+	struct drm_bridge	*ptn_bridge;
 	struct clk		*clock;
 	unsigned int		irq;
 	void __iomem		*reg_base;
-- 
1.9.1

[PATCH v2 2/7] drm/exynos: add pm_runtime to DP

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with
proper refcnt instead of rely on specific flags to track the enabled
state.

Changelog v2:
- no change

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c | 58 ++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index e4d32a1..b6e8b89 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -1058,8 +1058,7 @@ static void exynos_dp_bridge_enable(struct drm_bridge *bridge)
 	struct exynos_dp_device *dp = bridge->driver_private;
 	struct exynos_drm_crtc *crtc = dp_to_crtc(dp);
 
-	if (dp->dpms_mode == DRM_MODE_DPMS_ON)
-		return;
+	pm_runtime_get_sync(dp->dev);
 
 	if (dp->panel) {
 		if (drm_panel_prepare(dp->panel)) {
@@ -1071,13 +1070,10 @@ static void exynos_dp_bridge_enable(struct drm_bridge *bridge)
 	if (crtc->ops->clock_enable)
 		crtc->ops->clock_enable(dp_to_crtc(dp), true);
 
-	clk_prepare_enable(dp->clock);
 	phy_power_on(dp->phy);
 	exynos_dp_init_dp(dp);
 	enable_irq(dp->irq);
 	exynos_dp_commit(&dp->encoder);
-
-	dp->dpms_mode = DRM_MODE_DPMS_ON;
 }
 
 static void exynos_dp_bridge_disable(struct drm_bridge *bridge)
@@ -1085,9 +1081,6 @@ static void exynos_dp_bridge_disable(struct drm_bridge *bridge)
 	struct exynos_dp_device *dp = bridge->driver_private;
 	struct exynos_drm_crtc *crtc = dp_to_crtc(dp);
 
-	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
-		return;
-
 	if (dp->panel) {
 		if (drm_panel_disable(dp->panel)) {
 			DRM_ERROR("failed to disable the panel\n");
@@ -1098,7 +1091,6 @@ static void exynos_dp_bridge_disable(struct drm_bridge *bridge)
 	disable_irq(dp->irq);
 	flush_work(&dp->hotplug_work);
 	phy_power_off(dp->phy);
-	clk_disable_unprepare(dp->clock);
 
 	if (crtc->ops->clock_enable)
 		crtc->ops->clock_enable(dp_to_crtc(dp), false);
@@ -1108,7 +1100,7 @@ static void exynos_dp_bridge_disable(struct drm_bridge *bridge)
 			DRM_ERROR("failed to turnoff the panel\n");
 	}
 
-	dp->dpms_mode = DRM_MODE_DPMS_OFF;
+	pm_runtime_put_sync(dp->dev);
 }
 
 static void exynos_dp_bridge_nop(struct drm_bridge *bridge)
@@ -1267,7 +1259,6 @@ static int exynos_dp_bind(struct device *dev, struct device *master, void *data)
 	int pipe, ret = 0;
 
 	dp->dev = &pdev->dev;
-	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 
 	dp->video_info = exynos_dp_dt_parse_pdata(&pdev->dev);
 	if (IS_ERR(dp->video_info))
@@ -1392,6 +1383,7 @@ static int exynos_dp_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *panel_node, *bridge_node, *endpoint;
 	struct exynos_dp_device *dp;
+	int ret;
 
 	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
 				GFP_KERNEL);
@@ -1420,16 +1412,57 @@ static int exynos_dp_probe(struct platform_device *pdev)
 			return -EPROBE_DEFER;
 	}
 
-	return component_add(&pdev->dev, &exynos_dp_ops);
+	pm_runtime_enable(dev);
+
+	ret = component_add(&pdev->dev, &exynos_dp_ops);
+	if (ret)
+		goto err_disable_pm_runtime;
+
+	return ret;
+
+err_disable_pm_runtime:
+	pm_runtime_disable(dev);
+
+	return ret;
 }
 
 static int exynos_dp_remove(struct platform_device *pdev)
 {
+	pm_runtime_disable(&pdev->dev);
 	component_del(&pdev->dev, &exynos_dp_ops);
 
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int exynos_dp_suspend(struct device *dev)
+{
+	struct exynos_dp_device *dp = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(dp->clock);
+
+	return 0;
+}
+
+static int exynos_dp_resume(struct device *dev)
+{
+	struct exynos_dp_device *dp = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(dp->clock);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the clock clk [%d]\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops exynos_dp_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos_dp_suspend, exynos_dp_resume, NULL)
+};
+
 static const struct of_device_id exynos_dp_match[] = {
 	{ .compatible = "samsung,exynos5-dp" },
 	{},
@@ -1442,6 +1475,7 @@ struct platform_driver dp_driver = {
 	.driver		= {
 		.name	= "exynos-dp",
 		.owner	= THIS_MODULE,
+		.pm	= &exynos_dp_pm_ops,
 		.of_match_table = exynos_dp_match,
 	},
 };
-- 
1.9.1

[PATCH v2 3/7] drm/exynos: add pm_runtime to HDMI

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with proper
refcnt instead of rely on specific flags to track the enabled state.

Changelog v2:
- Mofidy CONFIG_PM_SLEEP -> CONFIG_PM

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 56 ++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 57b6755..d0362af 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -102,7 +102,6 @@ struct hdmi_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
 	struct drm_connector		connector;
-	bool				powered;
 	bool				dvi_mode;
 	struct delayed_work		hotplug_work;
 	struct drm_display_mode		current_mode;
@@ -113,7 +112,7 @@ struct hdmi_context {
 	void __iomem			*regs_hdmiphy;
 	struct i2c_client		*hdmiphy_port;
 	struct i2c_adapter		*ddc_adpt;
-	struct gpio_desc 		*hpd_gpio;
+	struct gpio_desc		*hpd_gpio;
 	int				irq;
 	struct regmap			*pmureg;
 	struct clk			*hdmi;
@@ -1585,11 +1584,6 @@ static void hdmi_enable(struct drm_encoder *encoder)
 {
 	struct hdmi_context *hdata = encoder_to_hdmi(encoder);
 
-	if (hdata->powered)
-		return;
-
-	hdata->powered = true;
-
 	pm_runtime_get_sync(hdata->dev);
 
 	if (regulator_bulk_enable(ARRAY_SIZE(supply), hdata->regul_bulk))
@@ -1599,9 +1593,6 @@ static void hdmi_enable(struct drm_encoder *encoder)
 	regmap_update_bits(hdata->pmureg, PMU_HDMI_PHY_CONTROL,
 			PMU_HDMI_PHY_ENABLE_BIT, 1);
 
-	clk_prepare_enable(hdata->hdmi);
-	clk_prepare_enable(hdata->sclk_hdmi);
-
 	hdmi_conf_apply(hdata);
 }
 
@@ -1611,9 +1602,6 @@ static void hdmi_disable(struct drm_encoder *encoder)
 	struct drm_crtc *crtc = encoder->crtc;
 	const struct drm_crtc_helper_funcs *funcs = NULL;
 
-	if (!hdata->powered)
-		return;
-
 	/*
 	 * The SFRs of VP and Mixer are updated by Vertical Sync of
 	 * Timing generator which is a part of HDMI so the sequence
@@ -1633,9 +1621,6 @@ static void hdmi_disable(struct drm_encoder *encoder)
 
 	cancel_delayed_work(&hdata->hotplug_work);
 
-	clk_disable_unprepare(hdata->sclk_hdmi);
-	clk_disable_unprepare(hdata->hdmi);
-
 	/* reset pmu hdmiphy control bit to disable hdmiphy */
 	regmap_update_bits(hdata->pmureg, PMU_HDMI_PHY_CONTROL,
 			PMU_HDMI_PHY_ENABLE_BIT, 0);
@@ -1643,8 +1628,6 @@ static void hdmi_disable(struct drm_encoder *encoder)
 	regulator_bulk_disable(ARRAY_SIZE(supply), hdata->regul_bulk);
 
 	pm_runtime_put_sync(hdata->dev);
-
-	hdata->powered = false;
 }
 
 static struct drm_encoder_helper_funcs exynos_hdmi_encoder_helper_funcs = {
@@ -1978,12 +1961,49 @@ static int hdmi_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int exynos_hdmi_suspend(struct device *dev)
+{
+	struct hdmi_context *hdata = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(hdata->sclk_hdmi);
+	clk_disable_unprepare(hdata->hdmi);
+
+	return 0;
+}
+
+static int exynos_hdmi_resume(struct device *dev)
+{
+	struct hdmi_context *hdata = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(hdata->hdmi);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the hdmi clk [%d]\n", ret);
+		return ret;
+	}
+	ret = clk_prepare_enable(hdata->sclk_hdmi);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the sclk_mixer clk [%d]\n",
+			  ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops exynos_hdmi_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos_hdmi_suspend, exynos_hdmi_resume, NULL)
+};
+
 struct platform_driver hdmi_driver = {
 	.probe		= hdmi_probe,
 	.remove		= hdmi_remove,
 	.driver		= {
 		.name	= "exynos-hdmi",
 		.owner	= THIS_MODULE,
+		.pm	= &exynos_hdmi_pm_ops,
 		.of_match_table = hdmi_match_types,
 	},
 };
-- 
1.9.1

[PATCH v2 4/7] drm/exynos: add pm_runtime to Mixer

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with proper
refcnt instead of rely on specific flags to track the enabled state.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 125 +++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index d09f8f9..7498c6e 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -70,7 +70,6 @@ enum mixer_version_id {
 };
 
 enum mixer_flag_bits {
-	MXR_BIT_POWERED,
 	MXR_BIT_VSYNC,
 };
 
@@ -926,8 +925,6 @@ static int mixer_enable_vblank(struct exynos_drm_crtc *crtc)
 	struct mixer_resources *res = &mixer_ctx->mixer_res;
 
 	__set_bit(MXR_BIT_VSYNC, &mixer_ctx->flags);
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
-		return 0;
 
 	/* enable vsync interrupt */
 	mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
@@ -943,9 +940,6 @@ static void mixer_disable_vblank(struct exynos_drm_crtc *crtc)
 
 	__clear_bit(MXR_BIT_VSYNC, &mixer_ctx->flags);
 
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
-		return;
-
 	/* disable vsync interrupt */
 	mixer_reg_writemask(res, MXR_INT_STATUS, ~0, MXR_INT_CLEAR_VSYNC);
 	mixer_reg_writemask(res, MXR_INT_EN, 0, MXR_INT_EN_VSYNC);
@@ -958,9 +952,6 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 
 	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
 
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
-		return;
-
 	if (plane->zpos > 1 && mixer_ctx->vp_enabled)
 		vp_video_buffer(mixer_ctx, plane);
 	else
@@ -976,9 +967,6 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 
 	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
 
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
-		return;
-
 	spin_lock_irqsave(&res->reg_slock, flags);
 	mixer_vsync_set_update(mixer_ctx, false);
 
@@ -993,9 +981,6 @@ static void mixer_wait_for_vblank(struct exynos_drm_crtc *crtc)
 	struct mixer_context *mixer_ctx = crtc->ctx;
 	int err;
 
-	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
-		return;
-
 	err = drm_vblank_get(mixer_ctx->drm_dev, mixer_ctx->pipe);
 	if (err < 0) {
 		DRM_DEBUG_KMS("failed to acquire vblank counter\n");
@@ -1020,43 +1005,9 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
 	struct mixer_resources *res = &ctx->mixer_res;
-	int ret;
-
-	if (test_bit(MXR_BIT_POWERED, &ctx->flags))
-		return;
 
 	pm_runtime_get_sync(ctx->dev);
 
-	ret = clk_prepare_enable(res->mixer);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the mixer clk [%d]\n", ret);
-		return;
-	}
-	ret = clk_prepare_enable(res->hdmi);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the hdmi clk [%d]\n", ret);
-		return;
-	}
-	if (ctx->vp_enabled) {
-		ret = clk_prepare_enable(res->vp);
-		if (ret < 0) {
-			DRM_ERROR("Failed to prepare_enable the vp clk [%d]\n",
-				  ret);
-			return;
-		}
-		if (ctx->has_sclk) {
-			ret = clk_prepare_enable(res->sclk_mixer);
-			if (ret < 0) {
-				DRM_ERROR("Failed to prepare_enable the " \
-					   "sclk_mixer clk [%d]\n",
-					  ret);
-				return;
-			}
-		}
-	}
-
-	set_bit(MXR_BIT_POWERED, &ctx->flags);
-
 	mixer_reg_writemask(res, MXR_STATUS, ~0, MXR_STATUS_SOFT_RESET);
 
 	if (test_bit(MXR_BIT_VSYNC, &ctx->flags)) {
@@ -1069,29 +1020,15 @@ static void mixer_enable(struct exynos_drm_crtc *crtc)
 static void mixer_disable(struct exynos_drm_crtc *crtc)
 {
 	struct mixer_context *ctx = crtc->ctx;
-	struct mixer_resources *res = &ctx->mixer_res;
 	int i;
 
-	if (!test_bit(MXR_BIT_POWERED, &ctx->flags))
-		return;
-
 	mixer_stop(ctx);
 	mixer_regs_dump(ctx);
 
 	for (i = 0; i < MIXER_WIN_NR; i++)
 		mixer_disable_plane(crtc, &ctx->planes[i]);
 
-	clear_bit(MXR_BIT_POWERED, &ctx->flags);
-
-	clk_disable_unprepare(res->hdmi);
-	clk_disable_unprepare(res->mixer);
-	if (ctx->vp_enabled) {
-		clk_disable_unprepare(res->vp);
-		if (ctx->has_sclk)
-			clk_disable_unprepare(res->sclk_mixer);
-	}
-
-	pm_runtime_put_sync(ctx->dev);
+	pm_runtime_put(ctx->dev);
 }
 
 /* Only valid for Mixer version 16.0.33.0 */
@@ -1293,10 +1230,70 @@ static int mixer_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int exynos_mixer_suspend(struct device *dev)
+{
+	struct mixer_context *ctx = dev_get_drvdata(dev);
+	struct mixer_resources *res = &ctx->mixer_res;
+
+	clk_disable_unprepare(res->hdmi);
+	clk_disable_unprepare(res->mixer);
+	if (ctx->vp_enabled) {
+		clk_disable_unprepare(res->vp);
+		if (ctx->has_sclk)
+			clk_disable_unprepare(res->sclk_mixer);
+	}
+
+	return 0;
+}
+
+static int exynos_mixer_resume(struct device *dev)
+{
+	struct mixer_context *ctx = dev_get_drvdata(dev);
+	struct mixer_resources *res = &ctx->mixer_res;
+	int ret;
+
+	ret = clk_prepare_enable(res->mixer);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the mixer clk [%d]\n", ret);
+		return ret;
+	}
+	ret = clk_prepare_enable(res->hdmi);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the hdmi clk [%d]\n", ret);
+		return ret;
+	}
+	if (ctx->vp_enabled) {
+		ret = clk_prepare_enable(res->vp);
+		if (ret < 0) {
+			DRM_ERROR("Failed to prepare_enable the vp clk [%d]\n",
+				  ret);
+			return ret;
+		}
+		if (ctx->has_sclk) {
+			ret = clk_prepare_enable(res->sclk_mixer);
+			if (ret < 0) {
+				DRM_ERROR("Failed to prepare_enable the " \
+					   "sclk_mixer clk [%d]\n",
+					  ret);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops exynos_mixer_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos_mixer_suspend, exynos_mixer_resume, NULL)
+};
+
 struct platform_driver mixer_driver = {
 	.driver = {
 		.name = "exynos-mixer",
 		.owner = THIS_MODULE,
+		.pm = &exynos_mixer_pm_ops,
 		.of_match_table = mixer_match_types,
 	},
 	.probe = mixer_probe,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/7] drm/exynos: add pm_runtime to FIMD

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with proper
refcnt instead of rely on specific flags to track the enabled state.

Changelog v2:
- Remove unnecessary changes which removed commit callback from decon drivers
  and modify CONFIG_PM_SLEEP -> CONFIG_PM

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 91 +++++++++++++-------------------
 1 file changed, 37 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index bd75c15..ae97271 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -160,7 +160,6 @@ struct fimd_context {
 	u32				vidout_con;
 	u32				i80ifcon;
 	bool				i80_if;
-	bool				suspended;
 	int				pipe;
 	wait_queue_head_t		wait_vsync_queue;
 	atomic_t			wait_vsync_event;
@@ -209,9 +208,6 @@ static int fimd_enable_vblank(struct exynos_drm_crtc *crtc)
 	struct fimd_context *ctx = crtc->ctx;
 	u32 val;
 
-	if (ctx->suspended)
-		return -EPERM;
-
 	if (!test_and_set_bit(0, &ctx->irq_flags)) {
 		val = readl(ctx->regs + VIDINTCON0);
 
@@ -241,9 +237,6 @@ static void fimd_disable_vblank(struct exynos_drm_crtc *crtc)
 	struct fimd_context *ctx = crtc->ctx;
 	u32 val;
 
-	if (ctx->suspended)
-		return;
-
 	if (test_and_clear_bit(0, &ctx->irq_flags)) {
 		val = readl(ctx->regs + VIDINTCON0);
 
@@ -264,9 +257,6 @@ static void fimd_wait_for_vblank(struct exynos_drm_crtc *crtc)
 {
 	struct fimd_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	atomic_set(&ctx->wait_vsync_event, 1);
 
 	/*
@@ -339,14 +329,12 @@ static void fimd_clear_channels(struct exynos_drm_crtc *crtc)
 		int pipe = ctx->pipe;
 
 		/* ensure that vblank interrupt won't be reported to core */
-		ctx->suspended = false;
 		ctx->pipe = -1;
 
 		fimd_enable_vblank(ctx->crtc);
 		fimd_wait_for_vblank(ctx->crtc);
 		fimd_disable_vblank(ctx->crtc);
 
-		ctx->suspended = true;
 		ctx->pipe = pipe;
 	}
 
@@ -384,9 +372,6 @@ static void fimd_commit(struct exynos_drm_crtc *crtc)
 	void *timing_base = ctx->regs + driver_data->timing_base;
 	u32 val, clkdiv;
 
-	if (ctx->suspended)
-		return;
-
 	/* nothing to do if we haven't set the mode yet */
 	if (mode->htotal == 0 || mode->vtotal == 0)
 		return;
@@ -620,9 +605,6 @@ static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
 {
 	struct fimd_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	fimd_shadow_protect_win(ctx, plane->zpos, true);
 }
 
@@ -631,9 +613,6 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
 {
 	struct fimd_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	fimd_shadow_protect_win(ctx, plane->zpos, false);
 }
 
@@ -649,9 +628,6 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned int bpp = state->fb->bits_per_pixel >> 3;
 	unsigned int pitch = state->fb->pitches[0];
 
-	if (ctx->suspended)
-		return;
-
 	offset = plane->src_x * bpp;
 	offset += plane->src_y * pitch;
 
@@ -733,9 +709,6 @@ static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
 	struct fimd_context *ctx = crtc->ctx;
 	unsigned int win = plane->zpos;
 
-	if (ctx->suspended)
-		return;
-
 	fimd_enable_video_output(ctx, win, false);
 
 	if (ctx->driver_data->has_shadowcon)
@@ -745,27 +718,9 @@ static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
 static void fimd_enable(struct exynos_drm_crtc *crtc)
 {
 	struct fimd_context *ctx = crtc->ctx;
-	int ret;
-
-	if (!ctx->suspended)
-		return;
-
-	ctx->suspended = false;
 
 	pm_runtime_get_sync(ctx->dev);
 
-	ret = clk_prepare_enable(ctx->bus_clk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
-		return;
-	}
-
-	ret = clk_prepare_enable(ctx->lcd_clk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n", ret);
-		return;
-	}
-
 	/* if vblank was enabled status, enable it again. */
 	if (test_and_clear_bit(0, &ctx->irq_flags))
 		fimd_enable_vblank(ctx->crtc);
@@ -778,9 +733,6 @@ static void fimd_disable(struct exynos_drm_crtc *crtc)
 	struct fimd_context *ctx = crtc->ctx;
 	int i;
 
-	if (ctx->suspended)
-		return;
-
 	/*
 	 * We need to make sure that all windows are disabled before we
 	 * suspend that connector. Otherwise we might try to scan from
@@ -795,12 +747,7 @@ static void fimd_disable(struct exynos_drm_crtc *crtc)
 
 	writel(0, ctx->regs + VIDCON0);
 
-	clk_disable_unprepare(ctx->lcd_clk);
-	clk_disable_unprepare(ctx->bus_clk);
-
 	pm_runtime_put_sync(ctx->dev);
-
-	ctx->suspended = true;
 }
 
 static void fimd_trigger(struct device *dev)
@@ -1011,7 +958,6 @@ static int fimd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctx->dev = dev;
-	ctx->suspended = true;
 	ctx->driver_data = drm_fimd_get_driver_data(pdev);
 
 	if (of_property_read_bool(dev->of_node, "samsung,invert-vden"))
@@ -1121,12 +1067,49 @@ static int fimd_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int exynos_fimd_suspend(struct device *dev)
+{
+	struct fimd_context *ctx = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(ctx->lcd_clk);
+	clk_disable_unprepare(ctx->bus_clk);
+
+	return 0;
+}
+
+static int exynos_fimd_resume(struct device *dev)
+{
+	struct fimd_context *ctx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(ctx->bus_clk);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the bus clk [%d]\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ctx->lcd_clk);
+	if  (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the lcd clk [%d]\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops exynos_fimd_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos_fimd_suspend, exynos_fimd_resume, NULL)
+};
+
 struct platform_driver fimd_driver = {
 	.probe		= fimd_probe,
 	.remove		= fimd_remove,
 	.driver		= {
 		.name	= "exynos4-fb",
 		.owner	= THIS_MODULE,
+		.pm	= &exynos_fimd_pm_ops,
 		.of_match_table = fimd_driver_dt_match,
 	},
 };
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/7] drm/exynos: add pm_runtime to DECON 5433

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with
proper refcnt instead of rely on specific flags to track the enabled
state.

Changelog v2:
- Change CONFIG_PM_SLEEP -> CONFIG_PM

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 54 +++++++++++++++++++--------
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index fbe1b31..edfd6e3 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -385,12 +385,6 @@ static void decon_enable(struct exynos_drm_crtc *crtc)
 
 	pm_runtime_get_sync(ctx->dev);
 
-	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
-		ret = clk_prepare_enable(ctx->clks[i]);
-		if (ret < 0)
-			goto err;
-	}
-
 	set_bit(BIT_CLKS_ENABLED, &ctx->flags);
 
 	/* if vblank was enabled status, enable it again. */
@@ -399,11 +393,6 @@ static void decon_enable(struct exynos_drm_crtc *crtc)
 
 	decon_commit(ctx->crtc);
 
-	return;
-err:
-	while (--i >= 0)
-		clk_disable_unprepare(ctx->clks[i]);
-
 	set_bit(BIT_SUSPENDED, &ctx->flags);
 }
 
@@ -425,9 +414,6 @@ static void decon_disable(struct exynos_drm_crtc *crtc)
 
 	decon_swreset(ctx);
 
-	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++)
-		clk_disable_unprepare(ctx->clks[i]);
-
 	clear_bit(BIT_CLKS_ENABLED, &ctx->flags);
 
 	pm_runtime_put_sync(ctx->dev);
@@ -478,7 +464,6 @@ err:
 static struct exynos_drm_crtc_ops decon_crtc_ops = {
 	.enable			= decon_enable,
 	.disable		= decon_disable,
-	.commit			= decon_commit,
 	.enable_vblank		= decon_enable_vblank,
 	.disable_vblank		= decon_disable_vblank,
 	.atomic_begin		= decon_atomic_begin,
@@ -581,6 +566,44 @@ out:
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_PM
+static int exynos5433_decon_suspend(struct device *dev)
+{
+	struct decon_context *ctx = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++)
+		clk_disable_unprepare(ctx->clks[i]);
+
+	return 0;
+}
+
+static int exynos5433_decon_resume(struct device *dev)
+{
+	struct decon_context *ctx = dev_get_drvdata(dev);
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {
+		ret = clk_prepare_enable(ctx->clks[i]);
+		if (ret < 0)
+			goto err;
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0)
+		clk_disable_unprepare(ctx->clks[i]);
+
+	return ret;
+}
+#endif
+
+static const struct dev_pm_ops exynos5433_decon_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos5433_decon_suspend, exynos5433_decon_resume,
+			   NULL)
+};
+
 static const struct of_device_id exynos5433_decon_driver_dt_match[] = {
 	{
 		.compatible = "samsung,exynos5433-decon",
@@ -684,6 +707,7 @@ struct platform_driver exynos5433_decon_driver = {
 	.remove		= exynos5433_decon_remove,
 	.driver		= {
 		.name	= "exynos5433-decon",
+		.pm	= &exynos5433_decon_pm_ops,
 		.of_match_table = exynos5433_decon_driver_dt_match,
 	},
 };
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 7/7] drm/exynos: add pm_runtime to DECON 7

[Inki Dae]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Let pm_runtime handle the enabling/disabling of the device with
proper refcnt instead of rely on specific flags to track the enabled
state.

Changelog v2:
- Modify CONFIG_PM_SLEEP -> CONFIG_PM

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Signed-off-by: Inki Dae <inki.dae@samsung.com>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c | 125 ++++++++++++-----------------
 1 file changed, 53 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index ead2b16..3119aba 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -54,7 +54,6 @@ struct decon_context {
 	void __iomem			*regs;
 	unsigned long			irq_flags;
 	bool				i80_if;
-	bool				suspended;
 	int				pipe;
 	wait_queue_head_t		wait_vsync_queue;
 	atomic_t			wait_vsync_event;
@@ -85,9 +84,6 @@ static void decon_wait_for_vblank(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	atomic_set(&ctx->wait_vsync_event, 1);
 
 	/*
@@ -119,13 +115,8 @@ static void decon_clear_channels(struct exynos_drm_crtc *crtc)
 	}
 
 	/* Wait for vsync, as disable channel takes effect at next vsync */
-	if (ch_enabled) {
-		unsigned int state = ctx->suspended;
-
-		ctx->suspended = 0;
+	if (ch_enabled)
 		decon_wait_for_vblank(ctx->crtc);
-		ctx->suspended = state;
-	}
 }
 
 static int decon_ctx_initialize(struct decon_context *ctx,
@@ -170,9 +161,6 @@ static void decon_commit(struct exynos_drm_crtc *crtc)
 	struct drm_display_mode *mode = &crtc->base.state->adjusted_mode;
 	u32 val, clkdiv;
 
-	if (ctx->suspended)
-		return;
-
 	/* nothing to do if we haven't set the mode yet */
 	if (mode->htotal == 0 || mode->vtotal == 0)
 		return;
@@ -234,9 +222,6 @@ static int decon_enable_vblank(struct exynos_drm_crtc *crtc)
 	struct decon_context *ctx = crtc->ctx;
 	u32 val;
 
-	if (ctx->suspended)
-		return -EPERM;
-
 	if (!test_and_set_bit(0, &ctx->irq_flags)) {
 		val = readl(ctx->regs + VIDINTCON0);
 
@@ -259,9 +244,6 @@ static void decon_disable_vblank(struct exynos_drm_crtc *crtc)
 	struct decon_context *ctx = crtc->ctx;
 	u32 val;
 
-	if (ctx->suspended)
-		return;
-
 	if (test_and_clear_bit(0, &ctx->irq_flags)) {
 		val = readl(ctx->regs + VIDINTCON0);
 
@@ -389,9 +371,6 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
 {
 	struct decon_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	decon_shadow_protect_win(ctx, plane->zpos, true);
 }
 
@@ -409,9 +388,6 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned int bpp = state->fb->bits_per_pixel >> 3;
 	unsigned int pitch = state->fb->pitches[0];
 
-	if (ctx->suspended)
-		return;
-
 	/*
 	 * SHADOWCON/PRTCON register is used for enabling timing.
 	 *
@@ -508,9 +484,6 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 	unsigned int win = plane->zpos;
 	u32 val;
 
-	if (ctx->suspended)
-		return;
-
 	/* protect windows */
 	decon_shadow_protect_win(ctx, win, true);
 
@@ -529,9 +502,6 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
 {
 	struct decon_context *ctx = crtc->ctx;
 
-	if (ctx->suspended)
-		return;
-
 	decon_shadow_protect_win(ctx, plane->zpos, false);
 }
 
@@ -555,39 +525,9 @@ static void decon_init(struct decon_context *ctx)
 static void decon_enable(struct exynos_drm_crtc *crtc)
 {
 	struct decon_context *ctx = crtc->ctx;
-	int ret;
-
-	if (!ctx->suspended)
-		return;
-
-	ctx->suspended = false;
 
 	pm_runtime_get_sync(ctx->dev);
 
-	ret = clk_prepare_enable(ctx->pclk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
-		return;
-	}
-
-	ret = clk_prepare_enable(ctx->aclk);
-	if (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
-		return;
-	}
-
-	ret = clk_prepare_enable(ctx->eclk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
-		return;
-	}
-
-	ret = clk_prepare_enable(ctx->vclk);
-	if  (ret < 0) {
-		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
-		return;
-	}
-
 	decon_init(ctx);
 
 	/* if vblank was enabled status, enable it again. */
@@ -602,9 +542,6 @@ static void decon_disable(struct exynos_drm_crtc *crtc)
 	struct decon_context *ctx = crtc->ctx;
 	int i;
 
-	if (ctx->suspended)
-		return;
-
 	/*
 	 * We need to make sure that all windows are disabled before we
 	 * suspend that connector. Otherwise we might try to scan from
@@ -613,14 +550,7 @@ static void decon_disable(struct exynos_drm_crtc *crtc)
 	for (i = 0; i < WINDOWS_NR; i++)
 		decon_disable_plane(crtc, &ctx->planes[i]);
 
-	clk_disable_unprepare(ctx->vclk);
-	clk_disable_unprepare(ctx->eclk);
-	clk_disable_unprepare(ctx->aclk);
-	clk_disable_unprepare(ctx->pclk);
-
 	pm_runtime_put_sync(ctx->dev);
-
-	ctx->suspended = true;
 }
 
 static const struct exynos_drm_crtc_ops decon_crtc_ops = {
@@ -748,7 +678,6 @@ static int decon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctx->dev = dev;
-	ctx->suspended = true;
 
 	i80_if_timings = of_get_child_by_name(dev->of_node, "i80-if-timings");
 	if (i80_if_timings)
@@ -843,11 +772,63 @@ static int decon_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int exynos7_decon_suspend(struct device *dev)
+{
+	struct decon_context *ctx = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(ctx->vclk);
+	clk_disable_unprepare(ctx->eclk);
+	clk_disable_unprepare(ctx->aclk);
+	clk_disable_unprepare(ctx->pclk);
+
+	return 0;
+}
+
+static int exynos7_decon_resume(struct device *dev)
+{
+	struct decon_context *ctx = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(ctx->pclk);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the pclk [%d]\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ctx->aclk);
+	if (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the aclk [%d]\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ctx->eclk);
+	if  (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the eclk [%d]\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(ctx->vclk);
+	if  (ret < 0) {
+		DRM_ERROR("Failed to prepare_enable the vclk [%d]\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops exynos7_decon_pm_ops = {
+	SET_RUNTIME_PM_OPS(exynos7_decon_suspend, exynos7_decon_resume,
+			   NULL)
+};
+
 struct platform_driver decon_driver = {
 	.probe		= decon_probe,
 	.remove		= decon_remove,
 	.driver		= {
 		.name	= "exynos-decon",
+		.pm	= &exynos7_decon_pm_ops,
 		.of_match_table = decon_driver_dt_match,
 	},
 };
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Andrzej Hajda]

Hi Inki,

On 11/03/2015 11:47 AM, Inki Dae wrote:
> This patch series adds pm runtime support for Exynos drm.
> 
> Originally, this patch was posted by Gustavo but there was no any
> answer about some comments. So I rebased this patch series on top of
> exynos-drm-next, removed unnecessary patches and modified wrong macro.

I have sent comment to original patchset[1], but for some strange reasons
it went only to mailing lists.
My concerns were as follows:
- exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
  why should we add per component support?
- component suspend sequence is non deterministic, but in case of
  video pipelines, specification often requires fixed order,
- the patchset adds implicit dependency on PM_SLEEP.


Current solution should work correctly and it was OK last time I have tested it.
I am not sure about atomic requirements, are there special ones?

There are other issues with current solution, rather easy to solve:
- it assumes that exynos-drm device will be suspended first - it should be true,
 as it is created at the end and suspend order is reverse to creation order, but
 I am not sure if we can rely on it - some solution is to add pm callbacks to
 all components, and from those callbacks call one centralized pm routine,
- suspend/resume callbacks theoretically can be called during component
  master initialization/deinitailization it could be racy,
- exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
  for historical reasons, these function can be merged together.

[1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395

Regards
Andrzej

> 
> Changelog v2:
> - Remove patch 5 and 6.
>   . commit callback are already removed so isn't required anymore.
> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>   . Really not mendatory for FIMD uses DP, and it could be different
>     according to Board.
> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>     CONFIG_PM_SLEEP.
> 
> Gustavo Padovan (7):
>   drm/exynos: do not start enabling DP at bind() phase
>   drm/exynos: add pm_runtime to DP
>   drm/exynos: add pm_runtime to HDMI
>   drm/exynos: add pm_runtime to Mixer
>   drm/exynos: add pm_runtime to FIMD
>   drm/exynos: add pm_runtime to DECON 5433
>   drm/exynos: add pm_runtime to DECON 7
> 
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>  7 files changed, 352 insertions(+), 265 deletions(-)
> 

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 4637 bytes --]

2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com>:
> Hi Inki,
>
> On 11/03/2015 11:47 AM, Inki Dae wrote:
>> This patch series adds pm runtime support for Exynos drm.
>>
>> Originally, this patch was posted by Gustavo but there was no any
>> answer about some comments. So I rebased this patch series on top of
>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>
> I have sent comment to original patchset[1], but for some strange reasons
> it went only to mailing lists.
> My concerns were as follows:
> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
>   why should we add per component support?

For this, I think I already mentioned enough,
        http://www.spinics.net/lists/linux-samsung-soc/msg47695.html

In brief, exynos_drm_drv doesn't control each power domain of each kms
device. It means that we cannot power off fully without pm runtime
interface of each kms device - just they can only control their clock not
power domain. So question. How we could control power domain of each kms
device without runtime pm interface?

> - component suspend sequence is non deterministic, but in case of
>   video pipelines, specification often requires fixed order,

Can your show me an example? I think component suspend and resume are
invoked by only drm dpms interface, and the dpms interface has a fixed
order - when dpms goes to off, encoder first, and when dpms goes to on,
crtc first.

My only concern is that runtime pm interface of each kms driver is called
within pm sleep context of Exynos drm driver. So I will look into this no
problem.

> - the patchset adds implicit dependency on PM_SLEEP.
>
>
> Current solution should work correctly and it was OK last time I have
tested it.
> I am not sure about atomic requirements, are there special ones?

Not related to atomic feature.

>
> There are other issues with current solution, rather easy to solve:
> - it assumes that exynos-drm device will be suspended first - it should
be true,
>  as it is created at the end and suspend order is reverse to creation
order, but
>  I am not sure if we can rely on it - some solution is to add pm
callbacks to
>  all components, and from those callbacks call one centralized pm routine,

You mean pm callbacks are pm sleep interface? And you mean let's add sleep
pm interface to each kms driver?  If so, I'm not agree with you because
sleep pm should be controlled by only drm top like single driver does.

> - suspend/resume callbacks theoretically can be called during component
>   master initialization/deinitailization it could be racy,

I'm not sure what you mean but component master
initialization/deinitailization mean bind and unbind? If so, bind/unbind
interfaces of all components will never call pm relevant interfaces but the
the pm relevant interfaces are called by only dpms interface.

> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
>   for historical reasons, these function can be merged together.

Also can you show me more detail?

Thanks,
Inki Dae

>
> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
>
> Regards
> Andrzej
>
>>
>> Changelog v2:
>> - Remove patch 5 and 6.
>>   . commit callback are already removed so isn't required anymore.
>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>   . Really not mendatory for FIMD uses DP, and it could be different
>>     according to Board.
>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>     CONFIG_PM_SLEEP.
>>
>> Gustavo Padovan (7):
>>   drm/exynos: do not start enabling DP at bind() phase
>>   drm/exynos: add pm_runtime to DP
>>   drm/exynos: add pm_runtime to HDMI
>>   drm/exynos: add pm_runtime to Mixer
>>   drm/exynos: add pm_runtime to FIMD
>>   drm/exynos: add pm_runtime to DECON 5433
>>   drm/exynos: add pm_runtime to DECON 7
>>
>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165
+++++++++++++++++++-------
>>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>>  7 files changed, 352 insertions(+), 265 deletions(-)
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: Type: text/html, Size: 6562 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Andrzej Hajda]

On 11/03/2015 04:38 PM, Inki Dae wrote:
>  
> 2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com
> <mailto:a.hajda@samsung.com>>:
> > Hi Inki,
> >
> > On 11/03/2015 11:47 AM, Inki Dae wrote:
> >> This patch series adds pm runtime support for Exynos drm.
> >>
> >> Originally, this patch was posted by Gustavo but there was no any
> >> answer about some comments. So I rebased this patch series on top of
> >> exynos-drm-next, removed unnecessary patches and modified wrong macro.
> >
> > I have sent comment to original patchset[1], but for some strange reasons
> > it went only to mailing lists.
> > My concerns were as follows:
> > - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
> >   why should we add per component support?
>  
> For this, I think I already mentioned enough,
>         http://www.spinics.net/lists/linux-samsung-soc/msg47695.html
>
> In brief, exynos_drm_drv doesn't control each power domain of each kms device.
> It means that we cannot power off fully without pm runtime interface of each
> kms device - just they can only control their clock not power domain. So
> question. How we could control power domain of each kms device without runtime
> pm interface?

Hmm, but to enable pm_runtime in components it is enough to use
pm_runtime_enable/disable and pm_runtime_get/put(_sync), there is no need to add
pm callbacks.
In fact most of the components already supports runtime pm,  but quick grep
shows that it is not implemented in:
exynos_drm_dsi.c, exynos_dp_core.c, exynos_drm_mic.c.
So I guess adding pm_runtime support by adding calls pm_runtime_enable/disable
and pm_runtime_get/put(_sync) in appropriate places should be enough.

>
> > - component suspend sequence is non deterministic, but in case of
> >   video pipelines, specification often requires fixed order,
>
> Can your show me an example? I think component suspend and resume are invoked
> by only drm dpms interface, and the dpms interface has a fixed order - when
> dpms goes to off, encoder first, and when dpms goes to on, crtc first.

Now as I understand you do not want to remove exynos_drm_drv pm callbacks so
they will disable devices properly, after that clock disabling order should not
matter, so the whole point is not valid.

>
> My only concern is that runtime pm interface of each kms driver is called
> within pm sleep context of Exynos drm driver. So I will look into this no problem.
>
> > - the patchset adds implicit dependency on PM_SLEEP.
> >
> >
> > Current solution should work correctly and it was OK last time I have tested it.
> > I am not sure about atomic requirements, are there special ones?
>
> Not related to atomic feature.
>
> >
> > There are other issues with current solution, rather easy to solve:
> > - it assumes that exynos-drm device will be suspended first - it should be true,
> >  as it is created at the end and suspend order is reverse to creation order, but
> >  I am not sure if we can rely on it - some solution is to add pm callbacks to
> >  all components, and from those callbacks call one centralized pm routine,
>
> You mean pm callbacks are pm sleep interface? And you mean let's add sleep pm
> interface to each kms driver?  If so, I'm not agree with you because sleep pm
> should be controlled by only drm top like single driver does.

Lets look at an example:
Assume we have present only fimd and dsi components.
In such case kernel creates two platform devices for fimd and dsi in early stage
of parsing device tree blob.
During exynos_drm_drv initialization exynos-drm platform device is created, the
important thing it is created after fimd and dsi.
Now when platform enters sleep mode suspend callbacks are called, the important
thing here is
in which order. If I remember correctly PM guarantees only that children will be
handled before parents.
Since we have no parent-child relation between exynos-drm, fimd and dsi, the
order is unknown.
So it will be possible that component will enter sleep mode before exynos-drm
and in such case system can even
hang if exynos-drm callbacks will try to access registers of such component.
Fortunately in current implementation of PM order of suspending is reversed
order of device creation, so
it will be always exynos-drm first.

So in short we rely here on implementation detail of PM framework.

>
> > - suspend/resume callbacks theoretically can be called during component
> >   master initialization/deinitailization it could be racy,
>
> I'm not sure what you mean but component master
> initialization/deinitailization mean bind and unbind? If so, bind/unbind
> interfaces of all components will never call pm relevant interfaces but the
> the pm relevant interfaces are called by only dpms interface.

exynos_drm_sys_suspend and exynos_drm_sys_resume takes pointer
to drm_device and uses it. It is possible that in the same time drm_driver
is destroyed for some reason it will result in invalid pointer in resume/suspend
callbacks.

The problem is common for componentized drivers and was already reported [1],
but there
is no general solution for it. On the other side probability it will ever happen
seems to be very low.

[1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/115820

>
> > - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
> >   for historical reasons, these function can be merged together.
>
> Also can you show me more detail?

This is quite simple, I will post patch for it.

Regards
Andrzej

>
> Thanks,
> Inki Dae
>
> >
> > [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
> >
> > Regards
> > Andrzej
> >
> >>
> >> Changelog v2:
> >> - Remove patch 5 and 6.
> >>   . commit callback are already removed so isn't required anymore.
> >> - Remove patch 8 which makes dp clock enabled directly from FIMD.
> >>   . Really not mendatory for FIMD uses DP, and it could be different
> >>     according to Board.
> >> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
> >>   . In case of runtime pm, CONFIG_PM macro should be used instead of
> >>     CONFIG_PM_SLEEP.
> >>
> >> Gustavo Padovan (7):
> >>   drm/exynos: do not start enabling DP at bind() phase
> >>   drm/exynos: add pm_runtime to DP
> >>   drm/exynos: add pm_runtime to HDMI
> >>   drm/exynos: add pm_runtime to Mixer
> >>   drm/exynos: add pm_runtime to FIMD
> >>   drm/exynos: add pm_runtime to DECON 5433
> >>   drm/exynos: add pm_runtime to DECON 7
> >>
> >>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
> >>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
> >>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
> >>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
> >>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
> >>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
> >>  7 files changed, 352 insertions(+), 265 deletions(-)
> >>
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>  
>  

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

2015년 11월 04일 16:24에 Andrzej Hajda 이(가) 쓴 글:
> On 11/03/2015 04:38 PM, Inki Dae wrote:
>>  
>> 2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com
>> <mailto:a.hajda@samsung.com>>:
>>> Hi Inki,
>>>
>>> On 11/03/2015 11:47 AM, Inki Dae wrote:
>>>> This patch series adds pm runtime support for Exynos drm.
>>>>
>>>> Originally, this patch was posted by Gustavo but there was no any
>>>> answer about some comments. So I rebased this patch series on top of
>>>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>>>
>>> I have sent comment to original patchset[1], but for some strange reasons
>>> it went only to mailing lists.
>>> My concerns were as follows:
>>> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
>>>   why should we add per component support?
>>  
>> For this, I think I already mentioned enough,
>>         http://www.spinics.net/lists/linux-samsung-soc/msg47695.html
>>
>> In brief, exynos_drm_drv doesn't control each power domain of each kms device.
>> It means that we cannot power off fully without pm runtime interface of each
>> kms device - just they can only control their clock not power domain. So
>> question. How we could control power domain of each kms device without runtime
>> pm interface?
> 
> Hmm, but to enable pm_runtime in components it is enough to use
> pm_runtime_enable/disable and pm_runtime_get/put(_sync), there is no need to add
> pm callbacks.

That is this patch series does, and pm callback is implemented in exynos_drm_drv not in component drivers.

> In fact most of the components already supports runtime pm,  but quick grep
> shows that it is not implemented in:
> exynos_drm_dsi.c, exynos_dp_core.c, exynos_drm_mic.c.

exynos_dp_core already has runtime pm interface with this patch series. For dsi and mic, we need to add the runtime pm interfaces.

> So I guess adding pm_runtime support by adding calls pm_runtime_enable/disable
> and pm_runtime_get/put(_sync) in appropriate places should be enough.
> 
>>
>>> - component suspend sequence is non deterministic, but in case of
>>>   video pipelines, specification often requires fixed order,
>>
>> Can your show me an example? I think component suspend and resume are invoked
>> by only drm dpms interface, and the dpms interface has a fixed order - when
>> dpms goes to off, encoder first, and when dpms goes to on, crtc first.
> 
> Now as I understand you do not want to remove exynos_drm_drv pm callbacks so
> they will disable devices properly, after that clock disabling order should not
> matter, so the whole point is not valid.

In this case, the dpms actually is invoked by SLEEP PM of Linux power management framework. DRM KMS has a interface to control power of kms device, which is really different from SLEEP PM.
So this request can be done by userspace in runtime - just Display power off/on.

> 
>>
>> My only concern is that runtime pm interface of each kms driver is called
>> within pm sleep context of Exynos drm driver. So I will look into this no problem.
>>
>>> - the patchset adds implicit dependency on PM_SLEEP.
>>>
>>>
>>> Current solution should work correctly and it was OK last time I have tested it.
>>> I am not sure about atomic requirements, are there special ones?
>>
>> Not related to atomic feature.
>>
>>>
>>> There are other issues with current solution, rather easy to solve:
>>> - it assumes that exynos-drm device will be suspended first - it should be true,
>>>  as it is created at the end and suspend order is reverse to creation order, but
>>>  I am not sure if we can rely on it - some solution is to add pm callbacks to
>>>  all components, and from those callbacks call one centralized pm routine,
>>
>> You mean pm callbacks are pm sleep interface? And you mean let's add sleep pm
>> interface to each kms driver?  If so, I'm not agree with you because sleep pm
>> should be controlled by only drm top like single driver does.
> 
> Lets look at an example:
> Assume we have present only fimd and dsi components.
> In such case kernel creates two platform devices for fimd and dsi in early stage
> of parsing device tree blob.
> During exynos_drm_drv initialization exynos-drm platform device is created, the
> important thing it is created after fimd and dsi.
> Now when platform enters sleep mode suspend callbacks are called, the important
> thing here is
> in which order. If I remember correctly PM guarantees only that children will be
> handled before parents.
> Since we have no parent-child relation between exynos-drm, fimd and dsi, the
> order is unknown.

exynos-drm is not real hardware. So the order is only valid for kms devices - components.
The only thing exynos-drm does is to invoke dpms so that kms devices are enabled or disabled in fixed order.

And as I mentioned before, my only concern is that runtime pm interface of each kms driver is called
within pm sleep context of Exynos drm driver. So required for more review and test.

> So it will be possible that component will enter sleep mode before exynos-drm

How component can enter sleep mode before exynos-drm? compoments have no sleep pm interfaces but only runtime pm interface,
and even the runtime pm interfaces will be called by pm interfaces of exynos-drm only when sleep pm is requested.
Otherwise, the runtime pm interface of each component will be called by dpms request from userspace.

> and in such case system can even
> hang if exynos-drm callbacks will try to access registers of such component.

So seems not true.

> Fortunately in current implementation of PM order of suspending is reversed
> order of device creation, so
> it will be always exynos-drm first.
> 
> So in short we rely here on implementation detail of PM framework.
> 
>>
>>> - suspend/resume callbacks theoretically can be called during component
>>>   master initialization/deinitailization it could be racy,
>>
>> I'm not sure what you mean but component master
>> initialization/deinitailization mean bind and unbind? If so, bind/unbind
>> interfaces of all components will never call pm relevant interfaces but the
>> the pm relevant interfaces are called by only dpms interface.
> 
> exynos_drm_sys_suspend and exynos_drm_sys_resume takes pointer
> to drm_device and uses it. It is possible that in the same time drm_driver
> is destroyed for some reason it will result in invalid pointer in resume/suspend
> callbacks.

Not sure but seems like to need more review and test anyway.

> 
> The problem is common for componentized drivers and was already reported [1],
> but there
> is no general solution for it. On the other side probability it will ever happen
> seems to be very low.
> 
> [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/115820
> 
>>
>>> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
>>>   for historical reasons, these function can be merged together.
>>
>> Also can you show me more detail?
> 
> This is quite simple, I will post patch for it.

Thanks for sharing.

Anyway, I believe Gustavo to take care of this patch series after back from his vacation.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>
>> Thanks,
>> Inki Dae
>>
>>>
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
>>>
>>> Regards
>>> Andrzej
>>>
>>>>
>>>> Changelog v2:
>>>> - Remove patch 5 and 6.
>>>>   . commit callback are already removed so isn't required anymore.
>>>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>>>   . Really not mendatory for FIMD uses DP, and it could be different
>>>>     according to Board.
>>>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>>>     CONFIG_PM_SLEEP.
>>>>
>>>> Gustavo Padovan (7):
>>>>   drm/exynos: do not start enabling DP at bind() phase
>>>>   drm/exynos: add pm_runtime to DP
>>>>   drm/exynos: add pm_runtime to HDMI
>>>>   drm/exynos: add pm_runtime to Mixer
>>>>   drm/exynos: add pm_runtime to FIMD
>>>>   drm/exynos: add pm_runtime to DECON 5433
>>>>   drm/exynos: add pm_runtime to DECON 7
>>>>
>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
>>>>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>>>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>>>>  7 files changed, 352 insertions(+), 265 deletions(-)
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>  
>>  
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Andrzej Hajda]

On 11/04/2015 08:56 AM, Inki Dae wrote:
>
> 2015년 11월 04일 16:24에 Andrzej Hajda 이(가) 쓴 글:
>> On 11/03/2015 04:38 PM, Inki Dae wrote:
>>>  
>>> 2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com
>>> <mailto:a.hajda@samsung.com>>:
>>>> Hi Inki,
>>>>
>>>> On 11/03/2015 11:47 AM, Inki Dae wrote:
>>>>> This patch series adds pm runtime support for Exynos drm.
>>>>>
>>>>> Originally, this patch was posted by Gustavo but there was no any
>>>>> answer about some comments. So I rebased this patch series on top of
>>>>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>>>> I have sent comment to original patchset[1], but for some strange reasons
>>>> it went only to mailing lists.
>>>> My concerns were as follows:
>>>> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
>>>>   why should we add per component support?
>>>  
>>> For this, I think I already mentioned enough,
>>>         http://www.spinics.net/lists/linux-samsung-soc/msg47695.html
>>>
>>> In brief, exynos_drm_drv doesn't control each power domain of each kms device.
>>> It means that we cannot power off fully without pm runtime interface of each
>>> kms device - just they can only control their clock not power domain. So
>>> question. How we could control power domain of each kms device without runtime
>>> pm interface?
>> Hmm, but to enable pm_runtime in components it is enough to use
>> pm_runtime_enable/disable and pm_runtime_get/put(_sync), there is no need to add
>> pm callbacks.
> That is this patch series does, and pm callback is implemented in exynos_drm_drv not in component drivers.

But this patchset adds runtime_pm ops to each component. Runtime_pm support does
not require
implementing runtime_pm ops, they just increases complexity of the code, and I
see no gain in splitting
power off/on sequence between drm enable/disable callbacks and suspend/resume
callbacks.

Regards
Andrzej

>
>> In fact most of the components already supports runtime pm,  but quick grep
>> shows that it is not implemented in:
>> exynos_drm_dsi.c, exynos_dp_core.c, exynos_drm_mic.c.
> exynos_dp_core already has runtime pm interface with this patch series. For dsi and mic, we need to add the runtime pm interfaces.
>
>> So I guess adding pm_runtime support by adding calls pm_runtime_enable/disable
>> and pm_runtime_get/put(_sync) in appropriate places should be enough.
>>
>>>> - component suspend sequence is non deterministic, but in case of
>>>>   video pipelines, specification often requires fixed order,
>>> Can your show me an example? I think component suspend and resume are invoked
>>> by only drm dpms interface, and the dpms interface has a fixed order - when
>>> dpms goes to off, encoder first, and when dpms goes to on, crtc first.
>> Now as I understand you do not want to remove exynos_drm_drv pm callbacks so
>> they will disable devices properly, after that clock disabling order should not
>> matter, so the whole point is not valid.
> In this case, the dpms actually is invoked by SLEEP PM of Linux power management framework. DRM KMS has a interface to control power of kms device, which is really different from SLEEP PM.
> So this request can be done by userspace in runtime - just Display power off/on.
>
>>> My only concern is that runtime pm interface of each kms driver is called
>>> within pm sleep context of Exynos drm driver. So I will look into this no problem.
>>>
>>>> - the patchset adds implicit dependency on PM_SLEEP.
>>>>
>>>>
>>>> Current solution should work correctly and it was OK last time I have tested it.
>>>> I am not sure about atomic requirements, are there special ones?
>>> Not related to atomic feature.
>>>
>>>> There are other issues with current solution, rather easy to solve:
>>>> - it assumes that exynos-drm device will be suspended first - it should be true,
>>>>  as it is created at the end and suspend order is reverse to creation order, but
>>>>  I am not sure if we can rely on it - some solution is to add pm callbacks to
>>>>  all components, and from those callbacks call one centralized pm routine,
>>> You mean pm callbacks are pm sleep interface? And you mean let's add sleep pm
>>> interface to each kms driver?  If so, I'm not agree with you because sleep pm
>>> should be controlled by only drm top like single driver does.
>> Lets look at an example:
>> Assume we have present only fimd and dsi components.
>> In such case kernel creates two platform devices for fimd and dsi in early stage
>> of parsing device tree blob.
>> During exynos_drm_drv initialization exynos-drm platform device is created, the
>> important thing it is created after fimd and dsi.
>> Now when platform enters sleep mode suspend callbacks are called, the important
>> thing here is
>> in which order. If I remember correctly PM guarantees only that children will be
>> handled before parents.
>> Since we have no parent-child relation between exynos-drm, fimd and dsi, the
>> order is unknown.
> exynos-drm is not real hardware. So the order is only valid for kms devices - components.
> The only thing exynos-drm does is to invoke dpms so that kms devices are enabled or disabled in fixed order.
>
> And as I mentioned before, my only concern is that runtime pm interface of each kms driver is called
> within pm sleep context of Exynos drm driver. So required for more review and test.
>
>> So it will be possible that component will enter sleep mode before exynos-drm
> How component can enter sleep mode before exynos-drm? compoments have no sleep pm interfaces but only runtime pm interface,
> and even the runtime pm interfaces will be called by pm interfaces of exynos-drm only when sleep pm is requested.
> Otherwise, the runtime pm interface of each component will be called by dpms request from userspace.
>
>> and in such case system can even
>> hang if exynos-drm callbacks will try to access registers of such component.
> So seems not true.
>
>> Fortunately in current implementation of PM order of suspending is reversed
>> order of device creation, so
>> it will be always exynos-drm first.
>>
>> So in short we rely here on implementation detail of PM framework.
>>
>>>> - suspend/resume callbacks theoretically can be called during component
>>>>   master initialization/deinitailization it could be racy,
>>> I'm not sure what you mean but component master
>>> initialization/deinitailization mean bind and unbind? If so, bind/unbind
>>> interfaces of all components will never call pm relevant interfaces but the
>>> the pm relevant interfaces are called by only dpms interface.
>> exynos_drm_sys_suspend and exynos_drm_sys_resume takes pointer
>> to drm_device and uses it. It is possible that in the same time drm_driver
>> is destroyed for some reason it will result in invalid pointer in resume/suspend
>> callbacks.
> Not sure but seems like to need more review and test anyway.
>
>> The problem is common for componentized drivers and was already reported [1],
>> but there
>> is no general solution for it. On the other side probability it will ever happen
>> seems to be very low.
>>
>> [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/115820
>>
>>>> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
>>>>   for historical reasons, these function can be merged together.
>>> Also can you show me more detail?
>> This is quite simple, I will post patch for it.
> Thanks for sharing.
>
> Anyway, I believe Gustavo to take care of this patch series after back from his vacation.
>
> Thanks,
> Inki Dae
>
>> Regards
>> Andrzej
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> Changelog v2:
>>>>> - Remove patch 5 and 6.
>>>>>   . commit callback are already removed so isn't required anymore.
>>>>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>>>>   . Really not mendatory for FIMD uses DP, and it could be different
>>>>>     according to Board.
>>>>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>>>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>>>>     CONFIG_PM_SLEEP.
>>>>>
>>>>> Gustavo Padovan (7):
>>>>>   drm/exynos: do not start enabling DP at bind() phase
>>>>>   drm/exynos: add pm_runtime to DP
>>>>>   drm/exynos: add pm_runtime to HDMI
>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>   drm/exynos: add pm_runtime to DECON 5433
>>>>>   drm/exynos: add pm_runtime to DECON 7
>>>>>
>>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>>>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>>>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>>>>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>>>>>  7 files changed, 352 insertions(+), 265 deletions(-)
>>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>  
>>>  
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

2015년 11월 04일 19:13에 Andrzej Hajda 이(가) 쓴 글:
> On 11/04/2015 08:56 AM, Inki Dae wrote:
>>
>> 2015년 11월 04일 16:24에 Andrzej Hajda 이(가) 쓴 글:
>>> On 11/03/2015 04:38 PM, Inki Dae wrote:
>>>>  
>>>> 2015-11-03 22:24 GMT+09:00 Andrzej Hajda <a.hajda@samsung.com
>>>> <mailto:a.hajda@samsung.com>>:
>>>>> Hi Inki,
>>>>>
>>>>> On 11/03/2015 11:47 AM, Inki Dae wrote:
>>>>>> This patch series adds pm runtime support for Exynos drm.
>>>>>>
>>>>>> Originally, this patch was posted by Gustavo but there was no any
>>>>>> answer about some comments. So I rebased this patch series on top of
>>>>>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>>>>> I have sent comment to original patchset[1], but for some strange reasons
>>>>> it went only to mailing lists.
>>>>> My concerns were as follows:
>>>>> - exynos_drm has already pm_runtime support via exynos_drm_drv pm ops,
>>>>>   why should we add per component support?
>>>>  
>>>> For this, I think I already mentioned enough,
>>>>         http://www.spinics.net/lists/linux-samsung-soc/msg47695.html
>>>>
>>>> In brief, exynos_drm_drv doesn't control each power domain of each kms device.
>>>> It means that we cannot power off fully without pm runtime interface of each
>>>> kms device - just they can only control their clock not power domain. So
>>>> question. How we could control power domain of each kms device without runtime
>>>> pm interface?
>>> Hmm, but to enable pm_runtime in components it is enough to use
>>> pm_runtime_enable/disable and pm_runtime_get/put(_sync), there is no need to add
>>> pm callbacks.
>> That is this patch series does, and pm callback is implemented in exynos_drm_drv not in component drivers.
> 
> But this patchset adds runtime_pm ops to each component. Runtime_pm support does
> not require
> implementing runtime_pm ops, they just increases complexity of the code, and I
> see no gain in splitting
> power off/on sequence between drm enable/disable callbacks and suspend/resume
> callbacks.

Seems reasonable but if there is no any implemention of runtime pm ops and
only runtime pm api is called in enable or disable callbacks of each compoment
driver, then we wouldn't know which component driver runtime pm request failed
at - we could check if the power domain of each compoment device is enabled
or disabled correctly by checking each compoment's runtime pm suspend/resume call.

So I think it'd be better to implement component's runtime pm ops to make sure
to check if the runtime pm call successed, and I think also to fulfill
the use of runtime pm api correctly.

Anyway, I'd like to open this patchset for more review for a while before going to -next.
Welcome to any opinions.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>
>>> In fact most of the components already supports runtime pm,  but quick grep
>>> shows that it is not implemented in:
>>> exynos_drm_dsi.c, exynos_dp_core.c, exynos_drm_mic.c.
>> exynos_dp_core already has runtime pm interface with this patch series. For dsi and mic, we need to add the runtime pm interfaces.
>>
>>> So I guess adding pm_runtime support by adding calls pm_runtime_enable/disable
>>> and pm_runtime_get/put(_sync) in appropriate places should be enough.
>>>
>>>>> - component suspend sequence is non deterministic, but in case of
>>>>>   video pipelines, specification often requires fixed order,
>>>> Can your show me an example? I think component suspend and resume are invoked
>>>> by only drm dpms interface, and the dpms interface has a fixed order - when
>>>> dpms goes to off, encoder first, and when dpms goes to on, crtc first.
>>> Now as I understand you do not want to remove exynos_drm_drv pm callbacks so
>>> they will disable devices properly, after that clock disabling order should not
>>> matter, so the whole point is not valid.
>> In this case, the dpms actually is invoked by SLEEP PM of Linux power management framework. DRM KMS has a interface to control power of kms device, which is really different from SLEEP PM.
>> So this request can be done by userspace in runtime - just Display power off/on.
>>
>>>> My only concern is that runtime pm interface of each kms driver is called
>>>> within pm sleep context of Exynos drm driver. So I will look into this no problem.
>>>>
>>>>> - the patchset adds implicit dependency on PM_SLEEP.
>>>>>
>>>>>
>>>>> Current solution should work correctly and it was OK last time I have tested it.
>>>>> I am not sure about atomic requirements, are there special ones?
>>>> Not related to atomic feature.
>>>>
>>>>> There are other issues with current solution, rather easy to solve:
>>>>> - it assumes that exynos-drm device will be suspended first - it should be true,
>>>>>  as it is created at the end and suspend order is reverse to creation order, but
>>>>>  I am not sure if we can rely on it - some solution is to add pm callbacks to
>>>>>  all components, and from those callbacks call one centralized pm routine,
>>>> You mean pm callbacks are pm sleep interface? And you mean let's add sleep pm
>>>> interface to each kms driver?  If so, I'm not agree with you because sleep pm
>>>> should be controlled by only drm top like single driver does.
>>> Lets look at an example:
>>> Assume we have present only fimd and dsi components.
>>> In such case kernel creates two platform devices for fimd and dsi in early stage
>>> of parsing device tree blob.
>>> During exynos_drm_drv initialization exynos-drm platform device is created, the
>>> important thing it is created after fimd and dsi.
>>> Now when platform enters sleep mode suspend callbacks are called, the important
>>> thing here is
>>> in which order. If I remember correctly PM guarantees only that children will be
>>> handled before parents.
>>> Since we have no parent-child relation between exynos-drm, fimd and dsi, the
>>> order is unknown.
>> exynos-drm is not real hardware. So the order is only valid for kms devices - components.
>> The only thing exynos-drm does is to invoke dpms so that kms devices are enabled or disabled in fixed order.
>>
>> And as I mentioned before, my only concern is that runtime pm interface of each kms driver is called
>> within pm sleep context of Exynos drm driver. So required for more review and test.
>>
>>> So it will be possible that component will enter sleep mode before exynos-drm
>> How component can enter sleep mode before exynos-drm? compoments have no sleep pm interfaces but only runtime pm interface,
>> and even the runtime pm interfaces will be called by pm interfaces of exynos-drm only when sleep pm is requested.
>> Otherwise, the runtime pm interface of each component will be called by dpms request from userspace.
>>
>>> and in such case system can even
>>> hang if exynos-drm callbacks will try to access registers of such component.
>> So seems not true.
>>
>>> Fortunately in current implementation of PM order of suspending is reversed
>>> order of device creation, so
>>> it will be always exynos-drm first.
>>>
>>> So in short we rely here on implementation detail of PM framework.
>>>
>>>>> - suspend/resume callbacks theoretically can be called during component
>>>>>   master initialization/deinitailization it could be racy,
>>>> I'm not sure what you mean but component master
>>>> initialization/deinitailization mean bind and unbind? If so, bind/unbind
>>>> interfaces of all components will never call pm relevant interfaces but the
>>>> the pm relevant interfaces are called by only dpms interface.
>>> exynos_drm_sys_suspend and exynos_drm_sys_resume takes pointer
>>> to drm_device and uses it. It is possible that in the same time drm_driver
>>> is destroyed for some reason it will result in invalid pointer in resume/suspend
>>> callbacks.
>> Not sure but seems like to need more review and test anyway.
>>
>>> The problem is common for componentized drivers and was already reported [1],
>>> but there
>>> is no general solution for it. On the other side probability it will ever happen
>>> seems to be very low.
>>>
>>> [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/115820
>>>
>>>>> - exynos_drm_sys_suspend/resume calls exynos_drm_suspend/resume
>>>>>   for historical reasons, these function can be merged together.
>>>> Also can you show me more detail?
>>> This is quite simple, I will post patch for it.
>> Thanks for sharing.
>>
>> Anyway, I believe Gustavo to take care of this patch series after back from his vacation.
>>
>> Thanks,
>> Inki Dae
>>
>>> Regards
>>> Andrzej
>>>
>>>> Thanks,
>>>> Inki Dae
>>>>
>>>>> [1]: http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/48395
>>>>>
>>>>> Regards
>>>>> Andrzej
>>>>>
>>>>>> Changelog v2:
>>>>>> - Remove patch 5 and 6.
>>>>>>   . commit callback are already removed so isn't required anymore.
>>>>>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>>>>>   . Really not mendatory for FIMD uses DP, and it could be different
>>>>>>     according to Board.
>>>>>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>>>>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>>>>>     CONFIG_PM_SLEEP.
>>>>>>
>>>>>> Gustavo Padovan (7):
>>>>>>   drm/exynos: do not start enabling DP at bind() phase
>>>>>>   drm/exynos: add pm_runtime to DP
>>>>>>   drm/exynos: add pm_runtime to HDMI
>>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>>   drm/exynos: add pm_runtime to DECON 5433
>>>>>>   drm/exynos: add pm_runtime to DECON 7
>>>>>>
>>>>>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  54 ++++++---
>>>>>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 125 +++++++++----------
>>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.c       | 165 +++++++++++++++++++-------
>>>>>>  drivers/gpu/drm/exynos/exynos_dp_core.h       |   1 +
>>>>>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  91 ++++++--------
>>>>>>  drivers/gpu/drm/exynos/exynos_hdmi.c          |  56 ++++++---
>>>>>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 125 ++++++++++---------
>>>>>>  7 files changed, 352 insertions(+), 265 deletions(-)
>>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org <mailto:dri-devel@lists.freedesktop.org>
>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>  
>>>>  
>>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

[adding Kevin and Tyler to cc list]

Hello Inki and Gustavo,

On 11/03/2015 07:47 AM, Inki Dae wrote:
> This patch series adds pm runtime support for Exynos drm.
> 
> Originally, this patch was posted by Gustavo but there was no any
> answer about some comments. So I rebased this patch series on top of
> exynos-drm-next, removed unnecessary patches and modified wrong macro.
> 
> Changelog v2:
> - Remove patch 5 and 6.
>   . commit callback are already removed so isn't required anymore.
> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>   . Really not mendatory for FIMD uses DP, and it could be different
>     according to Board.
> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>     CONFIG_PM_SLEEP.
>

This series causes a boot failure on at least an Exynos5800 Peach Pi
Chromebook (tested myself) and seems to be the cause of other Exynos
boards failing to boot: http://kernelci.org/boot/?exynos&fail

[snip]

>   drm/exynos: add pm_runtime to Mixer
>   drm/exynos: add pm_runtime to FIMD

I had to revert these patches in order to get the machine in a bootable
state again, the sha1 hash for these patches in next-20151119 are:

045febd5f813 drm/exynos: add pm_runtime to FIMD
dd766fb8479b drm/exynos: add pm_runtime to Mixer

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

On 11/19/2015 11:51 AM, Javier Martinez Canillas wrote:
> [adding Kevin and Tyler to cc list]
>
> Hello Inki and Gustavo,

I didn't notice before that Gustavo was not cc'ed in the
original email from Inki so I'm adding him as well.

Sorry to others for the noise.

> 
> On 11/03/2015 07:47 AM, Inki Dae wrote:
>> This patch series adds pm runtime support for Exynos drm.
>>
>> Originally, this patch was posted by Gustavo but there was no any
>> answer about some comments. So I rebased this patch series on top of
>> exynos-drm-next, removed unnecessary patches and modified wrong macro.
>>
>> Changelog v2:
>> - Remove patch 5 and 6.
>>   . commit callback are already removed so isn't required anymore.
>> - Remove patch 8 which makes dp clock enabled directly from FIMD.
>>   . Really not mendatory for FIMD uses DP, and it could be different
>>     according to Board.
>> - Modified CONFIG_PM_SLEEP to CONFIG_PM.
>>   . In case of runtime pm, CONFIG_PM macro should be used instead of
>>     CONFIG_PM_SLEEP.
>>
> 
> This series causes a boot failure on at least an Exynos5800 Peach Pi
> Chromebook (tested myself) and seems to be the cause of other Exynos
> boards failing to boot: http://kernelci.org/boot/?exynos&fail
> 
> [snip]
> 
>>   drm/exynos: add pm_runtime to Mixer
>>   drm/exynos: add pm_runtime to FIMD
> 
> I had to revert these patches in order to get the machine in a bootable
> state again, the sha1 hash for these patches in next-20151119 are:
> 
> 045febd5f813 drm/exynos: add pm_runtime to FIMD
> dd766fb8479b drm/exynos: add pm_runtime to Mixer
> 
> Best regards,
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>
>>
>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>> Chromebook (tested myself) and seems to be the cause of other Exynos
>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>
>> [snip]
>>
>>>   drm/exynos: add pm_runtime to Mixer
>>>   drm/exynos: add pm_runtime to FIMD
>>
>> I had to revert these patches in order to get the machine in a bootable
>> state again, the sha1 hash for these patches in next-20151119 are:
>>
>> 045febd5f813 drm/exynos: add pm_runtime to FIMD

On a closer look, only reverting the FIMD patch is enough
to make at least the Exynos5800 Peach Pi to boot again.

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

Hi Javier,

2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>
>>>
>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>
>>> [snip]
>>>
>>>>   drm/exynos: add pm_runtime to Mixer
>>>>   drm/exynos: add pm_runtime to FIMD
>>>
>>> I had to revert these patches in order to get the machine in a bootable
>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>
>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
> 
> On a closer look, only reverting the FIMD patch is enough
> to make at least the Exynos5800 Peach Pi to boot again.

Thanks for report.

I assume that the issue is because above patch removed 'suspended' variable
for checking the suspend status in runtime so I revived it.

I'm not sure that the change could resolve the issue. Could you test it
with the change again? I have no Exynos5800 Peach Pi board. :(

For this, I pushed it to below exynos-drm/for-next branch,
	https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c

If the issue is resolved by the change then I will modify other patches for
DECON series. And if really so, there may be a corner case we missed.

Thanks,
Inki Dae

> 
> Best regards,
> 

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

2015년 11월 20일 19:59에 Inki Dae 이(가) 쓴 글:
> Hi Javier,
> 
> 2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>>
>>>>
>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>>
>>>> [snip]
>>>>
>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>
>>>> I had to revert these patches in order to get the machine in a bootable
>>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>>
>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
>>
>> On a closer look, only reverting the FIMD patch is enough
>> to make at least the Exynos5800 Peach Pi to boot again.
> 
> Thanks for report.
> 
> I assume that the issue is because above patch removed 'suspended' variable
> for checking the suspend status in runtime so I revived it.
> 
> I'm not sure that the change could resolve the issue. Could you test it
> with the change again? I have no Exynos5800 Peach Pi board. :(
> 
> For this, I pushed it to below exynos-drm/for-next branch,
> 	https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c
> 
> If the issue is resolved by the change then I will modify other patches for
> DECON series. And if really so, there may be a corner case we missed.

Oops, I found out one error at the boot log,
http://storage.kernelci.org/next/next-20151120/arm-multi_v7_defconfig+CONFIG_LKDTM=y/lab-collabora/boot-exynos5800-peach-pi.html

The boot log says,
[    5.754493] vdd_ldo9: supplied by vdd_2v
[    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000

Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.

Below is dp node description of exynos5420-peach-pit.dts file.
&dp {
	status = "okay";
	pinctrl-names = "default";
	pinctrl-0 = <&dp_hpd_gpio>;
	samsung,color-space = <0>;
	samsung,dynamic-range = <0>;
	samsung,ycbcr-coeff = <0>;
	samsung,color-depth = <1>;
	samsung,link-rate = <0x06>;
	samsung,lane-count = <2>;
	samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;

	ports {
		port@0 {
			dp_out: endpoint {
				remote-endpoint = <&bridge_in>;
			};
		};
	};
};

And below is for exynos5800-peash-pit.dts,
&dp {
	status = "okay";
	pinctrl-names = "default";
	pinctrl-0 = <&dp_hpd_gpio>;
	samsung,color-space = <0>;
	samsung,dynamic-range = <0>;
	samsung,ycbcr-coeff = <0>;
	samsung,color-depth = <1>;
	samsung,link-rate = <0x0a>;
	samsung,lane-count = <2>;
	samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
	panel = <&panel>;
};

Thanks,
Inki Dae

> 
> Thanks,
> Inki Dae
> 
>>
>> Best regards,
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

Hello Inki,

On 11/20/2015 08:13 AM, Inki Dae wrote:
> 
> 
> 2015년 11월 20일 19:59에 Inki Dae 이(가) 쓴 글:
>> Hi Javier,
>>
>> 2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
>>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>>>
>>>>>
>>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>>>
>>>>> [snip]
>>>>>
>>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>
>>>>> I had to revert these patches in order to get the machine in a bootable
>>>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>>>
>>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
>>>
>>> On a closer look, only reverting the FIMD patch is enough
>>> to make at least the Exynos5800 Peach Pi to boot again.
>>
>> Thanks for report.
>>
>> I assume that the issue is because above patch removed 'suspended' variable
>> for checking the suspend status in runtime so I revived it.
>>
>> I'm not sure that the change could resolve the issue. Could you test it
>> with the change again? I have no Exynos5800 Peach Pi board. :(
>>
>> For this, I pushed it to below exynos-drm/for-next branch,
>> 	https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c
>>
>> If the issue is resolved by the change then I will modify other patches for
>> DECON series. And if really so, there may be a corner case we missed.
> 
> Oops, I found out one error at the boot log,
> http://storage.kernelci.org/next/next-20151120/arm-multi_v7_defconfig+CONFIG_LKDTM=y/lab-collabora/boot-exynos5800-peach-pi.html
> 
> The boot log says,
> [    5.754493] vdd_ldo9: supplied by vdd_2v
> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>

This message is a red herring for the reported issue, the message is also
present when the machine boots and the display is brought correctly.
 
> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>
> Below is dp node description of exynos5420-peach-pit.dts file.
> &dp {
> 	status = "okay";
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&dp_hpd_gpio>;
> 	samsung,color-space = <0>;
> 	samsung,dynamic-range = <0>;
> 	samsung,ycbcr-coeff = <0>;
> 	samsung,color-depth = <1>;
> 	samsung,link-rate = <0x06>;
> 	samsung,lane-count = <2>;
> 	samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
> 
> 	ports {
> 		port@0 {
> 			dp_out: endpoint {
> 				remote-endpoint = <&bridge_in>;
> 			};
> 		};
> 	};
> };
> 
> And below is for exynos5800-peash-pit.dts,
> &dp {
> 	status = "okay";
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&dp_hpd_gpio>;
> 	samsung,color-space = <0>;
> 	samsung,dynamic-range = <0>;
> 	samsung,ycbcr-coeff = <0>;
> 	samsung,color-depth = <1>;
> 	samsung,link-rate = <0x0a>;
> 	samsung,lane-count = <2>;
> 	samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
> 	panel = <&panel>;
> };
> 

The difference is because the Exynos5420 Peach Pit Display Port is not
attached directly to the display panel, there is an eDP/LVDS bridge chip
in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.

The Exynos DP driver lookups for either a panel phandle or an OF graph
endpoint that points to a bridge chip and the bridge enpoint has a port
that points to the panel.

So the DT is correct but of_graph_get_next_endpoint() always prints an
error if the port so OF graph endpoints it seems can't be optional as
used in this driver. Maybe that message should be change to debug then?

Another option is to extend the DP driver DT binding to be more generic
supporting having a port to a panel besides a bridge, so we could have
something like this for Exynos5800 Peach and be consistent in both cases:

diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index 7b018e451880..9c6fd7314ee0 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -122,6 +122,12 @@
 		compatible = "auo,b133htn01";
 		power-supply = <&tps65090_fet6>;
 		backlight = <&backlight>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&dp_out>;
+			};
+		};
 	};
 
 	mmc1_pwrseq: mmc1_pwrseq {
@@ -148,7 +154,14 @@
 	samsung,link-rate = <0x0a>;
 	samsung,lane-count = <2>;
 	samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
-	panel = <&panel>;
+
+	ports {
+		port@0 {
+			dp_out: endpoint {
+				remote-endpoint = <&panel_in>;
+			};
+		};
+	};
 };
 
 &fimd {

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

Hi Javier,

2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello Inki,
>
> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>
>>
>> 2015년 11월 20일 19:59에 Inki Dae 이(가) 쓴 글:
>>> Hi Javier,
>>>
>>> 2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
>>>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>>>>
>>>>>>
>>>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>>>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>>
>>>>>> I had to revert these patches in order to get the machine in a bootable
>>>>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>>>>
>>>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
>>>>
>>>> On a closer look, only reverting the FIMD patch is enough
>>>> to make at least the Exynos5800 Peach Pi to boot again.
>>>
>>> Thanks for report.
>>>
>>> I assume that the issue is because above patch removed 'suspended' variable
>>> for checking the suspend status in runtime so I revived it.
>>>
>>> I'm not sure that the change could resolve the issue. Could you test it
>>> with the change again? I have no Exynos5800 Peach Pi board. :(
>>>
>>> For this, I pushed it to below exynos-drm/for-next branch,
>>>      https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c
>>>
>>> If the issue is resolved by the change then I will modify other patches for
>>> DECON series. And if really so, there may be a corner case we missed.
>>
>> Oops, I found out one error at the boot log,
>> http://storage.kernelci.org/next/next-20151120/arm-multi_v7_defconfig+CONFIG_LKDTM=y/lab-collabora/boot-exynos5800-peach-pi.html
>>
>> The boot log says,
>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>
>
> This message is a red herring for the reported issue, the message is also
> present when the machine boots and the display is brought correctly.
>
>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>
>> Below is dp node description of exynos5420-peach-pit.dts file.
>> &dp {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hpd_gpio>;
>>       samsung,color-space = <0>;
>>       samsung,dynamic-range = <0>;
>>       samsung,ycbcr-coeff = <0>;
>>       samsung,color-depth = <1>;
>>       samsung,link-rate = <0x06>;
>>       samsung,lane-count = <2>;
>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>
>>       ports {
>>               port@0 {
>>                       dp_out: endpoint {
>>                               remote-endpoint = <&bridge_in>;
>>                       };
>>               };
>>       };
>> };
>>
>> And below is for exynos5800-peash-pit.dts,
>> &dp {
>>       status = "okay";
>>       pinctrl-names = "default";
>>       pinctrl-0 = <&dp_hpd_gpio>;
>>       samsung,color-space = <0>;
>>       samsung,dynamic-range = <0>;
>>       samsung,ycbcr-coeff = <0>;
>>       samsung,color-depth = <1>;
>>       samsung,link-rate = <0x0a>;
>>       samsung,lane-count = <2>;
>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>       panel = <&panel>;
>> };
>>
>
> The difference is because the Exynos5420 Peach Pit Display Port is not
> attached directly to the display panel, there is an eDP/LVDS bridge chip
> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>
> The Exynos DP driver lookups for either a panel phandle or an OF graph
> endpoint that points to a bridge chip and the bridge enpoint has a port
> that points to the panel.
>
> So the DT is correct but of_graph_get_next_endpoint() always prints an

Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
board doesn't use eDP, then the dp node __should be removed__ from
exynos5800-peach-pit.dts.

From a common-sense standpoint, there is no any reason to build
and probe dp driver if the board doesn't use dp hardware.

> error if the port so OF graph endpoints it seems can't be optional as
> used in this driver. Maybe that message should be change to debug then?
>
> Another option is to extend the DP driver DT binding to be more generic
> supporting having a port to a panel besides a bridge, so we could have
> something like this for Exynos5800 Peach and be consistent in both cases:

It's really not good. This would make it more complex. The best
solution is just to
remove the dt node from the device tree file.

Thanks,
Inki Dae

>
> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> index 7b018e451880..9c6fd7314ee0 100644
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> @@ -122,6 +122,12 @@
>                 compatible = "auo,b133htn01";
>                 power-supply = <&tps65090_fet6>;
>                 backlight = <&backlight>;
> +
> +               port {
> +                       panel_in: endpoint {
> +                               remote-endpoint = <&dp_out>;
> +                       };
> +               };
>         };
>
>         mmc1_pwrseq: mmc1_pwrseq {
> @@ -148,7 +154,14 @@
>         samsung,link-rate = <0x0a>;
>         samsung,lane-count = <2>;
>         samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
> -       panel = <&panel>;
> +
> +       ports {
> +               port@0 {
> +                       dp_out: endpoint {
> +                               remote-endpoint = <&panel_in>;
> +                       };
> +               };
> +       };
>  };
>
>  &fimd {
>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Daniel Stone]

Hi Inki,

On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>> The boot log says,
>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>
>>
>> This message is a red herring for the reported issue, the message is also
>> present when the machine boots and the display is brought correctly.
>>
>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>
>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>> &dp {
>>>       status = "okay";
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>       samsung,color-space = <0>;
>>>       samsung,dynamic-range = <0>;
>>>       samsung,ycbcr-coeff = <0>;
>>>       samsung,color-depth = <1>;
>>>       samsung,link-rate = <0x06>;
>>>       samsung,lane-count = <2>;
>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>
>>>       ports {
>>>               port@0 {
>>>                       dp_out: endpoint {
>>>                               remote-endpoint = <&bridge_in>;
>>>                       };
>>>               };
>>>       };
>>> };
>>>
>>> And below is for exynos5800-peash-pit.dts,
>>> &dp {
>>>       status = "okay";
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>       samsung,color-space = <0>;
>>>       samsung,dynamic-range = <0>;
>>>       samsung,ycbcr-coeff = <0>;
>>>       samsung,color-depth = <1>;
>>>       samsung,link-rate = <0x0a>;
>>>       samsung,lane-count = <2>;
>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>       panel = <&panel>;
>>> };
>>>
>>
>> The difference is because the Exynos5420 Peach Pit Display Port is not
>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>
>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>> endpoint that points to a bridge chip and the bridge enpoint has a port
>> that points to the panel.
>>
>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>
> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
> board doesn't use eDP, then the dp node __should be removed__ from
> exynos5800-peach-pit.dts.
>
> From a common-sense standpoint, there is no any reason to build
> and probe dp driver if the board doesn't use dp hardware.

I agree with what you say, but unfortunately you've slightly misread
what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
which I am writing this) has an eDP panel directly connected. The DT
describes both the eDP connector from FIMD and the eDP panel, except
that there is no connection between the DT nodes.

>> error if the port so OF graph endpoints it seems can't be optional as
>> used in this driver. Maybe that message should be change to debug then?
>>
>> Another option is to extend the DP driver DT binding to be more generic
>> supporting having a port to a panel besides a bridge, so we could have
>> something like this for Exynos5800 Peach and be consistent in both cases:
>
> It's really not good. This would make it more complex. The best
> solution is just to
> remove the dt node from the device tree file.

Given the above, not really. Javier's patch seems correct to me - as
you can see, there is a panel node, and that is the panel that's
really connected.

>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>> @@ -122,6 +122,12 @@
>>                 compatible = "auo,b133htn01";
>>                 power-supply = <&tps65090_fet6>;
>>                 backlight = <&backlight>;
>> +
>> +               port {
>> +                       panel_in: endpoint {
>> +                               remote-endpoint = <&dp_out>;
>> +                       };
>> +               };
>>         };
>>
>>         mmc1_pwrseq: mmc1_pwrseq {
>> @@ -148,7 +154,14 @@
>>         samsung,link-rate = <0x0a>;
>>         samsung,lane-count = <2>;
>>         samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>> -       panel = <&panel>;
>> +
>> +       ports {
>> +               port@0 {
>> +                       dp_out: endpoint {
>> +                               remote-endpoint = <&panel_in>;
>> +                       };
>> +               };
>> +       };
>>  };

Cheers,
Daniel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

Hi Daniel,


2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
> Hi Inki,
>
> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>> The boot log says,
>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>
>>>
>>> This message is a red herring for the reported issue, the message is also
>>> present when the machine boots and the display is brought correctly.
>>>
>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>
>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>> &dp {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>       samsung,color-space = <0>;
>>>>       samsung,dynamic-range = <0>;
>>>>       samsung,ycbcr-coeff = <0>;
>>>>       samsung,color-depth = <1>;
>>>>       samsung,link-rate = <0x06>;
>>>>       samsung,lane-count = <2>;
>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>
>>>>       ports {
>>>>               port@0 {
>>>>                       dp_out: endpoint {
>>>>                               remote-endpoint = <&bridge_in>;
>>>>                       };
>>>>               };
>>>>       };
>>>> };
>>>>
>>>> And below is for exynos5800-peash-pit.dts,
>>>> &dp {
>>>>       status = "okay";
>>>>       pinctrl-names = "default";
>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>       samsung,color-space = <0>;
>>>>       samsung,dynamic-range = <0>;
>>>>       samsung,ycbcr-coeff = <0>;
>>>>       samsung,color-depth = <1>;
>>>>       samsung,link-rate = <0x0a>;
>>>>       samsung,lane-count = <2>;
>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>       panel = <&panel>;
>>>> };
>>>>
>>>
>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>
>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>> that points to the panel.
>>>
>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>
>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>> board doesn't use eDP, then the dp node __should be removed__ from
>> exynos5800-peach-pit.dts.
>>
>> From a common-sense standpoint, there is no any reason to build
>> and probe dp driver if the board doesn't use dp hardware.
>
> I agree with what you say, but unfortunately you've slightly misread
> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
> which I am writing this) has an eDP panel directly connected. The DT
> describes both the eDP connector from FIMD and the eDP panel, except
> that there is no connection between the DT nodes.

Right. I misread what Javier said. :)

>
>>> error if the port so OF graph endpoints it seems can't be optional as
>>> used in this driver. Maybe that message should be change to debug then?
>>>
>>> Another option is to extend the DP driver DT binding to be more generic
>>> supporting having a port to a panel besides a bridge, so we could have
>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>
>> It's really not good. This would make it more complex. The best
>> solution is just to
>> remove the dt node from the device tree file.
>
> Given the above, not really. Javier's patch seems correct to me - as
> you can see, there is a panel node, and that is the panel that's
> really connected.

Indeed. Javier's patch will correct it.

Thanks,
Inki Dae

>
>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>> @@ -122,6 +122,12 @@
>>>                 compatible = "auo,b133htn01";
>>>                 power-supply = <&tps65090_fet6>;
>>>                 backlight = <&backlight>;
>>> +
>>> +               port {
>>> +                       panel_in: endpoint {
>>> +                               remote-endpoint = <&dp_out>;
>>> +                       };
>>> +               };
>>>         };
>>>
>>>         mmc1_pwrseq: mmc1_pwrseq {
>>> @@ -148,7 +154,14 @@
>>>         samsung,link-rate = <0x0a>;
>>>         samsung,lane-count = <2>;
>>>         samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>> -       panel = <&panel>;
>>> +
>>> +       ports {
>>> +               port@0 {
>>> +                       dp_out: endpoint {
>>> +                               remote-endpoint = <&panel_in>;
>>> +                       };
>>> +               };
>>> +       };
>>>  };
>
> Cheers,
> Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

Hello,

On 11/21/2015 11:59 AM, Inki Dae wrote:
> Hi Daniel,
> 
> 
> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>> Hi Inki,
>>
>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>> The boot log says,
>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>
>>>>
>>>> This message is a red herring for the reported issue, the message is also
>>>> present when the machine boots and the display is brought correctly.
>>>>
>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>
>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>> &dp {
>>>>>       status = "okay";
>>>>>       pinctrl-names = "default";
>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>       samsung,color-space = <0>;
>>>>>       samsung,dynamic-range = <0>;
>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>       samsung,color-depth = <1>;
>>>>>       samsung,link-rate = <0x06>;
>>>>>       samsung,lane-count = <2>;
>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>
>>>>>       ports {
>>>>>               port@0 {
>>>>>                       dp_out: endpoint {
>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>                       };
>>>>>               };
>>>>>       };
>>>>> };
>>>>>
>>>>> And below is for exynos5800-peash-pit.dts,
>>>>> &dp {
>>>>>       status = "okay";
>>>>>       pinctrl-names = "default";
>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>       samsung,color-space = <0>;
>>>>>       samsung,dynamic-range = <0>;
>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>       samsung,color-depth = <1>;
>>>>>       samsung,link-rate = <0x0a>;
>>>>>       samsung,lane-count = <2>;
>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>       panel = <&panel>;
>>>>> };
>>>>>
>>>>
>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>
>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>> that points to the panel.
>>>>
>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>
>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>> board doesn't use eDP, then the dp node __should be removed__ from
>>> exynos5800-peach-pit.dts.
>>>
>>> From a common-sense standpoint, there is no any reason to build
>>> and probe dp driver if the board doesn't use dp hardware.
>>
>> I agree with what you say, but unfortunately you've slightly misread
>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>> which I am writing this) has an eDP panel directly connected. The DT

Thanks a lot Daniel for clarifying my comments to Inki :)

>> describes both the eDP connector from FIMD and the eDP panel, except
>> that there is no connection between the DT nodes.

There *is* a connection between the FIMD eDP connector and the eDP panel
nodes but these are connected using a phandle while the connection for
the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
bindings (Documentation/devicetree/bindings/graph.txt).

And also the connection between the eDP/LVDS bridge and the LVDS panel
is using an OF graph node, so what I meant is that it would be much more
consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
connections used the OF graph DT bindings.

>
> Right. I misread what Javier said. :)
>
>>
>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>> used in this driver. Maybe that message should be change to debug then?
>>>>
>>>> Another option is to extend the DP driver DT binding to be more generic
>>>> supporting having a port to a panel besides a bridge, so we could have
>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>
>>> It's really not good. This would make it more complex. The best
>>> solution is just to
>>> remove the dt node from the device tree file.
>>
>> Given the above, not really. Javier's patch seems correct to me - as
>> you can see, there is a panel node, and that is the panel that's
>> really connected.
> 
> Indeed. Javier's patch will correct it.
>

Just to be clear, my patch is not correct since the Exynos DP driver and
its DT binding does not support to connect an FIMD eDP connector to an
eDP panel directly using OF graph ports / endpoints (only a phandle). But
is an example of how the DT will look like if we extend to support that.

IIRC at the beginning only eDP -> panel was supported and the phandle
was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
case was needed, then a bridge phandle was added but Ajay was asked to
use OF graph instead a phandle and we ended with different approaches
to connect components depending if a bridge is used or not.

> Thanks,
> Inki Dae
> 
>>
>>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>>> @@ -122,6 +122,12 @@
>>>>                 compatible = "auo,b133htn01";
>>>>                 power-supply = <&tps65090_fet6>;
>>>>                 backlight = <&backlight>;
>>>> +
>>>> +               port {
>>>> +                       panel_in: endpoint {
>>>> +                               remote-endpoint = <&dp_out>;
>>>> +                       };
>>>> +               };
>>>>         };
>>>>
>>>>         mmc1_pwrseq: mmc1_pwrseq {
>>>> @@ -148,7 +154,14 @@
>>>>         samsung,link-rate = <0x0a>;
>>>>         samsung,lane-count = <2>;
>>>>         samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>> -       panel = <&panel>;
>>>> +
>>>> +       ports {
>>>> +               port@0 {
>>>> +                       dp_out: endpoint {
>>>> +                               remote-endpoint = <&panel_in>;
>>>> +                       };
>>>> +               };
>>>> +       };
>>>>  };
>>
>> Cheers,
>> Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello,
>
> On 11/21/2015 11:59 AM, Inki Dae wrote:
>> Hi Daniel,
>>
>>
>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>> Hi Inki,
>>>
>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>> The boot log says,
>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>
>>>>>
>>>>> This message is a red herring for the reported issue, the message is also
>>>>> present when the machine boots and the display is brought correctly.
>>>>>
>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>
>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>> &dp {
>>>>>>       status = "okay";
>>>>>>       pinctrl-names = "default";
>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>       samsung,color-space = <0>;
>>>>>>       samsung,dynamic-range = <0>;
>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>       samsung,color-depth = <1>;
>>>>>>       samsung,link-rate = <0x06>;
>>>>>>       samsung,lane-count = <2>;
>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>
>>>>>>       ports {
>>>>>>               port@0 {
>>>>>>                       dp_out: endpoint {
>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>                       };
>>>>>>               };
>>>>>>       };
>>>>>> };
>>>>>>
>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>> &dp {
>>>>>>       status = "okay";
>>>>>>       pinctrl-names = "default";
>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>       samsung,color-space = <0>;
>>>>>>       samsung,dynamic-range = <0>;
>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>       samsung,color-depth = <1>;
>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>       samsung,lane-count = <2>;
>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>       panel = <&panel>;
>>>>>> };
>>>>>>
>>>>>
>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>
>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>> that points to the panel.
>>>>>
>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>
>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>> exynos5800-peach-pit.dts.
>>>>
>>>> From a common-sense standpoint, there is no any reason to build
>>>> and probe dp driver if the board doesn't use dp hardware.
>>>
>>> I agree with what you say, but unfortunately you've slightly misread
>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>> which I am writing this) has an eDP panel directly connected. The DT
>
> Thanks a lot Daniel for clarifying my comments to Inki :)
>
>>> describes both the eDP connector from FIMD and the eDP panel, except
>>> that there is no connection between the DT nodes.
>
> There *is* a connection between the FIMD eDP connector and the eDP panel
> nodes but these are connected using a phandle while the connection for
> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
> bindings (Documentation/devicetree/bindings/graph.txt).
>
> And also the connection between the eDP/LVDS bridge and the LVDS panel
> is using an OF graph node, so what I meant is that it would be much more
> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
> connections used the OF graph DT bindings.
>
>>
>> Right. I misread what Javier said. :)
>>
>>>
>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>
>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>
>>>> It's really not good. This would make it more complex. The best
>>>> solution is just to
>>>> remove the dt node from the device tree file.
>>>
>>> Given the above, not really. Javier's patch seems correct to me - as
>>> you can see, there is a panel node, and that is the panel that's
>>> really connected.
>>
>> Indeed. Javier's patch will correct it.
>>
>
> Just to be clear, my patch is not correct since the Exynos DP driver and
> its DT binding does not support to connect an FIMD eDP connector to an
> eDP panel directly using OF graph ports / endpoints (only a phandle). But
> is an example of how the DT will look like if we extend to support that.

Yes, you added just a port node for the panel device and removed a panel
property from the device tree file so now dp driver cannot get a device node
object of panel node because now dp driver isn't considered for it yet.

I think there are two ways to correct it. One is,
1. Add a port node for the panel device to the device tree file.
2. Add of_graph dt bindings support for getting the panel node to dp driver
   and remove existing of_parse_phandle function call for getting a device
   node object for the panel device.

Other is,
1. Revive a panel property and remove the port node you added.

In addition, it seems that existing bridge of_graph dt bindings codes of now
dp driver should be modified like below,

endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
if (endpoint) {
        bridge_node = of_graph_get_remote_port_parent(endpoint);
        if (bridge_node) {
                dp->bridge = of_drm_find_bridge(bridge_node);
                of_node_put(bridge_node);
                if (!dp->bridge)
                        return -EPROBE_DEFER;
        } else {
                DRM_ERROR("has no port node for the bridge deivce");
                return -ENXIO;
        }
}

If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
shouldn't be NULL.

The former looks more reasonable to me.

>
> IIRC at the beginning only eDP -> panel was supported and the phandle
> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
> case was needed, then a bridge phandle was added but Ajay was asked to
> use OF graph instead a phandle and we ended with different approaches
> to connect components depending if a bridge is used or not.

Well, wouldn't it be enough to remove the panel phandle relevant codes
from dp driver and add of_graph dt bindings support for the panel device
to the dp driver instead?

Thanks,
Inki Dae

>
>> Thanks,
>> Inki Dae
>>
>>>
>>>>> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>>>> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
>>>>> @@ -122,6 +122,12 @@
>>>>>                 compatible = "auo,b133htn01";
>>>>>                 power-supply = <&tps65090_fet6>;
>>>>>                 backlight = <&backlight>;
>>>>> +
>>>>> +               port {
>>>>> +                       panel_in: endpoint {
>>>>> +                               remote-endpoint = <&dp_out>;
>>>>> +                       };
>>>>> +               };
>>>>>         };
>>>>>
>>>>>         mmc1_pwrseq: mmc1_pwrseq {
>>>>> @@ -148,7 +154,14 @@
>>>>>         samsung,link-rate = <0x0a>;
>>>>>         samsung,lane-count = <2>;
>>>>>         samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>> -       panel = <&panel>;
>>>>> +
>>>>> +       ports {
>>>>> +               port@0 {
>>>>> +                       dp_out: endpoint {
>>>>> +                               remote-endpoint = <&panel_in>;
>>>>> +                       };
>>>>> +               };
>>>>> +       };
>>>>>  };
>>>
>>> Cheers,
>>> Daniel
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

Hello Inki,

On 11/23/2015 01:47 PM, Inki Dae wrote:
> 2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>> Hello,
>>
>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>> Hi Daniel,
>>>
>>>
>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>>> Hi Inki,
>>>>
>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>>> The boot log says,
>>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>>
>>>>>>
>>>>>> This message is a red herring for the reported issue, the message is also
>>>>>> present when the machine boots and the display is brought correctly.
>>>>>>
>>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>>
>>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>>> &dp {
>>>>>>>       status = "okay";
>>>>>>>       pinctrl-names = "default";
>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>       samsung,color-space = <0>;
>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>       samsung,color-depth = <1>;
>>>>>>>       samsung,link-rate = <0x06>;
>>>>>>>       samsung,lane-count = <2>;
>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>
>>>>>>>       ports {
>>>>>>>               port@0 {
>>>>>>>                       dp_out: endpoint {
>>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>>                       };
>>>>>>>               };
>>>>>>>       };
>>>>>>> };
>>>>>>>
>>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>>> &dp {
>>>>>>>       status = "okay";
>>>>>>>       pinctrl-names = "default";
>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>       samsung,color-space = <0>;
>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>       samsung,color-depth = <1>;
>>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>>       samsung,lane-count = <2>;
>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>       panel = <&panel>;
>>>>>>> };
>>>>>>>
>>>>>>
>>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>>
>>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>>> that points to the panel.
>>>>>>
>>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>>
>>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>>> exynos5800-peach-pit.dts.
>>>>>
>>>>> From a common-sense standpoint, there is no any reason to build
>>>>> and probe dp driver if the board doesn't use dp hardware.
>>>>
>>>> I agree with what you say, but unfortunately you've slightly misread
>>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>>> which I am writing this) has an eDP panel directly connected. The DT
>>
>> Thanks a lot Daniel for clarifying my comments to Inki :)
>>
>>>> describes both the eDP connector from FIMD and the eDP panel, except
>>>> that there is no connection between the DT nodes.
>>
>> There *is* a connection between the FIMD eDP connector and the eDP panel
>> nodes but these are connected using a phandle while the connection for
>> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
>> bindings (Documentation/devicetree/bindings/graph.txt).
>>
>> And also the connection between the eDP/LVDS bridge and the LVDS panel
>> is using an OF graph node, so what I meant is that it would be much more
>> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
>> connections used the OF graph DT bindings.
>>
>>>
>>> Right. I misread what Javier said. :)
>>>
>>>>
>>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>>
>>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>>
>>>>> It's really not good. This would make it more complex. The best
>>>>> solution is just to
>>>>> remove the dt node from the device tree file.
>>>>
>>>> Given the above, not really. Javier's patch seems correct to me - as
>>>> you can see, there is a panel node, and that is the panel that's
>>>> really connected.
>>>
>>> Indeed. Javier's patch will correct it.
>>>
>>
>> Just to be clear, my patch is not correct since the Exynos DP driver and
>> its DT binding does not support to connect an FIMD eDP connector to an
>> eDP panel directly using OF graph ports / endpoints (only a phandle). But
>> is an example of how the DT will look like if we extend to support that.
> 
> Yes, you added just a port node for the panel device and removed a panel
> property from the device tree file so now dp driver cannot get a device node
> object of panel node because now dp driver isn't considered for it yet.
> 
> I think there are two ways to correct it. One is,
> 1. Add a port node for the panel device to the device tree file.
> 2. Add of_graph dt bindings support for getting the panel node to dp driver
>    and remove existing of_parse_phandle function call for getting a device
>    node object for the panel device.
>

Exactly.
 
> Other is,
> 1. Revive a panel property and remove the port node you added.
>

Yes, this is the current code that works. Is just that is not consistent but
I don't really mind. I just wanted to explain why the DTS was different for
both boards but it seems that I created more confusion than anything else :)
 
> In addition, it seems that existing bridge of_graph dt bindings codes of now
> dp driver should be modified like below,
> 
> endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
> if (endpoint) {
>         bridge_node = of_graph_get_remote_port_parent(endpoint);
>         if (bridge_node) {
>                 dp->bridge = of_drm_find_bridge(bridge_node);
>                 of_node_put(bridge_node);
>                 if (!dp->bridge)
>                         return -EPROBE_DEFER;
>         } else {
>                 DRM_ERROR("has no port node for the bridge deivce");
>                 return -ENXIO;
>         }
> }
> 
> If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
> shouldn't be NULL.
> 
> The former looks more reasonable to me.
>

I'm not too familiar with the OF graph API but I agree that returning a
-EPROBE_DEFER when of_graph_get_remote_port_parent() returns NULL seems
like the wrong thing to do.

Now I don't know if -ENXIO is the right errno code, maybe -EINVAL (since
means the DTS is invalid)? or maybe just omit that case as it is ommited
if of_graph_get_next_endpoint() fails?

>>
>> IIRC at the beginning only eDP -> panel was supported and the phandle
>> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
>> case was needed, then a bridge phandle was added but Ajay was asked to
>> use OF graph instead a phandle and we ended with different approaches
>> to connect components depending if a bridge is used or not.
> 
> Well, wouldn't it be enough to remove the panel phandle relevant codes
> from dp driver and add of_graph dt bindings support for the panel device
> to the dp driver instead?
>

The problem is that removing the panel phandle is not an option without
breaking DT backward compatibility since now an eDP -> panel lookup by
using a phandle is a DT ABI and old DTBs could be shipped that use it.

So even if the driver and DT binding are extended to allow an eDP -> panel
lookup using ports and endpoints, both approaches have to be kept in the
driver and DT ABI so I don't think the complexity is worth it just for the
sake of consistency.

> Thanks,
> Inki Dae
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

Hi Javier,

2015년 11월 24일 03:38에 Javier Martinez Canillas 이(가) 쓴 글:
> Hello Inki,
> 
> On 11/23/2015 01:47 PM, Inki Dae wrote:
>> 2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>> Hello,
>>>
>>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>>> Hi Daniel,
>>>>
>>>>
>>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>>>> Hi Inki,
>>>>>
>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>>>> The boot log says,
>>>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>>>
>>>>>>>
>>>>>>> This message is a red herring for the reported issue, the message is also
>>>>>>> present when the machine boots and the display is brought correctly.
>>>>>>>
>>>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>>>
>>>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>>>> &dp {
>>>>>>>>       status = "okay";
>>>>>>>>       pinctrl-names = "default";
>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>       samsung,link-rate = <0x06>;
>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>
>>>>>>>>       ports {
>>>>>>>>               port@0 {
>>>>>>>>                       dp_out: endpoint {
>>>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>>>                       };
>>>>>>>>               };
>>>>>>>>       };
>>>>>>>> };
>>>>>>>>
>>>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>>>> &dp {
>>>>>>>>       status = "okay";
>>>>>>>>       pinctrl-names = "default";
>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>       panel = <&panel>;
>>>>>>>> };
>>>>>>>>
>>>>>>>
>>>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>>>
>>>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>>>> that points to the panel.
>>>>>>>
>>>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>>>
>>>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>>>> exynos5800-peach-pit.dts.
>>>>>>
>>>>>> From a common-sense standpoint, there is no any reason to build
>>>>>> and probe dp driver if the board doesn't use dp hardware.
>>>>>
>>>>> I agree with what you say, but unfortunately you've slightly misread
>>>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>>>> which I am writing this) has an eDP panel directly connected. The DT
>>>
>>> Thanks a lot Daniel for clarifying my comments to Inki :)
>>>
>>>>> describes both the eDP connector from FIMD and the eDP panel, except
>>>>> that there is no connection between the DT nodes.
>>>
>>> There *is* a connection between the FIMD eDP connector and the eDP panel
>>> nodes but these are connected using a phandle while the connection for
>>> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
>>> bindings (Documentation/devicetree/bindings/graph.txt).
>>>
>>> And also the connection between the eDP/LVDS bridge and the LVDS panel
>>> is using an OF graph node, so what I meant is that it would be much more
>>> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
>>> connections used the OF graph DT bindings.
>>>
>>>>
>>>> Right. I misread what Javier said. :)
>>>>
>>>>>
>>>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>>>
>>>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>>>
>>>>>> It's really not good. This would make it more complex. The best
>>>>>> solution is just to
>>>>>> remove the dt node from the device tree file.
>>>>>
>>>>> Given the above, not really. Javier's patch seems correct to me - as
>>>>> you can see, there is a panel node, and that is the panel that's
>>>>> really connected.
>>>>
>>>> Indeed. Javier's patch will correct it.
>>>>
>>>
>>> Just to be clear, my patch is not correct since the Exynos DP driver and
>>> its DT binding does not support to connect an FIMD eDP connector to an
>>> eDP panel directly using OF graph ports / endpoints (only a phandle). But
>>> is an example of how the DT will look like if we extend to support that.
>>
>> Yes, you added just a port node for the panel device and removed a panel
>> property from the device tree file so now dp driver cannot get a device node
>> object of panel node because now dp driver isn't considered for it yet.
>>
>> I think there are two ways to correct it. One is,
>> 1. Add a port node for the panel device to the device tree file.
>> 2. Add of_graph dt bindings support for getting the panel node to dp driver
>>    and remove existing of_parse_phandle function call for getting a device
>>    node object for the panel device.
>>
> 
> Exactly.
>  
>> Other is,
>> 1. Revive a panel property and remove the port node you added.
>>
> 
> Yes, this is the current code that works. Is just that is not consistent but
> I don't really mind. I just wanted to explain why the DTS was different for
> both boards but it seems that I created more confusion than anything else :)
>  
>> In addition, it seems that existing bridge of_graph dt bindings codes of now
>> dp driver should be modified like below,
>>
>> endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
>> if (endpoint) {
>>         bridge_node = of_graph_get_remote_port_parent(endpoint);
>>         if (bridge_node) {
>>                 dp->bridge = of_drm_find_bridge(bridge_node);
>>                 of_node_put(bridge_node);
>>                 if (!dp->bridge)
>>                         return -EPROBE_DEFER;
>>         } else {
>>                 DRM_ERROR("has no port node for the bridge deivce");
>>                 return -ENXIO;
>>         }
>> }
>>
>> If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
>> shouldn't be NULL.
>>
>> The former looks more reasonable to me.
>>
> 
> I'm not too familiar with the OF graph API but I agree that returning a
> -EPROBE_DEFER when of_graph_get_remote_port_parent() returns NULL seems
> like the wrong thing to do.
> 
> Now I don't know if -ENXIO is the right errno code, maybe -EINVAL (since
> means the DTS is invalid)? or maybe just omit that case as it is ommited
> if of_graph_get_next_endpoint() fails?
> 
>>>
>>> IIRC at the beginning only eDP -> panel was supported and the phandle
>>> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
>>> case was needed, then a bridge phandle was added but Ajay was asked to
>>> use OF graph instead a phandle and we ended with different approaches
>>> to connect components depending if a bridge is used or not.
>>
>> Well, wouldn't it be enough to remove the panel phandle relevant codes
>> from dp driver and add of_graph dt bindings support for the panel device
>> to the dp driver instead?
>>
> 
> The problem is that removing the panel phandle is not an option without
> breaking DT backward compatibility since now an eDP -> panel lookup by
> using a phandle is a DT ABI and old DTBs could be shipped that use it.

Right. The backward compatibility should be kept.
For this, I think we could update the dp driver like below,

	panel_node = NULL;
	
	/* This is for the backward compatibility. */
	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
	if (panel_node) {
		...
	} else {
		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
		if (endpoint) {
			panel_node = of_graph_get_remote_port_parent(endpoint);
			if (panel_node) {
				...
			} else {
				...
			}
		}
	}

	endpoint = of_graph_get_next_endpoint(dev->of_node, panel_node);
	...

With the change, we could not only follow the graph concept but also keep the backward compatibility.
Javier, do you have other opinion?

Thanks,
Inki Dae

> 
> So even if the driver and DT binding are extended to allow an eDP -> panel
> lookup using ports and endpoints, both approaches have to be kept in the
> driver and DT ABI so I don't think the complexity is worth it just for the
> sake of consistency.
> 
>> Thanks,
>> Inki Dae
>>
> 
> Best regards,
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

Hello Inki,

On 11/23/2015 11:28 PM, Inki Dae wrote:
> Hi Javier,
> 
> 2015년 11월 24일 03:38에 Javier Martinez Canillas 이(가) 쓴 글:
>> Hello Inki,
>>
>> On 11/23/2015 01:47 PM, Inki Dae wrote:
>>> 2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>> Hello,
>>>>
>>>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>>>> Hi Daniel,
>>>>>
>>>>>
>>>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>>>>> Hi Inki,
>>>>>>
>>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>>>>> The boot log says,
>>>>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>>>>
>>>>>>>>
>>>>>>>> This message is a red herring for the reported issue, the message is also
>>>>>>>> present when the machine boots and the display is brought correctly.
>>>>>>>>
>>>>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>>>>
>>>>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>>>>> &dp {
>>>>>>>>>       status = "okay";
>>>>>>>>>       pinctrl-names = "default";
>>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>>       samsung,link-rate = <0x06>;
>>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>>
>>>>>>>>>       ports {
>>>>>>>>>               port@0 {
>>>>>>>>>                       dp_out: endpoint {
>>>>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>>>>                       };
>>>>>>>>>               };
>>>>>>>>>       };
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>>>>> &dp {
>>>>>>>>>       status = "okay";
>>>>>>>>>       pinctrl-names = "default";
>>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>>       panel = <&panel>;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>
>>>>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>>>>
>>>>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>>>>> that points to the panel.
>>>>>>>>
>>>>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>>>>
>>>>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>>>>> exynos5800-peach-pit.dts.
>>>>>>>
>>>>>>> From a common-sense standpoint, there is no any reason to build
>>>>>>> and probe dp driver if the board doesn't use dp hardware.
>>>>>>
>>>>>> I agree with what you say, but unfortunately you've slightly misread
>>>>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>>>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>>>>> which I am writing this) has an eDP panel directly connected. The DT
>>>>
>>>> Thanks a lot Daniel for clarifying my comments to Inki :)
>>>>
>>>>>> describes both the eDP connector from FIMD and the eDP panel, except
>>>>>> that there is no connection between the DT nodes.
>>>>
>>>> There *is* a connection between the FIMD eDP connector and the eDP panel
>>>> nodes but these are connected using a phandle while the connection for
>>>> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
>>>> bindings (Documentation/devicetree/bindings/graph.txt).
>>>>
>>>> And also the connection between the eDP/LVDS bridge and the LVDS panel
>>>> is using an OF graph node, so what I meant is that it would be much more
>>>> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
>>>> connections used the OF graph DT bindings.
>>>>
>>>>>
>>>>> Right. I misread what Javier said. :)
>>>>>
>>>>>>
>>>>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>>>>
>>>>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>>>>
>>>>>>> It's really not good. This would make it more complex. The best
>>>>>>> solution is just to
>>>>>>> remove the dt node from the device tree file.
>>>>>>
>>>>>> Given the above, not really. Javier's patch seems correct to me - as
>>>>>> you can see, there is a panel node, and that is the panel that's
>>>>>> really connected.
>>>>>
>>>>> Indeed. Javier's patch will correct it.
>>>>>
>>>>
>>>> Just to be clear, my patch is not correct since the Exynos DP driver and
>>>> its DT binding does not support to connect an FIMD eDP connector to an
>>>> eDP panel directly using OF graph ports / endpoints (only a phandle). But
>>>> is an example of how the DT will look like if we extend to support that.
>>>
>>> Yes, you added just a port node for the panel device and removed a panel
>>> property from the device tree file so now dp driver cannot get a device node
>>> object of panel node because now dp driver isn't considered for it yet.
>>>
>>> I think there are two ways to correct it. One is,
>>> 1. Add a port node for the panel device to the device tree file.
>>> 2. Add of_graph dt bindings support for getting the panel node to dp driver
>>>    and remove existing of_parse_phandle function call for getting a device
>>>    node object for the panel device.
>>>
>>
>> Exactly.
>>  
>>> Other is,
>>> 1. Revive a panel property and remove the port node you added.
>>>
>>
>> Yes, this is the current code that works. Is just that is not consistent but
>> I don't really mind. I just wanted to explain why the DTS was different for
>> both boards but it seems that I created more confusion than anything else :)
>>  
>>> In addition, it seems that existing bridge of_graph dt bindings codes of now
>>> dp driver should be modified like below,
>>>
>>> endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
>>> if (endpoint) {
>>>         bridge_node = of_graph_get_remote_port_parent(endpoint);
>>>         if (bridge_node) {
>>>                 dp->bridge = of_drm_find_bridge(bridge_node);
>>>                 of_node_put(bridge_node);
>>>                 if (!dp->bridge)
>>>                         return -EPROBE_DEFER;
>>>         } else {
>>>                 DRM_ERROR("has no port node for the bridge deivce");
>>>                 return -ENXIO;
>>>         }
>>> }
>>>
>>> If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
>>> shouldn't be NULL.
>>>
>>> The former looks more reasonable to me.
>>>
>>
>> I'm not too familiar with the OF graph API but I agree that returning a
>> -EPROBE_DEFER when of_graph_get_remote_port_parent() returns NULL seems
>> like the wrong thing to do.
>>
>> Now I don't know if -ENXIO is the right errno code, maybe -EINVAL (since
>> means the DTS is invalid)? or maybe just omit that case as it is ommited
>> if of_graph_get_next_endpoint() fails?
>>
>>>>
>>>> IIRC at the beginning only eDP -> panel was supported and the phandle
>>>> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
>>>> case was needed, then a bridge phandle was added but Ajay was asked to
>>>> use OF graph instead a phandle and we ended with different approaches
>>>> to connect components depending if a bridge is used or not.
>>>
>>> Well, wouldn't it be enough to remove the panel phandle relevant codes
>>> from dp driver and add of_graph dt bindings support for the panel device
>>> to the dp driver instead?
>>>
>>
>> The problem is that removing the panel phandle is not an option without
>> breaking DT backward compatibility since now an eDP -> panel lookup by
>> using a phandle is a DT ABI and old DTBs could be shipped that use it.
> 
> Right. The backward compatibility should be kept.
> For this, I think we could update the dp driver like below,
> 
> 	panel_node = NULL;
> 	
> 	/* This is for the backward compatibility. */
> 	panel_node = of_parse_phandle(dev->of_node, "panel", 0);
> 	if (panel_node) {
> 		...
> 	} else {
> 		endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> 		if (endpoint) {
> 			panel_node = of_graph_get_remote_port_parent(endpoint);
> 			if (panel_node) {
> 				...
> 			} else {
> 				...
> 			}
> 		}
> 	}
> 
> 	endpoint = of_graph_get_next_endpoint(dev->of_node, panel_node);
> 	...
> 
> With the change, we could not only follow the graph concept but also keep the backward compatibility.
> Javier, do you have other opinion?
>

Assuming you can make a distinction if the endpoint is a panel or a bridge,
then yes, I agree with the idea of the patch. Please feel free to cc me if
you post such a patch and I'll gladly test it on my Exynos5800 Peach Pi.
 
> Thanks,
> Inki Dae
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

Hi Javier,

2015-11-24 22:19 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello Inki,
>
> On 11/23/2015 11:28 PM, Inki Dae wrote:
>> Hi Javier,
>>
>> 2015년 11월 24일 03:38에 Javier Martinez Canillas 이(가) 쓴 글:
>>> Hello Inki,
>>>
>>> On 11/23/2015 01:47 PM, Inki Dae wrote:
>>>> 2015-11-23 21:25 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>> Hello,
>>>>>
>>>>> On 11/21/2015 11:59 AM, Inki Dae wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>>
>>>>>> 2015-11-21 22:40 GMT+09:00 Daniel Stone <daniel@fooishbar.org>:
>>>>>>> Hi Inki,
>>>>>>>
>>>>>>> On 21 November 2015 at 09:38, Inki Dae <daeinki@gmail.com> wrote:
>>>>>>>> 2015-11-21 1:44 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
>>>>>>>>> On 11/20/2015 08:13 AM, Inki Dae wrote:
>>>>>>>>>> The boot log says,
>>>>>>>>>> [    5.754493] vdd_ldo9: supplied by vdd_2v
>>>>>>>>>> [    5.765510] of_graph_get_next_endpoint(): no port node found in /dp-controller@145B0000
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This message is a red herring for the reported issue, the message is also
>>>>>>>>> present when the machine boots and the display is brought correctly.
>>>>>>>>>
>>>>>>>>>> Seems this error is because exynos5800-peach-pit.dts file doesn't have 'ports' node in dp node.
>>>>>>>>>>
>>>>>>>>>> Below is dp node description of exynos5420-peach-pit.dts file.
>>>>>>>>>> &dp {
>>>>>>>>>>       status = "okay";
>>>>>>>>>>       pinctrl-names = "default";
>>>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>>>       samsung,link-rate = <0x06>;
>>>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>>>
>>>>>>>>>>       ports {
>>>>>>>>>>               port@0 {
>>>>>>>>>>                       dp_out: endpoint {
>>>>>>>>>>                               remote-endpoint = <&bridge_in>;
>>>>>>>>>>                       };
>>>>>>>>>>               };
>>>>>>>>>>       };
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> And below is for exynos5800-peash-pit.dts,
>>>>>>>>>> &dp {
>>>>>>>>>>       status = "okay";
>>>>>>>>>>       pinctrl-names = "default";
>>>>>>>>>>       pinctrl-0 = <&dp_hpd_gpio>;
>>>>>>>>>>       samsung,color-space = <0>;
>>>>>>>>>>       samsung,dynamic-range = <0>;
>>>>>>>>>>       samsung,ycbcr-coeff = <0>;
>>>>>>>>>>       samsung,color-depth = <1>;
>>>>>>>>>>       samsung,link-rate = <0x0a>;
>>>>>>>>>>       samsung,lane-count = <2>;
>>>>>>>>>>       samsung,hpd-gpio = <&gpx2 6 GPIO_ACTIVE_HIGH>;
>>>>>>>>>>       panel = <&panel>;
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The difference is because the Exynos5420 Peach Pit Display Port is not
>>>>>>>>> attached directly to the display panel, there is an eDP/LVDS bridge chip
>>>>>>>>> in the middle (PS8622) while the Exynos5800 Peach Pi doesn't have that.
>>>>>>>>>
>>>>>>>>> The Exynos DP driver lookups for either a panel phandle or an OF graph
>>>>>>>>> endpoint that points to a bridge chip and the bridge enpoint has a port
>>>>>>>>> that points to the panel.
>>>>>>>>>
>>>>>>>>> So the DT is correct but of_graph_get_next_endpoint() always prints an
>>>>>>>>
>>>>>>>> Then, the DT is really incorrect. As you mentioned, if the Exynos5800 Peach PI
>>>>>>>> board doesn't use eDP, then the dp node __should be removed__ from
>>>>>>>> exynos5800-peach-pit.dts.
>>>>>>>>
>>>>>>>> From a common-sense standpoint, there is no any reason to build
>>>>>>>> and probe dp driver if the board doesn't use dp hardware.
>>>>>>>
>>>>>>> I agree with what you say, but unfortunately you've slightly misread
>>>>>>> what Javier has said. :) exynos5420-peach-pit has an LVDS panel, with
>>>>>>> the eDP -> LVDS bridge in between (ps8622). exynos5800-peach-pi (from
>>>>>>> which I am writing this) has an eDP panel directly connected. The DT
>>>>>
>>>>> Thanks a lot Daniel for clarifying my comments to Inki :)
>>>>>
>>>>>>> describes both the eDP connector from FIMD and the eDP panel, except
>>>>>>> that there is no connection between the DT nodes.
>>>>>
>>>>> There *is* a connection between the FIMD eDP connector and the eDP panel
>>>>> nodes but these are connected using a phandle while the connection for
>>>>> the FIMD eDP connector and the eDP/LVDS bridge is using the OF graph DT
>>>>> bindings (Documentation/devicetree/bindings/graph.txt).
>>>>>
>>>>> And also the connection between the eDP/LVDS bridge and the LVDS panel
>>>>> is using an OF graph node, so what I meant is that it would be much more
>>>>> consistent if both the eDP -> panel and eDP -> eDP/LVDS bridge -> panel
>>>>> connections used the OF graph DT bindings.
>>>>>
>>>>>>
>>>>>> Right. I misread what Javier said. :)
>>>>>>
>>>>>>>
>>>>>>>>> error if the port so OF graph endpoints it seems can't be optional as
>>>>>>>>> used in this driver. Maybe that message should be change to debug then?
>>>>>>>>>
>>>>>>>>> Another option is to extend the DP driver DT binding to be more generic
>>>>>>>>> supporting having a port to a panel besides a bridge, so we could have
>>>>>>>>> something like this for Exynos5800 Peach and be consistent in both cases:
>>>>>>>>
>>>>>>>> It's really not good. This would make it more complex. The best
>>>>>>>> solution is just to
>>>>>>>> remove the dt node from the device tree file.
>>>>>>>
>>>>>>> Given the above, not really. Javier's patch seems correct to me - as
>>>>>>> you can see, there is a panel node, and that is the panel that's
>>>>>>> really connected.
>>>>>>
>>>>>> Indeed. Javier's patch will correct it.
>>>>>>
>>>>>
>>>>> Just to be clear, my patch is not correct since the Exynos DP driver and
>>>>> its DT binding does not support to connect an FIMD eDP connector to an
>>>>> eDP panel directly using OF graph ports / endpoints (only a phandle). But
>>>>> is an example of how the DT will look like if we extend to support that.
>>>>
>>>> Yes, you added just a port node for the panel device and removed a panel
>>>> property from the device tree file so now dp driver cannot get a device node
>>>> object of panel node because now dp driver isn't considered for it yet.
>>>>
>>>> I think there are two ways to correct it. One is,
>>>> 1. Add a port node for the panel device to the device tree file.
>>>> 2. Add of_graph dt bindings support for getting the panel node to dp driver
>>>>    and remove existing of_parse_phandle function call for getting a device
>>>>    node object for the panel device.
>>>>
>>>
>>> Exactly.
>>>
>>>> Other is,
>>>> 1. Revive a panel property and remove the port node you added.
>>>>
>>>
>>> Yes, this is the current code that works. Is just that is not consistent but
>>> I don't really mind. I just wanted to explain why the DTS was different for
>>> both boards but it seems that I created more confusion than anything else :)
>>>
>>>> In addition, it seems that existing bridge of_graph dt bindings codes of now
>>>> dp driver should be modified like below,
>>>>
>>>> endpoint = of_graph_get_next_endpoint(dev->of_node, panel_endpoint_node);
>>>> if (endpoint) {
>>>>         bridge_node = of_graph_get_remote_port_parent(endpoint);
>>>>         if (bridge_node) {
>>>>                 dp->bridge = of_drm_find_bridge(bridge_node);
>>>>                 of_node_put(bridge_node);
>>>>                 if (!dp->bridge)
>>>>                         return -EPROBE_DEFER;
>>>>         } else {
>>>>                 DRM_ERROR("has no port node for the bridge deivce");
>>>>                 return -ENXIO;
>>>>         }
>>>> }
>>>>
>>>> If some board has a bridge device then of_graph_get_remote_port_parent(endpoint)
>>>> shouldn't be NULL.
>>>>
>>>> The former looks more reasonable to me.
>>>>
>>>
>>> I'm not too familiar with the OF graph API but I agree that returning a
>>> -EPROBE_DEFER when of_graph_get_remote_port_parent() returns NULL seems
>>> like the wrong thing to do.
>>>
>>> Now I don't know if -ENXIO is the right errno code, maybe -EINVAL (since
>>> means the DTS is invalid)? or maybe just omit that case as it is ommited
>>> if of_graph_get_next_endpoint() fails?
>>>
>>>>>
>>>>> IIRC at the beginning only eDP -> panel was supported and the phandle
>>>>> was used and later when the eDP -> eDP/LVDS bridge -> LVDS panel use
>>>>> case was needed, then a bridge phandle was added but Ajay was asked to
>>>>> use OF graph instead a phandle and we ended with different approaches
>>>>> to connect components depending if a bridge is used or not.
>>>>
>>>> Well, wouldn't it be enough to remove the panel phandle relevant codes
>>>> from dp driver and add of_graph dt bindings support for the panel device
>>>> to the dp driver instead?
>>>>
>>>
>>> The problem is that removing the panel phandle is not an option without
>>> breaking DT backward compatibility since now an eDP -> panel lookup by
>>> using a phandle is a DT ABI and old DTBs could be shipped that use it.
>>
>> Right. The backward compatibility should be kept.
>> For this, I think we could update the dp driver like below,
>>
>>       panel_node = NULL;
>>
>>       /* This is for the backward compatibility. */
>>       panel_node = of_parse_phandle(dev->of_node, "panel", 0);
>>       if (panel_node) {
>>               ...
>>       } else {
>>               endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>>               if (endpoint) {
>>                       panel_node = of_graph_get_remote_port_parent(endpoint);
>>                       if (panel_node) {
>>                               ...
>>                       } else {
>>                               ...
>>                       }
>>               }
>>       }
>>
>>       endpoint = of_graph_get_next_endpoint(dev->of_node, panel_node);
>>       ...
>>
>> With the change, we could not only follow the graph concept but also keep the backward compatibility.
>> Javier, do you have other opinion?
>>
>
> Assuming you can make a distinction if the endpoint is a panel or a bridge,
> then yes, I agree with the idea of the patch. Please feel free to cc me if
> you post such a patch and I'll gladly test it on my Exynos5800 Peach Pi.

Thanks a lot. I will post the patch set soon CCing you. :)

Thanks,
Inki Dae

>
>> Thanks,
>> Inki Dae
>>
>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Javier Martinez Canillas]

Hello Inki,

On 11/20/2015 07:59 AM, Inki Dae wrote:
> Hi Javier,
> 
> 2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>>
>>>>
>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>>
>>>> [snip]
>>>>
>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>
>>>> I had to revert these patches in order to get the machine in a bootable
>>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>>
>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
>>
>> On a closer look, only reverting the FIMD patch is enough
>> to make at least the Exynos5800 Peach Pi to boot again.
> 
> Thanks for report.
>

Thanks to you for the quick answer and providing a fix.
 
> I assume that the issue is because above patch removed 'suspended' variable
> for checking the suspend status in runtime so I revived it.
>

It seems your assumption was correct...
 
> I'm not sure that the change could resolve the issue. Could you test it
> with the change again? I have no Exynos5800 Peach Pi board. :(
> 
> For this, I pushed it to below exynos-drm/for-next branch,
> 	https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c
> 
> If the issue is resolved by the change then I will modify other patches for
> DECON series. And if really so, there may be a corner case we missed.
>

... since I reverted the offending commit and cherry-picked the one
in your for-next branch and the machine booted again.
 
> Thanks,
> Inki Dae
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v2 0/7] drm/exynos: add pm runtime support

[Inki Dae]

2015-11-21 1:23 GMT+09:00 Javier Martinez Canillas <javier@osg.samsung.com>:
> Hello Inki,
>
> On 11/20/2015 07:59 AM, Inki Dae wrote:
>> Hi Javier,
>>
>> 2015년 11월 20일 00:51에 Javier Martinez Canillas 이(가) 쓴 글:
>>> On 11/19/2015 11:55 AM, Javier Martinez Canillas wrote:
>>>>>>
>>>>>
>>>>> This series causes a boot failure on at least an Exynos5800 Peach Pi
>>>>> Chromebook (tested myself) and seems to be the cause of other Exynos
>>>>> boards failing to boot: http://kernelci.org/boot/?exynos&fail
>>>>>
>>>>> [snip]
>>>>>
>>>>>>   drm/exynos: add pm_runtime to Mixer
>>>>>>   drm/exynos: add pm_runtime to FIMD
>>>>>
>>>>> I had to revert these patches in order to get the machine in a bootable
>>>>> state again, the sha1 hash for these patches in next-20151119 are:
>>>>>
>>>>> 045febd5f813 drm/exynos: add pm_runtime to FIMD
>>>
>>> On a closer look, only reverting the FIMD patch is enough
>>> to make at least the Exynos5800 Peach Pi to boot again.
>>
>> Thanks for report.
>>
>
> Thanks to you for the quick answer and providing a fix.
>
>> I assume that the issue is because above patch removed 'suspended' variable
>> for checking the suspend status in runtime so I revived it.
>>
>
> It seems your assumption was correct...
>
>> I'm not sure that the change could resolve the issue. Could you test it
>> with the change again? I have no Exynos5800 Peach Pi board. :(
>>
>> For this, I pushed it to below exynos-drm/for-next branch,
>>       https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-drm/for-next&id=e84f43e2b2c3388694b0b3a58c2c4447f1fbae7c
>>
>> If the issue is resolved by the change then I will modify other patches for
>> DECON series. And if really so, there may be a corner case we missed.
>>
>
> ... since I reverted the offending commit and cherry-picked the one
> in your for-next branch and the machine booted again.

Great. I'll update and post the patch series again.

Thanks,
Inki Dae

>
>> Thanks,
>> Inki Dae
>>
>
> Best regards,
> --
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel