[PATCH] drm/ttm: Only try to preserve caching in moves in the same space
Currently TTM tries to preserve the previous caching even for moves between unrelated memory spaces. This results for instance in moves from VRAM to PCIE GART changing system memory to WC state needlessly. This patch changes TTM to only try to preserve caching if moving from system/TT to system/TT. In theory, we should also do that if moving between two device memory spaces that are backend by the same physical memory area. However, I'm not sure how to do that in the current TTM framework and I suspect no card/driver uses memory spaces in that way. Signed-off-by: Luca Barbieri --- drivers/gpu/drm/ttm/ttm_bo.c | 18 ++ 1 files changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 2920f9a..27ee1be 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -876,6 +876,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, mem->mm_node = NULL; for (i = 0; i < placement->num_placement; ++i) { + int is_tt_move; ret = ttm_mem_type_from_flags(placement->placement[i], &mem_type); if (ret) @@ -891,8 +892,12 @@ 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); + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && + !(bdev->man[bo->mem.mem_type].flags & TTM_MEMTYPE_FLAG_FIXED); + + /* Only try to keep the current flags if we are in the same memory space */ + cur_flags = ttm_bo_select_caching(man, is_tt_move ? bo->mem.placement : 0, cur_flags); + /* * Use the access and other non-mapping-related flag bits from * the memory placement flags to the current flags @@ -927,6 +932,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, return -EINVAL; for (i = 0; i < placement->num_busy_placement; ++i) { + int is_tt_move; ret = ttm_mem_type_from_flags(placement->busy_placement[i], &mem_type); if (ret) @@ -941,8 +947,12 @@ 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); + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && + !(bdev->man[bo->mem.mem_type].flags & TTM_MEMTYPE_FLAG_FIXED); + + /* Only try to keep the current flags if we are in the same memory space */ + cur_flags = ttm_bo_select_caching(man, is_tt_move ? bo->mem.placement : 0, cur_flags); + /* * Use the access and other non-mapping-related flag bits from * the memory placement flags to the current flags -- 1.6.6.1.476.g01ddb -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
On Thu, Jan 28, 2010 at 7:26 PM, Luca Barbieri wrote: > Currently TTM tries to preserve the previous caching even for moves > between unrelated memory spaces. > > This results for instance in moves from VRAM to PCIE GART changing > system memory to WC state needlessly. > > This patch changes TTM to only try to preserve caching if moving from > system/TT to system/TT. > > In theory, we should also do that if moving between two device memory > spaces that are backend by the same physical memory area. > However, I'm not sure how to do that in the current TTM framework and > I suspect no card/driver uses memory spaces in that way. Thomas or Jerome (since I think placement changes were you), any chance of ack or review? Dave. > > Signed-off-by: Luca Barbieri > --- > drivers/gpu/drm/ttm/ttm_bo.c | 18 ++ > 1 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 2920f9a..27ee1be 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -876,6 +876,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > > mem->mm_node = NULL; > for (i = 0; i < placement->num_placement; ++i) { > + int is_tt_move; > ret = ttm_mem_type_from_flags(placement->placement[i], > &mem_type); > if (ret) > @@ -891,8 +892,12 @@ 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); > + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && > + !(bdev->man[bo->mem.mem_type].flags & > TTM_MEMTYPE_FLAG_FIXED); > + > + /* Only try to keep the current flags if we are in the same > memory space */ > + cur_flags = ttm_bo_select_caching(man, is_tt_move ? > bo->mem.placement : 0, cur_flags); > + > /* > * Use the access and other non-mapping-related flag bits from > * the memory placement flags to the current flags > @@ -927,6 +932,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > return -EINVAL; > > for (i = 0; i < placement->num_busy_placement; ++i) { > + int is_tt_move; > ret = ttm_mem_type_from_flags(placement->busy_placement[i], > &mem_type); > if (ret) > @@ -941,8 +947,12 @@ 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); > + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && > + !(bdev->man[bo->mem.mem_type].flags & > TTM_MEMTYPE_FLAG_FIXED); > + > + /* Only try to keep the current flags if we are in the same > memory space */ > + cur_flags = ttm_bo_select_caching(man, is_tt_move ? > bo->mem.placement : 0, cur_flags); > + > /* > * Use the access and other non-mapping-related flag bits from > * the memory placement flags to the current flags > -- > 1.6.6.1.476.g01ddb > > > -- > The Planet: dedicated and managed hosting, cloud storage, colocation > Stay online with enterprise data centers and the best network in the business > Choose flexible plans and management services without long-term contracts > Personal 24x7 support from experience hosting pros just a phone call away. > http://p.sf.net/sfu/theplanet-com > -- > ___ > Dri-devel mailing list > Dri-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/dri-devel > -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
On Mon, Feb 01, 2010 at 11:35:21AM +1000, Dave Airlie wrote: > On Thu, Jan 28, 2010 at 7:26 PM, Luca Barbieri wrote: > > Currently TTM tries to preserve the previous caching even for moves > > between unrelated memory spaces. > > > > This results for instance in moves from VRAM to PCIE GART changing > > system memory to WC state needlessly. > > > > This patch changes TTM to only try to preserve caching if moving from > > system/TT to system/TT. > > > > In theory, we should also do that if moving between two device memory > > spaces that are backend by the same physical memory area. > > However, I'm not sure how to do that in the current TTM framework and > > I suspect no card/driver uses memory spaces in that way. > > Thomas or Jerome (since I think placement changes were you), any > chance of ack or review? > > Dave. > I think i will NACK, the idea to avoid switching to WC is good but i think it can be achieve with current code by using cleverly the infrastructure. What happen is that WC is prefered over cached in ttm_tt_set_placement_caching so to avoid WC taking over cached all you have to do is : At memory type init you init default caching as : SYSTEM.default_caching = TTM_PL_MASK_CACHING if PCI|PCIE TT.default_caching = TTM_PL_MASK_CACHED else TT.default_caching = TTM_PL_MASK_UNCACHED VRAM.default_caching = TTM_PL_FLAG_WC Now when ever you move a buffer for the proposed placement the flag you supply for caching are the default_caching of the memory type you want to use. So on VRAM->TT (PCIE case) ttm_bo_select_caching get call with : cur_placement = TTM_PL_FLAG_WC & proposed_placement = TTM_PL_MASK_CACHED the first if will fail but the second one will pass and so we get: result = TTM_PL_MASK_CACHED AGP is special case for the driver, instead of using the default_cahing from the memory type you are targetting the driver should always ask for uncached on TT->SYSTEM and VRAM->TT, and WC on SYSTEM|TT->VRAM. To avoid having if/else you can simply change SYSTEM.default_caching to UNCACHED for AGP and as before always use default_caching flag of the type you are moving to in the proposed placement. This way the only if/else is in driver's ttm initialization. Radeon is more or less (well more less than more ;)) doing that, i gonna test with tweaking the default_caching stuff and see how it performs. Btw maybe we can add a debugfs files to monitor how much page cache flag transition we are doing. Cheers, Jerome > > > > Signed-off-by: Luca Barbieri > > --- > > drivers/gpu/drm/ttm/ttm_bo.c | 18 ++ > > 1 files changed, 14 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > > index 2920f9a..27ee1be 100644 > > --- a/drivers/gpu/drm/ttm/ttm_bo.c > > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > > @@ -876,6 +876,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > > > > mem->mm_node = NULL; > > for (i = 0; i < placement->num_placement; ++i) { > > + int is_tt_move; > > ret = ttm_mem_type_from_flags(placement->placement[i], > > &mem_type); > > if (ret) > > @@ -891,8 +892,12 @@ 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); > > + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && > > + !(bdev->man[bo->mem.mem_type].flags & > > TTM_MEMTYPE_FLAG_FIXED); > > + > > + /* Only try to keep the current flags if we are in the same > > memory space */ > > + cur_flags = ttm_bo_select_caching(man, is_tt_move ? > > bo->mem.placement : 0, cur_flags); > > + > > /* > > * Use the access and other non-mapping-related flag bits > > from > > * the memory placement flags to the current flags > > @@ -927,6 +932,7 @@ int ttm_bo_mem_space(struct ttm_buffer_object *bo, > > return -EINVAL; > > > > for (i = 0; i < placement->num_busy_placement; ++i) { > > + int is_tt_move; > > ret = ttm_mem_type_from_flags(placement->busy_placement[i], > > &mem_type); > > if (ret) > > @@ -941,8 +947,12 @@ 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); > > + is_tt_move = !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && > > + !(bdev->man[bo->mem.mem_type].flags & > > TTM_MEMTYPE_FLAG_FIXED); > > + > > + /* Only try to keep the curre
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
Jerome Glisse wrote: > On Mon, Feb 01, 2010 at 11:35:21AM +1000, Dave Airlie wrote: > >> On Thu, Jan 28, 2010 at 7:26 PM, Luca Barbieri >> wrote: >> >>> Currently TTM tries to preserve the previous caching even for moves >>> between unrelated memory spaces. >>> >>> This results for instance in moves from VRAM to PCIE GART changing >>> system memory to WC state needlessly. >>> >>> This patch changes TTM to only try to preserve caching if moving from >>> system/TT to system/TT. >>> >>> In theory, we should also do that if moving between two device memory >>> spaces that are backend by the same physical memory area. >>> However, I'm not sure how to do that in the current TTM framework and >>> I suspect no card/driver uses memory spaces in that way. >>> >> Thomas or Jerome (since I think placement changes were you), any >> chance of ack or review? >> >> Dave. >> >> > > I think i will NACK, the idea to avoid switching to WC is good but i think it > can be achieve with current code by using cleverly the infrastructure. > I agree with Jerome. It *should* be possible to have the driver control this sort of thing depending on what triggered the validate / move call. Thanks, Thomas -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
> If you set to uncached | cached, VRAM->SYSTEM will still be broken > because it will choose uncached. Assuming VRAM is uncached and not WC of course. The reasoning still holds if you replace all instances of "uncached" with WC. -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
I see a problem with this. If you have uncached TT (e.g. AGP) you want to get uncached for TT->SYSTEM, but you want cached for VRAM->SYSTEM. If you set SYSTEM to uncached, then VRAM->SYSTEM will broken, and if you set SYSTEM to cached, TT->SYSTEM will be broken. If you set to uncached | cached, VRAM->SYSTEM will still be broken because it will choose uncached. You would need to make the proposed placement conditional on the current BO memory type, which is possible but somewhat defeats the purpose of having all the ttm_bo_mem_space logic in ttm. Perhaps this doesn't usually happen because you would do VRAM->TT instead of SYSTEM (is that really always the case? what if we don't have enough space in GART?), but it seems a design deficiency in TTM anyway. The current placement caching should not really be a factor *unless* we can actually do the move just by changing the caching attributes. What do you think? Of course we could also just drop ttm_bo_mem_space and let the driver do that itself (perhaps using hardcorded helpers for the AGP and PCIE case). It seems quite a radical solution though. -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
> Idea is to mask all move which involve TT (AGP) with the agp_caching_mask, > it's easy to do in radeon ttm layer code and i believe it's easy to do > in nouveau > too. On non AGP you set agp_caching_mask to UC|WC|CACHED. Sure, but isn't that uglier and much more ad-hoc that the patch I proposed? -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
> Your patch remove the consistency of caching attribute and make move btw > non fixed area different than others move, while driver can already achieve > so. It is already different, because TTM does it by changing the page attributes, as opposed to copying data. Thus, it is useful to preserve the existing caching since that means we aren't going to do any work for the move. If instead the move is actually performed by copying (either memcpy or unaccelerated) we don't care about the current caching. The code should actually be: cur_flags = ttm_bo_select_caching(man, ttm_is_going_to_move_by_setting_page_caching_flags ? bo->mem.placement : 0, cur_flags); I wrote ttm_is_going_to_move_by_setting_page_caching_flags as !(man->flags & TTM_MEMTYPE_FLAG_FIXED) && !(bdev->man[bo->mem.mem_type].flags & TTM_MEMTYPE_FLAG_FIXED); If there is a better way to express that in TTM, we should use that instead. This would fix all cases without requiring odd hacks in the drivers. -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
On Mon, Feb 1, 2010 at 5:26 PM, Luca Barbieri wrote: > I see a problem with this. > > If you have uncached TT (e.g. AGP) you want to get uncached for > TT->SYSTEM, but you want cached for VRAM->SYSTEM. > If you set SYSTEM to uncached, then VRAM->SYSTEM will broken, and if > you set SYSTEM to cached, TT->SYSTEM will be broken. > If you set to uncached | cached, VRAM->SYSTEM will still be broken > because it will choose uncached. > > You would need to make the proposed placement conditional on the > current BO memory type, which is possible but somewhat defeats the > purpose of having all the ttm_bo_mem_space logic in ttm. > > Perhaps this doesn't usually happen because you would do VRAM->TT > instead of SYSTEM (is that really always the case? what if we don't > have enough space in GART?), but it seems a design deficiency in TTM > anyway. > The current placement caching should not really be a factor *unless* > we can actually do the move just by changing the caching attributes. > > What do you think? > > Of course we could also just drop ttm_bo_mem_space and let the driver > do that itself (perhaps using hardcorded helpers for the AGP and PCIE > case). It seems quite a radical solution though. > I assumed that VRAM<->SYSTEM always did go through TT (which is the case for radeon and i think for nouveau too, haven't checked :)). Assuming we have a system which can do VRAM<->SYSTEM directly and which is AGP, what you can do is : agp_caching_mask=UC and each time you do ask for a bo move you & the caching flags with agp_caching_mask, then you do SYSTEM.default_caching=CACHED You endup : VRAM->SYSTEM proposed_flags = SYSTEM.default_caching which is picking cached: TT->SYSTEM proposed_flags = SYSTEM.default_caching & agp_caching_mask so you endup keeping uncached VRAM->TT proposed_flags = TT.default_caching(=UC) & agp_caching_mask Idea is to mask all move which involve TT (AGP) with the agp_caching_mask, it's easy to do in radeon ttm layer code and i believe it's easy to do in nouveau too. On non AGP you set agp_caching_mask to UC|WC|CACHED. Anyway i think here the issue is more in ttm_tt_set_placement_caching which favor WC over UC and UC over cached, Thomas maybe you can enlight us on this ? If we change it to choose in this order : CACHED,WC,UC i think it would be easier (well it's not that hard anyway ;)) for the driver to tweak it's caching flags. Cheers, Jerome -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
On Mon, Feb 1, 2010 at 6:46 PM, Luca Barbieri wrote: >> Idea is to mask all move which involve TT (AGP) with the agp_caching_mask, >> it's easy to do in radeon ttm layer code and i believe it's easy to do >> in nouveau >> too. On non AGP you set agp_caching_mask to UC|WC|CACHED. > > Sure, but isn't that uglier and much more ad-hoc that the patch I proposed? > Your patch remove the consistency of caching attribute and make move btw non fixed area different than others move, while driver can already achieve so. As i said others change in ttm which wouldn't change consistency can simplify the problem from the driver POV. I think it's better to avoid tweaking core ttm to fix issue your driver can fix by using already existing functionalities. That's my feeling. Cheers, Jerome -- The Planet: dedicated and managed hosting, cloud storage, colocation Stay online with enterprise data centers and the best network in the business Choose flexible plans and management services without long-term contracts Personal 24x7 support from experience hosting pros just a phone call away. http://p.sf.net/sfu/theplanet-com -- ___ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel