Unfortunately, fbdev does not create its own "struct device" for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is.
Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe.
Reported-by: Tom Gundersen teg@jklm.no Signed-off-by: David Herrmann dh.herrmann@gmail.com --- Hi
I know that simplefb was supposed to stay "as simple as possible" but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb.
Both patches are not critical at all and are targeted at 3.13-rc1.
Thanks David
drivers/video/simplefb.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c index 8d78106..74b016c 100644 --- a/drivers/video/simplefb.c +++ b/drivers/video/simplefb.c @@ -66,8 +66,15 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue, return 0; }
+static void simplefb_destroy(struct fb_info *info) +{ + if (info->screen_base) + iounmap(info->screen_base); +} + static struct fb_ops simplefb_ops = { .owner = THIS_MODULE, + .fb_destroy = simplefb_destroy, .fb_setcolreg = simplefb_setcolreg, .fb_fillrect = cfb_fillrect, .fb_copyarea = cfb_copyarea, @@ -212,8 +219,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->fbops = &simplefb_ops; info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE; - info->screen_base = devm_ioremap(&pdev->dev, info->fix.smem_start, - info->fix.smem_len); + info->screen_base = ioremap(info->fix.smem_start, + info->fix.smem_len); if (!info->screen_base) { framebuffer_release(info); return -ENODEV; @@ -223,6 +230,7 @@ static int simplefb_probe(struct platform_device *pdev) ret = register_framebuffer(info); if (ret < 0) { dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret); + iounmap(info->screen_base); framebuffer_release(info); return ret; }
Framebuffers shouldn't be cached and it is usually very uncommon to read them. Therefore, use ioremap_wc() to get significant speed improvements on systems which provide it. On all other systems it's aliased to ioremap_nocache() which is also fine.
Reported-by: Tom Gundersen teg@jklm.no Signed-off-by: David Herrmann dh.herrmann@gmail.com Tested-by: Tom Gundersen teg@jklm.no Tested-by: Alexandre Courbot acourbot@nvidia.com Tested-by: Stephen Warren swarren@wwwdotorg.org --- drivers/video/simplefb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c index 74b016c..64db54a 100644 --- a/drivers/video/simplefb.c +++ b/drivers/video/simplefb.c @@ -219,8 +219,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->fbops = &simplefb_ops; info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE; - info->screen_base = ioremap(info->fix.smem_start, - info->fix.smem_len); + info->screen_base = ioremap_wc(info->fix.smem_start, + info->fix.smem_len); if (!info->screen_base) { framebuffer_release(info); return -ENODEV;
Hi Tomi
Ping?
Thanks David
On Wed, Oct 2, 2013 at 4:58 PM, David Herrmann dh.herrmann@gmail.com wrote:
Framebuffers shouldn't be cached and it is usually very uncommon to read them. Therefore, use ioremap_wc() to get significant speed improvements on systems which provide it. On all other systems it's aliased to ioremap_nocache() which is also fine.
Reported-by: Tom Gundersen teg@jklm.no Signed-off-by: David Herrmann dh.herrmann@gmail.com Tested-by: Tom Gundersen teg@jklm.no Tested-by: Alexandre Courbot acourbot@nvidia.com Tested-by: Stephen Warren swarren@wwwdotorg.org
drivers/video/simplefb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c index 74b016c..64db54a 100644 --- a/drivers/video/simplefb.c +++ b/drivers/video/simplefb.c @@ -219,8 +219,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->fbops = &simplefb_ops; info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
info->screen_base = ioremap(info->fix.smem_start,info->fix.smem_len);
info->screen_base = ioremap_wc(info->fix.smem_start,info->fix.smem_len); if (!info->screen_base) { framebuffer_release(info); return -ENODEV;-- 1.8.4
On 2013-10-30 09:49, David Herrmann wrote:
Hi Tomi
Ping?
Thanks, queued this and the 1/2 patch for 3.13.
Tomi
Thanks David
On Wed, Oct 2, 2013 at 4:58 PM, David Herrmann dh.herrmann@gmail.com wrote:
Framebuffers shouldn't be cached and it is usually very uncommon to read them. Therefore, use ioremap_wc() to get significant speed improvements on systems which provide it. On all other systems it's aliased to ioremap_nocache() which is also fine.
Reported-by: Tom Gundersen teg@jklm.no Signed-off-by: David Herrmann dh.herrmann@gmail.com Tested-by: Tom Gundersen teg@jklm.no Tested-by: Alexandre Courbot acourbot@nvidia.com Tested-by: Stephen Warren swarren@wwwdotorg.org
drivers/video/simplefb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c index 74b016c..64db54a 100644 --- a/drivers/video/simplefb.c +++ b/drivers/video/simplefb.c @@ -219,8 +219,8 @@ static int simplefb_probe(struct platform_device *pdev)
info->fbops = &simplefb_ops; info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
info->screen_base = ioremap(info->fix.smem_start,info->fix.smem_len);
info->screen_base = ioremap_wc(info->fix.smem_start,info->fix.smem_len); if (!info->screen_base) { framebuffer_release(info); return -ENODEV;-- 1.8.4
On 10/02/2013 08:58 AM, David Herrmann wrote:
Unfortunately, fbdev does not create its own "struct device" for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is.
Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe.
So, once the .fb_destroy callback has been executed, there's no other callback to resurrect it? The framebuffer itself is still registered until the device's remove, yet after .fb_destroy, the memory is unmapped, which would be dangerous if the FB can be re-started.
If that's not an issue, this patch seems fine, so Acked-by: Stephen Warren swarren@nvidia.com
I know that simplefb was supposed to stay "as simple as possible" but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb.
I don't think this patch adds any significant complexity, so I'm not worried at least.
Hi
On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 10/02/2013 08:58 AM, David Herrmann wrote:
Unfortunately, fbdev does not create its own "struct device" for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is.
Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe.
So, once the .fb_destroy callback has been executed, there's no other callback to resurrect it? The framebuffer itself is still registered until the device's remove, yet after .fb_destroy, the memory is unmapped, which would be dangerous if the FB can be re-started.
fbdev lifetime tracking is weird.. ->fb_destroy() is called by unregister_framebuffer() _after_ it got unlinked. So no, the framebuffer is gone and cannot be resurrected. However, the unregistered/dead fbdev object itself stays around until you call framebuffer_release(). We cannot call it from fb_destroy(), though as the platform_data in your platform device still points to the fbdev device. Therefore we keep it and wait for the platform-driver to be removed which then again calls unregister_framebuffer() (which will immediately return as the fbdev device is not registered) and then you can finally call framebuffer_release().
Note that even though there's fb_get_info() and fb_put_info(), both are not exported and never used. So there is *no* fbdev ref-counting (which would be horribly broken anyway) and the driver is the sole owner of the fbdev object.
If that's not an issue, this patch seems fine, so Acked-by: Stephen Warren swarren@nvidia.com
Thanks!
I know that simplefb was supposed to stay "as simple as possible" but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb.
I don't think this patch adds any significant complexity, so I'm not worried at least.
Good to hear!
Thanks David
Hi Tomi
Could we get this in -next before the merge-window starts? Stephen already ack'ed it.
Thanks David
On Wed, Oct 2, 2013 at 6:23 PM, David Herrmann dh.herrmann@gmail.com wrote:
Hi
On Wed, Oct 2, 2013 at 6:16 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 10/02/2013 08:58 AM, David Herrmann wrote:
Unfortunately, fbdev does not create its own "struct device" for framebuffers. Instead, it attaches to the device of the parent layer. This has the side-effect that devm_* managed resources are not cleaned up on framebuffer-destruction but rather during destruction of the parent-device. In case of fbdev this might be too late, though. remove_conflicting_framebuffer() may remove fbdev devices but keep the parent device as it is.
Therefore, we now use plain ioremap() and unmap the framebuffer in the fb_destroy() callback. Note that we must not free the device here as this might race with the parent-device removal. Instead, we rely on unregister_framebuffer() as barrier and we're safe.
So, once the .fb_destroy callback has been executed, there's no other callback to resurrect it? The framebuffer itself is still registered until the device's remove, yet after .fb_destroy, the memory is unmapped, which would be dangerous if the FB can be re-started.
fbdev lifetime tracking is weird.. ->fb_destroy() is called by unregister_framebuffer() _after_ it got unlinked. So no, the framebuffer is gone and cannot be resurrected. However, the unregistered/dead fbdev object itself stays around until you call framebuffer_release(). We cannot call it from fb_destroy(), though as the platform_data in your platform device still points to the fbdev device. Therefore we keep it and wait for the platform-driver to be removed which then again calls unregister_framebuffer() (which will immediately return as the fbdev device is not registered) and then you can finally call framebuffer_release().
Note that even though there's fb_get_info() and fb_put_info(), both are not exported and never used. So there is *no* fbdev ref-counting (which would be horribly broken anyway) and the driver is the sole owner of the fbdev object.
If that's not an issue, this patch seems fine, so Acked-by: Stephen Warren swarren@nvidia.com
Thanks!
I know that simplefb was supposed to stay "as simple as possible" but I really think this series is the last set of fixes I have. Unfortunately framebuffer DRM handover is mandatory so we cannot ignore it in simplefb.
I don't think this patch adds any significant complexity, so I'm not worried at least.
Good to hear!
Thanks David
dri-devel@lists.freedesktop.org
-
David Herrmann -
Stephen Warren -
Tomi Valkeinen