Messages in this thread |  | | | Subject | Re: [RFC] Make AGP work with IOMMU | | From | David Woodhouse <> | | Date | Wed, 29 Jul 2009 08:15:12 +0100 |
| |
On Wed, 2009-07-29 at 16:28 +1000, Dave Airlie wrote: > Yup pretty much we always got lucky, its not like AGP and IOMMU systems > are a huge item, its really only Intel IGPs which use the AGP > subsystem these days.
Ah, really? One thing which was bothering me was what happens when I use non-onboard graphics in one of these beasts -- are individual gfx drivers going to need to be fixed too?
> > cur_gatt = GET_GATT(addr); > > writel(agp_generic_mask_memory(agp_bridge, > > - mem->pages[i], mem->type), cur_gatt+GET_GATT_OFF(addr)); > > + phys_to_gart(page_to_phys(mem->pages[i])), > > don't suppose we want page_to_gart or is the double function nicer?
I pondered that briefly. But then observed that phys_to_gart() and gart_to_phys() _always_ describe an identity mapping, so perhaps they could just be ditched completely?
> > @@ -150,8 +150,17 @@ static int agp_backend_initialize(struct agp_bridge_data *bridge) > > } > > > > bridge->scratch_page_real = phys_to_gart(page_to_phys(page)); > > - bridge->scratch_page = > > - bridge->driver->mask_memory(bridge, page, 0); > > + bridge->scratch_page = bridge->driver->mask_memory(bridge, > > + phys_to_gart(page_to_phys(page)), 0); > > + > > + if (bridge->driver->agp_map_page && > > + bridge->driver->agp_map_page(phys_to_virt(page_to_phys(page)), > > and maybe page_to_virt.
That's called page_address(), and it (as well as the above construct) is broken with highmem pages. It's actually OK here, since this page is allocated with GFP_DMA32 -- but for cleanliness' sake I should probably switch agp_map_page() to take a 'struct page *' rather than a virtual address.
> > + if ((mem->page_count * sizeof(*mem->sg_list)) < 2*PAGE_SIZE) > > + mem->sg_list = kcalloc(mem->page_count, sizeof(*mem->sg_list), > > + GFP_KERNEL); > > + > > + if (mem->sg_list == NULL) { > > + mem->sg_list = vmalloc(mem->page_count * sizeof(*mem->sg_list)); > > + mem->sg_vmalloc_flag = 1; > > Can we drop vmalloc_flag and use is_vmalloc_addr on the free function?
I suppose so -- we could eliminate the other vmalloc_flag field in 'struct agp_memory' that way too? Doesn't shrink the structure any -- we'd just end up with padding where the flags were.
> (aside: yet another place that wants a kmalloc/vmalloc allocator. I suspect > vmalloc here to be slow but I suppose there isn't much we can do.)
http://lwn.net/Articles/342915/ ?
In fact, can't scatterlists do something like that already?
> > + mem->num_sg = pci_map_sg(intel_private.pcidev, mem->sg_list, > > + mem->page_count, PCI_DMA_BIDIRECTIONAL); > > + if (!mem->num_sg) { > > + if (mem->sg_vmalloc_flag) > > + vfree(mem->sg_list); > > + else > > + kfree(mem->sg_list); > > + mem->sg_list = NULL; > > + mem->sg_vmalloc_flag = 0; > > some common cleanup function? ... > since we reproduce it here.
Yeah, probably a good idea.
-- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
|  |