Reviewed-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
On Wed, Jan 16, 2013 at 4:22 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> The reasoning behind our code taking two paths depending upon whether or
> not we may have been configured for IOMMU isn't clear to me. It should
> always be safe to use the pci mapping functions as they are designed to
> abstract the decision we were handling in i915.
>
> Aside from simpler code, removing another member for the intel_gtt
> struct is a nice motivation.
>
> I ran this by Chris, and he wasn't concerned about the extra kzalloc,
> and memory references vs. page_to_phys calculation in the case without
> IOMMU.
>
> v2: Update commit message
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/char/agp/intel-gtt.c | 10 ++++++----
> drivers/gpu/drm/i915/i915_gem_gtt.c | 35 +++++++++++++----------------------
> include/drm/intel-gtt.h | 2 --
> 3 files changed, 19 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index eb05eb5..a531377 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -84,6 +84,8 @@ static struct _intel_private {
> * this is not the full gtt. */
> unsigned int gtt_mappable_entries;
> phys_addr_t gma_bus_addr;
> + /* Whether i915 needs to use the dmar apis or not. */
> + unsigned int needs_dmar : 1;
> } intel_private;
>
> #define INTEL_GTT_GEN intel_private.driver->gen
> @@ -299,7 +301,7 @@ static int intel_gtt_setup_scratch_page(void)
> get_page(page);
> set_pages_uc(page, 1);
>
> - if (intel_private.base.needs_dmar) {
> + if (intel_private.needs_dmar) {
> dma_addr = pci_map_page(intel_private.pcidev, page, 0,
> PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
> if (pci_dma_mapping_error(intel_private.pcidev, dma_addr))
> @@ -615,7 +617,7 @@ static int intel_gtt_init(void)
>
> intel_private.stolen_size = intel_gtt_stolen_size();
>
> - intel_private.base.needs_dmar = USE_PCI_DMA_API && INTEL_GTT_GEN > 2;
> + intel_private.needs_dmar = USE_PCI_DMA_API && INTEL_GTT_GEN > 2;
>
> ret = intel_gtt_setup_scratch_page();
> if (ret != 0) {
> @@ -875,7 +877,7 @@ static int intel_fake_agp_insert_entries(struct agp_memory *mem,
> if (!mem->is_flushed)
> global_cache_flush();
>
> - if (intel_private.base.needs_dmar) {
> + if (intel_private.needs_dmar) {
> struct sg_table st;
>
> ret = intel_gtt_map_memory(mem->pages, mem->page_count, &st);
> @@ -919,7 +921,7 @@ static int intel_fake_agp_remove_entries(struct agp_memory *mem,
>
> intel_gtt_clear_range(pg_start, mem->page_count);
>
> - if (intel_private.base.needs_dmar) {
> + if (intel_private.needs_dmar) {
> intel_gtt_unmap_memory(mem->sg_list, mem->num_sg);
> mem->sg_list = NULL;
> mem->num_sg = 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a92d8cd..ae96835 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -139,28 +139,23 @@ int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> goto err_pt_alloc;
> }
>
> - if (dev_priv->mm.gtt->needs_dmar) {
> - ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t)
> - *ppgtt->num_pd_entries,
> - GFP_KERNEL);
> - if (!ppgtt->pt_dma_addr)
> - goto err_pt_alloc;
> + ppgtt->pt_dma_addr = kzalloc(sizeof(dma_addr_t) *ppgtt->num_pd_entries,
> + GFP_KERNEL);
> + if (!ppgtt->pt_dma_addr)
> + goto err_pt_alloc;
>
> - for (i = 0; i < ppgtt->num_pd_entries; i++) {
> - dma_addr_t pt_addr;
> + for (i = 0; i < ppgtt->num_pd_entries; i++) {
> + dma_addr_t pt_addr;
>
> - pt_addr = pci_map_page(dev->pdev, ppgtt->pt_pages[i],
> - 0, 4096,
> - PCI_DMA_BIDIRECTIONAL);
> + pt_addr = pci_map_page(dev->pdev, ppgtt->pt_pages[i], 0, 4096,
> + PCI_DMA_BIDIRECTIONAL);
>
> - if (pci_dma_mapping_error(dev->pdev,
> - pt_addr)) {
> - ret = -EIO;
> - goto err_pd_pin;
> + if (pci_dma_mapping_error(dev->pdev, pt_addr)) {
> + ret = -EIO;
> + goto err_pd_pin;
>
> - }
> - ppgtt->pt_dma_addr[i] = pt_addr;
> }
> + ppgtt->pt_dma_addr[i] = pt_addr;
> }
>
> ppgtt->scratch_page_dma_addr = dev_priv->mm.gtt->scratch_page_dma;
> @@ -295,11 +290,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
> for (i = 0; i < ppgtt->num_pd_entries; i++) {
> dma_addr_t pt_addr;
>
> - if (dev_priv->mm.gtt->needs_dmar)
> - pt_addr = ppgtt->pt_dma_addr[i];
> - else
> - pt_addr = page_to_phys(ppgtt->pt_pages[i]);
> -
> + pt_addr = ppgtt->pt_dma_addr[i];
> pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr);
> pd_entry |= GEN6_PDE_VALID;
>
> diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> index 6f53ecd..63157c5 100644
> --- a/include/drm/intel-gtt.h
> +++ b/include/drm/intel-gtt.h
> @@ -4,8 +4,6 @@
> #define _DRM_INTEL_GTT_H
>
> struct intel_gtt {
> - /* Whether i915 needs to use the dmar apis or not. */
> - unsigned int needs_dmar : 1;
> /* Whether we idle the gpu before mapping/unmapping */
> unsigned int do_idle_maps : 1;
> /* Share the scratch page dma with ppgtts. */
> --
> 1.8.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
