[4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

Submitted by Christian König on Sept. 12, 2017, 9:08 a.m.

Details

Message ID 1505207316-4623-4-git-send-email-deathsimple@vodafone.de
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Christian König Sept. 12, 2017, 9:08 a.m.
From: Christian König <christian.koenig@amd.com>

Nobody is using the min/max interface any more.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++-------------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
 2 files changed, 6 insertions(+), 36 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 726a662..8a8add3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -629,20 +629,15 @@  void amdgpu_bo_unref(struct amdgpu_bo **bo)
 		*bo = NULL;
 }
 
-int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
-			     u64 min_offset, u64 max_offset,
-			     u64 *gpu_addr)
+int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
+	unsigned lpfn;
 	int r, i;
-	unsigned fpfn, lpfn;
 
 	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
 		return -EPERM;
 
-	if (WARN_ON_ONCE(min_offset > max_offset))
-		return -EINVAL;
-
 	/* A shared bo cannot be migrated to VRAM */
 	if (bo->prime_shared_count && (domain == AMDGPU_GEM_DOMAIN_VRAM))
 		return -EINVAL;
@@ -657,12 +652,6 @@  int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 		if (gpu_addr)
 			*gpu_addr = amdgpu_bo_gpu_offset(bo);
 
-		if (max_offset != 0) {
-			u64 domain_start = bo->tbo.bdev->man[mem_type].gpu_offset;
-			WARN_ON_ONCE(max_offset <
-				     (amdgpu_bo_gpu_offset(bo) - domain_start));
-		}
-
 		return 0;
 	}
 
@@ -671,23 +660,12 @@  int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 	for (i = 0; i < bo->placement.num_placement; i++) {
 		/* force to pin into visible video ram */
 		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
-		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS) &&
-		    (!max_offset || max_offset >
-		     adev->mc.visible_vram_size)) {
-			if (WARN_ON_ONCE(min_offset >
-					 adev->mc.visible_vram_size))
-				return -EINVAL;
-			fpfn = min_offset >> PAGE_SHIFT;
+		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) {
 			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
-		} else {
-			fpfn = min_offset >> PAGE_SHIFT;
-			lpfn = max_offset >> PAGE_SHIFT;
+			if (!bo->placements[i].lpfn ||
+			    (lpfn && lpfn < bo->placements[i].lpfn))
+				bo->placements[i].lpfn = lpfn;
 		}
-		if (fpfn > bo->placements[i].fpfn)
-			bo->placements[i].fpfn = fpfn;
-		if (!bo->placements[i].lpfn ||
-		    (lpfn && lpfn < bo->placements[i].lpfn))
-			bo->placements[i].lpfn = lpfn;
 		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
 	}
 
@@ -718,11 +696,6 @@  int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
 	return r;
 }
 
-int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
-{
-	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
-}
-
 int amdgpu_bo_unpin(struct amdgpu_bo *bo)
 {
 	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 39b6bf6..4b2c042 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -211,9 +211,6 @@  void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
 struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
 void amdgpu_bo_unref(struct amdgpu_bo **bo);
 int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
-int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
-			     u64 min_offset, u64 max_offset,
-			     u64 *gpu_addr);
 int amdgpu_bo_unpin(struct amdgpu_bo *bo);
 int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
 int amdgpu_bo_init(struct amdgpu_device *adev);

Comments

> -----Original Message-----

> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Christian König

> Sent: Tuesday, September 12, 2017 5:09 AM

> To: amd-gfx@lists.freedesktop.org

> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted

> 

> From: Christian König <christian.koenig@amd.com>

> 

> Nobody is using the min/max interface any more.

> 

> Signed-off-by: Christian König <christian.koenig@amd.com>


I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.

Alex

> ---

>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------

> -------

>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---

>  2 files changed, 6 insertions(+), 36 deletions(-)

> 

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

> index 726a662..8a8add3 100644

> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)

>  		*bo = NULL;

>  }

> 

> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,

> -			     u64 min_offset, u64 max_offset,

> -			     u64 *gpu_addr)

> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)

>  {

>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);

> +	unsigned lpfn;

>  	int r, i;

> -	unsigned fpfn, lpfn;

> 

>  	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))

>  		return -EPERM;

> 

> -	if (WARN_ON_ONCE(min_offset > max_offset))

> -		return -EINVAL;

> -

>  	/* A shared bo cannot be migrated to VRAM */

>  	if (bo->prime_shared_count && (domain ==

> AMDGPU_GEM_DOMAIN_VRAM))

>  		return -EINVAL;

> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo

> *bo, u32 domain,

>  		if (gpu_addr)

>  			*gpu_addr = amdgpu_bo_gpu_offset(bo);

> 

> -		if (max_offset != 0) {

> -			u64 domain_start = bo->tbo.bdev-

> >man[mem_type].gpu_offset;

> -			WARN_ON_ONCE(max_offset <

> -				     (amdgpu_bo_gpu_offset(bo) -

> domain_start));

> -		}

> -

>  		return 0;

>  	}

> 

> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo

> *bo, u32 domain,

>  	for (i = 0; i < bo->placement.num_placement; i++) {

>  		/* force to pin into visible video ram */

>  		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&

> -		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)

> &&

> -		    (!max_offset || max_offset >

> -		     adev->mc.visible_vram_size)) {

> -			if (WARN_ON_ONCE(min_offset >

> -					 adev->mc.visible_vram_size))

> -				return -EINVAL;

> -			fpfn = min_offset >> PAGE_SHIFT;

> +		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))

> {

>  			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;

> -		} else {

> -			fpfn = min_offset >> PAGE_SHIFT;

> -			lpfn = max_offset >> PAGE_SHIFT;

> +			if (!bo->placements[i].lpfn ||

> +			    (lpfn && lpfn < bo->placements[i].lpfn))

> +				bo->placements[i].lpfn = lpfn;

>  		}

> -		if (fpfn > bo->placements[i].fpfn)

> -			bo->placements[i].fpfn = fpfn;

> -		if (!bo->placements[i].lpfn ||

> -		    (lpfn && lpfn < bo->placements[i].lpfn))

> -			bo->placements[i].lpfn = lpfn;

>  		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;

>  	}

> 

> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo

> *bo, u32 domain,

>  	return r;

>  }

> 

> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)

> -{

> -	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);

> -}

> -

>  int amdgpu_bo_unpin(struct amdgpu_bo *bo)

>  {

>  	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

> index 39b6bf6..4b2c042 100644

> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h

> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);

>  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);

>  void amdgpu_bo_unref(struct amdgpu_bo **bo);

>  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);

> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,

> -			     u64 min_offset, u64 max_offset,

> -			     u64 *gpu_addr);

>  int amdgpu_bo_unpin(struct amdgpu_bo *bo);

>  int amdgpu_bo_evict_vram(struct amdgpu_device *adev);

>  int amdgpu_bo_init(struct amdgpu_device *adev);

> --

> 2.7.4

> 

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2017年09月12日 23:59, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Christian König
>> Sent: Tuesday, September 12, 2017 5:09 AM
>> To: amd-gfx@lists.freedesktop.org
>> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>>
>> From: Christian König <christian.koenig@amd.com>
>>
>> Nobody is using the min/max interface any more.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> I'm not sure it's a good idea to get rid of this.  I can see a need to reserve memory at specific offsets in memory.  Specifically I think SR-IOV will be placing structures in memory to communicate configuration details from the host to the guest.  Also, we should be reserving the vbios scratch area, but we don't currently.
Yes, if our ISP ip is enabled, this reserve memory is must.

David Zhou
>
> Alex
>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
>> -------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>>   2 files changed, 6 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 726a662..8a8add3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>>   		*bo = NULL;
>>   }
>>
>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> -			     u64 min_offset, u64 max_offset,
>> -			     u64 *gpu_addr)
>> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>   {
>>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> +	unsigned lpfn;
>>   	int r, i;
>> -	unsigned fpfn, lpfn;
>>
>>   	if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>   		return -EPERM;
>>
>> -	if (WARN_ON_ONCE(min_offset > max_offset))
>> -		return -EINVAL;
>> -
>>   	/* A shared bo cannot be migrated to VRAM */
>>   	if (bo->prime_shared_count && (domain ==
>> AMDGPU_GEM_DOMAIN_VRAM))
>>   		return -EINVAL;
>> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   		if (gpu_addr)
>>   			*gpu_addr = amdgpu_bo_gpu_offset(bo);
>>
>> -		if (max_offset != 0) {
>> -			u64 domain_start = bo->tbo.bdev-
>>> man[mem_type].gpu_offset;
>> -			WARN_ON_ONCE(max_offset <
>> -				     (amdgpu_bo_gpu_offset(bo) -
>> domain_start));
>> -		}
>> -
>>   		return 0;
>>   	}
>>
>> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   	for (i = 0; i < bo->placement.num_placement; i++) {
>>   		/* force to pin into visible video ram */
>>   		if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>> -		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>> &&
>> -		    (!max_offset || max_offset >
>> -		     adev->mc.visible_vram_size)) {
>> -			if (WARN_ON_ONCE(min_offset >
>> -					 adev->mc.visible_vram_size))
>> -				return -EINVAL;
>> -			fpfn = min_offset >> PAGE_SHIFT;
>> +		    !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>> {
>>   			lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>> -		} else {
>> -			fpfn = min_offset >> PAGE_SHIFT;
>> -			lpfn = max_offset >> PAGE_SHIFT;
>> +			if (!bo->placements[i].lpfn ||
>> +			    (lpfn && lpfn < bo->placements[i].lpfn))
>> +				bo->placements[i].lpfn = lpfn;
>>   		}
>> -		if (fpfn > bo->placements[i].fpfn)
>> -			bo->placements[i].fpfn = fpfn;
>> -		if (!bo->placements[i].lpfn ||
>> -		    (lpfn && lpfn < bo->placements[i].lpfn))
>> -			bo->placements[i].lpfn = lpfn;
>>   		bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>   	}
>>
>> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> *bo, u32 domain,
>>   	return r;
>>   }
>>
>> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> -{
>> -	return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>> -}
>> -
>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>   {
>>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index 39b6bf6..4b2c042 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>>   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>>   void amdgpu_bo_unref(struct amdgpu_bo **bo);
>>   int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> -			     u64 min_offset, u64 max_offset,
>> -			     u64 *gpu_addr);
>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo);
>>   int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
>>   int amdgpu_bo_init(struct amdgpu_device *adev);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 13.09.2017 um 04:23 schrieb zhoucm1:
>
>
> On 2017年09月12日 23:59, Deucher, Alexander wrote:
>>> -----Original Message-----
>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>>> Of Christian König
>>> Sent: Tuesday, September 12, 2017 5:09 AM
>>> To: amd-gfx@lists.freedesktop.org
>>> Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>>>
>>> From: Christian König <christian.koenig@amd.com>
>>>
>>> Nobody is using the min/max interface any more.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> I'm not sure it's a good idea to get rid of this.  I can see a need 
>> to reserve memory at specific offsets in memory. Specifically I think 
>> SR-IOV will be placing structures in memory to communicate 
>> configuration details from the host to the guest.  Also, we should be 
>> reserving the vbios scratch area, but we don't currently.
> Yes, if our ISP ip is enabled, this reserve memory is must.

Ok in this case I'm going to drop this one.

Christian.

>
> David Zhou
>>
>> Alex
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 
>>> +++++------------------
>>> -------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>>>   2 files changed, 6 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 726a662..8a8add3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>>>           *bo = NULL;
>>>   }
>>>
>>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>>> -                 u64 min_offset, u64 max_offset,
>>> -                 u64 *gpu_addr)
>>> +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>>   {
>>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>> +    unsigned lpfn;
>>>       int r, i;
>>> -    unsigned fpfn, lpfn;
>>>
>>>       if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>>>           return -EPERM;
>>>
>>> -    if (WARN_ON_ONCE(min_offset > max_offset))
>>> -        return -EINVAL;
>>> -
>>>       /* A shared bo cannot be migrated to VRAM */
>>>       if (bo->prime_shared_count && (domain ==
>>> AMDGPU_GEM_DOMAIN_VRAM))
>>>           return -EINVAL;
>>> @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>           if (gpu_addr)
>>>               *gpu_addr = amdgpu_bo_gpu_offset(bo);
>>>
>>> -        if (max_offset != 0) {
>>> -            u64 domain_start = bo->tbo.bdev-
>>>> man[mem_type].gpu_offset;
>>> -            WARN_ON_ONCE(max_offset <
>>> -                     (amdgpu_bo_gpu_offset(bo) -
>>> domain_start));
>>> -        }
>>> -
>>>           return 0;
>>>       }
>>>
>>> @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>       for (i = 0; i < bo->placement.num_placement; i++) {
>>>           /* force to pin into visible video ram */
>>>           if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>>> -            !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>>> &&
>>> -            (!max_offset || max_offset >
>>> -             adev->mc.visible_vram_size)) {
>>> -            if (WARN_ON_ONCE(min_offset >
>>> -                     adev->mc.visible_vram_size))
>>> -                return -EINVAL;
>>> -            fpfn = min_offset >> PAGE_SHIFT;
>>> +            !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>>> {
>>>               lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>>> -        } else {
>>> -            fpfn = min_offset >> PAGE_SHIFT;
>>> -            lpfn = max_offset >> PAGE_SHIFT;
>>> +            if (!bo->placements[i].lpfn ||
>>> +                (lpfn && lpfn < bo->placements[i].lpfn))
>>> +                bo->placements[i].lpfn = lpfn;
>>>           }
>>> -        if (fpfn > bo->placements[i].fpfn)
>>> -            bo->placements[i].fpfn = fpfn;
>>> -        if (!bo->placements[i].lpfn ||
>>> -            (lpfn && lpfn < bo->placements[i].lpfn))
>>> -            bo->placements[i].lpfn = lpfn;
>>>           bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>>>       }
>>>
>>> @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>>> *bo, u32 domain,
>>>       return r;
>>>   }
>>>
>>> -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>>> -{
>>> -    return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>>> -}
>>> -
>>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>>>   {
>>>       struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> index 39b6bf6..4b2c042 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>>>   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>>>   void amdgpu_bo_unref(struct amdgpu_bo **bo);
>>>   int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>>> -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>>> -                 u64 min_offset, u64 max_offset,
>>> -                 u64 *gpu_addr);
>>>   int amdgpu_bo_unpin(struct amdgpu_bo *bo);
>>>   int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
>>>   int amdgpu_bo_init(struct amdgpu_device *adev);
>>> -- 
>>> 2.7.4
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
SRIOV need to reserve a memory at an offset that set by GIM/hypervisor side, but I'm not sure how to do it perfectly,  currently we call bo_create to allocate a VRAM BO, and call pin_restrict with "offset" as parameter for "min" and "offset + size" as "max",


I feel strange of above approach frankly speaking (unless the new offset equals to the original offset from bo_create),


Because the original gpu offset (from the bo_create) is different with the new "offset" provided by GIM, what will TTM/DRM do on the range of <original offset, new offset> after we pin the bo to <new offset, new offset+ size> ???


BR Monk
amdgpu_bo_create() doesn't necessarily allocate anything, it just 
creates the BO structure.

The backing memory for GTT and CPU domain is only allocated on first 
use, only VRAM is allocated directly.

So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then the 
pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.

Regards,
Christian.

Am 13.09.2017 um 11:14 schrieb Liu, Monk:
>
> SRIOV need to reserve a memory at an offset that set by GIM/hypervisor 
> side, but I'm not sure how to do it perfectly,  currently we call 
> bo_create to allocate a VRAM BO, and call pin_restrict with "offset" 
> as parameter for "min" and "offset + size" as "max",
>
>
> I feel strange of above approach frankly speaking (unless the new 
> offset equals to the original offset from bo_create),
>
>
> Because the original gpu offset (from the bo_create) is different with 
> the new "offset" provided by GIM, what will TTM/DRM do on the range of 
> <original offset, new offset> after we pin the bo to <new offset, new 
> offset+ size> ???
>
>
> BR Monk
>
>
> ------------------------------------------------------------------------
> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
> Deucher, Alexander <Alexander.Deucher@amd.com>
> *Sent:* Tuesday, September 12, 2017 11:59:35 PM
> *To:* 'Christian König'; amd-gfx@lists.freedesktop.org
> *Subject:* RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> > -----Original Message-----
> > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> > Of Christian König
> > Sent: Tuesday, September 12, 2017 5:09 AM
> > To: amd-gfx@lists.freedesktop.org
> > Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> >
> > From: Christian König <christian.koenig@amd.com>
> >
> > Nobody is using the min/max interface any more.
> >
> > Signed-off-by: Christian König <christian.koenig@amd.com>
>
> I'm not sure it's a good idea to get rid of this.  I can see a need to 
> reserve memory at specific offsets in memory. Specifically I think 
> SR-IOV will be placing structures in memory to communicate 
> configuration details from the host to the guest.  Also, we should be 
> reserving the vbios scratch area, but we don't currently.
>
> Alex
>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 +++++------------------
> > -------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
> >  2 files changed, 6 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 726a662..8a8add3 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
> >                *bo = NULL;
> >  }
> >
> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> > -                          u64 min_offset, u64 max_offset,
> > -                          u64 *gpu_addr)
> > +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> >  {
> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> > +     unsigned lpfn;
> >        int r, i;
> > -     unsigned fpfn, lpfn;
> >
> >        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
> >                return -EPERM;
> >
> > -     if (WARN_ON_ONCE(min_offset > max_offset))
> > -             return -EINVAL;
> > -
> >        /* A shared bo cannot be migrated to VRAM */
> >        if (bo->prime_shared_count && (domain ==
> > AMDGPU_GEM_DOMAIN_VRAM))
> >                return -EINVAL;
> > @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >                if (gpu_addr)
> >                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
> >
> > -             if (max_offset != 0) {
> > -                     u64 domain_start = bo->tbo.bdev-
> > >man[mem_type].gpu_offset;
> > -                     WARN_ON_ONCE(max_offset <
> > - (amdgpu_bo_gpu_offset(bo) -
> > domain_start));
> > -             }
> > -
> >                return 0;
> >        }
> >
> > @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >        for (i = 0; i < bo->placement.num_placement; i++) {
> >                /* force to pin into visible video ram */
> >                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
> > -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
> > &&
> > -                 (!max_offset || max_offset >
> > -                  adev->mc.visible_vram_size)) {
> > -                     if (WARN_ON_ONCE(min_offset >
> > - adev->mc.visible_vram_size))
> > -                             return -EINVAL;
> > -                     fpfn = min_offset >> PAGE_SHIFT;
> > +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
> > {
> >                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
> > -             } else {
> > -                     fpfn = min_offset >> PAGE_SHIFT;
> > -                     lpfn = max_offset >> PAGE_SHIFT;
> > +                     if (!bo->placements[i].lpfn ||
> > +                         (lpfn && lpfn < bo->placements[i].lpfn))
> > +                             bo->placements[i].lpfn = lpfn;
> >                }
> > -             if (fpfn > bo->placements[i].fpfn)
> > -                     bo->placements[i].fpfn = fpfn;
> > -             if (!bo->placements[i].lpfn ||
> > -                 (lpfn && lpfn < bo->placements[i].lpfn))
> > -                     bo->placements[i].lpfn = lpfn;
> >                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> >        }
> >
> > @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
> > *bo, u32 domain,
> >        return r;
> >  }
> >
> > -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
> > -{
> > -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
> > -}
> > -
> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
> >  {
> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > index 39b6bf6..4b2c042 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
> >  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
> >  void amdgpu_bo_unref(struct amdgpu_bo **bo);
> >  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> > -                          u64 min_offset, u64 max_offset,
> > -                          u64 *gpu_addr);
> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo);
> >  int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
> >  int amdgpu_bo_init(struct amdgpu_device *adev);
> > --
> > 2.7.4
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
but we use VRAM domain in the first bo_create stage,  so this BO is allocated directly ...


I think our current approach is incorrect ... because the space from bo_create(VRAM_DOMAIN) is missed somehow
> but we use VRAM domain in the first bo_create stage,  so this BO is 
> allocated directly ...
>
Yeah, but just stop doing so. amdgpu_bo_create can just be used to 
allocate the BO structure without any backing storage.

So what you do is calling amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU 
(or just 0 might work as well).

And then when you call amdgpu_bo_pin_restricted() you give 
AMDGPU_GEM_DOMAIN_VRAM and your offset and size as min/max value for the 
placement.

This way you can avoid the move and allocate the BO directly where you 
want it.

Regards,
Christian.

Am 13.09.2017 um 11:26 schrieb Liu, Monk:
>
> but we use VRAM domain in the first bo_create stage,  so this BO is 
> allocated directly ...
>
>
> I think our current approach is incorrect ... because the space from 
> bo_create(VRAM_DOMAIN) is missed somehow
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple@vodafone.de>
> *Sent:* Wednesday, September 13, 2017 5:21:13 PM
> *To:* Liu, Monk; Deucher, Alexander; amd-gfx@lists.freedesktop.org
> *Subject:* Re: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
> amdgpu_bo_create() doesn't necessarily allocate anything, it just 
> creates the BO structure.
>
> The backing memory for GTT and CPU domain is only allocated on first 
> use, only VRAM is allocated directly.
>
> So just call amdgpu_bo_create() with AMDGPU_GEM_DOMAIN_CPU and then 
> the pin with AMDGPU_GEM_DOMAIN_VRAM and your desired offset.
>
> Regards,
> Christian.
>
> Am 13.09.2017 um 11:14 schrieb Liu, Monk:
>>
>> SRIOV need to reserve a memory at an offset that set by 
>> GIM/hypervisor side, but I'm not sure how to do it perfectly,  
>> currently we call bo_create to allocate a VRAM BO, and call 
>> pin_restrict with "offset" as parameter for "min" and "offset + size" 
>> as "max",
>>
>>
>> I feel strange of above approach frankly speaking (unless the new 
>> offset equals to the original offset from bo_create),
>>
>>
>> Because the original gpu offset (from the bo_create) is different 
>> with the new "offset" provided by GIM, what will TTM/DRM do on the 
>> range of <original offset, new offset> after we pin the bo to <new 
>> offset, new offset+ size> ???
>>
>>
>> BR Monk
>>
>>
>> ------------------------------------------------------------------------
>> *From:* amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of 
>> Deucher, Alexander <Alexander.Deucher@amd.com>
>> *Sent:* Tuesday, September 12, 2017 11:59:35 PM
>> *To:* 'Christian König'; amd-gfx@lists.freedesktop.org
>> *Subject:* RE: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>> > -----Original Message-----
>> > From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> > Of Christian König
>> > Sent: Tuesday, September 12, 2017 5:09 AM
>> > To: amd-gfx@lists.freedesktop.org
>> > Subject: [PATCH 4/5] drm/amdgpu: cleanup amdgpu_bo_pin_restricted
>> >
>> > From: Christian König <christian.koenig@amd.com>
>> >
>> > Nobody is using the min/max interface any more.
>> >
>> > Signed-off-by: Christian König <christian.koenig@amd.com>
>>
>> I'm not sure it's a good idea to get rid of this.  I can see a need 
>> to reserve memory at specific offsets in memory.  Specifically I 
>> think SR-IOV will be placing structures in memory to communicate 
>> configuration details from the host to the guest.  Also, we should be 
>> reserving the vbios scratch area, but we don't currently.
>>
>> Alex
>>
>> > ---
>> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 39 
>> +++++------------------
>> > -------
>> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  3 ---
>> >  2 files changed, 6 insertions(+), 36 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > index 726a662..8a8add3 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> > @@ -629,20 +629,15 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo)
>> >                *bo = NULL;
>> >  }
>> >
>> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> > -                          u64 min_offset, u64 max_offset,
>> > -                          u64 *gpu_addr)
>> > +int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> >  {
>> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> > +     unsigned lpfn;
>> >        int r, i;
>> > -     unsigned fpfn, lpfn;
>> >
>> >        if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm))
>> >                return -EPERM;
>> >
>> > -     if (WARN_ON_ONCE(min_offset > max_offset))
>> > -             return -EINVAL;
>> > -
>> >        /* A shared bo cannot be migrated to VRAM */
>> >        if (bo->prime_shared_count && (domain ==
>> > AMDGPU_GEM_DOMAIN_VRAM))
>> >                return -EINVAL;
>> > @@ -657,12 +652,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >                if (gpu_addr)
>> >                        *gpu_addr = amdgpu_bo_gpu_offset(bo);
>> >
>> > -             if (max_offset != 0) {
>> > -                     u64 domain_start = bo->tbo.bdev-
>> > >man[mem_type].gpu_offset;
>> > -                     WARN_ON_ONCE(max_offset <
>> > - (amdgpu_bo_gpu_offset(bo) -
>> > domain_start));
>> > -             }
>> > -
>> >                return 0;
>> >        }
>> >
>> > @@ -671,23 +660,12 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >        for (i = 0; i < bo->placement.num_placement; i++) {
>> >                /* force to pin into visible video ram */
>> >                if ((bo->placements[i].flags & TTM_PL_FLAG_VRAM) &&
>> > -                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)
>> > &&
>> > -                 (!max_offset || max_offset >
>> > -                  adev->mc.visible_vram_size)) {
>> > -                     if (WARN_ON_ONCE(min_offset >
>> > - adev->mc.visible_vram_size))
>> > -                             return -EINVAL;
>> > -                     fpfn = min_offset >> PAGE_SHIFT;
>> > +                 !(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
>> > {
>> >                        lpfn = adev->mc.visible_vram_size >> PAGE_SHIFT;
>> > -             } else {
>> > -                     fpfn = min_offset >> PAGE_SHIFT;
>> > -                     lpfn = max_offset >> PAGE_SHIFT;
>> > +                     if (!bo->placements[i].lpfn ||
>> > +                         (lpfn && lpfn < bo->placements[i].lpfn))
>> > + bo->placements[i].lpfn = lpfn;
>> >                }
>> > -             if (fpfn > bo->placements[i].fpfn)
>> > -                     bo->placements[i].fpfn = fpfn;
>> > -             if (!bo->placements[i].lpfn ||
>> > -                 (lpfn && lpfn < bo->placements[i].lpfn))
>> > -                     bo->placements[i].lpfn = lpfn;
>> >                bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> >        }
>> >
>> > @@ -718,11 +696,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo
>> > *bo, u32 domain,
>> >        return r;
>> >  }
>> >
>> > -int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr)
>> > -{
>> > -     return amdgpu_bo_pin_restricted(bo, domain, 0, 0, gpu_addr);
>> > -}
>> > -
>> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo)
>> >  {
>> >        struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > index 39b6bf6..4b2c042 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> > @@ -211,9 +211,6 @@ void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>> >  struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
>> >  void amdgpu_bo_unref(struct amdgpu_bo **bo);
>> >  int amdgpu_bo_pin(struct amdgpu_bo *bo, u32 domain, u64 *gpu_addr);
>> > -int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>> > -                          u64 min_offset, u64 max_offset,
>> > -                          u64 *gpu_addr);
>> >  int amdgpu_bo_unpin(struct amdgpu_bo *bo);
>> >  int amdgpu_bo_evict_vram(struct amdgpu_device *adev);
>> >  int amdgpu_bo_init(struct amdgpu_device *adev);
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > amd-gfx mailing list
>> > amd-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx 
>> <https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
yeah, that's right !


Thanks

BR Monk