* 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
* 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: [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
* 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: 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
* 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: 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: 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: 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: [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: 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 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: 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
* [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: [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
* Re: [RFC] x86: sysfb: remove sysfb when probing real hw
From: Takashi Iwai @ 2013-12-18 13:03 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Stephen Warren, the arch/x86 maintainers,
linux-fbdev, Geert Uytterhoeven, Ingo Molnar
In-Reply-To: <1387367283-20078-1-git-send-email-dh.herrmann@gmail.com>
At Wed, 18 Dec 2013 12:48:03 +0100,
David Herrmann 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..
>
> 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;
> +}
Since sysfb_alloc() always follows sysfb_register() and they are
always coupled, we can simply combine both to one?
Also, do we really need a mutex? The functions in fbmem.c are already
in registeration_lock, so if this is called only from there, it should
be fine without an extra lock here. So, the function can be
simplified like:
int sysfb_register(const char *name, int id, const struct resource *res,
unsigned int res_num, const void *data, size_t data_size)
{
struct platform_device *pdev;
if (sysfb_dev)
return ret;
pdev = platform_device_register_resndata(....);
if (IS_ERR(pdev))
return PTR_ERR(pdev);
sysfb_dev = pdev;
return 0;
}
> +
> +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);
> +}
Simpler would be like:
void sysfb_unregister(const struct apertures_struct *apert, bool primary)
{
if (sysfb_dev && sysfb_match(apert, primary)) {
platform_device_unregister(sysfb_dev);
sysfb_dev = NULL;
}
}
> +
> +__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;
I don't think we need to open-code this if we can use
platform_device_register_*() helper.
> +
> +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
I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
So this can be called even for non-sysfb case (which is also good to
release the unused platform_device).
That is, this (and the above) can be #ifdef CONFIG_X86 instead.
thanks,
Takashi
^ permalink raw reply
* Re: [RFC] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-18 13:34 UTC (permalink / raw)
To: Takashi Iwai
Cc: linux-kernel, Stephen Warren, the arch/x86 maintainers,
linux-fbdev@vger.kernel.org, Geert Uytterhoeven, Ingo Molnar
In-Reply-To: <s5heh5aux8r.wl%tiwai@suse.de>
Hi
On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 18 Dec 2013 12:48:03 +0100,
> David Herrmann 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..
>>
>> 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;
>> +}
>
>
> Since sysfb_alloc() always follows sysfb_register() and they are
> always coupled, we can simply combine both to one?
We actually cannot call sysfb_register() for efi/vesa-framebuffer as
they lack a ->remove() callback. I fixed that in v2. I will send a
patch which adds ->remove() callbacks for vesafb and efifb later, but
this shouldn't go into this fix.
Furthermore, I think splitting them makes them easier to read.
> Also, do we really need a mutex? The functions in fbmem.c are already
> in registeration_lock, so if this is called only from there, it should
> be fine without an extra lock here. So, the function can be
> simplified like:
They have to be called outside of the fb-mutex. We trigger the
->remove() callback, which will call unregister_framebuffer() which
will lock the fb-mutex => deadlock. And as
remove_conflicting_framebuffers() can be called in parallel by many
drivers, we need the lock in sysfb. We could use an
atomic_test_and_dec(), but that might cause one removal to overtake
the previous unregistration. Thus I added the mutex.
Also note that with CONFIG_FB=n and the pending SimpleDRM driver, we
will call sysfb_unregister() from outside of fbmem.c. If we depend on
fbmem.c internal locks, we need to change it for 3.14/15 again.
> int sysfb_register(const char *name, int id, const struct resource *res,
> unsigned int res_num, const void *data, size_t data_size)
> {
> struct platform_device *pdev;
> if (sysfb_dev)
> return ret;
> pdev = platform_device_register_resndata(....);
> if (IS_ERR(pdev))
> return PTR_ERR(pdev);
> sysfb_dev = pdev;
> return 0;
> }
>
>
>> +
>> +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);
>> +}
>
> Simpler would be like:
>
> void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> {
> if (sysfb_dev && sysfb_match(apert, primary)) {
> platform_device_unregister(sysfb_dev);
> sysfb_dev = NULL;
> }
> }
Nope, if sysfb_dev is NULL, I need to set it to ERR_PTR(-sth).
Otherwise, imagine i915 getting loaded before sysfb_init(). It calls
sysfb_unregister() without a registered sysfb and continues. A later
sysfb_init() will then load sysfb anyway. With my code, this is
prevented by setting sysfb_dev to ERR_PTR(-EALREADY).
Maybe the device-init ordering prevents this, but I'm not entirely
sure about this so lets be safe and have a strict ordering.
>
>> +
>> +__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;
>
> I don't think we need to open-code this if we can use
> platform_device_register_*() helper.
>
>
>> +
>> +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
>
> I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
> So this can be called even for non-sysfb case (which is also good to
> release the unused platform_device).
>
> That is, this (and the above) can be #ifdef CONFIG_X86 instead.
Yepp, fixed.
I will send v2 in a minute. I tested it on x86 with efifb+no-sysfb and
simplefb+sysfb, both handovers worked fine.
Thanks
David
^ permalink raw reply
* [PATCH v2] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-18 13:50 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: <1387367283-20078-1-git-send-email-dh.herrmann@gmail.com>
With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
resource-conflicts and drivers will refuse to load. A call to
request_mem_region() will fail, if the region overlaps with the mem-region
used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
are not affected as they don't reserve their resources, but some others
do, including (nvidiafb, cirrus, ..).
The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
platform-device during bootup but never release it. Probing simplefb on
this platform-device is fine, but the handover to real-hw via
remove_conflicting_framebuffers() will only unload the fbdev driver, but
keep the platform-device around. Thus, if the real hw-driver calls
request_mem_region() and friends on the same PCI region, we will get a
resource conflict and most drivers refuse to load. Users will see
errors like:
"nvidiafb: cannot reserve video memory at <xyz>"
vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
and falling back to those drivers will avoid the bug. With sysfb enabled,
we need to properly unload the simple-framebuffer devices before probing
real hw-drivers.
This patch adds sysfb_unregister() for that purpose. It can be called from
any context (except from the platform-device ->probe and ->remove callback
path), synchronously unloads any global sysfb and prevents new sysfbs from
getting registered. Thus, you can call it even before any sysfb has been
loaded. Note that for now we only do that for simple-framebuffer devices,
as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
They're not affected by the resource-conflicts, so we can fix those later.
This also changes remove_conflicting_framebuffer() to call this helper
*before* trying its heuristic to remove conflicting drivers. This way, we
unload sysfbs properly on any conflict. But to avoid dead-locks in
register_framebuffer(), we must not call sysfb_unregister() for
framebuffers probing on sysfb devices. Hence, we simply remove any
aperture from simplefb and we're good to go. simplefb is unregistered by
sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
there currently is no handover from simplefb, so we're fine. If it's
added, they need to provide something like sysfb_unregister() too)
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
- remove simplefb apertures to avoid dead-lock
- skip sysfb_unregister() if no apertures are set
- merge sysfb_alloc() into sysfb_register()
- simplify sysfb_unregister() logic
- call sysfb_unregister() even if SYSFB=n but X86=y
Tested with efifb and simplefb + i915 handover on x86.
Thanks
David
arch/x86/include/asm/sysfb.h | 6 ++++
arch/x86/kernel/sysfb.c | 63 ++++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/sysfb_simplefb.c | 9 ++----
drivers/video/fbmem.c | 19 ++++++++++++
drivers/video/simplefb.c | 8 -----
5 files changed, 90 insertions(+), 15 deletions(-)
diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..10d719d 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,11 @@ struct efifb_dmi_info {
int flags;
};
+__init int sysfb_register(const char *name, int id,
+ const struct resource *res, unsigned int res_num,
+ const void *data, size_t data_size);
+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..9d8da8d 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,74 @@
#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;
+
+__init int sysfb_register(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 = 0;
+
+ mutex_lock(&sysfb_lock);
+ if (!sysfb_dev) {
+ pd = platform_device_register_resndata(NULL, name, id,
+ res, res_num,
+ data, data_size);
+ if (IS_ERR(pd))
+ ret = PTR_ERR(pd);
+ else
+ sysfb_dev = pd;
+ }
+ mutex_unlock(&sysfb_lock);
+
+ return ret;
+}
+
+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) && sysfb_dev) {
+ if (sysfb_match(apert, primary)) {
+ platform_device_unregister(sysfb_dev);
+ sysfb_dev = ERR_PTR(-EALREADY);
+ }
+ } else {
+ sysfb_dev = ERR_PTR(-EALREADY);
+ }
+ mutex_unlock(&sysfb_lock);
+}
+
static __init int sysfb_init(void)
{
struct screen_info *si = &screen_info;
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..a760d47 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
__init int create_simplefb(const struct screen_info *si,
const struct simplefb_platform_data *mode)
{
- struct platform_device *pd;
struct resource res;
unsigned long len;
@@ -86,10 +85,6 @@ __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));
- if (IS_ERR(pd))
- return PTR_ERR(pd);
-
- return 0;
+ return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
+ sizeof(*mode));
}
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..590a46a 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
#include <asm/fb.h>
+#if IS_ENABLED(CONFIG_X86)
+#include <asm/sysfb.h>
+#endif
/*
* Frame buffer device initialization and setup routines
@@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
}
}
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+ bool primary)
+{
+ if (!apert)
+ return;
+
+#if IS_ENABLED(CONFIG_X86)
+ sysfb_unregister(apert, primary);
+#endif
+}
+
static int do_register_framebuffer(struct fb_info *fb_info)
{
int i;
@@ -1742,6 +1756,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 +1778,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);
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..9f4a0cf 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
info->var.blue = params.format->blue;
info->var.transp = params.format->transp;
- info->apertures = alloc_apertures(1);
- if (!info->apertures) {
- framebuffer_release(info);
- return -ENOMEM;
- }
- info->apertures->ranges[0].base = info->fix.smem_start;
- info->apertures->ranges[0].size = info->fix.smem_len;
-
info->fbops = &simplefb_ops;
info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
info->screen_base = ioremap_wc(info->fix.smem_start,
--
1.8.5.1
^ permalink raw reply related
* Re: [RFC] x86: sysfb: remove sysfb when probing real hw
From: Takashi Iwai @ 2013-12-18 14:02 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Stephen Warren, the arch/x86 maintainers,
linux-fbdev@vger.kernel.org, Geert Uytterhoeven, Ingo Molnar
In-Reply-To: <CANq1E4QZuB_iBcnfUd=Kr1LYqKUoWykriT0wyacZYjcjHcSFMw@mail.gmail.com>
At Wed, 18 Dec 2013 14:34:53 +0100,
David Herrmann wrote:
>
> Hi
>
> On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Wed, 18 Dec 2013 12:48:03 +0100,
> > David Herrmann 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..
> >>
> >> 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;
> >> +}
> >
> >
> > Since sysfb_alloc() always follows sysfb_register() and they are
> > always coupled, we can simply combine both to one?
>
> We actually cannot call sysfb_register() for efi/vesa-framebuffer as
> they lack a ->remove() callback. I fixed that in v2. I will send a
> patch which adds ->remove() callbacks for vesafb and efifb later, but
> this shouldn't go into this fix.
> Furthermore, I think splitting them makes them easier to read.
>
> > Also, do we really need a mutex? The functions in fbmem.c are already
> > in registeration_lock, so if this is called only from there, it should
> > be fine without an extra lock here. So, the function can be
> > simplified like:
>
> They have to be called outside of the fb-mutex. We trigger the
> ->remove() callback, which will call unregister_framebuffer() which
> will lock the fb-mutex => deadlock. And as
> remove_conflicting_framebuffers() can be called in parallel by many
> drivers, we need the lock in sysfb. We could use an
> atomic_test_and_dec(), but that might cause one removal to overtake
> the previous unregistration. Thus I added the mutex.
>
> Also note that with CONFIG_FB=n and the pending SimpleDRM driver, we
> will call sysfb_unregister() from outside of fbmem.c. If we depend on
> fbmem.c internal locks, we need to change it for 3.14/15 again.
OK, but please add a comment on it (that it can be called outside fb
lock).
> > int sysfb_register(const char *name, int id, const struct resource *res,
> > unsigned int res_num, const void *data, size_t data_size)
> > {
> > struct platform_device *pdev;
> > if (sysfb_dev)
> > return ret;
> > pdev = platform_device_register_resndata(....);
> > if (IS_ERR(pdev))
> > return PTR_ERR(pdev);
> > sysfb_dev = pdev;
> > return 0;
> > }
> >
> >
> >> +
> >> +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);
> >> +}
> >
> > Simpler would be like:
> >
> > void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> > {
> > if (sysfb_dev && sysfb_match(apert, primary)) {
> > platform_device_unregister(sysfb_dev);
> > sysfb_dev = NULL;
> > }
> > }
>
> Nope, if sysfb_dev is NULL, I need to set it to ERR_PTR(-sth).
> Otherwise, imagine i915 getting loaded before sysfb_init(). It calls
> sysfb_unregister() without a registered sysfb and continues. A later
> sysfb_init() will then load sysfb anyway. With my code, this is
> prevented by setting sysfb_dev to ERR_PTR(-EALREADY).
Fair enough. But this would be better to be commented there, too, for
avoiding further stray sheep :)
> Maybe the device-init ordering prevents this, but I'm not entirely
> sure about this so lets be safe and have a strict ordering.
> >> +
> >> +__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;
> >
> > I don't think we need to open-code this if we can use
> > platform_device_register_*() helper.
> >
> >
> >> +
> >> +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
> >
> > I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
> > So this can be called even for non-sysfb case (which is also good to
> > release the unused platform_device).
> >
> > That is, this (and the above) can be #ifdef CONFIG_X86 instead.
>
> Yepp, fixed.
>
> I will send v2 in a minute. I tested it on x86 with efifb+no-sysfb and
> simplefb+sysfb, both handovers worked fine.
OK, I'll test it soon.
thanks!
Takashi
^ permalink raw reply
* Re: [PATCHv2 23/27] OMAPDSS: connector-dvi: Add DT support
From: Tomi Valkeinen @ 2013-12-18 14:02 UTC (permalink / raw)
To: Laurent Pinchart, Tony Lindgren
Cc: Sascha Hauer, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <52B17BBF.8090003-l0cyMroinI0@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]
On 2013-12-18 12:41, Tomi Valkeinen wrote:
> 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.
This wasn't actually too hard. It says "hack" all over it, but the code
was quite compact. I call the omapdss_early_init_of() as the first thing
in omap_generic_init(), before the devices are created:
+static const char* const dss_compat_conv_list[] = {
+ "hdmi-connector",
+ "dvi-connector",
+ "ti,tpd12s015",
+ "panel-dsi-cm",
+};
+
+static void omapdss_omapify_node(struct device_node *node, const char
*compat)
+{
+ char *new_compat;
+ struct property *prop;
+
+ new_compat = kasprintf(GFP_KERNEL, "omapdss,%s", compat);
+
+ prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+ prop->name = "compatible";
+ prop->value = new_compat;
+ prop->length = strlen(new_compat) + 1;
+
+ of_update_property(node, prop);
+}
+
+void __init omapdss_early_init_of(void)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(dss_compat_conv_list); ++i) {
+ const char *compat = dss_compat_conv_list[i];
+ struct device_node *node = NULL;
+
+ while ((node = of_find_compatible_node(node, NULL,
compat))) {
+ if (!of_device_is_available(node))
+ continue;
+
+ omapdss_omapify_node(node, compat);
+ }
+ }
+}
The list has just part of the devices, and I've so far only tested on
OMAP 4430sdp board, but it seemed to work fine.
So with this, I can have "hdmi-connector" in the .dts file, and
"omapdss,hdmi-connector" as a compat string in the omap specific driver.
Does it make your eyes bleed, or is it maybe something that could be
fine for the time being?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v2] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-18 14:15 UTC (permalink / raw)
To: linux-kernel, Pavel Roskin
Cc: Takashi Iwai, Stephen Warren, the arch/x86 maintainers,
linux-fbdev@vger.kernel.org, Geert Uytterhoeven, Ingo Molnar,
David Herrmann
In-Reply-To: <1387374611-12493-1-git-send-email-dh.herrmann@gmail.com>
Hi Pavel
On Wed, Dec 18, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> resource-conflicts and drivers will refuse to load. A call to
> request_mem_region() will fail, if the region overlaps with the mem-region
> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> are not affected as they don't reserve their resources, but some others
> do, including (nvidiafb, cirrus, ..).
>
> The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
> platform-device during bootup but never release it. Probing simplefb on
> this platform-device is fine, but the handover to real-hw via
> remove_conflicting_framebuffers() will only unload the fbdev driver, but
> keep the platform-device around. Thus, if the real hw-driver calls
> request_mem_region() and friends on the same PCI region, we will get a
> resource conflict and most drivers refuse to load. Users will see
> errors like:
> "nvidiafb: cannot reserve video memory at <xyz>"
>
> vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
> and falling back to those drivers will avoid the bug. With sysfb enabled,
> we need to properly unload the simple-framebuffer devices before probing
> real hw-drivers.
>
> This patch adds sysfb_unregister() for that purpose. It can be called from
> any context (except from the platform-device ->probe and ->remove callback
> path), synchronously unloads any global sysfb and prevents new sysfbs from
> getting registered. Thus, you can call it even before any sysfb has been
> loaded. Note that for now we only do that for simple-framebuffer devices,
> as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
> They're not affected by the resource-conflicts, so we can fix those later.
>
> This also changes remove_conflicting_framebuffer() to call this helper
> *before* trying its heuristic to remove conflicting drivers. This way, we
> unload sysfbs properly on any conflict. But to avoid dead-locks in
> register_framebuffer(), we must not call sysfb_unregister() for
> framebuffers probing on sysfb devices. Hence, we simply remove any
> aperture from simplefb and we're good to go. simplefb is unregistered by
> sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
> there currently is no handover from simplefb, so we're fine. If it's
> added, they need to provide something like sysfb_unregister() too)
As you reported issues regarding nvidiafb, can you give this a try, too?
Thanks
David
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
> - remove simplefb apertures to avoid dead-lock
> - skip sysfb_unregister() if no apertures are set
> - merge sysfb_alloc() into sysfb_register()
> - simplify sysfb_unregister() logic
> - call sysfb_unregister() even if SYSFB=n but X86=y
>
> Tested with efifb and simplefb + i915 handover on x86.
>
> Thanks
> David
>
> arch/x86/include/asm/sysfb.h | 6 ++++
> arch/x86/kernel/sysfb.c | 63 ++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/sysfb_simplefb.c | 9 ++----
> drivers/video/fbmem.c | 19 ++++++++++++
> drivers/video/simplefb.c | 8 -----
> 5 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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,11 @@ struct efifb_dmi_info {
> int flags;
> };
>
> +__init int sysfb_register(const char *name, int id,
> + const struct resource *res, unsigned int res_num,
> + const void *data, size_t data_size);
> +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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
> #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;
> +
> +__init int sysfb_register(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 = 0;
> +
> + mutex_lock(&sysfb_lock);
> + if (!sysfb_dev) {
> + pd = platform_device_register_resndata(NULL, name, id,
> + res, res_num,
> + data, data_size);
> + if (IS_ERR(pd))
> + ret = PTR_ERR(pd);
> + else
> + sysfb_dev = pd;
> + }
> + mutex_unlock(&sysfb_lock);
> +
> + return ret;
> +}
> +
> +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) && sysfb_dev) {
> + if (sysfb_match(apert, primary)) {
> + platform_device_unregister(sysfb_dev);
> + sysfb_dev = ERR_PTR(-EALREADY);
> + }
> + } else {
> + sysfb_dev = ERR_PTR(-EALREADY);
> + }
> + mutex_unlock(&sysfb_lock);
> +}
> +
> static __init int sysfb_init(void)
> {
> struct screen_info *si = &screen_info;
> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> index 86179d4..a760d47 100644
> --- a/arch/x86/kernel/sysfb_simplefb.c
> +++ b/arch/x86/kernel/sysfb_simplefb.c
> @@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
> __init int create_simplefb(const struct screen_info *si,
> const struct simplefb_platform_data *mode)
> {
> - struct platform_device *pd;
> struct resource res;
> unsigned long len;
>
> @@ -86,10 +85,6 @@ __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));
> - if (IS_ERR(pd))
> - return PTR_ERR(pd);
> -
> - return 0;
> + return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
> + sizeof(*mode));
> }
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 010d191..590a46a 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -35,6 +35,9 @@
>
> #include <asm/fb.h>
>
> +#if IS_ENABLED(CONFIG_X86)
> +#include <asm/sysfb.h>
> +#endif
>
> /*
> * Frame buffer device initialization and setup routines
> @@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
> }
> }
>
> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> + bool primary)
> +{
> + if (!apert)
> + return;
> +
> +#if IS_ENABLED(CONFIG_X86)
> + sysfb_unregister(apert, primary);
> +#endif
> +}
> +
> static int do_register_framebuffer(struct fb_info *fb_info)
> {
> int i;
> @@ -1742,6 +1756,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 +1778,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);
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a0..9f4a0cf 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
> info->var.blue = params.format->blue;
> info->var.transp = params.format->transp;
>
> - info->apertures = alloc_apertures(1);
> - if (!info->apertures) {
> - framebuffer_release(info);
> - return -ENOMEM;
> - }
> - info->apertures->ranges[0].base = info->fix.smem_start;
> - info->apertures->ranges[0].size = info->fix.smem_len;
> -
> info->fbops = &simplefb_ops;
> info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
> info->screen_base = ioremap_wc(info->fix.smem_start,
> --
> 1.8.5.1
>
^ permalink raw reply
* Re: [PATCH v2] x86: sysfb: remove sysfb when probing real hw
From: Takashi Iwai @ 2013-12-18 14:21 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Stephen Warren, the arch/x86 maintainers,
linux-fbdev, Geert Uytterhoeven, Ingo Molnar
In-Reply-To: <1387374611-12493-1-git-send-email-dh.herrmann@gmail.com>
At Wed, 18 Dec 2013 14:50:11 +0100,
David Herrmann wrote:
>
> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> resource-conflicts and drivers will refuse to load. A call to
> request_mem_region() will fail, if the region overlaps with the mem-region
> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> are not affected as they don't reserve their resources, but some others
> do, including (nvidiafb, cirrus, ..).
>
> The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
> platform-device during bootup but never release it. Probing simplefb on
> this platform-device is fine, but the handover to real-hw via
> remove_conflicting_framebuffers() will only unload the fbdev driver, but
> keep the platform-device around. Thus, if the real hw-driver calls
> request_mem_region() and friends on the same PCI region, we will get a
> resource conflict and most drivers refuse to load. Users will see
> errors like:
> "nvidiafb: cannot reserve video memory at <xyz>"
>
> vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
> and falling back to those drivers will avoid the bug. With sysfb enabled,
> we need to properly unload the simple-framebuffer devices before probing
> real hw-drivers.
>
> This patch adds sysfb_unregister() for that purpose. It can be called from
> any context (except from the platform-device ->probe and ->remove callback
> path), synchronously unloads any global sysfb and prevents new sysfbs from
> getting registered. Thus, you can call it even before any sysfb has been
> loaded. Note that for now we only do that for simple-framebuffer devices,
> as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
> They're not affected by the resource-conflicts, so we can fix those later.
>
> This also changes remove_conflicting_framebuffer() to call this helper
> *before* trying its heuristic to remove conflicting drivers. This way, we
> unload sysfbs properly on any conflict. But to avoid dead-locks in
> register_framebuffer(), we must not call sysfb_unregister() for
> framebuffers probing on sysfb devices. Hence, we simply remove any
> aperture from simplefb and we're good to go. simplefb is unregistered by
> sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
> there currently is no handover from simplefb, so we're fine. If it's
> added, they need to provide something like sysfb_unregister() too)
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
> - remove simplefb apertures to avoid dead-lock
> - skip sysfb_unregister() if no apertures are set
> - merge sysfb_alloc() into sysfb_register()
> - simplify sysfb_unregister() logic
> - call sysfb_unregister() even if SYSFB=n but X86=y
>
> Tested with efifb and simplefb + i915 handover on x86.
The patch worked fine on QEMU with cirrus driver.
I'd like to see comments, as suggested in my previous mail, in a
couple of places. Other than that, the patch looks good to me.
So, feel free to my tested-by and reviewed-by tags:
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
>
> Thanks
> David
>
> arch/x86/include/asm/sysfb.h | 6 ++++
> arch/x86/kernel/sysfb.c | 63 ++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/sysfb_simplefb.c | 9 ++----
> drivers/video/fbmem.c | 19 ++++++++++++
> drivers/video/simplefb.c | 8 -----
> 5 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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,11 @@ struct efifb_dmi_info {
> int flags;
> };
>
> +__init int sysfb_register(const char *name, int id,
> + const struct resource *res, unsigned int res_num,
> + const void *data, size_t data_size);
In most cases, we declare like
int __init sysfb_register(...
> +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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
> #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;
> +
> +__init int sysfb_register(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 = 0;
> +
> + mutex_lock(&sysfb_lock);
> + if (!sysfb_dev) {
> + pd = platform_device_register_resndata(NULL, name, id,
> + res, res_num,
> + data, data_size);
> + if (IS_ERR(pd))
> + ret = PTR_ERR(pd);
> + else
> + sysfb_dev = pd;
> + }
> + mutex_unlock(&sysfb_lock);
> +
> + return ret;
> +}
> +
> +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) && sysfb_dev) {
> + if (sysfb_match(apert, primary)) {
> + platform_device_unregister(sysfb_dev);
> + sysfb_dev = ERR_PTR(-EALREADY);
> + }
> + } else {
> + sysfb_dev = ERR_PTR(-EALREADY);
> + }
> + mutex_unlock(&sysfb_lock);
> +}
> +
> static __init int sysfb_init(void)
> {
> struct screen_info *si = &screen_info;
> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
> index 86179d4..a760d47 100644
> --- a/arch/x86/kernel/sysfb_simplefb.c
> +++ b/arch/x86/kernel/sysfb_simplefb.c
> @@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
> __init int create_simplefb(const struct screen_info *si,
> const struct simplefb_platform_data *mode)
> {
> - struct platform_device *pd;
> struct resource res;
> unsigned long len;
>
> @@ -86,10 +85,6 @@ __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));
> - if (IS_ERR(pd))
> - return PTR_ERR(pd);
> -
> - return 0;
> + return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
> + sizeof(*mode));
> }
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 010d191..590a46a 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -35,6 +35,9 @@
>
> #include <asm/fb.h>
>
> +#if IS_ENABLED(CONFIG_X86)
> +#include <asm/sysfb.h>
> +#endif
>
> /*
> * Frame buffer device initialization and setup routines
> @@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
> }
> }
>
> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> + bool primary)
> +{
> + if (!apert)
> + return;
> +
> +#if IS_ENABLED(CONFIG_X86)
> + sysfb_unregister(apert, primary);
> +#endif
> +}
> +
> static int do_register_framebuffer(struct fb_info *fb_info)
> {
> int i;
> @@ -1742,6 +1756,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 +1778,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);
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index 210f3a0..9f4a0cf 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
> info->var.blue = params.format->blue;
> info->var.transp = params.format->transp;
>
> - info->apertures = alloc_apertures(1);
> - if (!info->apertures) {
> - framebuffer_release(info);
> - return -ENOMEM;
> - }
> - info->apertures->ranges[0].base = info->fix.smem_start;
> - info->apertures->ranges[0].size = info->fix.smem_len;
> -
> info->fbops = &simplefb_ops;
> info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
> info->screen_base = ioremap_wc(info->fix.smem_start,
> --
> 1.8.5.1
>
^ permalink raw reply
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tony Lindgren @ 2013-12-18 17:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B14AEE.8090807@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131217 23:14]:
> 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.
Oh OK yeah makes sense. I was worried about the legacy board-*.c files..
> 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.
OK
> 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.
No need for DSS related quirks for the DT boot case. I was concerned about
the legacy board-*.c case.
Tony
^ permalink raw reply
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tony Lindgren @ 2013-12-18 17:33 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B17729.3040807@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131218 02:22]:
> 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.
OK
Tony
^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-18 21:55 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: <52B089FE.8060704@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]
On Tue, Dec 17, 2013 at 07:29:34PM +0200, Tomi Valkeinen wrote:
> >> 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 */
Just removing the dss_data20.sdi_den pin was enough to get a working display. I
don't know if the other pins are needed, because the display pins are already
muxed correctly by the bootloader.
-- Sebastian
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index 39e5e50..33f29ac 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -163,7 +163,7 @@
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 */
+ //0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
0x0d8 (PIN_OUTPUT | MUX_MODE1) /* dss_data22.sdi_clkp */
0x0da (PIN_OUTPUT | MUX_MODE1) /* dss_data23.sdi_clkn */
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
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