Re: drm/i915/gen8: page directories rework allocation

Re: drm/i915/gen8: page directories rework allocation

[Dan Carpenter]

Hi Michel,

The patch 69876bed7e00: "drm/i915/gen8: page directories rework
allocation" from Apr 8, 2015, has the following issue:

	drivers/gpu/drm/i915/i915_gem_gtt.c:760
	warn: too many zeroes

drivers/gpu/drm/i915/i915_gem_gtt.c
   746  static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
   747                                       struct i915_page_directory_pointer *pdp,
   748                                       uint64_t start,
   749                                       uint64_t length,
   750                                       unsigned long *new_pds)
   751  {
   752          struct drm_device *dev = ppgtt->base.dev;
   753          struct i915_page_directory *pd;
   754          uint64_t temp;
   755          uint32_t pdpe;
   756  
   757          WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
   758  
   759          /* FIXME: PPGTT container_of won't work for 64b */
   760          WARN_ON((start + length) > 0x800000000ULL);
                                                     ^
This last zero was not intended.

   761  
   762          gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
   763                  if (pd)
   764                          continue;
   765  
   766                  pd = alloc_pd_single(dev);
   767                  if (IS_ERR(pd))
   768                          goto unwind_out;
   769  
   770                  gen8_initialize_pd(&ppgtt->base, pd);
   771                  pdp->page_directory[pdpe] = pd;
   772                  set_bit(pdpe, new_pds);
   773          }
   774  
   775          return 0;
   776  
   777  unwind_out:
   778          for_each_set_bit(pdpe, new_pds, GEN8_LEGACY_PDPES)
   779                  unmap_and_free_pd(pdp->page_directory[pdpe], dev);
   780  
   781          return -ENOMEM;
   782  }


regards,
dan carpenter
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: drm/i915/gen8: page directories rework allocation

[Dave Gordon]

On 30/04/15 11:47, Dan Carpenter wrote:
> Hi Michel,
> 
> The patch 69876bed7e00: "drm/i915/gen8: page directories rework
> allocation" from Apr 8, 2015, has the following issue:
> 
> 	drivers/gpu/drm/i915/i915_gem_gtt.c:760
> 	warn: too many zeroes
> 
> drivers/gpu/drm/i915/i915_gem_gtt.c
>    746  static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>    747                                       struct i915_page_directory_pointer *pdp,
>    748                                       uint64_t start,
>    749                                       uint64_t length,
>    750                                       unsigned long *new_pds)
>    751  {
>    752          struct drm_device *dev = ppgtt->base.dev;
>    753          struct i915_page_directory *pd;
>    754          uint64_t temp;
>    755          uint32_t pdpe;
>    756  
>    757          WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>    758  
>    759          /* FIXME: PPGTT container_of won't work for 64b */
>    760          WARN_ON((start + length) > 0x800000000ULL);
>                                                      ^
> This last zero was not intended.

Wouldn't (1ULL << 31) be clearer?

And shouldn't it be >= not > ?

Or perhaps,
	/* FIXME: upper bound must not overflow 31 bits */
	WARN_ON((start + length) & (~0ULL << 31));

Pick whichever you think is most comprehensible and least typo-prone!

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Michel Thierry]

The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
page directories rework allocation" added an overflow warning, but the
mask had an extra 0. Use typo-prone option suggested by Dave instead.

This check will be unnecessary after gen8_alloc_va_range handles more
than 4 PDPs (48b addressing).

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..6d894fc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
 
 	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
 
-	/* FIXME: PPGTT container_of won't work for 64b */
-	WARN_ON((start + length) > 0x800000000ULL);
+	/* FIXME: upper bound must not overflow 31 bits  */
+	WARN_ON((start + length) & (~0ULL << 31));
 
 	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
 		if (pd)
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Ville Syrjälä]

On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> page directories rework allocation" added an overflow warning, but the
> mask had an extra 0. Use typo-prone option suggested by Dave instead.
> 
> This check will be unnecessary after gen8_alloc_va_range handles more
> than 4 PDPs (48b addressing).
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6fae6bd..6d894fc 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>  
>  	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>  
> -	/* FIXME: PPGTT container_of won't work for 64b */
> -	WARN_ON((start + length) > 0x800000000ULL);
> +	/* FIXME: upper bound must not overflow 31 bits  */
> +	WARN_ON((start + length) & (~0ULL << 31));

Why is it 31 and not 32?

>  
>  	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>  		if (pd)
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Michel Thierry]

On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>> page directories rework allocation" added an overflow warning, but the
>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>
>> This check will be unnecessary after gen8_alloc_va_range handles more
>> than 4 PDPs (48b addressing).
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 6fae6bd..6d894fc 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>
>>   	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>
>> -	/* FIXME: PPGTT container_of won't work for 64b */
>> -	WARN_ON((start + length) > 0x800000000ULL);
>> +	/* FIXME: upper bound must not overflow 31 bits  */
>> +	WARN_ON((start + length) & (~0ULL << 31));
>
> Why is it 31 and not 32?
>

Right, the check really should be (start + length) >= 0x100000000ULL.

>>
>>   	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>>   		if (pd)
>> --
>> 2.1.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

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Dave Gordon]

On 30/04/15 15:33, Michel Thierry wrote:
> On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
>> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>>> page directories rework allocation" added an overflow warning, but the
>>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>>
>>> This check will be unnecessary after gen8_alloc_va_range handles more
>>> than 4 PDPs (48b addressing).
>>>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> index 6fae6bd..6d894fc 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>> @@ -756,8 +756,8 @@ static int
>>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>>
>>>       WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>>
>>> -    /* FIXME: PPGTT container_of won't work for 64b */
>>> -    WARN_ON((start + length) > 0x800000000ULL);
>>> +    /* FIXME: upper bound must not overflow 31 bits  */
>>> +    WARN_ON((start + length) & (~0ULL << 31));
>>
>> Why is it 31 and not 32?
> 
> Right, the check really should be (start + length) >= 0x100000000ULL.

Something with '32' in it might be more obvious and save anyone having
to count the zeroes ... and a comment that also mentioned the limit:

	/* FIXME: for now, upper bound must fit in 32 bits  */

	WARN_ON((start + length) >= (1ULL << 32))
	WARN_ON((start + length) & (~0ULL << 32))
	WARN_ON((start + length) >> 32) != 0)

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Michel Thierry]

On 4/30/2015 3:53 PM, Dave Gordon wrote:
> On 30/04/15 15:33, Michel Thierry wrote:
>> On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
>>> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
>>>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
>>>> page directories rework allocation" added an overflow warning, but the
>>>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
>>>>
>>>> This check will be unnecessary after gen8_alloc_va_range handles more
>>>> than 4 PDPs (48b addressing).
>>>>
>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> index 6fae6bd..6d894fc 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>>>> @@ -756,8 +756,8 @@ static int
>>>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>>>>
>>>>        WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>>>>
>>>> -    /* FIXME: PPGTT container_of won't work for 64b */
>>>> -    WARN_ON((start + length) > 0x800000000ULL);
>>>> +    /* FIXME: upper bound must not overflow 31 bits  */
>>>> +    WARN_ON((start + length) & (~0ULL << 31));
>>>
>>> Why is it 31 and not 32?
>>
>> Right, the check really should be (start + length) >= 0x100000000ULL.
>
> Something with '32' in it might be more obvious and save anyone having
> to count the zeroes ... and a comment that also mentioned the limit:
>
> 	/* FIXME: for now, upper bound must fit in 32 bits  */
>
> 	WARN_ON((start + length) >= (1ULL << 32))
> 	WARN_ON((start + length) & (~0ULL << 32))
> 	WARN_ON((start + length) >> 32) != 0)

Yes, I was planning to just replace _31_ with _32_ and avoid all those 
zeroes...

>
> .Dave.
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Ville Syrjälä]

On Thu, Apr 30, 2015 at 03:53:29PM +0100, Dave Gordon wrote:
> On 30/04/15 15:33, Michel Thierry wrote:
> > On 4/30/2015 3:22 PM, Ville Syrjälä wrote:
> >> On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> >>> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> >>> page directories rework allocation" added an overflow warning, but the
> >>> mask had an extra 0. Use typo-prone option suggested by Dave instead.
> >>>
> >>> This check will be unnecessary after gen8_alloc_va_range handles more
> >>> than 4 PDPs (48b addressing).
> >>>
> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> >>> Cc: Dave Gordon <david.s.gordon@intel.com>
> >>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> index 6fae6bd..6d894fc 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>> @@ -756,8 +756,8 @@ static int
> >>> gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
> >>>
> >>>       WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
> >>>
> >>> -    /* FIXME: PPGTT container_of won't work for 64b */
> >>> -    WARN_ON((start + length) > 0x800000000ULL);
> >>> +    /* FIXME: upper bound must not overflow 31 bits  */
> >>> +    WARN_ON((start + length) & (~0ULL << 31));
> >>
> >> Why is it 31 and not 32?
> > 
> > Right, the check really should be (start + length) >= 0x100000000ULL.
> 
> Something with '32' in it might be more obvious and save anyone having
> to count the zeroes ... and a comment that also mentioned the limit:
> 
> 	/* FIXME: for now, upper bound must fit in 32 bits  */
> 
> 	WARN_ON((start + length) >= (1ULL << 32))

This would match the '1ULL<<32' used in the ppgtt init.

> 	WARN_ON((start + length) & (~0ULL << 32))
> 	WARN_ON((start + length) >> 32) != 0)
> 
> .Dave.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Michel Thierry]

The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
page directories rework allocation" added an overflow warning, but the
mask had an extra 0. Use typo-prone option suggested by Dave instead,
to check for (start + length) >= 0x100000000ULL.

This check will be unnecessary after gen8_alloc_va_range handles more
than 4 PDPs (48b addressing).

v2: Really check for 32b overflow (Ville)

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 6fae6bd..3450cb5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
 
 	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
 
-	/* FIXME: PPGTT container_of won't work for 64b */
-	WARN_ON((start + length) > 0x800000000ULL);
+	/* FIXME: upper bound must not overflow 32 bits  */
+	WARN_ON((start + length) >= (1ULL << 32));
 
 	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
 		if (pd)
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6299
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(18)      TIMEOUT(1)PASS(1)
*BYT  igt@gem_exec_parse@bitmasks      FAIL(1)PASS(7)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(10)PASS(9)      TIMEOUT(2)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6300
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  264/264              264/264
BYT                 -3              227/227              224/227
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_dummy_reloc_loop@render      FAIL(1)PASS(18)      TIMEOUT(1)PASS(1)
 BYT  igt@gem_pipe_control_store_loop@fresh-buffer      FAIL(1)TIMEOUT(10)PASS(9)      TIMEOUT(1)PASS(1)
*BYT  igt@gem_threaded_access_tiled      FAIL(1)PASS(6)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[David Weinehall]

On Thu, Apr 30, 2015 at 02:59:34PM +0100, Michel Thierry wrote:
> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> page directories rework allocation" added an overflow warning, but the
> mask had an extra 0. Use typo-prone option suggested by Dave instead.

I think you mean "less typo-prone" here :)


Kind regards, David Weinehall
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix 32b overflow check in gen8_ppgtt_alloc_page_directories

[Daniel Vetter]

On Thu, Apr 30, 2015 at 04:06:51PM +0100, Michel Thierry wrote:
> The patch 69876bed7e008f5fe01538a2d47c09f2862129d0: "drm/i915/gen8:
> page directories rework allocation" added an overflow warning, but the
> mask had an extra 0. Use typo-prone option suggested by Dave instead,

I inserted the "less" above while applying.

> to check for (start + length) >= 0x100000000ULL.
> 
> This check will be unnecessary after gen8_alloc_va_range handles more
> than 4 PDPs (48b addressing).
> 
> v2: Really check for 32b overflow (Ville)
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6fae6bd..3450cb5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -756,8 +756,8 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>  
>  	WARN_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
>  
> -	/* FIXME: PPGTT container_of won't work for 64b */
> -	WARN_ON((start + length) > 0x800000000ULL);
> +	/* FIXME: upper bound must not overflow 32 bits  */
> +	WARN_ON((start + length) >= (1ULL << 32));
>  
>  	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>  		if (pd)
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx