[PATCH] drm/ttm: do not try to preserve caching state

[PATCH] drm/ttm: do not try to preserve caching state

[j.glisse]

From: Jerome Glisse <jglisse@redhat.com>

It make no sense to preserve caching state especialy when
moving from vram to system. It burden the page allocator to
match the vram caching (often WC) which just burn CPU cycle
for no good reasons.

Signed-off-by: Jerome Glisse <jglisse@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bf6e4b5..39dcc58 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -896,19 +896,12 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 }
 
 static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man,
-				      uint32_t cur_placement,
 				      uint32_t proposed_placement)
 {
 	uint32_t caching = proposed_placement & TTM_PL_MASK_CACHING;
 	uint32_t result = proposed_placement & ~TTM_PL_MASK_CACHING;
 
-	/**
-	 * Keep current caching if possible.
-	 */
-
-	if ((cur_placement & caching) != 0)
-		result |= (cur_placement & caching);
-	else if ((man->default_caching & caching) != 0)
+	if ((man->default_caching & caching) != 0)
 		result |= man->default_caching;
 	else if ((TTM_PL_FLAG_CACHED & caching) != 0)
 		result |= TTM_PL_FLAG_CACHED;
@@ -978,8 +971,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 		if (!type_ok)
 			continue;
 
-		cur_flags = ttm_bo_select_caching(man, bo->mem.placement,
-						  cur_flags);
+		cur_flags = ttm_bo_select_caching(man, cur_flags);
 		/*
 		 * Use the access and other non-mapping-related flag bits from
 		 * the memory placement flags to the current flags
@@ -1023,8 +1015,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
 						&cur_flags))
 			continue;
 
-		cur_flags = ttm_bo_select_caching(man, bo->mem.placement,
-						  cur_flags);
+		cur_flags = ttm_bo_select_caching(man, cur_flags);
 		/*
 		 * Use the access and other non-mapping-related flag bits from
 		 * the memory placement flags to the current flags
-- 
1.7.11.7

Re: [PATCH] drm/ttm: do not try to preserve caching state

[Jerome Glisse]

On Wed, Nov 28, 2012 at 10:05 AM,  <j.glisse@gmail.com> wrote:
> From: Jerome Glisse <jglisse@redhat.com>
>
> It make no sense to preserve caching state especialy when
> moving from vram to system. It burden the page allocator to
> match the vram caching (often WC) which just burn CPU cycle
> for no good reasons.
>
> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index bf6e4b5..39dcc58 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -896,19 +896,12 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
>  }
>
>  static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man,
> -                                     uint32_t cur_placement,
>                                       uint32_t proposed_placement)
>  {
>         uint32_t caching = proposed_placement & TTM_PL_MASK_CACHING;
>         uint32_t result = proposed_placement & ~TTM_PL_MASK_CACHING;
>
> -       /**
> -        * Keep current caching if possible.
> -        */
> -
> -       if ((cur_placement & caching) != 0)
> -               result |= (cur_placement & caching);
> -       else if ((man->default_caching & caching) != 0)
> +       if ((man->default_caching & caching) != 0)
>                 result |= man->default_caching;
>         else if ((TTM_PL_FLAG_CACHED & caching) != 0)
>                 result |= TTM_PL_FLAG_CACHED;
> @@ -978,8 +971,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                 if (!type_ok)
>                         continue;
>
> -               cur_flags = ttm_bo_select_caching(man, bo->mem.placement,
> -                                                 cur_flags);
> +               cur_flags = ttm_bo_select_caching(man, cur_flags);
>                 /*
>                  * Use the access and other non-mapping-related flag bits from
>                  * the memory placement flags to the current flags
> @@ -1023,8 +1015,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo,
>                                                 &cur_flags))
>                         continue;
>
> -               cur_flags = ttm_bo_select_caching(man, bo->mem.placement,
> -                                                 cur_flags);
> +               cur_flags = ttm_bo_select_caching(man, cur_flags);
>                 /*
>                  * Use the access and other non-mapping-related flag bits from
>                  * the memory placement flags to the current flags
> --
> 1.7.11.7
>

Note this is mostly to mitigate performance decrease when a lot of
eviction happen. It increase perf by as much of 60% on my computer.

Cheers,
Jerome

Re: [PATCH] drm/ttm: do not try to preserve caching state

[Thomas Hellstrom]

On 11/28/2012 09:09 PM, Jerome Glisse wrote:
> On Wed, Nov 28, 2012 at 10:05 AM,  <j.glisse@gmail.com> wrote:
>> From: Jerome Glisse <jglisse@redhat.com>
>>
>> It make no sense to preserve caching state especialy when
>> moving from vram to system. It burden the page allocator to
>> match the vram caching (often WC) which just burn CPU cycle
>> for no good reasons.

Nack.
This is a driver problem.

What happens with this patch if you evict write-combined TT memory to 
system?
That's why we want to preserve caching state in the first place.

If you need a different behavior, you can fine-tune in 
driver::evict_flags, or in the
radeon-case in radeon_move_ram_vram / radeon_move_vram_ram.

/Thomas

Re: [PATCH] drm/ttm: do not try to preserve caching state

[Jerome Glisse]

On Wed, Nov 28, 2012 at 5:18 PM, Thomas Hellstrom <thomas@shipmail.org> wrote:
> On 11/28/2012 09:09 PM, Jerome Glisse wrote:
>>
>> On Wed, Nov 28, 2012 at 10:05 AM,  <j.glisse@gmail.com> wrote:
>>>
>>> From: Jerome Glisse <jglisse@redhat.com>
>>>
>>> It make no sense to preserve caching state especialy when
>>> moving from vram to system. It burden the page allocator to
>>> match the vram caching (often WC) which just burn CPU cycle
>>> for no good reasons.
>
>
> Nack.
> This is a driver problem.
>
> What happens with this patch if you evict write-combined TT memory to
> system?
> That's why we want to preserve caching state in the first place.
>
> If you need a different behavior, you can fine-tune in driver::evict_flags,
> or in the
> radeon-case in radeon_move_ram_vram / radeon_move_vram_ram.

No you can't. Because cur placement of vram will be wc, and thus it
will want to preserve it for gtt. Only way to force it is to declare
vram as cached too.

Cheers,
Jerome

Re: [PATCH] drm/ttm: do not try to preserve caching state

[Jerome Glisse]

On Wed, Nov 28, 2012 at 6:13 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Wed, Nov 28, 2012 at 5:18 PM, Thomas Hellstrom <thomas@shipmail.org> wrote:
>> On 11/28/2012 09:09 PM, Jerome Glisse wrote:
>>>
>>> On Wed, Nov 28, 2012 at 10:05 AM,  <j.glisse@gmail.com> wrote:
>>>>
>>>> From: Jerome Glisse <jglisse@redhat.com>
>>>>
>>>> It make no sense to preserve caching state especialy when
>>>> moving from vram to system. It burden the page allocator to
>>>> match the vram caching (often WC) which just burn CPU cycle
>>>> for no good reasons.
>>
>>
>> Nack.
>> This is a driver problem.
>>
>> What happens with this patch if you evict write-combined TT memory to
>> system?
>> That's why we want to preserve caching state in the first place.
>>
>> If you need a different behavior, you can fine-tune in driver::evict_flags,
>> or in the
>> radeon-case in radeon_move_ram_vram / radeon_move_vram_ram.
>
> No you can't. Because cur placement of vram will be wc, and thus it
> will want to preserve it for gtt. Only way to force it is to declare
> vram as cached too.
>
> Cheers,
> Jerome

Now that i think a bit more to it, in all fairness you can do so from
the driver side.

Cheers,
Jerome