[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[Jeffy Chen]

Verified on rk3399 chromebook kevin:
1/ stop ui && pkill -9 frecon
2/ unbind/bind drm

Changes in v5:
Fix wrong git account.

Changes in v4:
Address Andrzej Hajda <a.hajda@samsung.com>'s comments.

Changes in v3:
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.

Changes in v2:
Fix some commit messages.

Jeffy Chen (12):
  drm: bridge: analogix: Detach panel when unbinding analogix dp
  drm: bridge: analogix: Unregister dp aux when unbinding
  drm: bridge: analogix: Disable clock when unbinding
  drm: bridge: analogix: Destroy connector & encoder when unbinding
  drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
  drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
  drm/rockchip: vop: Enable pm domain before vop_initial
  drm/rockchip: vop: Unprepare clocks when unbinding
  drm/rockchip: analogix_dp: Disable clock when unbinding
  drm/rockchip: Reoder drm bind/unbind sequence
  drm/rockchip: Shutdown all crtcs when unbinding drm
  drm/drm_ioctl.c: Break ioctl when drm device not registered

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
 drivers/gpu/drm/drm_ioctl.c                        |  2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
 6 files changed, 67 insertions(+), 37 deletions(-)

-- 
2.1.4

[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[Jeffy Chen]

Verified on rk3399 chromebook kevin:
1/ stop ui && pkill -9 frecon
2/ unbind/bind drm

Changes in v5:
Fix wrong git account.

Changes in v4:
Address Andrzej Hajda <a.hajda@samsung.com>'s comments.

Changes in v3:
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.

Changes in v2:
Fix some commit messages.

Jeffy Chen (12):
  drm: bridge: analogix: Detach panel when unbinding analogix dp
  drm: bridge: analogix: Unregister dp aux when unbinding
  drm: bridge: analogix: Disable clock when unbinding
  drm: bridge: analogix: Destroy connector & encoder when unbinding
  drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
  drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
  drm/rockchip: vop: Enable pm domain before vop_initial
  drm/rockchip: vop: Unprepare clocks when unbinding
  drm/rockchip: analogix_dp: Disable clock when unbinding
  drm/rockchip: Reoder drm bind/unbind sequence
  drm/rockchip: Shutdown all crtcs when unbinding drm
  drm/drm_ioctl.c: Break ioctl when drm device not registered

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
 drivers/gpu/drm/drm_ioctl.c                        |  2 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
 drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
 6 files changed, 67 insertions(+), 37 deletions(-)

-- 
2.1.4

[PATCH v5 01/12] drm: bridge: analogix: Detach panel when unbinding analogix dp

[Jeffy Chen]

The panel is attached when binding analogix dp.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

Changes in v5:
Fix wrong git account.

Changes in v4: None
Changes in v3: None
Changes in v2:
Fix some commit messages.

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index c26997a..28144a1 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1443,6 +1443,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
 	if (dp->plat_data->panel) {
 		if (drm_panel_unprepare(dp->plat_data->panel))
 			DRM_ERROR("failed to turnoff the panel\n");
+		if (drm_panel_detach(dp->plat_data->panel))
+			DRM_ERROR("failed to detach the panel\n");
 	}
 
 	pm_runtime_disable(dev);
-- 
2.1.4

[PATCH v5 02/12] drm: bridge: analogix: Unregister dp aux when unbinding

[Jeffy Chen]

The dp aux is registered when binding analogix dp.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 28144a1..7b75f82 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1447,6 +1447,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
 			DRM_ERROR("failed to detach the panel\n");
 	}
 
+	drm_dp_aux_unregister(&dp->aux);
 	pm_runtime_disable(dev);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
-- 
2.1.4

[PATCH v5 03/12] drm: bridge: analogix: Disable clock when unbinding

[Jeffy Chen]

The clock is enabled when binding analogix dp.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7b75f82..d05ade4 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1449,6 +1449,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
 
 	drm_dp_aux_unregister(&dp->aux);
 	pm_runtime_disable(dev);
+	clk_disable_unprepare(dp->clock);
 }
 EXPORT_SYMBOL_GPL(analogix_dp_unbind);
 
-- 
2.1.4

[PATCH v5 04/12] drm: bridge: analogix: Destroy connector & encoder when unbinding

[Jeffy Chen]

Normally we do this in drm_mode_config_cleanup. But:
1/ analogix dp's connector is allocated in bind, and freed after unbind.
So we need to destroy it in unbind to avoid further access.
2/ the drm bridge is attached in bind, and detached in encoder cleanup.
So we need to destroy encoder in unbind.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4:
Address Andrzej Hajda <a.hajda@samsung.com>'s comments.

Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index d05ade4..4c758ed 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1439,6 +1439,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
 	struct analogix_dp_device *dp = dev_get_drvdata(dev);
 
 	analogix_dp_bridge_disable(dp->bridge);
+	dp->connector.funcs->destroy(&dp->connector);
+	dp->encoder->funcs->destroy(dp->encoder);
 
 	if (dp->plat_data->panel) {
 		if (drm_panel_unprepare(dp->plat_data->panel))
-- 
2.1.4

[PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded

[Jeffy Chen]

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 4e55d63..ee4195d 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1053,6 +1053,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->connected = false;
 	dp->active = false;
 	dp->active_port = -1;
+	dp->fw_loaded = false;
 
 	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
 
@@ -1133,7 +1134,8 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	connector->funcs->destroy(connector);
 
 	pm_runtime_disable(dev);
-	release_firmware(dp->fw);
+	if (dp->fw_loaded)
+		release_firmware(dp->fw);
 	kfree(dp->edid);
 	dp->edid = NULL;
 }
-- 
2.1.4

[PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded

[Jeffy Chen]

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 4e55d63..ee4195d 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1053,6 +1053,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->connected = false;
 	dp->active = false;
 	dp->active_port = -1;
+	dp->fw_loaded = false;
 
 	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
 
@@ -1133,7 +1134,8 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	connector->funcs->destroy(connector);
 
 	pm_runtime_disable(dev);
-	release_firmware(dp->fw);
+	if (dp->fw_loaded)
+		release_firmware(dp->fw);
 	kfree(dp->edid);
 	dp->edid = NULL;
 }
-- 
2.1.4

[PATCH v5 05/12] drm/rockchip: cdn-dp: Don't try to release firmware when not loaded

[Jeffy Chen]

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 4e55d63..ee4195d 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1053,6 +1053,7 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 	dp->connected = false;
 	dp->active = false;
 	dp->active_port = -1;
+	dp->fw_loaded = false;
 
 	INIT_WORK(&dp->event_work, cdn_dp_pd_event_work);
 
@@ -1133,7 +1134,8 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	connector->funcs->destroy(connector);
 
 	pm_runtime_disable(dev);
-	release_firmware(dp->fw);
+	if (dp->fw_loaded)
+		release_firmware(dp->fw);
 	kfree(dp->edid);
 	dp->edid = NULL;
 }
-- 
2.1.4

[PATCH v5 06/12] drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding

[Jeffy Chen]

After snd_soc_unregister_codec, the dai link would remain bound to
the invalid codec. That would cause crashes after unbind dp driver.

Let's unregister audio codec when removing dp driver to prevent that.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index ee4195d..a2169dd 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1092,8 +1092,6 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 		goto err_free_connector;
 	}
 
-	cdn_dp_audio_codec_init(dp, dev);
-
 	for (i = 0; i < dp->ports; i++) {
 		port = dp->port[i];
 
@@ -1128,7 +1126,6 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	struct drm_connector *connector = &dp->connector;
 
 	cancel_work_sync(&dp->event_work);
-	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_encoder_disable(encoder);
 	encoder->funcs->destroy(encoder);
 	connector->funcs->destroy(connector);
@@ -1221,6 +1218,8 @@ static int cdn_dp_probe(struct platform_device *pdev)
 	mutex_init(&dp->lock);
 	dev_set_drvdata(dev, dp);
 
+	cdn_dp_audio_codec_init(dp, dev);
+
 	return component_add(dev, &cdn_dp_component_ops);
 }
 
@@ -1228,6 +1227,7 @@ static int cdn_dp_remove(struct platform_device *pdev)
 {
 	struct cdn_dp_device *dp = platform_get_drvdata(pdev);
 
+	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_suspend(dp->dev);
 	component_del(&pdev->dev, &cdn_dp_component_ops);
 
-- 
2.1.4

[PATCH v5 06/12] drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding

[Jeffy Chen]

After snd_soc_unregister_codec, the dai link would remain bound to
the invalid codec. That would cause crashes after unbind dp driver.

Let's unregister audio codec when removing dp driver to prevent that.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index ee4195d..a2169dd 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -1092,8 +1092,6 @@ static int cdn_dp_bind(struct device *dev, struct device *master, void *data)
 		goto err_free_connector;
 	}
 
-	cdn_dp_audio_codec_init(dp, dev);
-
 	for (i = 0; i < dp->ports; i++) {
 		port = dp->port[i];
 
@@ -1128,7 +1126,6 @@ static void cdn_dp_unbind(struct device *dev, struct device *master, void *data)
 	struct drm_connector *connector = &dp->connector;
 
 	cancel_work_sync(&dp->event_work);
-	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_encoder_disable(encoder);
 	encoder->funcs->destroy(encoder);
 	connector->funcs->destroy(connector);
@@ -1221,6 +1218,8 @@ static int cdn_dp_probe(struct platform_device *pdev)
 	mutex_init(&dp->lock);
 	dev_set_drvdata(dev, dp);
 
+	cdn_dp_audio_codec_init(dp, dev);
+
 	return component_add(dev, &cdn_dp_component_ops);
 }
 
@@ -1228,6 +1227,7 @@ static int cdn_dp_remove(struct platform_device *pdev)
 {
 	struct cdn_dp_device *dp = platform_get_drvdata(pdev);
 
+	platform_device_unregister(dp->audio_pdev);
 	cdn_dp_suspend(dp->dev);
 	component_del(&pdev->dev, &cdn_dp_component_ops);
 
-- 
2.1.4

[PATCH v5 07/12] drm/rockchip: vop: Enable pm domain before vop_initial

[Jeffy Chen]

We're trying to access vop registers here, so need to make sure
the pm domain is on.

Normally it should be enabled by the bootloader, but there's no
guarantee of it. And if we wanna do unbind/bind, it would also
cause the device to hang.

And this patch also does these:
1/ move vop_initial to the end of vop_bind for eaiser error handling.
2/ correct the err_put_pm_runtime of vop_enable.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 2151e1c..b65b296 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -506,7 +506,7 @@ static int vop_enable(struct drm_crtc *crtc)
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
-		goto err_put_pm_runtime;
+		return ret;
 	}
 
 	ret = clk_enable(vop->hclk);
@@ -1405,10 +1405,16 @@ static int vop_initial(struct vop *vop)
 		return PTR_ERR(vop->dclk);
 	}
 
+	ret = pm_runtime_get_sync(vop->dev);
+	if (ret < 0) {
+		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare(vop->dclk);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to prepare dclk\n");
-		return ret;
+		goto err_put_pm_runtime;
 	}
 
 	/* Enable both the hclk and aclk to setup the vop */
@@ -1468,6 +1474,8 @@ static int vop_initial(struct vop *vop)
 
 	vop->is_enabled = false;
 
+	pm_runtime_put_sync(vop->dev);
+
 	return 0;
 
 err_disable_aclk:
@@ -1476,6 +1484,8 @@ static int vop_initial(struct vop *vop)
 	clk_disable_unprepare(vop->hclk);
 err_unprepare_dclk:
 	clk_unprepare(vop->dclk);
+err_put_pm_runtime:
+	pm_runtime_put_sync(vop->dev);
 	return ret;
 }
 
@@ -1576,12 +1586,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	if (!vop->regsbak)
 		return -ENOMEM;
 
-	ret = vop_initial(vop);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
-		return ret;
-	}
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(dev, "cannot find irq for vop\n");
@@ -1608,8 +1612,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_enable(&pdev->dev);
 
+	ret = vop_initial(vop);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
+		goto err_disable_pm_runtime;
+	}
+
 	return 0;
 
+err_disable_pm_runtime:
+	pm_runtime_disable(&pdev->dev);
+	vop_destroy_crtc(vop);
 err_enable_irq:
 	enable_irq(vop->irq); /* To balance out the disable_irq above */
 	return ret;
-- 
2.1.4

[PATCH v5 07/12] drm/rockchip: vop: Enable pm domain before vop_initial

[Jeffy Chen]

We're trying to access vop registers here, so need to make sure
the pm domain is on.

Normally it should be enabled by the bootloader, but there's no
guarantee of it. And if we wanna do unbind/bind, it would also
cause the device to hang.

And this patch also does these:
1/ move vop_initial to the end of vop_bind for eaiser error handling.
2/ correct the err_put_pm_runtime of vop_enable.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 2151e1c..b65b296 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -506,7 +506,7 @@ static int vop_enable(struct drm_crtc *crtc)
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
-		goto err_put_pm_runtime;
+		return ret;
 	}
 
 	ret = clk_enable(vop->hclk);
@@ -1405,10 +1405,16 @@ static int vop_initial(struct vop *vop)
 		return PTR_ERR(vop->dclk);
 	}
 
+	ret = pm_runtime_get_sync(vop->dev);
+	if (ret < 0) {
+		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
+		return ret;
+	}
+
 	ret = clk_prepare(vop->dclk);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to prepare dclk\n");
-		return ret;
+		goto err_put_pm_runtime;
 	}
 
 	/* Enable both the hclk and aclk to setup the vop */
@@ -1468,6 +1474,8 @@ static int vop_initial(struct vop *vop)
 
 	vop->is_enabled = false;
 
+	pm_runtime_put_sync(vop->dev);
+
 	return 0;
 
 err_disable_aclk:
@@ -1476,6 +1484,8 @@ static int vop_initial(struct vop *vop)
 	clk_disable_unprepare(vop->hclk);
 err_unprepare_dclk:
 	clk_unprepare(vop->dclk);
+err_put_pm_runtime:
+	pm_runtime_put_sync(vop->dev);
 	return ret;
 }
 
@@ -1576,12 +1586,6 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 	if (!vop->regsbak)
 		return -ENOMEM;
 
-	ret = vop_initial(vop);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
-		return ret;
-	}
-
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(dev, "cannot find irq for vop\n");
@@ -1608,8 +1612,17 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_enable(&pdev->dev);
 
+	ret = vop_initial(vop);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "cannot initial vop dev - err %d\n", ret);
+		goto err_disable_pm_runtime;
+	}
+
 	return 0;
 
+err_disable_pm_runtime:
+	pm_runtime_disable(&pdev->dev);
+	vop_destroy_crtc(vop);
 err_enable_irq:
 	enable_irq(vop->irq); /* To balance out the disable_irq above */
 	return ret;
-- 
2.1.4

[PATCH v5 08/12] drm/rockchip: vop: Unprepare clocks when unbinding

[Jeffy Chen]

The clocks are prepared when binding vop.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index b65b296..3f7a82d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1634,6 +1634,10 @@ static void vop_unbind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_disable(dev);
 	vop_destroy_crtc(vop);
+
+	clk_unprepare(vop->aclk);
+	clk_unprepare(vop->hclk);
+	clk_unprepare(vop->dclk);
 }
 
 const struct component_ops vop_component_ops = {
-- 
2.1.4

[PATCH v5 08/12] drm/rockchip: vop: Unprepare clocks when unbinding

[Jeffy Chen]

The clocks are prepared when binding vop.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index b65b296..3f7a82d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1634,6 +1634,10 @@ static void vop_unbind(struct device *dev, struct device *master, void *data)
 
 	pm_runtime_disable(dev);
 	vop_destroy_crtc(vop);
+
+	clk_unprepare(vop->aclk);
+	clk_unprepare(vop->hclk);
+	clk_unprepare(vop->dclk);
 }
 
 const struct component_ops vop_component_ops = {
-- 
2.1.4

[PATCH v5 09/12] drm/rockchip: analogix_dp: Disable clock when unbinding

[Jeffy Chen]

The clock is enabled when binding cdn dp.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 91ebe5c..c3b9f1d 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -417,7 +417,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 
-	return analogix_dp_unbind(dev, master, data);
+	analogix_dp_unbind(dev, master, data);
+	clk_disable_unprepare(dp->pclk);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
-- 
2.1.4

[PATCH v5 09/12] drm/rockchip: analogix_dp: Disable clock when unbinding

[Jeffy Chen]

The clock is enabled when binding cdn dp.

Signed-off-by: Jeffy Chen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 91ebe5c..c3b9f1d 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -417,7 +417,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 
-	return analogix_dp_unbind(dev, master, data);
+	analogix_dp_unbind(dev, master, data);
+	clk_disable_unprepare(dp->pclk);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
-- 
2.1.4

[PATCH v5 09/12] drm/rockchip: analogix_dp: Disable clock when unbinding

[Jeffy Chen]

The clock is enabled when binding cdn dp.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 91ebe5c..c3b9f1d 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -417,7 +417,8 @@ static void rockchip_dp_unbind(struct device *dev, struct device *master,
 
 	rockchip_drm_psr_unregister(&dp->encoder);
 
-	return analogix_dp_unbind(dev, master, data);
+	analogix_dp_unbind(dev, master, data);
+	clk_disable_unprepare(dp->pclk);
 }
 
 static const struct component_ops rockchip_dp_component_ops = {
-- 
2.1.4

[PATCH v5 10/12] drm/rockchip: Reoder drm bind/unbind sequence

[Jeffy Chen]

Current drm bind/unbind sequence would cause some memory issues.
For example we should not cleanup iommu before cleanup mode config.

Reorder bind/unbind sequence, follow exynos drm.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 49 +++++++++++++++--------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index cd7d02e1..f24968f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -136,21 +136,24 @@ static int rockchip_drm_bind(struct device *dev)
 	INIT_LIST_HEAD(&private->psr_list);
 	spin_lock_init(&private->psr_list_lock);
 
+	ret = rockchip_drm_init_iommu(drm_dev);
+	if (ret)
+		goto err_free;
+
 	drm_mode_config_init(drm_dev);
 
 	rockchip_drm_mode_config_init(drm_dev);
 
-	ret = rockchip_drm_init_iommu(drm_dev);
-	if (ret)
-		goto err_config_cleanup;
-
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(dev, drm_dev);
 	if (ret)
-		goto err_iommu_cleanup;
+		goto err_mode_config_cleanup;
 
-	/* init kms poll for handling hpd */
-	drm_kms_helper_poll_init(drm_dev);
+	ret = drm_vblank_init(drm_dev, drm_dev->mode_config.num_crtc);
+	if (ret)
+		goto err_unbind_all;
+
+	drm_mode_config_reset(drm_dev);
 
 	/*
 	 * enable drm irq mode.
@@ -158,15 +161,12 @@ static int rockchip_drm_bind(struct device *dev)
 	 */
 	drm_dev->irq_enabled = true;
 
-	ret = drm_vblank_init(drm_dev, ROCKCHIP_MAX_CRTC);
-	if (ret)
-		goto err_kms_helper_poll_fini;
-
-	drm_mode_config_reset(drm_dev);
+	/* init kms poll for handling hpd */
+	drm_kms_helper_poll_init(drm_dev);
 
 	ret = rockchip_drm_fbdev_init(drm_dev);
 	if (ret)
-		goto err_vblank_cleanup;
+		goto err_kms_helper_poll_fini;
 
 	ret = drm_dev_register(drm_dev, 0);
 	if (ret)
@@ -175,17 +175,17 @@ static int rockchip_drm_bind(struct device *dev)
 	return 0;
 err_fbdev_fini:
 	rockchip_drm_fbdev_fini(drm_dev);
-err_vblank_cleanup:
-	drm_vblank_cleanup(drm_dev);
 err_kms_helper_poll_fini:
 	drm_kms_helper_poll_fini(drm_dev);
+	drm_vblank_cleanup(drm_dev);
+err_unbind_all:
 	component_unbind_all(dev, drm_dev);
-err_iommu_cleanup:
-	rockchip_iommu_cleanup(drm_dev);
-err_config_cleanup:
+err_mode_config_cleanup:
 	drm_mode_config_cleanup(drm_dev);
-	drm_dev->dev_private = NULL;
+	rockchip_iommu_cleanup(drm_dev);
 err_free:
+	drm_dev->dev_private = NULL;
+	dev_set_drvdata(dev, NULL);
 	drm_dev_unref(drm_dev);
 	return ret;
 }
@@ -194,16 +194,19 @@ static void rockchip_drm_unbind(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 
+	drm_dev_unregister(drm_dev);
+
 	rockchip_drm_fbdev_fini(drm_dev);
-	drm_vblank_cleanup(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
+
+	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
-	rockchip_iommu_cleanup(drm_dev);
 	drm_mode_config_cleanup(drm_dev);
+	rockchip_iommu_cleanup(drm_dev);
+
 	drm_dev->dev_private = NULL;
-	drm_dev_unregister(drm_dev);
-	drm_dev_unref(drm_dev);
 	dev_set_drvdata(dev, NULL);
+	drm_dev_unref(drm_dev);
 }
 
 static void rockchip_drm_lastclose(struct drm_device *dev)
-- 
2.1.4

[PATCH v5 10/12] drm/rockchip: Reoder drm bind/unbind sequence

[Jeffy Chen]

Current drm bind/unbind sequence would cause some memory issues.
For example we should not cleanup iommu before cleanup mode config.

Reorder bind/unbind sequence, follow exynos drm.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <seanpaul@chromium.org>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 49 +++++++++++++++--------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index cd7d02e1..f24968f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -136,21 +136,24 @@ static int rockchip_drm_bind(struct device *dev)
 	INIT_LIST_HEAD(&private->psr_list);
 	spin_lock_init(&private->psr_list_lock);
 
+	ret = rockchip_drm_init_iommu(drm_dev);
+	if (ret)
+		goto err_free;
+
 	drm_mode_config_init(drm_dev);
 
 	rockchip_drm_mode_config_init(drm_dev);
 
-	ret = rockchip_drm_init_iommu(drm_dev);
-	if (ret)
-		goto err_config_cleanup;
-
 	/* Try to bind all sub drivers. */
 	ret = component_bind_all(dev, drm_dev);
 	if (ret)
-		goto err_iommu_cleanup;
+		goto err_mode_config_cleanup;
 
-	/* init kms poll for handling hpd */
-	drm_kms_helper_poll_init(drm_dev);
+	ret = drm_vblank_init(drm_dev, drm_dev->mode_config.num_crtc);
+	if (ret)
+		goto err_unbind_all;
+
+	drm_mode_config_reset(drm_dev);
 
 	/*
 	 * enable drm irq mode.
@@ -158,15 +161,12 @@ static int rockchip_drm_bind(struct device *dev)
 	 */
 	drm_dev->irq_enabled = true;
 
-	ret = drm_vblank_init(drm_dev, ROCKCHIP_MAX_CRTC);
-	if (ret)
-		goto err_kms_helper_poll_fini;
-
-	drm_mode_config_reset(drm_dev);
+	/* init kms poll for handling hpd */
+	drm_kms_helper_poll_init(drm_dev);
 
 	ret = rockchip_drm_fbdev_init(drm_dev);
 	if (ret)
-		goto err_vblank_cleanup;
+		goto err_kms_helper_poll_fini;
 
 	ret = drm_dev_register(drm_dev, 0);
 	if (ret)
@@ -175,17 +175,17 @@ static int rockchip_drm_bind(struct device *dev)
 	return 0;
 err_fbdev_fini:
 	rockchip_drm_fbdev_fini(drm_dev);
-err_vblank_cleanup:
-	drm_vblank_cleanup(drm_dev);
 err_kms_helper_poll_fini:
 	drm_kms_helper_poll_fini(drm_dev);
+	drm_vblank_cleanup(drm_dev);
+err_unbind_all:
 	component_unbind_all(dev, drm_dev);
-err_iommu_cleanup:
-	rockchip_iommu_cleanup(drm_dev);
-err_config_cleanup:
+err_mode_config_cleanup:
 	drm_mode_config_cleanup(drm_dev);
-	drm_dev->dev_private = NULL;
+	rockchip_iommu_cleanup(drm_dev);
 err_free:
+	drm_dev->dev_private = NULL;
+	dev_set_drvdata(dev, NULL);
 	drm_dev_unref(drm_dev);
 	return ret;
 }
@@ -194,16 +194,19 @@ static void rockchip_drm_unbind(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 
+	drm_dev_unregister(drm_dev);
+
 	rockchip_drm_fbdev_fini(drm_dev);
-	drm_vblank_cleanup(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
+
+	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
-	rockchip_iommu_cleanup(drm_dev);
 	drm_mode_config_cleanup(drm_dev);
+	rockchip_iommu_cleanup(drm_dev);
+
 	drm_dev->dev_private = NULL;
-	drm_dev_unregister(drm_dev);
-	drm_dev_unref(drm_dev);
 	dev_set_drvdata(dev, NULL);
+	drm_dev_unref(drm_dev);
 }
 
 static void rockchip_drm_lastclose(struct drm_device *dev)
-- 
2.1.4

[PATCH v5 11/12] drm/rockchip: Shutdown all crtcs when unbinding drm

[Jeffy Chen]

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f24968f..c6b1b7f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -199,6 +199,7 @@ static void rockchip_drm_unbind(struct device *dev)
 	rockchip_drm_fbdev_fini(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
 
+	drm_atomic_helper_shutdown(drm_dev);
 	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
 	drm_mode_config_cleanup(drm_dev);
-- 
2.1.4

[PATCH v5 11/12] drm/rockchip: Shutdown all crtcs when unbinding drm

[Jeffy Chen]

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
Address Daniel Vetter <daniel@ffwll.ch>'s comments.
Update commit message.

Changes in v2: None

 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f24968f..c6b1b7f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -199,6 +199,7 @@ static void rockchip_drm_unbind(struct device *dev)
 	rockchip_drm_fbdev_fini(drm_dev);
 	drm_kms_helper_poll_fini(drm_dev);
 
+	drm_atomic_helper_shutdown(drm_dev);
 	drm_vblank_cleanup(drm_dev);
 	component_unbind_all(dev, drm_dev);
 	drm_mode_config_cleanup(drm_dev);
-- 
2.1.4

[PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[Jeffy Chen]

After unbinding drm, the user space may still owns the drm dev fd,
and may still be able to call drm ioctl.

Add a sanity check here to prevent that from happening.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/gpu/drm/drm_ioctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 7d6deaa..15beb11 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
 
 	dev = file_priv->minor->dev;
 
-	if (drm_device_is_unplugged(dev))
+	if (drm_device_is_unplugged(dev) || !dev->registered)
 		return -ENODEV;
 
 	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
-- 
2.1.4

Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[Daniel Vetter]

On Thu, Apr 06, 2017 at 08:31:25PM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may still be able to call drm ioctl.
> 
> Add a sanity check here to prevent that from happening.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/drm_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7d6deaa..15beb11 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
>  
>  	dev = file_priv->minor->dev;
>  
> -	if (drm_device_is_unplugged(dev))
> +	if (drm_device_is_unplugged(dev) || !dev->registered)

Shouldn't we instead automatically unplug the device in
drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
|| !registered all over the place?

That should catch a few more issues where userspace might creep into the
driver after unregistering ...
-Daniel

>  		return -ENODEV;
>  
>  	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[Daniel Vetter]

On Thu, Apr 06, 2017 at 08:31:25PM +0800, Jeffy Chen wrote:
> After unbinding drm, the user space may still owns the drm dev fd,
> and may still be able to call drm ioctl.
> 
> Add a sanity check here to prevent that from happening.
> 
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/gpu/drm/drm_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 7d6deaa..15beb11 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
>  
>  	dev = file_priv->minor->dev;
>  
> -	if (drm_device_is_unplugged(dev))
> +	if (drm_device_is_unplugged(dev) || !dev->registered)

Shouldn't we instead automatically unplug the device in
drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
|| !registered all over the place?

That should catch a few more issues where userspace might creep into the
driver after unregistering ...
-Daniel

>  		return -ENODEV;
>  
>  	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[jeffy]

Hi Daniel,

On 04/07/2017 03:16 PM, Daniel Vetter wrote:
> On Thu, Apr 06, 2017 at 08:31:25PM +0800, Jeffy Chen wrote:
>> After unbinding drm, the user space may still owns the drm dev fd,
>> and may still be able to call drm ioctl.
>>
>> Add a sanity check here to prevent that from happening.
>>
>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
>> ---
>>
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>   drivers/gpu/drm/drm_ioctl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 7d6deaa..15beb11 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
>>
>>   	dev = file_priv->minor->dev;
>>
>> -	if (drm_device_is_unplugged(dev))
>> +	if (drm_device_is_unplugged(dev) || !dev->registered)
>
> Shouldn't we instead automatically unplug the device in
> drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
> || !registered all over the place?
>
it looks like the drm_unplug_dev would call drm_dev_unregister...
maybe we can:
1/ replace the dev_unplug_dev in udl_drv.c to drm_dev_unregister
2/ call dev_unplug_dev in drm_dev_unregister, and remove 
drm_dev_unregister in dev_unplug_dev
3/ add a drm_plug_dev or drm_device_set_plugged, and call it in 
drm_dev_register

> That should catch a few more issues where userspace might creep into the
> driver after unregistering ...
> -Daniel
>
>>   		return -ENODEV;
>>
>>   	is_driver_ioctl = nr >= DRM_COMMAND_BASE && nr < DRM_COMMAND_END;
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH v5 03/12] drm: bridge: analogix: Disable clock when unbinding

[Andrzej Hajda]

On 06.04.2017 14:31, Jeffy Chen wrote:
> The clock is enabled when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 7b75f82..d05ade4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1449,6 +1449,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>  
>  	drm_dp_aux_unregister(&dp->aux);
>  	pm_runtime_disable(dev);
> +	clk_disable_unprepare(dp->clock);
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>  

Re: [PATCH v5 03/12] drm: bridge: analogix: Disable clock when unbinding

[Andrzej Hajda]

On 06.04.2017 14:31, Jeffy Chen wrote:
> The clock is enabled when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
--
Regards
Andrzej
> ---
>
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 7b75f82..d05ade4 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1449,6 +1449,7 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>  
>  	drm_dp_aux_unregister(&dp->aux);
>  	pm_runtime_disable(dev);
> +	clk_disable_unprepare(dp->clock);
>  }
>  EXPORT_SYMBOL_GPL(analogix_dp_unbind);
>  


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

Re: [PATCH v5 04/12] drm: bridge: analogix: Destroy connector & encoder when unbinding

[Andrzej Hajda]

On 06.04.2017 14:31, Jeffy Chen wrote:
> Normally we do this in drm_mode_config_cleanup. But:
> 1/ analogix dp's connector is allocated in bind, and freed after unbind.
> So we need to destroy it in unbind to avoid further access.
> 2/ the drm bridge is attached in bind, and detached in encoder cleanup.
> So we need to destroy encoder in unbind.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

In general drm core should free drm resources, doing it in component can
hurt some day. Maybe it would be good to move some stuff from
bind/unbind to probe/remove if necessary, to allow connector and encoder
to live little bit longer, and be destroyed by drm core. This is just
suggestion, I am not familiar enough with the driver to make stronger
statements :)

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> ---
>
> Changes in v5: None
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index d05ade4..4c758ed 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1439,6 +1439,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>  
>  	analogix_dp_bridge_disable(dp->bridge);
> +	dp->connector.funcs->destroy(&dp->connector);
> +	dp->encoder->funcs->destroy(dp->encoder);
>  
>  	if (dp->plat_data->panel) {
>  		if (drm_panel_unprepare(dp->plat_data->panel))

Re: [PATCH v5 04/12] drm: bridge: analogix: Destroy connector & encoder when unbinding

[Andrzej Hajda]

On 06.04.2017 14:31, Jeffy Chen wrote:
> Normally we do this in drm_mode_config_cleanup. But:
> 1/ analogix dp's connector is allocated in bind, and freed after unbind.
> So we need to destroy it in unbind to avoid further access.
> 2/ the drm bridge is attached in bind, and detached in encoder cleanup.
> So we need to destroy encoder in unbind.
>
> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>

In general drm core should free drm resources, doing it in component can
hurt some day. Maybe it would be good to move some stuff from
bind/unbind to probe/remove if necessary, to allow connector and encoder
to live little bit longer, and be destroyed by drm core. This is just
suggestion, I am not familiar enough with the driver to make stronger
statements :)

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> ---
>
> Changes in v5: None
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
>
> Changes in v3: None
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index d05ade4..4c758ed 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1439,6 +1439,8 @@ void analogix_dp_unbind(struct device *dev, struct device *master,
>  	struct analogix_dp_device *dp = dev_get_drvdata(dev);
>  
>  	analogix_dp_bridge_disable(dp->bridge);
> +	dp->connector.funcs->destroy(&dp->connector);
> +	dp->encoder->funcs->destroy(dp->encoder);
>  
>  	if (dp->plat_data->panel) {
>  		if (drm_panel_unprepare(dp->plat_data->panel))


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

Re: [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[Sean Paul]

On Thu, Apr 06, 2017 at 08:31:13PM +0800, Jeffy Chen wrote:
> 
> Verified on rk3399 chromebook kevin:
> 1/ stop ui && pkill -9 frecon
> 2/ unbind/bind drm
> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
> 
> Changes in v3:
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> Update commit message.
> 
> Changes in v2:
> Fix some commit messages.
> 
> Jeffy Chen (12):
>   drm: bridge: analogix: Detach panel when unbinding analogix dp
>   drm: bridge: analogix: Unregister dp aux when unbinding
>   drm: bridge: analogix: Disable clock when unbinding
>   drm: bridge: analogix: Destroy connector & encoder when unbinding
>   drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
>   drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
>   drm/rockchip: vop: Enable pm domain before vop_initial
>   drm/rockchip: vop: Unprepare clocks when unbinding
>   drm/rockchip: analogix_dp: Disable clock when unbinding
>   drm/rockchip: Reoder drm bind/unbind sequence
>   drm/rockchip: Shutdown all crtcs when unbinding drm

Hi Jeffy,
I've applied the first 11 patches from this set to -misc. The last patch
is still a work in progress.

Thanks,

Sean

>   drm/drm_ioctl.c: Break ioctl when drm device not registered
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
>  drivers/gpu/drm/drm_ioctl.c                        |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
>  6 files changed, 67 insertions(+), 37 deletions(-)
> 
> -- 
> 2.1.4
> 

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

Re: [PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[Sean Paul]

On Thu, Apr 06, 2017 at 08:31:13PM +0800, Jeffy Chen wrote:
> 
> Verified on rk3399 chromebook kevin:
> 1/ stop ui && pkill -9 frecon
> 2/ unbind/bind drm
> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
> 
> Changes in v3:
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> Update commit message.
> 
> Changes in v2:
> Fix some commit messages.
> 
> Jeffy Chen (12):
>   drm: bridge: analogix: Detach panel when unbinding analogix dp
>   drm: bridge: analogix: Unregister dp aux when unbinding
>   drm: bridge: analogix: Disable clock when unbinding
>   drm: bridge: analogix: Destroy connector & encoder when unbinding
>   drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
>   drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
>   drm/rockchip: vop: Enable pm domain before vop_initial
>   drm/rockchip: vop: Unprepare clocks when unbinding
>   drm/rockchip: analogix_dp: Disable clock when unbinding
>   drm/rockchip: Reoder drm bind/unbind sequence
>   drm/rockchip: Shutdown all crtcs when unbinding drm

Hi Jeffy,
I've applied the first 11 patches from this set to -misc. The last patch
is still a work in progress.

Thanks,

Sean

>   drm/drm_ioctl.c: Break ioctl when drm device not registered
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
>  drivers/gpu/drm/drm_ioctl.c                        |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
>  6 files changed, 67 insertions(+), 37 deletions(-)
> 
> -- 
> 2.1.4
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v5 00/12] drm: rockchip: Fix rockchip drm unbind crash error

[Sean Paul]

On Thu, Apr 06, 2017 at 08:31:13PM +0800, Jeffy Chen wrote:
> 
> Verified on rk3399 chromebook kevin:
> 1/ stop ui && pkill -9 frecon
> 2/ unbind/bind drm
> 
> Changes in v5:
> Fix wrong git account.
> 
> Changes in v4:
> Address Andrzej Hajda <a.hajda@samsung.com>'s comments.
> 
> Changes in v3:
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Sean Paul <seanpaul@chromium.org>'s comments.
> Update commit message.
> Address Daniel Vetter <daniel@ffwll.ch>'s comments.
> Update commit message.
> 
> Changes in v2:
> Fix some commit messages.
> 
> Jeffy Chen (12):
>   drm: bridge: analogix: Detach panel when unbinding analogix dp
>   drm: bridge: analogix: Unregister dp aux when unbinding
>   drm: bridge: analogix: Disable clock when unbinding
>   drm: bridge: analogix: Destroy connector & encoder when unbinding
>   drm/rockchip: cdn-dp: Don't try to release firmware when not loaded
>   drm/rockchip: cdn-dp: Don't unregister audio dev when unbinding
>   drm/rockchip: vop: Enable pm domain before vop_initial
>   drm/rockchip: vop: Unprepare clocks when unbinding
>   drm/rockchip: analogix_dp: Disable clock when unbinding
>   drm/rockchip: Reoder drm bind/unbind sequence
>   drm/rockchip: Shutdown all crtcs when unbinding drm

Hi Jeffy,
I've applied the first 11 patches from this set to -misc. The last patch
is still a work in progress.

Thanks,

Sean

>   drm/drm_ioctl.c: Break ioctl when drm device not registered
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  6 +++
>  drivers/gpu/drm/drm_ioctl.c                        |  2 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    |  3 +-
>  drivers/gpu/drm/rockchip/cdn-dp-core.c             | 10 +++--
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c        | 50 ++++++++++++----------
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c        | 33 ++++++++++----
>  6 files changed, 67 insertions(+), 37 deletions(-)
> 
> -- 
> 2.1.4
> 

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

Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[Daniel Vetter]

On Fri, Apr 07, 2017 at 05:24:59PM +0800, jeffy wrote:
> Hi Daniel,
> 
> On 04/07/2017 03:16 PM, Daniel Vetter wrote:
> > On Thu, Apr 06, 2017 at 08:31:25PM +0800, Jeffy Chen wrote:
> > > After unbinding drm, the user space may still owns the drm dev fd,
> > > and may still be able to call drm ioctl.
> > > 
> > > Add a sanity check here to prevent that from happening.
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > > Changes in v5: None
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > > 
> > >   drivers/gpu/drm/drm_ioctl.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 7d6deaa..15beb11 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
> > > 
> > >   	dev = file_priv->minor->dev;
> > > 
> > > -	if (drm_device_is_unplugged(dev))
> > > +	if (drm_device_is_unplugged(dev) || !dev->registered)
> > 
> > Shouldn't we instead automatically unplug the device in
> > drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
> > || !registered all over the place?
> > 
> it looks like the drm_unplug_dev would call drm_dev_unregister...
> maybe we can:
> 1/ replace the dev_unplug_dev in udl_drv.c to drm_dev_unregister
> 2/ call dev_unplug_dev in drm_dev_unregister, and remove drm_dev_unregister
> in dev_unplug_dev
> 3/ add a drm_plug_dev or drm_device_set_plugged, and call it in
> drm_dev_register

Yeah, sounds like a reasonable plan. I didn't review the full implications
of this because Fri evening :-) So pls double-check before you rewrite the
world ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 12/12] drm/drm_ioctl.c: Break ioctl when drm device not registered

[Daniel Vetter]

On Fri, Apr 07, 2017 at 05:24:59PM +0800, jeffy wrote:
> Hi Daniel,
> 
> On 04/07/2017 03:16 PM, Daniel Vetter wrote:
> > On Thu, Apr 06, 2017 at 08:31:25PM +0800, Jeffy Chen wrote:
> > > After unbinding drm, the user space may still owns the drm dev fd,
> > > and may still be able to call drm ioctl.
> > > 
> > > Add a sanity check here to prevent that from happening.
> > > 
> > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> > > ---
> > > 
> > > Changes in v5: None
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > > 
> > >   drivers/gpu/drm/drm_ioctl.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > > index 7d6deaa..15beb11 100644
> > > --- a/drivers/gpu/drm/drm_ioctl.c
> > > +++ b/drivers/gpu/drm/drm_ioctl.c
> > > @@ -674,7 +674,7 @@ long drm_ioctl(struct file *filp,
> > > 
> > >   	dev = file_priv->minor->dev;
> > > 
> > > -	if (drm_device_is_unplugged(dev))
> > > +	if (drm_device_is_unplugged(dev) || !dev->registered)
> > 
> > Shouldn't we instead automatically unplug the device in
> > drm_dev_unregister, instead of sprinkling tons of drm_device_is_unplugged
> > || !registered all over the place?
> > 
> it looks like the drm_unplug_dev would call drm_dev_unregister...
> maybe we can:
> 1/ replace the dev_unplug_dev in udl_drv.c to drm_dev_unregister
> 2/ call dev_unplug_dev in drm_dev_unregister, and remove drm_dev_unregister
> in dev_unplug_dev
> 3/ add a drm_plug_dev or drm_device_set_plugged, and call it in
> drm_dev_register

Yeah, sounds like a reasonable plan. I didn't review the full implications
of this because Fri evening :-) So pls double-check before you rewrite the
world ...

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel