[v2] drm/i915: limit PPGTT size to 2GB in 32-bit platforms

Submitted by Michel Thierry on May 28, 2015, 5:09 p.m.

Details

Message ID 1432832974-5658-1-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry May 28, 2015, 5:09 p.m.
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(-)

Patch hide | download patch | download mbox

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;

Comments

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