Skip to contentSkip to conversation

fbmem: fix aperture overlapping check

PATCH In review [email protected] 4 participants 19 messages Apr 10, 2010 – Apr 20, 2010

Subsystems (7)
DRM drivers DRM misc Intel i915 Radeon and amdgpu DRM drivers +3 more subsystems
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/3 fbmem: fix aperture overlapping check

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.

Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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
Signature · 1 line
1.7.0.4
author
#1 lore raw

PATCH 2/3 fbdev: allow passing more than one aperture for handoff

It simplifies nouveau code by removal of detection which region to pass to kick vesafb/efifb.

Signed-off-by Marcin Slusarz <[email protected]>
Cc Eric Anholt <[email protected]>
Cc Ben Skeggs <[email protected]>
Cc Thomas Hellstrom <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
---
 drivers/gpu/drm/i915/intel_fb.c         |   11 +++-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |   84 +++++++++---------------------
 drivers/gpu/drm/radeon/radeon_fb.c      |    9 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c      |   10 +++-
 drivers/video/efifb.c                   |   11 +++-
 drivers/video/fbmem.c                   |   20 +++++++-
 drivers/video/fbsysfs.c                 |    1 +
 drivers/video/offb.c                    |   28 ++++++----
 drivers/video/vesafb.c                  |   11 +++-
 include/linux/fb.h                      |   16 +++++-
 10 files changed, 114 insertions(+), 87 deletions(-)
drivers/gpu/drm/i915/intel_fb.c +8 -3
index 8cd791d..a9192ab 100644
@@ -192,11 +192,16 @@ static int intelfb_create(struct drm_device *dev, uint32_t fb_width,
 
 
 	/* setup aperture base/size for vesafb takeover */
-	info->aperture_base = dev->mode_config.fb_base;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto out_unpin;
+	}
+	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	if (IS_I9XX(dev))
-		info->aperture_size = pci_resource_len(dev->pdev, 2);
+		info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 2);
 	else
-		info->aperture_size = pci_resource_len(dev->pdev, 0);
+		info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 0);
 
 	info->fix.smem_start = dev->mode_config.fb_base + obj_priv->gtt_offset;
 	info->fix.smem_len = size;
drivers/gpu/drm/nouveau/nouveau_fbcon.c +25 -57

102 lines not on this page · raw message

drivers/gpu/drm/radeon/radeon_fb.c +7 -2
index 8fccbf2..2b57cb4 100644
@@ -263,8 +263,13 @@ int radeonfb_create(struct drm_device *dev,
 	drm_fb_helper_fill_var(info, fb, fb_width, fb_height);
 
 	/* setup aperture base/size for vesafb takeover */
-	info->aperture_base = rdev->ddev->mode_config.fb_base;
-	info->aperture_size = rdev->mc.real_vram_size;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto out_unref;
+	}
+	info->apertures->ranges[0].base = rdev->ddev->mode_config.fb_base;
+	info->apertures->ranges[0].size = rdev->mc.real_vram_size;
 
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +8 -2
index a933670..0248c6b 100644
@@ -559,8 +559,13 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
 	info->pixmap.scan_align = 1;
 #endif
 
-	info->aperture_base = vmw_priv->vram_start;
-	info->aperture_size = vmw_priv->vram_size;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto err_aper;
+	}
+	info->apertures->ranges[0].base = vmw_priv->vram_start;
+	info->apertures->ranges[0].size = vmw_priv->vram_size;
 
 	/*
 	 * Dirty & Deferred IO

@@ -580,6 +585,7 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
 
 err_defio:
 	fb_deferred_io_cleanup(info);
+err_aper:
 	ttm_bo_kunmap(&par->map);
 err_unref:
 	ttm_bo_unref((struct ttm_buffer_object **)&par->vmw_bo);
drivers/video/efifb.c +8 -3
index 581d2db..3b98656 100644
@@ -165,7 +165,7 @@ static void efifb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -289,8 +289,13 @@ static int __devinit efifb_probe(struct platform_device *dev)
 	info->pseudo_palette = info->par;
 	info->par = NULL;
 
-	info->aperture_base = efifb_fix.smem_start;
-	info->aperture_size = size_remap;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		err = -ENOMEM;
+		goto err_release_fb;
+	}
+	info->apertures->ranges[0].base = efifb_fix.smem_start;
+	info->apertures->ranges[0].size = size_remap;
 
 	info->screen_base = ioremap(efifb_fix.smem_start, efifb_fix.smem_len);
 	if (!info->screen_base) {
drivers/video/fbmem.c +18 -2
index 41f2e5e..8824d2c 100644
@@ -1485,8 +1485,24 @@ static int fb_check_foreignness(struct fb_info *fi)
 
 static bool fb_do_apertures_overlap(struct fb_info *gen, struct fb_info *hw)
 {
-	return ranges_overlap(gen->aperture_base, gen->aperture_size,
-				hw->aperture_base, hw->aperture_size);
+	int i, j;
+	struct apertures_struct *hwa = hw->apertures;
+	struct apertures_struct *gena = gen->apertures;
+	if (!hwa || !gena)
+		return false;
+
+	for (i = 0; i < hwa->count; ++i) {
+		struct aperture *h = &hwa->ranges[i];
+		for (j = 0; j < gena->count; ++j) {
+			struct aperture *g = &gena->ranges[j];
+			printk(KERN_DEBUG "checking generic (%llx %llx) vs hw (%llx %llx)\n",
+				g->base, g->size, h->base, h->size);
+			if (ranges_overlap(h->base, h->size, g->base, g->size))
+				return true;
+		}
+	}
+
+	return false;
 }
 /**
  *	register_framebuffer - registers a frame buffer device
drivers/video/fbsysfs.c +1 -0
index d4a2c11..fd5519b 100644
@@ -79,6 +79,7 @@ EXPORT_SYMBOL(framebuffer_alloc);
  */
 void framebuffer_release(struct fb_info *info)
 {
+	kfree(info->apertures);
 	kfree(info);
 }
 EXPORT_SYMBOL(framebuffer_release);
drivers/video/offb.c +17 -11
index b043ac8..3d202b2 100644
@@ -286,7 +286,7 @@ static void offb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -492,8 +492,11 @@ static void __init offb_init_fb(const char *name, const char *full_name,
 	var->vmode = FB_VMODE_NONINTERLACED;
 
 	/* set offb aperture size for generic probing */
-	info->aperture_base = address;
-	info->aperture_size = fix->smem_len;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures)
+		goto out_aper;
+	info->apertures->ranges[0].base = address;
+	info->apertures->ranges[0].size = fix->smem_len;
 
 	info->fbops = &offb_ops;
 	info->screen_base = ioremap(address, fix->smem_len);

@@ -502,17 +505,20 @@ static void __init offb_init_fb(const char *name, const char *full_name,
 
 	fb_alloc_cmap(&info->cmap, 256, 0);
 
-	if (register_framebuffer(info) < 0) {
-		iounmap(par->cmap_adr);
-		par->cmap_adr = NULL;
-		iounmap(info->screen_base);
-		framebuffer_release(info);
-		release_mem_region(res_start, res_size);
-		return;
-	}
+	if (register_framebuffer(info) < 0)
+		goto out_err;
 
 	printk(KERN_INFO "fb%d: Open Firmware frame buffer device on %s\n",
 	       info->node, full_name);
+	return;
+
+out_err:
+	iounmap(info->screen_base);
+out_aper:
+	iounmap(par->cmap_adr);
+	par->cmap_adr = NULL;
+	framebuffer_release(info);
+	release_mem_region(res_start, res_size);
 }
drivers/video/vesafb.c +8 -3
index ef4128c..5de9ee1 100644
@@ -178,7 +178,7 @@ static void vesafb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -296,8 +296,13 @@ static int __devinit vesafb_probe(struct platform_device *dev)
 	info->par = NULL;
 
 	/* set vesafb aperture size for generic probing */
-	info->aperture_base = screen_info.lfb_base;
-	info->aperture_size = size_total;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		err = -ENOMEM;
+		goto err;
+	}
+	info->apertures->ranges[0].base = screen_info.lfb_base;
+	info->apertures->ranges[0].size = size_total;
 
 	info->screen_base = ioremap(vesafb_fix.smem_start, vesafb_fix.smem_len);
 	if (!info->screen_base) {
include/linux/fb.h +14 -2
index c10163b..9605b81 100644
@@ -862,10 +862,22 @@ struct fb_info {
 	/* we need the PCI or similiar aperture base/size not
 	   smem_start/size as smem_start may just be an object
 	   allocated inside the aperture so may not actually overlap */
-	resource_size_t aperture_base;
-	resource_size_t aperture_size;
+	struct apertures_struct {
+		unsigned int count;
+		struct aperture {
+			resource_size_t base;
+			resource_size_t size;
+		} ranges[0];
+	} *apertures;
 };
 
+static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
+	struct apertures_struct *a = kzalloc(sizeof(struct apertures_struct)
+			+ max_num * sizeof(struct aperture), GFP_KERNEL);
+	a->count = max_num;
+	return a;
+}
+
 #ifdef MODULE
 #define FBINFO_DEFAULT	FBINFO_MODULE
 #else
Signature · 1 line
1.7.0.4
author
#2 lore raw

PATCH 2/3 fbdev: allow passing more than one aperture for handoff

In reply to [email protected]

It simplifies nouveau code by removal of detection which region to pass to kick vesafb/efifb.

Signed-off-by Marcin Slusarz <[email protected]>
Cc Eric Anholt <[email protected]>
Cc Ben Skeggs <[email protected]>
Cc Thomas Hellstrom <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
---
 drivers/gpu/drm/i915/intel_fb.c         |   11 +++-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |   84 +++++++++---------------------
 drivers/gpu/drm/radeon/radeon_fb.c      |    9 +++-
 drivers/gpu/drm/vmwgfx/vmwgfx_fb.c      |   10 +++-
 drivers/video/efifb.c                   |   11 +++-
 drivers/video/fbmem.c                   |   20 +++++++-
 drivers/video/fbsysfs.c                 |    1 +
 drivers/video/offb.c                    |   28 ++++++----
 drivers/video/vesafb.c                  |   11 +++-
 include/linux/fb.h                      |   16 +++++-
 10 files changed, 114 insertions(+), 87 deletions(-)
drivers/gpu/drm/i915/intel_fb.c +8 -3
index 8cd791d..a9192ab 100644
@@ -192,11 +192,16 @@ static int intelfb_create(struct drm_device *dev, uint32_t fb_width,
 
 
 	/* setup aperture base/size for vesafb takeover */
-	info->aperture_base = dev->mode_config.fb_base;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto out_unpin;
+	}
+	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	if (IS_I9XX(dev))
-		info->aperture_size = pci_resource_len(dev->pdev, 2);
+		info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 2);
 	else
-		info->aperture_size = pci_resource_len(dev->pdev, 0);
+		info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 0);
 
 	info->fix.smem_start = dev->mode_config.fb_base + obj_priv->gtt_offset;
 	info->fix.smem_len = size;
drivers/gpu/drm/nouveau/nouveau_fbcon.c +25 -59

104 lines not on this page · raw message

drivers/gpu/drm/radeon/radeon_fb.c +7 -2
index 8fccbf2..2b57cb4 100644
@@ -263,8 +263,13 @@ int radeonfb_create(struct drm_device *dev,
 	drm_fb_helper_fill_var(info, fb, fb_width, fb_height);
 
 	/* setup aperture base/size for vesafb takeover */
-	info->aperture_base = rdev->ddev->mode_config.fb_base;
-	info->aperture_size = rdev->mc.real_vram_size;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto out_unref;
+	}
+	info->apertures->ranges[0].base = rdev->ddev->mode_config.fb_base;
+	info->apertures->ranges[0].size = rdev->mc.real_vram_size;
 
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +8 -2
index a933670..0248c6b 100644
@@ -559,8 +559,13 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
 	info->pixmap.scan_align = 1;
 #endif
 
-	info->aperture_base = vmw_priv->vram_start;
-	info->aperture_size = vmw_priv->vram_size;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto err_aper;
+	}
+	info->apertures->ranges[0].base = vmw_priv->vram_start;
+	info->apertures->ranges[0].size = vmw_priv->vram_size;
 
 	/*
 	 * Dirty & Deferred IO

@@ -580,6 +585,7 @@ int vmw_fb_init(struct vmw_private *vmw_priv)
 
 err_defio:
 	fb_deferred_io_cleanup(info);
+err_aper:
 	ttm_bo_kunmap(&par->map);
 err_unref:
 	ttm_bo_unref((struct ttm_buffer_object **)&par->vmw_bo);
drivers/video/efifb.c +8 -3
index 581d2db..3b98656 100644
@@ -165,7 +165,7 @@ static void efifb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -289,8 +289,13 @@ static int __devinit efifb_probe(struct platform_device *dev)
 	info->pseudo_palette = info->par;
 	info->par = NULL;
 
-	info->aperture_base = efifb_fix.smem_start;
-	info->aperture_size = size_remap;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		err = -ENOMEM;
+		goto err_release_fb;
+	}
+	info->apertures->ranges[0].base = efifb_fix.smem_start;
+	info->apertures->ranges[0].size = size_remap;
 
 	info->screen_base = ioremap(efifb_fix.smem_start, efifb_fix.smem_len);
 	if (!info->screen_base) {
drivers/video/fbmem.c +18 -2
index 41f2e5e..8824d2c 100644
@@ -1485,8 +1485,24 @@ static int fb_check_foreignness(struct fb_info *fi)
 
 static bool fb_do_apertures_overlap(struct fb_info *gen, struct fb_info *hw)
 {
-	return ranges_overlap(gen->aperture_base, gen->aperture_size,
-				hw->aperture_base, hw->aperture_size);
+	int i, j;
+	struct apertures_struct *hwa = hw->apertures;
+	struct apertures_struct *gena = gen->apertures;
+	if (!hwa || !gena)
+		return false;
+
+	for (i = 0; i < hwa->count; ++i) {
+		struct aperture *h = &hwa->ranges[i];
+		for (j = 0; j < gena->count; ++j) {
+			struct aperture *g = &gena->ranges[j];
+			printk(KERN_DEBUG "checking generic (%llx %llx) vs hw (%llx %llx)\n",
+				g->base, g->size, h->base, h->size);
+			if (ranges_overlap(h->base, h->size, g->base, g->size))
+				return true;
+		}
+	}
+
+	return false;
 }
 /**
  *	register_framebuffer - registers a frame buffer device
drivers/video/fbsysfs.c +1 -0
index d4a2c11..fd5519b 100644
@@ -79,6 +79,7 @@ EXPORT_SYMBOL(framebuffer_alloc);
  */
 void framebuffer_release(struct fb_info *info)
 {
+	kfree(info->apertures);
 	kfree(info);
 }
 EXPORT_SYMBOL(framebuffer_release);
drivers/video/offb.c +17 -11
index b043ac8..3d202b2 100644
@@ -286,7 +286,7 @@ static void offb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -492,8 +492,11 @@ static void __init offb_init_fb(const char *name, const char *full_name,
 	var->vmode = FB_VMODE_NONINTERLACED;
 
 	/* set offb aperture size for generic probing */
-	info->aperture_base = address;
-	info->aperture_size = fix->smem_len;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures)
+		goto out_aper;
+	info->apertures->ranges[0].base = address;
+	info->apertures->ranges[0].size = fix->smem_len;
 
 	info->fbops = &offb_ops;
 	info->screen_base = ioremap(address, fix->smem_len);

@@ -502,17 +505,20 @@ static void __init offb_init_fb(const char *name, const char *full_name,
 
 	fb_alloc_cmap(&info->cmap, 256, 0);
 
-	if (register_framebuffer(info) < 0) {
-		iounmap(par->cmap_adr);
-		par->cmap_adr = NULL;
-		iounmap(info->screen_base);
-		framebuffer_release(info);
-		release_mem_region(res_start, res_size);
-		return;
-	}
+	if (register_framebuffer(info) < 0)
+		goto out_err;
 
 	printk(KERN_INFO "fb%d: Open Firmware frame buffer device on %s\n",
 	       info->node, full_name);
+	return;
+
+out_err:
+	iounmap(info->screen_base);
+out_aper:
+	iounmap(par->cmap_adr);
+	par->cmap_adr = NULL;
+	framebuffer_release(info);
+	release_mem_region(res_start, res_size);
 }
drivers/video/vesafb.c +8 -3
index ef4128c..5de9ee1 100644
@@ -178,7 +178,7 @@ static void vesafb_destroy(struct fb_info *info)
 {
 	if (info->screen_base)
 		iounmap(info->screen_base);
-	release_mem_region(info->aperture_base, info->aperture_size);
+	release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size);
 	framebuffer_release(info);
 }
 

@@ -296,8 +296,13 @@ static int __devinit vesafb_probe(struct platform_device *dev)
 	info->par = NULL;
 
 	/* set vesafb aperture size for generic probing */
-	info->aperture_base = screen_info.lfb_base;
-	info->aperture_size = size_total;
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		err = -ENOMEM;
+		goto err;
+	}
+	info->apertures->ranges[0].base = screen_info.lfb_base;
+	info->apertures->ranges[0].size = size_total;
 
 	info->screen_base = ioremap(vesafb_fix.smem_start, vesafb_fix.smem_len);
 	if (!info->screen_base) {
include/linux/fb.h +14 -2
index c10163b..9605b81 100644
@@ -862,10 +862,22 @@ struct fb_info {
 	/* we need the PCI or similiar aperture base/size not
 	   smem_start/size as smem_start may just be an object
 	   allocated inside the aperture so may not actually overlap */
-	resource_size_t aperture_base;
-	resource_size_t aperture_size;
+	struct apertures_struct {
+		unsigned int count;
+		struct aperture {
+			resource_size_t base;
+			resource_size_t size;
+		} ranges[0];
+	} *apertures;
 };
 
+static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
+	struct apertures_struct *a = kzalloc(sizeof(struct apertures_struct)
+			+ max_num * sizeof(struct aperture), GFP_KERNEL);
+	a->count = max_num;
+	return a;
+}
+
 #ifdef MODULE
 #define FBINFO_DEFAULT	FBINFO_MODULE
 #else
Signature · 1 line
1.7.0.4
author
#3 lore raw

PATCH 3/3 fbmem, drm/nouveau: kick firmware framebuffers as soon as possible

In reply to [email protected]

Currently vesafb/efifb/... is kicked when hardware driver is registering framebuffer. To do it hardware must be fully functional, so there's a short window between start of initialisation and framebuffer registration when two drivers touch the hardware. Unfortunately sometimes it breaks nouveau initialisation.

Fix it by kicking firmware driver(s) before we start touching the hardware.

Reported-by Didier Spaier <[email protected]>
Signed-off-by Marcin Slusarz <[email protected]>
Cc Ben Skeggs <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_drv.h   |    2 +
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |   19 +-------------
 drivers/gpu/drm/nouveau/nouveau_state.c |   43 +++++++++++++++++++++++++++++++
 drivers/video/fbmem.c                   |   43 ++++++++++++++++++-------------
 include/linux/fb.h                      |    1 +
 5 files changed, 72 insertions(+), 36 deletions(-)
drivers/gpu/drm/nouveau/nouveau_drv.h +2 -0
index cb16a1b..a1f61f7 100644
@@ -621,6 +621,8 @@ struct drm_nouveau_private {
 	struct {
 		struct dentry *channel_root;
 	} debugfs;
+
+	struct apertures_struct *apertures;
 };
 
 static inline struct drm_nouveau_private *
drivers/gpu/drm/nouveau/nouveau_fbcon.c +1 -18
index 206368b..5e4367b 100644
@@ -195,7 +195,6 @@ nouveau_fbcon_create(struct drm_device *dev, uint32_t fb_width,
 	struct drm_mode_fb_cmd mode_cmd;
 	struct pci_dev *pdev = dev->pdev;
 	struct device *device = &pdev->dev;
-	struct apertures_struct *aper;
 	int size, ret;
 
 	mode_cmd.width = surface_width;

@@ -282,28 +281,12 @@ nouveau_fbcon_create(struct drm_device *dev, uint32_t fb_width,
 	info->fix.mmio_len = pci_resource_len(pdev, 1);
 
 	/* Set aperture base/size for vesafb takeover */
-	aper = info->apertures = alloc_apertures(3);
+	info->apertures = dev_priv->apertures;
 	if (!info->apertures) {
 		ret = -ENOMEM;
 		goto out_unref;
 	}
 
-	aper->ranges[0].base = pci_resource_start(pdev, 1);
-	aper->ranges[0].size = pci_resource_len(pdev, 1);
-	aper->count = 1;
-
-	if (pci_resource_len(pdev, 2)) {
-		aper->ranges[aper->count].base = pci_resource_start(pdev, 2);
-		aper->ranges[aper->count].size = pci_resource_len(pdev, 2);
-		aper->count++;
-	}
-
-	if (pci_resource_len(pdev, 3)) {
-		aper->ranges[aper->count].base = pci_resource_start(pdev, 3);
-		aper->ranges[aper->count].size = pci_resource_len(pdev, 3);
-		aper->count++;
-	}
-
 	info->pixmap.size = 64*1024;
 	info->pixmap.buf_align = 8;
 	info->pixmap.access_align = 32;
drivers/gpu/drm/nouveau/nouveau_state.c +43 -0

55 lines not on this page · raw message

drivers/video/fbmem.c +25 -18

63 lines not on this page · raw message

include/linux/fb.h +1 -0
index 9605b81..9e854a7 100644
@@ -970,6 +970,7 @@ 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 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);
Signature · 1 line
1.7.0.4
author Tested-by Didier Spaier on v1 3/3
#4 lore raw

Re: [PATCH 3/3] fbmem, drm/nouveau: kick firmware framebuffers as soon as possible

Quoted 8 lines · On Sat, Apr 10, 2010 at 09:55:34PM +0200, [email protected] wrote:

Currently vesafb/efifb/... is kicked when hardware driver is registering framebuffer. To do it hardware must be fully functional, so there's a short window between start of initialisation and framebuffer registration when two drivers touch the hardware. Unfortunately sometimes it breaks nouveau initialisation.

Fix it by kicking firmware driver(s) before we start touching the hardware.

Quoted 7 lines
Reported-by Didier Spaier <[email protected]>
Signed-off-by Marcin Slusarz <[email protected]>
Cc Ben Skeggs <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>

---

With this little patch below:

Tested-by Didier Spaier <[email protected]>

---

drivers/video/fbmem.c +1 -0
index f5297a0..42ae782 100644
@@ -1524,6 +1524,7 @@ void remove_conflicting_framebuffers(struct apertures_struct *a, const char *nam
 		}
 	}
 }
+EXPORT_SYMBOL(remove_conflicting_framebuffers);
 
 /**
  *	register_framebuffer - registers a frame buffer device
author
#5 lore raw

PATCH 3/3 fbmem, drm/nouveau: kick firmware framebuffers as soon as possible

In reply to [email protected]

Currently vesafb/efifb/... is kicked when hardware driver is registering framebuffer. To do it hardware must be fully functional, so there's a short window between start of initialisation and framebuffer registration when two drivers touch the hardware. Unfortunately sometimes it breaks nouveau initialisation.

Fix it by kicking firmware driver(s) before we start touching the hardware.

Reported-by Didier Spaier <[email protected]>
Signed-off-by Marcin Slusarz <[email protected]>
Cc Ben Skeggs <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_drv.h   |    2 +
 drivers/gpu/drm/nouveau/nouveau_fbcon.c |   19 +-------------
 drivers/gpu/drm/nouveau/nouveau_state.c |   43 +++++++++++++++++++++++++++++++
 drivers/video/fbmem.c                   |   43 ++++++++++++++++++-------------
 include/linux/fb.h                      |    1 +
 5 files changed, 72 insertions(+), 36 deletions(-)
drivers/gpu/drm/nouveau/nouveau_drv.h +2 -0
index cb16a1b..a1f61f7 100644
@@ -621,6 +621,8 @@ struct drm_nouveau_private {
 	struct {
 		struct dentry *channel_root;
 	} debugfs;
+
+	struct apertures_struct *apertures;
 };
 
 static inline struct drm_nouveau_private *
drivers/gpu/drm/nouveau/nouveau_fbcon.c +1 -18
index 206368b..5e4367b 100644
@@ -195,7 +195,6 @@ nouveau_fbcon_create(struct drm_device *dev, uint32_t fb_width,
 	struct drm_mode_fb_cmd mode_cmd;
 	struct pci_dev *pdev = dev->pdev;
 	struct device *device = &pdev->dev;
-	struct apertures_struct *aper;
 	int size, ret;
 
 	mode_cmd.width = surface_width;

@@ -282,28 +281,12 @@ nouveau_fbcon_create(struct drm_device *dev, uint32_t fb_width,
 	info->fix.mmio_len = pci_resource_len(pdev, 1);
 
 	/* Set aperture base/size for vesafb takeover */
-	aper = info->apertures = alloc_apertures(3);
+	info->apertures = dev_priv->apertures;
 	if (!info->apertures) {
 		ret = -ENOMEM;
 		goto out_unref;
 	}
 
-	aper->ranges[0].base = pci_resource_start(pdev, 1);
-	aper->ranges[0].size = pci_resource_len(pdev, 1);
-	aper->count = 1;
-
-	if (pci_resource_len(pdev, 2)) {
-		aper->ranges[aper->count].base = pci_resource_start(pdev, 2);
-		aper->ranges[aper->count].size = pci_resource_len(pdev, 2);
-		aper->count++;
-	}
-
-	if (pci_resource_len(pdev, 3)) {
-		aper->ranges[aper->count].base = pci_resource_start(pdev, 3);
-		aper->ranges[aper->count].size = pci_resource_len(pdev, 3);
-		aper->count++;
-	}
-
 	info->pixmap.size = 64*1024;
 	info->pixmap.buf_align = 8;
 	info->pixmap.access_align = 32;
drivers/gpu/drm/nouveau/nouveau_state.c +43 -0

55 lines not on this page · raw message

drivers/video/fbmem.c +25 -18

63 lines not on this page · raw message

include/linux/fb.h +1 -0
index 9605b81..9e854a7 100644
@@ -970,6 +970,7 @@ 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 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);
Signature · 1 line
1.7.0.4
#6 lore raw
In reply to [email protected]
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

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.

Quoted 45 lines
Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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
author
#7 lore raw
Quoted 6 lines · Dave Airlie
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

Quoted 7 lines

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?

Quoted 2 lines

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?

Quoted 1 line

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).

Quoted 50 lines

Dave.

Quoted 45 lines
Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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
#8 lore raw
Quoted 9 lines · Marcin Slusarz
Quoted 8 lines · Dave Airlie
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

Quoted 7 lines
Quoted 5 lines

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.

Quoted 10 lines
Quoted 3 lines

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?

Quoted 1 line

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.

author
#9 lore raw
Quoted 18 lines · Dave Airlie
Quoted 9 lines · Marcin Slusarz
Quoted 8 lines · Dave Airlie
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

Quoted 7 lines
Quoted 5 lines

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?

Quoted 8 lines

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.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

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?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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

maintainer
#10 lore raw
Quoted 2 lines

Ok, thanks for explanation. I'll drop this patch and rebase the others.

Cool,

Quoted 8 lines
Quoted 3 lines

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.

maintainer
#11 lore raw
In reply to Marcin Slusarz
Quoted 2 lines

Ok, thanks for explanation. I'll drop this patch and rebase the others.

Cool,

Quoted 8 lines
Quoted 3 lines

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.

author
#12 lore raw

PATCH 1/1 vga16fb, drm/nouveau: vga16fb->nouveau handoff

In reply to Marcin Slusarz
Quoted 5 lines · Marcin Slusarz
Quoted 5 lines
Quoted 4 lines
Quoted 1 line

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).

Quoted 8 lines
Quoted 3 lines

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 <[email protected]>
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 <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_state.c |   11 ++++++++++-
 drivers/video/vga16fb.c                 |   26 +++++++++++++++++++-------
 2 files changed, 29 insertions(+), 8 deletions(-)
drivers/gpu/drm/nouveau/nouveau_state.c +10 -1
index cfa3239..a101861 100644
@@ -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;
 }
drivers/video/vga16fb.c +19 -7
index 76d8dae..45d7a3d 100644
@@ -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;
 }
Signature · 1 line
1.7.0.4
author
#13 lore raw

PATCH 1/1 vga16fb, drm: vga16fb->drm handoff

Quoted 27 lines · Marcin Slusarz
Quoted 5 lines · Marcin Slusarz
Quoted 5 lines
Quoted 4 lines
Quoted 1 line

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).

Quoted 8 lines
Quoted 3 lines

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 <[email protected]>
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)

Quoted 6 lines
Signed-off-by Marcin Slusarz <[email protected]>
---
 drivers/gpu/drm/nouveau/nouveau_state.c |   11 ++++++++++-
 drivers/video/vga16fb.c                 |   26 +++++++++++++++++++-------
 2 files changed, 29 insertions(+), 8 deletions(-)

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 <[email protected]>
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 <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/intel_fb.c +1 -0
index 6dbc277..8e403be 100644
@@ -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;
drivers/gpu/drm/nouveau/nouveau_fbcon.c +1 -0
index 660746b..2e0d840 100644
@@ -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;
drivers/gpu/drm/nouveau/nouveau_state.c +2 -1
index 3efc339..4916cf2 100644
@@ -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;
 }
drivers/gpu/drm/radeon/radeon_fb.c +1 -0
index df06cdf..17de92a 100644
@@ -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;
drivers/gpu/drm/vmwgfx/vmwgfx_fb.c +1 -0
index 0248c6b..eda1336 100644
@@ -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
drivers/video/fbmem.c +16 -3
index 7cfcd71..4628a59 100644
@@ -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++)
drivers/video/vga16fb.c +19 -7
index bf638a4..149c47a 100644
@@ -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;
 }
include/linux/fb.h +3 -1
index f88e254..4b48e2f 100644
@@ -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);
Signature · 1 line
1.7.0.4
#14 lore raw

Re: [PATCH] vga16fb, drm: vga16fb->drm handoff

Quoted 8 lines

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.

Quoted 26 lines

---
From: Marcin Slusarz <[email protected]>
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 <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/intel_fb.c +0 -0
index 6dbc277..8e403be 100644

drivers/gpu/drm/i915/intel_fb.c static int intelfb_create(struct drm_device *dev, uint32_t fb_width, · line 204

Quoted 4 lines
drivers/gpu/drm/i915/intel_fb.c +1 -0
@@ -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;

No pci junk in struct fb_info. It needs to be bus independent.

drivers/video/fbmem.c line 35

Quoted 9 lines
drivers/video/fbmem.c +1 -0
index 7cfcd71..4628a59 100644
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/fb.h>
+#include <linux/pci.h>

Please remove pci header.

Quoted 6 lines
drivers/video/fbmem.c +0 -0
 #include <asm/fb.h>
 

@@ -1500,19 +1501,30 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 	return false;
 }

drivers/video/fbmem.c static bool fb_do_apertures_overlap(struct apertures_struct *gena, · line 1512

Quoted 10 lines
drivers/video/fbmem.c +7 -1
-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.

author
#15 lore raw

Re: [PATCH] vga16fb, drm: vga16fb->drm handoff

Quoted 10 lines · James Simmons
Quoted 8 lines

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.

Quoted 51 lines
Quoted 26 lines

---
From: Marcin Slusarz <[email protected]>
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 <[email protected]>
---
 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(-)
drivers/gpu/drm/i915/intel_fb.c +0 -0
index 6dbc277..8e403be 100644

drivers/gpu/drm/i915/intel_fb.c static int intelfb_create(struct drm_device *dev, uint32_t fb_width, · line 204

Quoted 4 lines
drivers/gpu/drm/i915/intel_fb.c +1 -0
@@ -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;

No pci junk in struct fb_info. It needs to be bus independent.

drivers/video/fbmem.c line 35

Quoted 9 lines
drivers/video/fbmem.c +1 -0
index 7cfcd71..4628a59 100644
@@ -32,6 +32,7 @@
 #include <linux/device.h>
 #include <linux/efi.h>
 #include <linux/fb.h>
+#include <linux/pci.h>

Please remove pci header.

Quoted 6 lines
drivers/video/fbmem.c +0 -0
 #include <asm/fb.h>
 

@@ -1500,19 +1501,30 @@ static bool fb_do_apertures_overlap(struct apertures_struct *gena,
 	return false;
 }
Quoted 14 lines

drivers/video/fbmem.c static bool fb_do_apertures_overlap(struct apertures_struct *gena, · line 1512

Quoted 10 lines
drivers/video/fbmem.c +7 -1
-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.

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 <[email protected]>
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 <[email protected]>
---
 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(-)
drivers/gpu/drm/nouveau/nouveau_state.c +6 -1
index 3efc339..4c193ed 100644
@@ -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;
 }
drivers/video/fbmem.c +11 -3
index 7cfcd71..6deb57c 100644
@@ -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));
 
 	num_registered_fb++;
 	for (i = 0 ; i < FB_MAX; i++)
drivers/video/vga16fb.c +19 -7
index bf638a4..149c47a 100644
@@ -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;
 }
include/linux/fb.h +2 -1
index f88e254..1296af4 100644
@@ -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);
Signature · 1 line
1.7.0.4
author
#16 lore raw
In reply to Dave Airlie
Quoted 6 lines · Dave Airlie
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

Quoted 7 lines

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?

Quoted 2 lines

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?

Quoted 1 line

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).

Quoted 50 lines

Dave.

Quoted 45 lines
Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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
author
#17 lore raw
In reply to Dave Airlie
Quoted 6 lines · Dave Airlie
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

Quoted 7 lines

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?

Quoted 2 lines

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?

Quoted 1 line

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).

Quoted 50 lines

Dave.

Quoted 45 lines
Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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
#18 lore raw
In reply to [email protected]
Quoted 4 lines · On Sat, 2010-04-10 at 21:55 +0200, [email protected] 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.

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.

Quoted 45 lines
Signed-off-by Marcin Slusarz <[email protected]>
Cc Dave Airlie <[email protected]>
Cc Peter Jones <[email protected]>
Cc Andrew Morton <[email protected]>
---
 drivers/video/fbmem.c |   24 +++++++++++++++++-------
 1 files changed, 17 insertions(+), 7 deletions(-)
drivers/video/fbmem.c +17 -7
index a15b44e..41f2e5e 100644
@@ -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