All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
@ 2010-01-28  9:26 Luca Barbieri
  2010-02-01  1:35 ` Dave Airlie
  0 siblings, 1 reply; 10+ messages in thread
From: Luca Barbieri @ 2010-01-28  9:26 UTC (permalink / raw)
  To: thellstrom; +Cc: airlied, Luca Barbieri, dri-devel

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-01-28  9:26 [PATCH] drm/ttm: Only try to preserve caching in moves in the same space Luca Barbieri
@ 2010-02-01  1:35 ` Dave Airlie
  2010-02-01 13:33   ` Jerome Glisse
  0 siblings, 1 reply; 10+ messages in thread
From: Dave Airlie @ 2010-02-01  1:35 UTC (permalink / raw)
  To: Luca Barbieri; +Cc: airlied, Jerome Glisse, thellstrom, dri-devel

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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01  1:35 ` Dave Airlie
@ 2010-02-01 13:33   ` Jerome Glisse
  2010-02-01 16:18     ` Thomas Hellstrom
  2010-02-01 16:26     ` Luca Barbieri
  0 siblings, 2 replies; 10+ messages in thread
From: Jerome Glisse @ 2010-02-01 13:33 UTC (permalink / raw)
  To: Dave Airlie; +Cc: airlied, Jerome Glisse, Luca Barbieri, dri-devel, thellstrom

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 13:33   ` Jerome Glisse
@ 2010-02-01 16:18     ` Thomas Hellstrom
  2010-02-01 16:26     ` Luca Barbieri
  1 sibling, 0 replies; 10+ messages in thread
From: Thomas Hellstrom @ 2010-02-01 16:18 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: airlied, Jerome Glisse, Luca Barbieri, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 13:33   ` Jerome Glisse
  2010-02-01 16:18     ` Thomas Hellstrom
@ 2010-02-01 16:26     ` Luca Barbieri
  2010-02-01 16:28       ` Luca Barbieri
  2010-02-01 17:05       ` Jerome Glisse
  1 sibling, 2 replies; 10+ messages in thread
From: Luca Barbieri @ 2010-02-01 16:26 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: airlied, Jerome Glisse, thellstrom, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 16:26     ` Luca Barbieri
@ 2010-02-01 16:28       ` Luca Barbieri
  2010-02-01 17:05       ` Jerome Glisse
  1 sibling, 0 replies; 10+ messages in thread
From: Luca Barbieri @ 2010-02-01 16:28 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: airlied, Jerome Glisse, thellstrom, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 16:26     ` Luca Barbieri
  2010-02-01 16:28       ` Luca Barbieri
@ 2010-02-01 17:05       ` Jerome Glisse
  2010-02-01 17:46         ` Luca Barbieri
  1 sibling, 1 reply; 10+ messages in thread
From: Jerome Glisse @ 2010-02-01 17:05 UTC (permalink / raw)
  To: Luca Barbieri; +Cc: airlied, thellstrom, dri-devel

On Mon, Feb 1, 2010 at 5:26 PM, Luca Barbieri <luca@luca-barbieri.com> 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
--

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 17:05       ` Jerome Glisse
@ 2010-02-01 17:46         ` Luca Barbieri
  2010-02-01 18:05           ` Jerome Glisse
  0 siblings, 1 reply; 10+ messages in thread
From: Luca Barbieri @ 2010-02-01 17:46 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: airlied, thellstrom, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 17:46         ` Luca Barbieri
@ 2010-02-01 18:05           ` Jerome Glisse
  2010-02-01 18:21             ` Luca Barbieri
  0 siblings, 1 reply; 10+ messages in thread
From: Jerome Glisse @ 2010-02-01 18:05 UTC (permalink / raw)
  To: Luca Barbieri; +Cc: airlied, thellstrom, dri-devel

On Mon, Feb 1, 2010 at 6:46 PM, Luca Barbieri <luca@luca-barbieri.com> 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
--

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/ttm: Only try to preserve caching in moves in the same space
  2010-02-01 18:05           ` Jerome Glisse
@ 2010-02-01 18:21             ` Luca Barbieri
  0 siblings, 0 replies; 10+ messages in thread
From: Luca Barbieri @ 2010-02-01 18:21 UTC (permalink / raw)
  To: Jerome Glisse; +Cc: airlied, thellstrom, dri-devel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2010-02-01 18:21 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-28  9:26 [PATCH] drm/ttm: Only try to preserve caching in moves in the same space Luca Barbieri
2010-02-01  1:35 ` Dave Airlie
2010-02-01 13:33   ` Jerome Glisse
2010-02-01 16:18     ` Thomas Hellstrom
2010-02-01 16:26     ` Luca Barbieri
2010-02-01 16:28       ` Luca Barbieri
2010-02-01 17:05       ` Jerome Glisse
2010-02-01 17:46         ` Luca Barbieri
2010-02-01 18:05           ` Jerome Glisse
2010-02-01 18:21             ` Luca Barbieri

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.