ongoing writecombine on ppc

Commit Message

Dave Airlie Oct. 2, 2015, 4:42 a.m. UTC

I don't think we resolved this the last time we talked about it,

but radeon writecombine maps fail hard on ppc, I think all the fixes
either did something bad to AGP systems or weren't liked.

My patch attached just fixes radeon, which is where I'm still seeing the issue.

Dave.

Comments

Benjamin Herrenschmidt Oct. 2, 2015, 4:45 a.m. UTC | #1

On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
> I don't think we resolved this the last time we talked about it,
> 
> but radeon writecombine maps fail hard on ppc, I think all the fixes
> either did something bad to AGP systems or weren't liked.
> 
> My patch attached just fixes radeon, which is where I'm still seeing
> the issue.

Yes, you MUST NOT set the flags of system memory to anything other than
fully cachable on any cache coherent powerpc machine. This should be
bolted into the DRM core imho.

_wc is only suitable for MMIO.

Cheers,
Ben.

Benjamin Herrenschmidt Oct. 2, 2015, 4:47 a.m. UTC | #2

On Fri, 2015-10-02 at 14:45 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
> > I don't think we resolved this the last time we talked about it,
> > 
> > but radeon writecombine maps fail hard on ppc, I think all the
> > fixes
> > either did something bad to AGP systems or weren't liked.
> > 
> > My patch attached just fixes radeon, which is where I'm still
> > seeing
> > the issue.
> 
> Yes, you MUST NOT set the flags of system memory to anything other
> than
> fully cachable on any cache coherent powerpc machine. This should be
> bolted into the DRM core imho.
> 
> _wc is only suitable for MMIO.

Note that I wouldn't be surprised if we weren't the only arch like
that.

Playing with caching attributes of main memory smells from a system
design and cache coherency protocol standpoint. x86 supports it but
I wouldn't be surprised if some ARMs puke in interesting way as well.

Cheers,
Ben.

Benjamin Herrenschmidt Oct. 2, 2015, 4:50 a.m. UTC | #3

On Fri, 2015-10-02 at 14:47 +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2015-10-02 at 14:45 +1000, Benjamin Herrenschmidt wrote:
> > On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
> > > I don't think we resolved this the last time we talked about it,
> > > 
> > > but radeon writecombine maps fail hard on ppc, I think all the
> > > fixes
> > > either did something bad to AGP systems or weren't liked.
> > > 
> > > My patch attached just fixes radeon, which is where I'm still
> > > seeing
> > > the issue.
> > 
> > Yes, you MUST NOT set the flags of system memory to anything other
> > than
> > fully cachable on any cache coherent powerpc machine. This should
> > be
> > bolted into the DRM core imho.
> > 
> > _wc is only suitable for MMIO.
> 
> Note that I wouldn't be surprised if we weren't the only arch like
> that.
> 
> Playing with caching attributes of main memory smells from a system
> design and cache coherency protocol standpoint. x86 supports it but
> I wouldn't be surprised if some ARMs puke in interesting way as well.

Similarily I remember something in the TTM core at some point that
was trying to use the same caching attribtues for memory as the
original (MMIO) object when moving it.

That will blow up on POWER and possibly others. Here too, it's a
complete heresy for the core to assume that it can apply MMIO
attributes to system memory. I don't know if that still happens
though.

Cheers,
Ben.

Dave Airlie Oct. 2, 2015, 4:53 a.m. UTC | #4

On 2 October 2015 at 14:45, Benjamin Herrenschmidt
<[e-mail]> wrote:
> On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
>> I don't think we resolved this the last time we talked about it,
>>
>> but radeon writecombine maps fail hard on ppc, I think all the fixes
>> either did something bad to AGP systems or weren't liked.
>>
>> My patch attached just fixes radeon, which is where I'm still seeing
>> the issue.
>
> Yes, you MUST NOT set the flags of system memory to anything other than
> fully cachable on any cache coherent powerpc machine. This should be
> bolted into the DRM core imho.
>

Except you guys screwed up and allowed AGP to exist on Power, and that
requires it.

So we have to keep the AGP trapdoor in place, and at the moment only
the drivers know
if they are AGP, hence why I had to add this in the driver instead of
in the drm core.

Dave.

Benjamin Herrenschmidt Oct. 2, 2015, 4:56 a.m. UTC | #5

On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote:
> On 2 October 2015 at 14:45, Benjamin Herrenschmidt
> <[e-mail]> wrote:
> > On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
> > > I don't think we resolved this the last time we talked about it,
> > > 
> > > but radeon writecombine maps fail hard on ppc, I think all the
> > > fixes
> > > either did something bad to AGP systems or weren't liked.
> > > 
> > > My patch attached just fixes radeon, which is where I'm still
> > > seeing
> > > the issue.
> > 
> > Yes, you MUST NOT set the flags of system memory to anything other
> > than
> > fully cachable on any cache coherent powerpc machine. This should
> > be
> > bolted into the DRM core imho.
> > 
> 
> Except you guys screwed up and allowed AGP to exist on Power, and 
> that requires it.

Well, Apple did :-) Yes, AGP should never have existed in the first
place, I think that's a given :)

> So we have to keep the AGP trapdoor in place, and at the moment only
> the drivers know if they are AGP, hence why I had to add this in the 
> driver instead of in the drm core.
Cheers,
Ben.

Oded Gabbay Jan. 21, 2016, 3:24 p.m. UTC | #6

On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt
<[e-mail]> wrote:
> On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote:
>> On 2 October 2015 at 14:45, Benjamin Herrenschmidt
>> <[e-mail]> wrote:
>> > On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
>> > > I don't think we resolved this the last time we talked about it,
>> > >
>> > > but radeon writecombine maps fail hard on ppc, I think all the
>> > > fixes
>> > > either did something bad to AGP systems or weren't liked.
>> > >
>> > > My patch attached just fixes radeon, which is where I'm still
>> > > seeing
>> > > the issue.
>> >
>> > Yes, you MUST NOT set the flags of system memory to anything other
>> > than
>> > fully cachable on any cache coherent powerpc machine. This should
>> > be
>> > bolted into the DRM core imho.
>> >
>>
>> Except you guys screwed up and allowed AGP to exist on Power, and
>> that requires it.
>
> Well, Apple did :-) Yes, AGP should never have existed in the first
> place, I think that's a given :)
>
>> So we have to keep the AGP trapdoor in place, and at the moment only
>> the drivers know if they are AGP, hence why I had to add this in the
>> driver instead of in the drm core.
> Cheers,
> Ben.
>
>
>
Bumping this and adding my r-b:
Reviewed-by: Oded Gabbay <[e-mail]>

Oded Gabbay Jan. 21, 2016, 3:39 p.m. UTC | #7

+Alex

On Thu, Jan 21, 2016 at 5:24 PM, Oded Gabbay <[e-mail]> wrote:
> On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt
> <[e-mail]> wrote:
>> On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote:
>>> On 2 October 2015 at 14:45, Benjamin Herrenschmidt
>>> <[e-mail]> wrote:
>>> > On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
>>> > > I don't think we resolved this the last time we talked about it,
>>> > >
>>> > > but radeon writecombine maps fail hard on ppc, I think all the
>>> > > fixes
>>> > > either did something bad to AGP systems or weren't liked.
>>> > >
>>> > > My patch attached just fixes radeon, which is where I'm still
>>> > > seeing
>>> > > the issue.
>>> >
>>> > Yes, you MUST NOT set the flags of system memory to anything other
>>> > than
>>> > fully cachable on any cache coherent powerpc machine. This should
>>> > be
>>> > bolted into the DRM core imho.
>>> >
>>>
>>> Except you guys screwed up and allowed AGP to exist on Power, and
>>> that requires it.
>>
>> Well, Apple did :-) Yes, AGP should never have existed in the first
>> place, I think that's a given :)
>>
>>> So we have to keep the AGP trapdoor in place, and at the moment only
>>> the drivers know if they are AGP, hence why I had to add this in the
>>> driver instead of in the drm core.
>> Cheers,
>> Ben.
>>
>>
>>
> Bumping this and adding my r-b:
> Reviewed-by: Oded Gabbay <[e-mail]>

Alex Deucher Jan. 21, 2016, 5:10 p.m. UTC | #8

On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <[e-mail]> wrote:
> +Alex

No objections from me.  Care to respin with amdgpu support and signed
off?  Would probably also be nice to split the core drm change from
the driver specific ones.

Alex

>
> On Thu, Jan 21, 2016 at 5:24 PM, Oded Gabbay <[e-mail]> wrote:
>> On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt
>> <[e-mail]> wrote:
>>> On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote:
>>>> On 2 October 2015 at 14:45, Benjamin Herrenschmidt
>>>> <[e-mail]> wrote:
>>>> > On Fri, 2015-10-02 at 14:42 +1000, Dave Airlie wrote:
>>>> > > I don't think we resolved this the last time we talked about it,
>>>> > >
>>>> > > but radeon writecombine maps fail hard on ppc, I think all the
>>>> > > fixes
>>>> > > either did something bad to AGP systems or weren't liked.
>>>> > >
>>>> > > My patch attached just fixes radeon, which is where I'm still
>>>> > > seeing
>>>> > > the issue.
>>>> >
>>>> > Yes, you MUST NOT set the flags of system memory to anything other
>>>> > than
>>>> > fully cachable on any cache coherent powerpc machine. This should
>>>> > be
>>>> > bolted into the DRM core imho.
>>>> >
>>>>
>>>> Except you guys screwed up and allowed AGP to exist on Power, and
>>>> that requires it.
>>>
>>> Well, Apple did :-) Yes, AGP should never have existed in the first
>>> place, I think that's a given :)
>>>
>>>> So we have to keep the AGP trapdoor in place, and at the moment only
>>>> the drivers know if they are AGP, hence why I had to add this in the
>>>> driver instead of in the drm core.
>>> Cheers,
>>> Ben.
>>>
>>>
>>>
>> Bumping this and adding my r-b:
>> Reviewed-by: Oded Gabbay <[e-mail]>

Michel Dänzer Jan. 22, 2016, 2:32 a.m. UTC | #9

On 22.01.2016 02:10, Alex Deucher wrote:
> On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <[e-mail]> wrote:
>> +Alex
> 
> No objections from me.  Care to respin with amdgpu support and signed
> off?  Would probably also be nice to split the core drm change from
> the driver specific ones.

Also, it would be better to mask out the RADEON_GEM_GTT_WC flag in
radeon_bo_create, as is already done for a few other cases.

Oded Gabbay Jan. 28, 2016, 2:30 p.m. UTC | #10

On Fri, Jan 22, 2016 at 4:32 AM, Michel Dänzer <[e-mail]> wrote:
> On 22.01.2016 02:10, Alex Deucher wrote:
>> On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <[e-mail]> wrote:
>>> +Alex
>>
>> No objections from me.  Care to respin with amdgpu support and signed
>> off?  Would probably also be nice to split the core drm change from
>> the driver specific ones.
>
> Also, it would be better to mask out the RADEON_GEM_GTT_WC flag in
> radeon_bo_create, as is already done for a few other cases.
>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer

ok, sent the patches now. Please check if I got all the flags correctly.

Oded

Patch

From 67d00d2fcb756a3d2b6061d0831242138889a81f Mon Sep 17 00:00:00 2001
From: Dave Airlie <[e-mail]>
Date: Fri, 2 Oct 2015 12:22:08 +1000
Subject: [PATCH] radeon: don't attempt WC mappings on powerpc

---
 drivers/gpu/drm/radeon/radeon_object.c | 16 ++++++++++++----
 include/drm/drm_cache.h                |  9 +++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c
index d302488..4acda0e 100644
--- a/drivers/gpu/drm/radeon/radeon_object.c
+++ b/drivers/gpu/drm/radeon/radeon_object.c
@@ -33,6 +33,7 @@ 
 #include <linux/slab.h>
 #include <drm/drmP.h>
 #include <drm/radeon_drm.h>
+#include <drm/drm_cache.h>
 #include "radeon.h"
 #include "radeon_trace.h"
 
@@ -92,6 +93,15 @@  bool radeon_ttm_bo_is_radeon_bo(struct ttm_buffer_object *bo)
 	return false;
 }
 
+static inline bool radeon_placement_can_wc(struct radeon_bo *rbo)
+{
+	if ((rbo->flags & RADEON_GEM_GTT_WC) && drm_arch_can_wc_memory())
+		return true;
+	if (rbo->rdev->flags & RADEON_IS_AGP)
+		return true;
+	return false;
+}
+
 void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
 {
 	u32 c = 0, i;
@@ -123,8 +133,7 @@  void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
 			rbo->placements[c++].flags = TTM_PL_FLAG_UNCACHED |
 				TTM_PL_FLAG_TT;
 
-		} else if ((rbo->flags & RADEON_GEM_GTT_WC) ||
-			   (rbo->rdev->flags & RADEON_IS_AGP)) {
+		} else if (radeon_placement_can_wc(rbo)) {
 			rbo->placements[c].fpfn = 0;
 			rbo->placements[c++].flags = TTM_PL_FLAG_WC |
 				TTM_PL_FLAG_UNCACHED |
@@ -142,8 +151,7 @@  void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain)
 			rbo->placements[c++].flags = TTM_PL_FLAG_UNCACHED |
 				TTM_PL_FLAG_SYSTEM;
 
-		} else if ((rbo->flags & RADEON_GEM_GTT_WC) ||
-		    rbo->rdev->flags & RADEON_IS_AGP) {
+		} else if (radeon_placement_can_wc(rbo)) {
 			rbo->placements[c].fpfn = 0;
 			rbo->placements[c++].flags = TTM_PL_FLAG_WC |
 				TTM_PL_FLAG_UNCACHED |
diff --git a/include/drm/drm_cache.h b/include/drm/drm_cache.h
index 7bfb063..461a055 100644
--- a/include/drm/drm_cache.h
+++ b/include/drm/drm_cache.h
@@ -35,4 +35,13 @@ 
 
 void drm_clflush_pages(struct page *pages[], unsigned long num_pages);
 
+static inline bool drm_arch_can_wc_memory(void)
+{
+#if defined(CONFIG_PPC) && !defined(CONFIG_NOT_COHERENT_CACHE)
+	return false;
+#else
+	return true;
+#endif
+}
+
 #endif
-- 
2.4.3