[PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The radeon driver uses placement range restrictions for several reasons,
in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
during a page fault.

Without this change, TTM could evict other BOs while trying to satisfy
the requested placement, even if the evicted BOs were outside of the
requested placement range. Doing so didn't free up any space in the
requested placement range, so the (potentially high) eviction cost was
incurred for no benefit.

Nominating for stable because radeon driver changes in 3.17 made this
much more noticeable than before.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
Cc: stable@vger.kernel.org
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 8f5cec6..407fa2d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -709,6 +709,7 @@ out:
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 				uint32_t mem_type,
+				const struct ttm_place *place,
 				bool interruptible,
 				bool no_wait_gpu)
 {
@@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	list_for_each_entry(bo, &man->lru, lru) {
 		ret = __ttm_bo_reserve(bo, false, true, false, NULL);
-		if (!ret)
+		if (!ret) {
+			if (place && (place->fpfn || place->lpfn)) {
+				/* Don't evict this BO if it's outside of the
+				 * requested placement range
+				 */
+				if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
+				    (place->lpfn && place->lpfn <= bo->mem.start)) {
+					__ttm_bo_unreserve(bo);
+					ret = -EBUSY;
+					continue;
+				}
+			}
+
 			break;
+		}
 	}
 
 	if (ret) {
@@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type,
+		ret = ttm_mem_evict_first(bdev, mem_type, place,
 					  interruptible, no_wait_gpu);
 		if (unlikely(ret != 0))
 			return ret;
@@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	while (!list_empty(&man->lru)) {
 		spin_unlock(&glob->lru_lock);
-		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
+		ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
 		if (ret) {
 			if (allow_errors) {
 				return ret;
-- 
2.1.1

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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Christian König]

Am 09.10.2014 um 08:02 schrieb Michel Dänzer:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> The radeon driver uses placement range restrictions for several reasons,
> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> during a page fault.
>
> Without this change, TTM could evict other BOs while trying to satisfy
> the requested placement, even if the evicted BOs were outside of the
> requested placement range. Doing so didn't free up any space in the
> requested placement range, so the (potentially high) eviction cost was
> incurred for no benefit.
>
> Nominating for stable because radeon driver changes in 3.17 made this
> much more noticeable than before.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> Cc: stable@vger.kernel.org
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

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

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 8f5cec6..407fa2d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -709,6 +709,7 @@ out:
>   
>   static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   				uint32_t mem_type,
> +				const struct ttm_place *place,
>   				bool interruptible,
>   				bool no_wait_gpu)
>   {
> @@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>   	spin_lock(&glob->lru_lock);
>   	list_for_each_entry(bo, &man->lru, lru) {
>   		ret = __ttm_bo_reserve(bo, false, true, false, NULL);
> -		if (!ret)
> +		if (!ret) {
> +			if (place && (place->fpfn || place->lpfn)) {
> +				/* Don't evict this BO if it's outside of the
> +				 * requested placement range
> +				 */
> +				if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
> +				    (place->lpfn && place->lpfn <= bo->mem.start)) {
> +					__ttm_bo_unreserve(bo);
> +					ret = -EBUSY;
> +					continue;
> +				}
> +			}
> +
>   			break;
> +		}
>   	}
>   
>   	if (ret) {
> @@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>   			return ret;
>   		if (mem->mm_node)
>   			break;
> -		ret = ttm_mem_evict_first(bdev, mem_type,
> +		ret = ttm_mem_evict_first(bdev, mem_type, place,
>   					  interruptible, no_wait_gpu);
>   		if (unlikely(ret != 0))
>   			return ret;
> @@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
>   	spin_lock(&glob->lru_lock);
>   	while (!list_empty(&man->lru)) {
>   		spin_unlock(&glob->lru_lock);
> -		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
> +		ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
>   		if (ret) {
>   			if (allow_errors) {
>   				return ret;

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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Alan Swanson]

On 2014-10-09 07:02, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> The radeon driver uses placement range restrictions for several 
> reasons,
> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> during a page fault.
> 
> Without this change, TTM could evict other BOs while trying to satisfy
> the requested placement, even if the evicted BOs were outside of the
> requested placement range. Doing so didn't free up any space in the
> requested placement range, so the (potentially high) eviction cost was
> incurred for no benefit.
> 
> Nominating for stable because radeon driver changes in 3.17 made this
> much more noticeable than before.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> Cc: stable@vger.kernel.org
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c 
> b/drivers/gpu/drm/ttm/ttm_bo.c
> index 8f5cec6..407fa2d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -709,6 +709,7 @@ out:
> 
>  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>  				uint32_t mem_type,
> +				const struct ttm_place *place,
>  				bool interruptible,
>  				bool no_wait_gpu)
>  {
> @@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct 
> ttm_bo_device *bdev,
>  	spin_lock(&glob->lru_lock);
>  	list_for_each_entry(bo, &man->lru, lru) {
>  		ret = __ttm_bo_reserve(bo, false, true, false, NULL);
> -		if (!ret)
> +		if (!ret) {
> +			if (place && (place->fpfn || place->lpfn)) {
> +				/* Don't evict this BO if it's outside of the
> +				 * requested placement range
> +				 */
> +				if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
> +				    (place->lpfn && place->lpfn <= bo->mem.start)) {
> +					__ttm_bo_unreserve(bo);
> +					ret = -EBUSY;
> +					continue;
> +				}
> +			}
> +
>  			break;
> +		}
>  	}
> 
>  	if (ret) {
> @@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct
> ttm_buffer_object *bo,
>  			return ret;
>  		if (mem->mm_node)
>  			break;
> -		ret = ttm_mem_evict_first(bdev, mem_type,
> +		ret = ttm_mem_evict_first(bdev, mem_type, place,
>  					  interruptible, no_wait_gpu);
>  		if (unlikely(ret != 0))
>  			return ret;
> @@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct
> ttm_bo_device *bdev,
>  	spin_lock(&glob->lru_lock);
>  	while (!list_empty(&man->lru)) {
>  		spin_unlock(&glob->lru_lock);
> -		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
> +		ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
>  		if (ret) {
>  			if (allow_errors) {
>  				return ret;

I believe you need to "s/place/placement/" over this patch.

-- 
Alan.

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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Michel Dänzer]

On 09.10.2014 19:22, Alan Swanson wrote:
> On 2014-10-09 07:02, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> The radeon driver uses placement range restrictions for several reasons,
>> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
>> during a page fault.
>>
>> Without this change, TTM could evict other BOs while trying to satisfy
>> the requested placement, even if the evicted BOs were outside of the
>> requested placement range. Doing so didn't free up any space in the
>> requested placement range, so the (potentially high) eviction cost was
>> incurred for no benefit.
>>
>> Nominating for stable because radeon driver changes in 3.17 made this
>> much more noticeable than before.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 8f5cec6..407fa2d 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -709,6 +709,7 @@ out:
>>
>>  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>                  uint32_t mem_type,
>> +                const struct ttm_place *place,
>>                  bool interruptible,
>>                  bool no_wait_gpu)
>>  {
>> @@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct
>> ttm_bo_device *bdev,
>>      spin_lock(&glob->lru_lock);
>>      list_for_each_entry(bo, &man->lru, lru) {
>>          ret = __ttm_bo_reserve(bo, false, true, false, NULL);
>> -        if (!ret)
>> +        if (!ret) {
>> +            if (place && (place->fpfn || place->lpfn)) {
>> +                /* Don't evict this BO if it's outside of the
>> +                 * requested placement range
>> +                 */
>> +                if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
>> +                    (place->lpfn && place->lpfn <= bo->mem.start)) {
>> +                    __ttm_bo_unreserve(bo);
>> +                    ret = -EBUSY;
>> +                    continue;
>> +                }
>> +            }
>> +
>>              break;
>> +        }
>>      }
>>
>>      if (ret) {
>> @@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct
>> ttm_buffer_object *bo,
>>              return ret;
>>          if (mem->mm_node)
>>              break;
>> -        ret = ttm_mem_evict_first(bdev, mem_type,
>> +        ret = ttm_mem_evict_first(bdev, mem_type, place,
>>                        interruptible, no_wait_gpu);
>>          if (unlikely(ret != 0))
>>              return ret;

[...]

> I believe you need to "s/place/placement/" over this patch.

The fpfn and lpfn members were moved from struct ttm_placement to a new 
struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.

If you mean something else, please elaborate.


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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Alan Swanson]

On Fri, 2014-10-10 at 12:20 +0900, Michel Dänzer wrote:
> On 09.10.2014 19:22, Alan Swanson wrote:
> > On 2014-10-09 07:02, Michel Dänzer wrote:
> >> From: Michel Dänzer <michel.daenzer@amd.com>
> >>
> >> The radeon driver uses placement range restrictions for several reasons,
> >> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> >> during a page fault.
> >>
> >> Without this change, TTM could evict other BOs while trying to satisfy
> >> the requested placement, even if the evicted BOs were outside of the
> >> requested placement range. Doing so didn't free up any space in the
> >> requested placement range, so the (potentially high) eviction cost was
> >> incurred for no benefit.
> >>
> >> Nominating for stable because radeon driver changes in 3.17 made this
> >> much more noticeable than before.
> >>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> >> ---
> >>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
> >>  1 file changed, 17 insertions(+), 3 deletions(-)
> >>
> [...]
> 
> > I believe you need to "s/place/placement/" over this patch.
> 
> The fpfn and lpfn members were moved from struct ttm_placement to a new 
> struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.
> 
> If you mean something else, please elaborate.

This patch failed to build on 3.17.0 so wouldn't be a candidate for
stable unless the currently drm-next only ttm_place patch also goes to
stable (else replace ttm_place with ttm_placements in the patch for
stable)?

-- 
Alan.

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Michel Dänzer]

On 10.10.2014 17:51, Alan Swanson wrote:
> On Fri, 2014-10-10 at 12:20 +0900, Michel Dänzer wrote:
>> On 09.10.2014 19:22, Alan Swanson wrote:
>>> On 2014-10-09 07:02, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>> The radeon driver uses placement range restrictions for several reasons,
>>>> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
>>>> during a page fault.
>>>>
>>>> Without this change, TTM could evict other BOs while trying to satisfy
>>>> the requested placement, even if the evicted BOs were outside of the
>>>> requested placement range. Doing so didn't free up any space in the
>>>> requested placement range, so the (potentially high) eviction cost was
>>>> incurred for no benefit.
>>>>
>>>> Nominating for stable because radeon driver changes in 3.17 made this
>>>> much more noticeable than before.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>>>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>>>
>> [...]
>>
>>> I believe you need to "s/place/placement/" over this patch.
>>
>> The fpfn and lpfn members were moved from struct ttm_placement to a new
>> struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.
>>
>> If you mean something else, please elaborate.
>
> This patch failed to build on 3.17.0 so wouldn't be a candidate for
> stable unless the currently drm-next only ttm_place patch also goes to
> stable (else replace ttm_place with ttm_placements in the patch for
> stable)?

Right, I guess I should drop the Cc: stable then and submit a manual 
backport of it to the stable list once it has landed in Linus' tree.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

[PATCH v2] drm/ttm: Don't evict BOs outside of the requested placement range

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The radeon driver uses placement range restrictions for several reasons,
in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
during a page fault.

Without this change, TTM could evict other BOs while trying to satisfy
the requested placement, even if the evicted BOs were outside of the
requested placement range. Doing so didn't free up any space in the
requested placement range, so the (potentially high) eviction cost was
incurred for no benefit.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
Reviewed-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

v2: Dropped Cc: stable, will need to backport manually because the code
    has changed since 3.17

 drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 8f5cec6..407fa2d 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -709,6 +709,7 @@ out:
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 				uint32_t mem_type,
+				const struct ttm_place *place,
 				bool interruptible,
 				bool no_wait_gpu)
 {
@@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	list_for_each_entry(bo, &man->lru, lru) {
 		ret = __ttm_bo_reserve(bo, false, true, false, NULL);
-		if (!ret)
+		if (!ret) {
+			if (place && (place->fpfn || place->lpfn)) {
+				/* Don't evict this BO if it's outside of the
+				 * requested placement range
+				 */
+				if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
+				    (place->lpfn && place->lpfn <= bo->mem.start)) {
+					__ttm_bo_unreserve(bo);
+					ret = -EBUSY;
+					continue;
+				}
+			}
+
 			break;
+		}
 	}
 
 	if (ret) {
@@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type,
+		ret = ttm_mem_evict_first(bdev, mem_type, place,
 					  interruptible, no_wait_gpu);
 		if (unlikely(ret != 0))
 			return ret;
@@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	spin_lock(&glob->lru_lock);
 	while (!list_empty(&man->lru)) {
 		spin_unlock(&glob->lru_lock);
-		ret = ttm_mem_evict_first(bdev, mem_type, false, false);
+		ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
 		if (ret) {
 			if (allow_errors) {
 				return ret;
-- 
2.1.1

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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Daniel Vetter]

On Fri, Oct 10, 2014 at 05:59:19PM +0900, Michel Dänzer wrote:
> On 10.10.2014 17:51, Alan Swanson wrote:
> >On Fri, 2014-10-10 at 12:20 +0900, Michel Dänzer wrote:
> >>On 09.10.2014 19:22, Alan Swanson wrote:
> >>>On 2014-10-09 07:02, Michel Dänzer wrote:
> >>>>From: Michel Dänzer <michel.daenzer@amd.com>
> >>>>
> >>>>The radeon driver uses placement range restrictions for several reasons,
> >>>>in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> >>>>during a page fault.
> >>>>
> >>>>Without this change, TTM could evict other BOs while trying to satisfy
> >>>>the requested placement, even if the evicted BOs were outside of the
> >>>>requested placement range. Doing so didn't free up any space in the
> >>>>requested placement range, so the (potentially high) eviction cost was
> >>>>incurred for no benefit.
> >>>>
> >>>>Nominating for stable because radeon driver changes in 3.17 made this
> >>>>much more noticeable than before.
> >>>>
> >>>>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> >>>>Cc: stable@vger.kernel.org
> >>>>Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> >>>>---
> >>>>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
> >>>>  1 file changed, 17 insertions(+), 3 deletions(-)
> >>>>
> >>[...]
> >>
> >>>I believe you need to "s/place/placement/" over this patch.
> >>
> >>The fpfn and lpfn members were moved from struct ttm_placement to a new
> >>struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.
> >>
> >>If you mean something else, please elaborate.
> >
> >This patch failed to build on 3.17.0 so wouldn't be a candidate for
> >stable unless the currently drm-next only ttm_place patch also goes to
> >stable (else replace ttm_place with ttm_placements in the patch for
> >stable)?
> 
> Right, I guess I should drop the Cc: stable then and submit a manual
> backport of it to the stable list once it has landed in Linus' tree.

I've thought it's ok to cc: stable a patch - Greg's scripts will send you
a mail as a nice reminder if the patch fails to apply. At least we
regularly pull this stunt with i915 patches. Cc'ing Greg for
clarification.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Greg KH]

On Sat, Oct 11, 2014 at 08:31:49PM +0200, Daniel Vetter wrote:
> On Fri, Oct 10, 2014 at 05:59:19PM +0900, Michel Dänzer wrote:
> > On 10.10.2014 17:51, Alan Swanson wrote:
> > >On Fri, 2014-10-10 at 12:20 +0900, Michel Dänzer wrote:
> > >>On 09.10.2014 19:22, Alan Swanson wrote:
> > >>>On 2014-10-09 07:02, Michel Dänzer wrote:
> > >>>>From: Michel Dänzer <michel.daenzer@amd.com>
> > >>>>
> > >>>>The radeon driver uses placement range restrictions for several reasons,
> > >>>>in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> > >>>>during a page fault.
> > >>>>
> > >>>>Without this change, TTM could evict other BOs while trying to satisfy
> > >>>>the requested placement, even if the evicted BOs were outside of the
> > >>>>requested placement range. Doing so didn't free up any space in the
> > >>>>requested placement range, so the (potentially high) eviction cost was
> > >>>>incurred for no benefit.
> > >>>>
> > >>>>Nominating for stable because radeon driver changes in 3.17 made this
> > >>>>much more noticeable than before.
> > >>>>
> > >>>>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> > >>>>Cc: stable@vger.kernel.org
> > >>>>Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > >>>>---
> > >>>>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
> > >>>>  1 file changed, 17 insertions(+), 3 deletions(-)
> > >>>>
> > >>[...]
> > >>
> > >>>I believe you need to "s/place/placement/" over this patch.
> > >>
> > >>The fpfn and lpfn members were moved from struct ttm_placement to a new
> > >>struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.
> > >>
> > >>If you mean something else, please elaborate.
> > >
> > >This patch failed to build on 3.17.0 so wouldn't be a candidate for
> > >stable unless the currently drm-next only ttm_place patch also goes to
> > >stable (else replace ttm_place with ttm_placements in the patch for
> > >stable)?
> > 
> > Right, I guess I should drop the Cc: stable then and submit a manual
> > backport of it to the stable list once it has landed in Linus' tree.
> 
> I've thought it's ok to cc: stable a patch - Greg's scripts will send you
> a mail as a nice reminder if the patch fails to apply. At least we
> regularly pull this stunt with i915 patches. Cc'ing Greg for
> clarification.

Yup, that's fine to do, it's what the scripts are for :)

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Alex Deucher]

On Thu, Oct 9, 2014 at 2:02 AM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> The radeon driver uses placement range restrictions for several reasons,
> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
> during a page fault.
>
> Without this change, TTM could evict other BOs while trying to satisfy
> the requested placement, even if the evicted BOs were outside of the
> requested placement range. Doing so didn't free up any space in the
> requested placement range, so the (potentially high) eviction cost was
> incurred for no benefit.
>
> Nominating for stable because radeon driver changes in 3.17 made this
> much more noticeable than before.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
> Cc: stable@vger.kernel.org
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thomas, do you want to pull this through the ttm tree or can I take it
through radeon?

Alex

> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 8f5cec6..407fa2d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -709,6 +709,7 @@ out:
>
>  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>                                 uint32_t mem_type,
> +                               const struct ttm_place *place,
>                                 bool interruptible,
>                                 bool no_wait_gpu)
>  {
> @@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>         spin_lock(&glob->lru_lock);
>         list_for_each_entry(bo, &man->lru, lru) {
>                 ret = __ttm_bo_reserve(bo, false, true, false, NULL);
> -               if (!ret)
> +               if (!ret) {
> +                       if (place && (place->fpfn || place->lpfn)) {
> +                               /* Don't evict this BO if it's outside of the
> +                                * requested placement range
> +                                */
> +                               if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
> +                                   (place->lpfn && place->lpfn <= bo->mem.start)) {
> +                                       __ttm_bo_unreserve(bo);
> +                                       ret = -EBUSY;
> +                                       continue;
> +                               }
> +                       }
> +
>                         break;
> +               }
>         }
>
>         if (ret) {
> @@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>                         return ret;
>                 if (mem->mm_node)
>                         break;
> -               ret = ttm_mem_evict_first(bdev, mem_type,
> +               ret = ttm_mem_evict_first(bdev, mem_type, place,
>                                           interruptible, no_wait_gpu);
>                 if (unlikely(ret != 0))
>                         return ret;
> @@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
>         spin_lock(&glob->lru_lock);
>         while (!list_empty(&man->lru)) {
>                 spin_unlock(&glob->lru_lock);
> -               ret = ttm_mem_evict_first(bdev, mem_type, false, false);
> +               ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
>                 if (ret) {
>                         if (allow_errors) {
>                                 return ret;
> --
> 2.1.1
>
> _______________________________________________
> 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/ttm: Don't evict BOs outside of the requested placement range

[Thomas Hellstrom]

Alex,

Please pull these patches through Radeon,

Thanks,
Thomas


On 10/13/2014 08:22 PM, Alex Deucher wrote:
> On Thu, Oct 9, 2014 at 2:02 AM, Michel Dänzer <michel@daenzer.net> wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> The radeon driver uses placement range restrictions for several reasons,
>> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
>> during a page fault.
>>
>> Without this change, TTM could evict other BOs while trying to satisfy
>> the requested placement, even if the evicted BOs were outside of the
>> requested placement range. Doing so didn't free up any space in the
>> requested placement range, so the (potentially high) eviction cost was
>> incurred for no benefit.
>>
>> Nominating for stable because radeon driver changes in 3.17 made this
>> much more noticeable than before.
>>
>> Bugzilla: https://urldefense.proofpoint.com/v1/url?u=https://bugs.freedesktop.org/show_bug.cgi?id%3D84662&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=fUnmlW1Tlnje4oNF3duZPPLcUcRvxyQZ%2BA%2Fyl5YOGBc%3D%0A&s=53d70e00aa98306fc71f222fe9aa14b3ff9cf30dab9a060633a2341526cb09cb
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Thomas, do you want to pull this through the ttm tree or can I take it
> through radeon?
>
> Alex
>
>> ---
>>  drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>>  1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 8f5cec6..407fa2d 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -709,6 +709,7 @@ out:
>>
>>  static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>                                 uint32_t mem_type,
>> +                               const struct ttm_place *place,
>>                                 bool interruptible,
>>                                 bool no_wait_gpu)
>>  {
>> @@ -720,8 +721,21 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>>         spin_lock(&glob->lru_lock);
>>         list_for_each_entry(bo, &man->lru, lru) {
>>                 ret = __ttm_bo_reserve(bo, false, true, false, NULL);
>> -               if (!ret)
>> +               if (!ret) {
>> +                       if (place && (place->fpfn || place->lpfn)) {
>> +                               /* Don't evict this BO if it's outside of the
>> +                                * requested placement range
>> +                                */
>> +                               if (place->fpfn >= (bo->mem.start + bo->mem.size) ||
>> +                                   (place->lpfn && place->lpfn <= bo->mem.start)) {
>> +                                       __ttm_bo_unreserve(bo);
>> +                                       ret = -EBUSY;
>> +                                       continue;
>> +                               }
>> +                       }
>> +
>>                         break;
>> +               }
>>         }
>>
>>         if (ret) {
>> @@ -782,7 +796,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>>                         return ret;
>>                 if (mem->mm_node)
>>                         break;
>> -               ret = ttm_mem_evict_first(bdev, mem_type,
>> +               ret = ttm_mem_evict_first(bdev, mem_type, place,
>>                                           interruptible, no_wait_gpu);
>>                 if (unlikely(ret != 0))
>>                         return ret;
>> @@ -1233,7 +1247,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
>>         spin_lock(&glob->lru_lock);
>>         while (!list_empty(&man->lru)) {
>>                 spin_unlock(&glob->lru_lock);
>> -               ret = ttm_mem_evict_first(bdev, mem_type, false, false);
>> +               ret = ttm_mem_evict_first(bdev, mem_type, NULL, false, false);
>>                 if (ret) {
>>                         if (allow_errors) {
>>                                 return ret;
>> --
>> 2.1.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/dri-devel&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=fUnmlW1Tlnje4oNF3duZPPLcUcRvxyQZ%2BA%2Fyl5YOGBc%3D%0A&s=3c28f2dc220442e12a62cd069fd203cefbe4b157be43987cebd908d2084e32e9

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

Re: [PATCH] drm/ttm: Don't evict BOs outside of the requested placement range

[Michel Dänzer]

On 12.10.2014 05:24, Greg KH wrote:
> On Sat, Oct 11, 2014 at 08:31:49PM +0200, Daniel Vetter wrote:
>> On Fri, Oct 10, 2014 at 05:59:19PM +0900, Michel Dänzer wrote:
>>> On 10.10.2014 17:51, Alan Swanson wrote:
>>>> On Fri, 2014-10-10 at 12:20 +0900, Michel Dänzer wrote:
>>>>> On 09.10.2014 19:22, Alan Swanson wrote:
>>>>>> On 2014-10-09 07:02, Michel Dänzer wrote:
>>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>
>>>>>>> The radeon driver uses placement range restrictions for several reasons,
>>>>>>> in particular to make sure BOs in VRAM can be accessed by the CPU, e.g.
>>>>>>> during a page fault.
>>>>>>>
>>>>>>> Without this change, TTM could evict other BOs while trying to satisfy
>>>>>>> the requested placement, even if the evicted BOs were outside of the
>>>>>>> requested placement range. Doing so didn't free up any space in the
>>>>>>> requested placement range, so the (potentially high) eviction cost was
>>>>>>> incurred for no benefit.
>>>>>>>
>>>>>>> Nominating for stable because radeon driver changes in 3.17 made this
>>>>>>> much more noticeable than before.
>>>>>>>
>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84662
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/ttm/ttm_bo.c | 20 +++++++++++++++++---
>>>>>>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>>>>>>
>>>>> [...]
>>>>>
>>>>>> I believe you need to "s/place/placement/" over this patch.
>>>>>
>>>>> The fpfn and lpfn members were moved from struct ttm_placement to a new
>>>>> struct ttm_place in f1217ed09f827e42a49ffa6a5aab672aa6f57a65.
>>>>>
>>>>> If you mean something else, please elaborate.
>>>>
>>>> This patch failed to build on 3.17.0 so wouldn't be a candidate for
>>>> stable unless the currently drm-next only ttm_place patch also goes to
>>>> stable (else replace ttm_place with ttm_placements in the patch for
>>>> stable)?
>>>
>>> Right, I guess I should drop the Cc: stable then and submit a manual
>>> backport of it to the stable list once it has landed in Linus' tree.
>>
>> I've thought it's ok to cc: stable a patch - Greg's scripts will send you
>> a mail as a nice reminder if the patch fails to apply. At least we
>> regularly pull this stunt with i915 patches. Cc'ing Greg for
>> clarification.
>
> Yup, that's fine to do, it's what the scripts are for :)

Okay, thanks guys for the clarification.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer