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(-)
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;
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, }, };
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, }, };
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,
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, }, };
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, }, };
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, }, };
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(-)
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
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
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
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
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.
- 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
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
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.
- 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
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
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.
- 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
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
[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,
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,
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,
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/?...
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,
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/?...
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...
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,
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/?...
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...
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,
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/?...
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...
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
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
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
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,
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
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,
- Add a port node for the panel device to the device tree file.
- 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,
- 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,
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,
- Add a port node for the panel device to the device tree file.
- 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,
- 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,
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,
- Add a port node for the panel device to the device tree file.
- 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,
- 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,
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,
- Add a port node for the panel device to the device tree file.
- 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,
- 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
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/?...
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,
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/?...
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
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Daniel Stone -
Inki Dae -
Inki Dae -
Javier Martinez Canillas