Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space

[Jerome Glisse <glisse@freedesktop.org>]

On Mon, Feb 01, 2010 at 11:35:21AM +1000, Dave Airlie wrote:
> On Thu, Jan 28, 2010 at 7:26 PM, Luca Barbieri <luca@luca-barbieri.com> 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 <luca@luca-barbieri.com>
> > ---
> >  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
> 

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