| Message ID | 20170926192908.28055-2-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Paulo Zanoni (2017-09-26 20:29:08) > Stolen memory pointers are dma_addr_t, which means they can be 64 bit > things. By using u32 we leave room for bugs in case we ever get huge > amounts of stolen memory. By using size_t we don't risk running into > those problems. What platform? -Chris
Quoting Paulo Zanoni (2017-09-26 20:29:08) > Stolen memory pointers are dma_addr_t, which means they can be 64 bit > things. By using u32 we leave room for bugs in case we ever get huge > amounts of stolen memory. By using size_t we don't risk running into > those problems. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/char/agp/intel-gtt.c | 10 +++++----- > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- > drivers/gpu/drm/i915/i915_gem_stolen.c | 19 +++++++++---------- > include/drm/intel-gtt.h | 2 +- > 5 files changed, 19 insertions(+), 20 deletions(-) > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > index 9b6b602..a1db230 100644 > --- a/drivers/char/agp/intel-gtt.c > +++ b/drivers/char/agp/intel-gtt.c > @@ -80,7 +80,7 @@ static struct _intel_private { > unsigned int needs_dmar : 1; > phys_addr_t gma_bus_addr; > /* Size of memory reserved for graphics by the BIOS */ > - unsigned int stolen_size; > + size_t stolen_size; What is size_t? How does that correspond to a physical or dma addr? You either meant kernel_size_t or unsigned long, or a proper type for the address space. -Chris
On Tue, 2017-09-26 at 21:13 +0100, Chris Wilson wrote: > Quoting Paulo Zanoni (2017-09-26 20:29:08) > > Stolen memory pointers are dma_addr_t, which means they can be 64 bit > > things. By using u32 we leave room for bugs in case we ever get huge > > amounts of stolen memory. By using size_t we don't risk running into > > those problems. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/char/agp/intel-gtt.c | 10 +++++----- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- > > drivers/gpu/drm/i915/i915_gem_stolen.c | 19 +++++++++---------- > > include/drm/intel-gtt.h | 2 +- > > 5 files changed, 19 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > > index 9b6b602..a1db230 100644 > > --- a/drivers/char/agp/intel-gtt.c > > +++ b/drivers/char/agp/intel-gtt.c > > @@ -80,7 +80,7 @@ static struct _intel_private { > > unsigned int needs_dmar : 1; > > phys_addr_t gma_bus_addr; > > /* Size of memory reserved for graphics by the BIOS */ > > - unsigned int stolen_size; > > + size_t stolen_size; > > What is size_t? How does that correspond to a physical or dma addr? > You either meant kernel_size_t or unsigned long, or a proper type for > the address space. We're using phys_addr_t + size_t in early-quirks.c too, so we want to keep both places consistent. If we're using something else than size_t, then we should update both places (it's still on my todo to get rid of the code duplication). Regards, Joonas
Quoting Joonas Lahtinen (2017-09-29 10:23:10) > On Tue, 2017-09-26 at 21:13 +0100, Chris Wilson wrote: > > Quoting Paulo Zanoni (2017-09-26 20:29:08) > > > Stolen memory pointers are dma_addr_t, which means they can be 64 bit > > > things. By using u32 we leave room for bugs in case we ever get huge > > > amounts of stolen memory. By using size_t we don't risk running into > > > those problems. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/char/agp/intel-gtt.c | 10 +++++----- > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- > > > drivers/gpu/drm/i915/i915_gem_stolen.c | 19 +++++++++---------- > > > include/drm/intel-gtt.h | 2 +- > > > 5 files changed, 19 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > > > index 9b6b602..a1db230 100644 > > > --- a/drivers/char/agp/intel-gtt.c > > > +++ b/drivers/char/agp/intel-gtt.c > > > @@ -80,7 +80,7 @@ static struct _intel_private { > > > unsigned int needs_dmar : 1; > > > phys_addr_t gma_bus_addr; > > > /* Size of memory reserved for graphics by the BIOS */ > > > - unsigned int stolen_size; > > > + size_t stolen_size; > > > > What is size_t? How does that correspond to a physical or dma addr? > > You either meant kernel_size_t or unsigned long, or a proper type for > > the address space. > > We're using phys_addr_t + size_t in early-quirks.c too, so we want to > keep both places consistent. If we're using something else than size_t, > then we should update both places (it's still on my todo to get rid of > the code duplication). > Re duplication: move the discovery into early-quirks and export the resource_t ? -Chris
On Fri, 2017-09-29 at 10:40 +0100, Chris Wilson wrote: > Quoting Joonas Lahtinen (2017-09-29 10:23:10) > > On Tue, 2017-09-26 at 21:13 +0100, Chris Wilson wrote: > > > Quoting Paulo Zanoni (2017-09-26 20:29:08) > > > > Stolen memory pointers are dma_addr_t, which means they can be 64 bit > > > > things. By using u32 we leave room for bugs in case we ever get huge > > > > amounts of stolen memory. By using size_t we don't risk running into > > > > those problems. > > > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/char/agp/intel-gtt.c | 10 +++++----- > > > > drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- > > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- > > > > drivers/gpu/drm/i915/i915_gem_stolen.c | 19 +++++++++---------- > > > > include/drm/intel-gtt.h | 2 +- > > > > 5 files changed, 19 insertions(+), 20 deletions(-) > > > > > > > > diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c > > > > index 9b6b602..a1db230 100644 > > > > --- a/drivers/char/agp/intel-gtt.c > > > > +++ b/drivers/char/agp/intel-gtt.c > > > > @@ -80,7 +80,7 @@ static struct _intel_private { > > > > unsigned int needs_dmar : 1; > > > > phys_addr_t gma_bus_addr; > > > > /* Size of memory reserved for graphics by the BIOS */ > > > > - unsigned int stolen_size; > > > > + size_t stolen_size; > > > > > > What is size_t? How does that correspond to a physical or dma addr? > > > You either meant kernel_size_t or unsigned long, or a proper type for > > > the address space. > > > > We're using phys_addr_t + size_t in early-quirks.c too, so we want to > > keep both places consistent. If we're using something else than size_t, > > then we should update both places (it's still on my todo to get rid of > > the code duplication). > > > > Re duplication: move the discovery into early-quirks and export the > resource_t ? Might be an idea, the biggest hurdle is that now early quirks are __init so if you don't load i915, they have no impact on resident code size, they get overridden. So pretty much any way will increase either resident code or data memory size, so I've so far been looking to just share the codebase some way. Because the code applies to _all_ x86. Regards, Joonas
diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index 9b6b602..a1db230 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -80,7 +80,7 @@ static struct _intel_private { unsigned int needs_dmar : 1; phys_addr_t gma_bus_addr; /* Size of memory reserved for graphics by the BIOS */ - unsigned int stolen_size; + size_t stolen_size; /* Total number of gtt entries. */ unsigned int gtt_total_entries; /* Part of the gtt that is mappable by the cpu, for those chips where @@ -333,13 +333,13 @@ static void i810_write_entry(dma_addr_t addr, unsigned int entry, writel_relaxed(addr | pte_flags, intel_private.gtt + entry); } -static unsigned int intel_gtt_stolen_size(void) +static size_t intel_gtt_stolen_size(void) { u16 gmch_ctrl; u8 rdct; int local = 0; static const int ddt[4] = { 0, 16, 32, 64 }; - unsigned int stolen_size = 0; + size_t stolen_size = 0; if (INTEL_GTT_GEN == 1) return 0; /* no stolen mem on i81x */ @@ -417,7 +417,7 @@ static unsigned int intel_gtt_stolen_size(void) } if (stolen_size > 0) { - dev_info(&intel_private.bridge_dev->dev, "detected %dK %s memory\n", + dev_info(&intel_private.bridge_dev->dev, "detected %zuK %s memory\n", stolen_size / KB(1), local ? "local" : "stolen"); } else { dev_info(&intel_private.bridge_dev->dev, @@ -1422,7 +1422,7 @@ int intel_gmch_probe(struct pci_dev *bridge_pdev, struct pci_dev *gpu_pdev, EXPORT_SYMBOL(intel_gmch_probe); void intel_gtt_get(u64 *gtt_total, - u32 *stolen_size, + size_t *stolen_size, phys_addr_t *mappable_base, u64 *mappable_end) { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 64d7852..733fbff 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3316,7 +3316,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv) DRM_INFO("Memory usable by graphics device = %lluM\n", ggtt->base.total >> 20); DRM_DEBUG_DRIVER("GMADR size = %lldM\n", ggtt->mappable_end >> 20); - DRM_DEBUG_DRIVER("GTT stolen size = %uM\n", ggtt->stolen_size >> 20); + DRM_DEBUG_DRIVER("GTT stolen size = %zuM\n", ggtt->stolen_size >> 20); if (intel_vtd_active()) DRM_INFO("VT-d active for gfx access\n"); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index dd2ef5b..d0356be7 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -363,10 +363,10 @@ struct i915_ggtt { * avoid the first page! The upper end of stolen memory is reserved for * hardware functions and similarly removed from the accessible range. */ - u32 stolen_size; /* Total size of stolen memory */ - u32 stolen_usable_size; /* Total size minus reserved ranges */ + size_t stolen_size; /* Total size of stolen memory */ + size_t stolen_usable_size; /* Total size minus reserved ranges */ dma_addr_t stolen_reserved_base; - u32 stolen_reserved_size; + size_t stolen_reserved_size; /** "Graphics Stolen Memory" holds the global PTEs */ void __iomem *gsm; diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 507c9f0..7c9913a 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -286,7 +286,7 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) } static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv, - dma_addr_t *base, u32 *size) + dma_addr_t *base, size_t *size) { struct i915_ggtt *ggtt = &dev_priv->ggtt; uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ? @@ -309,7 +309,7 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv, } static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv, - dma_addr_t *base, u32 *size) + dma_addr_t *base, size_t *size) { uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); @@ -335,7 +335,7 @@ static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv, } static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv, - dma_addr_t *base, u32 *size) + dma_addr_t *base, size_t *size) { uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); @@ -355,7 +355,7 @@ static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv, } static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv, - dma_addr_t *base, u32 *size) + dma_addr_t *base, size_t *size) { uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); @@ -381,7 +381,7 @@ static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv, } static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv, - dma_addr_t *base, u32 *size) + dma_addr_t *base, size_t *size) { struct i915_ggtt *ggtt = &dev_priv->ggtt; uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED); @@ -405,8 +405,8 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) { struct i915_ggtt *ggtt = &dev_priv->ggtt; dma_addr_t reserved_base, stolen_top; - u32 reserved_total, reserved_size; - u32 stolen_usable_start; + size_t reserved_total, reserved_size; + size_t stolen_usable_start; mutex_init(&dev_priv->mm.stolen_lock); @@ -486,7 +486,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) * memory, so just consider the start. */ reserved_total = stolen_top - reserved_base; - DRM_DEBUG_KMS("Memory reserved for graphics device: %uK, usable: %uK\n", + DRM_DEBUG_KMS("Memory reserved for graphics device: %zuK, usable: %zuK\n", ggtt->stolen_size >> 10, (ggtt->stolen_size - reserved_total) >> 10); @@ -506,8 +506,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv) } static struct sg_table * -i915_pages_create_for_stolen(struct drm_device *dev, - u32 offset, u32 size) +i915_pages_create_for_stolen(struct drm_device *dev, size_t offset, size_t size) { struct drm_i915_private *dev_priv = to_i915(dev); struct sg_table *st; diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h index b3bf717..1f11151 100644 --- a/include/drm/intel-gtt.h +++ b/include/drm/intel-gtt.h @@ -4,7 +4,7 @@ #define _DRM_INTEL_GTT_H void intel_gtt_get(u64 *gtt_total, - u32 *stolen_size, + size_t *stolen_size, phys_addr_t *mappable_base, u64 *mappable_end);
Stolen memory pointers are dma_addr_t, which means they can be 64 bit things. By using u32 we leave room for bugs in case we ever get huge amounts of stolen memory. By using size_t we don't risk running into those problems. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/char/agp/intel-gtt.c | 10 +++++----- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- drivers/gpu/drm/i915/i915_gem_stolen.c | 19 +++++++++---------- include/drm/intel-gtt.h | 2 +- 5 files changed, 19 insertions(+), 20 deletions(-)