[PATCH v9 0/5] rockchip: kevin: Enable edp display

[PATCH v9 0/5] rockchip: kevin: Enable edp display

[Enric Balletbo i Serra]

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=eea034af90c64802fd747a9dc534c26a7ebe7754
- 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(-)

-- 
2.16.1

[PATCH v9 1/5] drm/rockchip: dw-mipi-dsi: Fix connector and encoder cleanup.

[Enric Balletbo i Serra]

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);
 }
 
-- 
2.16.1

Re: [PATCH v9 1/5] drm/rockchip: dw-mipi-dsi: Fix connector and encoder cleanup.

[Heiko Stübner]

Am Freitag, 2. März 2018, 18:57:53 CET schrieb Enric Balletbo i Serra:
> 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>

applied to drm-misc-next

Thanks
Heiko

[PATCH v9 2/5] drm/rockchip: inno_hdmi: Fix error handling path.

[Enric Balletbo i Serra]

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;
 }
 
-- 
2.16.1

Re: [PATCH v9 2/5] drm/rockchip: inno_hdmi: Fix error handling path.

[Heiko Stübner]

Am Freitag, 2. März 2018, 18:57:54 CET schrieb Enric Balletbo i Serra:
> 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>

applied to drm-misc-next

Thanks
Heiko

[PATCH v9 3/5] drm/rockchip: inno_hdmi: reorder clk_disable_unprepare call in unbind

[Enric Balletbo i Serra]

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 = {
-- 
2.16.1

Re: [PATCH v9 3/5] drm/rockchip: inno_hdmi: reorder clk_disable_unprepare call in unbind

[Heiko Stübner]

Am Freitag, 2. März 2018, 18:57:55 CET schrieb Enric Balletbo i Serra:
> 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>

applied to drm-misc-next

Thanks
Heiko

[PATCH v9 4/5] drm/rockchip: dw_hdmi: Move HDMI vpll clock enable to bind()

[Enric Balletbo i Serra]

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 = {
-- 
2.16.1

Re: [PATCH v9 4/5] drm/rockchip: dw_hdmi: Move HDMI vpll clock enable to bind()

[Heiko Stübner]

Am Freitag, 2. März 2018, 18:57:56 CET schrieb Enric Balletbo i Serra:
> 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>

applied to drm-misc-next

Thanks
Heiko

[PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

[Enric Balletbo i Serra]

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,
-- 
2.16.1

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

[Laurent Pinchart]

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.

> 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,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

[JeffyChen]

Hi Laurent,

On 03/03/2018 05:49 AM, Laurent Pinchart wrote:
> 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.

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?

>
>> 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,
>

Re: [PATCH v9 5/5] drm/bridge/synopsys: dw-hdmi: Add missing bridge detach

[JeffyChen]

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:
> Hi Laurent,
>
> On 03/03/2018 05:49 AM, Laurent Pinchart wrote:
>> 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.
>
> 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?
>
>>
>>> 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>
>>> ---