Re: [PATCH 03/18] drm/i915/gtt: Allow >= 4GB sizes for vm.

Daniel Vetter
2015-06-26T08:45:20Z

On Thu, Jun 25, 2015 at 06:46:06PM +0100, Michel Thierry wrote:
> On 6/25/2015 4:35 PM, Mika Kuoppala wrote:
> >We can have exactly 4GB sized ppgtt with 32bit system.
> >size_t is inadequate for this.
> >
> >v2: Convert a lot more places (Daniel)
>
> Looks good to me.
>
> The only possible remaining size_t are the return values in the *_pte_count
> functions in i915_gem_gtt.h, but these won't return anything larger than
> '512' (so they could be u32 if we want to get rid of size_t completely).
>
> I also tried reverting commit 501fd70f ("drm/i915: limit PPGTT size to 2GB
> in 32-bit platforms") in a bdw running a 32-bit kernel and it worked fine.
>
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

i915_gem_obj_offset is the first one I've checked and still unsigned long.
Essentially everything dealing with a g/ppgtt offset needs to be an u64,
starting from drm_mm down to gtt pte writing on the other end.

I merged up to this patch meanwhile.
-Daniel

>
> >
> >Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >---
> > drivers/char/agp/intel-gtt.c | 4 ++--
> > drivers/gpu/drm/i915/i915_debugfs.c | 42 ++++++++++++++++++-------------------
> > drivers/gpu/drm/i915/i915_gem.c | 6 +++---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +++++++++----------
> > drivers/gpu/drm/i915/i915_gem_gtt.h | 12 +++++------
> > include/drm/intel-gtt.h | 4 ++--
> > 6 files changed, 45 insertions(+), 45 deletions(-)
> >
> >diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> >index 0b4188b..4734d02 100644
> >--- a/drivers/char/agp/intel-gtt.c
> >+++ b/drivers/char/agp/intel-gtt.c
> >@@ -1408,8 +1408,8 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
> > }
> > EXPORT_SYMBOL(intel_gmch_probe);
> >
> >-void intel_gtt_get(size_t *gtt_total, size_t *stolen_size,
> >- phys_addr_t *mappable_base, unsigned long *mappable_end)
> >+void intel_gtt_get(u64 *gtt_total, size_t *stolen_size,
> >+ phys_addr_t *mappable_base, u64 *mappable_end)
> > {
> > *gtt_total = intel_private.gtt_total_entries << PAGE_SHIFT;
> > *stolen_size = intel_private.stolen_size;
> >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> >index f3b8062..c654b7d 100644
> >--- a/drivers/gpu/drm/i915/i915_debugfs.c
> >+++ b/drivers/gpu/drm/i915/i915_debugfs.c
> >@@ -198,7 +198,7 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct i915_address_space *vm = &dev_priv->gtt.base;
> > struct i915_vma *vma;
> >- size_t total_obj_size, total_gtt_size;
> >+ u64 total_obj_size, total_gtt_size;
> > int count, ret;
> >
> > ret = mutex_lock_interruptible(&dev->struct_mutex);
> >@@ -231,7 +231,7 @@ static int i915_gem_object_list_info(struct seq_file *m, void *data)
> > }
> > mutex_unlock(&dev->struct_mutex);
> >
> >- seq_printf(m, "Total %d objects, %zu bytes, %zu GTT size\n",
> >+ seq_printf(m, "Total %d objects, %llu bytes, %llu GTT size\n",
> > count, total_obj_size, total_gtt_size);
> > return 0;
> > }
> >@@ -253,7 +253,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> > struct drm_device *dev = node->minor->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj;
> >- size_t total_obj_size, total_gtt_size;
> >+ u64 total_obj_size, total_gtt_size;
> > LIST_HEAD(stolen);
> > int count, ret;
> >
> >@@ -292,7 +292,7 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> > }
> > mutex_unlock(&dev->struct_mutex);
> >
> >- seq_printf(m, "Total %d objects, %zu bytes, %zu GTT size\n",
> >+ seq_printf(m, "Total %d objects, %llu bytes, %llu GTT size\n",
> > count, total_obj_size, total_gtt_size);
> > return 0;
> > }
> >@@ -310,10 +310,10 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
> >
> > struct file_stats {
> > struct drm_i915_file_private *file_priv;
> >- int count;
> >- size_t total, unbound;
> >- size_t global, shared;
> >- size_t active, inactive;
> >+ unsigned long count;
> >+ u64 total, unbound;
> >+ u64 global, shared;
> >+ u64 active, inactive;
> > };
> >
> > static int per_file_stats(int id, void *ptr, void *data)
> >@@ -370,7 +370,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> >
> > #define print_file_stats(m, name, stats) do { \
> > if (stats.count) \
> >- seq_printf(m, "%s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu shared, %zu unbound)\n", \
> >+ seq_printf(m, "%s: %lu objects, %llu bytes (%llu active, %llu inactive, %llu global, %llu shared, %llu unbound)\n", \
> > name, \
> > stats.count, \
> > stats.total, \
> >@@ -420,7 +420,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> > struct drm_device *dev = node->minor->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 count, mappable_count, purgeable_count;
> >- size_t size, mappable_size, purgeable_size;
> >+ u64 size, mappable_size, purgeable_size;
> > struct drm_i915_gem_object *obj;
> > struct i915_address_space *vm = &dev_priv->gtt.base;
> > struct drm_file *file;
> >@@ -437,17 +437,17 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> >
> > size = count = mappable_size = mappable_count = 0;
> > count_objects(&dev_priv->mm.bound_list, global_list);
> >- seq_printf(m, "%u [%u] objects, %zu [%zu] bytes in gtt\n",
> >+ seq_printf(m, "%u [%u] objects, %llu [%llu] bytes in gtt\n",
> > count, mappable_count, size, mappable_size);
> >
> > size = count = mappable_size = mappable_count = 0;
> > count_vmas(&vm->active_list, mm_list);
> >- seq_printf(m, " %u [%u] active objects, %zu [%zu] bytes\n",
> >+ seq_printf(m, " %u [%u] active objects, %llu [%llu] bytes\n",
> > count, mappable_count, size, mappable_size);
> >
> > size = count = mappable_size = mappable_count = 0;
> > count_vmas(&vm->inactive_list, mm_list);
> >- seq_printf(m, " %u [%u] inactive objects, %zu [%zu] bytes\n",
> >+ seq_printf(m, " %u [%u] inactive objects, %llu [%llu] bytes\n",
> > count, mappable_count, size, mappable_size);
> >
> > size = count = purgeable_size = purgeable_count = 0;
> >@@ -456,7 +456,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> > if (obj->madv == I915_MADV_DONTNEED)
> > purgeable_size += obj->base.size, ++purgeable_count;
> > }
> >- seq_printf(m, "%u unbound objects, %zu bytes\n", count, size);
> >+ seq_printf(m, "%u unbound objects, %llu bytes\n", count, size);
> >
> > size = count = mappable_size = mappable_count = 0;
> > list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> >@@ -473,16 +473,16 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
> > ++purgeable_count;
> > }
> > }
> >- seq_printf(m, "%u purgeable objects, %zu bytes\n",
> >+ seq_printf(m, "%u purgeable objects, %llu bytes\n",
> > purgeable_count, purgeable_size);
> >- seq_printf(m, "%u pinned mappable objects, %zu bytes\n",
> >+ seq_printf(m, "%u pinned mappable objects, %llu bytes\n",
> > mappable_count, mappable_size);
> >- seq_printf(m, "%u fault mappable objects, %zu bytes\n",
> >+ seq_printf(m, "%u fault mappable objects, %llu bytes\n",
> > count, size);
> >
> >- seq_printf(m, "%zu [%lu] gtt total\n",
> >+ seq_printf(m, "%llu [%llu] gtt total\n",
> > dev_priv->gtt.base.total,
> >- dev_priv->gtt.mappable_end - dev_priv->gtt.base.start);
> >+ (u64)dev_priv->gtt.mappable_end - dev_priv->gtt.base.start);
> >
> > seq_putc(m, '\n');
> > print_batch_pool_stats(m, dev_priv);
> >@@ -519,7 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
> > uintptr_t list = (uintptr_t) node->info_ent->data;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct drm_i915_gem_object *obj;
> >- size_t total_obj_size, total_gtt_size;
> >+ u64 total_obj_size, total_gtt_size;
> > int count, ret;
> >
> > ret = mutex_lock_interruptible(&dev->struct_mutex);
> >@@ -541,7 +541,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
> >
> > mutex_unlock(&dev->struct_mutex);
> >
> >- seq_printf(m, "Total %d objects, %zu bytes, %zu GTT size\n",
> >+ seq_printf(m, "Total %d objects, %llu bytes, %llu GTT size\n",
> > count, total_obj_size, total_gtt_size);
> >
> > return 0;
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 52efe43..cc5bf93 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3712,9 +3712,9 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > struct drm_device *dev = obj->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 size, fence_size, fence_alignment, unfenced_alignment;
> >- unsigned long start =
> >+ u64 start =
> > flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> >- unsigned long end =
> >+ u64 end =
> > flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
> > struct i915_vma *vma;
> > int ret;
> >@@ -3770,7 +3770,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > * attempt to find space.
> > */
> > if (size > end) {
> >- DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
> >+ DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: size=%u > %s aperture=%llu\n",
> > ggtt_view ? ggtt_view->type : 0,
> > size,
> > flags & PIN_MAPPABLE ? "mappable" : "total",
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >index 68705e3..7a7789e 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -2112,7 +2112,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
> > void i915_gem_init_global_gtt(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >- unsigned long gtt_size, mappable_size;
> >+ u64 gtt_size, mappable_size;
> >
> > gtt_size = dev_priv->gtt.base.total;
> > mappable_size = dev_priv->gtt.mappable_end;
> >@@ -2369,13 +2369,13 @@ static void chv_setup_private_ppat(struct drm_i915_private *dev_priv)
> > }
> >
> > static int gen8_gmch_probe(struct drm_device *dev,
> >- size_t *gtt_total,
> >+ u64 *gtt_total,
> > size_t *stolen,
> > phys_addr_t *mappable_base,
> >- unsigned long *mappable_end)
> >+ u64 *mappable_end)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >- unsigned int gtt_size;
> >+ u64 gtt_size;
> > u16 snb_gmch_ctl;
> > int ret;
> >
> >@@ -2417,10 +2417,10 @@ static int gen8_gmch_probe(struct drm_device *dev,
> > }
> >
> > static int gen6_gmch_probe(struct drm_device *dev,
> >- size_t *gtt_total,
> >+ u64 *gtt_total,
> > size_t *stolen,
> > phys_addr_t *mappable_base,
> >- unsigned long *mappable_end)
> >+ u64 *mappable_end)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > unsigned int gtt_size;
> >@@ -2434,7 +2434,7 @@ static int gen6_gmch_probe(struct drm_device *dev,
> > * a coarse sanity check.
> > */
> > if ((*mappable_end < (64<<20) || (*mappable_end > (512<<20)))) {
> >- DRM_ERROR("Unknown GMADR size (%lx)\n",
> >+ DRM_ERROR("Unknown GMADR size (%llx)\n",
> > dev_priv->gtt.mappable_end);
> > return -ENXIO;
> > }
> >@@ -2468,10 +2468,10 @@ static void gen6_gmch_remove(struct i915_address_space *vm)
> > }
> >
> > static int i915_gmch_probe(struct drm_device *dev,
> >- size_t *gtt_total,
> >+ u64 *gtt_total,
> > size_t *stolen,
> > phys_addr_t *mappable_base,
> >- unsigned long *mappable_end)
> >+ u64 *mappable_end)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int ret;
> >@@ -2536,9 +2536,9 @@ int i915_gem_gtt_init(struct drm_device *dev)
> > gtt->base.dev = dev;
> >
> > /* GMADR is the PCI mmio aperture into the global GTT. */
> >- DRM_INFO("Memory usable by graphics device = %zdM\n",
> >+ DRM_INFO("Memory usable by graphics device = %lluM\n",
> > gtt->base.total >> 20);
> >- DRM_DEBUG_DRIVER("GMADR size = %ldM\n", gtt->mappable_end >> 20);
> >+ DRM_DEBUG_DRIVER("GMADR size = %lldM\n", gtt->mappable_end >> 20);
> > DRM_DEBUG_DRIVER("GTT stolen size = %zdM\n", gtt->stolen_size >> 20);
> > #ifdef CONFIG_INTEL_IOMMU
> > if (intel_iommu_gfx_mapped)
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >index 017ea30..600eec0 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> >@@ -235,8 +235,8 @@ struct i915_address_space {
> > struct drm_mm mm;
> > struct drm_device *dev;
> > struct list_head global_link;
> >- unsigned long start; /* Start offset always 0 for dri2 */
> >- size_t total; /* size addr space maps (ex. 2GB for ggtt) */
> >+ u64 start; /* Start offset always 0 for dri2 */
> >+ u64 total; /* size addr space maps (ex. 2GB for ggtt) */
> >
> > struct {
> > dma_addr_t addr;
> >@@ -302,9 +302,9 @@ struct i915_address_space {
> > */
> > struct i915_gtt {
> > struct i915_address_space base;
> >- size_t stolen_size; /* Total size of stolen memory */
> >
> >- unsigned long mappable_end; /* End offset that we can CPU map */
> >+ size_t stolen_size; /* Total size of stolen memory */
> >+ u64 mappable_end; /* End offset that we can CPU map */
> > struct io_mapping *mappable; /* Mapping to our CPU mappable region */
> > phys_addr_t mappable_base; /* PA of our GMADR */
> >
> >@@ -316,9 +316,9 @@ struct i915_gtt {
> > int mtrr;
> >
> > /* global gtt ops */
> >- int (*gtt_probe)(struct drm_device *dev, size_t *gtt_total,
> >+ int (*gtt_probe)(struct drm_device *dev, u64 *gtt_total,
> > size_t *stolen, phys_addr_t *mappable_base,
> >- unsigned long *mappable_end);
> >+ u64 *mappable_end);
> > };
> >
> > struct i915_hw_ppgtt {
> >diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> >index b08bdad..9e9bddaa5 100644
> >--- a/include/drm/intel-gtt.h
> >+++ b/include/drm/intel-gtt.h
> >@@ -3,8 +3,8 @@
> > #ifndef _DRM_INTEL_GTT_H
> > #define _DRM_INTEL_GTT_H
> >
> >-void intel_gtt_get(size_t *gtt_total, size_t *stolen_size,
> >- phys_addr_t *mappable_base, unsigned long *mappable_end);
> >+void intel_gtt_get(u64 *gtt_total, size_t *stolen_size,
> >+ phys_addr_t *mappable_base, u64 *mappable_end);
> >
> > int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev,
> > struct agp_bridge_data *bridge);
> >--
> >1.9.1
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation