On Sunday 09 December 2001 10:28 pm, you wrote:
> And this is true about the kernel-level API as well. I have just figured
> out that agp_memory_t.memory field is an array of addresses to allocated
> pages - but I have not figured out whether these are physical or not.
The memory[] table currently contains the values stored in the GATT (the
hardware-visible translation table). Typically this is the physical
address plus a bit or two to indicate a valid entry. In order to get the
actual physical address, you have to strip out those extra bits ("unmask"
in agpgart terminology).
I am proposing a patch that will make the memory[] values genuine physical
addresses, with no extra bits. The Geocrawler list archive seems
hopelessly hosed, so I'll attach a couple recent messages about it.
Bjorn
--
Linux Systems Operation R&D
Hewlett-Packard
Fort Collins, CO
--- Begin Message ---
In the Linux AGP/GART driver, the agp_memory struct contains a memory[]
array with the physical address of every page in the block.
Currently, the elements of the memory[] array are the actual values put
into the GATT, i.e., they contain the valid bit and any other bits
required by the GART chipset as well as the physical address.
These mangled physical addresses are used by DRM, for example in
DRM(vm_nopage), where those extra chipset-specific bits have to be masked
out to obtain the actual physical address.
I propose a change along the lines of the following patch, which makes the
elements of memory[] plain physical addresses and keeps all the
chipset-specific bits encapsulated inside the AGP/GART module. The
general idea is to move the "masking" (chipset-specific address mangling)
from the memory allocation path to the GATT insertion path.
I'm not a DRM guru, so I'm sure there are subtleties I'm missing, and I
know there are some interface versioning issues to work out, but I'd
appreciate any feedback on the approach and advice on how to proceed. The
attached patch is against 2.4.16 with the IA64 patch applied.
--- linux+ia64/drivers/char/drm/drmP.h Tue Nov 27 13:54:53 2001
+++ linux/drivers/char/drm/drmP.h Thu Dec 6 14:11:34 2001
@@ -628,7 +628,9 @@
unsigned long base;
int agp_mtrr;
int cant_use_aperture;
+#ifdef PAGE_MASK_INTERFACE
unsigned long page_mask;
+#endif
} drm_agp_head_t;
#endif
--- linux+ia64/drivers/char/drm/drm_vm.h Tue Nov 27 13:54:53 2001
+++ linux/drivers/char/drm/drm_vm.h Thu Dec 6 10:57:39 2001
@@ -115,18 +115,7 @@
* Get the page, inc the use count, and return it
*/
offset = (baddr - agpmem->bound) >> PAGE_SHIFT;
-
- /*
- * This is bad. What we really want to do here is unmask
- * the GART table entry held in the agp_memory structure.
- * There isn't a convenient way to call agp_bridge.unmask_
- * memory from here, so hard code it for now.
- */
-#if defined(__ia64__)
- paddr = (agpmem->memory->memory[offset] & 0xffffff) << 12;
-#else
- paddr = agpmem->memory->memory[offset] & dev->agp->page_mask;
-#endif
+ paddr = agpmem->memory->memory[offset];
page = virt_to_page(__va(paddr));
get_page(page);
--- linux+ia64/drivers/char/drm/drm_agpsupport.h Tue Nov 27 13:54:53 2001
+++ linux/drivers/char/drm/drm_agpsupport.h Thu Dec 6 10:56:34 2001
@@ -321,10 +321,14 @@
}
#if LINUX_VERSION_CODE <= 0x020408
head->cant_use_aperture = 0;
+#ifdef PAGE_MASK_INTERFACE
head->page_mask = ~(0xfff);
+#endif
#else
head->cant_use_aperture = head->agp_info.cant_use_aperture;
+#ifdef PAGE_MASK_INTERFACE
head->page_mask = head->agp_info.page_mask;
+#endif
#endif
DRM_INFO("AGP %d.%d on %s @ 0x%08lx %ZuMB\n",
--- linux+ia64/drivers/char/agp/agpgart_be.c Tue Nov 27 13:54:53 2001
+++ linux/drivers/char/agp/agpgart_be.c Thu Dec 6 10:59:51 2001
@@ -212,8 +212,6 @@
if(agp_bridge.cant_use_aperture == 0) {
if (curr->page_count != 0) {
for (i = 0; i < curr->page_count; i++) {
- curr->memory[i] = agp_bridge.unmask_memory(
- curr->memory[i]);
agp_bridge.agp_destroy_page((unsigned long)
phys_to_virt(curr->memory[i]));
}
@@ -302,10 +300,7 @@
agp_free_memory(new);
return NULL;
}
- new->memory[i] =
- agp_bridge.mask_memory(
- virt_to_phys((void *) new->memory[i]),
- type);
+ new->memory[i] = virt_to_phys((void *) new->memory[i]);
new->page_count++;
}
} else {
@@ -338,7 +333,7 @@
#else
paddr = pte_val(*pte) & PAGE_MASK;
#endif
- new->memory[i] = agp_bridge.mask_memory(paddr, type);
+ new->memory[i] = paddr;
}
new->page_count = page_count;
@@ -403,10 +398,12 @@
info->current_memory = atomic_read(&agp_bridge.current_memory_agp);
info->cant_use_aperture = agp_bridge.cant_use_aperture;
+#ifdef PAGE_MASK_INTERFACE
for(i = 0; i < agp_bridge.num_of_masks; i++)
page_mask |= agp_bridge.mask_memory(page_mask, i);
info->page_mask = ~page_mask;
+#endif
}
/* End - Routine to copy over information structure */
@@ -825,7 +822,7 @@
mem->is_flushed = TRUE;
}
for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
- agp_bridge.gatt_table[j] = mem->memory[i];
+ agp_bridge.gatt_table[j] = agp_bridge.mask_memory(mem->memory[i],
mem->type);
}
agp_bridge.tlb_flush(mem);
@@ -1067,7 +1064,8 @@
CACHE_FLUSH();
for (i = 0, j = pg_start; i < mem->page_count; i++, j++) {
OUTREG32(intel_i810_private.registers,
- I810_PTE_BASE + (j * 4), mem->memory[i]);
+ I810_PTE_BASE + (j * 4),
+ agp_bridge.mask_memory(mem->memory[i], mem->type));
}
CACHE_FLUSH();
@@ -1133,10 +1131,7 @@
agp_free_memory(new);
return NULL;
}
- new->memory[0] =
- agp_bridge.mask_memory(
- virt_to_phys((void *) new->memory[0]),
- type);
+ new->memory[0] = virt_to_phys((void *) new->memory[0]);
new->page_count = 1;
new->num_scratch_pages = 1;
new->type = AGP_PHYS_MEMORY;
@@ -1373,7 +1368,8 @@
CACHE_FLUSH();
for (i = 0, j = pg_start; i < mem->page_count; i++, j++)
- OUTREG32(intel_i830_private.registers,I810_PTE_BASE + (j *
4),mem->memory[i]);
+ OUTREG32(intel_i830_private.registers,I810_PTE_BASE + (j * 4),
+ agp_bridge.mask_memory(mem->memory[i], mem->type));
CACHE_FLUSH();
@@ -1434,7 +1430,7 @@
return(NULL);
}
- nw->memory[0] = agp_bridge.mask_memory(virt_to_phys((void *)
nw->memory[0]),type);
+ nw->memory[0] = virt_to_phys((void *) nw->memory[0]);
nw->page_count = 1;
nw->num_scratch_pages = 1;
nw->type = AGP_PHYS_MEMORY;
@@ -1485,10 +1481,6 @@
#endif /* CONFIG_AGP_I810 */
- #ifdef CONFIG_AGP_INTEL
-
-#endif /* CONFIG_AGP_I810 */
-
#ifdef CONFIG_AGP_I460
/* BIOS configures the chipset so that one of two apbase registers are
used */
@@ -1496,6 +1488,7 @@
/* 460 supports multiple GART page sizes, so GART pageshift is dynamic */
static u8 intel_i460_pageshift = 12;
+static u32 intel_i460_pagesize;
/* Keep track of which is larger, chipset or kernel page size. */
static u32 intel_i460_cpk = 1;
@@ -1523,6 +1516,7 @@
/* Determine the GART page size */
pci_read_config_byte(agp_bridge.dev, INTEL_I460_GXBCTL, &temp);
intel_i460_pageshift = (temp & I460_4M_PS) ? 22 : 12;
+ intel_i460_pagesize = 1UL << intel_i460_pageshift;
values = A_SIZE_8(agp_bridge.aperture_sizes);
@@ -1737,7 +1731,7 @@
{
int i, j, k, num_entries;
void *temp;
- unsigned int hold;
+ unsigned long paddr;
unsigned int read_back;
/*
@@ -1769,10 +1763,11 @@
for (i = 0, j = pg_start; i < mem->page_count; i++) {
- hold = (unsigned int) (mem->memory[i]);
+ paddr = mem->memory[i];
- for (k = 0; k < I460_CPAGES_PER_KPAGE; k++, j++, hold++)
- agp_bridge.gatt_table[j] = hold;
+ for (k = 0; k < I460_CPAGES_PER_KPAGE; k++, j++, paddr +=
intel_i460_pagesize)
+ agp_bridge.gatt_table[j] = (unsigned int)
+ agp_bridge.mask_memory(paddr, mem->type);
}
/*
@@ -1886,6 +1881,7 @@
int num_entries;
void *temp;
unsigned int read_back;
+ unsigned long paddr;
temp = agp_bridge.current_size;
num_entries = A_SIZE_8(temp)->num_entries;
@@ -1934,18 +1930,17 @@
for(pg = start_pg, i = 0; pg <= end_pg; pg++)
{
+ paddr = agp_bridge.unmask_memory(agp_bridge.gatt_table[pg]);
for(idx = ((pg == start_pg) ? start_offset : 0);
idx < ((pg == end_pg) ? (end_offset + 1)
: I460_KPAGES_PER_CPAGE);
idx++, i++)
{
- i460_pg_detail[pg][idx] = agp_bridge.gatt_table[pg] +
- ((idx * PAGE_SIZE) >> 12);
+ mem->memory[i] = paddr + (idx * PAGE_SIZE);
+ i460_pg_detail[pg][idx] =
+ agp_bridge.mask_memory(mem->memory[i], mem->type);
+
i460_pg_count[pg]++;
-
- /* Finally we fill in mem->memory... */
- mem->memory[i] = ((unsigned long) (0xffffff &
- i460_pg_detail[pg][idx])) << 12;
}
}
@@ -1959,7 +1954,7 @@
int num_entries;
void *temp;
unsigned int read_back;
- unsigned long addr;
+ unsigned long paddr;
temp = agp_bridge.current_size;
num_entries = A_SIZE_8(temp)->num_entries;
@@ -1986,13 +1981,11 @@
/* Free GART pages if they are unused */
if(i460_pg_count[pg] == 0) {
- addr = (0xffffffUL & (unsigned long)
- (agp_bridge.gatt_table[pg])) << 12;
-
- agp_bridge.gatt_table[pg] = 0;
+ paddr = agp_bridge.unmask_memory(agp_bridge.gatt_table[pg]);
+ agp_bridge.gatt_table[pg] = agp_bridge.scratch_page;
read_back = agp_bridge.gatt_table[pg];
- intel_i460_free_large_page(pg, addr);
+ intel_i460_free_large_page(pg, paddr);
}
}
--- End Message ---
--- Begin Message ---
On Friday 07 December 2001 2:09 am, Keith Whitwell wrote:
> What are the goals of your patch? Is it a cleanup, or is there
> something deeper you hope to gain?
Yes, I should have made the goal clearer. This section of the diff is
the relevant piece:
--- linux+ia64/drivers/char/drm/drm_vm.h Tue Nov 27 13:54:53 2001
+++ linux/drivers/char/drm/drm_vm.h Thu Dec 6 10:57:39 2001
@@ -115,18 +115,7 @@
* Get the page, inc the use count, and return it
*/
offset = (baddr - agpmem->bound) >> PAGE_SHIFT;
-
- /*
- * This is bad. What we really want to do here is unmask
- * the GART table entry held in the agp_memory structure.
- * There isn't a convenient way to call agp_bridge.unmask_
- * memory from here, so hard code it for now.
- */
-#if defined(__ia64__)
- paddr = (agpmem->memory->memory[offset] & 0xffffff) << 12;
-#else
- paddr = agpmem->memory->memory[offset] &
dev->agp->page_mask;
-#endif
+ paddr = agpmem->memory->memory[offset];
page = virt_to_page(__va(paddr));
get_page(page);
Note that the computation of paddr is machine-dependent. The existing
code for __ia64__ (paddr = (agpmem->memory->memory[offset] & 0xffffff) <<
12) is specific to the Intel 460GX chipset. I'm implementing support for
an HP IA64 chipset, and our unmasking function is of course different from
the 460's.
So, we either have to have chipset-specific code in DRM to compute paddr,
or export an API from AGP/GART (the page_mask stuff is a simple start at
this, but as you can see, is not general enough to describe all unmask
operations), or try to remove the need for the unmasking completely.
This last is the avenue that I'm trying to pursue. The unmask operation
is chipset-dependent, and ideally would be encapsulated within the
AGP/GART module. I don't see any reason why the memory[] table needs to
contain the "masked" values, so I simply put the unmasked values there
(the unmodified physical addresses).
The place where the masked values are needed is when they are actually
inserted in the GATT, the table read by the chipset. Before my patch, the
memory allocation functions apply the mask operation and put the result
into memory[]. The functions that insert into the GATT just copied the
values from memory[]. My patch changes things so that the memory
allocation functions do not apply the mask, but rather put the unmodified
physical addresses into memory[]. The GATT insertion functions then take
the values from memory[], apply the mask operation, *then* put them into
the GATT.
Make sense?
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
--- End Message ---
