[PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[jglisse]

From: Jerome Glisse <jglisse@redhat.com>

Forbid allocating buffer bigger than visible VRAM or GTT, also
properly set lpfn field.

v2 - use max macro
   - silence warning

Signed-off-by: Jerome Glisse <jglisse@redhat.com>
---
 drivers/gpu/drm/radeon/radeon_object.c |   34 ++++++++++++++++++++++++++-----
 1 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index 1d06774..c2fa64c 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
 	u32 c = 0;
 
 	rbo->placement.fpfn = 0;
-	rbo->placement.lpfn = rbo->rdev->mc.active_vram_size >> PAGE_SHIFT;
+	rbo->placement.lpfn = 0;
 	rbo->placement.placement = rbo->placements;
 	rbo->placement.busy_placement = rbo->placements;
-	if (domain & RADEON_GEM_DOMAIN_VRAM)
+	if (domain & RADEON_GEM_DOMAIN_VRAM) {
+		rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.active_vram_size >> PAGE_SHIFT);
 		rbo->placements[c++] = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
 					TTM_PL_FLAG_VRAM;
-	if (domain & RADEON_GEM_DOMAIN_GTT)
+	}
+	if (domain & RADEON_GEM_DOMAIN_GTT) {
+		rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.gtt_size >> PAGE_SHIFT);
 		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
-	if (domain & RADEON_GEM_DOMAIN_CPU)
+	}
+	if (domain & RADEON_GEM_DOMAIN_CPU) {
+		/* 4G limit for CPU domain */
+		rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
 		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
-	if (!c)
+	}
+	if (!c) {
+		/* 4G limit for CPU domain */
+		rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
 		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
+	}
 	rbo->placement.num_placement = c;
 	rbo->placement.num_busy_placement = c;
 }
@@ -91,7 +101,8 @@ int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj,
 {
 	struct radeon_bo *bo;
 	enum ttm_bo_type type;
-	int page_align = roundup(byte_align, PAGE_SIZE) >> PAGE_SHIFT;
+	unsigned long page_align = roundup(byte_align, PAGE_SIZE) >> PAGE_SHIFT;
+	unsigned long max_size = 0;
 	int r;
 
 	if (unlikely(rdev->mman.bdev.dev_mapping == NULL)) {
@@ -104,6 +115,17 @@ int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj,
 	}
 	*bo_ptr = NULL;
 
+	/* maximun bo size is the minimun btw visible vram and gtt size */
+	max_size = rdev->mc.visible_vram_size;
+	if (max_size > rdev->mc.gtt_size) {
+		max_size = rdev->mc.gtt_size;
+	}
+	if ((page_align << PAGE_SHIFT) >= max_size) {
+		printk(KERN_WARNING "%s:%d alloc size %ldM bigger than %ldMb limit\n",
+			__func__, __LINE__, page_align  >> (20 - PAGE_SHIFT), max_size >> 20);
+		return -ENOMEM;
+	}
+
 retry:
 	bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL);
 	if (bo == NULL)
-- 
1.7.3.2

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Michel Dänzer]

On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote: 
> From: Jerome Glisse <jglisse@redhat.com>
> 
> Forbid allocating buffer bigger than visible VRAM or GTT, also
> properly set lpfn field.
> 
> v2 - use max macro
>    - silence warning
> 
> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> ---
>  drivers/gpu/drm/radeon/radeon_object.c |   34 ++++++++++++++++++++++++++-----
>  1 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
> index 1d06774..c2fa64c 100644
> --- a/drivers/gpu/drm/radeon/radeon_object.c
> +++ b/drivers/gpu/drm/radeon/radeon_object.c
> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
>  	u32 c = 0;
>  
>  	rbo->placement.fpfn = 0;
> -	rbo->placement.lpfn = rbo->rdev->mc.active_vram_size >> PAGE_SHIFT;
> +	rbo->placement.lpfn = 0;
>  	rbo->placement.placement = rbo->placements;
>  	rbo->placement.busy_placement = rbo->placements;
> -	if (domain & RADEON_GEM_DOMAIN_VRAM)
> +	if (domain & RADEON_GEM_DOMAIN_VRAM) {
> +		rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.active_vram_size >> PAGE_SHIFT);
>  		rbo->placements[c++] = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
>  					TTM_PL_FLAG_VRAM;
> -	if (domain & RADEON_GEM_DOMAIN_GTT)
> +	}
> +	if (domain & RADEON_GEM_DOMAIN_GTT) {
> +		rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.gtt_size >> PAGE_SHIFT);
>  		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
> -	if (domain & RADEON_GEM_DOMAIN_CPU)
> +	}
> +	if (domain & RADEON_GEM_DOMAIN_CPU) {
> +		/* 4G limit for CPU domain */
> +		rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>  		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
> -	if (!c)
> +	}
> +	if (!c) {
> +		/* 4G limit for CPU domain */
> +		rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>  		rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
> +	}

I don't think taxing the maximum is the right thing to do: If domain is
(RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
to be the same size as GTT, lpfn will end up larger than one of them.

AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
(i.e. unrestricted), the callers that need it to be non-0 already set it
afterwards.

Out of curiosity, where does the 4G limit come from?


> @@ -104,6 +115,17 @@ int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj,
>  	}
>  	*bo_ptr = NULL;
>  
> +	/* maximun bo size is the minimun btw visible vram and gtt size */
> +	max_size = rdev->mc.visible_vram_size;
> +	if (max_size > rdev->mc.gtt_size) {
> +		max_size = rdev->mc.gtt_size;
> +	}

max_size = min(rdev->mc.visible_vram_size, rdev->mc.gtt_size);

Except I think this should actually be max(...).


> +	if ((page_align << PAGE_SHIFT) >= max_size) {
> +		printk(KERN_WARNING "%s:%d alloc size %ldM bigger than %ldMb limit\n",
> +			__func__, __LINE__, page_align  >> (20 - PAGE_SHIFT), max_size >> 20);
> +		return -ENOMEM;
> +	}

Maybe this should be rate-limited, or userspace can spam dmesg.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Jerome Glisse]

2010/11/22 Michel Dänzer <michel@daenzer.net>:
> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>> From: Jerome Glisse <jglisse@redhat.com>
>>
>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>> properly set lpfn field.
>>
>> v2 - use max macro
>>    - silence warning
>>
>> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
>> ---
>>  drivers/gpu/drm/radeon/radeon_object.c |   34 ++++++++++++++++++++++++++-----
>>  1 files changed, 28 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
>> index 1d06774..c2fa64c 100644
>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
>>       u32 c = 0;
>>
>>       rbo->placement.fpfn = 0;
>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size >> PAGE_SHIFT;
>> +     rbo->placement.lpfn = 0;
>>       rbo->placement.placement = rbo->placements;
>>       rbo->placement.busy_placement = rbo->placements;
>> -     if (domain & RADEON_GEM_DOMAIN_VRAM)
>> +     if (domain & RADEON_GEM_DOMAIN_VRAM) {
>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.active_vram_size >> PAGE_SHIFT);
>>               rbo->placements[c++] = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
>>                                       TTM_PL_FLAG_VRAM;
>> -     if (domain & RADEON_GEM_DOMAIN_GTT)
>> +     }
>> +     if (domain & RADEON_GEM_DOMAIN_GTT) {
>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.gtt_size >> PAGE_SHIFT);
>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
>> -     if (domain & RADEON_GEM_DOMAIN_CPU)
>> +     }
>> +     if (domain & RADEON_GEM_DOMAIN_CPU) {
>> +             /* 4G limit for CPU domain */
>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>> -     if (!c)
>> +     }
>> +     if (!c) {
>> +             /* 4G limit for CPU domain */
>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>> +     }
>
> I don't think taxing the maximum is the right thing to do: If domain is
> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
> to be the same size as GTT, lpfn will end up larger than one of them.
>
> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
> (i.e. unrestricted), the callers that need it to be non-0 already set it
> afterwards.
>
> Out of curiosity, where does the 4G limit come from?
>
>
>From my hat, but iirc ttm limit things to 1G anyway (vm size for
mapping object in drm file and iirc we will report error if we can't
find a mapping for userspace object). I guess at one point we should
start thinking about what we want to do on that front.

Doing a v3.

Cheers,
Jerome

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Alex Deucher]

2010/11/22 Jerome Glisse <j.glisse@gmail.com>:
> 2010/11/22 Michel Dänzer <michel@daenzer.net>:
>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>> From: Jerome Glisse <jglisse@redhat.com>
>>>
>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>> properly set lpfn field.
>>>
>>> v2 - use max macro
>>>    - silence warning
>>>
>>> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
>>> ---
>>>  drivers/gpu/drm/radeon/radeon_object.c |   34 ++++++++++++++++++++++++++-----
>>>  1 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
>>> index 1d06774..c2fa64c 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
>>>       u32 c = 0;
>>>
>>>       rbo->placement.fpfn = 0;
>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size >> PAGE_SHIFT;
>>> +     rbo->placement.lpfn = 0;
>>>       rbo->placement.placement = rbo->placements;
>>>       rbo->placement.busy_placement = rbo->placements;
>>> -     if (domain & RADEON_GEM_DOMAIN_VRAM)
>>> +     if (domain & RADEON_GEM_DOMAIN_VRAM) {
>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.active_vram_size >> PAGE_SHIFT);
>>>               rbo->placements[c++] = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
>>>                                       TTM_PL_FLAG_VRAM;
>>> -     if (domain & RADEON_GEM_DOMAIN_GTT)
>>> +     }
>>> +     if (domain & RADEON_GEM_DOMAIN_GTT) {
>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.gtt_size >> PAGE_SHIFT);
>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
>>> -     if (domain & RADEON_GEM_DOMAIN_CPU)
>>> +     }
>>> +     if (domain & RADEON_GEM_DOMAIN_CPU) {
>>> +             /* 4G limit for CPU domain */
>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>>> -     if (!c)
>>> +     }
>>> +     if (!c) {
>>> +             /* 4G limit for CPU domain */
>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF >> PAGE_SHIFT);
>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>>> +     }
>>
>> I don't think taxing the maximum is the right thing to do: If domain is
>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
>> to be the same size as GTT, lpfn will end up larger than one of them.
>>
>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>> (i.e. unrestricted), the callers that need it to be non-0 already set it
>> afterwards.
>>
>> Out of curiosity, where does the 4G limit come from?
>>
>>
> From my hat, but iirc ttm limit things to 1G anyway (vm size for
> mapping object in drm file and iirc we will report error if we can't
> find a mapping for userspace object). I guess at one point we should
> start thinking about what we want to do on that front.
>

FWIW, r1xx-r5xx have a 32 bit MC address space while r6xx+ have a 40
bit address space (with certain limitations on 6xx/7xx).

Alex

> Doing a v3.
>
> Cheers,
> Jerome
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Thomas Hellstrom]

On 11/22/2010 06:07 PM, Jerome Glisse wrote:
> 2010/11/22 Michel Dänzer<michel@daenzer.net>:
>    
>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>      
>>> From: Jerome Glisse<jglisse@redhat.com>
>>>
>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>> properly set lpfn field.
>>>
>>> v2 - use max macro
>>>     - silence warning
>>>
>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>> ---
>>>   drivers/gpu/drm/radeon/radeon_object.c |   34 ++++++++++++++++++++++++++-----
>>>   1 files changed, 28 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
>>> index 1d06774..c2fa64c 100644
>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
>>>        u32 c = 0;
>>>
>>>        rbo->placement.fpfn = 0;
>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT;
>>> +     rbo->placement.lpfn = 0;
>>>        rbo->placement.placement = rbo->placements;
>>>        rbo->placement.busy_placement = rbo->placements;
>>> -     if (domain&  RADEON_GEM_DOMAIN_VRAM)
>>> +     if (domain&  RADEON_GEM_DOMAIN_VRAM) {
>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT);
>>>                rbo->placements[c++] = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
>>>                                        TTM_PL_FLAG_VRAM;
>>> -     if (domain&  RADEON_GEM_DOMAIN_GTT)
>>> +     }
>>> +     if (domain&  RADEON_GEM_DOMAIN_GTT) {
>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn, (unsigned)rbo->rdev->mc.gtt_size>>  PAGE_SHIFT);
>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT;
>>> -     if (domain&  RADEON_GEM_DOMAIN_CPU)
>>> +     }
>>> +     if (domain&  RADEON_GEM_DOMAIN_CPU) {
>>> +             /* 4G limit for CPU domain */
>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF>>  PAGE_SHIFT);
>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>>> -     if (!c)
>>> +     }
>>> +     if (!c) {
>>> +             /* 4G limit for CPU domain */
>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 0xFFFFFFFF>>  PAGE_SHIFT);
>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM;
>>> +     }
>>>        
>> I don't think taxing the maximum is the right thing to do: If domain is
>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
>> to be the same size as GTT, lpfn will end up larger than one of them.
>>
>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>> (i.e. unrestricted), the callers that need it to be non-0 already set it
>> afterwards.
>>
>> Out of curiosity, where does the 4G limit come from?
>>
>>
>>      
> > From my hat, but iirc ttm limit things to 1G anyway (vm size for
> mapping object in drm file and iirc we will report error if we can't
> find a mapping for userspace object). I guess at one point we should
> start thinking about what we want to do on that front.
>    

That limit is taken from *my* hat. Nothing prevents us to increase that 
limit to whatever.

/Thomas



> Doing a v3.
>
> Cheers,
> Jerome
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>    

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Jerome Glisse]

On Mon, Nov 22, 2010 at 12:31 PM, Thomas Hellstrom <thomas@shipmail.org> wrote:
> On 11/22/2010 06:07 PM, Jerome Glisse wrote:
>>
>> 2010/11/22 Michel Dänzer<michel@daenzer.net>:
>>
>>>
>>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>>
>>>>
>>>> From: Jerome Glisse<jglisse@redhat.com>
>>>>
>>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>>> properly set lpfn field.
>>>>
>>>> v2 - use max macro
>>>>    - silence warning
>>>>
>>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>>> ---
>>>>  drivers/gpu/drm/radeon/radeon_object.c |   34
>>>> ++++++++++++++++++++++++++-----
>>>>  1 files changed, 28 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>>>> b/drivers/gpu/drm/radeon/radeon_object.c
>>>> index 1d06774..c2fa64c 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct
>>>> radeon_bo *rbo, u32 domain)
>>>>       u32 c = 0;
>>>>
>>>>       rbo->placement.fpfn = 0;
>>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size>>
>>>>  PAGE_SHIFT;
>>>> +     rbo->placement.lpfn = 0;
>>>>       rbo->placement.placement = rbo->placements;
>>>>       rbo->placement.busy_placement = rbo->placements;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_VRAM)
>>>> +     if (domain&  RADEON_GEM_DOMAIN_VRAM) {
>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>> (unsigned)rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT);
>>>>               rbo->placements[c++] = TTM_PL_FLAG_WC |
>>>> TTM_PL_FLAG_UNCACHED |
>>>>                                       TTM_PL_FLAG_VRAM;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_GTT)
>>>> +     }
>>>> +     if (domain&  RADEON_GEM_DOMAIN_GTT) {
>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>> (unsigned)rbo->rdev->mc.gtt_size>>  PAGE_SHIFT);
>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>> TTM_PL_FLAG_TT;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_CPU)
>>>> +     }
>>>> +     if (domain&  RADEON_GEM_DOMAIN_CPU) {
>>>> +             /* 4G limit for CPU domain */
>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>> TTM_PL_FLAG_SYSTEM;
>>>> -     if (!c)
>>>> +     }
>>>> +     if (!c) {
>>>> +             /* 4G limit for CPU domain */
>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>> TTM_PL_FLAG_SYSTEM;
>>>> +     }
>>>>
>>>
>>> I don't think taxing the maximum is the right thing to do: If domain is
>>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
>>> to be the same size as GTT, lpfn will end up larger than one of them.
>>>
>>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>>> (i.e. unrestricted), the callers that need it to be non-0 already set it
>>> afterwards.
>>>
>>> Out of curiosity, where does the 4G limit come from?
>>>
>>>
>>>
>>
>> > From my hat, but iirc ttm limit things to 1G anyway (vm size for
>> mapping object in drm file and iirc we will report error if we can't
>> find a mapping for userspace object). I guess at one point we should
>> start thinking about what we want to do on that front.
>>
>
> That limit is taken from *my* hat. Nothing prevents us to increase that
> limit to whatever.
>
> /Thomas
>

I think we can at most have 3G as the lower space is use by old drm stuff iirc.

Cheers,
Jerome

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Alex Deucher]

On Mon, Nov 22, 2010 at 12:35 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Mon, Nov 22, 2010 at 12:31 PM, Thomas Hellstrom <thomas@shipmail.org> wrote:
>> On 11/22/2010 06:07 PM, Jerome Glisse wrote:
>>>
>>> 2010/11/22 Michel Dänzer<michel@daenzer.net>:
>>>
>>>>
>>>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>>>
>>>>>
>>>>> From: Jerome Glisse<jglisse@redhat.com>
>>>>>
>>>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>>>> properly set lpfn field.
>>>>>
>>>>> v2 - use max macro
>>>>>    - silence warning
>>>>>
>>>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>>>> ---
>>>>>  drivers/gpu/drm/radeon/radeon_object.c |   34
>>>>> ++++++++++++++++++++++++++-----
>>>>>  1 files changed, 28 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>>>>> b/drivers/gpu/drm/radeon/radeon_object.c
>>>>> index 1d06774..c2fa64c 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct
>>>>> radeon_bo *rbo, u32 domain)
>>>>>       u32 c = 0;
>>>>>
>>>>>       rbo->placement.fpfn = 0;
>>>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size>>
>>>>>  PAGE_SHIFT;
>>>>> +     rbo->placement.lpfn = 0;
>>>>>       rbo->placement.placement = rbo->placements;
>>>>>       rbo->placement.busy_placement = rbo->placements;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_VRAM)
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_VRAM) {
>>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>>> (unsigned)rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_FLAG_WC |
>>>>> TTM_PL_FLAG_UNCACHED |
>>>>>                                       TTM_PL_FLAG_VRAM;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_GTT)
>>>>> +     }
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_GTT) {
>>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>>> (unsigned)rbo->rdev->mc.gtt_size>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_TT;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_CPU)
>>>>> +     }
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_CPU) {
>>>>> +             /* 4G limit for CPU domain */
>>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_SYSTEM;
>>>>> -     if (!c)
>>>>> +     }
>>>>> +     if (!c) {
>>>>> +             /* 4G limit for CPU domain */
>>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_SYSTEM;
>>>>> +     }
>>>>>
>>>>
>>>> I don't think taxing the maximum is the right thing to do: If domain is
>>>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
>>>> to be the same size as GTT, lpfn will end up larger than one of them.
>>>>
>>>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>>>> (i.e. unrestricted), the callers that need it to be non-0 already set it
>>>> afterwards.
>>>>
>>>> Out of curiosity, where does the 4G limit come from?
>>>>
>>>>
>>>>
>>>
>>> > From my hat, but iirc ttm limit things to 1G anyway (vm size for
>>> mapping object in drm file and iirc we will report error if we can't
>>> find a mapping for userspace object). I guess at one point we should
>>> start thinking about what we want to do on that front.
>>>
>>
>> That limit is taken from *my* hat. Nothing prevents us to increase that
>> limit to whatever.
>>
>> /Thomas
>>
>
> I think we can at most have 3G as the lower space is use by old drm stuff iirc.

We already sell cards with 4 GB of vram.

Alex

>
> Cheers,
> Jerome
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Thomas Hellstrom]

On 11/22/2010 06:31 PM, Thomas Hellstrom wrote:
> On 11/22/2010 06:07 PM, Jerome Glisse wrote:
>> 2010/11/22 Michel Dänzer<michel@daenzer.net>:
>>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>>> From: Jerome Glisse<jglisse@redhat.com>
>>>>
>>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>>> properly set lpfn field.
>>>>
>>>> v2 - use max macro
>>>>     - silence warning
>>>>
>>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>>> ---
>>>>   drivers/gpu/drm/radeon/radeon_object.c |   34 
>>>> ++++++++++++++++++++++++++-----
>>>>   1 files changed, 28 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c 
>>>> b/drivers/gpu/drm/radeon/radeon_object.c
>>>> index 1d06774..c2fa64c 100644
>>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct 
>>>> radeon_bo *rbo, u32 domain)
>>>>        u32 c = 0;
>>>>
>>>>        rbo->placement.fpfn = 0;
>>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size>>  
>>>> PAGE_SHIFT;
>>>> +     rbo->placement.lpfn = 0;
>>>>        rbo->placement.placement = rbo->placements;
>>>>        rbo->placement.busy_placement = rbo->placements;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_VRAM)
>>>> +     if (domain&  RADEON_GEM_DOMAIN_VRAM) {
>>>> +             rbo->placement.lpfn = 
>>>> max((unsigned)rbo->placement.lpfn, 
>>>> (unsigned)rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT);
>>>>                rbo->placements[c++] = TTM_PL_FLAG_WC | 
>>>> TTM_PL_FLAG_UNCACHED |
>>>>                                        TTM_PL_FLAG_VRAM;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_GTT)
>>>> +     }
>>>> +     if (domain&  RADEON_GEM_DOMAIN_GTT) {
>>>> +             rbo->placement.lpfn = 
>>>> max((unsigned)rbo->placement.lpfn, 
>>>> (unsigned)rbo->rdev->mc.gtt_size>>  PAGE_SHIFT);
>>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | 
>>>> TTM_PL_FLAG_TT;
>>>> -     if (domain&  RADEON_GEM_DOMAIN_CPU)
>>>> +     }
>>>> +     if (domain&  RADEON_GEM_DOMAIN_CPU) {
>>>> +             /* 4G limit for CPU domain */
>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 
>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | 
>>>> TTM_PL_FLAG_SYSTEM;
>>>> -     if (!c)
>>>> +     }
>>>> +     if (!c) {
>>>> +             /* 4G limit for CPU domain */
>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn, 
>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>                rbo->placements[c++] = TTM_PL_MASK_CACHING | 
>>>> TTM_PL_FLAG_SYSTEM;
>>>> +     }
>>> I don't think taxing the maximum is the right thing to do: If domain is
>>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't 
>>> happen
>>> to be the same size as GTT, lpfn will end up larger than one of them.
>>>
>>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>>> (i.e. unrestricted), the callers that need it to be non-0 already 
>>> set it
>>> afterwards.
>>>
>>> Out of curiosity, where does the 4G limit come from?
>>>
>>>
>> > From my hat, but iirc ttm limit things to 1G anyway (vm size for
>> mapping object in drm file and iirc we will report error if we can't
>> find a mapping for userspace object). I guess at one point we should
>> start thinking about what we want to do on that front.
>
> That limit is taken from *my* hat. Nothing prevents us to increase 
> that limit to whatever.
>
> /Thomas
>
>

Hmm.
Did you guys ever run into trouble with this? Looking at the code, it 
seems the current limit is not 1GB but 1TB? (0x10000000 << PAGE_SHIFT)

/Thomas



>
>> Doing a v3.
>>
>> Cheers,
>> Jerome
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/radeon/kms: forbid big bo allocation (fdo 31708) v2

[Jerome Glisse]

On Mon, Nov 29, 2010 at 5:13 AM, Thomas Hellstrom <thomas@shipmail.org> wrote:
> On 11/22/2010 06:31 PM, Thomas Hellstrom wrote:
>>
>> On 11/22/2010 06:07 PM, Jerome Glisse wrote:
>>>
>>> 2010/11/22 Michel Dänzer<michel@daenzer.net>:
>>>>
>>>> On Fre, 2010-11-19 at 16:34 -0500, jglisse@redhat.com wrote:
>>>>>
>>>>> From: Jerome Glisse<jglisse@redhat.com>
>>>>>
>>>>> Forbid allocating buffer bigger than visible VRAM or GTT, also
>>>>> properly set lpfn field.
>>>>>
>>>>> v2 - use max macro
>>>>>    - silence warning
>>>>>
>>>>> Signed-off-by: Jerome Glisse<jglisse@redhat.com>
>>>>> ---
>>>>>  drivers/gpu/drm/radeon/radeon_object.c |   34
>>>>> ++++++++++++++++++++++++++-----
>>>>>  1 files changed, 28 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/radeon/radeon_object.c
>>>>> b/drivers/gpu/drm/radeon/radeon_object.c
>>>>> index 1d06774..c2fa64c 100644
>>>>> --- a/drivers/gpu/drm/radeon/radeon_object.c
>>>>> +++ b/drivers/gpu/drm/radeon/radeon_object.c
>>>>> @@ -69,18 +69,28 @@ void radeon_ttm_placement_from_domain(struct
>>>>> radeon_bo *rbo, u32 domain)
>>>>>       u32 c = 0;
>>>>>
>>>>>       rbo->placement.fpfn = 0;
>>>>> -     rbo->placement.lpfn = rbo->rdev->mc.active_vram_size>>
>>>>>  PAGE_SHIFT;
>>>>> +     rbo->placement.lpfn = 0;
>>>>>       rbo->placement.placement = rbo->placements;
>>>>>       rbo->placement.busy_placement = rbo->placements;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_VRAM)
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_VRAM) {
>>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>>> (unsigned)rbo->rdev->mc.active_vram_size>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_FLAG_WC |
>>>>> TTM_PL_FLAG_UNCACHED |
>>>>>                                       TTM_PL_FLAG_VRAM;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_GTT)
>>>>> +     }
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_GTT) {
>>>>> +             rbo->placement.lpfn = max((unsigned)rbo->placement.lpfn,
>>>>> (unsigned)rbo->rdev->mc.gtt_size>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_TT;
>>>>> -     if (domain&  RADEON_GEM_DOMAIN_CPU)
>>>>> +     }
>>>>> +     if (domain&  RADEON_GEM_DOMAIN_CPU) {
>>>>> +             /* 4G limit for CPU domain */
>>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_SYSTEM;
>>>>> -     if (!c)
>>>>> +     }
>>>>> +     if (!c) {
>>>>> +             /* 4G limit for CPU domain */
>>>>> +             rbo->placement.lpfn = max(rbo->placement.lpfn,
>>>>> 0xFFFFFFFF>>  PAGE_SHIFT);
>>>>>               rbo->placements[c++] = TTM_PL_MASK_CACHING |
>>>>> TTM_PL_FLAG_SYSTEM;
>>>>> +     }
>>>>
>>>> I don't think taxing the maximum is the right thing to do: If domain is
>>>> (RADEON_GEM_DOMAIN_VRAM | RADEON_GEM_DOMAIN_GTT) and VRAM doesn't happen
>>>> to be the same size as GTT, lpfn will end up larger than one of them.
>>>>
>>>> AFAICT radeon_ttm_placement_from_domain() should just set lpfn to 0
>>>> (i.e. unrestricted), the callers that need it to be non-0 already set it
>>>> afterwards.
>>>>
>>>> Out of curiosity, where does the 4G limit come from?
>>>>
>>>>
>>> > From my hat, but iirc ttm limit things to 1G anyway (vm size for
>>> mapping object in drm file and iirc we will report error if we can't
>>> find a mapping for userspace object). I guess at one point we should
>>> start thinking about what we want to do on that front.
>>
>> That limit is taken from *my* hat. Nothing prevents us to increase that
>> limit to whatever.
>>
>> /Thomas
>>
>>
>
> Hmm.
> Did you guys ever run into trouble with this? Looking at the code, it seems
> the current limit is not 1GB but 1TB? (0x10000000 << PAGE_SHIFT)
>
> /Thomas
>

No never run into it, i also wrongly parsed the limit or my memory of
it wasn't good.

Cheers,
Jerome