| Message ID | 1446710938-13173-1-git-send-email-michel@daenzer.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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.
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",