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 <[email protected]>'s comments.
Changes in v3:
Update commit message.
Address Sean Paul <[email protected]>'s comments.
Update commit message.
Address Sean Paul <[email protected]>'s comments.
Update commit message.
Address Daniel Vetter <[email protected]>'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
The panel is attached when binding analogix dp.
Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
---
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
The dp aux is registered when binding analogix dp.
Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
---
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
The clock is enabled when binding analogix dp.
Signed-off-by: Jeffy Chen <[email protected]>
---
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
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 <[email protected]>
---
Changes in v5: None
Changes in v4:
Address Andrzej Hajda <[email protected]>'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
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 <[email protected]>
---
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
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 <[email protected]>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <[email protected]>'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
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 <[email protected]>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Address Sean Paul <[email protected]>'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
The clocks are prepared when binding vop.
Signed-off-by: Jeffy Chen <[email protected]>
---
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
Signed-off-by: Jeffy Chen <[email protected]>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
Address Daniel Vetter <[email protected]>'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
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 <[email protected]>
---
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
The clock is enabled when binding cdn dp.
Signed-off-by: Jeffy Chen <[email protected]>
---
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
Signed-off-by: Jeffy Chen <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
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 <[email protected]>
>> ---
>>
>> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
On 06.04.2017 14:31, Jeffy Chen wrote:
> The clock is enabled when binding analogix dp.
>
> Signed-off-by: Jeffy Chen <[email protected]>
Reviewed-by: Andrzej Hajda <[email protected]>
--
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);
>
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 <[email protected]>
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 <[email protected]>
--
Regards
Andrzej
> ---
>
> Changes in v5: None
> Changes in v4:
> Address Andrzej Hajda <[email protected]>'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))
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 <[email protected]>'s comments.
>
> Changes in v3:
> Update commit message.
> Address Sean Paul <[email protected]>'s comments.
> Update commit message.
> Address Sean Paul <[email protected]>'s comments.
> Update commit message.
> Address Daniel Vetter <[email protected]>'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
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 <[email protected]>
> > > ---
> > >
> > > 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