| Message ID | 1458290579-27783-4-git-send-email-joonas.lahtinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/03/16 08:42, Joonas Lahtinen wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Throughout the code base, we use u32 for offsets into the global GTT. If > we ever see any hardware with a larger GGTT, then we run the real risk > of silent corruption. So test for our assumption up front so that we > have a nice reminder should the time come when it fails. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > [Rebased and changed 1ull -> 1ULL] > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 8a9fa03..ba144ba 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3191,6 +3191,13 @@ int i915_gem_gtt_init(struct drm_device *dev) > if (ret) > return ret; > > + if ((ggtt->base.total - 1) >> 32) { > + DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %lldM!\n", > + ggtt->base.total >> 20); MB or MiB? > + ggtt->base.total = 1ULL << 32; > + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); > + } > + > /* > * Initialise stolen early so that we may reserve preallocated > * objects for the BIOS to KMS transition. > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Fri, Mar 18, 2016 at 10:56:55AM +0000, Tvrtko Ursulin wrote: > > On 18/03/16 08:42, Joonas Lahtinen wrote: > >From: Chris Wilson <chris@chris-wilson.co.uk> > > > >Throughout the code base, we use u32 for offsets into the global GTT. If > >we ever see any hardware with a larger GGTT, then we run the real risk > >of silent corruption. So test for our assumption up front so that we > >have a nice reminder should the time come when it fails. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Cc: Daniel Vetter <daniel@ffwll.ch> > >Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >[Rebased and changed 1ull -> 1ULL] > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > >index 8a9fa03..ba144ba 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >@@ -3191,6 +3191,13 @@ int i915_gem_gtt_init(struct drm_device *dev) > > if (ret) > > return ret; > > > >+ if ((ggtt->base.total - 1) >> 32) { > >+ DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %lldM!\n", > >+ ggtt->base.total >> 20); > > MB or MiB? if ((ggtt->base.total - 1) >> 32) { char buf[10]; string_get_size(ggtt->base.total, 1, STRING_UNITS_2, buf, sizeof(buf)); DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %s [%dbits]\n", buf, fls(size)); } -Chris
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes: > [ text/plain ] > From: Chris Wilson <chris@chris-wilson.co.uk> > > Throughout the code base, we use u32 for offsets into the global GTT. If > we ever see any hardware with a larger GGTT, then we run the real risk > of silent corruption. So test for our assumption up front so that we > have a nice reminder should the time come when it fails. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > [Rebased and changed 1ull -> 1ULL] > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 8a9fa03..ba144ba 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -3191,6 +3191,13 @@ int i915_gem_gtt_init(struct drm_device *dev) > if (ret) > return ret; > > + if ((ggtt->base.total - 1) >> 32) { > + DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %lldM!\n", > + ggtt->base.total >> 20); > + ggtt->base.total = 1ULL << 32; > + ggtt->mappable_end = min(ggtt->mappable_end, > ggtt->base.total); mappable_end is not u64 so perhaps s/min/min_t(u64, The mappable_end is really confusing after you realize it is used in 2 different contexts where only in the other, it holds true. In the pa space, it definitely is not end but len. If we would rename it to mappable_len it would be ambivalent to context and more clear. With min_t change, Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > + } > + > /* > * Initialise stolen early so that we may reserve preallocated > * objects for the BIOS to KMS transition. > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Mika Kuoppala <mika.kuoppala@linux.intel.com> writes: > [ text/plain ] > Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes: > >> [ text/plain ] >> From: Chris Wilson <chris@chris-wilson.co.uk> >> >> Throughout the code base, we use u32 for offsets into the global GTT. If >> we ever see any hardware with a larger GGTT, then we run the real risk >> of silent corruption. So test for our assumption up front so that we >> have a nice reminder should the time come when it fails. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> [Rebased and changed 1ull -> 1ULL] >> --- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 8a9fa03..ba144ba 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -3191,6 +3191,13 @@ int i915_gem_gtt_init(struct drm_device *dev) >> if (ret) >> return ret; >> >> + if ((ggtt->base.total - 1) >> 32) { >> + DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %lldM!\n", >> + ggtt->base.total >> 20); >> + ggtt->base.total = 1ULL << 32; >> + ggtt->mappable_end = min(ggtt->mappable_end, >> ggtt->base.total); > > mappable_end is not u64 so perhaps > s/min/min_t(u64, > Joonas pointed in irc that it is u64. Have been quite long already. You can add my r-b without any change. -Mika > The mappable_end is really confusing after you realize it is used in 2 > different contexts where only in the other, it holds true. In the pa > space, it definitely is not end but len. If we would rename it to > mappable_len it would be ambivalent to context and more clear. > > With min_t change, > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> + } >> + >> /* >> * Initialise stolen early so that we may reserve preallocated >> * objects for the BIOS to KMS transition. >> -- >> 2.5.0 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 8a9fa03..ba144ba 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3191,6 +3191,13 @@ int i915_gem_gtt_init(struct drm_device *dev) if (ret) return ret; + if ((ggtt->base.total - 1) >> 32) { + DRM_ERROR("We never expected a Global GTT with more than 32bits of address space! Found %lldM!\n", + ggtt->base.total >> 20); + ggtt->base.total = 1ULL << 32; + ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total); + } + /* * Initialise stolen early so that we may reserve preallocated * objects for the BIOS to KMS transition.