[PATCH 1/3] fbmem: fix aperture overlapping check
marcin....@gmail.com
is completely whithin the other. Add generic ranges_overlap macro
(probably kernel.h candidate) and use it here.
Signed-off-by: Marcin Slusarz <marcin....@gmail.com>
Cc: Dave Airlie <air...@redhat.com>
Cc: Peter Jones <pjo...@redhat.com>
Cc: Andrew Morton <ak...@linux-foundation.org>
---
drivers/video/fbmem.c | 24 +++++++++++++++++-------
1 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a15b44e..41f2e5e 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1468,15 +1468,25 @@ static int fb_check_foreignness(struct fb_info *fi)
return 0;
}
+/**
+ * ranges_overlap - check whether two ranges overlap (their intersection is not empty)
+ * @start1: start of the first range
+ * @size1: length of the first range
+ * @start2: start of the second range
+ * @size2: length of the second range
+ */
+#define ranges_overlap(start1, size1, start2, size2) ({ \
+ typeof(start1) __start1 = (start1); \
+ typeof(size1) __size1 = (size1); \
+ typeof(start2) __start2 = (start2); \
+ typeof(size2) __size2 = (size2); \
+ __start1 < __start2 + __size2 && __start1 + __size1 > __start2; \
+})
+
static bool fb_do_apertures_overlap(struct fb_info *gen, struct fb_info *hw)
{
- /* is the generic aperture base the same as the HW one */
- if (gen->aperture_base == hw->aperture_base)
- return true;
- /* is the generic aperture base inside the hw base->hw base+size */
- if (gen->aperture_base > hw->aperture_base && gen->aperture_base <= hw->aperture_base + hw->aperture_size)
- return true;
- return false;
+ return ranges_overlap(gen->aperture_base, gen->aperture_size,
+ hw->aperture_base, hw->aperture_size);
}
/**
* register_framebuffer - registers a frame buffer device
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Dave Airlie
> fb_do_apertures_overlap is returning wrong value when one aperture
> is completely whithin the other. Add generic ranges_overlap macro
> (probably kernel.h candidate) and use it here.
>
That doesn't seem right.
The rules are:
the generic aperture has to be equal or smaller than the hw aperture,
otherwise the generic driver will be trashing random hw pieces on the
machine.
So with that in mind, the check makes sure the generic aperture starts
somewhere inside the hw aperture, which the test clearly gets right.
Have you got a pointer to a machine where it fails?
Dave.
--
Marcin Slusarz
> On Sat, 2010-04-10 at 21:55 +0200, marcin....@gmail.com wrote:
> > fb_do_apertures_overlap is returning wrong value when one aperture
> > is completely whithin the other. Add generic ranges_overlap macro
> > (probably kernel.h candidate) and use it here.
> >
>
> That doesn't seem right.
>
> The rules are:
>
> the generic aperture has to be equal or smaller than the hw aperture,
> otherwise the generic driver will be trashing random hw pieces on the
> machine.
Why "it has to"? Why generic aperture can't start one byte before hw?
> So with that in mind, the check makes sure the generic aperture starts
> somewhere inside the hw aperture, which the test clearly gets right.
So your only objection is that it's impossible with the current code?
> Have you got a pointer to a machine where it fails?
No, it failed with an artifical test while I was working on vga16fb handoff
(unfinished).
Dave Airlie
> On Mon, Apr 12, 2010 at 09:54:28AM +1000, Dave Airlie wrote:
> > On Sat, 2010-04-10 at 21:55 +0200, marcin....@gmail.com wrote:
> > > fb_do_apertures_overlap is returning wrong value when one aperture
> > > is completely whithin the other. Add generic ranges_overlap macro
> > > (probably kernel.h candidate) and use it here.
> > >
> >
> > That doesn't seem right.
> >
> > The rules are:
> >
> > the generic aperture has to be equal or smaller than the hw aperture,
> > otherwise the generic driver will be trashing random hw pieces on the
> > machine.
> Why "it has to"? Why generic aperture can't start one byte before hw?
Think about how it works a bit more, the answer is obvious.
The hw aperture is defined by the actual hardware, not by the OS or my
code. The PCI apertures are well defined. Now for a generic driver like
vesa or offb to access the hardware it needs to access the hw apertures.
It can't go accessing one byte before the hw aperture, as that will
crash the machine, or write to some other devices hw.
>
> > So with that in mind, the check makes sure the generic aperture starts
> > somewhere inside the hw aperture, which the test clearly gets right.
>
> So your only objection is that it's impossible with the current code?
>
> > Have you got a pointer to a machine where it fails?
>
> No, it failed with an artifical test while I was working on vga16fb handoff
> (unfinished).
You won't be able to make this work for vga16fb from what I can see
since it access 0xa000 directly, not via any of the defined apertures
that vesafb/offb use. vga16fb will need a different approach I suspect.
Dave.
Marcin Slusarz
> On Mon, 2010-04-12 at 13:34 +0200, Marcin Slusarz wrote:
> > On Mon, Apr 12, 2010 at 09:54:28AM +1000, Dave Airlie wrote:
> > > On Sat, 2010-04-10 at 21:55 +0200, marcin....@gmail.com wrote:
> > > > fb_do_apertures_overlap is returning wrong value when one aperture
> > > > is completely whithin the other. Add generic ranges_overlap macro
> > > > (probably kernel.h candidate) and use it here.
> > > >
> > >
> > > That doesn't seem right.
> > >
> > > The rules are:
> > >
> > > the generic aperture has to be equal or smaller than the hw aperture,
> > > otherwise the generic driver will be trashing random hw pieces on the
> > > machine.
>
> > Why "it has to"? Why generic aperture can't start one byte before hw?
>
> Think about how it works a bit more, the answer is obvious.
>
> The hw aperture is defined by the actual hardware, not by the OS or my
> code. The PCI apertures are well defined. Now for a generic driver like
> vesa or offb to access the hardware it needs to access the hw apertures.
>
> It can't go accessing one byte before the hw aperture, as that will
> crash the machine, or write to some other devices hw.
Ok, thanks for explanation. I'll drop this patch and rebase the others.
> > > So with that in mind, the check makes sure the generic aperture starts
> > > somewhere inside the hw aperture, which the test clearly gets right.
> >
> > So your only objection is that it's impossible with the current code?
> >
> > > Have you got a pointer to a machine where it fails?
> >
> > No, it failed with an artifical test while I was working on vga16fb handoff
> > (unfinished).
>
> You won't be able to make this work for vga16fb from what I can see
> since it access 0xa000 directly, not via any of the defined apertures
> that vesafb/offb use. vga16fb will need a different approach I suspect.
Yeah, there's an idea to claim 0xa0000 as an aperture of vga16fb and then
do the same in KMS driver but only if it's the primary card.
(You hinted to use SHADOW resource bit to check whether card is primary)
I think I'll try this approach soon.
Marcin
Dave Airlie
> Ok, thanks for explanation. I'll drop this patch and rebase the others.
Cool,
>> You won't be able to make this work for vga16fb from what I can see
>> since it access 0xa000 directly, not via any of the defined apertures
>> that vesafb/offb use. vga16fb will need a different approach I suspect.
>
> Yeah, there's an idea to claim 0xa0000 as an aperture of vga16fb and then
> do the same in KMS driver but only if it's the primary card.
> (You hinted to use SHADOW resource bit to check whether card is primary)
> I think I'll try this approach soon.
Yeah as long as we kick it off once, certain things like vga switcheroo change
who the primary gpu is at runtime.
but it should be okay if both drivers have the kickoff paths.
Dave.
Marcin Slusarz
> > > > Have you got a pointer to a machine where it fails?
> > >
> > > No, it failed with an artifical test while I was working on vga16fb handoff
> > > (unfinished).
> >
> > You won't be able to make this work for vga16fb from what I can see
> > since it access 0xa000 directly, not via any of the defined apertures
> > that vesafb/offb use. vga16fb will need a different approach I suspect.
>
> Yeah, there's an idea to claim 0xa0000 as an aperture of vga16fb and then
> do the same in KMS driver but only if it's the primary card.
> (You hinted to use SHADOW resource bit to check whether card is primary)
> I think I'll try this approach soon.
Patch below works for me, but I couldn't test the case when nvidia card is secondary.
---
From: Marcin Slusarz <marcin....@gmail.com>
Date: Tue, 13 Apr 2010 09:20:53 +0200
Subject: [PATCH] vga16fb, drm/nouveau: vga16fb->nouveau handoff
let vga16fb claim 0xA0000+0x10000 region as its aperture;
nouveau does not use it, so we need to lie to kick vga16fb,
but only if it's driving the primary card (by checking
IORESOURCE_ROM_SHADOW resource flag)
Signed-off-by: Marcin Slusarz <marcin....@gmail.com>
---
drivers/gpu/drm/nouveau/nouveau_state.c | 11 ++++++++++-
drivers/video/vga16fb.c | 26 +++++++++++++++++++-------
2 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c
index cfa3239..a101861 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -636,10 +636,12 @@ static void nouveau_OF_copy_vbios_to_ramin(struct drm_device *dev)
#endif
}
+#define VGA_FB_PHYS 0xA0000
+#define VGA_FB_PHYS_LEN 65536
static struct apertures_struct *nouveau_get_apertures(struct drm_device *dev)
{
struct pci_dev *pdev = dev->pdev;
- struct apertures_struct *aper = alloc_apertures(3);
+ struct apertures_struct *aper = alloc_apertures(4);
if (!aper)
return NULL;
@@ -658,6 +660,13 @@ static struct apertures_struct *nouveau_get_apertures(struct drm_device *dev)
aper->ranges[aper->count].size = pci_resource_len(pdev, 3);
aper->count++;
}
+
+ /* if it's the primary card, claim 0xa0000 as ours to kick vga16fb */
+ if (pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW) {
+ aper->ranges[aper->count].base = VGA_FB_PHYS;
+ aper->ranges[aper->count].size = VGA_FB_PHYS_LEN;
+ aper->count++;
+ }
return aper;
}
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 76d8dae..45d7a3d 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1264,10 +1264,19 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
vga_imageblit_color(info, image);
}
+static void vga16fb_destroy(struct fb_info *info)
+{
+ iounmap(info->screen_base);
+ fb_dealloc_cmap(&info->cmap);
+ /* XXX unshare VGA regions */
+ framebuffer_release(info);
+}
+
static struct fb_ops vga16fb_ops = {
.owner = THIS_MODULE,
.fb_open = vga16fb_open,
.fb_release = vga16fb_release,
+ .fb_destroy = vga16fb_destroy,
.fb_check_var = vga16fb_check_var,
.fb_set_par = vga16fb_set_par,
.fb_setcolreg = vga16fb_setcolreg,
@@ -1307,6 +1316,11 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
ret = -ENOMEM;
goto err_fb_alloc;
}
+ info->apertures = alloc_apertures(1);
+ if (!info->apertures) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, 0);
@@ -1336,7 +1350,7 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
info->fix = vga16fb_fix;
/* supports rectangles with widths of multiples of 8 */
info->pixmap.blit_x = 1 << 7 | 1 << 15 | 1 << 23 | 1 << 31;
- info->flags = FBINFO_FLAG_DEFAULT |
+ info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE |
FBINFO_HWACCEL_YPAN;
i = (info->var.bits_per_pixel == 8) ? 256 : 16;
@@ -1355,6 +1369,9 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
vga16fb_update_fix(info);
+ info->apertures->ranges[0].base = VGA_FB_PHYS;
+ info->apertures->ranges[0].size = VGA_FB_PHYS_LEN;
+
if (register_framebuffer(info) < 0) {
printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
ret = -EINVAL;
@@ -1381,13 +1398,8 @@ static int vga16fb_remove(struct platform_device *dev)
{
struct fb_info *info = platform_get_drvdata(dev);
- if (info) {
+ if (info)
unregister_framebuffer(info);
- iounmap(info->screen_base);
- fb_dealloc_cmap(&info->cmap);
- /* XXX unshare VGA regions */
- framebuffer_release(info);
- }
return 0;
}
--
1.7.0.4
Marcin Slusarz
More generic approach below - it should work for all drm drivers.
Unfortunately vga16fb handoff has 2 other issues:
- It can be compiled as module, so it can be loaded after KMS driver (and
nothing prevents it right now)
- vga16fb registration error path is iounmapping memory which was not
ioremapped.
I'm going to fix it soon.
---
From: Marcin Slusarz <marcin....@gmail.com>
Subject: [PATCH] vga16fb, drm: vga16fb->drm handoff
let vga16fb claim 0xA0000+0x10000 region as its aperture;
drm drivers don't use it, so we have to detect it and kick
vga16fb manually - but only if drm is driving the primary card
(by checking IORESOURCE_ROM_SHADOW resource flag)
Signed-off-by: Marcin Slusarz <marcin....@gmail.com>
---
drivers/gpu/drm/i915/intel_fb.c | 1 +
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 1 +
drivers/gpu/drm/nouveau/nouveau_state.c | 3 ++-
drivers/gpu/drm/radeon/radeon_fb.c | 1 +
drivers/gpu/drm/vmwgfx/vmwgfx_fb.c | 1 +
drivers/video/fbmem.c | 19 ++++++++++++++++---
drivers/video/vga16fb.c | 26 +++++++++++++++++++-------
include/linux/fb.h | 4 +++-
8 files changed, 44 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 6dbc277..8e403be 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -201,6 +201,7 @@ static int intelfb_create(struct drm_device *dev, uint32_t fb_width,
info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 2);
else
info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 0);
+ info->pcidev = dev->pdev;
info->fix.smem_start = dev->mode_config.fb_base + obj_priv->gtt_offset;
info->fix.smem_len = size;
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 660746b..2e0d840 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -285,6 +285,7 @@ nouveau_fbcon_create(struct drm_device *dev, uint32_t fb_width,
ret = -ENOMEM;
goto out_unref;
}
+ info->pcidev = pdev;
info->pixmap.size = 64*1024;
info->pixmap.buf_align = 8;
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c
index 3efc339..4916cf2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -670,7 +670,8 @@ static int nouveau_remove_conflicting_drivers(struct drm_device *dev)
if (!dev_priv->apertures)
return -ENOMEM;
- remove_conflicting_framebuffers(dev_priv->apertures, "nouveaufb");
+ remove_conflicting_framebuffers(dev_priv->apertures, "nouveaufb",
+ dev->pdev);
return 0;
}
diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index df06cdf..17de92a 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -271,6 +271,7 @@ int radeonfb_create(struct drm_device *dev,
}
info->apertures->ranges[0].base = rdev->ddev->mode_config.fb_base;
info->apertures->ranges[0].size = rdev->mc.real_vram_size;
+ info->pcidev = dev->pdev;
info->fix.mmio_start = 0;
info->fix.mmio_len = 0;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
index 0248c6b..eda1336 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fb.c
@@ -566,6 +566,7 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
}
info->apertures->ranges[0].base = vmw_priv->vram_start;
info->apertures->ranges[0].size = vmw_priv->vram_size;
+ info->pcidev = vmw_priv->dev->pdev;
/*
* Dirty & Deferred IO
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 7cfcd71..4628a59 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -32,6 +32,7 @@
#include <linux/device.h>
#include <linux/efi.h>
#include <linux/fb.h>
+#include <linux/pci.h>
#include <asm/fb.h>
@@ -1500,19 +1501,30 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
return false;
}
-void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name)
+#define VGA_FB_PHYS 0xA0000
+void remove_conflicting_framebuffers(struct apertures_struct *a,
+ const char *name, struct pci_dev *pcidev)
{
int i;
+ bool primary = false;
+
+ if (pcidev)
+ primary = pcidev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
/* check all firmware fbs and kick off if the base addr overlaps */
for (i = 0 ; i < FB_MAX; i++) {
+ struct apertures_struct *gen_aper;
if (!registered_fb[i])
continue;
if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
continue;
+
+ gen_aper = registered_fb[i]->apertures;
+ if (fb_do_apertures_overlap(gen_aper, a) ||
+ (primary && gen_aper && gen_aper->count &&
+ gen_aper->ranges[0].base == VGA_FB_PHYS)) {
- if (fb_do_apertures_overlap(registered_fb[i]->apertures, a)) {
printk(KERN_ERR "fb: conflicting fb hw usage "
"%s vs %s - removing generic driver\n",
name, registered_fb[i]->fix.id);
@@ -1545,7 +1557,8 @@ register_framebuffer(struct fb_info *fb_info)
if (fb_check_foreignness(fb_info))
return -ENOSYS;
- remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id);
+ remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id,
+ fb_info->pcidev);
num_registered_fb++;
for (i = 0 ; i < FB_MAX; i++)
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index bf638a4..149c47a 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1263,10 +1263,19 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
vga_imageblit_color(info, image);
}
+static void vga16fb_destroy(struct fb_info *info)
+{
+ iounmap(info->screen_base);
+ fb_dealloc_cmap(&info->cmap);
+ /* XXX unshare VGA regions */
+ framebuffer_release(info);
+}
+
static struct fb_ops vga16fb_ops = {
.owner = THIS_MODULE,
.fb_open = vga16fb_open,
.fb_release = vga16fb_release,
+ .fb_destroy = vga16fb_destroy,
.fb_check_var = vga16fb_check_var,
.fb_set_par = vga16fb_set_par,
.fb_setcolreg = vga16fb_setcolreg,
@@ -1306,6 +1315,11 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
ret = -ENOMEM;
goto err_fb_alloc;
}
+ info->apertures = alloc_apertures(1);
+ if (!info->apertures) {
+ ret = -ENOMEM;
+ goto err_ioremap;
+ }
/* XXX share VGA_FB_PHYS and I/O region with vgacon and others */
info->screen_base = (void __iomem *)VGA_MAP_MEM(VGA_FB_PHYS, 0);
@@ -1335,7 +1349,7 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
info->fix = vga16fb_fix;
/* supports rectangles with widths of multiples of 8 */
info->pixmap.blit_x = 1 << 7 | 1 << 15 | 1 << 23 | 1 << 31;
- info->flags = FBINFO_FLAG_DEFAULT |
+ info->flags = FBINFO_FLAG_DEFAULT | FBINFO_MISC_FIRMWARE |
FBINFO_HWACCEL_YPAN;
i = (info->var.bits_per_pixel == 8) ? 256 : 16;
@@ -1354,6 +1368,9 @@ static int __devinit vga16fb_probe(struct platform_device *dev)
vga16fb_update_fix(info);
+ info->apertures->ranges[0].base = VGA_FB_PHYS;
+ info->apertures->ranges[0].size = VGA_FB_PHYS_LEN;
+
if (register_framebuffer(info) < 0) {
printk(KERN_ERR "vga16fb: unable to register framebuffer\n");
ret = -EINVAL;
@@ -1380,13 +1397,8 @@ static int vga16fb_remove(struct platform_device *dev)
{
struct fb_info *info = platform_get_drvdata(dev);
- if (info) {
+ if (info)
unregister_framebuffer(info);
- iounmap(info->screen_base);
- fb_dealloc_cmap(&info->cmap);
- /* XXX unshare VGA regions */
- framebuffer_release(info);
- }
return 0;
}
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f88e254..4b48e2f 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -870,6 +870,7 @@ struct fb_info {
resource_size_t size;
} ranges[0];
} *apertures;
+ struct pci_dev *pcidev;
};
static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
@@ -971,7 +972,8 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
/* drivers/video/fbmem.c */
extern int register_framebuffer(struct fb_info *fb_info);
extern int unregister_framebuffer(struct fb_info *fb_info);
-extern void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name);
+extern void remove_conflicting_framebuffers(struct apertures_struct *a,
+ const char *name, struct pci_dev *pcidev);
extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
extern int fb_show_logo(struct fb_info *fb_info, int rotate);
extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);
James Simmons
> More generic approach below - it should work for all drm drivers.
> Unfortunately vga16fb handoff has 2 other issues:
> - It can be compiled as module, so it can be loaded after KMS driver (and
> nothing prevents it right now)
> - vga16fb registration error path is iounmapping memory which was not
> ioremapped.
> I'm going to fix it soon.
Nak. See comments below.
No pci junk in struct fb_info. It needs to be bus independent.
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 7cfcd71..4628a59 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -32,6 +32,7 @@
> #include <linux/device.h>
> #include <linux/efi.h>
> #include <linux/fb.h>
> +#include <linux/pci.h>
Please remove pci header.
> #include <asm/fb.h>
>
> @@ -1500,19 +1501,30 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
> return false;
> }
>
> -void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name)
> +#define VGA_FB_PHYS 0xA0000
> +void remove_conflicting_framebuffers(struct apertures_struct *a,
> + const char *name, struct pci_dev *pcidev)
> {
> int i;
> + bool primary = false;
> +
> + if (pcidev)
> + primary = pcidev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
Not portable. Please use fb_is_primary_device(info) instead. It will also
make your code cleaner.
Marcin Slusarz
>
> > More generic approach below - it should work for all drm drivers.
> > Unfortunately vga16fb handoff has 2 other issues:
> > - It can be compiled as module, so it can be loaded after KMS driver (and
> > nothing prevents it right now)
> > - vga16fb registration error path is iounmapping memory which was not
> > ioremapped.
> > I'm going to fix it soon.
>
> Nak. See comments below.
Thanks for a review.
It simplified the code, but I still need to figure out whether the card is primary when going
the early kick path (when I don't have struct fb_info yet) - the check is now in nouveau.
---
From: Marcin Slusarz <marcin....@gmail.com>
Subject: [PATCH v2] vga16fb, drm: vga16fb->drm handoff
let vga16fb claim 0xA0000+0x10000 region as its aperture;
drm drivers don't use it, so we have to detect it and kick
vga16fb manually - but only if drm is driving the primary card
Signed-off-by: Marcin Slusarz <marcin....@gmail.com>
---
drivers/gpu/drm/nouveau/nouveau_state.c | 7 ++++++-
drivers/video/fbmem.c | 14 +++++++++++---
drivers/video/vga16fb.c | 26 +++++++++++++++++++-------
include/linux/fb.h | 3 ++-
4 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c b/drivers/gpu/drm/nouveau/nouveau_state.c
index 3efc339..4c193ed 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -669,8 +669,13 @@ static int nouveau_remove_conflicting_drivers(struct drm_device *dev)
dev_priv->apertures = nouveau_get_apertures(dev);
if (!dev_priv->apertures)
return -ENOMEM;
+ bool primary = false;
+
+#ifdef CONFIG_X86
+ primary = dev->pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW;
+#endif
- remove_conflicting_framebuffers(dev_priv->apertures, "nouveaufb");
+ remove_conflicting_framebuffers(dev_priv->apertures, "nouveaufb", primary);
return 0;
}
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 7cfcd71..6deb57c 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1500,19 +1500,26 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
return false;
}
-void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name)
+#define VGA_FB_PHYS 0xA0000
+void remove_conflicting_framebuffers(struct apertures_struct *a,
+ const char *name, bool primary)
{
int i;
/* check all firmware fbs and kick off if the base addr overlaps */
for (i = 0 ; i < FB_MAX; i++) {
+ struct apertures_struct *gen_aper;
if (!registered_fb[i])
continue;
if (!(registered_fb[i]->flags & FBINFO_MISC_FIRMWARE))
continue;
+
+ gen_aper = registered_fb[i]->apertures;
+ if (fb_do_apertures_overlap(gen_aper, a) ||
+ (primary && gen_aper && gen_aper->count &&
+ gen_aper->ranges[0].base == VGA_FB_PHYS)) {
- if (fb_do_apertures_overlap(registered_fb[i]->apertures, a)) {
printk(KERN_ERR "fb: conflicting fb hw usage "
"%s vs %s - removing generic driver\n",
name, registered_fb[i]->fix.id);
@@ -1545,7 +1552,8 @@ register_framebuffer(struct fb_info *fb_info)
if (fb_check_foreignness(fb_info))
return -ENOSYS;
- remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id);
+ remove_conflicting_framebuffers(fb_info->apertures, fb_info->fix.id,
+ fb_is_primary_device(fb_info));
index f88e254..1296af4 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -971,7 +971,8 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
/* drivers/video/fbmem.c */
extern int register_framebuffer(struct fb_info *fb_info);
extern int unregister_framebuffer(struct fb_info *fb_info);
-extern void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name);
+extern void remove_conflicting_framebuffers(struct apertures_struct *a,
+ const char *name, bool primary);
extern int fb_prepare_logo(struct fb_info *fb_info, int rotate);
extern int fb_show_logo(struct fb_info *fb_info, int rotate);
extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);
--
1.7.0.4
--