[PATCH 0/9] drm/exynos: initialization/deinitialization fixes

[PATCH 0/9] drm/exynos: initialization/deinitialization fixes

[Andrzej Hajda]

This set of patches contains fixes of initialization and deinitialization
code of exynos_drm core and components.
It is based on exynos-drm-next branch.

Patchset has been tested on trats and universal_c210 platforms.

Regards
Andrzej


Andrzej Hajda (9):
  drm/exynos/ipp: traverse ipp drivers list safely
  drm/exynos: fix drm driver de-initialization order
  drm/exynos/fbdev: fix fbdev gem object cleanup
  drm/exynos/fb: free exynos framebuffer on error
  drm/exynos/crtc: fix framebuffer reference sequence
  drm/exynos/dsi: unregister connector on removal
  drm/exynos/dpi: unregister connector and panel on removal
  drm/exynos/dp: unregister connector on removal
  drm/exynos/hdmi: unregister connector on removal

 drivers/gpu/drm/exynos/exynos_dp_core.c   | 4 +++-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 6 ++++++
 drivers/gpu/drm/exynos/exynos_drm_dpi.c   | 6 +++++-
 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 8 ++++----
 drivers/gpu/drm/exynos/exynos_drm_dsi.c   | 9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_fb.c    | 1 +
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c   | 4 ++--
 drivers/gpu/drm/exynos/exynos_hdmi.c      | 4 +++-
 9 files changed, 30 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 0/9] drm/exynos: initialization/deinitialization fixes

[Andrzej Hajda]

This set of patches contains fixes of initialization and deinitialization
code of exynos_drm core and components.
It is based on exynos-drm-next branch.

Patchset has been tested on trats and universal_c210 platforms.

Regards
Andrzej


Andrzej Hajda (9):
  drm/exynos/ipp: traverse ipp drivers list safely
  drm/exynos: fix drm driver de-initialization order
  drm/exynos/fbdev: fix fbdev gem object cleanup
  drm/exynos/fb: free exynos framebuffer on error
  drm/exynos/crtc: fix framebuffer reference sequence
  drm/exynos/dsi: unregister connector on removal
  drm/exynos/dpi: unregister connector and panel on removal
  drm/exynos/dp: unregister connector on removal
  drm/exynos/hdmi: unregister connector on removal

 drivers/gpu/drm/exynos/exynos_dp_core.c   | 4 +++-
 drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 6 ++++++
 drivers/gpu/drm/exynos/exynos_drm_dpi.c   | 6 +++++-
 drivers/gpu/drm/exynos/exynos_drm_drv.c   | 8 ++++----
 drivers/gpu/drm/exynos/exynos_drm_dsi.c   | 9 ++++++---
 drivers/gpu/drm/exynos/exynos_drm_fb.c    | 1 +
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c   | 4 ++--
 drivers/gpu/drm/exynos/exynos_hdmi.c      | 4 +++-
 9 files changed, 30 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/9] drm/exynos/ipp: traverse ipp drivers list safely

[Andrzej Hajda]

On ipp subsystem removal list of ipp drivers is traversed
and their members are deleted. To do it properly safe version
of list_for_each* should be used.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 4f36a9d..00d74b1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1605,11 +1605,11 @@ err:
 
 static void ipp_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
 {
-	struct exynos_drm_ippdrv *ippdrv;
+	struct exynos_drm_ippdrv *ippdrv, *t;
 	struct ipp_context *ctx = get_ipp_context(dev);
 
 	/* get ipp driver entry */
-	list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) {
+	list_for_each_entry_safe(ippdrv, t, &exynos_drm_ippdrv_list, drv_list) {
 		if (is_drm_iommu_supported(drm_dev))
 			drm_iommu_detach_device(drm_dev, ippdrv->dev);
 
-- 
1.9.1

[PATCH 1/9] drm/exynos/ipp: traverse ipp drivers list safely

[Andrzej Hajda]

On ipp subsystem removal list of ipp drivers is traversed
and their members are deleted. To do it properly safe version
of list_for_each* should be used.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_ipp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_ipp.c b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
index 4f36a9d..00d74b1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_ipp.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_ipp.c
@@ -1605,11 +1605,11 @@ err:
 
 static void ipp_subdrv_remove(struct drm_device *drm_dev, struct device *dev)
 {
-	struct exynos_drm_ippdrv *ippdrv;
+	struct exynos_drm_ippdrv *ippdrv, *t;
 	struct ipp_context *ctx = get_ipp_context(dev);
 
 	/* get ipp driver entry */
-	list_for_each_entry(ippdrv, &exynos_drm_ippdrv_list, drv_list) {
+	list_for_each_entry_safe(ippdrv, t, &exynos_drm_ippdrv_list, drv_list) {
 		if (is_drm_iommu_supported(drm_dev))
 			drm_iommu_detach_device(drm_dev, ippdrv->dev);
 
-- 
1.9.1

[PATCH 2/9] drm/exynos: fix drm driver de-initialization order

[Andrzej Hajda]

Since components have their own cleanup routines calling
drm_mode_config_cleanup before component_unbind_all causes errors
due to double free of KMS objects. The patch fixes it by changing
de-initialization order. Now it is exactly opposite to init order.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index e11c4d6..b2c710a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -136,14 +136,14 @@ static int exynos_drm_unload(struct drm_device *dev)
 	exynos_drm_device_subdrv_remove(dev);
 
 	exynos_drm_fbdev_fini(dev);
-	drm_vblank_cleanup(dev);
 	drm_kms_helper_poll_fini(dev);
-	drm_mode_config_cleanup(dev);
 
+	component_unbind_all(dev->dev, dev);
+	drm_vblank_cleanup(dev);
+	drm_mode_config_cleanup(dev);
 	drm_release_iommu_mapping(dev);
-	kfree(dev->dev_private);
 
-	component_unbind_all(dev->dev, dev);
+	kfree(dev->dev_private);
 	dev->dev_private = NULL;
 
 	return 0;
-- 
1.9.1

[PATCH 2/9] drm/exynos: fix drm driver de-initialization order

[Andrzej Hajda]

Since components have their own cleanup routines calling
drm_mode_config_cleanup before component_unbind_all causes errors
due to double free of KMS objects. The patch fixes it by changing
de-initialization order. Now it is exactly opposite to init order.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
index e11c4d6..b2c710a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
@@ -136,14 +136,14 @@ static int exynos_drm_unload(struct drm_device *dev)
 	exynos_drm_device_subdrv_remove(dev);
 
 	exynos_drm_fbdev_fini(dev);
-	drm_vblank_cleanup(dev);
 	drm_kms_helper_poll_fini(dev);
-	drm_mode_config_cleanup(dev);
 
+	component_unbind_all(dev->dev, dev);
+	drm_vblank_cleanup(dev);
+	drm_mode_config_cleanup(dev);
 	drm_release_iommu_mapping(dev);
-	kfree(dev->dev_private);
 
-	component_unbind_all(dev->dev, dev);
+	kfree(dev->dev_private);
 	dev->dev_private = NULL;
 
 	return 0;
-- 
1.9.1

[PATCH 3/9] drm/exynos/fbdev: fix fbdev gem object cleanup

[Andrzej Hajda]

exynos_gem_obj is used by exynos_drm_fbdev_destroy so it cannot be destroyed
before calling the latter. exynos_gem_obj will be destroyed anyway by
exynos_drm_fbdev_destroy->...->exynos_drm_fb_destroy.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 32e63f6..586e40d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -353,9 +353,6 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
 
 	fbdev = to_exynos_fbdev(private->fb_helper);
 
-	if (fbdev->exynos_gem_obj)
-		exynos_drm_gem_destroy(fbdev->exynos_gem_obj);
-
 	exynos_drm_fbdev_destroy(dev, private->fb_helper);
 	kfree(fbdev);
 	private->fb_helper = NULL;
-- 
1.9.1

[PATCH 3/9] drm/exynos/fbdev: fix fbdev gem object cleanup

[Andrzej Hajda]

exynos_gem_obj is used by exynos_drm_fbdev_destroy so it cannot be destroyed
before calling the latter. exynos_gem_obj will be destroyed anyway by
exynos_drm_fbdev_destroy->...->exynos_drm_fb_destroy.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index 32e63f6..586e40d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -353,9 +353,6 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
 
 	fbdev = to_exynos_fbdev(private->fb_helper);
 
-	if (fbdev->exynos_gem_obj)
-		exynos_drm_gem_destroy(fbdev->exynos_gem_obj);
-
 	exynos_drm_fbdev_destroy(dev, private->fb_helper);
 	kfree(fbdev);
 	private->fb_helper = NULL;
-- 
1.9.1

[PATCH 4/9] drm/exynos/fb: free exynos framebuffer on error

[Andrzej Hajda]

In case drm_framebuffer_init fails exynos_fb should be freed
before returning an error.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 65a22ca..d346d1e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -165,6 +165,7 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
 
 	ret = drm_framebuffer_init(dev, &exynos_fb->fb, &exynos_drm_fb_funcs);
 	if (ret) {
+		kfree(exynos_fb);
 		DRM_ERROR("failed to initialize framebuffer\n");
 		return ERR_PTR(ret);
 	}
-- 
1.9.1

[PATCH 4/9] drm/exynos/fb: free exynos framebuffer on error

[Andrzej Hajda]

In case drm_framebuffer_init fails exynos_fb should be freed
before returning an error.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 65a22ca..d346d1e 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -165,6 +165,7 @@ exynos_drm_framebuffer_init(struct drm_device *dev,
 
 	ret = drm_framebuffer_init(dev, &exynos_fb->fb, &exynos_drm_fb_funcs);
 	if (ret) {
+		kfree(exynos_fb);
 		DRM_ERROR("failed to initialize framebuffer\n");
 		return ERR_PTR(ret);
 	}
-- 
1.9.1

[PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Andrzej Hajda]

Adding reference to framebuffer should be accompanied with removing
reference to old framebuffer assigned to the plane.
This patch removes following warning:

[   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
[   95.048086] Modules linked in:
[   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
[   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
[   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
[   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
[   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
[   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
[   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
[   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
[   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
[   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
[   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
[   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
[   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
[   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
[   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
[   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
[   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
[   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
[   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
[   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
[   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
[   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b68e58f..bde19f4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	if (ret)
 		return ret;
 
+	/* we need to unreference current fb after replacing it with new one */
+	old_fb = plane->fb;
+
 	plane->crtc = crtc;
 	plane->fb = crtc->primary->fb;
 	drm_framebuffer_reference(plane->fb);
 
+	if (old_fb)
+		drm_framebuffer_unreference(old_fb);
+
 	return 0;
 }
 
-- 
1.9.1

[PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Andrzej Hajda]

Adding reference to framebuffer should be accompanied with removing
reference to old framebuffer assigned to the plane.
This patch removes following warning:

[   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
[   95.048086] Modules linked in:
[   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
[   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
[   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
[   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
[   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
[   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
[   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
[   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
[   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
[   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
[   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
[   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
[   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
[   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
[   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
[   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
[   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
[   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
[   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
[   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
[   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
[   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
index b68e58f..bde19f4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
@@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	if (ret)
 		return ret;
 
+	/* we need to unreference current fb after replacing it with new one */
+	old_fb = plane->fb;
+
 	plane->crtc = crtc;
 	plane->fb = crtc->primary->fb;
 	drm_framebuffer_reference(plane->fb);
 
+	if (old_fb)
+		drm_framebuffer_unreference(old_fb);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

Hi Andrzej,

On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> Adding reference to framebuffer should be accompanied with removing
> reference to old framebuffer assigned to the plane.
> This patch removes following warning:
> 
> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> [   95.048086] Modules linked in:
> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b68e58f..bde19f4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  	if (ret)
>  		return ret;
>  
> +	/* we need to unreference current fb after replacing it with new one */
> +	old_fb = plane->fb;
> +
>  	plane->crtc = crtc;
>  	plane->fb = crtc->primary->fb;
>  	drm_framebuffer_reference(plane->fb);
>  
> +	if (old_fb)
> +		drm_framebuffer_unreference(old_fb);

This time would be a good chance that we can consider drm flip queue to
make sure that whole memory region to old_fb is scanned out completely
before dropping a reference of old_fb. the reference of old_fb should be
dropped at irq handler of each crtc devices, fimd and mixer.

Thanks,
Inki Dae

> +
>  	return 0;
>  }
>  
> 

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

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

Hi Andrzej,

On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> Adding reference to framebuffer should be accompanied with removing
> reference to old framebuffer assigned to the plane.
> This patch removes following warning:
> 
> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> [   95.048086] Modules linked in:
> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index b68e58f..bde19f4 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  	if (ret)
>  		return ret;
>  
> +	/* we need to unreference current fb after replacing it with new one */
> +	old_fb = plane->fb;
> +
>  	plane->crtc = crtc;
>  	plane->fb = crtc->primary->fb;
>  	drm_framebuffer_reference(plane->fb);
>  
> +	if (old_fb)
> +		drm_framebuffer_unreference(old_fb);

This time would be a good chance that we can consider drm flip queue to
make sure that whole memory region to old_fb is scanned out completely
before dropping a reference of old_fb. the reference of old_fb should be
dropped at irq handler of each crtc devices, fimd and mixer.

Thanks,
Inki Dae

> +
>  	return 0;
>  }
>  
> 

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
> Hi Andrzej,
> 
> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> > Adding reference to framebuffer should be accompanied with removing
> > reference to old framebuffer assigned to the plane.
> > This patch removes following warning:
> > 
> > [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> > [   95.048086] Modules linked in:
> > [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> > [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> > [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> > [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> > [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> > [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> > [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> > [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> > [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> > [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> > [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> > [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> > [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> > [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> > [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> > [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> > [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> > [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> > [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> > [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> > [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> > [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> > 
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index b68e58f..bde19f4 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* we need to unreference current fb after replacing it with new one */
> > +	old_fb = plane->fb;
> > +
> >  	plane->crtc = crtc;
> >  	plane->fb = crtc->primary->fb;
> >  	drm_framebuffer_reference(plane->fb);
> >  
> > +	if (old_fb)
> > +		drm_framebuffer_unreference(old_fb);
> 
> This time would be a good chance that we can consider drm flip queue to
> make sure that whole memory region to old_fb is scanned out completely
> before dropping a reference of old_fb. the reference of old_fb should be
> dropped at irq handler of each crtc devices, fimd and mixer.

Generally it's not a good idea to drop fb references from irq context,
since if you actually drop the last reference it'll blow up: fb cleanup
needs a bunch of mutexes.

Also the drm core really should be taking care of this for you, you only
need to grab references yourself for async flips if you want the buffer to
survive a bit. crtc_mode_set has not need for this. I expect that the
refcounting bug is somewhere else, at least from my experience chasing
such issues in i915 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
> Hi Andrzej,
> 
> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> > Adding reference to framebuffer should be accompanied with removing
> > reference to old framebuffer assigned to the plane.
> > This patch removes following warning:
> > 
> > [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> > [   95.048086] Modules linked in:
> > [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> > [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> > [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> > [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> > [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> > [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> > [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> > [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> > [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> > [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> > [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> > [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> > [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> > [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> > [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> > [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> > [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> > [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> > [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> > [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> > [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> > [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> > 
> > Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > index b68e58f..bde19f4 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> > @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* we need to unreference current fb after replacing it with new one */
> > +	old_fb = plane->fb;
> > +
> >  	plane->crtc = crtc;
> >  	plane->fb = crtc->primary->fb;
> >  	drm_framebuffer_reference(plane->fb);
> >  
> > +	if (old_fb)
> > +		drm_framebuffer_unreference(old_fb);
> 
> This time would be a good chance that we can consider drm flip queue to
> make sure that whole memory region to old_fb is scanned out completely
> before dropping a reference of old_fb. the reference of old_fb should be
> dropped at irq handler of each crtc devices, fimd and mixer.

Generally it's not a good idea to drop fb references from irq context,
since if you actually drop the last reference it'll blow up: fb cleanup
needs a bunch of mutexes.

Also the drm core really should be taking care of this for you, you only
need to grab references yourself for async flips if you want the buffer to
survive a bit. crtc_mode_set has not need for this. I expect that the
refcounting bug is somewhere else, at least from my experience chasing
such issues in i915 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

On 2014년 09월 12일 17:57, Daniel Vetter wrote:
> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>> Hi Andrzej,
>>
>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>> Adding reference to framebuffer should be accompanied with removing
>>> reference to old framebuffer assigned to the plane.
>>> This patch removes following warning:
>>>
>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>> [   95.048086] Modules linked in:
>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..bde19f4 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	/* we need to unreference current fb after replacing it with new one */
>>> +	old_fb = plane->fb;
>>> +
>>>  	plane->crtc = crtc;
>>>  	plane->fb = crtc->primary->fb;
>>>  	drm_framebuffer_reference(plane->fb);
>>>  
>>> +	if (old_fb)
>>> +		drm_framebuffer_unreference(old_fb);
>>
>> This time would be a good chance that we can consider drm flip queue to
>> make sure that whole memory region to old_fb is scanned out completely
>> before dropping a reference of old_fb. the reference of old_fb should be
>> dropped at irq handler of each crtc devices, fimd and mixer.
> 
> Generally it's not a good idea to drop fb references from irq context,
> since if you actually drop the last reference it'll blow up: fb cleanup
> needs a bunch of mutexes.

Actually, drm_flip_work_commit function will be called at irq context so
the reference will be dropped by work queue handler so mutex lock would
be required before dropping the reference. My concern was that gem
memory may be freed before the memory region is scanned out completely
so I thought we need to make sure to scan out completely somehow.

> 
> Also the drm core really should be taking care of this for you, you only
> need to grab references yourself for async flips if you want the buffer to
> survive a bit. crtc_mode_set has not need for this. I expect that the
> refcounting bug is somewhere else, at least from my experience chasing
> such issues in i915 ;-)

Thanks for comments. Yes, there may be refcounting bug somewhere else if
drm core makes sure to take care of this. It seems to need more checking.

Thanks,
Inki Dae

> -Daniel
> 

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

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

On 2014년 09월 12일 17:57, Daniel Vetter wrote:
> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>> Hi Andrzej,
>>
>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>> Adding reference to framebuffer should be accompanied with removing
>>> reference to old framebuffer assigned to the plane.
>>> This patch removes following warning:
>>>
>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>> [   95.048086] Modules linked in:
>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..bde19f4 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	/* we need to unreference current fb after replacing it with new one */
>>> +	old_fb = plane->fb;
>>> +
>>>  	plane->crtc = crtc;
>>>  	plane->fb = crtc->primary->fb;
>>>  	drm_framebuffer_reference(plane->fb);
>>>  
>>> +	if (old_fb)
>>> +		drm_framebuffer_unreference(old_fb);
>>
>> This time would be a good chance that we can consider drm flip queue to
>> make sure that whole memory region to old_fb is scanned out completely
>> before dropping a reference of old_fb. the reference of old_fb should be
>> dropped at irq handler of each crtc devices, fimd and mixer.
> 
> Generally it's not a good idea to drop fb references from irq context,
> since if you actually drop the last reference it'll blow up: fb cleanup
> needs a bunch of mutexes.

Actually, drm_flip_work_commit function will be called at irq context so
the reference will be dropped by work queue handler so mutex lock would
be required before dropping the reference. My concern was that gem
memory may be freed before the memory region is scanned out completely
so I thought we need to make sure to scan out completely somehow.

> 
> Also the drm core really should be taking care of this for you, you only
> need to grab references yourself for async flips if you want the buffer to
> survive a bit. crtc_mode_set has not need for this. I expect that the
> refcounting bug is somewhere else, at least from my experience chasing
> such issues in i915 ;-)

Thanks for comments. Yes, there may be refcounting bug somewhere else if
drm core makes sure to take care of this. It seems to need more checking.

Thanks,
Inki Dae

> -Daniel
> 

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Andrzej Hajda]

On 09/12/2014 10:57 AM, Daniel Vetter wrote:
> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>> Hi Andrzej,
>>
>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>> Adding reference to framebuffer should be accompanied with removing
>>> reference to old framebuffer assigned to the plane.
>>> This patch removes following warning:
>>>
>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>> [   95.048086] Modules linked in:
>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..bde19f4 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	/* we need to unreference current fb after replacing it with new one */
>>> +	old_fb = plane->fb;
>>> +
>>>  	plane->crtc = crtc;
>>>  	plane->fb = crtc->primary->fb;
>>>  	drm_framebuffer_reference(plane->fb);
>>>  
>>> +	if (old_fb)
>>> +		drm_framebuffer_unreference(old_fb);
>> This time would be a good chance that we can consider drm flip queue to
>> make sure that whole memory region to old_fb is scanned out completely
>> before dropping a reference of old_fb. the reference of old_fb should be
>> dropped at irq handler of each crtc devices, fimd and mixer.
> Generally it's not a good idea to drop fb references from irq context,
> since if you actually drop the last reference it'll blow up: fb cleanup
> needs a bunch of mutexes.

I agree with that.

>
> Also the drm core really should be taking care of this for you, you only
> need to grab references yourself for async flips if you want the buffer to
> survive a bit. crtc_mode_set has not need for this. I expect that the
> refcounting bug is somewhere else, at least from my experience chasing
> such issues in i915 ;-)

Hmm, maybe I miss something but I do not see the core grabbing fb reference
on plane->fb update. On the other side drm_framebuffer_remove calls
drm_plane_force_disable which drops plane->fb reference.
I am not yet familiar with this code so maybe there is better solution.

If not I guess it would be better to move this code to
exynos_plane_mode_set.
At least it is done this way in omap and msm, in fact it seems better place
for such things. What do you think?

Regards
Andrzej

> -Daniel

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

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Andrzej Hajda]

On 09/12/2014 10:57 AM, Daniel Vetter wrote:
> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>> Hi Andrzej,
>>
>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>> Adding reference to framebuffer should be accompanied with removing
>>> reference to old framebuffer assigned to the plane.
>>> This patch removes following warning:
>>>
>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>> [   95.048086] Modules linked in:
>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> index b68e58f..bde19f4 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	/* we need to unreference current fb after replacing it with new one */
>>> +	old_fb = plane->fb;
>>> +
>>>  	plane->crtc = crtc;
>>>  	plane->fb = crtc->primary->fb;
>>>  	drm_framebuffer_reference(plane->fb);
>>>  
>>> +	if (old_fb)
>>> +		drm_framebuffer_unreference(old_fb);
>> This time would be a good chance that we can consider drm flip queue to
>> make sure that whole memory region to old_fb is scanned out completely
>> before dropping a reference of old_fb. the reference of old_fb should be
>> dropped at irq handler of each crtc devices, fimd and mixer.
> Generally it's not a good idea to drop fb references from irq context,
> since if you actually drop the last reference it'll blow up: fb cleanup
> needs a bunch of mutexes.

I agree with that.

>
> Also the drm core really should be taking care of this for you, you only
> need to grab references yourself for async flips if you want the buffer to
> survive a bit. crtc_mode_set has not need for this. I expect that the
> refcounting bug is somewhere else, at least from my experience chasing
> such issues in i915 ;-)

Hmm, maybe I miss something but I do not see the core grabbing fb reference
on plane->fb update. On the other side drm_framebuffer_remove calls
drm_plane_force_disable which drops plane->fb reference.
I am not yet familiar with this code so maybe there is better solution.

If not I guess it would be better to move this code to
exynos_plane_mode_set.
At least it is done this way in omap and msm, in fact it seems better place
for such things. What do you think?

Regards
Andrzej

> -Daniel

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

On 2014년 09월 12일 18:27, Andrzej Hajda wrote:
> On 09/12/2014 10:57 AM, Daniel Vetter wrote:
>> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>>> Hi Andrzej,
>>>
>>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>>> Adding reference to framebuffer should be accompanied with removing
>>>> reference to old framebuffer assigned to the plane.
>>>> This patch removes following warning:
>>>>
>>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>>> [   95.048086] Modules linked in:
>>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> index b68e58f..bde19f4 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> +	/* we need to unreference current fb after replacing it with new one */
>>>> +	old_fb = plane->fb;
>>>> +
>>>>  	plane->crtc = crtc;
>>>>  	plane->fb = crtc->primary->fb;
>>>>  	drm_framebuffer_reference(plane->fb);
>>>>  
>>>> +	if (old_fb)
>>>> +		drm_framebuffer_unreference(old_fb);
>>> This time would be a good chance that we can consider drm flip queue to
>>> make sure that whole memory region to old_fb is scanned out completely
>>> before dropping a reference of old_fb. the reference of old_fb should be
>>> dropped at irq handler of each crtc devices, fimd and mixer.
>> Generally it's not a good idea to drop fb references from irq context,
>> since if you actually drop the last reference it'll blow up: fb cleanup
>> needs a bunch of mutexes.
> 
> I agree with that.
> 
>>
>> Also the drm core really should be taking care of this for you, you only
>> need to grab references yourself for async flips if you want the buffer to
>> survive a bit. crtc_mode_set has not need for this. I expect that the
>> refcounting bug is somewhere else, at least from my experience chasing
>> such issues in i915 ;-)
> 
> Hmm, maybe I miss something but I do not see the core grabbing fb reference
> on plane->fb update. On the other side drm_framebuffer_remove calls
> drm_plane_force_disable which drops plane->fb reference.
> I am not yet familiar with this code so maybe there is better solution.
> 
> If not I guess it would be better to move this code to
> exynos_plane_mode_set.
> At least it is done this way in omap and msm, in fact it seems better place

I cannot see it in msm. In case of msm, drm flip queue is used in
mdp4/5_crtc_mode_set function. See update_fb function there.

> for such things. What do you think?
> 
> Regards
> Andrzej
> 
>> -Daniel
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Andrzej Hajda]

On 09/12/2014 12:45 PM, Inki Dae wrote:
> On 2014년 09월 12일 18:27, Andrzej Hajda wrote:
>> On 09/12/2014 10:57 AM, Daniel Vetter wrote:
>>> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>>>> Hi Andrzej,
>>>>
>>>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>>>> Adding reference to framebuffer should be accompanied with removing
>>>>> reference to old framebuffer assigned to the plane.
>>>>> This patch removes following warning:
>>>>>
>>>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>>>> [   95.048086] Modules linked in:
>>>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>>>
>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>> ---
>>>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>> index b68e58f..bde19f4 100644
>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>  
>>>>> +	/* we need to unreference current fb after replacing it with new one */
>>>>> +	old_fb = plane->fb;
>>>>> +
>>>>>  	plane->crtc = crtc;
>>>>>  	plane->fb = crtc->primary->fb;
>>>>>  	drm_framebuffer_reference(plane->fb);
>>>>>  
>>>>> +	if (old_fb)
>>>>> +		drm_framebuffer_unreference(old_fb);
>>>> This time would be a good chance that we can consider drm flip queue to
>>>> make sure that whole memory region to old_fb is scanned out completely
>>>> before dropping a reference of old_fb. the reference of old_fb should be
>>>> dropped at irq handler of each crtc devices, fimd and mixer.
>>> Generally it's not a good idea to drop fb references from irq context,
>>> since if you actually drop the last reference it'll blow up: fb cleanup
>>> needs a bunch of mutexes.
>> I agree with that.
>>
>>> Also the drm core really should be taking care of this for you, you only
>>> need to grab references yourself for async flips if you want the buffer to
>>> survive a bit. crtc_mode_set has not need for this. I expect that the
>>> refcounting bug is somewhere else, at least from my experience chasing
>>> such issues in i915 ;-)
>> Hmm, maybe I miss something but I do not see the core grabbing fb reference
>> on plane->fb update. On the other side drm_framebuffer_remove calls
>> drm_plane_force_disable which drops plane->fb reference.
>> I am not yet familiar with this code so maybe there is better solution.
>>
>> If not I guess it would be better to move this code to
>> exynos_plane_mode_set.
>> At least it is done this way in omap and msm, in fact it seems better place
> I cannot see it in msm. In case of msm, drm flip queue is used in
> mdp4/5_crtc_mode_set function. See update_fb function there.

fb reference dance for planes is in mdp*_plane_update.

Regarding drm flip in msm old fb unreferencing is done via
drm_flip_work_queue.

Regards
Andrzej

>
>> for such things. What do you think?
>>
>> Regards
>> Andrzej
>>
>>> -Daniel
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Inki Dae]

On 2014년 09월 12일 20:04, Andrzej Hajda wrote:
> On 09/12/2014 12:45 PM, Inki Dae wrote:
>> On 2014년 09월 12일 18:27, Andrzej Hajda wrote:
>>> On 09/12/2014 10:57 AM, Daniel Vetter wrote:
>>>> On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
>>>>> Hi Andrzej,
>>>>>
>>>>> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
>>>>>> Adding reference to framebuffer should be accompanied with removing
>>>>>> reference to old framebuffer assigned to the plane.
>>>>>> This patch removes following warning:
>>>>>>
>>>>>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
>>>>>> [   95.048086] Modules linked in:
>>>>>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
>>>>>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
>>>>>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
>>>>>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
>>>>>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
>>>>>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
>>>>>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
>>>>>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
>>>>>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
>>>>>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
>>>>>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
>>>>>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
>>>>>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
>>>>>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
>>>>>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
>>>>>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
>>>>>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
>>>>>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
>>>>>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
>>>>>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
>>>>>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
>>>>>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
>>>>>>
>>>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>>> index b68e58f..bde19f4 100644
>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>>>>>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
>>>>>>  	if (ret)
>>>>>>  		return ret;
>>>>>>  
>>>>>> +	/* we need to unreference current fb after replacing it with new one */
>>>>>> +	old_fb = plane->fb;
>>>>>> +
>>>>>>  	plane->crtc = crtc;
>>>>>>  	plane->fb = crtc->primary->fb;
>>>>>>  	drm_framebuffer_reference(plane->fb);
>>>>>>  
>>>>>> +	if (old_fb)
>>>>>> +		drm_framebuffer_unreference(old_fb);
>>>>> This time would be a good chance that we can consider drm flip queue to
>>>>> make sure that whole memory region to old_fb is scanned out completely
>>>>> before dropping a reference of old_fb. the reference of old_fb should be
>>>>> dropped at irq handler of each crtc devices, fimd and mixer.
>>>> Generally it's not a good idea to drop fb references from irq context,
>>>> since if you actually drop the last reference it'll blow up: fb cleanup
>>>> needs a bunch of mutexes.
>>> I agree with that.
>>>
>>>> Also the drm core really should be taking care of this for you, you only
>>>> need to grab references yourself for async flips if you want the buffer to
>>>> survive a bit. crtc_mode_set has not need for this. I expect that the
>>>> refcounting bug is somewhere else, at least from my experience chasing
>>>> such issues in i915 ;-)
>>> Hmm, maybe I miss something but I do not see the core grabbing fb reference
>>> on plane->fb update. On the other side drm_framebuffer_remove calls
>>> drm_plane_force_disable which drops plane->fb reference.
>>> I am not yet familiar with this code so maybe there is better solution.
>>>
>>> If not I guess it would be better to move this code to
>>> exynos_plane_mode_set.
>>> At least it is done this way in omap and msm, in fact it seems better place
>> I cannot see it in msm. In case of msm, drm flip queue is used in
>> mdp4/5_crtc_mode_set function. See update_fb function there.
> 
> fb reference dance for planes is in mdp*_plane_update.
> 
> Regarding drm flip in msm old fb unreferencing is done via
> drm_flip_work_queue.

Yes, that is what drm flip queue does and  I mentioned earlier below,
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg724161.html

I think we have a talk about how we have to drop a reference to old_fb.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
>>
>>> for such things. What do you think?
>>>
>>> Regards
>>> Andrzej
>>>
>>>> -Daniel
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 11:27:41AM +0200, Andrzej Hajda wrote:
> On 09/12/2014 10:57 AM, Daniel Vetter wrote:
> > On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
> >> Hi Andrzej,
> >>
> >> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> >>> Adding reference to framebuffer should be accompanied with removing
> >>> reference to old framebuffer assigned to the plane.
> >>> This patch removes following warning:
> >>>
> >>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> >>> [   95.048086] Modules linked in:
> >>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> >>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> >>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> >>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> >>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> >>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> >>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> >>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> >>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> >>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> >>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> >>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> >>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> >>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> >>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> >>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> >>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> >>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> >>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> >>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> >>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> >>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> >>>
> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> index b68e58f..bde19f4 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> >>>  	if (ret)
> >>>  		return ret;
> >>>  
> >>> +	/* we need to unreference current fb after replacing it with new one */
> >>> +	old_fb = plane->fb;
> >>> +
> >>>  	plane->crtc = crtc;
> >>>  	plane->fb = crtc->primary->fb;
> >>>  	drm_framebuffer_reference(plane->fb);
> >>>  
> >>> +	if (old_fb)
> >>> +		drm_framebuffer_unreference(old_fb);
> >> This time would be a good chance that we can consider drm flip queue to
> >> make sure that whole memory region to old_fb is scanned out completely
> >> before dropping a reference of old_fb. the reference of old_fb should be
> >> dropped at irq handler of each crtc devices, fimd and mixer.
> > Generally it's not a good idea to drop fb references from irq context,
> > since if you actually drop the last reference it'll blow up: fb cleanup
> > needs a bunch of mutexes.
> 
> I agree with that.
> 
> >
> > Also the drm core really should be taking care of this for you, you only
> > need to grab references yourself for async flips if you want the buffer to
> > survive a bit. crtc_mode_set has not need for this. I expect that the
> > refcounting bug is somewhere else, at least from my experience chasing
> > such issues in i915 ;-)
> 
> Hmm, maybe I miss something but I do not see the core grabbing fb reference
> on plane->fb update. On the other side drm_framebuffer_remove calls
> drm_plane_force_disable which drops plane->fb reference.
> I am not yet familiar with this code so maybe there is better solution.

It does, see e.g. drm_mode_set_config_internal. All the other places also
do it.

> If not I guess it would be better to move this code to
> exynos_plane_mode_set.
> At least it is done this way in omap and msm, in fact it seems better place
> for such things. What do you think?

Drivers _only_ need to do their own refcounting if they do asynchronous
updates (pageflips or asynchronous modesets). Which omap does (and iirc
msm for pageflips). But if that need is common we should have some helpers
for this (like the drm flip queue as guidelines or in the future atomic
helpers which will take care of everything).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm/exynos/crtc: fix framebuffer reference sequence

[Daniel Vetter]

On Fri, Sep 12, 2014 at 11:27:41AM +0200, Andrzej Hajda wrote:
> On 09/12/2014 10:57 AM, Daniel Vetter wrote:
> > On Fri, Sep 12, 2014 at 05:34:50PM +0900, Inki Dae wrote:
> >> Hi Andrzej,
> >>
> >> On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> >>> Adding reference to framebuffer should be accompanied with removing
> >>> reference to old framebuffer assigned to the plane.
> >>> This patch removes following warning:
> >>>
> >>> [   95.038017] WARNING: CPU: 1 PID: 3067 at drivers/gpu/drm/drm_crtc.c:5115 drm_mode_config_cleanup+0x258/0x268()
> >>> [   95.048086] Modules linked in:
> >>> [   95.051430] CPU: 1 PID: 3067 Comm: bash Tainted: G        W      3.16.0-11355-g7a6eca5-dirty #3015
> >>> [   95.060058] [<c0013e90>] (unwind_backtrace) from [<c0011128>] (show_stack+0x10/0x14)
> >>> [   95.067766] [<c0011128>] (show_stack) from [<c04a5dc4>] (dump_stack+0x70/0xbc)
> >>> [   95.074953] [<c04a5dc4>] (dump_stack) from [<c0021784>] (warn_slowpath_common+0x64/0x88)
> >>> [   95.083005] [<c0021784>] (warn_slowpath_common) from [<c00217c4>] (warn_slowpath_null+0x1c/0x24)
> >>> [   95.091780] [<c00217c4>] (warn_slowpath_null) from [<c0275fa0>] (drm_mode_config_cleanup+0x258/0x268)
> >>> [   95.100983] [<c0275fa0>] (drm_mode_config_cleanup) from [<c0281724>] (exynos_drm_unload+0x38/0x50)
> >>> [   95.109915] [<c0281724>] (exynos_drm_unload) from [<c026e248>] (drm_dev_unregister+0x24/0x98)
> >>> [   95.118414] [<c026e248>] (drm_dev_unregister) from [<c026ecb0>] (drm_put_dev+0x28/0x64)
> >>> [   95.126412] [<c026ecb0>] (drm_put_dev) from [<c02947c4>] (take_down_master+0x24/0x44)
> >>> [   95.134218] [<c02947c4>] (take_down_master) from [<c0294870>] (component_del+0x8c/0xc8)
> >>> [   95.142201] [<c0294870>] (component_del) from [<c0286c10>] (exynos_dsi_remove+0x18/0x2c)
> >>> [   95.150294] [<c0286c10>] (exynos_dsi_remove) from [<c0299e04>] (platform_drv_remove+0x18/0x1c)
> >>> [   95.158872] [<c0299e04>] (platform_drv_remove) from [<c02987a4>] (__device_release_driver+0x70/0xc4)
> >>> [   95.167981] [<c02987a4>] (__device_release_driver) from [<c0298818>] (device_release_driver+0x20/0x2c)
> >>> [   95.177268] [<c0298818>] (device_release_driver) from [<c02979d0>] (unbind_store+0x5c/0x94)
> >>> [   95.185597] [<c02979d0>] (unbind_store) from [<c0297278>] (drv_attr_store+0x20/0x2c)
> >>> [   95.193323] [<c0297278>] (drv_attr_store) from [<c012aa90>] (sysfs_kf_write+0x4c/0x50)
> >>> [   95.201224] [<c012aa90>] (sysfs_kf_write) from [<c0129e94>] (kernfs_fop_write+0xc4/0x184)
> >>> [   95.209393] [<c0129e94>] (kernfs_fop_write) from [<c00d03ec>] (vfs_write+0xa0/0x1a8)
> >>> [   95.217111] [<c00d03ec>] (vfs_write) from [<c00d07f8>] (SyS_write+0x40/0x8c)
> >>> [   95.224146] [<c00d07f8>] (SyS_write) from [<c000e660>] (ret_fast_syscall+0x0/0x48)
> >>>
> >>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos_drm_crtc.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> index b68e58f..bde19f4 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> >>> @@ -145,10 +145,16 @@ exynos_drm_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode,
> >>>  	if (ret)
> >>>  		return ret;
> >>>  
> >>> +	/* we need to unreference current fb after replacing it with new one */
> >>> +	old_fb = plane->fb;
> >>> +
> >>>  	plane->crtc = crtc;
> >>>  	plane->fb = crtc->primary->fb;
> >>>  	drm_framebuffer_reference(plane->fb);
> >>>  
> >>> +	if (old_fb)
> >>> +		drm_framebuffer_unreference(old_fb);
> >> This time would be a good chance that we can consider drm flip queue to
> >> make sure that whole memory region to old_fb is scanned out completely
> >> before dropping a reference of old_fb. the reference of old_fb should be
> >> dropped at irq handler of each crtc devices, fimd and mixer.
> > Generally it's not a good idea to drop fb references from irq context,
> > since if you actually drop the last reference it'll blow up: fb cleanup
> > needs a bunch of mutexes.
> 
> I agree with that.
> 
> >
> > Also the drm core really should be taking care of this for you, you only
> > need to grab references yourself for async flips if you want the buffer to
> > survive a bit. crtc_mode_set has not need for this. I expect that the
> > refcounting bug is somewhere else, at least from my experience chasing
> > such issues in i915 ;-)
> 
> Hmm, maybe I miss something but I do not see the core grabbing fb reference
> on plane->fb update. On the other side drm_framebuffer_remove calls
> drm_plane_force_disable which drops plane->fb reference.
> I am not yet familiar with this code so maybe there is better solution.

It does, see e.g. drm_mode_set_config_internal. All the other places also
do it.

> If not I guess it would be better to move this code to
> exynos_plane_mode_set.
> At least it is done this way in omap and msm, in fact it seems better place
> for such things. What do you think?

Drivers _only_ need to do their own refcounting if they do asynchronous
updates (pageflips or asynchronous modesets). Which omap does (and iirc
msm for pageflips). But if that need is common we should have some helpers
for this (like the drm flip queue as guidelines or in the future atomic
helpers which will take care of everything).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 6/9] drm/exynos/dsi: unregister connector on removal

[Andrzej Hajda]

During component unbind connector should be unregistered.
Also DSI host should be unregistered after KMS cleanup.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 0065a5a..24741d8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1441,6 +1441,9 @@ exynos_dsi_detect(struct drm_connector *connector, bool force)
 
 static void exynos_dsi_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+	connector->dev = NULL;
 }
 
 static struct drm_connector_funcs exynos_dsi_connector_funcs = {
@@ -1661,10 +1664,10 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 
 	exynos_dsi_dpms(&exynos_dsi_display, DRM_MODE_DPMS_OFF);
 
-	mipi_dsi_host_unregister(&dsi->dsi_host);
-
+	exynos_dsi_connector_destroy(&dsi->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&dsi->connector);
+
+	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
 
 static const struct component_ops exynos_dsi_component_ops = {
-- 
1.9.1

[PATCH 6/9] drm/exynos/dsi: unregister connector on removal

[Andrzej Hajda]

During component unbind connector should be unregistered.
Also DSI host should be unregistered after KMS cleanup.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 0065a5a..24741d8 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -1441,6 +1441,9 @@ exynos_dsi_detect(struct drm_connector *connector, bool force)
 
 static void exynos_dsi_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+	connector->dev = NULL;
 }
 
 static struct drm_connector_funcs exynos_dsi_connector_funcs = {
@@ -1661,10 +1664,10 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 
 	exynos_dsi_dpms(&exynos_dsi_display, DRM_MODE_DPMS_OFF);
 
-	mipi_dsi_host_unregister(&dsi->dsi_host);
-
+	exynos_dsi_connector_destroy(&dsi->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&dsi->connector);
+
+	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
 
 static const struct component_ops exynos_dsi_component_ops = {
-- 
1.9.1

[PATCH 7/9] drm/exynos/dpi: unregister connector and panel on removal

[Andrzej Hajda]

During component removal it should unregister connector
and optionally detach the panel.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index fa08f05..96c87db 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -342,8 +342,12 @@ int exynos_dpi_remove(struct device *dev)
 	struct exynos_dpi *ctx = exynos_dpi_display.ctx;
 
 	exynos_dpi_dpms(&exynos_dpi_display, DRM_MODE_DPMS_OFF);
+
+	exynos_dpi_connector_destroy(&ctx->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&ctx->connector);
+
+	if (ctx->panel)
+		drm_panel_detach(ctx->panel);
 
 	exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CONNECTOR);
 
-- 
1.9.1

[PATCH 7/9] drm/exynos/dpi: unregister connector and panel on removal

[Andrzej Hajda]

During component removal it should unregister connector
and optionally detach the panel.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index fa08f05..96c87db 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -342,8 +342,12 @@ int exynos_dpi_remove(struct device *dev)
 	struct exynos_dpi *ctx = exynos_dpi_display.ctx;
 
 	exynos_dpi_dpms(&exynos_dpi_display, DRM_MODE_DPMS_OFF);
+
+	exynos_dpi_connector_destroy(&ctx->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&ctx->connector);
+
+	if (ctx->panel)
+		drm_panel_detach(ctx->panel);
 
 	exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CONNECTOR);
 
-- 
1.9.1

[PATCH 8/9] drm/exynos/dp: unregister connector on removal

[Andrzej Hajda]

During component removal driver should unregister connector.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 4f3c7eb..32716c0 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -937,6 +937,8 @@ static enum drm_connector_status exynos_dp_detect(
 
 static void exynos_dp_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
 }
 
 static struct drm_connector_funcs exynos_dp_connector_funcs = {
@@ -1358,8 +1360,8 @@ static void exynos_dp_unbind(struct device *dev, struct device *master,
 
 	exynos_dp_dpms(display, DRM_MODE_DPMS_OFF);
 
+	exynos_dp_connector_destroy(&dp->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&dp->connector);
 }
 
 static const struct component_ops exynos_dp_ops = {
-- 
1.9.1

[PATCH 8/9] drm/exynos/dp: unregister connector on removal

[Andrzej Hajda]

During component removal driver should unregister connector.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_dp_core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
index 4f3c7eb..32716c0 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -937,6 +937,8 @@ static enum drm_connector_status exynos_dp_detect(
 
 static void exynos_dp_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
 }
 
 static struct drm_connector_funcs exynos_dp_connector_funcs = {
@@ -1358,8 +1360,8 @@ static void exynos_dp_unbind(struct device *dev, struct device *master,
 
 	exynos_dp_dpms(display, DRM_MODE_DPMS_OFF);
 
+	exynos_dp_connector_destroy(&dp->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&dp->connector);
 }
 
 static const struct component_ops exynos_dp_ops = {
-- 
1.9.1

[PATCH 9/9] drm/exynos/hdmi: unregister connector on removal

[Andrzej Hajda]

During component removal driver should unregister connector.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 562966d..7910fb3 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1040,6 +1040,8 @@ static enum drm_connector_status hdmi_detect(struct drm_connector *connector,
 
 static void hdmi_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
 }
 
 static struct drm_connector_funcs hdmi_connector_funcs = {
@@ -2314,8 +2316,8 @@ static void hdmi_unbind(struct device *dev, struct device *master, void *data)
 	struct drm_encoder *encoder = display->encoder;
 	struct hdmi_context *hdata = display->ctx;
 
+	hdmi_connector_destroy(&hdata->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&hdata->connector);
 }
 
 static const struct component_ops hdmi_component_ops = {
-- 
1.9.1

[PATCH 9/9] drm/exynos/hdmi: unregister connector on removal

[Andrzej Hajda]

During component removal driver should unregister connector.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 562966d..7910fb3 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -1040,6 +1040,8 @@ static enum drm_connector_status hdmi_detect(struct drm_connector *connector,
 
 static void hdmi_connector_destroy(struct drm_connector *connector)
 {
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
 }
 
 static struct drm_connector_funcs hdmi_connector_funcs = {
@@ -2314,8 +2316,8 @@ static void hdmi_unbind(struct device *dev, struct device *master, void *data)
 	struct drm_encoder *encoder = display->encoder;
 	struct hdmi_context *hdata = display->ctx;
 
+	hdmi_connector_destroy(&hdata->connector);
 	encoder->funcs->destroy(encoder);
-	drm_connector_cleanup(&hdata->connector);
 }
 
 static const struct component_ops hdmi_component_ops = {
-- 
1.9.1

Re: [PATCH 0/9] drm/exynos: initialization/deinitialization fixes

[Inki Dae]

On 2014년 09월 09일 22:16, Andrzej Hajda wrote:
> This set of patches contains fixes of initialization and deinitialization
> code of exynos_drm core and components.
> It is based on exynos-drm-next branch.
> 
> Patchset has been tested on trats and universal_c210 platforms.

Applied all patches except patch #5. As of now, it seems good to merge
also patch #5 if you couldn't post next version of that patch until the
end of this week. In this case, I will have a pull request including
that patch so that we can fix it up correctly later. Give me your
opinion if there is other plan.

Thanks,
Inki Dae

> 
> Regards
> Andrzej
> 
> 
> Andrzej Hajda (9):
>   drm/exynos/ipp: traverse ipp drivers list safely
>   drm/exynos: fix drm driver de-initialization order
>   drm/exynos/fbdev: fix fbdev gem object cleanup
>   drm/exynos/fb: free exynos framebuffer on error
>   drm/exynos/crtc: fix framebuffer reference sequence
>   drm/exynos/dsi: unregister connector on removal
>   drm/exynos/dpi: unregister connector and panel on removal
>   drm/exynos/dp: unregister connector on removal
>   drm/exynos/hdmi: unregister connector on removal
> 
>  drivers/gpu/drm/exynos/exynos_dp_core.c   | 4 +++-
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c  | 6 ++++++
>  drivers/gpu/drm/exynos/exynos_drm_dpi.c   | 6 +++++-
>  drivers/gpu/drm/exynos/exynos_drm_drv.c   | 8 ++++----
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c   | 9 ++++++---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c    | 1 +
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ---
>  drivers/gpu/drm/exynos/exynos_drm_ipp.c   | 4 ++--
>  drivers/gpu/drm/exynos/exynos_hdmi.c      | 4 +++-
>  9 files changed, 30 insertions(+), 15 deletions(-)
>