Linux Framebuffer Layer development
 help / color / mirror / Atom feed
* [PATCH v3 7/9] uvesafb: Clean up MTRR code
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

The old code allowed very strange memory types.  Now it works like
all the other video drivers: ioremap_wc is used unconditionally,
and MTRRs are set if PAT is unavailable (unless MTRR is disabled
by a module parameter).

UC, WB, and WT support is gone.  If there are MTRR conflicts that prevent
addition of a WC MTRR, adding a non-conflicting MTRR is pointless; it's
better to just turn off MTRR support entirely.

As an added bonus, any MTRR added is freed on unload.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 Documentation/fb/uvesafb.txt | 16 ++++------
 drivers/video/uvesafb.c      | 70 +++++++++++---------------------------------
 include/video/uvesafb.h      |  1 +
 3 files changed, 23 insertions(+), 64 deletions(-)

diff --git a/Documentation/fb/uvesafb.txt b/Documentation/fb/uvesafb.txt
index eefdd91..f6362d8 100644
--- a/Documentation/fb/uvesafb.txt
+++ b/Documentation/fb/uvesafb.txt
@@ -81,17 +81,11 @@ pmipal  Use the protected mode interface for palette changes.
 
 mtrr:n  Setup memory type range registers for the framebuffer
         where n:
-              0 - disabled (equivalent to nomtrr) (default)
-              1 - uncachable
-              2 - write-back
-              3 - write-combining
-              4 - write-through
-
-        If you see the following in dmesg, choose the type that matches
-        the old one.  In this example, use "mtrr:2".
-...
-mtrr: type mismatch for e0000000,8000000 old: write-back new: write-combining
-...
+              0 - disabled (equivalent to nomtrr)
+              3 - write-combining (default)
+
+	Values other than 0 and 3 will result in a warning and will be
+	treated just like 3.
 
 nomtrr  Do not use memory type range registers.
 
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index d428445..8701f96 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -24,9 +24,6 @@
 #ifdef CONFIG_X86
 #include <video/vga.h>
 #endif
-#ifdef CONFIG_MTRR
-#include <asm/mtrr.h>
-#endif
 #include "edid.h"
 
 static struct cb_id uvesafb_cn_id = {
@@ -1540,67 +1537,30 @@ static void uvesafb_init_info(struct fb_info *info, struct vbe_mode_ib *mode)
 
 static void uvesafb_init_mtrr(struct fb_info *info)
 {
-#ifdef CONFIG_MTRR
+	struct uvesafb_par *par = info->par;
+
 	if (mtrr && !(info->fix.smem_start & (PAGE_SIZE - 1))) {
 		int temp_size = info->fix.smem_len;
-		unsigned int type = 0;
 
-		switch (mtrr) {
-		case 1:
-			type = MTRR_TYPE_UNCACHABLE;
-			break;
-		case 2:
-			type = MTRR_TYPE_WRBACK;
-			break;
-		case 3:
-			type = MTRR_TYPE_WRCOMB;
-			break;
-		case 4:
-			type = MTRR_TYPE_WRTHROUGH;
-			break;
-		default:
-			type = 0;
-			break;
-		}
+		int rc;
 
-		if (type) {
-			int rc;
+		/* Find the largest power-of-two */
+		temp_size = roundup_pow_of_two(temp_size);
 
-			/* Find the largest power-of-two */
-			temp_size = roundup_pow_of_two(temp_size);
+		/* Try and find a power of two to add */
+		do {
+			rc = arch_phys_wc_add(info->fix.smem_start, temp_size);
+			temp_size >>= 1;
+		} while (temp_size >= PAGE_SIZE && rc = -EINVAL);
 
-			/* Try and find a power of two to add */
-			do {
-				rc = mtrr_add(info->fix.smem_start,
-					      temp_size, type, 1);
-				temp_size >>= 1;
-			} while (temp_size >= PAGE_SIZE && rc = -EINVAL);
-		}
+		if (rc >= 0)
+			par->mtrr_handle = rc;
 	}
-#endif /* CONFIG_MTRR */
 }
 
 static void uvesafb_ioremap(struct fb_info *info)
 {
-#ifdef CONFIG_X86
-	switch (mtrr) {
-	case 1: /* uncachable */
-		info->screen_base = ioremap_nocache(info->fix.smem_start, info->fix.smem_len);
-		break;
-	case 2: /* write-back */
-		info->screen_base = ioremap_cache(info->fix.smem_start, info->fix.smem_len);
-		break;
-	case 3: /* write-combining */
-		info->screen_base = ioremap_wc(info->fix.smem_start, info->fix.smem_len);
-		break;
-	case 4: /* write-through */
-	default:
-		info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
-		break;
-	}
-#else
-	info->screen_base = ioremap(info->fix.smem_start, info->fix.smem_len);
-#endif /* CONFIG_X86 */
+	info->screen_base = ioremap_wc(info->fix.smem_start, info->fix.smem_len);
 }
 
 static ssize_t uvesafb_show_vbe_ver(struct device *dev,
@@ -1851,6 +1811,7 @@ static int uvesafb_remove(struct platform_device *dev)
 		unregister_framebuffer(info);
 		release_region(0x3c0, 32);
 		iounmap(info->screen_base);
+		arch_phys_wc_del(par->mtrr_handle);
 		release_mem_region(info->fix.smem_start, info->fix.smem_len);
 		fb_destroy_modedb(info->monspecs.modedb);
 		fb_dealloc_cmap(&info->cmap);
@@ -1930,6 +1891,9 @@ static int uvesafb_setup(char *options)
 		}
 	}
 
+	if (mtrr != 3 && mtrr != 1)
+		pr_warn("uvesafb: mtrr should be set to 0 or 3; %d is unsupported", mtrr);
+
 	return 0;
 }
 #endif /* !MODULE */
diff --git a/include/video/uvesafb.h b/include/video/uvesafb.h
index 1a91850..30f5362 100644
--- a/include/video/uvesafb.h
+++ b/include/video/uvesafb.h
@@ -134,6 +134,7 @@ struct uvesafb_par {
 
 	int mode_idx;
 	struct vbe_crtc_ib crtc;
+	int mtrr_handle;
 };
 
 #endif /* _UVESAFB_H */
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 6/9] radeon: Switch to arch_phys_wc_add and add a missing ..._del
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/gpu/drm/radeon/radeon_object.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index d3aface..15cd34b 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -321,8 +321,8 @@ void radeon_bo_force_delete(struct radeon_device *rdev)
 int radeon_bo_init(struct radeon_device *rdev)
 {
 	/* Add an MTRR for the VRAM */
-	rdev->mc.vram_mtrr = mtrr_add(rdev->mc.aper_base, rdev->mc.aper_size,
-			MTRR_TYPE_WRCOMB, 1);
+	rdev->mc.vram_mtrr = arch_phys_wc_add(rdev->mc.aper_base,
+					      rdev->mc.aper_size);
 	DRM_INFO("Detected VRAM RAM=%lluM, BAR=%lluM\n",
 		rdev->mc.mc_vram_size >> 20,
 		(unsigned long long)rdev->mc.aper_size >> 20);
@@ -334,6 +334,7 @@ int radeon_bo_init(struct radeon_device *rdev)
 void radeon_bo_fini(struct radeon_device *rdev)
 {
 	radeon_ttm_fini(rdev);
+	arch_phys_wc_del(rdev->mc.vram_mtrr);
 }
 
 void radeon_bo_list_add_object(struct radeon_bo_list *lobj,
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 5/9] i915: Use arch_phys_wc_{add,del}
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

i915 open-coded logic that was essentially equivalent to the new API.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

Changes from v1: Don't zero the mtrr handle after freeing it

 drivers/gpu/drm/i915/i915_dma.c | 42 ++++-------------------------------------
 1 file changed, 4 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4fa6beb..b0bb381 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -42,7 +42,6 @@
 #include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 #include <acpi/video.h>
-#include <asm/pat.h>
 
 #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS])
 
@@ -1393,29 +1392,6 @@ void i915_master_destroy(struct drm_device *dev, struct drm_master *master)
 	master->driver_priv = NULL;
 }
 
-static void
-i915_mtrr_setup(struct drm_i915_private *dev_priv, unsigned long base,
-		unsigned long size)
-{
-	dev_priv->mm.gtt_mtrr = -1;
-
-#if defined(CONFIG_X86_PAT)
-	if (cpu_has_pat)
-		return;
-#endif
-
-	/* Set up a WC MTRR for non-PAT systems.  This is more common than
-	 * one would think, because the kernel disables PAT on first
-	 * generation Core chips because WC PAT gets overridden by a UC
-	 * MTRR if present.  Even if a UC MTRR isn't present.
-	 */
-	dev_priv->mm.gtt_mtrr = mtrr_add(base, size, MTRR_TYPE_WRCOMB, 1);
-	if (dev_priv->mm.gtt_mtrr < 0) {
-		DRM_INFO("MTRR allocation failed.  Graphics "
-			 "performance may suffer.\n");
-	}
-}
-
 static void i915_kick_out_firmware_fb(struct drm_i915_private *dev_priv)
 {
 	struct apertures_struct *ap;
@@ -1552,8 +1528,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto out_rmmap;
 	}
 
-	i915_mtrr_setup(dev_priv, dev_priv->gtt.mappable_base,
-			aperture_size);
+	dev_priv->mm.gtt_mtrr = arch_phys_wc_add(dev_priv->gtt.mappable_base,
+						 aperture_size);
 
 	/* The i915 workqueue is primarily used for batched retirement of
 	 * requests (and thus managing bo) once the task has been completed
@@ -1656,12 +1632,7 @@ out_gem_unload:
 	intel_teardown_mchbar(dev);
 	destroy_workqueue(dev_priv->wq);
 out_mtrrfree:
-	if (dev_priv->mm.gtt_mtrr >= 0) {
-		mtrr_del(dev_priv->mm.gtt_mtrr,
-			 dev_priv->gtt.mappable_base,
-			 aperture_size);
-		dev_priv->mm.gtt_mtrr = -1;
-	}
+	arch_phys_wc_del(dev_priv->mm.gtt_mtrr);
 	io_mapping_free(dev_priv->gtt.mappable);
 out_rmmap:
 	pci_iounmap(dev->pdev, dev_priv->regs);
@@ -1697,12 +1668,7 @@ int i915_driver_unload(struct drm_device *dev)
 	cancel_delayed_work_sync(&dev_priv->mm.retire_work);
 
 	io_mapping_free(dev_priv->gtt.mappable);
-	if (dev_priv->mm.gtt_mtrr >= 0) {
-		mtrr_del(dev_priv->mm.gtt_mtrr,
-			 dev_priv->gtt.mappable_base,
-			 dev_priv->gtt.mappable_end);
-		dev_priv->mm.gtt_mtrr = -1;
-	}
+	arch_phys_wc_del(dev_priv->mm.gtt_mtrr);
 
 	acpi_video_unregister();
 
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 4/9] drm,agpgart: Use pgprot_writecombine for AGP maps and make the MTRR optional
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

I'm not sure I understand the intent of the previous behavior.  mmap
on /dev/agpgart and DRM_AGP maps had no cache flags set, so they
would be fully cacheable.  But the DRM code (most of the time) would
add a write-combining MTRR that would change the effective memory
type to WC.

The new behavior just requests WC explicitly for all AGP maps.

If there is any code out there that expects cacheable access to the
AGP aperture (because the drm driver doesn't request an MTRR or
because it's using /dev/agpgart directly), then it will now end up
with a UC or WC mapping, depending on the architecture and PAT
availability.  But cacheable access to the aperture seems like it's
asking for trouble, because, AIUI, the aperture is an alias of RAM.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

It's conceivable that libpciaccess could have issues with this due to
memtype conflicts, but I think this is unlikely (as long as a WC
mapping gets there first, I think the conflict resolution rules will
all work out).  In any case, everything that maps the AGP aperture
ought to be using /dev/agpgart or the DRM API, in which case
everything should be okay.

I don't have anything with an AGP slot to test AFAIK.

 drivers/char/agp/frontend.c |  8 +++++---
 drivers/gpu/drm/drm_pci.c   |  8 ++++----
 drivers/gpu/drm/drm_stub.c  | 10 ++--------
 drivers/gpu/drm/drm_vm.c    | 11 ++++-------
 4 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/char/agp/frontend.c b/drivers/char/agp/frontend.c
index 2e04433..1b19239 100644
--- a/drivers/char/agp/frontend.c
+++ b/drivers/char/agp/frontend.c
@@ -603,7 +603,8 @@ static int agp_mmap(struct file *file, struct vm_area_struct *vma)
 			vma->vm_ops = kerninfo.vm_ops;
 		} else if (io_remap_pfn_range(vma, vma->vm_start,
 				(kerninfo.aper_base + offset) >> PAGE_SHIFT,
-					    size, vma->vm_page_prot)) {
+				size,
+				pgprot_writecombine(vma->vm_page_prot))) {
 			goto out_again;
 		}
 		mutex_unlock(&(agp_fe.agp_mutex));
@@ -618,8 +619,9 @@ static int agp_mmap(struct file *file, struct vm_area_struct *vma)
 		if (kerninfo.vm_ops) {
 			vma->vm_ops = kerninfo.vm_ops;
 		} else if (io_remap_pfn_range(vma, vma->vm_start,
-					    kerninfo.aper_base >> PAGE_SHIFT,
-					    size, vma->vm_page_prot)) {
+				kerninfo.aper_base >> PAGE_SHIFT,
+				size,
+				pgprot_writecombine(vma->vm_page_prot))) {
 			goto out_again;
 		}
 		mutex_unlock(&(agp_fe.agp_mutex));
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index bd719e9..d0f6699 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -278,10 +278,10 @@ int drm_pci_agp_init(struct drm_device *dev)
 		}
 		if (drm_core_has_MTRR(dev)) {
 			if (dev->agp)
-				dev->agp->agp_mtrr -					mtrr_add(dev->agp->agp_info.aper_base,
-						 dev->agp->agp_info.aper_size *
-						 1024 * 1024, MTRR_TYPE_WRCOMB, 1);
+				dev->agp->agp_mtrr = arch_phys_wc_add(
+					dev->agp->agp_info.aper_base,
+					dev->agp->agp_info.aper_size *
+					1024 * 1024);
 		}
 	}
 	return 0;
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 7d30802..9e2acdf 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -451,14 +451,8 @@ void drm_put_dev(struct drm_device *dev)
 
 	drm_lastclose(dev);
 
-	if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) &&
-	    dev->agp && dev->agp->agp_mtrr >= 0) {
-		int retval;
-		retval = mtrr_del(dev->agp->agp_mtrr,
-				  dev->agp->agp_info.aper_base,
-				  dev->agp->agp_info.aper_size * 1024 * 1024);
-		DRM_DEBUG("mtrr_del=%d\n", retval);
-	}
+	if (drm_core_has_MTRR(dev) && drm_core_has_AGP(dev) && dev->agp)
+		arch_phys_wc_del(dev->agp->agp_mtrr);
 
 	if (dev->driver->unload)
 		dev->driver->unload(dev);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index 163f436..b1e4ec8 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -49,13 +49,10 @@ static pgprot_t drm_io_prot(struct drm_local_map *map,
 	pgprot_t tmp = vm_get_page_prot(vma->vm_flags);
 
 #if defined(__i386__) || defined(__x86_64__)
-	if (map->type != _DRM_AGP) {
-		if (map->type = _DRM_FRAME_BUFFER ||
-		    map->flags & _DRM_WRITE_COMBINING)
-			tmp = pgprot_writecombine(tmp);
-		else
-			tmp = pgprot_noncached(tmp);
-	}
+	if (map->type = _DRM_REGISTERS && !(map->flags & _DRM_WRITE_COMBINING))
+		tmp = pgprot_noncached(tmp);
+	else
+		tmp = pgprot_writecombine(tmp);
 #elif defined(__powerpc__)
 	pgprot_val(tmp) |= _PAGE_NO_CACHE;
 	if (map_type = _DRM_REGISTERS)
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 3/9] drm: Update drm_addmap and drm_mmap to use PAT WC instead of MTRRs
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

Previously, DRM_FRAME_BUFFER mappings, as well as DRM_REGISTERS
mappings with DRM_WRITE_COMBINING set, resulted in an unconditional
MTRR being added but the actual mappings being created as UC-.

Now these mappings have the MTRR added only if needed, but they will
be mapped with pgprot_writecombine.

The non-WC DRM_REGISTERS case now uses pgprot_noncached instead of
hardcoding the bit twiddling.

The DRM_AGP case is unchanged for now.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/gpu/drm/drm_bufs.c | 17 +++++++++--------
 drivers/gpu/drm/drm_vm.c   | 23 +++++++++++------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index 0128147..0190fce 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -210,12 +210,16 @@ static int drm_addmap_core(struct drm_device * dev, resource_size_t offset,
 		if (drm_core_has_MTRR(dev)) {
 			if (map->type = _DRM_FRAME_BUFFER ||
 			    (map->flags & _DRM_WRITE_COMBINING)) {
-				map->mtrr = mtrr_add(map->offset, map->size,
-						     MTRR_TYPE_WRCOMB, 1);
+				map->mtrr +					arch_phys_wc_add(map->offset, map->size);
 			}
 		}
 		if (map->type = _DRM_REGISTERS) {
-			map->handle = ioremap(map->offset, map->size);
+			if (map->flags & _DRM_WRITE_COMBINING)
+				map->handle = ioremap_wc(map->offset,
+							 map->size);
+			else
+				map->handle = ioremap(map->offset, map->size);
 			if (!map->handle) {
 				kfree(map);
 				return -ENOMEM;
@@ -451,11 +455,8 @@ int drm_rmmap_locked(struct drm_device *dev, struct drm_local_map *map)
 		iounmap(map->handle);
 		/* FALLTHROUGH */
 	case _DRM_FRAME_BUFFER:
-		if (drm_core_has_MTRR(dev) && map->mtrr >= 0) {
-			int retcode;
-			retcode = mtrr_del(map->mtrr, map->offset, map->size);
-			DRM_DEBUG("mtrr_del=%d\n", retcode);
-		}
+		if (drm_core_has_MTRR(dev))
+			arch_phys_wc_del(map->mtrr);
 		break;
 	case _DRM_SHM:
 		vfree(map->handle);
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index db7bd29..163f436 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -43,14 +43,18 @@
 static void drm_vm_open(struct vm_area_struct *vma);
 static void drm_vm_close(struct vm_area_struct *vma);
 
-static pgprot_t drm_io_prot(uint32_t map_type, struct vm_area_struct *vma)
+static pgprot_t drm_io_prot(struct drm_local_map *map,
+			    struct vm_area_struct *vma)
 {
 	pgprot_t tmp = vm_get_page_prot(vma->vm_flags);
 
 #if defined(__i386__) || defined(__x86_64__)
-	if (boot_cpu_data.x86 > 3 && map_type != _DRM_AGP) {
-		pgprot_val(tmp) |= _PAGE_PCD;
-		pgprot_val(tmp) &= ~_PAGE_PWT;
+	if (map->type != _DRM_AGP) {
+		if (map->type = _DRM_FRAME_BUFFER ||
+		    map->flags & _DRM_WRITE_COMBINING)
+			tmp = pgprot_writecombine(tmp);
+		else
+			tmp = pgprot_noncached(tmp);
 	}
 #elif defined(__powerpc__)
 	pgprot_val(tmp) |= _PAGE_NO_CACHE;
@@ -250,13 +254,8 @@ static void drm_vm_shm_close(struct vm_area_struct *vma)
 			switch (map->type) {
 			case _DRM_REGISTERS:
 			case _DRM_FRAME_BUFFER:
-				if (drm_core_has_MTRR(dev) && map->mtrr >= 0) {
-					int retcode;
-					retcode = mtrr_del(map->mtrr,
-							   map->offset,
-							   map->size);
-					DRM_DEBUG("mtrr_del = %d\n", retcode);
-				}
+				if (drm_core_has_MTRR(dev))
+					arch_phys_wc_del(map->mtrr);
 				iounmap(map->handle);
 				break;
 			case _DRM_SHM:
@@ -617,7 +616,7 @@ int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma)
 	case _DRM_REGISTERS:
 		offset = drm_core_get_reg_ofs(dev);
 		vma->vm_flags |= VM_IO;	/* not in core dump */
-		vma->vm_page_prot = drm_io_prot(map->type, vma);
+		vma->vm_page_prot = drm_io_prot(map, vma);
 		if (io_remap_pfn_range(vma, vma->vm_start,
 				       (map->offset + offset) >> PAGE_SHIFT,
 				       vma->vm_end - vma->vm_start,
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 2/9] drm (ast,cirrus,mgag200,nouveau,savage,vmwgfx): Remove drm_mtrr_{add,del}
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

This replaces drm_mtrr_{add,del} with arch_phys_wc_{add,del}.  The
interface is simplified (because the base and size parameters to
drm_mtrr_del never did anything), and it no longer adds MTRRs on
systems that don't need them.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 drivers/gpu/drm/ast/ast_ttm.c         | 13 +++--------
 drivers/gpu/drm/cirrus/cirrus_ttm.c   | 15 ++++--------
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 14 ++++--------
 drivers/gpu/drm/nouveau/nouveau_ttm.c | 13 ++++-------
 drivers/gpu/drm/savage/savage_bci.c   | 43 ++++++++++++-----------------------
 drivers/gpu/drm/savage/savage_drv.h   |  5 +---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c   | 10 ++++----
 include/drm/drmP.h                    | 29 -----------------------
 8 files changed, 35 insertions(+), 107 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ttm.c b/drivers/gpu/drm/ast/ast_ttm.c
index 3602731..c4574fd 100644
--- a/drivers/gpu/drm/ast/ast_ttm.c
+++ b/drivers/gpu/drm/ast/ast_ttm.c
@@ -271,26 +271,19 @@ int ast_mm_init(struct ast_private *ast)
 		return ret;
 	}
 
-	ast->fb_mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 0),
-				    pci_resource_len(dev->pdev, 0),
-				    DRM_MTRR_WC);
+	ast->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
+					pci_resource_len(dev->pdev, 0));
 
 	return 0;
 }
 
 void ast_mm_fini(struct ast_private *ast)
 {
-	struct drm_device *dev = ast->dev;
 	ttm_bo_device_release(&ast->ttm.bdev);
 
 	ast_ttm_global_release(ast);
 
-	if (ast->fb_mtrr >= 0) {
-		drm_mtrr_del(ast->fb_mtrr,
-			     pci_resource_start(dev->pdev, 0),
-			     pci_resource_len(dev->pdev, 0), DRM_MTRR_WC);
-		ast->fb_mtrr = -1;
-	}
+	arch_phys_wc_del(ast->fb_mtrr);
 }
 
 void ast_ttm_placement(struct ast_bo *bo, int domain)
diff --git a/drivers/gpu/drm/cirrus/cirrus_ttm.c b/drivers/gpu/drm/cirrus/cirrus_ttm.c
index 1413a26..09f06d3 100644
--- a/drivers/gpu/drm/cirrus/cirrus_ttm.c
+++ b/drivers/gpu/drm/cirrus/cirrus_ttm.c
@@ -271,9 +271,8 @@ int cirrus_mm_init(struct cirrus_device *cirrus)
 		return ret;
 	}
 
-	cirrus->fb_mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 0),
-				    pci_resource_len(dev->pdev, 0),
-				    DRM_MTRR_WC);
+	cirrus->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
+					   pci_resource_len(dev->pdev, 0));
 
 	cirrus->mm_inited = true;
 	return 0;
@@ -281,8 +280,6 @@ int cirrus_mm_init(struct cirrus_device *cirrus)
 
 void cirrus_mm_fini(struct cirrus_device *cirrus)
 {
-	struct drm_device *dev = cirrus->dev;
-
 	if (!cirrus->mm_inited)
 		return;
 
@@ -290,12 +287,8 @@ void cirrus_mm_fini(struct cirrus_device *cirrus)
 
 	cirrus_ttm_global_release(cirrus);
 
-	if (cirrus->fb_mtrr >= 0) {
-		drm_mtrr_del(cirrus->fb_mtrr,
-			     pci_resource_start(dev->pdev, 0),
-			     pci_resource_len(dev->pdev, 0), DRM_MTRR_WC);
-		cirrus->fb_mtrr = -1;
-	}
+	arch_phys_wc_del(cirrus->fb_mtrr);
+	cirrus->fb_mtrr = 0;
 }
 
 void cirrus_ttm_placement(struct cirrus_bo *bo, int domain)
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 8fc9d92..5c6f3c8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -270,26 +270,20 @@ int mgag200_mm_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	mdev->fb_mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 0),
-				    pci_resource_len(dev->pdev, 0),
-				    DRM_MTRR_WC);
+	mdev->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
+					 pci_resource_len(dev->pdev, 0));
 
 	return 0;
 }
 
 void mgag200_mm_fini(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
 	ttm_bo_device_release(&mdev->ttm.bdev);
 
 	mgag200_ttm_global_release(mdev);
 
-	if (mdev->fb_mtrr >= 0) {
-		drm_mtrr_del(mdev->fb_mtrr,
-			     pci_resource_start(dev->pdev, 0),
-			     pci_resource_len(dev->pdev, 0), DRM_MTRR_WC);
-		mdev->fb_mtrr = -1;
-	}
+	arch_phys_wc_del(mdev->fb_mtrr);
+	mdev->fb_mtrr = 0;
 }
 
 void mgag200_ttm_placement(struct mgag200_bo *bo, int domain)
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index 9be9cb5..63fa6a5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -377,9 +377,8 @@ nouveau_ttm_init(struct nouveau_drm *drm)
 		return ret;
 	}
 
-	drm->ttm.mtrr = drm_mtrr_add(pci_resource_start(dev->pdev, 1),
-				     pci_resource_len(dev->pdev, 1),
-				     DRM_MTRR_WC);
+	drm->ttm.mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 1),
+					 pci_resource_len(dev->pdev, 1));
 
 	/* GART init */
 	if (drm->agp.stat != ENABLED) {
@@ -414,10 +413,6 @@ nouveau_ttm_fini(struct nouveau_drm *drm)
 
 	nouveau_ttm_global_release(drm);
 
-	if (drm->ttm.mtrr >= 0) {
-		drm_mtrr_del(drm->ttm.mtrr,
-			     pci_resource_start(drm->dev->pdev, 1),
-			     pci_resource_len(drm->dev->pdev, 1), DRM_MTRR_WC);
-		drm->ttm.mtrr = -1;
-	}
+	arch_phys_wc_del(drm->ttm.mtrr);
+	drm->ttm.mtrr = 0;
 }
diff --git a/drivers/gpu/drm/savage/savage_bci.c b/drivers/gpu/drm/savage/savage_bci.c
index b55c1d6..bd6b2cf 100644
--- a/drivers/gpu/drm/savage/savage_bci.c
+++ b/drivers/gpu/drm/savage/savage_bci.c
@@ -570,9 +570,6 @@ int savage_driver_firstopen(struct drm_device *dev)
 	unsigned int fb_rsrc, aper_rsrc;
 	int ret = 0;
 
-	dev_priv->mtrr[0].handle = -1;
-	dev_priv->mtrr[1].handle = -1;
-	dev_priv->mtrr[2].handle = -1;
 	if (S3_SAVAGE3D_SERIES(dev_priv->chipset)) {
 		fb_rsrc = 0;
 		fb_base = pci_resource_start(dev->pdev, 0);
@@ -584,21 +581,14 @@ int savage_driver_firstopen(struct drm_device *dev)
 		if (pci_resource_len(dev->pdev, 0) = 0x08000000) {
 			/* Don't make MMIO write-cobining! We need 3
 			 * MTRRs. */
-			dev_priv->mtrr[0].base = fb_base;
-			dev_priv->mtrr[0].size = 0x01000000;
-			dev_priv->mtrr[0].handle -			    drm_mtrr_add(dev_priv->mtrr[0].base,
-				         dev_priv->mtrr[0].size, DRM_MTRR_WC);
-			dev_priv->mtrr[1].base = fb_base + 0x02000000;
-			dev_priv->mtrr[1].size = 0x02000000;
-			dev_priv->mtrr[1].handle -			    drm_mtrr_add(dev_priv->mtrr[1].base,
-					 dev_priv->mtrr[1].size, DRM_MTRR_WC);
-			dev_priv->mtrr[2].base = fb_base + 0x04000000;
-			dev_priv->mtrr[2].size = 0x04000000;
-			dev_priv->mtrr[2].handle -			    drm_mtrr_add(dev_priv->mtrr[2].base,
-					 dev_priv->mtrr[2].size, DRM_MTRR_WC);
+			dev_priv->mtrr_handles[0] +				arch_phys_wc_add(fb_base, 0x01000000);
+			dev_priv->mtrr_handles[1] +				arch_phys_wc_add(fb_base + 0x02000000,
+						 0x02000000);
+			dev_priv->mtrr_handles[2] +				arch_phys_wc_add(fb_base + 0x04000000,
+						0x04000000);
 		} else {
 			DRM_ERROR("strange pci_resource_len %08llx\n",
 				  (unsigned long long)
@@ -616,11 +606,9 @@ int savage_driver_firstopen(struct drm_device *dev)
 		if (pci_resource_len(dev->pdev, 1) = 0x08000000) {
 			/* Can use one MTRR to cover both fb and
 			 * aperture. */
-			dev_priv->mtrr[0].base = fb_base;
-			dev_priv->mtrr[0].size = 0x08000000;
-			dev_priv->mtrr[0].handle -			    drm_mtrr_add(dev_priv->mtrr[0].base,
-					 dev_priv->mtrr[0].size, DRM_MTRR_WC);
+			dev_priv->mtrr_handles[0] +				arch_phys_wc_add(fb_base,
+						 0x08000000);
 		} else {
 			DRM_ERROR("strange pci_resource_len %08llx\n",
 				  (unsigned long long)
@@ -660,11 +648,10 @@ void savage_driver_lastclose(struct drm_device *dev)
 	drm_savage_private_t *dev_priv = dev->dev_private;
 	int i;
 
-	for (i = 0; i < 3; ++i)
-		if (dev_priv->mtrr[i].handle >= 0)
-			drm_mtrr_del(dev_priv->mtrr[i].handle,
-				 dev_priv->mtrr[i].base,
-				 dev_priv->mtrr[i].size, DRM_MTRR_WC);
+	for (i = 0; i < 3; ++i) {
+		arch_phys_wc_del(dev_priv->mtrr_handles[i]);
+		dev_priv->mtrr_handles[i] = 0;
+	}
 }
 
 int savage_driver_unload(struct drm_device *dev)
diff --git a/drivers/gpu/drm/savage/savage_drv.h b/drivers/gpu/drm/savage/savage_drv.h
index df2aac6..c05082a 100644
--- a/drivers/gpu/drm/savage/savage_drv.h
+++ b/drivers/gpu/drm/savage/savage_drv.h
@@ -160,10 +160,7 @@ typedef struct drm_savage_private {
 	drm_local_map_t *cmd_dma;
 	drm_local_map_t fake_dma;
 
-	struct {
-		int handle;
-		unsigned long base, size;
-	} mtrr[3];
+	int mtrr_handles[3];
 
 	/* BCI and status-related stuff */
 	volatile uint32_t *status_ptr, *bci_ptr;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 07dfd82..78e2164 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -565,8 +565,8 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 		dev_priv->has_gmr = false;
 	}
 
-	dev_priv->mmio_mtrr = drm_mtrr_add(dev_priv->mmio_start,
-					   dev_priv->mmio_size, DRM_MTRR_WC);
+	dev_priv->mmio_mtrr = arch_phys_wc_add(dev_priv->mmio_start,
+					       dev_priv->mmio_size);
 
 	dev_priv->mmio_virt = ioremap_wc(dev_priv->mmio_start,
 					 dev_priv->mmio_size);
@@ -664,8 +664,7 @@ out_no_device:
 out_err4:
 	iounmap(dev_priv->mmio_virt);
 out_err3:
-	drm_mtrr_del(dev_priv->mmio_mtrr, dev_priv->mmio_start,
-		     dev_priv->mmio_size, DRM_MTRR_WC);
+	arch_phys_wc_del(dev_priv->mmio_mtrr);
 	if (dev_priv->has_gmr)
 		(void) ttm_bo_clean_mm(&dev_priv->bdev, VMW_PL_GMR);
 	(void)ttm_bo_clean_mm(&dev_priv->bdev, TTM_PL_VRAM);
@@ -709,8 +708,7 @@ static int vmw_driver_unload(struct drm_device *dev)
 
 	ttm_object_device_release(&dev_priv->tdev);
 	iounmap(dev_priv->mmio_virt);
-	drm_mtrr_del(dev_priv->mmio_mtrr, dev_priv->mmio_start,
-		     dev_priv->mmio_size, DRM_MTRR_WC);
+	arch_phys_wc_del(dev_priv->mmio_mtrr);
 	if (dev_priv->has_gmr)
 		(void)ttm_bo_clean_mm(&dev_priv->bdev, VMW_PL_GMR);
 	(void)ttm_bo_clean_mm(&dev_priv->bdev, TTM_PL_VRAM);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 2d94d74..3e6cfa0 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1249,37 +1249,8 @@ static inline int drm_core_has_MTRR(struct drm_device *dev)
 {
 	return drm_core_check_feature(dev, DRIVER_USE_MTRR);
 }
-
-#define DRM_MTRR_WC		MTRR_TYPE_WRCOMB
-
-static inline int drm_mtrr_add(unsigned long offset, unsigned long size,
-			       unsigned int flags)
-{
-	return mtrr_add(offset, size, flags, 1);
-}
-
-static inline int drm_mtrr_del(int handle, unsigned long offset,
-			       unsigned long size, unsigned int flags)
-{
-	return mtrr_del(handle, offset, size);
-}
-
 #else
 #define drm_core_has_MTRR(dev) (0)
-
-#define DRM_MTRR_WC		0
-
-static inline int drm_mtrr_add(unsigned long offset, unsigned long size,
-			       unsigned int flags)
-{
-	return 0;
-}
-
-static inline int drm_mtrr_del(int handle, unsigned long offset,
-			       unsigned long size, unsigned int flags)
-{
-	return 0;
-}
 #endif
 
 static inline void drm_device_set_unplugged(struct drm_device *dev)
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 1/9] Add arch_phys_wc_{add,del} to manipulate WC MTRRs if needed
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski
In-Reply-To: <cover.1368485053.git.luto@amacapital.net>

Several drivers currently use mtrr_add through various #ifdef guards
and/or drm wrappers.  The vast majority of them want to add WC MTRRs
on x86 systems and don't actually need the MTRR if PAT (i.e.
ioremap_wc, etc) are working.

arch_phys_wc_add and arch_phys_wc_del are new functions, available
on all architectures and configurations, that add WC MTRRs on x86 if
needed (and handle errors) and do nothing at all otherwise.  They're
also easier to use than mtrr_add and mtrr_del, so the call sites can
be simplified.

As an added benefit, this will avoid wasting MTRRs and possibly
warning pointlessly on PAT-supporting systems.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---

Changes from v2:
 - Make MTRR_TO_PHYS_WC_OFFSET a macro instead of open-coding it.
 - Add phys_wc_to_mtrr_index to make drmGetMap keep working.  Sigh.

 arch/x86/include/asm/io.h       |  7 ++++
 arch/x86/include/asm/mtrr.h     | 10 +++++-
 arch/x86/kernel/cpu/mtrr/main.c | 71 +++++++++++++++++++++++++++++++++++++++++
 include/linux/io.h              | 25 +++++++++++++++
 4 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index d8e8eef..34f69cb 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -345,4 +345,11 @@ extern bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
 
 #define IO_SPACE_LIMIT 0xffff
 
+#ifdef CONFIG_MTRR
+extern int __must_check arch_phys_wc_add(unsigned long base,
+					 unsigned long size);
+extern void arch_phys_wc_del(int handle);
+#define arch_phys_wc_add arch_phys_wc_add
+#endif
+
 #endif /* _ASM_X86_IO_H */
diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index e235582..f768f62 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -26,7 +26,10 @@
 #include <uapi/asm/mtrr.h>
 
 
-/*  The following functions are for use by other drivers  */
+/*
+ * The following functions are for use by other drivers that cannot use
+ * arch_phys_wc_add and arch_phys_wc_del.
+ */
 # ifdef CONFIG_MTRR
 extern u8 mtrr_type_lookup(u64 addr, u64 end);
 extern void mtrr_save_fixed_ranges(void *);
@@ -45,6 +48,7 @@ extern void mtrr_aps_init(void);
 extern void mtrr_bp_restore(void);
 extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
 extern int amd_special_default_mtrr(void);
+extern int phys_wc_to_mtrr_index(int handle);
 #  else
 static inline u8 mtrr_type_lookup(u64 addr, u64 end)
 {
@@ -80,6 +84,10 @@ static inline int mtrr_trim_uncached_memory(unsigned long end_pfn)
 static inline void mtrr_centaur_report_mcr(int mcr, u32 lo, u32 hi)
 {
 }
+static inline int phys_wc_to_mtrr_index(int handle)
+{
+	return -1;
+}
 
 #define mtrr_ap_init() do {} while (0)
 #define mtrr_bp_init() do {} while (0)
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index 726bf96..3533d4d 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -51,9 +51,13 @@
 #include <asm/e820.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
+#include <asm/pat.h>
 
 #include "mtrr.h"
 
+/* arch_phys_wc_add returns an MTRR register index plus this offset. */
+#define MTRR_TO_PHYS_WC_OFFSET 1000
+
 u32 num_var_ranges;
 
 unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
@@ -524,6 +528,73 @@ int mtrr_del(int reg, unsigned long base, unsigned long size)
 }
 EXPORT_SYMBOL(mtrr_del);
 
+/**
+ * arch_phys_wc_add - add a WC MTRR and handle errors if PAT is unavailable
+ * @base: Physical base address
+ * @size: Size of region
+ *
+ * If PAT is available, this does nothing.  If PAT is unavailable, it
+ * attempts to add a WC MTRR covering size bytes starting at base and
+ * logs an error if this fails.
+ *
+ * Drivers must store the return value to pass to mtrr_del_wc_if_needed,
+ * but drivers should not try to interpret that return value.
+ */
+int arch_phys_wc_add(unsigned long base, unsigned long size)
+{
+	int ret;
+
+	if (pat_enabled)
+		return 0;  /* Success!  (We don't need to do anything.) */
+
+	ret = mtrr_add(base, size, MTRR_TYPE_WRCOMB, true);
+	if (ret < 0) {
+		pr_warn("Failed to add WC MTRR for [%p-%p]; performance may suffer.",
+			(void *)base, (void *)(base + size - 1));
+		return ret;
+	}
+	return ret + MTRR_TO_PHYS_WC_OFFSET;
+}
+EXPORT_SYMBOL(arch_phys_wc_add);
+
+/*
+ * arch_phys_wc_del - undoes arch_phys_wc_add
+ * @handle: Return value from arch_phys_wc_add
+ *
+ * This cleans up after mtrr_add_wc_if_needed.
+ *
+ * The API guarantees that mtrr_del_wc_if_needed(error code) and
+ * mtrr_del_wc_if_needed(0) do nothing.
+ */
+void arch_phys_wc_del(int handle)
+{
+	if (handle >= 1) {
+		WARN_ON(handle < MTRR_TO_PHYS_WC_OFFSET);
+		mtrr_del(handle - MTRR_TO_PHYS_WC_OFFSET, 0, 0);
+	}
+}
+EXPORT_SYMBOL(arch_phys_wc_del);
+
+/*
+ * phys_wc_to_mtrr_index - translates arch_phys_wc_add's return value
+ * @handle: Return value from arch_phys_wc_add
+ *
+ * This will turn the return value from arch_phys_wc_add into an mtrr
+ * index suitable for debugging.
+ *
+ * Note: There is no legitimate use for this function, except possibly
+ * in printk line.  Alas there is an illegitimate use in some ancient
+ * drm ioctls.
+ */
+int phys_wc_to_mtrr_index(int handle)
+{
+	if (handle < MTRR_TO_PHYS_WC_OFFSET)
+		return -1;
+	else
+		return handle - MTRR_TO_PHYS_WC_OFFSET;
+}
+EXPORT_SYMBOL_GPL(phys_wc_to_mtrr_index);
+
 /*
  * HACK ALERT!
  * These should be called implicitly, but we can't yet until all the initcall
diff --git a/include/linux/io.h b/include/linux/io.h
index 069e407..f4f42fa 100644
--- a/include/linux/io.h
+++ b/include/linux/io.h
@@ -76,4 +76,29 @@ void devm_ioremap_release(struct device *dev, void *res);
 #define arch_has_dev_port()     (1)
 #endif
 
+/*
+ * Some systems (x86 without PAT) have a somewhat reliable way to mark a
+ * physical address range such that uncached mappings will actually
+ * end up write-combining.  This facility should be used in conjunction
+ * with pgprot_writecombine, ioremap-wc, or set_memory_wc, since it has
+ * no effect if the per-page mechanisms are functional.
+ * (On x86 without PAT, these functions manipulate MTRRs.)
+ *
+ * arch_phys_del_wc(0) or arch_phys_del_wc(any error code) is guaranteed
+ * to have no effect.
+ */
+#ifndef arch_phys_wc_add
+static inline int __must_check arch_phys_wc_add(unsigned long base,
+						unsigned long size)
+{
+	return 0;  /* It worked (i.e. did nothing). */
+}
+
+static inline void arch_phys_wc_del(int handle)
+{
+}
+
+#define arch_phys_wc_add arch_phys_wc_add
+#endif
+
 #endif /* _LINUX_IO_H */
-- 
1.8.1.4


^ permalink raw reply related

* [PATCH v3 0/9] Clean up write-combining MTRR addition
From: Andy Lutomirski @ 2013-05-13 23:58 UTC (permalink / raw)
  To: linux-kernel, dri-devel, linux-fbdev
  Cc: Daniel Vetter, Jerome Glisse, Alex Deucher, Dave Airlie,
	Andy Lutomirski

A fair number of drivers (mostly graphics) add write-combining MTRRs.
Most ignore errors and most add the MTRR even on PAT systems which don't
need to use MTRRs.

This series adds new functions arch_phys_wc_{add,del} that, on PAT-less
x86 systems with MTRRs, add MTRRs and report errors, and that do nothing
otherwise.  (Other architectures, if any, with a similar mechanism could
implement them.)

I've only tested the radeon driver, since I don't have test hardware
easily available for the other drivers.

Benefits include:
 - Simpler code
 - No more complaints about MTRR conflict warnings on PAT systems
 - Eventual unexporting of the MTRR API?

This series eliminates about half of the mtrr_add calls in drivers/.

Note: this series breaks and then fixes dritests from libdrm.  The breakage
is probably irrelevant for any practical purpose, and fixing it will
be a bit complicated due to header file breakage that's only fixed in
patch 8.

Daniel, can you check that patch 1 is still okay?

Changes from v2:
 - There's a new API phys_wc_to_mtrr_index (x86-only) to support drmGetMap
 - Minor cleanups to patche 1 and 5.
 - Patch 9 is new

Changes from v1:
 - Helpers renamed
 - Lots of bugs fixed

Andy Lutomirski (9):
  Add arch_phys_wc_{add,del} to manipulate WC MTRRs if needed
  drm (ast,cirrus,mgag200,nouveau,savage,vmwgfx): Remove
    drm_mtrr_{add,del}
  drm: Update drm_addmap and drm_mmap to use PAT WC instead of MTRRs
  drm,agpgart: Use pgprot_writecombine for AGP maps and make the MTRR
    optional
  i915: Use arch_phys_wc_{add,del}
  radeon: Switch to arch_phys_wc_add and add a missing ..._del
  uvesafb: Clean up MTRR code
  drm: Remove mtrr_add and mtrr_del fallback hack for non-MTRR systems
  drm: Don't leak phys_wc "handles" to userspace

Andy Lutomirski (9):
  Add arch_phys_wc_{add,del} to manipulate WC MTRRs if needed
  drm (ast,cirrus,mgag200,nouveau,savage,vmwgfx): Remove
    drm_mtrr_{add,del}
  drm: Update drm_addmap and drm_mmap to use PAT WC instead of MTRRs
  drm,agpgart: Use pgprot_writecombine for AGP maps and make the MTRR
    optional
  i915: Use arch_phys_wc_{add,del}
  radeon: Switch to arch_phys_wc_add and add a missing ..._del
  uvesafb: Clean up MTRR code
  drm: Remove mtrr_add and mtrr_del fallback hack for non-MTRR systems
  drm: Don't leak phys_wc "handles" to userspace

 Documentation/fb/uvesafb.txt           | 16 +++-----
 arch/x86/include/asm/io.h              |  7 ++++
 arch/x86/include/asm/mtrr.h            | 10 ++++-
 arch/x86/kernel/cpu/mtrr/main.c        | 71 ++++++++++++++++++++++++++++++++++
 drivers/char/agp/frontend.c            |  8 ++--
 drivers/gpu/drm/ast/ast_ttm.c          | 13 ++-----
 drivers/gpu/drm/cirrus/cirrus_ttm.c    | 15 ++-----
 drivers/gpu/drm/drm_bufs.c             | 26 +++++++++----
 drivers/gpu/drm/drm_ioctl.c            | 15 ++++++-
 drivers/gpu/drm/drm_pci.c              |  8 ++--
 drivers/gpu/drm/drm_stub.c             | 10 +----
 drivers/gpu/drm/drm_vm.c               | 22 +++++------
 drivers/gpu/drm/i915/i915_dma.c        | 42 ++------------------
 drivers/gpu/drm/mgag200/mgag200_ttm.c  | 14 ++-----
 drivers/gpu/drm/nouveau/nouveau_ttm.c  | 13 ++-----
 drivers/gpu/drm/radeon/radeon_object.c |  5 ++-
 drivers/gpu/drm/savage/savage_bci.c    | 43 +++++++-------------
 drivers/gpu/drm/savage/savage_drv.h    |  5 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c    | 10 ++---
 drivers/video/uvesafb.c                | 70 ++++++++-------------------------
 include/drm/drmP.h                     | 34 +---------------
 include/drm/drm_os_linux.h             | 16 --------
 include/linux/io.h                     | 25 ++++++++++++
 include/video/uvesafb.h                |  1 +
 24 files changed, 230 insertions(+), 269 deletions(-)

-- 
1.8.1.4


^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Tomasz Figa @ 2013-05-13 19:29 UTC (permalink / raw)
  To: dri-devel
  Cc: Inki Dae, 'Maarten Lankhorst', 'linux-fbdev',
	'Kyungmin Park', 'myungjoo.ham',
	'YoungJun Cho', linux-arm-kernel, linux-media
In-Reply-To: <027a01ce4fcc$5e7c7320$1b755960$%dae@samsung.com>

Hi,

On Monday 13 of May 2013 20:24:01 Inki Dae wrote:
> > -----Original Message-----
> > From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> > Sent: Monday, May 13, 2013 6:52 PM
> > To: Inki Dae
> > Cc: 'Rob Clark'; 'Daniel Vetter'; 'DRI mailing list'; linux-arm-
> > kernel@lists.infradead.org; linux-media@vger.kernel.org;
> > 'linux-fbdev';
> > 'Kyungmin Park'; 'myungjoo.ham'; 'YoungJun Cho'
> > Subject: Re: Introduce a new helper framework for buffer
> > synchronization> 
> > Op 13-05-13 11:21, Inki Dae schreef:
> > >> -----Original Message-----
> > >> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> > >> Sent: Monday, May 13, 2013 5:01 PM
> > >> To: Inki Dae
> > >> Cc: Rob Clark; Daniel Vetter; DRI mailing list; linux-arm-
> > >> kernel@lists.infradead.org; linux-media@vger.kernel.org;
> > >> linux-fbdev;
> > >> Kyungmin Park; myungjoo.ham; YoungJun Cho
> > >> Subject: Re: Introduce a new helper framework for buffer
> > 
> > synchronization
> > 
> > >> Op 09-05-13 09:33, Inki Dae schreef:
> > >>> Hi all,
> > >>> 
> > >>> This post introduces a new helper framework based on dma fence.
> > >>> And
> > 
> > the
> > 
> > >>> purpose of this post is to collect other opinions and advices
> > >>> before
> > 
> > RFC
> > 
> > >>> posting.
> > >>> 
> > >>> First of all, this helper framework, called fence helper, is in
> > 
> > progress
> > 
> > >>> yet so might not have enough comments in codes and also might need
> > >>> to
> > 
> > be
> > 
> > >>> more cleaned up. Moreover, we might be missing some parts of the
> > >>> dma
> > >> 
> > >> fence.
> > >> 
> > >>> However, I'd like to say that all things mentioned below has been
> > 
> > tested
> > 
> > >>> with Linux platform and worked well.
> > >>> ....
> > >>> 
> > >>> And tutorial for user process.
> > >>> 
> > >>>         just before cpu access
> > >>>         
> > >>>                 struct dma_buf_fence *df;
> > >>>                 
> > >>>                 df->type = DMA_BUF_ACCESS_READ or
> 
> DMA_BUF_ACCESS_WRITE;
> 
> > >>>                 ioctl(fd, DMA_BUF_GET_FENCE, &df);
> > >>>         
> > >>>         after memset or memcpy
> > >>>         
> > >>>                 ioctl(fd, DMA_BUF_PUT_FENCE, &df);
> > >> 
> > >> NAK.
> > >> 
> > >> Userspace doesn't need to trigger fences. It can do a buffer idle
> > >> wait,
> > >> and postpone submitting new commands until after it's done using
> > >> the
> > >> buffer.
> > > 
> > > Hi Maarten,
> > > 
> > > It seems that you say user should wait for a buffer like KDS does:
> > > KDS
> > 
> > uses
> > 
> > > select() to postpone submitting new commands. But I think this way
> > 
> > assumes
> > 
> > > that every data flows a DMA device to a CPU. For example, a CPU
> > > should
> > 
> > keep
> > 
> > > polling for the completion of a buffer access by a DMA device. This
> > 
> > means
> > 
> > > that the this way isn't considered for data flow to opposite case;
> > > CPU
> > 
> > to
> > 
> > > DMA device.
> > 
> > Not really. You do both things the same way. You first wait for the bo
> > to be idle, this could be implemented by adding poll support to the
> > dma-buf fd.
> > Then you either do your read or write. Since userspace is supposed to
> > be the one controlling the bo it should stay idle at that point. If
> > you have another thread queueing
> > the buffer againbefore your thread is done that's a bug in the
> 
> application,
> 
> > and can be solved with userspace locking primitives. No need for the
> > kernel to get involved.
> 
> Yes, that is how we have synchronized buffer between CPU and DMA device
> until now without buffer synchronization mechanism. I thought that it's
> best to make user not considering anything: user can access a buffer
> regardless of any DMA device controlling and the buffer synchronization
> is performed in kernel level. Moreover, I think we could optimize
> graphics and multimedia hardware performance because hardware can do
> more works: one thread accesses a shared buffer and the other controls
> DMA device with the shared buffer in parallel.

Could you explain this point? I thought that if there is a shared buffer 
accessible for user and DMA device, only one of them can use it at the 
moment, i.e. the buffer is useless for the reading user (or read DMA) 
until (write) DMA (or writing user) finishes writing for it. Is it 
incorrect? Or this is not the point here?

I'm not an expert here and I'm just trying to understand the idea, so 
correct me if I'm wrong. Thanks in advance.

Best regards,
Tomasz

> Thus, we could avoid
> sequential processing and that is my intention. Aren't you think about
> that we could improve hardware utilization with such way or other? of
> course, there could be better way.
> 
> > >> Kernel space doesn't need the root hole you created by giving a
> > >> dereferencing a pointer passed from userspace.
> > >> Your next exercise should be to write a security exploit from the
> > >> api
> > 
> > you
> > 
> > >> created here. It's the only way to learn how to write safe code.
> > >> Hint:
> > >> df.ctx = mmap(..);
> > > 
> > > Also I'm not clear to use our way yet and that is why I posted. As
> > > you
> > > mentioned, it seems like that using mmap() is more safe. But there
> > > is
> > 
> > one
> > 
> > > issue it makes me confusing. For your hint, df.ctx = mmap(..), the
> > > issue> 
> > is
> > 
> > > that dmabuf mmap can be used to map a dmabuf with user space. And
> > > the
> > 
> > dmabuf
> > 
> > > means a physical memory region allocated by some allocator such as
> > > drm
> > 
> > gem
> > 
> > > or ion.
> > > 
> > > There might be my missing point so could you please give me more
> > 
> > comments?
> > 
> > My point was that userspace could change df.ctx to some mmap'd memory,
> > forcing the kernel to execute some code prepared by userspace.
> 
> Understood. I have to find a better way. And for this, I'd like to
> listen attentively more opinions and advices.
> 
> Thanks for comments,
> Inki Dae
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Rob Clark @ 2013-05-13 17:58 UTC (permalink / raw)
  To: Inki Dae
  Cc: linux-fbdev, DRI mailing list, Kyungmin Park, myungjoo.ham,
	YoungJun Cho, linux-arm-kernel@lists.infradead.org,
	linux-media@vger.kernel.org
In-Reply-To: <CAAQKjZP=iOmHRpHZCbZD3v_RKUFSn0eM_WVZZvhe7F9g3eTmPA@mail.gmail.com>

On Mon, May 13, 2013 at 1:18 PM, Inki Dae <inki.dae@samsung.com> wrote:
>
>
> 2013/5/13 Rob Clark <robdclark@gmail.com>
>>
>> On Mon, May 13, 2013 at 8:21 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> >
>> >> In that case you still wouldn't give userspace control over the fences.
>> >> I
>> >> don't see any way that can end well.
>> >> What if userspace never signals? What if userspace gets killed by oom
>> >> killer. Who keeps track of that?
>> >>
>> >
>> > In all cases, all kernel resources to user fence will be released by
>> > kernel
>> > once the fence is timed out: never signaling and process killing by oom
>> > killer makes the fence timed out. And if we use mmap mechanism you
>> > mentioned
>> > before, I think user resource could also be freed properly.
>>
>>
>> I tend to agree w/ Maarten here.. there is no good reason for
>> userspace to be *signaling* fences.  The exception might be some blob
>> gpu drivers which don't have enough knowledge in the kernel to figure
>> out what to do.  (In which case you can add driver private ioctls for
>> that.. still not the right thing to do but at least you don't make a
>> public API out of it.)
>>
>
> Please do not care whether those are generic or not. Let's see the following
> three things. First, it's cache operation. As you know, ARM SoC has ACP
> (Accelerator Coherency Port) and can be connected to DMA engine or similar
> devices. And this port is used for cache coherency between CPU cache and DMA
> device. However, most devices on ARM based embedded systems don't use the
> ACP port. So they need proper cache operation before and after of DMA or CPU
> access in case of using cachable mapping. Actually, I see many Linux based
> platforms call cache control interfaces directly for that. I think the
> reason, they do so, is that kernel isn't aware of when and how CPU accessed
> memory.

I think we had kicked around the idea of giving dmabuf's a
prepare/finish ioctl quite some time back.  This is probably something
that should be at least a bit decoupled from fences.  (Possibly
'prepare' waits for dma access to complete, but not the other way
around.)

And I did implement in omapdrm support for simulating coherency via
page fault-in / shoot-down..  It is one option that makes it
completely transparent to userspace, although there is some
performance const, so I suppose it depends a bit on your use-case.

> And second, user process has to do so many things in case of using shared
> memory with DMA device. User process should understand how DMA device is
> operated and when interfaces for controling the DMA device are called. Such
> things would make user application so complicated.
>
> And third, it's performance optimization to multimedia and graphics devices.
> As I mentioned already, we should consider sequential processing for buffer
> sharing between CPU and DMA device. This means that CPU should stay with
> idle until DMA device is completed and vise versa.
>
> That is why I proposed such user interfaces. Of course, these interfaces
> might be so ugly yet: for this, Maarten pointed already out and I agree with
> him. But there must be another better way. Aren't you think we need similar
> thing? With such interfaces, cache control and buffer synchronization can be
> performed in kernel level. Moreover, user applization doesn't need to
> consider DMA device controlling anymore. Therefore, one thread can access a
> shared buffer and the other can control DMA device with the shared buffer in
> parallel. We can really make the best use of CPU and DMA idle time. In other
> words, we can really make the best use of multi tasking OS, Linux.
>
> So could you please tell me about that there is any reason we don't use
> public API for it? I think we can add and use public API if NECESSARY.

well, for cache management, I think it is a better idea.. I didn't
really catch that this was the motivation from the initial patch, but
maybe I read it too quickly.  But cache can be decoupled from
synchronization, because CPU access is not asynchronous.  For
userspace/CPU access to buffer, you should:

  1) wait for buffer
  2) prepare-access
  3)  ... do whatever cpu access to buffer ...
  4) finish-access
  5) submit buffer for new dma-operation

I suppose you could combine the syscall for #1 and #2.. not sure if
that is a good idea or not.  But you don't need to.  And there is
never really any need for userspace to signal a fence.

BR,
-R

> Thanks,
> Inki Dae
>
>>
>> BR,
>> -R
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Rob Clark @ 2013-05-13 13:48 UTC (permalink / raw)
  To: Inki Dae
  Cc: Maarten Lankhorst, Daniel Vetter, DRI mailing list,
	linux-arm-kernel, linux-media, linux-fbdev, Kyungmin Park,
	myungjoo.ham, YoungJun Cho
In-Reply-To: <028a01ce4fd4$5ec6f000$1c54d000$%dae@samsung.com>

On Mon, May 13, 2013 at 8:21 AM, Inki Dae <inki.dae@samsung.com> wrote:
>
>> In that case you still wouldn't give userspace control over the fences. I
>> don't see any way that can end well.
>> What if userspace never signals? What if userspace gets killed by oom
>> killer. Who keeps track of that?
>>
>
> In all cases, all kernel resources to user fence will be released by kernel
> once the fence is timed out: never signaling and process killing by oom
> killer makes the fence timed out. And if we use mmap mechanism you mentioned
> before, I think user resource could also be freed properly.


I tend to agree w/ Maarten here.. there is no good reason for
userspace to be *signaling* fences.  The exception might be some blob
gpu drivers which don't have enough knowledge in the kernel to figure
out what to do.  (In which case you can add driver private ioctls for
that.. still not the right thing to do but at least you don't make a
public API out of it.)

BR,
-R

^ permalink raw reply

* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-13 12:21 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>



> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> owner@vger.kernel.org] On Behalf Of Maarten Lankhorst
> Sent: Monday, May 13, 2013 8:41 PM
> To: Inki Dae
> Cc: 'Rob Clark'; 'Daniel Vetter'; 'DRI mailing list'; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org; 'linux-fbdev';
> 'Kyungmin Park'; 'myungjoo.ham'; 'YoungJun Cho'
> Subject: Re: Introduce a new helper framework for buffer synchronization
> 
> Op 13-05-13 13:24, Inki Dae schreef:
> >> and can be solved with userspace locking primitives. No need for the
> >> kernel to get involved.
> >>
> > Yes, that is how we have synchronized buffer between CPU and DMA device
> > until now without buffer synchronization mechanism. I thought that it's
> best
> > to make user not considering anything: user can access a buffer
> regardless
> > of any DMA device controlling and the buffer synchronization is
> performed in
> > kernel level. Moreover, I think we could optimize graphics and
> multimedia
> > hardware performance because hardware can do more works: one thread
> accesses
> > a shared buffer and the other controls DMA device with the shared buffer
> in
> > parallel. Thus, we could avoid sequential processing and that is my
> > intention. Aren't you think about that we could improve hardware
> utilization
> > with such way or other? of course, there could be better way.
> >
> If you don't want to block the hardware the only option is to allocate a
> temp bo and blit to/from it using the hardware.
> OpenGL already does that when you want to read back, because otherwise the
> entire pipeline can get stalled.
> The overhead of command submission for that shouldn't be high, if it is
> you should really try to optimize that first
> before coming up with this crazy scheme.
> 

I have considered all devices sharing buffer with CPU; multimedia, display
controller and graphics devices (including GPU). And we could provide
easy-to-use user interfaces for buffer synchronization. Of course, the
proposed user interfaces may be so ugly yet but at least, I think we need
something else for it.

> In that case you still wouldn't give userspace control over the fences. I
> don't see any way that can end well.
> What if userspace never signals? What if userspace gets killed by oom
> killer. Who keeps track of that?
> 

In all cases, all kernel resources to user fence will be released by kernel
once the fence is timed out: never signaling and process killing by oom
killer makes the fence timed out. And if we use mmap mechanism you mentioned
before, I think user resource could also be freed properly.

Thanks,
Inki Dae

> ~Maarten
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Maarten Lankhorst @ 2013-05-13 11:40 UTC (permalink / raw)
  To: Inki Dae
  Cc: 'Rob Clark', 'Daniel Vetter',
	'DRI mailing list', linux-arm-kernel, linux-media,
	'linux-fbdev', 'Kyungmin Park',
	'myungjoo.ham', 'YoungJun Cho'
In-Reply-To: <027a01ce4fcc$5e7c7320$1b755960$%dae@samsung.com>

Op 13-05-13 13:24, Inki Dae schreef:
>> and can be solved with userspace locking primitives. No need for the
>> kernel to get involved.
>>
> Yes, that is how we have synchronized buffer between CPU and DMA device
> until now without buffer synchronization mechanism. I thought that it's best
> to make user not considering anything: user can access a buffer regardless
> of any DMA device controlling and the buffer synchronization is performed in
> kernel level. Moreover, I think we could optimize graphics and multimedia
> hardware performance because hardware can do more works: one thread accesses
> a shared buffer and the other controls DMA device with the shared buffer in
> parallel. Thus, we could avoid sequential processing and that is my
> intention. Aren't you think about that we could improve hardware utilization
> with such way or other? of course, there could be better way.
>
If you don't want to block the hardware the only option is to allocate a temp bo and blit to/from it using the hardware.
OpenGL already does that when you want to read back, because otherwise the entire pipeline can get stalled.
The overhead of command submission for that shouldn't be high, if it is you should really try to optimize that first
before coming up with this crazy scheme.

In that case you still wouldn't give userspace control over the fences. I don't see any way that can end well.
What if userspace never signals? What if userspace gets killed by oom killer. Who keeps track of that?

~Maarten

^ permalink raw reply

* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-13 11:24 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>



> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> Sent: Monday, May 13, 2013 6:52 PM
> To: Inki Dae
> Cc: 'Rob Clark'; 'Daniel Vetter'; 'DRI mailing list'; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org; 'linux-fbdev';
> 'Kyungmin Park'; 'myungjoo.ham'; 'YoungJun Cho'
> Subject: Re: Introduce a new helper framework for buffer synchronization
> 
> Op 13-05-13 11:21, Inki Dae schreef:
> >
> >> -----Original Message-----
> >> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> >> Sent: Monday, May 13, 2013 5:01 PM
> >> To: Inki Dae
> >> Cc: Rob Clark; Daniel Vetter; DRI mailing list; linux-arm-
> >> kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-fbdev;
> >> Kyungmin Park; myungjoo.ham; YoungJun Cho
> >> Subject: Re: Introduce a new helper framework for buffer
> synchronization
> >>
> >> Op 09-05-13 09:33, Inki Dae schreef:
> >>> Hi all,
> >>>
> >>> This post introduces a new helper framework based on dma fence. And
> the
> >>> purpose of this post is to collect other opinions and advices before
> RFC
> >>> posting.
> >>>
> >>> First of all, this helper framework, called fence helper, is in
> progress
> >>> yet so might not have enough comments in codes and also might need to
> be
> >>> more cleaned up. Moreover, we might be missing some parts of the dma
> >> fence.
> >>> However, I'd like to say that all things mentioned below has been
> tested
> >>> with Linux platform and worked well.
> >>> ....
> >>>
> >>> And tutorial for user process.
> >>>         just before cpu access
> >>>                 struct dma_buf_fence *df;
> >>>
> >>>                 df->type = DMA_BUF_ACCESS_READ or
DMA_BUF_ACCESS_WRITE;
> >>>                 ioctl(fd, DMA_BUF_GET_FENCE, &df);
> >>>
> >>>         after memset or memcpy
> >>>                 ioctl(fd, DMA_BUF_PUT_FENCE, &df);
> >> NAK.
> >>
> >> Userspace doesn't need to trigger fences. It can do a buffer idle wait,
> >> and postpone submitting new commands until after it's done using the
> >> buffer.
> > Hi Maarten,
> >
> > It seems that you say user should wait for a buffer like KDS does: KDS
> uses
> > select() to postpone submitting new commands. But I think this way
> assumes
> > that every data flows a DMA device to a CPU. For example, a CPU should
> keep
> > polling for the completion of a buffer access by a DMA device. This
> means
> > that the this way isn't considered for data flow to opposite case; CPU
> to
> > DMA device.
> Not really. You do both things the same way. You first wait for the bo to
> be idle, this could be implemented by adding poll support to the dma-buf
> fd.
> Then you either do your read or write. Since userspace is supposed to be
> the one controlling the bo it should stay idle at that point. If you have
> another thread queueing
> the buffer againbefore your thread is done that's a bug in the
application,
> and can be solved with userspace locking primitives. No need for the
> kernel to get involved.
> 

Yes, that is how we have synchronized buffer between CPU and DMA device
until now without buffer synchronization mechanism. I thought that it's best
to make user not considering anything: user can access a buffer regardless
of any DMA device controlling and the buffer synchronization is performed in
kernel level. Moreover, I think we could optimize graphics and multimedia
hardware performance because hardware can do more works: one thread accesses
a shared buffer and the other controls DMA device with the shared buffer in
parallel. Thus, we could avoid sequential processing and that is my
intention. Aren't you think about that we could improve hardware utilization
with such way or other? of course, there could be better way.

> >> Kernel space doesn't need the root hole you created by giving a
> >> dereferencing a pointer passed from userspace.
> >> Your next exercise should be to write a security exploit from the api
> you
> >> created here. It's the only way to learn how to write safe code. Hint:
> >> df.ctx = mmap(..);
> >>
> > Also I'm not clear to use our way yet and that is why I posted. As you
> > mentioned, it seems like that using mmap() is more safe. But there is
> one
> > issue it makes me confusing. For your hint, df.ctx = mmap(..), the issue
> is
> > that dmabuf mmap can be used to map a dmabuf with user space. And the
> dmabuf
> > means a physical memory region allocated by some allocator such as drm
> gem
> > or ion.
> >
> > There might be my missing point so could you please give me more
> comments?
> >
> My point was that userspace could change df.ctx to some mmap'd memory,
> forcing the kernel to execute some code prepared by userspace.

Understood. I have to find a better way. And for this, I'd like to listen
attentively more opinions and advices.

Thanks for comments,
Inki Dae


^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Maarten Lankhorst @ 2013-05-13  9:52 UTC (permalink / raw)
  To: Inki Dae
  Cc: 'Rob Clark', 'Daniel Vetter',
	'DRI mailing list', linux-arm-kernel, linux-media,
	'linux-fbdev', 'Kyungmin Park',
	'myungjoo.ham', 'YoungJun Cho'
In-Reply-To: <025201ce4fbb$363d0390$a2b70ab0$%dae@samsung.com>

Op 13-05-13 11:21, Inki Dae schreef:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
>> Sent: Monday, May 13, 2013 5:01 PM
>> To: Inki Dae
>> Cc: Rob Clark; Daniel Vetter; DRI mailing list; linux-arm-
>> kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-fbdev;
>> Kyungmin Park; myungjoo.ham; YoungJun Cho
>> Subject: Re: Introduce a new helper framework for buffer synchronization
>>
>> Op 09-05-13 09:33, Inki Dae schreef:
>>> Hi all,
>>>
>>> This post introduces a new helper framework based on dma fence. And the
>>> purpose of this post is to collect other opinions and advices before RFC
>>> posting.
>>>
>>> First of all, this helper framework, called fence helper, is in progress
>>> yet so might not have enough comments in codes and also might need to be
>>> more cleaned up. Moreover, we might be missing some parts of the dma
>> fence.
>>> However, I'd like to say that all things mentioned below has been tested
>>> with Linux platform and worked well.
>>> ....
>>>
>>> And tutorial for user process.
>>>         just before cpu access
>>>                 struct dma_buf_fence *df;
>>>
>>>                 df->type = DMA_BUF_ACCESS_READ or DMA_BUF_ACCESS_WRITE;
>>>                 ioctl(fd, DMA_BUF_GET_FENCE, &df);
>>>
>>>         after memset or memcpy
>>>                 ioctl(fd, DMA_BUF_PUT_FENCE, &df);
>> NAK.
>>
>> Userspace doesn't need to trigger fences. It can do a buffer idle wait,
>> and postpone submitting new commands until after it's done using the
>> buffer.
> Hi Maarten,
>
> It seems that you say user should wait for a buffer like KDS does: KDS uses
> select() to postpone submitting new commands. But I think this way assumes
> that every data flows a DMA device to a CPU. For example, a CPU should keep
> polling for the completion of a buffer access by a DMA device. This means
> that the this way isn't considered for data flow to opposite case; CPU to
> DMA device.
Not really. You do both things the same way. You first wait for the bo to be idle, this could be implemented by adding poll support to the dma-buf fd.
Then you either do your read or write. Since userspace is supposed to be the one controlling the bo it should stay idle at that point. If you have another thread queueing
the buffer againbefore your thread is done that's a bug in the application, and can be solved with userspace locking primitives. No need for the kernel to get involved.

>> Kernel space doesn't need the root hole you created by giving a
>> dereferencing a pointer passed from userspace.
>> Your next exercise should be to write a security exploit from the api you
>> created here. It's the only way to learn how to write safe code. Hint:
>> df.ctx = mmap(..);
>>
> Also I'm not clear to use our way yet and that is why I posted. As you
> mentioned, it seems like that using mmap() is more safe. But there is one
> issue it makes me confusing. For your hint, df.ctx = mmap(..), the issue is
> that dmabuf mmap can be used to map a dmabuf with user space. And the dmabuf
> means a physical memory region allocated by some allocator such as drm gem
> or ion.
>
> There might be my missing point so could you please give me more comments?
>
My point was that userspace could change df.ctx to some mmap'd memory, forcing the kernel to execute some code prepared by userspace.

^ permalink raw reply

* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-13  9:21 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>



> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> Sent: Monday, May 13, 2013 5:01 PM
> To: Inki Dae
> Cc: Rob Clark; Daniel Vetter; DRI mailing list; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org; linux-fbdev;
> Kyungmin Park; myungjoo.ham; YoungJun Cho
> Subject: Re: Introduce a new helper framework for buffer synchronization
> 
> Op 09-05-13 09:33, Inki Dae schreef:
> > Hi all,
> >
> > This post introduces a new helper framework based on dma fence. And the
> > purpose of this post is to collect other opinions and advices before RFC
> > posting.
> >
> > First of all, this helper framework, called fence helper, is in progress
> > yet so might not have enough comments in codes and also might need to be
> > more cleaned up. Moreover, we might be missing some parts of the dma
> fence.
> > However, I'd like to say that all things mentioned below has been tested
> > with Linux platform and worked well.
> 
> > ....
> >
> > And tutorial for user process.
> >         just before cpu access
> >                 struct dma_buf_fence *df;
> >
> >                 df->type = DMA_BUF_ACCESS_READ or DMA_BUF_ACCESS_WRITE;
> >                 ioctl(fd, DMA_BUF_GET_FENCE, &df);
> >
> >         after memset or memcpy
> >                 ioctl(fd, DMA_BUF_PUT_FENCE, &df);
> NAK.
> 
> Userspace doesn't need to trigger fences. It can do a buffer idle wait,
> and postpone submitting new commands until after it's done using the
> buffer.

Hi Maarten,

It seems that you say user should wait for a buffer like KDS does: KDS uses
select() to postpone submitting new commands. But I think this way assumes
that every data flows a DMA device to a CPU. For example, a CPU should keep
polling for the completion of a buffer access by a DMA device. This means
that the this way isn't considered for data flow to opposite case; CPU to
DMA device.

> Kernel space doesn't need the root hole you created by giving a
> dereferencing a pointer passed from userspace.
> Your next exercise should be to write a security exploit from the api you
> created here. It's the only way to learn how to write safe code. Hint:
> df.ctx = mmap(..);
> 

Also I'm not clear to use our way yet and that is why I posted. As you
mentioned, it seems like that using mmap() is more safe. But there is one
issue it makes me confusing. For your hint, df.ctx = mmap(..), the issue is
that dmabuf mmap can be used to map a dmabuf with user space. And the dmabuf
means a physical memory region allocated by some allocator such as drm gem
or ion.

There might be my missing point so could you please give me more comments?

Thanks,
Inki Dae



> ~Maarten


^ permalink raw reply

* Re: Introduce a new helper framework for buffer synchronization
From: Maarten Lankhorst @ 2013-05-13  8:00 UTC (permalink / raw)
  To: Inki Dae
  Cc: Rob Clark, Daniel Vetter, DRI mailing list,
	linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org,
	linux-fbdev, Kyungmin Park, myungjoo.ham, YoungJun Cho
In-Reply-To: <CAAQKjZNNw4qddo6bE5OY_CahrqDtqkxdO7Pm9RCguXyj9F4cMQ@mail.gmail.com>

Op 09-05-13 09:33, Inki Dae schreef:
> Hi all,
>
> This post introduces a new helper framework based on dma fence. And the
> purpose of this post is to collect other opinions and advices before RFC
> posting.
>
> First of all, this helper framework, called fence helper, is in progress
> yet so might not have enough comments in codes and also might need to be
> more cleaned up. Moreover, we might be missing some parts of the dma fence.
> However, I'd like to say that all things mentioned below has been tested
> with Linux platform and worked well.

> ....
>
> And tutorial for user process.
>         just before cpu access
>                 struct dma_buf_fence *df;
>
>                 df->type = DMA_BUF_ACCESS_READ or DMA_BUF_ACCESS_WRITE;
>                 ioctl(fd, DMA_BUF_GET_FENCE, &df);
>
>         after memset or memcpy
>                 ioctl(fd, DMA_BUF_PUT_FENCE, &df);
NAK.

Userspace doesn't need to trigger fences. It can do a buffer idle wait, and postpone submitting new commands until after it's done using the buffer.
Kernel space doesn't need the root hole you created by giving a dereferencing a pointer passed from userspace.
Your next exercise should be to write a security exploit from the api you created here. It's the only way to learn how to write safe code. Hint: df.ctx = mmap(..);

~Maarten

^ permalink raw reply

* Re: [RFC 32/42] drivers/video: don't check resource with devm_ioremap_resource
From: Shawn Guo @ 2013-05-13  5:39 UTC (permalink / raw)
  To: Jingoo Han
  Cc: 'Wolfram Sang', linux-kernel,
	'Florian Tobias Schandinat', linux-fbdev,
	'Tomi Valkeinen', Fabio Estevam
In-Reply-To: <002c01ce4e09$218612b0$64923810$@samsung.com>

On Sat, May 11, 2013 at 02:33:56PM +0900, Jingoo Han wrote:
> On Friday, May 10, 2013 5:17 PM, Wolfram Sang wrote:
> > 
> > devm_ioremap_resource does sanity checks on the given resource. No need to
> > duplicate this in the driver.
> > 
> > Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
> 
> CC'ed Tomi Valkeinen, Shawn Guo, Fabio Estevam

Acked-by: Shawn Guo <shawn.guo@linaro.org>


^ permalink raw reply

* Re: [PATCH 05/15] video: mxsfb: remove unnecessary platform_set_drvdata()
From: Shawn Guo @ 2013-05-13  1:22 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <000501ce4e46$0acce340$2066a9c0$@samsung.com>

On Sat, May 11, 2013 at 09:49:57PM +0900, Jingoo Han wrote:
> The driver core clears the driver data to NULL after device_release
> or on probe failure, since commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
> (device-core: Ensure drvdata = NULL when no driver is bound).
> Thus, it is not needed to manually clear the device driver data to NULL.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
>  drivers/video/mxsfb.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> index 21223d4..10210a0 100644
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -986,8 +986,6 @@ static int mxsfb_remove(struct platform_device *pdev)
>  
>  	framebuffer_release(fb_info);
>  
> -	platform_set_drvdata(pdev, NULL);
> -
>  	return 0;
>  }
>  
> -- 
> 1.7.2.5
> 
> 


^ permalink raw reply

* Re: [PATCH 12/15] video: sh7760fb: remove unnecessary platform_set_drvdata()
From: Kuninori Morimoto @ 2013-05-13  0:49 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <000c01ce4e47$03d37f80$0b7a7e80$@samsung.com>


Hi

> The driver core clears the driver data to NULL after device_release
> or on probe failure, since commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
> (device-core: Ensure drvdata = NULL when no driver is bound).
> Thus, it is not needed to manually clear the device driver data to NULL.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

^ permalink raw reply

* Re: [PATCH 08/15] video: sh_mipi_dsi: remove unnecessary platform_set_drvdata()
From: Kuninori Morimoto @ 2013-05-13  0:49 UTC (permalink / raw)
  To: linux-fbdev
In-Reply-To: <000801ce4e46$74893770$5d9ba650$@samsung.com>


Hi

> The driver core clears the driver data to NULL after device_release
> or on probe failure, since commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
> (device-core: Ensure drvdata = NULL when no driver is bound).
> Thus, it is not needed to manually clear the device driver data to NULL.
> 
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Best regards
---
Kuninori Morimoto

^ permalink raw reply

* [PATCH] video: jz4740-fb: Use clk_prepare_enable/clk_disable_unprepare
From: Lars-Peter Clausen @ 2013-05-12 18:14 UTC (permalink / raw)
  To: linux-fbdev

In preparation to switching the jz4740 clk driver to the common clk framework
update the clk enable/disable calls to clk_prepare_enable/clk_disable_unprepare.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/video/jz4740_fb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/video/jz4740_fb.c b/drivers/video/jz4740_fb.c
index 36979b4..5ed6ffc 100644
--- a/drivers/video/jz4740_fb.c
+++ b/drivers/video/jz4740_fb.c
@@ -471,7 +471,7 @@ static int jzfb_set_par(struct fb_info *info)
 	writel(ctrl, jzfb->base + JZ_REG_LCD_CTRL);
 
 	if (!jzfb->is_enabled)
-		clk_disable(jzfb->ldclk);
+		clk_disable_unprepare(jzfb->ldclk);
 
 	mutex_unlock(&jzfb->lock);
 
@@ -485,7 +485,7 @@ static void jzfb_enable(struct jzfb *jzfb)
 {
 	uint32_t ctrl;
 
-	clk_enable(jzfb->ldclk);
+	clk_prepare_enable(jzfb->ldclk);
 
 	jz_gpio_bulk_resume(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
 	jz_gpio_bulk_resume(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
@@ -514,7 +514,7 @@ static void jzfb_disable(struct jzfb *jzfb)
 	jz_gpio_bulk_suspend(jz_lcd_ctrl_pins, jzfb_num_ctrl_pins(jzfb));
 	jz_gpio_bulk_suspend(jz_lcd_data_pins, jzfb_num_data_pins(jzfb));
 
-	clk_disable(jzfb->ldclk);
+	clk_disable_unprepare(jzfb->ldclk);
 }
 
 static int jzfb_blank(int blank_mode, struct fb_info *info)
@@ -693,7 +693,7 @@ static int jzfb_probe(struct platform_device *pdev)
 
 	fb_alloc_cmap(&fb->cmap, 256, 0);
 
-	clk_enable(jzfb->ldclk);
+	clk_prepare_enable(jzfb->ldclk);
 	jzfb->is_enabled = 1;
 
 	writel(jzfb->framedesc->next, jzfb->base + JZ_REG_LCD_DA0);
@@ -765,7 +765,7 @@ static int jzfb_suspend(struct device *dev)
 static int jzfb_resume(struct device *dev)
 {
 	struct jzfb *jzfb = dev_get_drvdata(dev);
-	clk_enable(jzfb->ldclk);
+	clk_prepare_enable(jzfb->ldclk);
 
 	mutex_lock(&jzfb->lock);
 	if (jzfb->is_enabled)
-- 
1.8.2.1


^ permalink raw reply related

* [PATCH v6, part3 07/16] mm, acornfb: use free_reserved_area() to simplify code
From: Jiang Liu @ 2013-05-11 17:34 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jiang Liu, David Rientjes, Wen Congyang, Mel Gorman, Minchan Kim,
	KAMEZAWA Hiroyuki, Michal Hocko, James Bottomley, Sergei Shtylyov,
	David Howells, Mark Salter, Jianguo Wu, linux-mm, linux-arch,
	linux-kernel, Florian Tobias Schandinat, linux-fbdev
In-Reply-To: <1368293689-16410-1-git-send-email-jiang.liu@huawei.com>

Use common help function free_reserved_area() to simplify code.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/video/acornfb.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/video/acornfb.c b/drivers/video/acornfb.c
index 6488a73..344f2bb 100644
--- a/drivers/video/acornfb.c
+++ b/drivers/video/acornfb.c
@@ -1188,32 +1188,8 @@ static int acornfb_detect_monitortype(void)
 static inline void
 free_unused_pages(unsigned int virtual_start, unsigned int virtual_end)
 {
-	int mb_freed = 0;
-
-	/*
-	 * Align addresses
-	 */
-	virtual_start = PAGE_ALIGN(virtual_start);
-	virtual_end = PAGE_ALIGN(virtual_end);
-
-	while (virtual_start < virtual_end) {
-		struct page *page;
-
-		/*
-		 * Clear page reserved bit,
-		 * set count to 1, and free
-		 * the page.
-		 */
-		page = virt_to_page(virtual_start);
-		ClearPageReserved(page);
-		init_page_count(page);
-		free_page(virtual_start);
-
-		virtual_start += PAGE_SIZE;
-		mb_freed += PAGE_SIZE / 1024;
-	}
-
-	printk("acornfb: freed %dK memory\n", mb_freed);
+	free_reserved_area(virtual_start, PAGE_ALIGN(virtual_end),
+			   -1, "acornfb");
 }
 
 static int acornfb_probe(struct platform_device *dev)
-- 
1.8.1.2


^ permalink raw reply related

* [PATCH 15/15] video: mmp: remove unnecessary platform_set_drvdata()
From: Jingoo Han @ 2013-05-11 13:03 UTC (permalink / raw)
  To: linux-fbdev

The driver core clears the driver data to NULL after device_release
or on probe failure, since commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
(device-core: Ensure drvdata = NULL when no driver is bound).
Thus, it is not needed to manually clear the device driver data to NULL.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/mmp/fb/mmpfb.c    |    1 -
 drivers/video/mmp/hw/mmp_ctrl.c |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/video/mmp/fb/mmpfb.c b/drivers/video/mmp/fb/mmpfb.c
index 6d1fa96..4ab95b8 100644
--- a/drivers/video/mmp/fb/mmpfb.c
+++ b/drivers/video/mmp/fb/mmpfb.c
@@ -659,7 +659,6 @@ failed_destroy_mutex:
 	mutex_destroy(&fbi->access_ok);
 failed:
 	dev_err(fbi->dev, "mmp-fb: frame buffer device init failed\n");
-	platform_set_drvdata(pdev, NULL);
 
 	framebuffer_release(info);
 
diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index 4bd31b2..c46bf5a 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -566,7 +566,6 @@ failed:
 		devm_kfree(ctrl->dev, ctrl);
 	}
 
-	platform_set_drvdata(pdev, NULL);
 	dev_err(&pdev->dev, "device init failed\n");
 
 	return ret;
-- 
1.7.2.5



^ permalink raw reply related

* [PATCH 14/15] video: vga16fb: remove unnecessary platform_set_drvdata()
From: Jingoo Han @ 2013-05-11 13:00 UTC (permalink / raw)
  To: linux-fbdev

The driver core clears the driver data to NULL after device_release
or on probe failure, since commit 0998d0631001288a5974afc0b2a5f568bcdecb4d
(device-core: Ensure drvdata = NULL when no driver is bound).
Thus, it is not needed to manually clear the device driver data to NULL.

Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
 drivers/video/vga16fb.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 545faec..830ded4 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1269,7 +1269,6 @@ static void vga16fb_destroy(struct fb_info *info)
 	iounmap(info->screen_base);
 	fb_dealloc_cmap(&info->cmap);
 	/* XXX unshare VGA regions */
-	platform_set_drvdata(dev, NULL);
 	framebuffer_release(info);
 }
 
-- 
1.7.2.5



^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox