Hi,
Actually the name of this patch series is not really true as the edp display is working, but to not confuse I decided to maintain it. Now this patchset is more a group of cleanups, the patchset has been originally posted by Jeffy Chen and the 2 first commits from a previous version (v6) are already merged in mainline. In v8, four patches more landed. And this version are the remaining patches.
Regards, Enric
Changes in v9: - The following patches from v8 has been applied: - [PATCH v8 1/8] drm/bridge: analogix: Do not use device's drvdata - [PATCH v8 2/8] drm/bridge: analogix_dp: Fix connector and encoder cleanup - [PATCH v8 3/8] drm/rockchip: analogix_dp: Add a sanity check for rockchip_drm_psr_register() - The following patches from v8 has been superseeded. - [PATCH v8 7/8] drm/bridge/synopsys: dw-hdmi: Do not use device's drvdata - Superseeded by https://cgit.freedesktop.org/drm/drm-misc/commit/?id=eea034af90c64802fd747a9... - Moved clk_disable_unprepare reordering in unbin to separate patch. - Added new patch to reorder clk_disable_unprepare call in inno_hdmi unbind() - dw_hdmi: Rewrite the commit message to reflect the change.
Jeffy Chen (5): drm/rockchip: dw-mipi-dsi: Fix connector and encoder cleanup. drm/rockchip: inno_hdmi: Fix error handling path. drm/rockchip: inno_hdmi: reorder clk_disable_unprepare call in unbind drm/rockchip: dw_hdmi: Move HDMI vpll clock enable to bind() drm/bridge/synopsys: dw-hdmi: Add missing bridge detach
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++++ drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 8 ++++++-- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 17 +++++++++-------- drivers/gpu/drm/rockchip/inno_hdmi.c | 22 +++++++++++++++++----- 4 files changed, 40 insertions(+), 15 deletions(-)
From: Jeffy Chen jeffy.chen@rock-chips.com
In bind()'s error handling path call destroy functions instead of cleanup functions for encoder and connector and reorder to match how is called in bind().
In unbind() call the connector and encoder destroy functions.
Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Signed-off-by: Thierry Escande thierry.escande@collabora.com Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v9: - Removed the pm_runtime_disable call as is not really needed.
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 591953cbdd18..d53d5a09547f 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -1302,8 +1302,8 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, err_mipi_dsi_host: mipi_dsi_host_unregister(&dsi->dsi_host); err_cleanup: - drm_encoder_cleanup(&dsi->encoder); - drm_connector_cleanup(&dsi->connector); + dsi->connector.funcs->destroy(&dsi->connector); + dsi->encoder.funcs->destroy(&dsi->encoder); err_pllref: clk_disable_unprepare(dsi->pllref_clk); return ret; @@ -1316,6 +1316,10 @@ static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
mipi_dsi_host_unregister(&dsi->dsi_host); pm_runtime_disable(dev); + + dsi->connector.funcs->destroy(&dsi->connector); + dsi->encoder.funcs->destroy(&dsi->encoder); + clk_disable_unprepare(dsi->pllref_clk); }
From: Jeffy Chen jeffy.chen@rock-chips.com
Add missing error handling in bind().
Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support") Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Signed-off-by: Thierry Escande thierry.escande@collabora.com [moved clk_disable_unprepare reordering in unbind to separate patch] Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v9: - Moved clk_disable_unprepare reordering in unbin to separate patch.
drivers/gpu/drm/rockchip/inno_hdmi.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c index f6ad48766d49..a5c661930250 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -849,8 +849,10 @@ static int inno_hdmi_bind(struct device *dev, struct device *master, }
irq = platform_get_irq(pdev, 0); - if (irq < 0) - return irq; + if (irq < 0) { + ret = irq; + goto err_disable_clk; + }
inno_hdmi_reset(hdmi);
@@ -858,7 +860,7 @@ static int inno_hdmi_bind(struct device *dev, struct device *master, if (IS_ERR(hdmi->ddc)) { ret = PTR_ERR(hdmi->ddc); hdmi->ddc = NULL; - return ret; + goto err_disable_clk; }
/* @@ -872,7 +874,7 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
ret = inno_hdmi_register(drm, hdmi); if (ret) - return ret; + goto err_put_adapter;
dev_set_drvdata(dev, hdmi);
@@ -882,7 +884,17 @@ static int inno_hdmi_bind(struct device *dev, struct device *master, ret = devm_request_threaded_irq(dev, irq, inno_hdmi_hardirq, inno_hdmi_irq, IRQF_SHARED, dev_name(dev), hdmi); + if (ret < 0) + goto err_cleanup_hdmi;
+ return 0; +err_cleanup_hdmi: + hdmi->connector.funcs->destroy(&hdmi->connector); + hdmi->encoder.funcs->destroy(&hdmi->encoder); +err_put_adapter: + i2c_put_adapter(hdmi->ddc); +err_disable_clk: + clk_disable_unprepare(hdmi->pclk); return ret; }
From: Jeffy Chen jeffy.chen@rock-chips.com
In bind the clk_prepare_enable of the HDMI pclk is called before adding the i2c_adapter. So it should be the other way around in unbind, first remove the i2c_adapter and then call the clk_disable_unprepare.
Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support") Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Signed-off-by: Thierry Escande thierry.escande@collabora.com Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v9: - Added new patch to reorder clk_disable_unprepare call in inno_hdmi unbind()
drivers/gpu/drm/rockchip/inno_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c index a5c661930250..88d0774c97bd 100644 --- a/drivers/gpu/drm/rockchip/inno_hdmi.c +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c @@ -906,8 +906,8 @@ static void inno_hdmi_unbind(struct device *dev, struct device *master, hdmi->connector.funcs->destroy(&hdmi->connector); hdmi->encoder.funcs->destroy(&hdmi->encoder);
- clk_disable_unprepare(hdmi->pclk); i2c_put_adapter(hdmi->ddc); + clk_disable_unprepare(hdmi->pclk); }
static const struct component_ops inno_hdmi_ops = {
From: Jeffy Chen jeffy.chen@rock-chips.com
The HDMI vpll clock should be enabled when bind() is called. So move the clk_prepare_enable of that clock to bind() function and add the missing clk_disable_unprepare() required in error handling path and unbind().
Fixes: 12b9f204e804 ("drm: bridge/dw_hdmi: add rockchip rk3288 support") Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Signed-off-by: Thierry Escande thierry.escande@collabora.com Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v9: - dw_hdmi: Rewrite the commit message to reflect the change.
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 3574b0ae2ad1..11309a2a4e43 100644 --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c @@ -165,7 +165,6 @@ static const struct dw_hdmi_phy_config rockchip_phy_config[] = { static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi) { struct device_node *np = hdmi->dev->of_node; - int ret;
hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "rockchip,grf"); if (IS_ERR(hdmi->regmap)) { @@ -193,13 +192,6 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi) return PTR_ERR(hdmi->grf_clk); }
- ret = clk_prepare_enable(hdmi->vpll_clk); - if (ret) { - DRM_DEV_ERROR(hdmi->dev, - "Failed to enable HDMI vpll: %d\n", ret); - return ret; - } - return 0; }
@@ -374,6 +366,13 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, return ret; }
+ ret = clk_prepare_enable(hdmi->vpll_clk); + if (ret) { + DRM_DEV_ERROR(hdmi->dev, "Failed to enable HDMI vpll: %d\n", + ret); + return ret; + } + drm_encoder_helper_add(encoder, &dw_hdmi_rockchip_encoder_helper_funcs); drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL); @@ -389,6 +388,7 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master, if (IS_ERR(hdmi->hdmi)) { ret = PTR_ERR(hdmi->hdmi); drm_encoder_cleanup(encoder); + clk_disable_unprepare(hdmi->vpll_clk); }
return ret; @@ -400,6 +400,7 @@ static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master, struct rockchip_hdmi *hdmi = dev_get_drvdata(dev);
dw_hdmi_unbind(hdmi->hdmi); + clk_disable_unprepare(hdmi->vpll_clk); }
static const struct component_ops dw_hdmi_rockchip_ops = {
From: Jeffy Chen jeffy.chen@rock-chips.com
We inited connector in attach(), so need a detach() to cleanup.
Signed-off-by: Jeffy Chen jeffy.chen@rock-chips.com Signed-off-by: Thierry Escande thierry.escande@collabora.com Signed-off-by: Enric Balletbo i Serra enric.balletbo@collabora.com ---
Changes in v9: None
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index f9802399cc0d..5626922f95f9 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -1985,6 +1985,13 @@ static int dw_hdmi_bridge_attach(struct drm_bridge *bridge) return 0; }
+static void dw_hdmi_bridge_detach(struct drm_bridge *bridge) +{ + struct dw_hdmi *hdmi = bridge->driver_private; + + drm_connector_cleanup(&hdmi->connector); +} + static enum drm_mode_status dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge, const struct drm_display_mode *mode) @@ -2041,6 +2048,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .attach = dw_hdmi_bridge_attach, + .detach = dw_hdmi_bridge_detach, .enable = dw_hdmi_bridge_enable, .disable = dw_hdmi_bridge_disable, .mode_set = dw_hdmi_bridge_mode_set,
Hi Enric,
Thank you for the patch.
On Friday, 2 March 2018 19:57:57 EET Enric Balletbo i Serra wrote:
From: Jeffy Chen jeffy.chen@rock-chips.com
We inited connector in attach(), so need a detach() to cleanup.
Do we ? The dw-hdmi driver already sets drm_connector_cleanup() as the connector .destroy() handler, and the .destroy() operation is called by the DRM core. None of the other bridge drivers call drm_connector_cleanup() directly.
Hi Laurent,
On 03/03/2018 05:49 AM, Laurent Pinchart wrote:
hmmm, checking the code, there are also lots of drivers do the cleanup(drm_connector_cleanup or funcs->destroy): drm# grep -r "connector.*funcs->destroy" . ./rockchip/inno_hdmi.c: hdmi->connector.funcs->destroy(&hdmi->connector); ./rockchip/cdn-dp-core.c: connector->funcs->destroy(connector); ./bridge/analogix/analogix_dp_core.c: dp->connector.funcs->destroy(&dp->connector); ./msm/hdmi/hdmi.c: hdmi->connector->funcs->destroy(hdmi->connector); ./msm/dsi/dsi.c: msm_dsi->connector->funcs->destroy(msm_dsi->connector); ./msm/edp/edp.c: edp->connector->funcs->destroy(edp->connector); ./zte/zx_hdmi.c: hdmi->connector.funcs->destroy(&hdmi->connector); ./drm_connector.c: connector->funcs->destroy(connector); ./drm_connector.c: connector->funcs->destroy(connector); ./nouveau/dispnv04/disp.c: connector->funcs->destroy(connector); ./nouveau/nv50_display.c: mstc->connector.funcs->destroy(&mstc->connector); ./nouveau/nv50_display.c: connector->funcs->destroy(connector);
when i debug analogix_dp bind/unbind, i found that we need to cleanup the connector(reported by kmemleak). so i added it to ./bridge/analogix/analogix_dp_core.c...after that i saw dw-hdmi missing that too(by checking the code), so make this patch.
but i didn't really tested it on devices using dw-hdmi, so i'm not very sure the dw-hdmi(maybe also other bridges) is the same with analogix_dp.
i can try to find a chromebook veyron to check it next week :)
but even there's a leak, i'm still not very sure about: should the caller of drm_connector_init cleanup it or the caller of drm_bridge_attach should do it(for example analogix_dp_bind/analogix_dp_unbind) or should the DRM core take care of that?
Hi Laurent,
sorry, you're right, this patch should not be needed.
the connector should be cleanup by drm_mode_config_cleanup->drm_connector_put.
i did that in analogix_dp is to avoid a use-after-free issue not kmemleak, because the connector was allocated/freed in analogix_dp's bind/unbind.
but i found a kmemleak issue(dma_mask not freed) in dw-hdmi when testing that, will send patch soon.
On 03/03/2018 08:20 AM, JeffyChen wrote:
dri-devel@lists.freedesktop.org
-
Enric Balletbo i Serra -
Heiko Stübner -
JeffyChen -
Laurent Pinchart