lkml.org 
[lkml]   [2009]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC] Make AGP work with IOMMU
From
> Based on some patches from Zhenyu that I found at
> http://people.freedesktop.org/~zhen/agp-mm-*, this set of patches stops
> the Intel graphics drivers from being utterly broken when the IOMMU is
> enabled...
>
>        http://git.infradead.org/users/dwmw2/iommu-agp.git
>
> I'm still seeing write faults to seemingly random addresses at startup
> when I first enable the IOMMU (and before the gfx driver actually
> starts). Those happen even when I don't use any graphics drivers at all,
> though.
>
> And I'm slightly confused about the way we use the scratch page without
> mask_memory().

Looks good, some comments below:

> commit d8450c7be46afd0b51d2dc171c9d2eb60366ce05
> Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
> Date:   Mon Jul 27 12:59:57 2009 +0100
>
>    intel_agp: Use PCI DMA API correctly on chipsets new enough to have IOMMU
>
>    When graphics dma remapping engine is active, we must fill
>    gart table with dma address from dmar engine, as now graphics
>    device access to graphics memory must go through dma remapping
>    table to get real physical address.
>
>    Add this support to all drivers which use intel_i915_insert_entries()
>
>    Signed-off-by: Zhenyu Wang <zhenyu.z.wang@intel.com>
>    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> commit ca8cdb1f983c508f586b90870703e85aab87659a
> Author: Zhenyu Wang <zhenyu.z.wang@intel.com>
> Date:   Thu Jul 23 17:25:49 2009 +0100
>
>    agp: Add generic support for graphics dma remapping
>
>    New driver hooks for support graphics memory dma remapping
>    are introduced in this patch. It makes generic code can
>    tell if current device needs dma remapping, then call driver
>    provided interfaces for mapping and unmapping. Change has
>    also been made to handle scratch_page in remapping case.
>
>    Signed-off-by: Zhenyu Wang <zhenyu.z.wang@intel.com>
>    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> commit 42a509b9cd513ddd536c38cc302c7457a3c811d7
> Author: David Woodhouse <David.Woodhouse@intel.com>
> Date:   Mon Jul 27 10:27:29 2009 +0100
>
>    agp: Switch mask_memory() method to take address argument again, not page
>
>    In commit 07613ba2 ("agp: switch AGP to use page array instead of
>    unsigned long array") we switched the mask_memory() method to take a
>    'struct page *' instead of an address. This is painful, because in some
>    cases it has to be an IOMMU-mapped virtual bus address (in fact,
>    shouldn't it _always_ be a dma_addr_t returned from pci_map_xxx(), and
>    we just happen to get lucky most of the time?)

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.

>


>  struct agp_bridge_data *agp_generic_find_bridge(struct pci_dev *pdev);
> diff --git a/drivers/char/agp/amd-k7-agp.c b/drivers/char/agp/amd-k7-agp.c
> index ba9bde7..542a878 100644
> --- a/drivers/char/agp/amd-k7-agp.c
> +++ b/drivers/char/agp/amd-k7-agp.c
> @@ -325,7 +325,9 @@ static int amd_insert_memory(struct agp_memory *mem, off_t pg_start, int type)
>                addr = (j * PAGE_SIZE) + agp_bridge->gart_bus_addr;
>                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?

> diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
> index cfa5a64..19ac366 100644
> --- a/drivers/char/agp/backend.c
> +++ b/drivers/char/agp/backend.c
> @@ -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.

>
> +#ifdef USE_PCI_DMA_API
> +static int intel_agp_map_page(void *addr, dma_addr_t *ret)
> +{
> +       *ret = pci_map_single(intel_private.pcidev, addr,
> +                             PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> +       if (pci_dma_mapping_error(intel_private.pcidev, *ret))
> +               return -EINVAL;
> +       return 0;
> +}
> +
> +static void intel_agp_unmap_page(void *addr, dma_addr_t dma)
> +{
> +       pci_unmap_single(intel_private.pcidev, dma,
> +                        PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> +}
> +
> +static int intel_agp_map_memory(struct agp_memory *mem)
> +{
> +       struct scatterlist *sg;
> +       int i;
> +
> +       DBG("try mapping %lu pages\n", (unsigned long)mem->page_count);
> +
> +       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?

(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.)

> +       }
> +
> +       if (!mem->sg_list) {
> +               mem->sg_vmalloc_flag = 0;
> +               return -ENOMEM;
> +       }
> +       sg_init_table(mem->sg_list, mem->page_count);
> +
> +       sg = mem->sg_list;
> +       for (i = 0 ; i < mem->page_count; i++, sg = sg_next(sg))
> +               sg_set_page(sg, mem->pages[i], PAGE_SIZE, 0);
> +
> +       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?

> +               return -ENOMEM;
> +       }
> +       return 0;
> +}
> +
> +static void intel_agp_unmap_memory(struct agp_memory *mem)
> +{
> +       DBG("try unmapping %lu pages\n", (unsigned long)mem->page_count);
> +
> +       pci_unmap_sg(intel_private.pcidev, mem->sg_list,
> +                    mem->page_count, PCI_DMA_BIDIRECTIONAL);
> +       if (mem->sg_vmalloc_flag)
> +               vfree(mem->sg_list);
> +       else
> +               kfree(mem->sg_list);
> +       mem->sg_vmalloc_flag = 0;
> +       mem->sg_list = NULL;

since we reproduce it here.

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

\
 
 \ /
  Last update: 2009-07-29 08:31    [W:0.062 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site