Message ID | CAPM=9twpPBLBb6j1ee5_DDv5UqfTzCJh-63CHyDxRU4Xd0ny4g@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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.
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.
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.
On 2 October 2015 at 14:45, Benjamin Herrenschmidt <benh@kernel.crashing.org> 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.
On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote: > On 2 October 2015 at 14:45, Benjamin Herrenschmidt > <benh@kernel.crashing.org> 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.
On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote: >> On 2 October 2015 at 14:45, Benjamin Herrenschmidt >> <benh@kernel.crashing.org> 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 <oded.gabbay@gmail.com>
+Alex On Thu, Jan 21, 2016 at 5:24 PM, Oded Gabbay <oded.gabbay@gmail.com> wrote: > On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt > <benh@kernel.crashing.org> wrote: >> On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote: >>> On 2 October 2015 at 14:45, Benjamin Herrenschmidt >>> <benh@kernel.crashing.org> 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 <oded.gabbay@gmail.com>
On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <oded.gabbay@gmail.com> 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 <oded.gabbay@gmail.com> wrote: >> On Fri, Oct 2, 2015 at 7:56 AM, Benjamin Herrenschmidt >> <benh@kernel.crashing.org> wrote: >>> On Fri, 2015-10-02 at 14:53 +1000, Dave Airlie wrote: >>>> On 2 October 2015 at 14:45, Benjamin Herrenschmidt >>>> <benh@kernel.crashing.org> 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 <oded.gabbay@gmail.com>
On 22.01.2016 02:10, Alex Deucher wrote: > On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <oded.gabbay@gmail.com> 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.
On Fri, Jan 22, 2016 at 4:32 AM, Michel Dänzer <michel@daenzer.net> wrote: > On 22.01.2016 02:10, Alex Deucher wrote: >> On Thu, Jan 21, 2016 at 10:39 AM, Oded Gabbay <oded.gabbay@gmail.com> 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
From 67d00d2fcb756a3d2b6061d0831242138889a81f Mon Sep 17 00:00:00 2001 From: Dave Airlie <airlied@redhat.com> 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