From patchwork Wed Dec 18 11:39:12 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 3369741 Return-Path: X-Original-To: patchwork-linux-fbdev@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 8A987C0D4A for ; Wed, 18 Dec 2013 11:39:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 158502045B for ; Wed, 18 Dec 2013 11:39:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 75E1920458 for ; Wed, 18 Dec 2013 11:39:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753731Ab3LRLjQ (ORCPT ); Wed, 18 Dec 2013 06:39:16 -0500 Received: from cantor2.suse.de ([195.135.220.15]:49890 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752336Ab3LRLjO (ORCPT ); Wed, 18 Dec 2013 06:39:14 -0500 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D7BD8AC07; Wed, 18 Dec 2013 11:39:12 +0000 (UTC) Date: Wed, 18 Dec 2013 12:39:12 +0100 Message-ID: From: Takashi Iwai To: David Herrmann Cc: Stephen Warren , "linux-fbdev@vger.kernel.org" , "the arch/x86 maintainers" , linux-kernel Subject: Re: cirrusdrmfb broken with simplefb In-Reply-To: References: User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Sender: linux-fbdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fbdev@vger.kernel.org X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP At Wed, 18 Dec 2013 11:21:46 +0100, David Herrmann wrote: > > Hi > > On Wed, Dec 18, 2013 at 11:17 AM, Takashi Iwai wrote: > > At Wed, 18 Dec 2013 09:44:35 +0100, > > Takashi Iwai wrote: > >> > >> At Wed, 18 Dec 2013 09:21:28 +0100, > >> David Herrmann wrote: > >> > > >> > Hi > >> > > >> > On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai wrote: > >> > > Hi, > >> > > > >> > > with the recent enablement of simplefb on x86, cirrusdrmfb on QEMU/KVM > >> > > gets broken now, as reported at: > >> > > https://bugzilla.novell.com/show_bug.cgi?id=855821 > >> > > > >> > > The cirrus VGA resource is reserved at first as "BOOTFB" in > >> > > arch/x86/kernel/sysfb_simplefb.c, which is taken by simplefb platform > >> > > device. This resource is, however, never released until the platform > >> > > device is destroyed, and the framebuffer switching doesn't trigger > >> > > it. It calls fb's destroy callback, at most. Then, cirrus driver > >> > > tries to assign the resource, fails and gives up, resulting in a > >> > > complete blank screen. > >> > > > >> > > The same problem should exist on other KMS drivers like mgag200 or > >> > > ast, not only cirrus. Intel graphics doesn't hit this problem just > >> > > because the reserved iomem by BOOTFB isn't required by i915 driver. > >> > > > >> > > The patch below is a quick attempt to solve the issue. It adds a new > >> > > API function for releasing resources of platform_device, and call it > >> > > in destroy op of simplefb. But, forcibly releasing resources of a > >> > > parent device doesn't sound like a correct design. We may take such > >> > > as a band aid, but definitely need a more fundamental fix. > >> > > > >> > > Any thoughts? > >> > > >> > That bug always existed, simplefb is just the first driver to hit it > >> > (vesafb/efifb didn't use resources). > >> > >> Heh, the bug didn't "exist" because no other grabbed the resource > >> before. The way the cirrus driver allocates the resource is no bug, > >> per se. But the proper resource takeover is missing. > >> > >> > I'm aware of the issue but as a > >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores the > >> > old behavior. > >> > >> Yes, but CONFIG_X86_SYSFB breaks things as of now. Shouldn't it be > >> mentioned or give some kconfig hints instead of silently giving a > >> broken output, if any quick fix isn't possible? > >> > >> > As a proper fix, I'd propose something like: > >> > dev = platform_find_device("platform-framebuffer"); > >> > platform_remove_device(dev); > >> > > >> > And we wrap this as: > >> > sysfb_remove_framebuffers() > >> > in arch/x86/kernel/sysfb.c > >> > > >> > This should cause a ->remove() event for the platform-driver and > >> > correctly release the resources. Comments? > >> > >> Who will call this? From destroy callback? > >> Or can we simply do put_device() the bound platform device instead? > > > > Just tested, put_device() doesn't work since the refcount isn't 1, so > > if any, we need to call platform_device_unregister() to release > > forcibly. > > > > The patch below seems working at a quick test. But I still have > > some bad feeling for unregistering the parent device from the child's > > destroy callback... > > The idea is right, but call it from > "remove_conflicting_framebuffers()". The problem is, if we load a > driver on top of a dummy like vesafb/efifb/simplefb, we never removed > them properly. Instead, we only unregistered their fb driver. But with > the driver-model, that's just removing the driver, not the dummy > device. > > So we'd just call platform_device_unregister() in > remove_conflicting_framebuffers() instead of unregister_framebuffer() > on such devices. I'm now at home and will have a look. So, make sysfb_remove_framebuffers() available on all archs and call it there? Hmm... I think we need a better binding between platform_device and framebuffer device. Maybe better than killing a parent device, but IMO, the root cause is rather that we have two individual devices (platform and framebuffer) unnecessarily. Can we sort this out? In anyway, the patch below is a quick revisited fix. It's calling sysfb_remove_framebuffers(). Instead of finding over a platform name string as you suggested, the patch just tries to remember the instance. The ifdef in fbmem.c looks ugly, but we can clean up such a thing later. And, yes, the patch should be split. thanks, Takashi --- -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h index 2aeb3e2..94ba0e4 100644 --- a/arch/x86/include/asm/sysfb.h +++ b/arch/x86/include/asm/sysfb.h @@ -76,8 +76,9 @@ static inline void sysfb_apply_efi_quirks(void) bool parse_mode(const struct screen_info *si, struct simplefb_platform_data *mode); -int create_simplefb(const struct screen_info *si, +struct platform_device *create_simplefb(const struct screen_info *si, const struct simplefb_platform_data *mode); +void sysfb_remove_framebuffers(void); #else /* CONFIG_X86_SYSFB */ @@ -87,10 +88,10 @@ static inline bool parse_mode(const struct screen_info *si, return false; } -static inline int create_simplefb(const struct screen_info *si, +static inline struct platform_device *create_simplefb(const struct screen_info *si, const struct simplefb_platform_data *mode) { - return -EINVAL; + return ERR_PTR(-EINVAL); } #endif /* CONFIG_X86_SYSFB */ diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c index 193ec2c..4faa5d0 100644 --- a/arch/x86/kernel/sysfb.c +++ b/arch/x86/kernel/sysfb.c @@ -38,23 +38,22 @@ #include #include +static struct platform_device *pd; + static __init int sysfb_init(void) { struct screen_info *si = &screen_info; struct simplefb_platform_data mode; - struct platform_device *pd; const char *name; bool compatible; - int ret; sysfb_apply_efi_quirks(); /* try to create a simple-framebuffer device */ compatible = parse_mode(si, &mode); if (compatible) { - ret = create_simplefb(si, &mode); - if (!ret) - return 0; + pd = create_simplefb(si, &mode); + return IS_ERR(pd) ? PTR_ERR(pd) : 0; } /* if the FB is incompatible, create a legacy framebuffer device */ @@ -70,5 +69,14 @@ static __init int sysfb_init(void) return IS_ERR(pd) ? PTR_ERR(pd) : 0; } +void sysfb_remove_framebuffers(void) +{ + if (!IS_ERR_OR_NULL(pd)) { + platform_device_unregister(pd); + pd = NULL; + } +} +EXPORT_SYMBOL_GPL(sysfb_remove_framebuffers); + /* must execute after PCI subsystem for EFI quirks */ device_initcall(sysfb_init); diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c index 86179d4..d90042ac 100644 --- a/arch/x86/kernel/sysfb_simplefb.c +++ b/arch/x86/kernel/sysfb_simplefb.c @@ -61,10 +61,9 @@ __init bool parse_mode(const struct screen_info *si, return false; } -__init int create_simplefb(const struct screen_info *si, +__init struct platform_device *create_simplefb(const struct screen_info *si, const struct simplefb_platform_data *mode) { - struct platform_device *pd; struct resource res; unsigned long len; @@ -74,7 +73,7 @@ __init int create_simplefb(const struct screen_info *si, len = PAGE_ALIGN(len); if (len > (u64)si->lfb_size << 16) { printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n"); - return -EINVAL; + return ERR_PTR(-EINVAL); } /* setup IORESOURCE_MEM as framebuffer memory */ @@ -84,12 +83,8 @@ __init int create_simplefb(const struct screen_info *si, res.start = si->lfb_base; res.end = si->lfb_base + len - 1; if (res.end <= res.start) - return -EINVAL; + return ERR_PTR(-EINVAL); - pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0, + return platform_device_register_resndata(NULL, "simple-framebuffer", 0, &res, 1, mode, sizeof(*mode)); - if (IS_ERR(pd)) - return PTR_ERR(pd); - - return 0; } diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c index 010d191..49b0f89 100644 --- a/drivers/video/fbmem.c +++ b/drivers/video/fbmem.c @@ -34,6 +34,9 @@ #include #include +#ifdef CONFIG_X86 +#include +#endif /* @@ -1600,6 +1603,9 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, "%s vs %s - removing generic driver\n", name, registered_fb[i]->fix.id); do_unregister_framebuffer(registered_fb[i]); +#ifdef CONFIG_X86 + sysfb_remove_framebuffers(); +#endif } } } diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c index 210f3a0..c825b88 100644 --- a/drivers/video/simplefb.c +++ b/drivers/video/simplefb.c @@ -41,6 +41,11 @@ static struct fb_var_screeninfo simplefb_var = { .vmode = FB_VMODE_NONINTERLACED, }; +struct simplefb_priv { + u32 pseudo_palette[16]; + bool fb_registered; +}; + static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue, u_int transp, struct fb_info *info) { @@ -68,8 +73,11 @@ static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue, static void simplefb_destroy(struct fb_info *info) { + struct simplefb_priv *priv = info->par; + if (info->screen_base) iounmap(info->screen_base); + priv->fb_registered = false; } static struct fb_ops simplefb_ops = { @@ -169,6 +177,7 @@ static int simplefb_probe(struct platform_device *pdev) struct simplefb_params params; struct fb_info *info; struct resource *mem; + struct simplefb_priv *priv; if (fb_get_options("simplefb", NULL)) return -ENODEV; @@ -188,7 +197,7 @@ static int simplefb_probe(struct platform_device *pdev) return -EINVAL; } - info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev); + info = framebuffer_alloc(sizeof(struct simplefb_priv), &pdev->dev); if (!info) return -ENOMEM; platform_set_drvdata(pdev, info); @@ -225,7 +234,9 @@ static int simplefb_probe(struct platform_device *pdev) framebuffer_release(info); return -ENODEV; } - info->pseudo_palette = (void *)(info + 1); + + priv = info->par; + info->pseudo_palette = priv->pseudo_palette; dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n", info->fix.smem_start, info->fix.smem_len, @@ -243,6 +254,7 @@ static int simplefb_probe(struct platform_device *pdev) return ret; } + priv->fb_registered = true; dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node); return 0; @@ -251,8 +263,11 @@ static int simplefb_probe(struct platform_device *pdev) static int simplefb_remove(struct platform_device *pdev) { struct fb_info *info = platform_get_drvdata(pdev); + struct simplefb_priv *priv = info->par; + + if (priv->fb_registered) + unregister_framebuffer(info); - unregister_framebuffer(info); framebuffer_release(info); return 0;