| Message ID | 1401992671-2548-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 05, 2014 at 11:24:28AM -0700, Jesse Barnes wrote: > Some machines (like MBAs) might use a tiled framebuffer but not enable > display swizzling at boot time. We want to preserve that configuration > if possible to prevent a boot time mode set. On IVB+ it shouldn't > affect performance anyway since the memory controller does internal > swizzling anyway. > > For most other configs we'll be able to enable swizzling at boot time, > since the initial framebuffer won't be tiled, thus we won't see any > corruption when we enable it. > > v2: preserve swizzling if BIOS had it set (Daniel) > v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel) > check display swizzle setting in detect_bit_6_swizzle (Daniel) > use gen6 as cutoff point (Daniel) > > Reported-by: Kristian Høgsberg <hoegsberg@gmail.com> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 3 +++ > drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++--------------- > drivers/gpu/drm/i915/intel_display.c | 3 +++ > 4 files changed, 28 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f57b752..f49fdcb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1405,6 +1405,7 @@ struct drm_i915_private { > struct intel_vbt_data vbt; > > bool bios_ssc; /* BIOS had SSC enabled at boot? */ > + bool preserve_bios_swizzle; > > /* overlay */ > struct intel_overlay *overlay; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index bfc7af0..0b168fb 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev) > dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE) > return; > > + if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle) > + return; > + Above two hunks shouldnt be needed if the setup in i915_gem_detect_bit_6_swizzle works correctly. > I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) | > DISP_TILE_SURFACE_SWIZZLING); > > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > index cb150e8..73005ad 100644 > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) > uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN; > uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN; > > - if (IS_VALLEYVIEW(dev)) { > - swizzle_x = I915_BIT_6_SWIZZLE_NONE; > - swizzle_y = I915_BIT_6_SWIZZLE_NONE; > - } else if (INTEL_INFO(dev)->gen >= 6) { > + if (INTEL_INFO(dev)->gen >= 6) { > uint32_t dimm_c0, dimm_c1; > - dimm_c0 = I915_READ(MAD_DIMM_C0); > - dimm_c1 = I915_READ(MAD_DIMM_C1); > - dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > - dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > - /* Enable swizzling when the channels are populated with > - * identically sized dimms. We don't need to check the 3rd > - * channel because no cpu with gpu attached ships in that > - * configuration. Also, swizzling only makes sense for 2 > - * channels anyway. */ > - if (dimm_c0 == dimm_c1) { > - swizzle_x = I915_BIT_6_SWIZZLE_9_10; > - swizzle_y = I915_BIT_6_SWIZZLE_9; > - } else { > + > + /* Make sure to honor boot time display configuration */ > + if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) { Not quite what I had in mind. Imo we need to check for whether any inherited fbs are tiled and if so also inherit the swizzle setting unconditionally, whether it is swizzled or unswizzled. See http://patchwork.freedesktop.org/patch/22204/ Cheers, Daniel > swizzle_x = I915_BIT_6_SWIZZLE_NONE; > swizzle_y = I915_BIT_6_SWIZZLE_NONE; > + } else { > + dimm_c0 = I915_READ(MAD_DIMM_C0); > + dimm_c1 = I915_READ(MAD_DIMM_C1); > + dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > + dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > + /* Enable swizzling when the channels are populated with > + * identically sized dimms. We don't need to check the > + * 3rd channel because no cpu with gpu attached ships > + * in that configuration. Also, swizzling only makes > + * sense for 2 channels anyway. */ > + if (dimm_c0 == dimm_c1) { > + swizzle_x = I915_BIT_6_SWIZZLE_9_10; > + swizzle_y = I915_BIT_6_SWIZZLE_9; > + } else { > + swizzle_x = I915_BIT_6_SWIZZLE_NONE; > + swizzle_y = I915_BIT_6_SWIZZLE_NONE; > + } > } > } else if (IS_GEN5(dev)) { > /* On Ironlake whatever DRAM config, GPU always do > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 0e8c9bc..7dbbef7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2361,6 +2361,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, > struct intel_plane_config *plane_config) > { > struct drm_device *dev = intel_crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *c; > struct intel_crtc *i; > struct intel_framebuffer *fb; > @@ -2389,6 +2390,8 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, > > fb = to_intel_framebuffer(c->primary->fb); > if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) { > + if (fb->obj->tiling_mode != I915_TILING_NONE) > + dev_priv->preserve_bios_swizzle = true; > drm_framebuffer_reference(c->primary->fb); > intel_crtc->base.primary->fb = c->primary->fb; > break; > -- > 1.8.3.2 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 10 Jun 2014 16:02:51 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Jun 05, 2014 at 11:24:28AM -0700, Jesse Barnes wrote: > > Some machines (like MBAs) might use a tiled framebuffer but not enable > > display swizzling at boot time. We want to preserve that configuration > > if possible to prevent a boot time mode set. On IVB+ it shouldn't > > affect performance anyway since the memory controller does internal > > swizzling anyway. > > > > For most other configs we'll be able to enable swizzling at boot time, > > since the initial framebuffer won't be tiled, thus we won't see any > > corruption when we enable it. > > > > v2: preserve swizzling if BIOS had it set (Daniel) > > v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel) > > check display swizzle setting in detect_bit_6_swizzle (Daniel) > > use gen6 as cutoff point (Daniel) > > > > Reported-by: Kristian Høgsberg <hoegsberg@gmail.com> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/i915_gem.c | 3 +++ > > drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++--------------- > > drivers/gpu/drm/i915/intel_display.c | 3 +++ > > 4 files changed, 28 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index f57b752..f49fdcb 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1405,6 +1405,7 @@ struct drm_i915_private { > > struct intel_vbt_data vbt; > > > > bool bios_ssc; /* BIOS had SSC enabled at boot? */ > > + bool preserve_bios_swizzle; > > > > /* overlay */ > > struct intel_overlay *overlay; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index bfc7af0..0b168fb 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev) > > dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE) > > return; > > > > + if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle) > > + return; > > + > > Above two hunks shouldnt be needed if the setup in > i915_gem_detect_bit_6_swizzle works correctly. > > > I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) | > > DISP_TILE_SURFACE_SWIZZLING); > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > > index cb150e8..73005ad 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > > @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) > > uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN; > > uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN; > > > > - if (IS_VALLEYVIEW(dev)) { > > - swizzle_x = I915_BIT_6_SWIZZLE_NONE; > > - swizzle_y = I915_BIT_6_SWIZZLE_NONE; > > - } else if (INTEL_INFO(dev)->gen >= 6) { > > + if (INTEL_INFO(dev)->gen >= 6) { > > uint32_t dimm_c0, dimm_c1; > > - dimm_c0 = I915_READ(MAD_DIMM_C0); > > - dimm_c1 = I915_READ(MAD_DIMM_C1); > > - dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > > - dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; > > - /* Enable swizzling when the channels are populated with > > - * identically sized dimms. We don't need to check the 3rd > > - * channel because no cpu with gpu attached ships in that > > - * configuration. Also, swizzling only makes sense for 2 > > - * channels anyway. */ > > - if (dimm_c0 == dimm_c1) { > > - swizzle_x = I915_BIT_6_SWIZZLE_9_10; > > - swizzle_y = I915_BIT_6_SWIZZLE_9; > > - } else { > > + > > + /* Make sure to honor boot time display configuration */ > > + if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) { > > Not quite what I had in mind. Imo we need to check for whether any > inherited fbs are tiled and if so also inherit the swizzle setting > unconditionally, whether it is swizzled or unswizzled. See > > http://patchwork.freedesktop.org/patch/22204/ Yes, that's what I do below... I even added it to the changelog: http://patchwork.freedesktop.org/patch/27223/ Did you miss the later hunk in intel_display.c? What we try to do here is enable swizzling if possible, which we can do if no inherited fbs are tiled. So I think I've done exactly what you repeated above, and documented it. So you're going to need to repeat it with different words so I can understand, if I'm still missing something. Thanks,
On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Yes, that's what I do below... I even added it to the changelog: > http://patchwork.freedesktop.org/patch/27223/ > > Did you miss the later hunk in intel_display.c? > > What we try to do here is enable swizzling if possible, which we can do > if no inherited fbs are tiled. > > So I think I've done exactly what you repeated above, and documented > it. So you're going to need to repeat it with different words so I can > understand, if I'm still missing something. In swizzle_detect: ... if (GEN6+) { if (preserve_bios_swizzle) { if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) { swizzle_x = I915_BIT_6_SWIZZLE_9_10; ... } else { swizzle_x = I915_BIT_6_SWIZZLE_NONE; ... } } else { /* existing/old logic to decide about swizzling */ } } ... Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use a small helper function to compute preserve_bios_swizzle instead of storing it in dev_priv (since we will only use it at exactly one place), but that's a pure style preference. -Daniel
On Tue, 10 Jun 2014 21:33:27 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > Yes, that's what I do below... I even added it to the changelog: > > http://patchwork.freedesktop.org/patch/27223/ > > > > Did you miss the later hunk in intel_display.c? > > > > What we try to do here is enable swizzling if possible, which we can do > > if no inherited fbs are tiled. > > > > So I think I've done exactly what you repeated above, and documented > > it. So you're going to need to repeat it with different words so I can > > understand, if I'm still missing something. > > In swizzle_detect: > > ... > > if (GEN6+) { > if (preserve_bios_swizzle) { > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) { > swizzle_x = I915_BIT_6_SWIZZLE_9_10; > ... > } else { > swizzle_x = I915_BIT_6_SWIZZLE_NONE; > ... > } > } else { > /* existing/old logic to decide about swizzling */ > } > } > > ... > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use > a small helper function to compute preserve_bios_swizzle instead of > storing it in dev_priv (since we will only use it at exactly one place), > but that's a pure style preference. Doesn't this amount to the same thing? I.e. we enable it if possible, otherwise just report it as unswizzled? So you're just wanting a style change here?
On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote: > On Tue, 10 Jun 2014 21:33:27 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > Yes, that's what I do below... I even added it to the changelog: > > > http://patchwork.freedesktop.org/patch/27223/ > > > > > > Did you miss the later hunk in intel_display.c? > > > > > > What we try to do here is enable swizzling if possible, which we can do > > > if no inherited fbs are tiled. > > > > > > So I think I've done exactly what you repeated above, and documented > > > it. So you're going to need to repeat it with different words so I can > > > understand, if I'm still missing something. > > > > In swizzle_detect: > > > > ... > > > > if (GEN6+) { > > if (preserve_bios_swizzle) { > > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) { > > swizzle_x = I915_BIT_6_SWIZZLE_9_10; > > ... > > } else { > > swizzle_x = I915_BIT_6_SWIZZLE_NONE; > > ... > > } > > } else { > > /* existing/old logic to decide about swizzling */ > > } > > } > > > > ... > > > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use > > a small helper function to compute preserve_bios_swizzle instead of > > storing it in dev_priv (since we will only use it at exactly one place), > > but that's a pure style preference. > > Doesn't this amount to the same thing? I.e. we enable it if possible, > otherwise just report it as unswizzled? So you're just wanting a style > change here? So presuming I read your code correctly there's two issues: - The first thing you check in swizzle_detect is the hw swizzle bit in DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't bother to set this (they use an untiled buffer after all) this means you've effectively disabled swizzling on almost all machines. The goal of keeping the old logic was that we actually want to enable swizzling in some situations. - If you have a machine which uses tiled framebuffers and enables swizzling in the BIOS your code will a) drop the swizzle setup in gem_init_hw, breaking resume b) not set the swizzle settings correctly in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such a machine exists, but still. -Daniel
On Wed, 11 Jun 2014 11:23:26 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Jun 10, 2014 at 12:45:38PM -0700, Jesse Barnes wrote: > > On Tue, 10 Jun 2014 21:33:27 +0200 > > Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > On Tue, Jun 10, 2014 at 7:27 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > Yes, that's what I do below... I even added it to the changelog: > > > > http://patchwork.freedesktop.org/patch/27223/ > > > > > > > > Did you miss the later hunk in intel_display.c? > > > > > > > > What we try to do here is enable swizzling if possible, which we can do > > > > if no inherited fbs are tiled. > > > > > > > > So I think I've done exactly what you repeated above, and documented > > > > it. So you're going to need to repeat it with different words so I can > > > > understand, if I'm still missing something. > > > > > > In swizzle_detect: > > > > > > ... > > > > > > if (GEN6+) { > > > if (preserve_bios_swizzle) { > > > if (I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING) { > > > swizzle_x = I915_BIT_6_SWIZZLE_9_10; > > > ... > > > } else { > > > swizzle_x = I915_BIT_6_SWIZZLE_NONE; > > > ... > > > } > > > } else { > > > /* existing/old logic to decide about swizzling */ > > > } > > > } > > > > > > ... > > > > > > Plus no shortcut in i915_gem_init_swizzling. Personally I'd also just use > > > a small helper function to compute preserve_bios_swizzle instead of > > > storing it in dev_priv (since we will only use it at exactly one place), > > > but that's a pure style preference. > > > > Doesn't this amount to the same thing? I.e. we enable it if possible, > > otherwise just report it as unswizzled? So you're just wanting a style > > change here? > > So presuming I read your code correctly there's two issues: > > - The first thing you check in swizzle_detect is the hw swizzle bit in > DSP_ARB. If that's not set you force unswizzled. Since most BIOS don't > bother to set this (they use an untiled buffer after all) this means > you've effectively disabled swizzling on almost all machines. The goal > of keeping the old logic was that we actually want to enable swizzling > in some situations. Ah damn, I had been thinking that bit_6_swizzle was the runtime call, and that the init_swizzle call would go ahead and set it up properly. I see that's too late though. > - If you have a machine which uses tiled framebuffers and enables > swizzling in the BIOS your code will a) drop the swizzle setup in > gem_init_hw, breaking resume b) not set the swizzle settings correctly > in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such > a machine exists, but still. This would affect krh's MBA, which is why I wanted testing here... anyway I'll spin a new one and ask krh to test again.
On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> - If you have a machine which uses tiled framebuffers and enables >> swizzling in the BIOS your code will a) drop the swizzle setup in >> gem_init_hw, breaking resume b) not set the swizzle settings correctly >> in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such >> a machine exists, but still. > > This would affect krh's MBA, which is why I wanted testing here... > anyway I'll spin a new one and ask krh to test again. Hm, I've thought the issue with the MBA is that it used tiled fbs, but non-swizzled. And then a mess ensued when we've enabled it. But yeah, unfortunately with the new logic we need to retest :( -Daniel
On Wed, 11 Jun 2014 17:39:29 +0200 Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >> - If you have a machine which uses tiled framebuffers and enables > >> swizzling in the BIOS your code will a) drop the swizzle setup in > >> gem_init_hw, breaking resume b) not set the swizzle settings correctly > >> in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such > >> a machine exists, but still. > > > > This would affect krh's MBA, which is why I wanted testing here... > > anyway I'll spin a new one and ask krh to test again. > > Hm, I've thought the issue with the MBA is that it used tiled fbs, but > non-swizzled. And then a mess ensued when we've enabled it. But yeah, > unfortunately with the new logic we need to retest :( Ah yeah I think you're right, either way, need more testing. Maybe we should have just gone with the first patch to never enable swizzling based on Art's assertion that it didn't matter.
On 06/11/2014 08:41 AM, Jesse Barnes wrote: > On Wed, 11 Jun 2014 17:39:29 +0200 > Daniel Vetter <daniel@ffwll.ch> wrote: > >> On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >>>> - If you have a machine which uses tiled framebuffers and enables >>>> swizzling in the BIOS your code will a) drop the swizzle setup in >>>> gem_init_hw, breaking resume b) not set the swizzle settings correctly >>>> in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such >>>> a machine exists, but still. >>> >>> This would affect krh's MBA, which is why I wanted testing here... >>> anyway I'll spin a new one and ask krh to test again. >> >> Hm, I've thought the issue with the MBA is that it used tiled fbs, but >> non-swizzled. And then a mess ensued when we've enabled it. But yeah, >> unfortunately with the new logic we need to retest :( > > Ah yeah I think you're right, either way, need more testing. > > Maybe we should have just gone with the first patch to never enable > swizzling based on Art's assertion that it didn't matter. > I hate to jump into the middle of a conversation that may or may not be related to a patch I just posted... but... There was a very long internal discussion that the Windows guys had with H/W. For Gen8+ H/W recommends disabling CSX swizzle. Technically, BDW still supports it, but there is a bug _somewhere_ that makes it problematic. In any case it goes away for sure with Gen9+, so disabling on Gen8 doesn't hurt. According to the other discussion, the H/W guys say that enabling actually hurts performance slightly, and the driver should leave the swizzle decisions to the memory controller. Stevo
On Fri, Jun 27, 2014 at 09:15:25AM -0700, Steve Aarnio wrote: > On 06/11/2014 08:41 AM, Jesse Barnes wrote: > >On Wed, 11 Jun 2014 17:39:29 +0200 > >Daniel Vetter <daniel@ffwll.ch> wrote: > > > >>On Wed, Jun 11, 2014 at 5:13 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > >>>>- If you have a machine which uses tiled framebuffers and enables > >>>> swizzling in the BIOS your code will a) drop the swizzle setup in > >>>> gem_init_hw, breaking resume b) not set the swizzle settings correctly > >>>> in swizzle_detect, breaking swap in/out and pwrite/pread. Not sure such > >>>> a machine exists, but still. > >>> > >>>This would affect krh's MBA, which is why I wanted testing here... > >>>anyway I'll spin a new one and ask krh to test again. > >> > >>Hm, I've thought the issue with the MBA is that it used tiled fbs, but > >>non-swizzled. And then a mess ensued when we've enabled it. But yeah, > >>unfortunately with the new logic we need to retest :( > > > >Ah yeah I think you're right, either way, need more testing. > > > >Maybe we should have just gone with the first patch to never enable > >swizzling based on Art's assertion that it didn't matter. > > > > I hate to jump into the middle of a conversation that may or may not be > related to a patch I just posted... but... > > There was a very long internal discussion that the Windows guys had with > H/W. For Gen8+ H/W recommends disabling CSX swizzle. Technically, BDW still > supports it, but there is a bug _somewhere_ that makes it problematic. In > any case it goes away for sure with Gen9+, so disabling on Gen8 doesn't > hurt. > > According to the other discussion, the H/W guys say that enabling actually > hurts performance slightly, and the driver should leave the swizzle > decisions to the memory controller. Patch to disable swizzling detection on gen8+ in i915_gem_tiling.c (only there, imo ok to keep the hw paths around for setting up the registers) welcome ;-) -Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f57b752..f49fdcb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1405,6 +1405,7 @@ struct drm_i915_private { struct intel_vbt_data vbt; bool bios_ssc; /* BIOS had SSC enabled at boot? */ + bool preserve_bios_swizzle; /* overlay */ struct intel_overlay *overlay; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index bfc7af0..0b168fb 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4580,6 +4580,9 @@ void i915_gem_init_swizzling(struct drm_device *dev) dev_priv->mm.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_NONE) return; + if (INTEL_INFO(dev)->gen >= 6 && dev_priv->preserve_bios_swizzle) + return; + I915_WRITE(DISP_ARB_CTL, I915_READ(DISP_ARB_CTL) | DISP_TILE_SURFACE_SWIZZLING); diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index cb150e8..73005ad 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -91,26 +91,30 @@ i915_gem_detect_bit_6_swizzle(struct drm_device *dev) uint32_t swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN; uint32_t swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN; - if (IS_VALLEYVIEW(dev)) { - swizzle_x = I915_BIT_6_SWIZZLE_NONE; - swizzle_y = I915_BIT_6_SWIZZLE_NONE; - } else if (INTEL_INFO(dev)->gen >= 6) { + if (INTEL_INFO(dev)->gen >= 6) { uint32_t dimm_c0, dimm_c1; - dimm_c0 = I915_READ(MAD_DIMM_C0); - dimm_c1 = I915_READ(MAD_DIMM_C1); - dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; - dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; - /* Enable swizzling when the channels are populated with - * identically sized dimms. We don't need to check the 3rd - * channel because no cpu with gpu attached ships in that - * configuration. Also, swizzling only makes sense for 2 - * channels anyway. */ - if (dimm_c0 == dimm_c1) { - swizzle_x = I915_BIT_6_SWIZZLE_9_10; - swizzle_y = I915_BIT_6_SWIZZLE_9; - } else { + + /* Make sure to honor boot time display configuration */ + if (!(I915_READ(DISP_ARB_CTL) & DISP_TILE_SURFACE_SWIZZLING)) { swizzle_x = I915_BIT_6_SWIZZLE_NONE; swizzle_y = I915_BIT_6_SWIZZLE_NONE; + } else { + dimm_c0 = I915_READ(MAD_DIMM_C0); + dimm_c1 = I915_READ(MAD_DIMM_C1); + dimm_c0 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; + dimm_c1 &= MAD_DIMM_A_SIZE_MASK | MAD_DIMM_B_SIZE_MASK; + /* Enable swizzling when the channels are populated with + * identically sized dimms. We don't need to check the + * 3rd channel because no cpu with gpu attached ships + * in that configuration. Also, swizzling only makes + * sense for 2 channels anyway. */ + if (dimm_c0 == dimm_c1) { + swizzle_x = I915_BIT_6_SWIZZLE_9_10; + swizzle_y = I915_BIT_6_SWIZZLE_9; + } else { + swizzle_x = I915_BIT_6_SWIZZLE_NONE; + swizzle_y = I915_BIT_6_SWIZZLE_NONE; + } } } else if (IS_GEN5(dev)) { /* On Ironlake whatever DRAM config, GPU always do diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 0e8c9bc..7dbbef7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2361,6 +2361,7 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, struct intel_plane_config *plane_config) { struct drm_device *dev = intel_crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *c; struct intel_crtc *i; struct intel_framebuffer *fb; @@ -2389,6 +2390,8 @@ static void intel_find_plane_obj(struct intel_crtc *intel_crtc, fb = to_intel_framebuffer(c->primary->fb); if (i915_gem_obj_ggtt_offset(fb->obj) == plane_config->base) { + if (fb->obj->tiling_mode != I915_TILING_NONE) + dev_priv->preserve_bios_swizzle = true; drm_framebuffer_reference(c->primary->fb); intel_crtc->base.primary->fb = c->primary->fb; break;
Some machines (like MBAs) might use a tiled framebuffer but not enable display swizzling at boot time. We want to preserve that configuration if possible to prevent a boot time mode set. On IVB+ it shouldn't affect performance anyway since the memory controller does internal swizzling anyway. For most other configs we'll be able to enable swizzling at boot time, since the initial framebuffer won't be tiled, thus we won't see any corruption when we enable it. v2: preserve swizzling if BIOS had it set (Daniel) v3: preserve swizzling only if we inherited a tiled framebuffer (Daniel) check display swizzle setting in detect_bit_6_swizzle (Daniel) use gen6 as cutoff point (Daniel) Reported-by: Kristian Høgsberg <hoegsberg@gmail.com> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 3 +++ drivers/gpu/drm/i915/i915_gem_tiling.c | 38 +++++++++++++++++++--------------- drivers/gpu/drm/i915/intel_display.c | 3 +++ 4 files changed, 28 insertions(+), 17 deletions(-)