[PATCH] drm/ttm: Set CPU caching mode to cached for BOs being swapped out

[PATCH] drm/ttm: Set CPU caching mode to cached for BOs being swapped out

[Michel Dänzer]

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

I ran into the BUG_ON in ttm_tt_swapout, presumably the BO being swapped
out was using a write-combined CPU mapping.

Instead of BUGging out, just set the caching mode to what's needed.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/ttm/ttm_tt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index 4e19d0f..c2794eb 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -334,7 +334,8 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
 	int ret = -ENOMEM;
 
 	BUG_ON(ttm->state != tt_unbound && ttm->state != tt_unpopulated);
-	BUG_ON(ttm->caching_state != tt_cached);
+
+	ttm_tt_set_caching(ttm, tt_cached);
 
 	if (!persistent_swap_storage) {
 		swap_storage = shmem_file_setup("ttm swap",
-- 
2.6.2

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

Re: [PATCH] drm/ttm: Set CPU caching mode to cached for BOs being swapped out

[Thomas Hellstrom]

Hi, Michel,

On 11/05/2015 09:08 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> I ran into the BUG_ON in ttm_tt_swapout, presumably the BO being swapped
> out was using a write-combined CPU mapping.
>
> Instead of BUGging out, just set the caching mode to what's needed.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_tt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index 4e19d0f..c2794eb 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -334,7 +334,8 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
>  	int ret = -ENOMEM;
>  
>  	BUG_ON(ttm->state != tt_unbound && ttm->state != tt_unpopulated);
> -	BUG_ON(ttm->caching_state != tt_cached);
> +
> +	ttm_tt_set_caching(ttm, tt_cached);
>  
>  	if (!persistent_swap_storage) {
>  		swap_storage = shmem_file_setup("ttm swap",

This *is* actually a bug somewhere, since before ttm_tt_swapout,
ttm_bo_swapout should have moved out the bo to system and set
the correct caching. A WARN_ON_ONCE instead of BUG_ON is ok, but this
should never happen.

/Thomas




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

Re: [PATCH] drm/ttm: Set CPU caching mode to cached for BOs being swapped out

[Michel Dänzer]

On 05.11.2015 17:47, Thomas Hellstrom wrote:
> Hi, Michel,
> 
> On 11/05/2015 09:08 AM, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> I ran into the BUG_ON in ttm_tt_swapout, presumably the BO being swapped
>> out was using a write-combined CPU mapping.
>>
>> Instead of BUGging out, just set the caching mode to what's needed.
>>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>  drivers/gpu/drm/ttm/ttm_tt.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
>> index 4e19d0f..c2794eb 100644
>> --- a/drivers/gpu/drm/ttm/ttm_tt.c
>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
>> @@ -334,7 +334,8 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
>>  	int ret = -ENOMEM;
>>  
>>  	BUG_ON(ttm->state != tt_unbound && ttm->state != tt_unpopulated);
>> -	BUG_ON(ttm->caching_state != tt_cached);
>> +
>> +	ttm_tt_set_caching(ttm, tt_cached);
>>  
>>  	if (!persistent_swap_storage) {
>>  		swap_storage = shmem_file_setup("ttm swap",
> 
> This *is* actually a bug somewhere, since before ttm_tt_swapout,
> ttm_bo_swapout should have moved out the bo to system and set
> the correct caching.

Maybe ttm_bo_swapout needs to check ttm->caching_state explicitly?
AFAICT it only checks the placement flags, but we allow all caching
modes for GTT.


-- 
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: Set CPU caching mode to cached for BOs being swapped out

[Thomas Hellstrom]

On 11/06/2015 03:33 AM, Michel Dänzer wrote:
> On 05.11.2015 17:47, Thomas Hellstrom wrote:
>> Hi, Michel,
>>
>> On 11/05/2015 09:08 AM, Michel Dänzer wrote:
>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>
>>> I ran into the BUG_ON in ttm_tt_swapout, presumably the BO being swapped
>>> out was using a write-combined CPU mapping.
>>>
>>> Instead of BUGging out, just set the caching mode to what's needed.
>>>
>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>> ---
>>>  drivers/gpu/drm/ttm/ttm_tt.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
>>> index 4e19d0f..c2794eb 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
>>> @@ -334,7 +334,8 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
>>>  	int ret = -ENOMEM;
>>>  
>>>  	BUG_ON(ttm->state != tt_unbound && ttm->state != tt_unpopulated);
>>> -	BUG_ON(ttm->caching_state != tt_cached);
>>> +
>>> +	ttm_tt_set_caching(ttm, tt_cached);
>>>  
>>>  	if (!persistent_swap_storage) {
>>>  		swap_storage = shmem_file_setup("ttm swap",
>> This *is* actually a bug somewhere, since before ttm_tt_swapout,
>> ttm_bo_swapout should have moved out the bo to system and set
>> the correct caching.
> Maybe ttm_bo_swapout needs to check ttm->caching_state explicitly?
> AFAICT it only checks the placement flags, but we allow all caching
> modes for GTT.
>

bo->mem.placement should only have two flags set, one for the current
memory type corresponding to bo->mem.mem_type, the other for the current
caching. If there are more than one caching flag set at this point,
that's a bug. The preferred caching mode is chosen from a set of modes
in ttm_bo_select_caching(), and bo->mem is reassigned to incorporate the
current caching mode after a successful move.

I quickly eyed through the TTM default move functions and they seem to
do the correct thing from what I can tell. Is the radeon ttm driver
assigning bo->mem in a move callback perhaps?

/Thomas


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

Re: [PATCH] drm/ttm: Set CPU caching mode to cached for BOs being swapped out

[Michel Dänzer]

On 06.11.2015 22:02, Thomas Hellstrom wrote:
> On 11/06/2015 03:33 AM, Michel Dänzer wrote:
>> On 05.11.2015 17:47, Thomas Hellstrom wrote:
>>> Hi, Michel,
>>>
>>> On 11/05/2015 09:08 AM, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>> I ran into the BUG_ON in ttm_tt_swapout, presumably the BO being swapped
>>>> out was using a write-combined CPU mapping.
>>>>
>>>> Instead of BUGging out, just set the caching mode to what's needed.
>>>>
>>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>> ---
>>>>  drivers/gpu/drm/ttm/ttm_tt.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
>>>> index 4e19d0f..c2794eb 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
>>>> @@ -334,7 +334,8 @@ int ttm_tt_swapout(struct ttm_tt *ttm, struct file *persistent_swap_storage)
>>>>  	int ret = -ENOMEM;
>>>>  
>>>>  	BUG_ON(ttm->state != tt_unbound && ttm->state != tt_unpopulated);
>>>> -	BUG_ON(ttm->caching_state != tt_cached);
>>>> +
>>>> +	ttm_tt_set_caching(ttm, tt_cached);
>>>>  
>>>>  	if (!persistent_swap_storage) {
>>>>  		swap_storage = shmem_file_setup("ttm swap",
>>> This *is* actually a bug somewhere, since before ttm_tt_swapout,
>>> ttm_bo_swapout should have moved out the bo to system and set
>>> the correct caching.
>> Maybe ttm_bo_swapout needs to check ttm->caching_state explicitly?
>> AFAICT it only checks the placement flags, but we allow all caching
>> modes for GTT.
>>
> 
> bo->mem.placement should only have two flags set, one for the current
> memory type corresponding to bo->mem.mem_type, the other for the current
> caching. If there are more than one caching flag set at this point,
> that's a bug. The preferred caching mode is chosen from a set of modes
> in ttm_bo_select_caching(), and bo->mem is reassigned to incorporate the
> current caching mode after a successful move.
> 
> I quickly eyed through the TTM default move functions and they seem to
> do the correct thing from what I can tell. Is the radeon ttm driver
> assigning bo->mem in a move callback perhaps?

Only via ttm_bo_move_ttm / to the values passed into the move callback
AFAICT.


-- 
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