Re: [RFC] Make AGP work with IOMMU

[Dave Airlie]

On Wed, Jul 29, 2009 at 5:15 PM, David Woodhouse<dwmw2@xxxxxxxxxxxxx> wrote:
> 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?

Yes, more than likely. I think radeon and radeon kms drivers dtrt, at
least they
call pci_map_single for the pages they use in their on-chip translation units,
and the cards have been used in sparc64 and ppc64 with iommu stuff happening.

nouveau might work when it gets upstream but it may not.

Other PCI and/or PCIE1x cards still around would be MGA and SiS based,
and I suspect
they would need work, but they might just work.

nvidia/fglrx binary drivers, who knows.

>
>> >                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?

Yeah that could work too, no idea why they were introduced, well before my time.

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

Yes that might be best, having a function called map_page taking an
address seems wrong.

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

Yeah we should drop both of them maybe I can do that as a cleanup
after this patch.

>
>> (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?

no idea, I think I've seen vmalloc used in other places before.

>
>> > +       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@xxxxxxxxx                              Intel Corporation
>
>

Dave.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/