| Message ID | 1432832974-5658-1-git-send-email-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 28, 2015 at 06:09:34PM +0100, Michel Thierry wrote: > And prevent overflow warning during compilation. We already set this limit > for the GGTT. > > This is a temporary patch until a full replacement of size_t variables > (inadequate in 32-bit kernel) is in place. > > Regression from: > commit a4e0bedca678c81eea4cd79a4bd502335639f73a > Author: Michel Thierry <michel.thierry@intel.com> > Date: Wed Apr 8 12:13:35 2015 +0100 > > drm/i915: Use complete address space in true PPGTT > > v2: Prettify code and explain why this is needed. (Chris) > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 17b7df0..0653c28 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -951,7 +951,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); > > ppgtt->base.start = 0; > - ppgtt->base.total = 1ULL << 32; > +#ifdef CONFIG_X86_32 FWIW, this is somewhat frowned upon (which is why Chris was using IS_ENABLED(). It's documented in Documentation/CodingStyle, Chapter 20. Within code, where possible, use the IS_ENABLED macro to convert a Kconfig symbol into a C boolean expression, and use it in a normal C conditional: if (IS_ENABLED(CONFIG_SOMETHING)) { ... } The compiler will constant-fold the conditional away, and include or exclude the block of code just as with an #ifdef, so this will not add any runtime overhead. However, this approach still allows the C compiler to see the code inside the block, and check it for correctness (syntax, types, symbol references, etc). Thus, you still have to use an #ifdef if the code inside the block references symbols that will not exist if the condition is not met.
On Thu, May 28, 2015 at 06:09:34PM +0100, Michel Thierry wrote: > And prevent overflow warning during compilation. We already set this limit > for the GGTT. > > This is a temporary patch until a full replacement of size_t variables > (inadequate in 32-bit kernel) is in place. > > Regression from: > commit a4e0bedca678c81eea4cd79a4bd502335639f73a > Author: Michel Thierry <michel.thierry@intel.com> > Date: Wed Apr 8 12:13:35 2015 +0100 > > drm/i915: Use complete address space in true PPGTT > > v2: Prettify code and explain why this is needed. (Chris) > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Michel Thierry <michel.thierry@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 17b7df0..0653c28 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -951,7 +951,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); > > ppgtt->base.start = 0; > - ppgtt->base.total = 1ULL << 32; > +#ifdef CONFIG_X86_32 > + /* While we have a proliferation of size_t variables > + * we cannot represent the full ppgtt size on 32bit, > + * so limit it to the same size as the GGTT (currently > + * 2GiB). > + */ > + ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; > +#else > + ppgtt->base.total = 1ULL << 32; > +#endif You missed the point of having it always compiled. Part of that is that the compiler warning here is important. -Chris
On 5/28/2015 10:14 PM, Chris Wilson wrote: > On Thu, May 28, 2015 at 06:09:34PM +0100, Michel Thierry wrote: >> And prevent overflow warning during compilation. We already set this limit >> for the GGTT. >> >> This is a temporary patch until a full replacement of size_t variables >> (inadequate in 32-bit kernel) is in place. >> >> Regression from: >> commit a4e0bedca678c81eea4cd79a4bd502335639f73a >> Author: Michel Thierry <michel.thierry@intel.com> >> Date: Wed Apr 8 12:13:35 2015 +0100 >> >> drm/i915: Use complete address space in true PPGTT >> >> v2: Prettify code and explain why this is needed. (Chris) >> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Michel Thierry <michel.thierry@intel.com> >> --- >> drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c >> index 17b7df0..0653c28 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c >> @@ -951,7 +951,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) >> gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); >> >> ppgtt->base.start = 0; >> - ppgtt->base.total = 1ULL << 32; >> +#ifdef CONFIG_X86_32 >> + /* While we have a proliferation of size_t variables >> + * we cannot represent the full ppgtt size on 32bit, >> + * so limit it to the same size as the GGTT (currently >> + * 2GiB). >> + */ >> + ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; >> +#else >> + ppgtt->base.total = 1ULL << 32; >> +#endif > You missed the point of having it always compiled. Part of that is that > the compiler warning here is important. > -Chris I was under the impression that Daniel wanted to get rid of the warning too, that's why I didn't use IS_ENABLED().
On Fri, May 29, 2015 at 09:58:33AM +0100, Michel Thierry wrote: > On 5/28/2015 10:14 PM, Chris Wilson wrote: > >On Thu, May 28, 2015 at 06:09:34PM +0100, Michel Thierry wrote: > >>And prevent overflow warning during compilation. We already set this limit > >>for the GGTT. > >> > >>This is a temporary patch until a full replacement of size_t variables > >>(inadequate in 32-bit kernel) is in place. > >> > >>Regression from: > >> commit a4e0bedca678c81eea4cd79a4bd502335639f73a > >> Author: Michel Thierry <michel.thierry@intel.com> > >> Date: Wed Apr 8 12:13:35 2015 +0100 > >> > >> drm/i915: Use complete address space in true PPGTT > >> > >>v2: Prettify code and explain why this is needed. (Chris) > >> > >>Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>Cc: Mika Kuoppala <mika.kuoppala@intel.com> > >>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>Signed-off-by: Michel Thierry <michel.thierry@intel.com> > >>--- > >> drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++- > >> 1 file changed, 10 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>index 17b7df0..0653c28 100644 > >>--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >>+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >>@@ -951,7 +951,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > >> gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); > >> ppgtt->base.start = 0; > >>- ppgtt->base.total = 1ULL << 32; > >>+#ifdef CONFIG_X86_32 > >>+ /* While we have a proliferation of size_t variables > >>+ * we cannot represent the full ppgtt size on 32bit, > >>+ * so limit it to the same size as the GGTT (currently > >>+ * 2GiB). > >>+ */ > >>+ ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; > >>+#else > >>+ ppgtt->base.total = 1ULL << 32; > >>+#endif > >You missed the point of having it always compiled. Part of that is that > >the compiler warning here is important. > >-Chris > I was under the impression that Daniel wanted to get rid of the > warning too, that's why I didn't use IS_ENABLED(). It's bad form to have the #ifdef inside code, and in this case the warning is good - it is a clear indicator that we have a large hole in the code that I don't just want to sweep under the carpet. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 17b7df0..0653c28 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -951,7 +951,16 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); ppgtt->base.start = 0; - ppgtt->base.total = 1ULL << 32; +#ifdef CONFIG_X86_32 + /* While we have a proliferation of size_t variables + * we cannot represent the full ppgtt size on 32bit, + * so limit it to the same size as the GGTT (currently + * 2GiB). + */ + ppgtt->base.total = to_i915(ppgtt->base.dev)->gtt.base.total; +#else + ppgtt->base.total = 1ULL << 32; +#endif ppgtt->base.cleanup = gen8_ppgtt_cleanup; ppgtt->base.allocate_va_range = gen8_alloc_va_range; ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
And prevent overflow warning during compilation. We already set this limit for the GGTT. This is a temporary patch until a full replacement of size_t variables (inadequate in 32-bit kernel) is in place. Regression from: commit a4e0bedca678c81eea4cd79a4bd502335639f73a Author: Michel Thierry <michel.thierry@intel.com> Date: Wed Apr 8 12:13:35 2015 +0100 drm/i915: Use complete address space in true PPGTT v2: Prettify code and explain why this is needed. (Chris) Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Michel Thierry <michel.thierry@intel.com> --- drivers/gpu/drm/i915/i915_gem_gtt.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)