2003-05-10 09:57:05

by David Mosberger

[permalink] [raw]
Subject: Improved DRM support for cant_use_aperture platforms

Hi Dave,

This patch is rather big, but actually very straight-forward: it adds
a "agp dev" argument to DRM_IOREMAP(), DRM_IOREMAP_NOCACHE(), and
DRM_IOREMAPFREE() and then uses it in drm_memory.h to support
platforms where CPU accesses to the AGP space are not translated by
the GART (true for ia64 and alpha, not true for x86, I don't know
about the other platforms). On platforms where cant_use_aperture is
always false, this whole patch will look like a no-op. On ia64, it
works. Don't know about other platforms, but it should simplify
things for Alpha at least (and if it breaks a platform, I shall be
happy to work with the respective maintainer to get fix back to
working).

Thanks,

--david

diff -Nru a/drivers/char/drm/drmP.h b/drivers/char/drm/drmP.h
--- a/drivers/char/drm/drmP.h Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/drmP.h Sat May 10 01:47:43 2003
@@ -225,16 +225,16 @@
if (len > DRM_PROC_LIMIT) { ret; *eof = 1; return len - offset; }

/* Mapping helper macros */
-#define DRM_IOREMAP(map) \
- (map)->handle = DRM(ioremap)( (map)->offset, (map)->size )
+#define DRM_IOREMAP(map, dev) \
+ (map)->handle = DRM(ioremap)( (map)->offset, (map)->size, (dev) )

-#define DRM_IOREMAP_NOCACHE(map) \
- (map)->handle = DRM(ioremap_nocache)((map)->offset, (map)->size)
+#define DRM_IOREMAP_NOCACHE(map, dev) \
+ (map)->handle = DRM(ioremap_nocache)((map)->offset, (map)->size, (dev))

-#define DRM_IOREMAPFREE(map) \
- do { \
- if ( (map)->handle && (map)->size ) \
- DRM(ioremapfree)( (map)->handle, (map)->size ); \
+#define DRM_IOREMAPFREE(map, dev) \
+ do { \
+ if ( (map)->handle && (map)->size ) \
+ DRM(ioremapfree)( (map)->handle, (map)->size, (dev) ); \
} while (0)

#define DRM_FIND_MAP(_map, _o) \
@@ -652,9 +652,10 @@
extern unsigned long DRM(alloc_pages)(int order, int area);
extern void DRM(free_pages)(unsigned long address, int order,
int area);
-extern void *DRM(ioremap)(unsigned long offset, unsigned long size);
-extern void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size);
-extern void DRM(ioremapfree)(void *pt, unsigned long size);
+extern void *DRM(ioremap)(unsigned long offset, unsigned long size, drm_device_t *dev);
+extern void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size,
+ drm_device_t *dev);
+extern void DRM(ioremapfree)(void *pt, unsigned long size, drm_device_t *dev);

#if __REALLY_HAVE_AGP
extern agp_memory *DRM(alloc_agp)(int pages, u32 type);
diff -Nru a/drivers/char/drm/drm_bufs.h b/drivers/char/drm/drm_bufs.h
--- a/drivers/char/drm/drm_bufs.h Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/drm_bufs.h Sat May 10 01:47:43 2003
@@ -123,7 +123,7 @@
MTRR_TYPE_WRCOMB, 1 );
}
#endif
- map->handle = DRM(ioremap)( map->offset, map->size );
+ map->handle = DRM(ioremap)( map->offset, map->size, dev );
break;

case _DRM_SHM:
@@ -245,7 +245,7 @@
DRM_DEBUG("mtrr_del = %d\n", retcode);
}
#endif
- DRM(ioremapfree)(map->handle, map->size);
+ DRM(ioremapfree)(map->handle, map->size, dev);
break;
case _DRM_SHM:
vfree(map->handle);
diff -Nru a/drivers/char/drm/drm_drv.h b/drivers/char/drm/drm_drv.h
--- a/drivers/char/drm/drm_drv.h Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/drm_drv.h Sat May 10 01:47:43 2003
@@ -454,7 +454,7 @@
DRM_DEBUG( "mtrr_del=%d\n", retcode );
}
#endif
- DRM(ioremapfree)( map->handle, map->size );
+ DRM(ioremapfree)( map->handle, map->size, dev );
break;
case _DRM_SHM:
vfree(map->handle);
diff -Nru a/drivers/char/drm/drm_memory.h b/drivers/char/drm/drm_memory.h
--- a/drivers/char/drm/drm_memory.h Sat May 10 01:47:42 2003
+++ b/drivers/char/drm/drm_memory.h Sat May 10 01:47:43 2003
@@ -31,6 +31,10 @@

#include <linux/config.h>
#include "drmP.h"
+#include <linux/vmalloc.h>
+
+#include <asm/agp.h>
+#include <asm/tlbflush.h>

/* Cut down version of drm_memory_debug.h, which used to be called
* drm_memory.h. If you want the debug functionality, change 0 to 1
@@ -38,6 +42,150 @@
*/
#define DEBUG_MEMORY 0

+#if __REALLY_HAVE_AGP
+
+/*
+ * Find the drm_map that covers the range [offset, offset+size).
+ */
+static inline drm_map_t *
+drm_lookup_map (unsigned long offset, unsigned long size, drm_device_t *dev)
+{
+ struct list_head *list;
+ drm_map_list_t *r_list;
+ drm_map_t *map;
+
+ list_for_each(list, &dev->maplist->head) {
+ r_list = (drm_map_list_t *) list;
+ map = r_list->map;
+ if (!map)
+ continue;
+ if (map->offset <= offset && (offset + size) <= (map->offset + map->size))
+ return map;
+ }
+ return NULL;
+}
+
+static inline void *
+agp_remap (unsigned long offset, unsigned long size, drm_device_t *dev)
+{
+ unsigned long *phys_addr_map, i, num_pages = PAGE_ALIGN(size) / PAGE_SIZE;
+ struct drm_agp_mem *agpmem;
+ struct page **page_map;
+ void *addr;
+
+ size = PAGE_ALIGN(size);
+
+ for (agpmem = dev->agp->memory; agpmem; agpmem = agpmem->next)
+ if (agpmem->bound <= offset
+ && (agpmem->bound + (agpmem->pages << PAGE_SHIFT)) >= (offset + size))
+ break;
+ if (!agpmem)
+ return NULL;
+
+ /*
+ * OK, we're mapping AGP space on a chipset/platform on which memory accesses by
+ * the CPU do not get remapped by the GART. We fix this by using the kernel's
+ * page-table instead (that's probably faster anyhow...).
+ */
+ /* note: use vmalloc() because num_pages could be large... */
+ page_map = vmalloc(num_pages * sizeof(struct page *));
+ if (!page_map)
+ return NULL;
+
+ phys_addr_map = agpmem->memory->memory + (offset - agpmem->bound) / PAGE_SIZE;
+ for (i = 0; i < num_pages; ++i)
+ page_map[i] = pfn_to_page(phys_addr_map[i] >> PAGE_SHIFT);
+ addr = vmap(page_map, num_pages, VM_IOREMAP, PAGE_AGP);
+ vfree(page_map);
+ if (!addr)
+ return NULL;
+
+ flush_tlb_kernel_range((unsigned long) addr, (unsigned long) addr + size);
+ return addr;
+}
+
+static inline unsigned long
+drm_follow_page (void *vaddr)
+{
+ pgd_t *pgd = pgd_offset_k((unsigned long) vaddr);
+ pmd_t *pmd = pmd_offset(pgd, (unsigned long) vaddr);
+ pte_t *ptep = pte_offset_kernel(pmd, (unsigned long) vaddr);
+ return pte_pfn(*ptep) << PAGE_SHIFT;
+}
+
+#else /* !__REALLY_HAVE_AGP */
+
+static inline void *
+agp_remap (unsigned long offset, unsigned long size, drm_device_t *dev)
+{
+ return NULL;
+}
+
+#endif /* !__REALLY_HAVE_AGP */
+
+static inline void *drm_ioremap(unsigned long offset, unsigned long size, drm_device_t *dev)
+{
+ int remap_aperture = 0;
+
+#if __REALLY_HAVE_AGP
+ if (dev->agp->cant_use_aperture) {
+ drm_map_t *map = drm_lookup_map(offset, size, dev);
+
+ if (map && map->type == _DRM_AGP)
+ remap_aperture = 1;
+ }
+#endif
+ if (remap_aperture)
+ return agp_remap(offset, size, dev);
+ else
+ return ioremap(offset, size);
+}
+
+static inline void *drm_ioremap_nocache(unsigned long offset, unsigned long size,
+ drm_device_t *dev)
+{
+ int remap_aperture = 0;
+
+#if __REALLY_HAVE_AGP
+ if (dev->agp->cant_use_aperture) {
+ drm_map_t *map = drm_lookup_map(offset, size, dev);
+
+ if (map && map->type == _DRM_AGP)
+ remap_aperture = 1;
+ }
+#endif
+ if (remap_aperture)
+ return agp_remap(offset, size, dev);
+ else
+ return ioremap_nocache(offset, size);
+}
+
+static inline void drm_ioremapfree(void *pt, unsigned long size, drm_device_t *dev)
+{
+ int unmap_aperture = 0;
+#if __REALLY_HAVE_AGP
+ /*
+ * This is a bit ugly. It would be much cleaner if the DRM API would use separate
+ * routines for handling mappings in the AGP space. Hopefully this can be done in
+ * a future revision of the interface...
+ */
+ if (dev->agp->cant_use_aperture
+ && ((unsigned long) pt >= VMALLOC_START && (unsigned long) pt < VMALLOC_END))
+ {
+ unsigned long offset;
+ drm_map_t *map;
+
+ offset = drm_follow_page(pt) | ((unsigned long) pt & ~PAGE_MASK);
+ map = drm_lookup_map(offset, size, dev);
+ if (map && map->type == _DRM_AGP)
+ unmap_aperture = 1;
+ }
+#endif
+ if (unmap_aperture)
+ vunmap(pt);
+ else
+ iounmap(pt);
+}

#if DEBUG_MEMORY
#include "drm_memory_debug.h"
@@ -118,19 +266,19 @@
free_pages(address, order);
}

-void *DRM(ioremap)(unsigned long offset, unsigned long size)
+void *DRM(ioremap)(unsigned long offset, unsigned long size, drm_device_t *dev)
{
- return ioremap(offset, size);
+ return drm_ioremap(offset, size, dev);
}

-void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size)
+void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size, drm_device_t *dev)
{
- return ioremap_nocache(offset, size);
+ return drm_ioremap_nocache(offset, size, dev);
}

-void DRM(ioremapfree)(void *pt, unsigned long size)
+void DRM(ioremapfree)(void *pt, unsigned long size, drm_device_t *dev)
{
- iounmap(pt);
+ drm_ioremapfree(pt, size, dev);
}

#if __REALLY_HAVE_AGP
diff -Nru a/drivers/char/drm/drm_memory_debug.h b/drivers/char/drm/drm_memory_debug.h
--- a/drivers/char/drm/drm_memory_debug.h Sat May 10 01:47:42 2003
+++ b/drivers/char/drm/drm_memory_debug.h Sat May 10 01:47:42 2003
@@ -269,7 +269,7 @@
}
}

-void *DRM(ioremap)(unsigned long offset, unsigned long size)
+void *DRM(ioremap)(unsigned long offset, unsigned long size, drm_device_t *dev)
{
void *pt;

@@ -279,7 +279,7 @@
return NULL;
}

- if (!(pt = ioremap(offset, size))) {
+ if (!(pt = drm_ioremap(offset, size, dev))) {
spin_lock(&DRM(mem_lock));
++DRM(mem_stats)[DRM_MEM_MAPPINGS].fail_count;
spin_unlock(&DRM(mem_lock));
@@ -292,7 +292,7 @@
return pt;
}

-void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size)
+void *DRM(ioremap_nocache)(unsigned long offset, unsigned long size, drm_device_t *dev)
{
void *pt;

@@ -302,7 +302,7 @@
return NULL;
}

- if (!(pt = ioremap_nocache(offset, size))) {
+ if (!(pt = drm_ioremap_nocache(offset, size, dev))) {
spin_lock(&DRM(mem_lock));
++DRM(mem_stats)[DRM_MEM_MAPPINGS].fail_count;
spin_unlock(&DRM(mem_lock));
@@ -315,7 +315,7 @@
return pt;
}

-void DRM(ioremapfree)(void *pt, unsigned long size)
+void DRM(ioremapfree)(void *pt, unsigned long size, drm_device_t *dev)
{
int alloc_count;
int free_count;
@@ -324,7 +324,7 @@
DRM_MEM_ERROR(DRM_MEM_MAPPINGS,
"Attempt to free NULL pointer\n");
else
- iounmap(pt);
+ drm_ioremapfree(pt, size, dev);

spin_lock(&DRM(mem_lock));
DRM(mem_stats)[DRM_MEM_MAPPINGS].bytes_freed += size;
diff -Nru a/drivers/char/drm/drm_vm.h b/drivers/char/drm/drm_vm.h
--- a/drivers/char/drm/drm_vm.h Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/drm_vm.h Sat May 10 01:47:43 2003
@@ -107,12 +107,12 @@
* Get the page, inc the use count, and return it
*/
offset = (baddr - agpmem->bound) >> PAGE_SHIFT;
- agpmem->memory->memory[offset] &= dev->agp->page_mask;
page = virt_to_page(__va(agpmem->memory->memory[offset]));
get_page(page);

- DRM_DEBUG("baddr = 0x%lx page = 0x%p, offset = 0x%lx\n",
- baddr, __va(agpmem->memory->memory[offset]), offset);
+ DRM_DEBUG("baddr = 0x%lx page = 0x%p, offset = 0x%lx, count=%d\n",
+ baddr, __va(agpmem->memory->memory[offset]), offset,
+ atomic_read(&page->count));

return page;
}
@@ -206,7 +206,7 @@
DRM_DEBUG("mtrr_del = %d\n", retcode);
}
#endif
- DRM(ioremapfree)(map->handle, map->size);
+ DRM(ioremapfree)(map->handle, map->size, dev);
break;
case _DRM_SHM:
vfree(map->handle);
@@ -420,15 +420,16 @@

switch (map->type) {
case _DRM_AGP:
-#if defined(__alpha__)
+#if __REALLY_HAVE_AGP
+ if (dev->agp->cant_use_aperture) {
/*
- * On Alpha we can't talk to bus dma address from the
- * CPU, so for memory of type DRM_AGP, we'll deal with
- * sorting out the real physical pages and mappings
- * in nopage()
+ * On some platforms we can't talk to bus dma address from the CPU, so for
+ * memory of type DRM_AGP, we'll deal with sorting out the real physical
+ * pages and mappings in nopage()
*/
vma->vm_ops = &DRM(vm_ops);
break;
+ }
#endif
/* fall through to _DRM_FRAME_BUFFER... */
case _DRM_FRAME_BUFFER:
@@ -439,15 +440,15 @@
pgprot_val(vma->vm_page_prot) |= _PAGE_PCD;
pgprot_val(vma->vm_page_prot) &= ~_PAGE_PWT;
}
-#elif defined(__ia64__)
- if (map->type != _DRM_AGP)
- vma->vm_page_prot =
- pgprot_writecombine(vma->vm_page_prot);
#elif defined(__powerpc__)
pgprot_val(vma->vm_page_prot) |= _PAGE_NO_CACHE | _PAGE_GUARDED;
#endif
vma->vm_flags |= VM_IO; /* not in core dump */
}
+#if defined(__ia64__)
+ if (map->type != _DRM_AGP)
+ vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
+#endif
offset = DRIVER_GET_REG_OFS();
#ifdef __sparc__
if (io_remap_page_range(DRM_RPR_ARG(vma) vma->vm_start,
diff -Nru a/drivers/char/drm/gamma_dma.c b/drivers/char/drm/gamma_dma.c
--- a/drivers/char/drm/gamma_dma.c Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/gamma_dma.c Sat May 10 01:47:43 2003
@@ -612,7 +612,7 @@
} else {
DRM_FIND_MAP( dev_priv->buffers, init->buffers_offset );

- DRM_IOREMAP( dev_priv->buffers );
+ DRM_IOREMAP( dev_priv->buffers, dev );

buf = dma->buflist[GLINT_DRI_BUF_COUNT];
pgt = buf->address;
@@ -651,7 +651,7 @@
drm_gamma_private_t *dev_priv = dev->dev_private;

if ( dev_priv->buffers != NULL )
- DRM_IOREMAPFREE( dev_priv->buffers );
+ DRM_IOREMAPFREE( dev_priv->buffers, dev );

DRM(free)( dev->dev_private, sizeof(drm_gamma_private_t),
DRM_MEM_DRIVER );
diff -Nru a/drivers/char/drm/i810_dma.c b/drivers/char/drm/i810_dma.c
--- a/drivers/char/drm/i810_dma.c Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/i810_dma.c Sat May 10 01:47:43 2003
@@ -246,7 +246,7 @@

if(dev_priv->ring.virtual_start) {
DRM(ioremapfree)((void *) dev_priv->ring.virtual_start,
- dev_priv->ring.Size);
+ dev_priv->ring.Size, dev);
}
if (dev_priv->hw_status_page) {
pci_free_consistent(dev->pdev, PAGE_SIZE,
@@ -263,7 +263,7 @@
drm_buf_t *buf = dma->buflist[ i ];
drm_i810_buf_priv_t *buf_priv = buf->dev_private;
if ( buf_priv->kernel_virtual && buf->total )
- DRM(ioremapfree)(buf_priv->kernel_virtual, buf->total);
+ DRM(ioremapfree)(buf_priv->kernel_virtual, buf->total, dev);
}
}
return 0;
@@ -333,7 +333,7 @@
*buf_priv->in_use = I810_BUF_FREE;

buf_priv->kernel_virtual = DRM(ioremap)(buf->bus_address,
- buf->total);
+ buf->total, dev);
}
return 0;
}
@@ -386,7 +386,7 @@

dev_priv->ring.virtual_start = DRM(ioremap)(dev->agp->base +
init->ring_start,
- init->ring_size);
+ init->ring_size, dev);

if (dev_priv->ring.virtual_start == NULL) {
dev->dev_private = (void *) dev_priv;
diff -Nru a/drivers/char/drm/i830_dma.c b/drivers/char/drm/i830_dma.c
--- a/drivers/char/drm/i830_dma.c Sat May 10 01:47:42 2003
+++ b/drivers/char/drm/i830_dma.c Sat May 10 01:47:42 2003
@@ -246,7 +246,7 @@

if (dev_priv->ring.virtual_start) {
DRM(ioremapfree)((void *) dev_priv->ring.virtual_start,
- dev_priv->ring.Size);
+ dev_priv->ring.Size, dev);
}
if (dev_priv->hw_status_page) {
pci_free_consistent(dev->pdev, PAGE_SIZE,
@@ -264,7 +264,7 @@
drm_buf_t *buf = dma->buflist[ i ];
drm_i830_buf_priv_t *buf_priv = buf->dev_private;
if ( buf_priv->kernel_virtual && buf->total )
- DRM(ioremapfree)(buf_priv->kernel_virtual, buf->total);
+ DRM(ioremapfree)(buf_priv->kernel_virtual, buf->total, dev);
}
}
return 0;
@@ -340,7 +340,7 @@
*buf_priv->in_use = I830_BUF_FREE;

buf_priv->kernel_virtual = DRM(ioremap)(buf->bus_address,
- buf->total);
+ buf->total, dev);
}
return 0;
}
@@ -394,7 +394,7 @@

dev_priv->ring.virtual_start = DRM(ioremap)(dev->agp->base +
init->ring_start,
- init->ring_size);
+ init->ring_size, dev);

if (dev_priv->ring.virtual_start == NULL) {
dev->dev_private = (void *) dev_priv;
diff -Nru a/drivers/char/drm/mga_dma.c b/drivers/char/drm/mga_dma.c
--- a/drivers/char/drm/mga_dma.c Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/mga_dma.c Sat May 10 01:47:43 2003
@@ -554,9 +554,9 @@
(drm_mga_sarea_t *)((u8 *)dev_priv->sarea->handle +
init->sarea_priv_offset);

- DRM_IOREMAP( dev_priv->warp );
- DRM_IOREMAP( dev_priv->primary );
- DRM_IOREMAP( dev_priv->buffers );
+ DRM_IOREMAP( dev_priv->warp, dev );
+ DRM_IOREMAP( dev_priv->primary, dev );
+ DRM_IOREMAP( dev_priv->buffers, dev );

if(!dev_priv->warp->handle ||
!dev_priv->primary->handle ||
@@ -651,11 +651,11 @@
drm_mga_private_t *dev_priv = dev->dev_private;

if ( dev_priv->warp != NULL )
- DRM_IOREMAPFREE( dev_priv->warp );
+ DRM_IOREMAPFREE( dev_priv->warp, dev );
if ( dev_priv->primary != NULL )
- DRM_IOREMAPFREE( dev_priv->primary );
+ DRM_IOREMAPFREE( dev_priv->primary, dev );
if ( dev_priv->buffers != NULL )
- DRM_IOREMAPFREE( dev_priv->buffers );
+ DRM_IOREMAPFREE( dev_priv->buffers, dev );

if ( dev_priv->head != NULL ) {
mga_freelist_cleanup( dev );
diff -Nru a/drivers/char/drm/mga_drv.h b/drivers/char/drm/mga_drv.h
--- a/drivers/char/drm/mga_drv.h Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/mga_drv.h Sat May 10 01:47:43 2003
@@ -226,7 +226,7 @@
if ( MGA_VERBOSE ) { \
DRM_INFO( "BEGIN_DMA( %d ) in %s\n", \
(n), __FUNCTION__ ); \
- DRM_INFO( " space=0x%x req=0x%x\n", \
+ DRM_INFO( " space=0x%x req=0x%Zx\n", \
dev_priv->prim.space, (n) * DMA_BLOCK_SIZE ); \
} \
prim = dev_priv->prim.start; \
@@ -276,7 +276,7 @@
#define DMA_WRITE( offset, val ) \
do { \
if ( MGA_VERBOSE ) { \
- DRM_INFO( " DMA_WRITE( 0x%08x ) at 0x%04x\n", \
+ DRM_INFO( " DMA_WRITE( 0x%08x ) at 0x%04Zx\n", \
(u32)(val), write + (offset) * sizeof(u32) ); \
} \
*(volatile u32 *)(prim + write + (offset) * sizeof(u32)) = val; \
diff -Nru a/drivers/char/drm/r128_cce.c b/drivers/char/drm/r128_cce.c
--- a/drivers/char/drm/r128_cce.c Sat May 10 01:47:42 2003
+++ b/drivers/char/drm/r128_cce.c Sat May 10 01:47:42 2003
@@ -350,8 +350,8 @@

R128_WRITE( R128_PM4_BUFFER_DL_RPTR_ADDR,
entry->busaddr[page_ofs]);
- DRM_DEBUG( "ring rptr: offset=0x%08x handle=0x%08lx\n",
- entry->busaddr[page_ofs],
+ DRM_DEBUG( "ring rptr: offset=0x%08lx handle=0x%08lx\n",
+ (unsigned long) entry->busaddr[page_ofs],
entry->handle + tmp_ofs );
}

@@ -540,9 +540,9 @@
init->sarea_priv_offset);

if ( !dev_priv->is_pci ) {
- DRM_IOREMAP( dev_priv->cce_ring );
- DRM_IOREMAP( dev_priv->ring_rptr );
- DRM_IOREMAP( dev_priv->buffers );
+ DRM_IOREMAP( dev_priv->cce_ring, dev );
+ DRM_IOREMAP( dev_priv->ring_rptr, dev );
+ DRM_IOREMAP( dev_priv->buffers, dev );
if(!dev_priv->cce_ring->handle ||
!dev_priv->ring_rptr->handle ||
!dev_priv->buffers->handle) {
@@ -629,11 +629,11 @@
if ( !dev_priv->is_pci ) {
#endif
if ( dev_priv->cce_ring != NULL )
- DRM_IOREMAPFREE( dev_priv->cce_ring );
+ DRM_IOREMAPFREE( dev_priv->cce_ring, dev );
if ( dev_priv->ring_rptr != NULL )
- DRM_IOREMAPFREE( dev_priv->ring_rptr );
+ DRM_IOREMAPFREE( dev_priv->ring_rptr, dev );
if ( dev_priv->buffers != NULL )
- DRM_IOREMAPFREE( dev_priv->buffers );
+ DRM_IOREMAPFREE( dev_priv->buffers, dev );
#if __REALLY_HAVE_SG
} else {
if (!DRM(ati_pcigart_cleanup)( dev,
diff -Nru a/drivers/char/drm/radeon_cp.c b/drivers/char/drm/radeon_cp.c
--- a/drivers/char/drm/radeon_cp.c Sat May 10 01:47:43 2003
+++ b/drivers/char/drm/radeon_cp.c Sat May 10 01:47:43 2003
@@ -903,8 +903,8 @@

RADEON_WRITE( RADEON_CP_RB_RPTR_ADDR,
entry->busaddr[page_ofs]);
- DRM_DEBUG( "ring rptr: offset=0x%08x handle=0x%08lx\n",
- entry->busaddr[page_ofs],
+ DRM_DEBUG( "ring rptr: offset=0x%08lx handle=0x%08lx\n",
+ (unsigned long) entry->busaddr[page_ofs],
entry->handle + tmp_ofs );
}

@@ -1152,9 +1152,9 @@
init->sarea_priv_offset);

if ( !dev_priv->is_pci ) {
- DRM_IOREMAP( dev_priv->cp_ring );
- DRM_IOREMAP( dev_priv->ring_rptr );
- DRM_IOREMAP( dev_priv->buffers );
+ DRM_IOREMAP( dev_priv->cp_ring, dev );
+ DRM_IOREMAP( dev_priv->ring_rptr, dev );
+ DRM_IOREMAP( dev_priv->buffers, dev );
if(!dev_priv->cp_ring->handle ||
!dev_priv->ring_rptr->handle ||
!dev_priv->buffers->handle) {
@@ -1279,11 +1279,11 @@

if ( !dev_priv->is_pci ) {
if ( dev_priv->cp_ring != NULL )
- DRM_IOREMAPFREE( dev_priv->cp_ring );
+ DRM_IOREMAPFREE( dev_priv->cp_ring, dev );
if ( dev_priv->ring_rptr != NULL )
- DRM_IOREMAPFREE( dev_priv->ring_rptr );
+ DRM_IOREMAPFREE( dev_priv->ring_rptr, dev );
if ( dev_priv->buffers != NULL )
- DRM_IOREMAPFREE( dev_priv->buffers );
+ DRM_IOREMAPFREE( dev_priv->buffers, dev );
} else {
#if __REALLY_HAVE_SG
if (!DRM(ati_pcigart_cleanup)( dev,
diff -Nru a/include/asm-alpha/agp.h b/include/asm-alpha/agp.h
--- a/include/asm-alpha/agp.h Sat May 10 01:47:42 2003
+++ b/include/asm-alpha/agp.h Sat May 10 01:47:42 2003
@@ -10,4 +10,11 @@
#define flush_agp_mappings()
#define flush_agp_cache() mb()

+/*
+ * Page-protection value to be used for AGP memory mapped into kernel space. For
+ * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
+ * be an uncached mapping (such as write-combining).
+ */
+#define PAGE_AGP PAGE_KERNEL_NOCACHE /* XXX fix me */
+
#endif
diff -Nru a/include/asm-i386/agp.h b/include/asm-i386/agp.h
--- a/include/asm-i386/agp.h Sat May 10 01:47:42 2003
+++ b/include/asm-i386/agp.h Sat May 10 01:47:42 2003
@@ -20,4 +20,11 @@
worth it. Would need a page for it. */
#define flush_agp_cache() asm volatile("wbinvd":::"memory")

+/*
+ * Page-protection value to be used for AGP memory mapped into kernel space. For
+ * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
+ * be an uncached mapping (such as write-combining).
+ */
+#define PAGE_AGP PAGE_KERNEL_NOCACHE
+
#endif
diff -Nru a/include/asm-sparc64/agp.h b/include/asm-sparc64/agp.h
--- a/include/asm-sparc64/agp.h Sat May 10 01:47:42 2003
+++ b/include/asm-sparc64/agp.h Sat May 10 01:47:42 2003
@@ -8,4 +8,11 @@
#define flush_agp_mappings()
#define flush_agp_cache() mb()

+/*
+ * Page-protection value to be used for AGP memory mapped into kernel space. For
+ * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
+ * be an uncached mapping (such as write-combining).
+ */
+#define PAGE_AGP PAGE_KERNEL_NOCACHE
+
#endif
diff -Nru a/include/asm-x86_64/agp.h b/include/asm-x86_64/agp.h
--- a/include/asm-x86_64/agp.h Sat May 10 01:47:42 2003
+++ b/include/asm-x86_64/agp.h Sat May 10 01:47:42 2003
@@ -20,4 +20,11 @@
worth it. Would need a page for it. */
#define flush_agp_cache() asm volatile("wbinvd":::"memory")

+/*
+ * Page-protection value to be used for AGP memory mapped into kernel space. For
+ * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
+ * be an uncached mapping (such as write-combining).
+ */
+#define PAGE_AGP PAGE_KERNEL_NOCACHE
+
#endif


2003-05-10 12:59:52

by Dave Jones

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Sat, May 10, 2003 at 03:09:16AM -0700, David Mosberger wrote:
> Hi Dave,
>
> This patch is rather big, but actually very straight-forward: it adds
> a "agp dev" argument to DRM_IOREMAP(), DRM_IOREMAP_NOCACHE(), and
> DRM_IOREMAPFREE() and then uses it in drm_memory.h to support
> platforms where CPU accesses to the AGP space are not translated by
> the GART

That's one to run by the [email protected] folks.

Dave

2003-05-11 11:31:02

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Sam, 2003-05-10 at 12:09, David Mosberger wrote:
>
> This patch is rather big, but actually very straight-forward: it adds
> a "agp dev" argument to DRM_IOREMAP(), DRM_IOREMAP_NOCACHE(), and
> DRM_IOREMAPFREE() and then uses it in drm_memory.h to support
> platforms where CPU accesses to the AGP space are not translated by
> the GART (true for ia64 and alpha, not true for x86, I don't know
> about the other platforms). On platforms where cant_use_aperture is
> always false, this whole patch will look like a no-op. On ia64, it
> works. Don't know about other platforms, but it should simplify
> things for Alpha at least (and if it breaks a platform, I shall be
> happy to work with the respective maintainer to get fix back to
> working).

I'd love to commit this to DRI CVS, but there'd have to be fallbacks for
older kernels (this is against 2.4.20-ben8):

gcc
-I/home/michdaen/src/dri-cvs/xc-trunk/programs/Xserver/hw/xfree86/os-support/linux/drm/kernel -D__KERNEL__ -I/home/michdaen/src/linux-2.4.20-ben8-xfs-lolat/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fno-strict-aliasing -fno-common -fomit-frame-pointer -I/home/michdaen/src/linux-2.4.20-ben8-xfs-lolat/arch/ppc -fsigned-char -msoft-float -pipe -ffixed-r2 -Wno-uninitialized -mmultiple -mstring -DMODULE -nostdinc -iwithprefix include -DKBUILD_BASENAME=radeon_drv -c -o radeon_drv.o radeon_drv.c
In file included from radeon_drv.c:49:
drm_memory.h:37:21: asm/agp.h: No such file or directory
drm_memory.h:38:26: asm/tlbflush.h: No such file or directory
In file included from radeon_drv.c:49:
drm_memory.h: In function `agp_remap':
drm_memory.h:98: warning: implicit declaration of function `pfn_to_page'
drm_memory.h:98: warning: assignment makes pointer from integer without
a cast
drm_memory.h:99: warning: implicit declaration of function `vmap'
drm_memory.h:99: `PAGE_AGP' undeclared (first use in this function)
drm_memory.h:99: (Each undeclared identifier is reported only once
drm_memory.h:99: for each function it appears in.)
drm_memory.h:99: warning: assignment makes pointer from integer without
a cast
drm_memory.h:104: warning: implicit declaration of function
`flush_tlb_kernel_range'
drm_memory.h: In function `drm_follow_page':
drm_memory.h:113: warning: implicit declaration of function
`pte_offset_kernel'
drm_memory.h:113: warning: initialization makes pointer from integer
without a cast
drm_memory.h:114: warning: implicit declaration of function `pte_pfn'
drm_memory.h: In function `drm_ioremapfree':
drm_memory.h:186: warning: implicit declaration of function `vunmap'
make[3]: *** [radeon_drv.o] Error 1


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-11 17:56:27

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

>>>>> On 11 May 2003 13:43:36 +0200, Michel D?nzer <[email protected]> said:

Michel> On Sam, 2003-05-10 at 12:09, David Mosberger wrote:
>> This patch is rather big, but actually very straight-forward: it
>> adds a "agp dev" argument to DRM_IOREMAP(),
>> DRM_IOREMAP_NOCACHE(), and DRM_IOREMAPFREE() and then uses it in
>> drm_memory.h to support platforms where CPU accesses to the AGP
>> space are not translated by the GART (true for ia64 and alpha,
>> not true for x86, I don't know about the other platforms). On
>> platforms where cant_use_aperture is always false, this whole
>> patch will look like a no-op. On ia64, it works. Don't know
>> about other platforms, but it should simplify things for Alpha at
>> least (and if it breaks a platform, I shall be happy to work with
>> the respective maintainer to get fix back to working).

Michel> I'd love to commit this to DRI CVS,

Great!

Michel> but there'd have to be fallbacks for older kernels (this is
Michel> against 2.4.20-ben8):

OK, we have a chicken & egg problem then: I could obviously add Linux
kernel version checks where needed, but to do that, the patch first
needs to go into the kernel. Dave, would you mind applying the patch
to your tree? Then once Linus picked it up, I can send a new patch to
dri-devel. Or does someone have a better suggestion?

--david

2003-05-11 19:43:08

by Dave Jones

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Sun, May 11, 2003 at 11:09:00AM -0700, David Mosberger wrote:
> Michel> but there'd have to be fallbacks for older kernels (this is
> Michel> against 2.4.20-ben8):
>
> OK, we have a chicken & egg problem then: I could obviously add Linux
> kernel version checks where needed, but to do that, the patch first
> needs to go into the kernel. Dave, would you mind applying the patch
> to your tree? Then once Linus picked it up, I can send a new patch to
> dri-devel. Or does someone have a better suggestion?

With Linus doing the DRI sync-ups himself, maybe just pushing it his
way directly would work.. I'll roll it into the next 2.5-dj for some extra
testing, but I'm not going to be the one who pushes that linuswards
unless I get asked by the DRI folks.

Dave

2003-05-11 21:42:56

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Son, 2003-05-11 at 21:55, Dave Jones wrote:
> On Sun, May 11, 2003 at 11:09:00AM -0700, David Mosberger wrote:
> > Michel> but there'd have to be fallbacks for older kernels (this is
> > Michel> against 2.4.20-ben8):
> >
> > OK, we have a chicken & egg problem then: I could obviously add Linux
> > kernel version checks where needed, but to do that, the patch first
> > needs to go into the kernel.

Mind elaborating on that? I don't see such a problem as you don't need
version checks for anything the patch itself adds, only for kernel
infrastructure that isn't available in older kernels (down to 2.4).

> > Dave, would you mind applying the patch to your tree? Then once
> > Linus picked it up, I can send a new patch to dri-devel. Or does
> > someone have a better suggestion?
>
> With Linus doing the DRI sync-ups himself, maybe just pushing it his
> way directly would work..

It's my impression that he prefers just merging from DRI CVS to pushing
something back, so putting it in there first seems like it would be the
easiest way to me.


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-12 18:41:23

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

>>>>> On 11 May 2003 23:55:33 +0200, Michel D?nzer <[email protected]> said:

>> OK, we have a chicken & egg problem then: I could obviously add
>> Linux kernel version checks where needed, but to do that, the
>> patch first needs to go into the kernel.

Michel> Mind elaborating on that? I don't see such a problem as you
Michel> don't need version checks for anything the patch itself
Michel> adds, only for kernel infrastructure that isn't available in
Michel> older kernels (down to 2.4).

OK, I'm confused then: earlier on, you reported this error:

asm/agp.h: No such file or directory

My patch adds the following to asm-i386/agp.h:

diff -Nru a/include/asm-i386/agp.h b/include/asm-i386/agp.h
--- a/include/asm-i386/agp.h Sat May 10 01:47:42 2003
+++ b/include/asm-i386/agp.h Sat May 10 01:47:42 2003
@@ -20,4 +20,11 @@
worth it. Would need a page for it. */
#define flush_agp_cache() asm volatile("wbinvd":::"memory")

+/*
+ * Page-protection value to be used for AGP memory mapped into kernel space. For
+ * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
+ * be an uncached mapping (such as write-combining).
+ */
+#define PAGE_AGP PAGE_KERNEL_NOCACHE
+
#endif

So, either you're using a platform which I don't know supports AGP, or
the patch didn't apply cleanly (perhaps because you're using an old
kernel that doesn't have asm/agp.h yet?).

Could you shed some light? Once I understand why things are failing
for you, I shall be happy to update the patch accordingly.

--david

2003-05-12 19:35:55

by Michel Dänzer

[permalink] [raw]
Subject: Re: [Dri-devel] Re: Improved DRM support for cant_use_aperture platforms

On Mon, 2003-05-12 at 20:53, David Mosberger wrote:
> >>>>> On 11 May 2003 23:55:33 +0200, Michel D?nzer <[email protected]> said:
>
> >> OK, we have a chicken & egg problem then: I could obviously add
> >> Linux kernel version checks where needed, but to do that, the
> >> patch first needs to go into the kernel.
>
> Michel> Mind elaborating on that? I don't see such a problem as you
> Michel> don't need version checks for anything the patch itself
> Michel> adds, only for kernel infrastructure that isn't available in
> Michel> older kernels (down to 2.4).
>
> OK, I'm confused then: earlier on, you reported this error:
>
> asm/agp.h: No such file or directory
>
> My patch adds the following to asm-i386/agp.h:
>
> diff -Nru a/include/asm-i386/agp.h b/include/asm-i386/agp.h
> --- a/include/asm-i386/agp.h Sat May 10 01:47:42 2003
> +++ b/include/asm-i386/agp.h Sat May 10 01:47:42 2003
> @@ -20,4 +20,11 @@
> worth it. Would need a page for it. */
> #define flush_agp_cache() asm volatile("wbinvd":::"memory")
>
> +/*
> + * Page-protection value to be used for AGP memory mapped into kernel space. For
> + * platforms which use coherent AGP DMA, this can be PAGE_KERNEL. For others, it needs to
> + * be an uncached mapping (such as write-combining).
> + */
> +#define PAGE_AGP PAGE_KERNEL_NOCACHE
> +
> #endif
>
> So, either you're using a platform which I don't know supports AGP, or
> the patch didn't apply cleanly (perhaps because you're using an old
> kernel that doesn't have asm/agp.h yet?).

That's it. So we have to check the version before #including
<asm/agp.h>. Then, we can do something like

#ifndef PAGE_AGP
#define PAGE_AGP PAGE_KERNEL_NOCACHE
#endif

Or am I missing something?


This is the easy part though, you probably know better than I what to do
about the functions you use that aren't available in 2.4 yet. :)


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-12 20:06:36

by David Mosberger

[permalink] [raw]
Subject: Re: [Dri-devel] Re: Improved DRM support for cant_use_aperture platforms

>>>>> On 12 May 2003 21:48:31 +0200, Michel D?nzer <[email protected]> said:

>> using an old kernel that doesn't have asm/agp.h yet?).

Michel> That's it.

OK, thanks for clarifying.

Michel> So we have to check the version before #including
Michel> <asm/agp.h>. Then, we can do something like

Michel> #ifndef PAGE_AGP #define PAGE_AGP PAGE_KERNEL_NOCACHE #endif

Michel> Or am I missing something?

Basically correct, except that the patch also needs an improved
version of vmap(), which was introduced in 2.5.68 only (IIRC). I'll
update my patch so it is a no-op unless you have a kernel >= 2.5.68.

--david

2003-05-12 20:27:48

by David Mosberger

[permalink] [raw]
Subject: Re: [Dri-devel] Re: Improved DRM support for cant_use_aperture platforms

OK, could you try with this patch applied on top of my previous patch?
Checking against KERNEL_VERSION(2,5,68) ensures that we have the new
(4-wargument) vmap() call and the "#ifndef PAGE_AGP" part ensures that
things will compile fine until the kernel's asm/agp.h gets updated.

--david

===== drivers/char/drm/drm_memory.h 1.16 vs edited =====
--- 1.16/drivers/char/drm/drm_memory.h Sat May 10 01:32:08 2003
+++ edited/drivers/char/drm/drm_memory.h Mon May 12 13:37:57 2003
@@ -42,7 +42,12 @@
*/
#define DEBUG_MEMORY 0

-#if __REALLY_HAVE_AGP
+/* Need at least kernel v2.5.68 to get the 4-argument version of vmap(). */
+#if __REALLY_HAVE_AGP && (LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,68))
+
+#ifndef PAGE_AGP
+# define PAGE_AGP PAGE_KERNEL_NOCACHE
+#endif

/*
* Find the drm_map that covers the range [offset, offset+size).
@@ -127,7 +132,7 @@
{
int remap_aperture = 0;

-#if __REALLY_HAVE_AGP
+#if __REALLY_HAVE_AGP && (LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,68))
if (dev->agp->cant_use_aperture) {
drm_map_t *map = drm_lookup_map(offset, size, dev);

@@ -146,7 +151,7 @@
{
int remap_aperture = 0;

-#if __REALLY_HAVE_AGP
+#if __REALLY_HAVE_AGP && (LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,68))
if (dev->agp->cant_use_aperture) {
drm_map_t *map = drm_lookup_map(offset, size, dev);

@@ -163,7 +168,7 @@
static inline void drm_ioremapfree(void *pt, unsigned long size, drm_device_t *dev)
{
int unmap_aperture = 0;
-#if __REALLY_HAVE_AGP
+#if __REALLY_HAVE_AGP && (LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,68))
/*
* This is a bit ugly. It would be much cleaner if the DRM API would use separate
* routines for handling mappings in the AGP space. Hopefully this can be done in

2003-05-12 21:09:24

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Mon, 2003-05-12 at 22:19, David Mosberger wrote:
> >>>>> On 12 May 2003 21:48:31 +0200, Michel D?nzer <[email protected]> said:
>
> >> using an old kernel that doesn't have asm/agp.h yet?).
>
> Michel> That's it.
>
> OK, thanks for clarifying.
>
> Michel> So we have to check the version before #including
> Michel> <asm/agp.h>. Then, we can do something like
>
> Michel> #ifndef PAGE_AGP #define PAGE_AGP PAGE_KERNEL_NOCACHE #endif
>
> Michel> Or am I missing something?
>
> Basically correct, except that the patch also needs an improved
> version of vmap(), which was introduced in 2.5.68 only (IIRC). I'll
> update my patch so it is a no-op unless you have a kernel >= 2.5.68.

Hmm, isn't there a way to make it work with older kernels as well? For
reference, we've been using
http://www.penguinppc.org/~daenzer/DRI/drm-ioremapagp.diff by Benjamin
Herrenschmidt for a while for Apple UniNorth northbridges. I was hoping
to replace it with a cleaner solution like yours.

Anyway, after applying your second patch, things looked much better, and
the attached patch against the DRI CVS trunk builds without warnings
here.


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer


Attachments:
drm-cant_use_aperture.diff (25.97 kB)

2003-05-12 21:38:26

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms


Michel> Hmm, isn't there a way to make it work with older kernels as
Michel> well? For reference, we've been using
Michel> http://www.penguinppc.org/~daenzer/DRI/drm-ioremapagp.diff
Michel> by Benjamin Herrenschmidt for a while for Apple UniNorth
Michel> northbridges.

It should be possible to add vmap() and vunmap() to kernel/vmalloc.c
on older kernels. I think those are the only dependencies (apart from
PAGE_AGP, which is taken care of by the latest patch).

Michel> I was hoping to replace it with a cleaner solution like
Michel> yours.

Is there someone else on this list who would be able to look into
backporting vmap()/vunmap() to 2.4? I don't use 2.4 with any
regularity anymore, but I suppose if nobody else is interested, I
could look into it.

Michel> Anyway, after applying your second patch, things looked much
Michel> better, and the attached patch against the DRI CVS trunk
Michel> builds without warnings here.

Great! Does this mean that next time Linus does a pull he'll pick up
this stuff?

--david

2003-05-12 21:45:35

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Mon, May 12, 2003 at 02:51:08PM -0700, David Mosberger wrote:
> Is there someone else on this list who would be able to look into
> backporting vmap()/vunmap() to 2.4? I don't use 2.4 with any
> regularity anymore, but I suppose if nobody else is interested, I
> could look into it.

I did that for the XFS tree ages ago, it's also in the -ac and -aa (the latter
still has the old three-arg version) now. I will submit it to Marcelo as soon
as 2.4.21 is out (so with the current 2.4 merge rate it'll be in in about 6 month..)

2003-05-12 22:07:47

by Christoph Hellwig

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

Note that the 2.4 vmap implementation in the XFS tree doesn't have this problem.
It's cludged into the old vmalloc design instead of beeing based on Linus'
suggestion whiches buggy implementation (by me) opened this race.

2003-05-12 22:00:26

by Andrew Morton

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

Christoph Hellwig <[email protected]> wrote:
>
> On Mon, May 12, 2003 at 02:51:08PM -0700, David Mosberger wrote:
> > Is there someone else on this list who would be able to look into
> > backporting vmap()/vunmap() to 2.4? I don't use 2.4 with any
> > regularity anymore, but I suppose if nobody else is interested, I
> > could look into it.
>
> I did that for the XFS tree ages ago, it's also in the -ac and -aa (the latter
> still has the old three-arg version) now. I will submit it to Marcelo as soon
> as 2.4.21 is out (so with the current 2.4 merge rate it'll be in in about 6 month..)
>

That 2.5 vmap rework introduced a subtle race-bug-goes-oops in
vmalloc()/vfree(). The below patch from Bill fixes it up.




From: William Lee Irwin III <[email protected]>

The new vmalloc() semantics from 2.5.32 had a race window. As things stand,
the presence of a vm_area in the vmlist protects from allocators other than
the owner examining the ptes in that area. This puts an ordering constraint
on unmapping, so that allocators are required to unmap areas before removing
them from the list or otherwise dropping the lock.

Currently, unmap_vm_area() is done outside the lock and after the area is
removed, which as we've seen from Felix von Leitner's test is oopsable.

The following patch folds calls to unmap_vm_area() into remove_vm_area() to
reinstate what are essentially the 2.4.x semantics of vfree(). This renders
a number of unmap_vm_area() calls unnecessary (and in fact oopsable since
they wipe ptes from later allocations). It's an open question as to whether
this is sufficiently performant, but it is the minimally invasive approach.
The more performant alternative is to provide the right API hooks to wipe the
vmalloc() area clean before removing them from the list, using the ownership
of the area to eliminate holding the vmlist_lock for the duration of the
unmapping. If it proves to be necessary wli is on standby to implement it.


25-akpm/arch/i386/mm/ioremap.c | 1 -
25-akpm/arch/sparc64/kernel/module.c | 2 ++
25-akpm/arch/x86_64/kernel/module.c | 2 +-
25-akpm/arch/x86_64/mm/ioremap.c | 1 -
25-akpm/mm/vmalloc.c | 3 +--
5 files changed, 4 insertions(+), 5 deletions(-)

diff -puN arch/i386/mm/ioremap.c~vmalloc-race-fix arch/i386/mm/ioremap.c
--- 25/arch/i386/mm/ioremap.c~vmalloc-race-fix Mon May 12 14:17:19 2003
+++ 25-akpm/arch/i386/mm/ioremap.c Mon May 12 14:17:19 2003
@@ -222,7 +222,6 @@ void iounmap(void *addr)
return;
}

- unmap_vm_area(p);
if (p->flags && p->phys_addr < virt_to_phys(high_memory)) {
change_page_attr(virt_to_page(__va(p->phys_addr)),
p->size >> PAGE_SHIFT,
diff -puN arch/sparc64/kernel/module.c~vmalloc-race-fix arch/sparc64/kernel/module.c
--- 25/arch/sparc64/kernel/module.c~vmalloc-race-fix Mon May 12 14:17:19 2003
+++ 25-akpm/arch/sparc64/kernel/module.c Mon May 12 14:17:19 2003
@@ -138,7 +138,9 @@ void *module_alloc(unsigned long size)
/* Free memory returned from module_core_alloc/module_init_alloc */
void module_free(struct module *mod, void *module_region)
{
+ write_lock(&vmlist_lock);
module_unmap(module_region);
+ write_unlock(&vmlist_lock);
/* FIXME: If module_region == mod->init_region, trim exception
table entries. */
}
diff -puN arch/x86_64/kernel/module.c~vmalloc-race-fix arch/x86_64/kernel/module.c
--- 25/arch/x86_64/kernel/module.c~vmalloc-race-fix Mon May 12 14:17:19 2003
+++ 25-akpm/arch/x86_64/kernel/module.c Mon May 12 14:17:19 2003
@@ -48,7 +48,6 @@ void module_free(struct module *mod, voi
for (prevp = &mod_vmlist ; (map = *prevp) ; prevp = &map->next) {
if ((unsigned long)map->addr == addr) {
*prevp = map->next;
- write_unlock(&vmlist_lock);
goto found;
}
}
@@ -57,6 +56,7 @@ void module_free(struct module *mod, voi
return;
found:
unmap_vm_area(map);
+ write_unlock(&vmlist_lock);
if (map->pages) {
for (i = 0; i < map->nr_pages; i++)
if (map->pages[i])
diff -puN arch/x86_64/mm/ioremap.c~vmalloc-race-fix arch/x86_64/mm/ioremap.c
--- 25/arch/x86_64/mm/ioremap.c~vmalloc-race-fix Mon May 12 14:17:19 2003
+++ 25-akpm/arch/x86_64/mm/ioremap.c Mon May 12 14:17:19 2003
@@ -222,7 +222,6 @@ void iounmap(void *addr)
return;
}

- unmap_vm_area(p);
if (p->flags && p->phys_addr < virt_to_phys(high_memory)) {
change_page_attr(virt_to_page(__va(p->phys_addr)),
p->size >> PAGE_SHIFT,
diff -puN mm/vmalloc.c~vmalloc-race-fix mm/vmalloc.c
--- 25/mm/vmalloc.c~vmalloc-race-fix Mon May 12 14:17:19 2003
+++ 25-akpm/mm/vmalloc.c Mon May 12 14:17:19 2003
@@ -260,6 +260,7 @@ struct vm_struct *remove_vm_area(void *a
return NULL;

found:
+ unmap_vm_area(tmp);
*p = tmp->next;
write_unlock(&vmlist_lock);
return tmp;
@@ -283,8 +284,6 @@ void __vunmap(void *addr, int deallocate
addr);
return;
}
-
- unmap_vm_area(area);

if (deallocate_pages) {
int i;

_

2003-05-13 00:23:02

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Mon, 2003-05-12 at 23:51, David Mosberger wrote:
> Michel> Hmm, isn't there a way to make it work with older kernels as
> Michel> well? For reference, we've been using
> Michel> http://www.penguinppc.org/~daenzer/DRI/drm-ioremapagp.diff
> Michel> by Benjamin Herrenschmidt for a while for Apple UniNorth
> Michel> northbridges.
>
> It should be possible to add vmap() and vunmap() to kernel/vmalloc.c
> on older kernels. I think those are the only dependencies

There are a couple more, like pte_offset_kernel(), pte_pfn(),
pfn_to_page() and flush_tlb_kernel_range(). Getting this working with
2.4 seems like a lot of work and/or ugly. :\

> (apart from PAGE_AGP, which is taken care of by the latest patch).

Not quite, PAGE_KERNEL_NOCACHE isn't defined on all architectures
either.


> Michel> Anyway, after applying your second patch, things looked much
> Michel> better, and the attached patch against the DRI CVS trunk
> Michel> builds without warnings here.
>
> Great! Does this mean that next time Linus does a pull he'll pick up
> this stuff?

No, I haven't committed it yet. I'd like to find a solution for PAGE_AGP
first at least. Looks like we have to wait for Linus to merge the
asm/agp.h part of your patch first?

Meanwhile, here's another version of the DRI CVS patch, I got rid of the
local variable in the drm_ioremap* functions.


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer


Attachments:
drm-cant_use_aperture.diff (25.62 kB)

2003-05-13 00:56:38

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms


Michel> There are a couple more, like pte_offset_kernel(),
Michel> pte_pfn(), pfn_to_page() and
Michel> flush_tlb_kernel_range(). Getting this working with 2.4
Michel> seems like a lot of work and/or ugly. :\

Just goes to show how quickly one gets used to 2.5... ;-)

Let me take a look at these.

>> (apart from PAGE_AGP, which is taken care of by the latest
>> patch).

Michel> Not quite, PAGE_KERNEL_NOCACHE isn't defined on all
Michel> architectures either.

OK, I believe the only other architecture that sets
"cant_use_aperture" is Alpha. I asked some Alpha folks several months
ago about my patch, but never got a conclusive answer. IIRC, on Alpha
the physical address itself determines cacheablity. If so, we can use
PAGE_KERNEL (which is universal) instead of PAGE_KERNEL_NOCACHE.

Clearly the patch needs to be tested on Alpha. The upside is that it
should let the Alpha folks get rid of a lot of ugliness in the
ioremap() code.

--david

2003-05-13 07:30:53

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

>>>>> On 13 May 2003 02:34:41 +0200, Michel D?nzer <[email protected]> said:

>> It should be possible to add vmap() and vunmap() to kernel/vmalloc.c
>> on older kernels. I think those are the only dependencies

Michel> There are a couple more, like pte_offset_kernel(), pte_pfn(),
Michel> pfn_to_page() and flush_tlb_kernel_range(). Getting this working with
Michel> 2.4 seems like a lot of work and/or ugly. :\

Actually, it turns out I'm really not well positioned to do this,
because the ia64 agp patch for 2.4 looks very different from the 2.5
and your tree looks rather different from the DRM stuff that's in the
official Linux tree (correct me if I'm wrong here).

Anyhow, this should get you close to compiling (and working,
hopefully), modulo vmap/vunmap:

#if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
# define pte_offset_kernel(dir, address) pte_offset(dir, address)
# define pte_pfn(pte) (pte_page(pte) - mem_map)
# define flush_tlb_kernel_range(s,e) flush_tlb_all()
#endif

The above definition of pte_pfn() is not truly platform-independent,
but I believe it works on all platforms that support AGP. The problem
is that we can't just use page_address(), because the physical address
in the PTE may not be a valid memory address (it could be an I/O
address).

--david

2003-05-13 13:23:35

by Ivan Kokshaysky

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Mon, May 12, 2003 at 06:09:16PM -0700, David Mosberger wrote:
> OK, I believe the only other architecture that sets
> "cant_use_aperture" is Alpha.

Note that some Alphas may have "cant_use_aperture" cleared.
I mean Nautilus - unfortunate product of Athlon northbridge
and EV6 interbreeding... I've just managed to get agpgart working
to some degree on one of these beasts (UP1500), but with so many
ugly and fragile hacks that I'm not sure that I ever want
to submit this. ;-)

> I asked some Alpha folks several months
> ago about my patch, but never got a conclusive answer. IIRC, on Alpha
> the physical address itself determines cacheablity. If so, we can use
> PAGE_KERNEL (which is universal) instead of PAGE_KERNEL_NOCACHE.

Yes.

> Clearly the patch needs to be tested on Alpha. The upside is that it
> should let the Alpha folks get rid of a lot of ugliness in the
> ioremap() code.

That would be great.
[Cc'd to Jeff, who wrote Alpha AGP code]

Ivan.

2003-05-13 16:09:30

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

>>>>> On Tue, 13 May 2003 17:33:47 +0400, Ivan Kokshaysky <[email protected]> said:

Ivan> On Mon, May 12, 2003 at 06:09:16PM -0700, David Mosberger
Ivan> wrote:
>> OK, I believe the only other architecture that sets
>> "cant_use_aperture" is Alpha.

Ivan> Note that some Alphas may have "cant_use_aperture" cleared.

"Interesting".

Ivan> I mean Nautilus - unfortunate product of Athlon northbridge
Ivan> and EV6 interbreeding... I've just managed to get agpgart
Ivan> working to some degree on one of these beasts (UP1500), but
Ivan> with so many ugly and fragile hacks that I'm not sure that I
Ivan> ever want to submit this. ;-)

If cant_use_aperture isn't set, my patch won't help, but it shouldn't
hurt anything either. I.e., you should be no worse off than before.

What's the nature of those "ugly and fragile" hacks? Are you saying
that CPU accesses to AGP space aren't remapped in the "normal" (PC)
way? Or is it something entirely different?

>> I asked some Alpha folks several months ago about my patch, but
>> never got a conclusive answer. IIRC, on Alpha the physical
>> address itself determines cacheablity. If so, we can use
>> PAGE_KERNEL (which is universal) instead of PAGE_KERNEL_NOCACHE.

Ivan> Yes.

Thanks for confirming.

--david

2003-05-14 09:29:53

by Ivan Kokshaysky

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Tue, May 13, 2003 at 09:20:09AM -0700, David Mosberger wrote:
> Ivan> Note that some Alphas may have "cant_use_aperture" cleared.
>
> "Interesting".

Yep. ;-)

> If cant_use_aperture isn't set, my patch won't help, but it shouldn't
> hurt anything either. I.e., you should be no worse off than before.

Sure. Basically, I'll only need to re-diff.

> What's the nature of those "ugly and fragile" hacks? Are you saying
> that CPU accesses to AGP space aren't remapped in the "normal" (PC)
> way? Or is it something entirely different?

Ok, you asked for it... :-)
As you know, Alpha architecture is entirely cache coherent
by design, i.e. there are no such things as non-cacheable mappings
or cache flushing in hardware. Native Alpha Titan/Marvel AGP controllers
are also cache coherent (kind of AGP extension of traditional
Alpha PCI IOMMU).
However, the "normal" PC AGP implementation isn't - this applies
to AMD-751/761 AGP controllers on Nautilus as well.
The AGP window on these chipsets is accessible by CPU *only* in the
system memory address space, i.e. it's always cacheable and thus
totally useless on Alpha.
Fortunately, AMD northbridges (at least 761) have some features
that allow *limited* access to system memory, including AGP space,
through EV6 non-cacheable IO address space (using "magic" 16 Gb offset).
Limitations:
- quadword writes corrupt ECC.
EV6 doesn't generate ECC for IO space, but the chipset expects ECC
from CPU on full quad writes (narrower writes are ok since the chipset
does read-modify-write and updates ECC by itself).
This wouldn't be a big deal since nobody should access pages mapped
into AGP, but there are occasional speculative loads which hit data
with bad ECC that causing the machine checks. So I must handle these
machine checks properly - check that the page is AGP and then either
scrub the affected location to fix ECC or dismiss the machine check.
- according to AMD specs, reads to this "non-cacheable" region are
not supported. Not quite true, as quadword loads (ldq/ldq_u) do work
(verified on 761, 751 needs testing). Narrower loads indeed don't work.
Fortunately, AGP space reads seem to be sort of exotic (in both
kernel and userspace drivers), so they can be easily found and converted
to ldq/ldq_u.

In addition I cannot trust PALcode error handler on UP1500 - sometimes it
does bogus things...
By now I have all this crap running more or less stable with
radeon 7200 card. Machine check handling needs some improvements though.

Ivan.

2003-05-14 10:14:25

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Mit, 2003-05-14 at 11:41, Ivan Kokshaysky wrote:
> On Tue, May 13, 2003 at 09:20:09AM -0700, David Mosberger wrote:
>
> > What's the nature of those "ugly and fragile" hacks? Are you saying
> > that CPU accesses to AGP space aren't remapped in the "normal" (PC)
> > way? Or is it something entirely different?
>
> Ok, you asked for it... :-)
> As you know, Alpha architecture is entirely cache coherent
> by design, i.e. there are no such things as non-cacheable mappings
> or cache flushing in hardware. Native Alpha Titan/Marvel AGP controllers
> are also cache coherent (kind of AGP extension of traditional
> Alpha PCI IOMMU).
> However, the "normal" PC AGP implementation isn't - this applies
> to AMD-751/761 AGP controllers on Nautilus as well.
> The AGP window on these chipsets is accessible by CPU *only* in the
> system memory address space, i.e. it's always cacheable and thus
> totally useless on Alpha.

Set cant_use_aperture and use David's patch then? :)


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-14 13:56:07

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Die, 2003-05-13 at 09:43, David Mosberger wrote:
> >>>>> On 13 May 2003 02:34:41 +0200, Michel D?nzer <[email protected]> said:
>
> >> It should be possible to add vmap() and vunmap() to kernel/vmalloc.c
> >> on older kernels. I think those are the only dependencies
>
> Michel> There are a couple more, like pte_offset_kernel(), pte_pfn(),
> Michel> pfn_to_page() and flush_tlb_kernel_range(). Getting this working with
> Michel> 2.4 seems like a lot of work and/or ugly. :\
>
> Actually, it turns out I'm really not well positioned to do this,
> because the ia64 agp patch for 2.4 looks very different from the 2.5
> and your tree looks rather different from the DRM stuff that's in the
> official Linux tree (correct me if I'm wrong here).
>
> Anyhow, this should get you close to compiling (and working,
> hopefully), modulo vmap/vunmap:
>
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,5,0)
> # define pte_offset_kernel(dir, address) pte_offset(dir, address)
> # define pte_pfn(pte) (pte_page(pte) - mem_map)
> # define flush_tlb_kernel_range(s,e) flush_tlb_all()
> #endif

[...]

> The above definition of pte_pfn() is not truly platform-independent,
> but I believe it works on all platforms that support AGP.

Looks like it should work on sane PPC systems as well. :)

# define pfn_to_page(pfn) (mem_map + (pfn))

is also needed.


After some more thinking, the way to go for deciding whether or not to
use the new code probably isn't by checking the version but by using
some Makefile trickery as there is already for do_munmap and
remap_page_range. Once that is in place, it looks like I can finally
commit it. :)


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-14 16:56:23

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

>>>>> On Wed, 14 May 2003 13:41:41 +0400, Ivan Kokshaysky <[email protected]> said:

>> What's the nature of those "ugly and fragile" hacks? Are you saying
>> that CPU accesses to AGP space aren't remapped in the "normal" (PC)
>> way? Or is it something entirely different?

Ivan> Ok, you asked for it... :-)

My golly, I don't envy you!

--david

2003-05-15 15:46:40

by David Mosberger

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

Hi Michel,

In regards to the Alpha platform: Jeff Wiedemeier was able to get things
to work after applying the attached small patch. He says:

What was happening is that the offset was a system-relative
representation of the address and the (agpmem->bound) was
bus-relative, so it couldn't find the right agpmem.

This patch makes the offset bus-relative before the scan (and with
this patch, DRI/DRM is working on a Marvel...)

If it looks OK to you, can you add it?

Thanks,

--david

diff -Nuar pre/drivers/char/drm/drm_memory.h post/drivers/char/drm/drm_memory.h
--- pre/drivers/char/drm/drm_memory.h Wed May 14 20:04:17 2003
+++ post/drivers/char/drm/drm_memory.h Wed May 14 20:05:31 2003
@@ -75,6 +75,10 @@

size = PAGE_ALIGN(size);

+#ifdef __alpha__
+ offset -= dev->hose->mem_space->start;
+#endif
+
for (agpmem = dev->agp->memory; agpmem; agpmem = agpmem->next)
if (agpmem->bound <= offset
&& (agpmem->bound + (agpmem->pages << PAGE_SHIFT)) >= (offset + size))

2003-05-15 22:25:07

by Michel Dänzer

[permalink] [raw]
Subject: Re: Improved DRM support for cant_use_aperture platforms

On Thu, 2003-05-15 at 17:59, David Mosberger wrote:
>
> In regards to the Alpha platform: Jeff Wiedemeier was able to get things
> to work after applying the attached small patch. He says:
>
> What was happening is that the offset was a system-relative
> representation of the address and the (agpmem->bound) was
> bus-relative, so it couldn't find the right agpmem.
>
> This patch makes the offset bus-relative before the scan (and with
> this patch, DRI/DRM is working on a Marvel...)
>
> If it looks OK to you, can you add it?

Sure, thanks.

http://www.penguinppc.org/~daenzer/DRI/drm-cant_use_aperture.diff

is more or less what I'd like to commit. Comments appreciated.


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer

2003-05-16 23:38:04

by Michel Dänzer

[permalink] [raw]
Subject: Re: [Dri-devel] Re: Improved DRM support for cant_use_aperture platforms

On Fri, 2003-05-16 at 00:37, Michel D?nzer wrote:
>
> http://www.penguinppc.org/~daenzer/DRI/drm-cant_use_aperture.diff
>
> is more or less what I'd like to commit. Comments appreciated.

I just committed it, thanks to everyone involved.


--
Earthling Michel D?nzer \ Debian (powerpc), XFree86 and DRI developer
Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer