* Re: [RFC] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar @ 2013-12-18 11:54 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Takashi Iwai, Stephen Warren,
the arch/x86 maintainers, linux-fbdev, Geert Uytterhoeven
In-Reply-To: <1387367283-20078-1-git-send-email-dh.herrmann@gmail.com>
* David Herrmann <dh.herrmann@gmail.com> wrote:
> If we probe a real hw driver for graphics devices, we need to unload any
> generic fallback driver like efifb/vesafb/simplefb on the system
> framebuffer. This is currently done via remove_conflicting_framebuffers()
> in fbmem.c. However, this only removes the fbdev driver, not the fake
> platform devices underneath. This hasn't been a problem so far, as efifb
> and vesafb didn't store any resources there. However, with simplefb this
> changed.
>
> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
> platform devices, we need to unregister the underlying platform device
> *before* probing any new hw driver. This patch adds sysfb_unregister() for
> that purpose. It can be called from any context (except from the
> platform-device ->remove callback path) and synchronously unloads any
> global sysfb and prevents new sysfbs from getting registered. Thus, you
> can call it even before any sysfb has been loaded.
>
> This also changes remove_conflicting_framebuffer() to call this helper
> *before* trying it's fbdev heuristic to remove conflicting drivers.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Hi
>
> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
> this in for 3.14. Your call..
>
> This patch basically simulates an unplug event for system-framebuffers when
> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
> optionally pass an aperture-struct and primary-flag similar to
> remove_conflicting_framebuffers(). If they're not passed, we remove it
> unconditionally.
>
> Untested, but my kernel compiles are already running. If my tests succeed and
> nobody has objections, I can resend it as proper PATCH and marked for stable.
> And maybe split the fbmem and sysfb changes into two patches..
Please fix the changelog to conform to the standard changelog style:
- first describe the symptoms of the bug - how does a user notice?
- then describe how the code behaves today and how that is causing the bug
- and then only describe how it's fixed.
The first item is the most important one - while developers
(naturally) tend to concentrate on the least important point, the last
one.
Thanks,
Ingo
^ permalink raw reply
* [RFC] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-18 11:48 UTC (permalink / raw)
To: linux-kernel
Cc: Takashi Iwai, Stephen Warren, the arch/x86 maintainers,
linux-fbdev, Geert Uytterhoeven, Ingo Molnar, David Herrmann
In-Reply-To: <s5hk3f2v15r.wl%tiwai@suse.de>
If we probe a real hw driver for graphics devices, we need to unload any
generic fallback driver like efifb/vesafb/simplefb on the system
framebuffer. This is currently done via remove_conflicting_framebuffers()
in fbmem.c. However, this only removes the fbdev driver, not the fake
platform devices underneath. This hasn't been a problem so far, as efifb
and vesafb didn't store any resources there. However, with simplefb this
changed.
To correctly release the IORESOURCE_MEM resources of simple-framebuffer
platform devices, we need to unregister the underlying platform device
*before* probing any new hw driver. This patch adds sysfb_unregister() for
that purpose. It can be called from any context (except from the
platform-device ->remove callback path) and synchronously unloads any
global sysfb and prevents new sysfbs from getting registered. Thus, you
can call it even before any sysfb has been loaded.
This also changes remove_conflicting_framebuffer() to call this helper
*before* trying it's fbdev heuristic to remove conflicting drivers.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi
This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
this in for 3.14. Your call..
This patch basically simulates an unplug event for system-framebuffers when
loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
optionally pass an aperture-struct and primary-flag similar to
remove_conflicting_framebuffers(). If they're not passed, we remove it
unconditionally.
Untested, but my kernel compiles are already running. If my tests succeed and
nobody has objections, I can resend it as proper PATCH and marked for stable.
And maybe split the fbmem and sysfb changes into two patches..
Thanks
David
arch/x86/include/asm/sysfb.h | 10 ++++
arch/x86/kernel/sysfb.c | 103 +++++++++++++++++++++++++++++++++++++--
arch/x86/kernel/sysfb_simplefb.c | 5 +-
drivers/video/fbmem.c | 16 ++++++
4 files changed, 128 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..713bc17 100644
--- a/arch/x86/include/asm/sysfb.h
+++ b/arch/x86/include/asm/sysfb.h
@@ -11,6 +11,7 @@
* any later version.
*/
+#include <linux/fb.h>
#include <linux/kernel.h>
#include <linux/platform_data/simplefb.h>
#include <linux/screen_info.h>
@@ -59,6 +60,15 @@ struct efifb_dmi_info {
int flags;
};
+__init struct platform_device *sysfb_alloc(const char *name,
+ int id,
+ const struct resource *res,
+ unsigned int res_num,
+ const void *data,
+ size_t data_size);
+__init int sysfb_register(struct platform_device *dev);
+void sysfb_unregister(const struct apertures_struct *apert, bool primary);
+
#ifdef CONFIG_EFI
extern struct efifb_dmi_info efifb_dmi_list[];
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
index 193ec2c..3d4554e 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,106 @@
#include <linux/init.h>
#include <linux/kernel.h>
#include <linux/mm.h>
+#include <linux/mutex.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
#include <linux/screen_info.h>
#include <asm/sysfb.h>
+static DEFINE_MUTEX(sysfb_lock);
+static struct platform_device *sysfb_dev;
+
+/* register @dev as sysfb; takes ownership over @dev */
+__init int sysfb_register(struct platform_device *dev)
+{
+ int r = 0;
+
+ mutex_lock(&sysfb_lock);
+ if (!sysfb_dev) {
+ r = platform_device_add(dev);
+ if (r < 0)
+ put_device(&dev->dev);
+ else
+ sysfb_dev = dev;
+ } else {
+ /* if there already is/was a sysfb, destroy @pd but return 0 */
+ platform_device_put(dev);
+ }
+ mutex_unlock(&sysfb_lock);
+
+ return r;
+}
+
+static bool sysfb_match(const struct apertures_struct *apert, bool primary)
+{
+ struct screen_info *si = &screen_info;
+ unsigned int i;
+ const struct aperture *a;
+
+ if (!apert || primary)
+ return true;
+
+ for (i = 0; i < apert->count; ++i) {
+ a = &apert->ranges[i];
+ if (a->base >= si->lfb_base &&
+ a->base < si->lfb_base + ((u64)si->lfb_size << 16))
+ return true;
+ if (si->lfb_base >= a->base &&
+ si->lfb_base < a->base + a->size)
+ return true;
+ }
+
+ return false;
+}
+
+/* unregister the sysfb and prevent new sysfbs from getting registered */
+void sysfb_unregister(const struct apertures_struct *apert, bool primary)
+{
+
+ mutex_lock(&sysfb_lock);
+ if (!IS_ERR(sysfb_dev)) {
+ if (sysfb_dev) {
+ if (sysfb_match(apert, primary)) {
+ platform_device_del(sysfb_dev);
+ platform_device_put(sysfb_dev);
+ sysfb_dev = ERR_PTR(-EALREADY);
+ }
+ } else {
+ sysfb_dev = ERR_PTR(-EALREADY);
+ }
+ }
+ mutex_unlock(&sysfb_lock);
+}
+
+__init struct platform_device *sysfb_alloc(const char *name,
+ int id,
+ const struct resource *res,
+ unsigned int res_num,
+ const void *data,
+ size_t data_size)
+{
+ struct platform_device *pd;
+ int ret;
+
+ pd = platform_device_alloc(name, id);
+ if (!pd)
+ return ERR_PTR(-ENOMEM);
+
+ ret = platform_device_add_resources(pd, res, res_num);
+ if (ret)
+ goto err;
+
+ ret = platform_device_add_data(pd, data, data_size);
+ if (ret)
+ goto err;
+
+ return pd;
+
+err:
+ platform_device_put(pd);
+ return ERR_PTR(ret);
+}
+
static __init int sysfb_init(void)
{
struct screen_info *si = &screen_info;
@@ -65,9 +160,11 @@ static __init int sysfb_init(void)
else
name = "platform-framebuffer";
- pd = platform_device_register_resndata(NULL, name, 0,
- NULL, 0, si, sizeof(*si));
- return IS_ERR(pd) ? PTR_ERR(pd) : 0;
+ pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
+ if (IS_ERR(pd))
+ return PTR_ERR(pd);
+
+ return sysfb_register(pd);
}
/* must execute after PCI subsystem for EFI quirks */
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..8e7bd23 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -86,10 +86,9 @@ __init int create_simplefb(const struct screen_info *si,
if (res.end <= res.start)
return -EINVAL;
- pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
- &res, 1, mode, sizeof(*mode));
+ pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
if (IS_ERR(pd))
return PTR_ERR(pd);
- return 0;
+ return sysfb_register(pd);
}
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..53e3894 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
#include <asm/fb.h>
+#if IS_ENABLED(CONFIG_X86_SYSFB)
+#include <asm/sysfb.h>
+#endif
/*
* Frame buffer device initialization and setup routines
@@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
}
}
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+ bool primary)
+{
+#if IS_ENABLED(CONFIG_X86_SYSFB)
+ sysfb_unregister(apert, primary);
+#endif
+}
+
static int do_register_framebuffer(struct fb_info *fb_info)
{
int i;
@@ -1742,6 +1753,8 @@ EXPORT_SYMBOL(unlink_framebuffer);
void remove_conflicting_framebuffers(struct apertures_struct *a,
const char *name, bool primary)
{
+ remove_conflicting_sysfb(a, primary);
+
mutex_lock(®istration_lock);
do_remove_conflicting_framebuffers(a, name, primary);
mutex_unlock(®istration_lock);
@@ -1762,6 +1775,9 @@ register_framebuffer(struct fb_info *fb_info)
{
int ret;
+ remove_conflicting_sysfb(fb_info->apertures,
+ fb_is_primary_device(fb_info));
+
mutex_lock(®istration_lock);
ret = do_register_framebuffer(fb_info);
mutex_unlock(®istration_lock);
--
1.8.5.1
^ permalink raw reply related
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-18 11:39 UTC (permalink / raw)
To: David Herrmann
Cc: Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <CANq1E4SEL6J50tvv-N9xSs6oA=Og96-7TQOECx=p-WGG3mU9=Q@mail.gmail.com>
At Wed, 18 Dec 2013 11:21:46 +0100,
David Herrmann wrote:
>
> Hi
>
> On Wed, Dec 18, 2013 at 11:17 AM, Takashi Iwai <tiwai@suse.de> 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 <tiwai@suse.de> 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…5821
> >> > >
> >> > > 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
---
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 <linux/screen_info.h>
#include <asm/sysfb.h>
+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 <linux/fb.h>
#include <asm/fb.h>
+#ifdef CONFIG_X86
+#include <asm/sysfb.h>
+#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;
^ permalink raw reply related
* Re: [PATCHv2 23/27] OMAPDSS: connector-dvi: Add DT support
From: Tomi Valkeinen @ 2013-12-18 10:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sascha Hauer, linux-omap, linux-fbdev, devicetree, Tony Lindgren
In-Reply-To: <1651476.iaILztiDEi@avalon>
[-- Attachment #1: Type: text/plain, Size: 3952 bytes --]
On 2013-12-17 17:15, Laurent Pinchart wrote:
>>> Either the driver is too specific or the binding is too generic, but
>>> having such a generic name for an omap specific driver seems wrong. Same
>>> for panel-dpi, svideo-connector, composite-video-connector and
>>> hdmi-connector,
>>
>> Hmm. Good point. I was thinking that the driver is only used on OMAP, but of
>> course that's not true, the driver is there for all platforms if the kernel
>> just happens to be compiled with the driver.
>>
>> And it's not just about those drivers you mention. The same issue is there
>> for, say, "ti,tpd12s015". I have an omapdss specific driver for that, but if
>> some other platform uses the same chip, they'll have a driver for it also...
>>
>> Sigh. I wonder how this should be handled... The only solution that comes to
>> my mind is to have all the compatible strings as "ti,...". But that's not
>> correct, as they are not TI components, but some are generic ones and some
>> from another vendor.
>>
>> And even "ti,..." is not good, as there are other TI SoCs with other display
>> drivers. So I'd need to prepend the compatible strings with "omapdss,...",
>> making the hardware components driver specific.
>>
>> The long term plan is to make the drivers generic (or implement the same
>> driver for common display framework). But for now I need to have future
>> proof DT bindings with omapdss specific drivers...
>
> I'll refrain from mentioning that the problem has been identified more than a
> year ago. Oh, wait, I've metioned it, sorry :-D
>
> We really want to make drivers generic enough to be shared by multiple display
> controllers. I would vote for making the DT bindings generic now already,
> which would be enough to buy some time to fix the problem properly. If we
> start prepending driver-specific prefixes such as "omapdss," to compatible
> string we'll just make the mess even larger and reduce the incentive to fix
> it. It would be the worst decision we could make.
Well... I think there are no good options here. I see the following options:
1. Add "omapdss," or similar to compat strings in DT, and the same for
the drivers. This solves the issue, but then we'll have bad DT data,
although I see similar method already being used in some places. When we
have common drivers, we can remove the "omapdss," strings from DT, but
we still need to keep them in the drivers to have backward compatibility.
2. Keep the compat strings generic in DT. This way we'll have correct DT
data, but if anyone happens to create a new driver with the same compat
string, things will break. omapdss can only be compiled for a kernel
with OMAP and ARM support, so it narrows the problematic drivers a bit.
3. Have correct DT data, but at init time, in omap arch code, go through
the DT data and change the compat strings for the display nodes to
include "omapdss,". This way the drivers would only work for omap
platforms. Like a combination of 1. and 2. I'm not sure if the DT-code
allows this at the moment, though.
We could also now select option 2., and go for 3. later if someone else
creates a driver with the same compat string.
While I'm obviously not very impartial here, I do think that using
generic bindings for omapdss is not the worst possible case, as:
I'm very much dedicated to get CDF merged at some point, and I already
have been working on it for each revision.
I also think that even if the omap panel drivers are currently omap
specific, they are not very much so. I have been changing them over the
years to be more and more generic, and I have used them as a base for
CDF panel drivers. If some platform specific driver may have a generic
compat string, omap panel drivers are not the worst option.
I will look at the option 3., hopefully that will be possible and
everybody will be happy. Any other ideas appreciated.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-18 10:21 UTC (permalink / raw)
To: Takashi Iwai
Cc: Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <s5hppouv4y4.wl%tiwai@suse.de>
Hi
On Wed, Dec 18, 2013 at 11:17 AM, Takashi Iwai <tiwai@suse.de> 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 <tiwai@suse.de> 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…5821
>> > >
>> > > 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.
Thanks
David
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a02121a..196360c54157 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];
> + struct platform_device *pdev;
> +};
> +
> 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,19 @@ 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;
> + struct platform_device *pdev = priv->pdev;
> +
> if (info->screen_base)
> iounmap(info->screen_base);
> +
> + /* release the bound platform device when called from
> + * remove_conflicting_framebuffers()
> + */
> + if (pdev) {
> + priv->pdev = NULL; /* to avoid double-free */
> + platform_device_unregister(pdev);
> + }
> }
>
> static struct fb_ops simplefb_ops = {
> @@ -169,6 +185,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 +205,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 +242,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 +262,7 @@ static int simplefb_probe(struct platform_device *pdev)
> return ret;
> }
>
> + priv->pdev = pdev;
> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>
> return 0;
> @@ -251,8 +271,13 @@ 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->pdev) {
> + priv->pdev = NULL; /* to avoid double-free */
> + unregister_framebuffer(info);
> + }
>
> - unregister_framebuffer(info);
> framebuffer_release(info);
>
> return 0;
^ permalink raw reply
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tomi Valkeinen @ 2013-12-18 10:21 UTC (permalink / raw)
To: Tony Lindgren; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B14AEE.8090807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]
On 2013-12-18 09:12, Tomi Valkeinen wrote:
>> Well yeah let's keep those separate still as at least Russell needed
>> some more time with the legacy booting. The point we can drop the
>> legacy booting for omap3 may still need to wait a bit, maybe even
>> until v3.15 to keep things working.
>
> They can't be separate. Once I add DT support for a board, I have to
> remove the legacy support for that board. This patch removes legacy
> support for the boards that are converted in the series.
>
> If I don't remove the legacy support, both DT and legacy side will be
> ran, and both create the DSS devices...
>
> But, it's true that this patch removes the whole file, as all the boards
> currently there are converted. But if new boards are added to the DSS
> quirks, then I can't remove the file. So I'll change this patch to only
> remove the parts for the converted boards, not the whole file.
>
> But but... If I understand right, the plan is to remove all omap3 board
> files for the next merge window. I'm not totally familiar with the
> quirks system, but how should this be handled:
>
> omap3.dtsi will contain the SoC's DSS nodes. This means that DSS devices
> are created via DT code. But if the display (i.e. panels) for a
> particular omap3 board has not been converted to DT, we should still use
> the legacy DSS initialization. But the DSS is already initialized via DT.
>
> I guess I can set the status for all the DSS nodes to "disabled" in
> omap3.dtsi, and that should prevent DT code from creating the DSS
> devices. Then, each omap3-board.dts that has been converted to DSS DT,
> can override those to "enabled".
>
> That way the DT code should not create DSS devices by default, and the
> quirks system would probably work fine.
I changed the DSS DT nodes to be disabled by default, and I think this
works nicely. It's actually better this way in any case, as we can leave
blocks like DSI out for boards that don't need it.
Also, I now remove the quirks only for the boards that are converted to
DT, not the whole dss-common.c file. So I think we can now add new
boards to dss-common.c, if and when there are ones that cannot be
converted to use DSS DT yet.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-18 10:17 UTC (permalink / raw)
To: David Herrmann
Cc: Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <s5hfvpqk0p8.wl%tiwai@suse.de>
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 <tiwai@suse.de> 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…5821
> > >
> > > 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...
thanks,
Takashi
---
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a02121a..196360c54157 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];
+ struct platform_device *pdev;
+};
+
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,19 @@ 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;
+ struct platform_device *pdev = priv->pdev;
+
if (info->screen_base)
iounmap(info->screen_base);
+
+ /* release the bound platform device when called from
+ * remove_conflicting_framebuffers()
+ */
+ if (pdev) {
+ priv->pdev = NULL; /* to avoid double-free */
+ platform_device_unregister(pdev);
+ }
}
static struct fb_ops simplefb_ops = {
@@ -169,6 +185,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 +205,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 +242,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 +262,7 @@ static int simplefb_probe(struct platform_device *pdev)
return ret;
}
+ priv->pdev = pdev;
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
@@ -251,8 +271,13 @@ 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->pdev) {
+ priv->pdev = NULL; /* to avoid double-free */
+ unregister_framebuffer(info);
+ }
- unregister_framebuffer(info);
framebuffer_release(info);
return 0;
^ permalink raw reply related
* Re: cirrusdrmfb broken with simplefb
From: Ingo Molnar @ 2013-12-18 9:29 UTC (permalink / raw)
To: David Herrmann
Cc: Takashi Iwai, Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <CANq1E4TFKDduy71jCXy+8CMS1_OFxWEpsHY3AA0RUJPW4VE8hg@mail.gmail.com>
* David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> 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…5821
> >
> > 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). I'm aware of the issue but as a
> workaround you can simply disable CONFIG_X86_SYSFB. That restores
> the old behavior.
This looks like a regression, so we'll either need a fix or we'll have
to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> 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?
> I will try to write a patch and send it later.
A fix would be awesome.
Thanks,
Ingo
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Geert Uytterhoeven @ 2013-12-18 8:48 UTC (permalink / raw)
To: Takashi Iwai
Cc: David Herrmann, Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <s5hfvpqk0p8.wl%tiwai@suse.de>
On Wed, Dec 18, 2013 at 9:44 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> 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?
Indeed. That's called a "regression" in distro/allmodconfig kernels.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-18 8:44 UTC (permalink / raw)
To: David Herrmann
Cc: Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <CANq1E4TFKDduy71jCXy+8CMS1_OFxWEpsHY3AA0RUJPW4VE8hg@mail.gmail.com>
At Wed, 18 Dec 2013 09:21:28 +0100,
David Herrmann wrote:
>
> Hi
>
> On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> 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…5821
> >
> > 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?
> I will try to write a patch and send it later.
Thanks!
Takashi
>
> Thanks
> David
>
> >
> > thanks,
> >
> > Takashi
> >
> > ---
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 3a94b79..f939236 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
> > }
> > EXPORT_SYMBOL_GPL(platform_device_add_data);
> >
> > +static void do_release_resources(struct platform_device *pdev, int nums)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < nums; i++) {
> > + struct resource *r = &pdev->resource[i];
> > + unsigned long type = resource_type(r);
> > +
> > + if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > + release_resource(r);
> > + }
> > +
> > + kfree(pdev->resource);
> > + pdev->resource = NULL;
> > + pdev->num_resources = 0;
> > +}
> > +
> > /**
> > * platform_device_add - add a platform device to device hierarchy
> > * @pdev: platform device we're adding
> > @@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
> > pdev->id = PLATFORM_DEVID_AUTO;
> > }
> >
> > - while (--i >= 0) {
> > - struct resource *r = &pdev->resource[i];
> > - unsigned long type = resource_type(r);
> > -
> > - if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > - release_resource(r);
> > - }
> > + do_release_resources(pdev, i - 1);
> >
> > err_out:
> > return ret;
> > @@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
> > */
> > void platform_device_del(struct platform_device *pdev)
> > {
> > - int i;
> > -
> > if (pdev) {
> > device_del(&pdev->dev);
> >
> > @@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
> > pdev->id = PLATFORM_DEVID_AUTO;
> > }
> >
> > - for (i = 0; i < pdev->num_resources; i++) {
> > - struct resource *r = &pdev->resource[i];
> > - unsigned long type = resource_type(r);
> > -
> > - if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> > - release_resource(r);
> > - }
> > + do_release_resources(pdev, pdev->num_resources);
> > }
> > }
> > EXPORT_SYMBOL_GPL(platform_device_del);
> >
> > +void platform_device_release_resources(struct platform_device *pdev)
> > +{
> > + do_release_resources(pdev, pdev->num_resources);
> > +}
> > +EXPORT_SYMBOL_GPL(platform_device_release_resources);
> > +
> > /**
> > * platform_device_register - add a platform-level device
> > * @pdev: platform device we're adding
> > diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> > index 210f3a0..fbf5e89 100644
> > --- a/drivers/video/simplefb.c
> > +++ b/drivers/video/simplefb.c
> > @@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
> > {
> > if (info->screen_base)
> > iounmap(info->screen_base);
> > + platform_device_release_resources(to_platform_device(info->device));
> > }
> >
> > static struct fb_ops simplefb_ops = {
> > diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> > index 16f6654..7cc1f54 100644
> > --- a/include/linux/platform_device.h
> > +++ b/include/linux/platform_device.h
> > @@ -42,6 +42,7 @@ struct platform_device {
> >
> > extern int platform_device_register(struct platform_device *);
> > extern void platform_device_unregister(struct platform_device *);
> > +extern void platform_device_release_resources(struct platform_device *pdev);
> >
> > extern struct bus_type platform_bus_type;
> > extern struct device platform_bus;
>
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-18 8:21 UTC (permalink / raw)
To: Takashi Iwai
Cc: Stephen Warren, linux-fbdev@vger.kernel.org,
the arch/x86 maintainers, linux-kernel
In-Reply-To: <s5hlhzik3kk.wl%tiwai@suse.de>
Hi
On Wed, Dec 18, 2013 at 8:42 AM, Takashi Iwai <tiwai@suse.de> 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…5821
>
> 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). I'm aware of the issue but as a
workaround you can simply disable CONFIG_X86_SYSFB. That restores the
old behavior.
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?
I will try to write a patch and send it later.
Thanks
David
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 3a94b79..f939236 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
> }
> EXPORT_SYMBOL_GPL(platform_device_add_data);
>
> +static void do_release_resources(struct platform_device *pdev, int nums)
> +{
> + int i;
> +
> + for (i = 0; i < nums; i++) {
> + struct resource *r = &pdev->resource[i];
> + unsigned long type = resource_type(r);
> +
> + if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> + release_resource(r);
> + }
> +
> + kfree(pdev->resource);
> + pdev->resource = NULL;
> + pdev->num_resources = 0;
> +}
> +
> /**
> * platform_device_add - add a platform device to device hierarchy
> * @pdev: platform device we're adding
> @@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
> pdev->id = PLATFORM_DEVID_AUTO;
> }
>
> - while (--i >= 0) {
> - struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> - release_resource(r);
> - }
> + do_release_resources(pdev, i - 1);
>
> err_out:
> return ret;
> @@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
> */
> void platform_device_del(struct platform_device *pdev)
> {
> - int i;
> -
> if (pdev) {
> device_del(&pdev->dev);
>
> @@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
> pdev->id = PLATFORM_DEVID_AUTO;
> }
>
> - for (i = 0; i < pdev->num_resources; i++) {
> - struct resource *r = &pdev->resource[i];
> - unsigned long type = resource_type(r);
> -
> - if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
> - release_resource(r);
> - }
> + do_release_resources(pdev, pdev->num_resources);
> }
> }
> EXPORT_SYMBOL_GPL(platform_device_del);
>
> +void platform_device_release_resources(struct platform_device *pdev)
> +{
> + do_release_resources(pdev, pdev->num_resources);
> +}
> +EXPORT_SYMBOL_GPL(platform_device_release_resources);
> +
> /**
> * platform_device_register - add a platform-level device
> * @pdev: platform device we're adding
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a0..fbf5e89 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
> {
> if (info->screen_base)
> iounmap(info->screen_base);
> + platform_device_release_resources(to_platform_device(info->device));
> }
>
> static struct fb_ops simplefb_ops = {
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index 16f6654..7cc1f54 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -42,6 +42,7 @@ struct platform_device {
>
> extern int platform_device_register(struct platform_device *);
> extern void platform_device_unregister(struct platform_device *);
> +extern void platform_device_release_resources(struct platform_device *pdev);
>
> extern struct bus_type platform_bus_type;
> extern struct device platform_bus;
^ permalink raw reply
* cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-18 7:42 UTC (permalink / raw)
To: David Herrmann; +Cc: Stephen Warren, linux-fbdev, x86, linux-kernel
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…5821
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?
thanks,
Takashi
---
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 3a94b79..f939236 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -267,6 +267,23 @@ int platform_device_add_data(struct platform_device *pdev, const void *data,
}
EXPORT_SYMBOL_GPL(platform_device_add_data);
+static void do_release_resources(struct platform_device *pdev, int nums)
+{
+ int i;
+
+ for (i = 0; i < nums; i++) {
+ struct resource *r = &pdev->resource[i];
+ unsigned long type = resource_type(r);
+
+ if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
+ release_resource(r);
+ }
+
+ kfree(pdev->resource);
+ pdev->resource = NULL;
+ pdev->num_resources = 0;
+}
+
/**
* platform_device_add - add a platform device to device hierarchy
* @pdev: platform device we're adding
@@ -342,13 +359,7 @@ int platform_device_add(struct platform_device *pdev)
pdev->id = PLATFORM_DEVID_AUTO;
}
- while (--i >= 0) {
- struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
-
- if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
- release_resource(r);
- }
+ do_release_resources(pdev, i - 1);
err_out:
return ret;
@@ -365,8 +376,6 @@ EXPORT_SYMBOL_GPL(platform_device_add);
*/
void platform_device_del(struct platform_device *pdev)
{
- int i;
-
if (pdev) {
device_del(&pdev->dev);
@@ -375,17 +384,17 @@ void platform_device_del(struct platform_device *pdev)
pdev->id = PLATFORM_DEVID_AUTO;
}
- for (i = 0; i < pdev->num_resources; i++) {
- struct resource *r = &pdev->resource[i];
- unsigned long type = resource_type(r);
-
- if (type = IORESOURCE_MEM || type = IORESOURCE_IO)
- release_resource(r);
- }
+ do_release_resources(pdev, pdev->num_resources);
}
}
EXPORT_SYMBOL_GPL(platform_device_del);
+void platform_device_release_resources(struct platform_device *pdev)
+{
+ do_release_resources(pdev, pdev->num_resources);
+}
+EXPORT_SYMBOL_GPL(platform_device_release_resources);
+
/**
* platform_device_register - add a platform-level device
* @pdev: platform device we're adding
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..fbf5e89 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -70,6 +70,7 @@ static void simplefb_destroy(struct fb_info *info)
{
if (info->screen_base)
iounmap(info->screen_base);
+ platform_device_release_resources(to_platform_device(info->device));
}
static struct fb_ops simplefb_ops = {
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 16f6654..7cc1f54 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -42,6 +42,7 @@ struct platform_device {
extern int platform_device_register(struct platform_device *);
extern void platform_device_unregister(struct platform_device *);
+extern void platform_device_release_resources(struct platform_device *pdev);
extern struct bus_type platform_bus_type;
extern struct device platform_bus;
^ permalink raw reply related
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tomi Valkeinen @ 2013-12-18 7:12 UTC (permalink / raw)
To: Tony Lindgren; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131217153001.GN12324@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]
On 2013-12-17 17:30, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 22:47]:
>> On 2013-12-16 20:41, Tony Lindgren wrote:
>>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 06:59]:
>>>> As a temporary solution to enable DSS for selected boards when booting
>>>> with DT, a hack was added to board-generic.c in
>>>> 63d5fc0c2f748e20f38a0a0ec1c8494bddf5c288 (OMAP: board-generic: enable
>>>> DSS for panda & sdp boards).
>>>>
>>>> We're now adding proper DT support, so the hack can be removed.
>>>
>>> I guess this patch should be merged later on after we have the DT support
>>> working?
>>
>> I'll move this patch also to the end of the series.
>>
>> "Merged later" sounds like you mean this could be merged in a separate
>> series. I think this and the other removal can be in this series, they
>> just need to be at the end.
>
> Well yeah let's keep those separate still as at least Russell needed
> some more time with the legacy booting. The point we can drop the
> legacy booting for omap3 may still need to wait a bit, maybe even
> until v3.15 to keep things working.
They can't be separate. Once I add DT support for a board, I have to
remove the legacy support for that board. This patch removes legacy
support for the boards that are converted in the series.
If I don't remove the legacy support, both DT and legacy side will be
ran, and both create the DSS devices...
But, it's true that this patch removes the whole file, as all the boards
currently there are converted. But if new boards are added to the DSS
quirks, then I can't remove the file. So I'll change this patch to only
remove the parts for the converted boards, not the whole file.
But but... If I understand right, the plan is to remove all omap3 board
files for the next merge window. I'm not totally familiar with the
quirks system, but how should this be handled:
omap3.dtsi will contain the SoC's DSS nodes. This means that DSS devices
are created via DT code. But if the display (i.e. panels) for a
particular omap3 board has not been converted to DT, we should still use
the legacy DSS initialization. But the DSS is already initialized via DT.
I guess I can set the status for all the DSS nodes to "disabled" in
omap3.dtsi, and that should prevent DT code from creating the DSS
devices. Then, each omap3-board.dts that has been converted to DSS DT,
can override those to "enabled".
That way the DT code should not create DSS devices by default, and the
quirks system would probably work fine.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 1/2] video/fb: Propagate error code from failing to unregister conflicting fb
From: Dave Airlie @ 2013-12-18 0:56 UTC (permalink / raw)
To: Chris Wilson, Jani Nikula, intel-gfx@lists.freedesktop.org,
Linux Fbdev development list, Tomi Valkeinen,
Jean-Christophe Plagniol-Villard, dri-devel
In-Reply-To: <20131217121435.GF22448@nuc-i3427.alporthouse.com>
On Tue, Dec 17, 2013 at 10:14 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Dec 17, 2013 at 09:33:08AM +0200, Jani Nikula wrote:
>> On Mon, 16 Dec 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > If we fail to remove a conflicting fb driver, we need to abort the
>> > loading of the second driver to avoid likely kernel panics.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
>> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> > Cc: linux-fbdev@vger.kernel.org
>> > Cc: dri-devel@lists.freedesktop.org
I'll merge this via the drm tree,
Dave.
^ permalink raw reply
* Re: [PATCH 00/26] OMAPDSS: DT support (Christmas edition)
From: Tony Lindgren @ 2013-12-18 0:30 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Laurent Pinchart, linux-omap, linux-fbdev, devicetree,
Archit Taneja, Darren Etheridge, Stefan Roese, Sebastian Reichel,
Robert Nelson, Dr . H . Nikolaus Schaller, Marek Belisko
In-Reply-To: <52AEAAB7.7010308@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131215 23:26]:
> On 2013-12-14 16:09, Tony Lindgren wrote:
>
> >> Why was e30b06f4d5f000c31a7747a7e7ada78a5fd419a1 merged into -rc2? It's
> >> not a fix, just a cleanup.
> >
> > Hmm OK sorry looks like I removed some non-legacy mux framework code
> > by accident. The omap_mux_* parts clearly are dead code for omap4 as
> > it's been DT only since v3.10, but the omap4_dsi_mux_pads() is not
> > using omap_mux_* functions.
> >
> > Yes, let's revert e30b06f4d5f0 except for the dead code parts, which
> > seems to be omap4_tpd12s015_mux_pads(), right?
>
> Yes. I tested the below patch on 4430SDP, both DSI and HDMI works.
OK thanks applying for the -rc cycle.
Tony
^ permalink raw reply
* [PATCH] video: vgacon: Don't build on arm64
From: Mark Brown @ 2013-12-17 23:37 UTC (permalink / raw)
To: linux-fbdev
From: Mark Brown <broonie@linaro.org>
arm64 is unlikely to have a VGA console and does not export screen_info
causing build failures if the driver is build, for example in all*config.
Add a dependency on !ARM64 to prevent this.
This list is getting quite long, it may be easier to depend on a symbol
which architectures that do support the driver can select.
Signed-off-by: Mark Brown <broonie@linaro.org>
---
drivers/video/console/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index 846caab75a46..c39d6c42c3ef 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -8,7 +8,8 @@ config VGA_CONSOLE
bool "VGA text console" if EXPERT || !X86
depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && \
!SUPERH && !BLACKFIN && !AVR32 && !MN10300 && !CRIS && \
- (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
+ (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER) \
+ && !ARM64
default y
help
Saying Y here will allow you to use Linux in text mode through a
--
1.8.5.1
^ permalink raw reply related
* [PATCH 05/11] fbcon: queue work on power efficient wq
From: Mark Brown @ 2013-12-17 21:56 UTC (permalink / raw)
To: linux-fbdev
From: Viresh Kumar <viresh.kumar@linaro.org>
fbcon uses workqueues and it has no real dependency of scheduling these on the
cpu which scheduled them.
On a idle system, it is observed that and idle cpu wakes up many times just to
service this work. It would be better if we can schedule it on a cpu which the
scheduler believes to be the most appropriate one.
This patch replaces system_wq with system_power_efficient_wq.
Cc: Dave Airlie <airlied@redhat.com>
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Tejun Heo <tj@kernel.org>
(cherry picked from commit a85f1a41f020bc2c97611060bcfae6f48a1db28d)
Signed-off-by: Mark Brown <broonie@linaro.org>
---
drivers/video/console/fbcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index a92783e480e6..0d8f98c79a6c 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -404,7 +404,7 @@ static void cursor_timer_handler(unsigned long dev_addr)
struct fb_info *info = (struct fb_info *) dev_addr;
struct fbcon_ops *ops = info->fbcon_par;
- schedule_work(&info->queue);
+ queue_work(system_power_efficient_wq, &info->queue);
mod_timer(&ops->cursor_timer, jiffies + HZ/5);
}
--
1.8.5.1
^ permalink raw reply related
* Re: [Xen-devel] [PATCH v3 1/2] xen/pvhvm: If xen_platform_pci=0 is set don't blow up (v3).
From: Konrad Rzeszutek Wilk @ 2013-12-17 21:23 UTC (permalink / raw)
To: Fabio Fantoni
Cc: axboe, stefano.stabellini, ian.campbell, xen-devel, linux-kernel,
boris.ostrovsky, david.vrabel, leosilva, ashley, peterhuewe, mail,
tpmdd, dmitry.torokhov, bhelgaas, plagnioj, tomi.valkeinen,
tpmdd-devel, linux-input, netdev, linux-pci, linux-fbdev
In-Reply-To: <20131217145150.GC4683@phenom.dumpdata.com>
On Tue, Dec 17, 2013 at 09:51:50AM -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Dec 17, 2013 at 10:54:47AM +0100, Fabio Fantoni wrote:
> > Il 16/12/2013 16:04, Konrad Rzeszutek Wilk ha scritto:
> > >The user has the option of disabling the platform driver:
> > >00:02.0 Unassigned class [ff80]: XenSource, Inc. Xen Platform Device (rev 01)
> > >
> > >which is used to unplug the emulated drivers (IDE, Realtek 8169, etc)
> > >and allow the PV drivers to take over. If the user wishes
> > >to disable that they can set:
> > >
> > > xen_platform_pci=0
> > > (in the guest config file)
> > >
> > >or
> > > xen_emul_unplug=never
> > > (on the Linux command line)
> > >
> > >except it does not work properly. The PV drivers still try to
> > >load and since the Xen platform driver is not run - and it
> > >has not initialized the grant tables, most of the PV drivers
> > >stumble upon:
> > >
> > >input: Xen Virtual Keyboard as /devices/virtual/input/input5
> > >input: Xen Virtual Pointer as /devices/virtual/input/input6M
> > >------------[ cut here ]------------
> > >kernel BUG at /home/konrad/ssd/konrad/linux/drivers/xen/grant-table.c:1206!
> > >invalid opcode: 0000 [#1] SMP
> > >Modules linked in: xen_kbdfront(+) xenfs xen_privcmd
> > >CPU: 6 PID: 1389 Comm: modprobe Not tainted 3.13.0-rc1upstream-00021-ga6c892b-dirty #1
> > >Hardware name: Xen HVM domU, BIOS 4.4-unstable 11/26/2013
> > >RIP: 0010:[<ffffffff813ddc40>] [<ffffffff813ddc40>] get_free_entries+0x2e0/0x300
> > >Call Trace:
> > > [<ffffffff8150d9a3>] ? evdev_connect+0x1e3/0x240
> > > [<ffffffff813ddd0e>] gnttab_grant_foreign_access+0x2e/0x70
> > > [<ffffffffa0010081>] xenkbd_connect_backend+0x41/0x290 [xen_kbdfront]
> > > [<ffffffffa0010a12>] xenkbd_probe+0x2f2/0x324 [xen_kbdfront]
> > > [<ffffffff813e5757>] xenbus_dev_probe+0x77/0x130
> > > [<ffffffff813e7217>] xenbus_frontend_dev_probe+0x47/0x50
> > > [<ffffffff8145e9a9>] driver_probe_device+0x89/0x230
> > > [<ffffffff8145ebeb>] __driver_attach+0x9b/0xa0
> > > [<ffffffff8145eb50>] ? driver_probe_device+0x230/0x230
> > > [<ffffffff8145eb50>] ? driver_probe_device+0x230/0x230
> > > [<ffffffff8145cf1c>] bus_for_each_dev+0x8c/0xb0
> > > [<ffffffff8145e7d9>] driver_attach+0x19/0x20
> > > [<ffffffff8145e260>] bus_add_driver+0x1a0/0x220
> > > [<ffffffff8145f1ff>] driver_register+0x5f/0xf0
> > > [<ffffffff813e55c5>] xenbus_register_driver_common+0x15/0x20
> > > [<ffffffff813e76b3>] xenbus_register_frontend+0x23/0x40
> > > [<ffffffffa0015000>] ? 0xffffffffa0014fff
> > > [<ffffffffa001502b>] xenkbd_init+0x2b/0x1000 [xen_kbdfront]
> > > [<ffffffff81002049>] do_one_initcall+0x49/0x170
> > >
> > >.. snip..
> > >
> > >which is hardly nice. This patch fixes this by having each
> > >PV driver check for:
> > > - if running in PV, then it is fine to execute (as that is their
> > > native environment).
> > > - if running in HVM, check if user wanted 'xen_emul_unplug=never',
> > > in which case bail out and don't load any PV drivers.
> > > - if running in HVM, and if PCI device 5853:0001 (xen_platform_pci)
> > > does not exist, then bail out and not load PV drivers.
> > > - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=disks',
> > > then bail out for all PV devices _except_ the block one.
> > > Ditto for the network one ('nics').
> > > - (v2) if running in HVM, and if the user wanted 'xen_emul_unplug=unnecessary'
> > > then load block PV driver, and also setup the legacy IDE paths.
> > > In (v3) make it actually load PV drivers.
> > >
> > >Reported-by: Sander Eikelenboom <linux@eikelenboom.it
> > >Reported-by: Anthony PERARD <anthony.perard@citrix.com>
> > >Reported-by: Fabio Fantoni <fabio.fantoni@m2r.biz>
> > >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > >[v2: Add extra logic to handle the myrid ways 'xen_emul_unplug'
> > >can be used per Ian and Stefano suggestion]
> > >[v3: Make the unnecessary case work properly]
> > >Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >
> > I tested this patch with all possible cases that I know, no crash or
> > calltrace found.
> >
> > I don't understand the utility of 'xen_emul_unplug=unnecessary' but
> > probably it is working correctly, it shows both pv and not pv for
> > blocks and nics.
>
> Great!
> >
> > About 'xen_emul_unplug=disks' and 'xen_emul_unplug=nics' probably
> > there is something wrong.
> > With 'xen_emul_unplug=nics' it shows pv nic (this should be correct)
> > and about disks it shows the same disk twice, as pv and not pv (xvda
> > and sda) with different device number.
>
> Good.
> > With 'xen_emul_unplug=disks' it shows pv block (this should be
> > correct) and it seems to show the pv nic too (this should be wrong).
>
> Correct. You should see only the PV disk and all other PV devices
> should not function. Let me dig in this and see whether there is
> another bug. Thank you!
That is because 'disks' is incorrect. It should have been 'ide-disks'
[ 0.000000] unrecognised option 'disks' in parameter 'xen_emul_unplug'
With the 'ide-disks' it should work. I will update the description to
mention 'ide-disks' instead of 'disks'. Thank you for finding this!
^ permalink raw reply
* Re: [PATCH] video: amba-clcd: Make CLCD driver available on more platforms
From: Mark Brown @ 2013-12-17 18:26 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1386868390-12231-1-git-send-email-broonie@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]
On Tue, Dec 17, 2013 at 04:27:34PM +0000, Catalin Marinas wrote:
> On 12 December 2013 17:13, Mark Brown <broonie@kernel.org> wrote:
> > The CLCD driver is used on ARM reference models for ARMv8 so add ARM64
> > to the list of dependencies. The driver also has no build time dependencies
> > on ARM (stubs are provided for ARM-specific DMA functions in the code) so
> > make it available with COMPILE_TEST in order to maximise build coverage.
> I haven't followed other CLCD patches but for arm64 it requires
> support for DT. I have been carrying some patches (done by Linaro) for
> a long time and Pawel @ ARM has another set of patches. Do you know
> what the status for those is before enabling CLCD on arm64? That's
> unless you are only looking at compile-testing.
Pawel's patches are going nowhere slowly as far as I can tell due to a
slow feedback loop and extensive discussion of the DT bindings.
I think this patch is an orthogonal thing, though - as you say it's
useful for compile testing on its own and it means the driver will load
as soon as the DT bindings are supported. It's one less patch to carry
out of tree and one less iteration when debugging why the framebuffer
isn't supported when testing.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Tomi Valkeinen @ 2013-12-17 17:29 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Benoît Cousson, Tony Lindgren, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131217171412.GA10442@earth.universe>
[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]
On 2013-12-17 19:14, Sebastian Reichel wrote:
> On Tue, Dec 17, 2013 at 09:37:35AM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 2013-12-13 20:17, Sebastian Reichel wrote:
>>> Hi,
>>>
>>> This patchset adds DT support for the N900 panel. The patchset is based on
>>> Tomi's work/dss-dt-2 branch [0]. I suggest to send the DT changes through
>>> Benoits queue, since I have more N900 DT changes for 3.14. Also the patch
>>> editing the rx51 boardcode can be dropped, since the file is removed in 3.14.
>>> I included those two with this patchset, since they are needed to test the
>>> other two patches.
>>>
>>> I did not include a documentation for the DT API, since the omapdss
>>> documentation is still missing.
>>>
>>> I have successfully tested this on the N900.
>>>
>>> [0] https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=work/dss-dt-2
>>
>> I added N900 display DT support on top of my v2 series, including
>> pinmuxing. Can you check if it looks right and works?
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
>
> I just tried it and it does not work. On a first look the pinmuxing
> looks fishy: 0x0d4 is muxed two times.
Hmm, so it is.
I'm not really familiar with SDI, I just muxed all the SDI pins, except
datapair3. I previously thought that there's only the data and clock
pairs for SDI, but the TRM revealed more sdi pins, so I included them.
It is well possible that these can be removed:
0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCHv2 17/27] ARM: omap3-tobi.dts: add lcd (TEST)
From: Florian Vaussard @ 2013-12-17 17:17 UTC (permalink / raw)
To: Tomi Valkeinen, linux-omap, linux-fbdev, devicetree
In-Reply-To: <1387205794-32246-18-git-send-email-tomi.valkeinen@ti.com>
Hello
On 12/16/2013 03:56 PM, Tomi Valkeinen wrote:
> This is a test for Overo with Palo43 expansion, _not_ Tobi. Palo43
> doesn't have a dts, but seems to work ok with omap3-tobi.dts, so I used
> it as a test.
>
> Not to be merged.
>
FYI, I got the DVI (HDMI connector) on the Overo/Tobi to work with the patch
below (not to be merged, and probably corrupted by our mail server), until a
framebuffer console. So for me:
Tested-by: Florian Vaussard <florian.vaussard@epfl.ch>
Regards,
Florian
8< --------------------------------
From 29237c38d35b833efe304b1a58463127555c4748 Mon Sep 17 00:00:00 2001
From: Florian Vaussard <florian.vaussard@epfl.ch>
Date: Tue, 17 Dec 2013 17:47:04 +0100
Subject: [PATCH] ARM: omap3-tobi.dts: add dvi output
Add DVI output to Overo/Tobi board.
Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
arch/arm/boot/dts/omap3-overo.dtsi | 33 +++++++++++++++++++++++
arch/arm/boot/dts/omap3-tobi.dts | 54
++++++++++++++++++++++++++++++++++++++
2 files changed, 87 insertions(+)
diff --git a/arch/arm/boot/dts/omap3-overo.dtsi
b/arch/arm/boot/dts/omap3-overo.dtsi
index a461d2f..90b08f6 100644
--- a/arch/arm/boot/dts/omap3-overo.dtsi
+++ b/arch/arm/boot/dts/omap3-overo.dtsi
@@ -89,6 +89,39 @@
0x170 (PIN_OUTPUT | MUX_MODE0) /*
uart3_tx_irtx.uart3_tx_irtx */
>;
};
+
+ dss_dpi_pins: pinmux_dss_dpi_pins {
+ pinctrl-single,pins = <
+ 0x0a4 (PIN_OUTPUT | MUX_MODE0) /*
dss_pclk.dss_pclk */
+ 0x0a6 (PIN_OUTPUT | MUX_MODE0) /*
dss_hsync.dss_hsync */
+ 0x0a8 (PIN_OUTPUT | MUX_MODE0) /*
dss_vsync.dss_vsync */
+ 0x0aa (PIN_OUTPUT | MUX_MODE0) /*
dss_acbias.dss_acbias */
+ 0x0ac (PIN_OUTPUT | MUX_MODE0) /*
dss_data0.dss_data0 */
+ 0x0ae (PIN_OUTPUT | MUX_MODE0) /*
dss_data1.dss_data1 */
+ 0x0b0 (PIN_OUTPUT | MUX_MODE0) /*
dss_data2.dss_data2 */
+ 0x0b2 (PIN_OUTPUT | MUX_MODE0) /*
dss_data3.dss_data3 */
+ 0x0b4 (PIN_OUTPUT | MUX_MODE0) /*
dss_data4.dss_data4 */
+ 0x0b6 (PIN_OUTPUT | MUX_MODE0) /*
dss_data5.dss_data5 */
+ 0x0b8 (PIN_OUTPUT | MUX_MODE0) /*
dss_data6.dss_data6 */
+ 0x0ba (PIN_OUTPUT | MUX_MODE0) /*
dss_data7.dss_data7 */
+ 0x0bc (PIN_OUTPUT | MUX_MODE0) /*
dss_data8.dss_data8 */
+ 0x0be (PIN_OUTPUT | MUX_MODE0) /*
dss_data9.dss_data9 */
+ 0x0c0 (PIN_OUTPUT | MUX_MODE0) /*
dss_data10.dss_data10 */
+ 0x0c2 (PIN_OUTPUT | MUX_MODE0) /*
dss_data11.dss_data11 */
+ 0x0c4 (PIN_OUTPUT | MUX_MODE0) /*
dss_data12.dss_data12 */
+ 0x0c6 (PIN_OUTPUT | MUX_MODE0) /*
dss_data13.dss_data13 */
+ 0x0c8 (PIN_OUTPUT | MUX_MODE0) /*
dss_data14.dss_data14 */
+ 0x0ca (PIN_OUTPUT | MUX_MODE0) /*
dss_data15.dss_data15 */
+ 0x0cc (PIN_OUTPUT | MUX_MODE0) /*
dss_data16.dss_data16 */
+ 0x0ce (PIN_OUTPUT | MUX_MODE0) /*
dss_data17.dss_data17 */
+ 0x0d0 (PIN_OUTPUT | MUX_MODE0) /*
dss_data18.dss_data18 */
+ 0x0d2 (PIN_OUTPUT | MUX_MODE0) /*
dss_data19.dss_data19 */
+ 0x0d4 (PIN_OUTPUT | MUX_MODE0) /*
dss_data20.dss_data20 */
+ 0x0d6 (PIN_OUTPUT | MUX_MODE0) /*
dss_data21.dss_data21 */
+ 0x0d8 (PIN_OUTPUT | MUX_MODE0) /*
dss_data22.dss_data22 */
+ 0x0da (PIN_OUTPUT | MUX_MODE0) /*
dss_data23.dss_data23 */
+ >;
+ };
};
&uart3 {
diff --git a/arch/arm/boot/dts/omap3-tobi.dts
b/arch/arm/boot/dts/omap3-tobi.dts
index 7e4ad2a..bfb1a1d 100644
--- a/arch/arm/boot/dts/omap3-tobi.dts
+++ b/arch/arm/boot/dts/omap3-tobi.dts
@@ -81,3 +81,57 @@
&mmc3 {
status = "disabled";
};
+
+&dss {
+ pinctrl-names = "default";
+ pinctrl-0 = <&dss_dpi_pins>;
+
+ vdds_dsi-supply = <&vpll2>;
+
+ dpi_out: endpoint {
+ remote-endpoint = <&tfp410_in>;
+ data-lines = <24>;
+ };
+};
+
+/ {
+ aliases {
+ display0 = &dvi0;
+ };
+
+ tfp410: encoder@0 {
+ compatible = "ti,tfp410";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ tfp410_in: endpoint@0 {
+ remote-endpoint = <&dpi_out>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ tfp410_out: endpoint@0 {
+ remote-endpoint <&dvi_connector_in>;
+ };
+ };
+ };
+ };
+
+ dvi0: connector@0 {
+ compatible = "dvi-connector";
+ label = "dvi";
+
+ i2c-bus = <&i2c3>;
+
+ dvi_connector_in: endpoint {
+ remote-endpoint = <&tfp410_out>;
+ };
+ };
+};
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-17 17:14 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Benoît Cousson, Tony Lindgren, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
linux-omap, linux-fbdev, devicetree
In-Reply-To: <52AFFF3F.7020807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]
On Tue, Dec 17, 2013 at 09:37:35AM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 2013-12-13 20:17, Sebastian Reichel wrote:
> > Hi,
> >
> > This patchset adds DT support for the N900 panel. The patchset is based on
> > Tomi's work/dss-dt-2 branch [0]. I suggest to send the DT changes through
> > Benoits queue, since I have more N900 DT changes for 3.14. Also the patch
> > editing the rx51 boardcode can be dropped, since the file is removed in 3.14.
> > I included those two with this patchset, since they are needed to test the
> > other two patches.
> >
> > I did not include a documentation for the DT API, since the omapdss
> > documentation is still missing.
> >
> > I have successfully tested this on the N900.
> >
> > [0] https://git.kernel.org/cgit/linux/kernel/git/tomba/linux.git/log/?h=work/dss-dt-2
>
> I added N900 display DT support on top of my v2 series, including
> pinmuxing. Can you check if it looks right and works?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
I just tried it and it does not work. On a first look the pinmuxing
looks fishy: 0x0d4 is muxed two times.
I will have a more detailed look into this later.
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] A patch to viafbdev.c
From: objectkuan @ 2013-12-17 16:57 UTC (permalink / raw)
To: linux-fbdev
There is a potential integer overflow in viafb_ioctl() if
users pass an unexpected value to viafb_second_size. An if
statement might be a simple solution to this issue.
Signed-off-by: objectkuan <objectkuan@gmail.com>
---
drivers/video/via/viafbdev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/via/viafbdev.c b/drivers/video/via/viafbdev.c
index 325c43c..382368e 100644
--- a/drivers/video/via/viafbdev.c
+++ b/drivers/video/via/viafbdev.c
@@ -463,6 +463,10 @@ static int viafb_ioctl(struct fb_info *info, u_int cmd, u_long arg)
u.viasamm.size_prim = viaparinfo->fbmem_free;
u.viasamm.size_sec = viaparinfo1->fbmem_free;
} else {
+ if (0 > viafb_second_size
+ || viafb_second_size > INT_MAX / 1024 / 1024) {
+ return -EFAULT;
+ }
if (viafb_second_size) {
u.viasamm.size_prim viaparinfo->fbmem_free -
@@ -1819,6 +1822,10 @@ int via_fb_pci_probe(struct viafb_dev *vdev)
default_var.accel_flags = 0;
}
+ if (0 > viafb_second_size
+ || viafb_second_size > INT_MAX / 1024 / 1024) {
+ return -EFAULT;
+ }
if (viafb_second_size && (viafb_second_size < 8)) {
viafb_second_offset = viaparinfo->fbmem_free -
viafb_second_size * 1024 * 1024;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH] video: amba-clcd: Make CLCD driver available on more platforms
From: Catalin Marinas @ 2013-12-17 16:27 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1386868390-12231-1-git-send-email-broonie@kernel.org>
On 12 December 2013 17:13, Mark Brown <broonie@kernel.org> wrote:
> From: Mark Brown <broonie@linaro.org>
>
> The CLCD driver is used on ARM reference models for ARMv8 so add ARM64
> to the list of dependencies. The driver also has no build time dependencies
> on ARM (stubs are provided for ARM-specific DMA functions in the code) so
> make it available with COMPILE_TEST in order to maximise build coverage.
I haven't followed other CLCD patches but for arm64 it requires
support for DT. I have been carrying some patches (done by Linaro) for
a long time and Pawel @ ARM has another set of patches. Do you know
what the status for those is before enabling CLCD on arm64? That's
unless you are only looking at compile-testing.
--
Catalin
^ permalink raw reply
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tony Lindgren @ 2013-12-17 15:30 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52AFF31C.6010100@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 22:47]:
> On 2013-12-16 20:41, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 06:59]:
> >> As a temporary solution to enable DSS for selected boards when booting
> >> with DT, a hack was added to board-generic.c in
> >> 63d5fc0c2f748e20f38a0a0ec1c8494bddf5c288 (OMAP: board-generic: enable
> >> DSS for panda & sdp boards).
> >>
> >> We're now adding proper DT support, so the hack can be removed.
> >
> > I guess this patch should be merged later on after we have the DT support
> > working?
>
> I'll move this patch also to the end of the series.
>
> "Merged later" sounds like you mean this could be merged in a separate
> series. I think this and the other removal can be in this series, they
> just need to be at the end.
Well yeah let's keep those separate still as at least Russell needed
some more time with the legacy booting. The point we can drop the
legacy booting for omap3 may still need to wait a bit, maybe even
until v3.15 to keep things working.
Regards,
Tony
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox