[Intel-gfx] [PATCH 2/4] drm/i915/bdw: Extract rp_state_caps logic
We have a need for duplicated parsing of the RP_STATE_CAPS register (and the setting of the associated fields). To reuse some code, we can extract the function into a simple helper. This patch also addresses the fact that we missed doing this for gen8, something we should have done anyway. This could be two patches, one to extract, and one to add gen8, but it's trivial enough that I think one is fine. I will accept a request to split it. Please notice the fix addressed by v2 below. Valleyview is left untouched because it is different. v2: Logically rebased on top of commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac Author: Jeff McGee jeff.mc...@intel.com Date: Tue Feb 4 11:32:31 2014 -0600 drm/i915: Restore rps/rc6 on reset Note with the above change the fix for gen8 is also handled (which was not the case in Jeff's original patch). Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 40 +++- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 51ff40e..ed45143 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3247,6 +3247,27 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev) spin_unlock_irq(dev_priv-irq_lock); } +static void parse_rp_state_cap(struct drm_i915_private *dev_priv, u32 rp_state_cap) +{ + /* All of these values are in units of 50MHz */ + dev_priv-rps.cur_freq = 0; + /* static values from HW: RP0 RPe RP1 RPn (min_freq) */ + dev_priv-rps.rp1_freq = (rp_state_cap 8) 0xff; + dev_priv-rps.rp0_freq = (rp_state_cap 0) 0xff; + dev_priv-rps.min_freq = (rp_state_cap 16) 0xff; + /* XXX: only BYT has a special efficient freq */ + dev_priv-rps.efficient_freq= dev_priv-rps.rp1_freq; + /* hw_max = RP0 until we check for overclocking */ + dev_priv-rps.max_freq = dev_priv-rps.rp0_freq; + + /* Preserve min/max settings in case of re-init */ + if (dev_priv-rps.max_freq_softlimit == 0) + dev_priv-rps.max_freq_softlimit = dev_priv-rps.max_freq; + + if (dev_priv-rps.min_freq_softlimit == 0) + dev_priv-rps.min_freq_softlimit = dev_priv-rps.min_freq; +} + static void gen8_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -3265,6 +3286,7 @@ static void gen8_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_RC_CONTROL, 0); rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); + parse_rp_state_cap(dev_priv, rp_state_cap); /* 2b: Program RC6 thresholds.*/ I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 16); @@ -3353,23 +3375,7 @@ static void gen6_enable_rps(struct drm_device *dev) rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS); - /* All of these values are in units of 50MHz */ - dev_priv-rps.cur_freq = 0; - /* static values from HW: RP0 RPe RP1 RPn (min_freq) */ - dev_priv-rps.rp1_freq = (rp_state_cap 8) 0xff; - dev_priv-rps.rp0_freq = (rp_state_cap 0) 0xff; - dev_priv-rps.min_freq = (rp_state_cap 16) 0xff; - /* XXX: only BYT has a special efficient freq */ - dev_priv-rps.efficient_freq= dev_priv-rps.rp1_freq; - /* hw_max = RP0 until we check for overclocking */ - dev_priv-rps.max_freq = dev_priv-rps.rp0_freq; - - /* Preserve min/max settings in case of re-init */ - if (dev_priv-rps.max_freq_softlimit == 0) - dev_priv-rps.max_freq_softlimit = dev_priv-rps.max_freq; - - if (dev_priv-rps.min_freq_softlimit == 0) - dev_priv-rps.min_freq_softlimit = dev_priv-rps.min_freq; + parse_rp_state_cap(dev_priv, rp_state_cap); /* disable the counters and set deterministic thresholds */ I915_WRITE(GEN6_RC_CONTROL, 0); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/4] drm/i915/bdw: Set initial rps freq to RP1
Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e9a9aef..51ff40e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3284,8 +3284,10 @@ static void gen8_enable_rps(struct drm_device *dev) rc6_mask); /* 4 Program defaults and thresholds for RPS*/ - I915_WRITE(GEN6_RPNSWREQ, HSW_FREQUENCY(10)); /* Request 500 MHz */ - I915_WRITE(GEN6_RC_VIDEO_FREQ, HSW_FREQUENCY(12)); /* Request 600 MHz */ + I915_WRITE(GEN6_RPNSWREQ, + HSW_FREQUENCY(dev_priv-rps.rp1_freq)); + I915_WRITE(GEN6_RC_VIDEO_FREQ, + HSW_FREQUENCY(dev_priv-rps.rp1_freq)); /* NB: Docs say 1s, and 100 - which aren't equivalent */ I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1 / 128); /* 1 second timeout */ -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915/bdw: RPS frequency bits are the same as HSW
Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ed45143..9728c2c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3041,7 +3041,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val) if (val != dev_priv-rps.cur_freq) { gen6_set_rps_thresholds(dev_priv, val); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) I915_WRITE(GEN6_RPNSWREQ, HSW_FREQUENCY(val)); else -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Match debugfs interface name to new RPS naming
On Sun, Mar 30, 2014 at 01:51:30PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@intel.com Let's change the i915_cur_delayinfo to i915_cur_freqinfo to be in sync with new RPS naming convention. Signed-off-by: Deepak S deepa...@intel.com If we're doing a rename, we might as well do a rename. The function does plenty more than cur my vote: i915_frequency_info --- drivers/gpu/drm/i915/i915_debugfs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 049dcb5..3cfc35c 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -966,7 +966,7 @@ static int i915_rstdby_delays(struct seq_file *m, void *unused) return 0; } -static int i915_cur_delayinfo(struct seq_file *m, void *unused) +static int i915_cur_freqinfo(struct seq_file *m, void *unused) { struct drm_info_node *node = (struct drm_info_node *) m-private; struct drm_device *dev = node-minor-dev; @@ -3772,7 +3772,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {i915_gem_hws_bsd, i915_hws_info, 0, (void *)VCS}, {i915_gem_hws_vebox, i915_hws_info, 0, (void *)VECS}, {i915_rstdby_delays, i915_rstdby_delays, 0}, - {i915_cur_delayinfo, i915_cur_delayinfo, 0}, + {i915_cur_freqinfo, i915_cur_freqinfo, 0}, {i915_delayfreq_table, i915_delayfreq_table, 0}, {i915_inttoext_table, i915_inttoext_table, 0}, {i915_drpc_info, i915_drpc_info, 0}, -- 1.8.5.2 -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915/bdw: enable eDRAM.
The same register exists for querying and programming eDRAM AKA eLLC. So we can simply use it. For now, use all the same defaults as we had for Haswell, since like Haswell, I have no further details. I do not actually have a part with eDRAM, so I cannot test this. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_uncore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 823d699..f5e7240 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -370,7 +370,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev) if (HAS_FPGA_DBG_UNCLAIMED(dev)) __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); - if (IS_HASWELL(dev) + if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) (__raw_i915_read32(dev_priv, HSW_EDRAM_PRESENT) == 1)) { /* The docs do not explain exactly how the calculation can be * made. It is somewhat guessable, but for now, it's always -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915/bdw: Add WT caching ability
I don't have any insight on what parts can do what. The docs do seem to suggest WT caching works in at least the same manner as it doesn't on Haswell. The addr = 0 is to shut up GCC: drivers/gpu/drm/i915/i915_gem_gtt.c:80:7: warning: 'addr' may be used uninitialized in this function [-Wmaybe-uninitialized] Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 11 ++- drivers/gpu/drm/i915/i915_gem_gtt.c | 17 + 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e23bb73..896fe8e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,11 +1822,12 @@ struct drm_i915_cmd_table { #define BSD_RING (1VCS) #define BLT_RING (1BCS) #define VEBOX_RING (1VECS) -#define HAS_BSD(dev)(INTEL_INFO(dev)-ring_mask BSD_RING) -#define HAS_BLT(dev)(INTEL_INFO(dev)-ring_mask BLT_RING) -#define HAS_VEBOX(dev)(INTEL_INFO(dev)-ring_mask VEBOX_RING) -#define HAS_LLC(dev)(INTEL_INFO(dev)-has_llc) -#define HAS_WT(dev)(IS_HASWELL(dev) to_i915(dev)-ellc_size) +#define HAS_BSD(dev) (INTEL_INFO(dev)-ring_mask BSD_RING) +#define HAS_BLT(dev) (INTEL_INFO(dev)-ring_mask BLT_RING) +#define HAS_VEBOX(dev) (INTEL_INFO(dev)-ring_mask VEBOX_RING) +#define HAS_LLC(dev) (INTEL_INFO(dev)-has_llc) +#define HAS_WT(dev)((IS_HASWELL(dev) || IS_BROADWELL(dev)) \ +to_i915(dev)-ellc_size) #define I915_NEED_GFX_HWS(dev) (INTEL_INFO(dev)-need_gfx_hws) #define HAS_HW_CONTEXTS(dev) (INTEL_INFO(dev)-gen = 6) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 4467974..10d00ee 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -68,10 +68,19 @@ static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr, { gen8_gtt_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0; pte |= addr; - if (level != I915_CACHE_NONE) - pte |= PPAT_CACHED_INDEX; - else + + switch (level) { + case I915_CACHE_NONE: pte |= PPAT_UNCACHED_INDEX; + break; + case I915_CACHE_WT: + pte |= PPAT_DISPLAY_ELLC_INDEX; + break; + default: + pte |= PPAT_CACHED_INDEX; + break; + } + return pte; } @@ -1367,7 +1376,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, (gen8_gtt_pte_t __iomem *)dev_priv-gtt.gsm + first_entry; int i = 0; struct sg_page_iter sg_iter; - dma_addr_t addr; + dma_addr_t addr = 0; for_each_sg_page(st-sgl, sg_iter, st-nents, 0) { addr = sg_dma_address(sg_iter.sg) + -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/bdw: Add WT caching ability
On Sat, Mar 29, 2014 at 03:56:41PM -0700, Ben Widawsky wrote: I don't have any insight on what parts can do what. The docs do seem to suggest WT caching works in at least the same manner as it doesn't on Haswell. That's a freudian slip... s/doesn't/does/ The addr = 0 is to shut up GCC: drivers/gpu/drm/i915/i915_gem_gtt.c:80:7: warning: 'addr' may be used uninitialized in this function [-Wmaybe-uninitialized] Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 11 ++- drivers/gpu/drm/i915/i915_gem_gtt.c | 17 + 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e23bb73..896fe8e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,11 +1822,12 @@ struct drm_i915_cmd_table { #define BSD_RING (1VCS) #define BLT_RING (1BCS) #define VEBOX_RING (1VECS) -#define HAS_BSD(dev)(INTEL_INFO(dev)-ring_mask BSD_RING) -#define HAS_BLT(dev)(INTEL_INFO(dev)-ring_mask BLT_RING) -#define HAS_VEBOX(dev)(INTEL_INFO(dev)-ring_mask VEBOX_RING) -#define HAS_LLC(dev)(INTEL_INFO(dev)-has_llc) -#define HAS_WT(dev)(IS_HASWELL(dev) to_i915(dev)-ellc_size) +#define HAS_BSD(dev) (INTEL_INFO(dev)-ring_mask BSD_RING) +#define HAS_BLT(dev) (INTEL_INFO(dev)-ring_mask BLT_RING) +#define HAS_VEBOX(dev) (INTEL_INFO(dev)-ring_mask VEBOX_RING) +#define HAS_LLC(dev) (INTEL_INFO(dev)-has_llc) +#define HAS_WT(dev) ((IS_HASWELL(dev) || IS_BROADWELL(dev)) \ + to_i915(dev)-ellc_size) #define I915_NEED_GFX_HWS(dev) (INTEL_INFO(dev)-need_gfx_hws) #define HAS_HW_CONTEXTS(dev) (INTEL_INFO(dev)-gen = 6) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 4467974..10d00ee 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -68,10 +68,19 @@ static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr, { gen8_gtt_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0; pte |= addr; - if (level != I915_CACHE_NONE) - pte |= PPAT_CACHED_INDEX; - else + + switch (level) { + case I915_CACHE_NONE: pte |= PPAT_UNCACHED_INDEX; + break; + case I915_CACHE_WT: + pte |= PPAT_DISPLAY_ELLC_INDEX; + break; + default: + pte |= PPAT_CACHED_INDEX; + break; + } + return pte; } @@ -1367,7 +1376,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm, (gen8_gtt_pte_t __iomem *)dev_priv-gtt.gsm + first_entry; int i = 0; struct sg_page_iter sg_iter; - dma_addr_t addr; + dma_addr_t addr = 0; for_each_sg_page(st-sgl, sg_iter, st-nents, 0) { addr = sg_dma_address(sg_iter.sg) + -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/20] drm/i915: enable SDEIER later
On Thu, Mar 27, 2014 at 11:39:36AM -0300, Paulo Zanoni wrote: 2014-03-18 17:29 GMT-03:00 Ben Widawsky b...@bwidawsk.net: On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com On the preinstall stage we should just disable all the interrupts, but we currently enable all the south display interrupts due to the way we touch SDEIER at the IRQ handlers (note: they are still masked and our IRQ handler is disabled). I think this statement is false. The interrupt is enabled right after preinstall(). For the nomodeset case, this actually seems to make some difference. It still looks fine to me though. Could you please clarify this paragraph? The point was, IRQ handler is disabled is false. At least when I last checked. We've registered the interrupt by then, which means we can potentially get interrupts from the hardware. I think we just might disagree on what enabled means. Perhaps this is due to me working for too long with buggy hardware. In any event, as I said, it does look fine to me as is. We currently enable SDEIER at ibx_irq_preinstall, but the reason why we do this is because we change SDEIER at the IRQ handler. So if we move that code from preinstall to postinstall, but at a point where no interrupts are enabled yet, we should be safe, and this is what the patch does. Instead of doing that, let's make the preinstall stage just disable all the south interrupts, and do the proper interrupt dance/ordering at the postinstall stage, including an assert to check if everything is behaving as expected. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 95f535b..4479e29 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev) if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev)) I915_WRITE(SERR_INT, 0x); +} - /* - * SDEIER is also touched by the interrupt handler to work around missed - * PCH interrupts. Hence we can't update it after the interrupt handler - * is enabled - instead we unconditionally enable all PCH interrupt - * sources here, but then only unmask them as needed with SDEIMR. - */ +/* + * SDEIER is also touched by the interrupt handler to work around missed PCH + * interrupts. Hence we can't update it after the interrupt handler is enabled - + * instead we unconditionally enable all PCH interrupt sources here, but then + * only unmask them as needed with SDEIMR. + * + * This function needs to be called before interrupts are enabled. + */ +static void ibx_irq_pre_postinstall(struct drm_device *dev) sde_irq_postinstall()? I agree the current name is bad, but we use the IBX naming for everything on the PCH at i915_irq.c, and ironlake_irq_postinstall() already calls a function named ibx_irq_postinstall(), so I needed a name that means call this just before the postinstall() steps. If we rename it to sde_irq_postinstall we may confuse users due to the fact that we also have ibx_irq_postinstall. So with the current patch, we have this: void ironlake_irq_postinstall() { /* We have to call this before enabling the interrupts */ ibx_irq_pre_postinstall(); /* Enable the interrupts */ GEN5_IRQ_INIT(DE, dev_priv-irq_mask, display_mask | extra_mask); /* Now do the necessary postinstall adjustments to the PCH stuff */ ibx_irq_postinstall(); } But I'm still open to new naming suggestions. I gave my suggestion. If you and or the maintainer think it's not a better one due to the existing scheme, then by all means, feel free to move on. There's nothing functionally incorrect that I can spot. +{ + struct drm_i915_private *dev_priv = dev-dev_private; + + if (HAS_PCH_NOP(dev)) + return; + + WARN_ON(I915_READ(SDEIER) != 0); I915_WRITE(SDEIER, 0x); POSTING_READ(SDEIER); } @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) dev_priv-irq_mask = ~display_mask; + ibx_irq_pre_postinstall(dev); + GEN5_IRQ_INIT(DE, dev_priv-irq_mask, display_mask | extra_mask); gen5_gt_irq_postinstall(dev); @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; + ibx_irq_pre_postinstall(dev); + gen8_gt_irq_postinstall(dev_priv); gen8_de_irq_postinstall(dev_priv); -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http
Re: [Intel-gfx] [PATCH] drm/i915: Prevent context obj from being corrupted
On Wed, Mar 26, 2014 at 08:04:38AM +0100, Daniel Vetter wrote: On Wed, Mar 26, 2014 at 07:24:16AM +0100, Daniel Vetter wrote: On Tue, Mar 25, 2014 at 07:01:50PM -0700, Ben Widawsky wrote: While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted. NOTE: This is a slightly different patch than what I posted to Bugzilla. References: https://bugs.freedesktop.org/show_bug.cgi?id=75724 Signed-off-by: Ben Widawsky b...@bwidawsk.net We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ... Actually your patch doesn't accomplish much, since MI_UPDATE_GTT only updates the global gtt, not the aliasing ppgtt. Which means that userspace batches can still happily stomp all over your precious context objects. See comment #5 in that bug report: https://bugs.freedesktop.org/show_bug.cgi?id=75724#c5 Cheers, Daniel Quoting you from comment #5: ctx objects should only be bound in the global gtt with aliasing ppgtt. The bug was not present with full PPGTT. -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Prevent context obj from being corrupted
On Wed, Mar 26, 2014 at 08:06:00AM -0700, Ben Widawsky wrote: On Wed, Mar 26, 2014 at 08:04:38AM +0100, Daniel Vetter wrote: On Wed, Mar 26, 2014 at 07:24:16AM +0100, Daniel Vetter wrote: On Tue, Mar 25, 2014 at 07:01:50PM -0700, Ben Widawsky wrote: While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted. NOTE: This is a slightly different patch than what I posted to Bugzilla. References: https://bugs.freedesktop.org/show_bug.cgi?id=75724 Signed-off-by: Ben Widawsky b...@bwidawsk.net We could stop the aliasing ppgtt binder from always binding into the ppgtt. Which is the bug I'm signed up to fix, and which probably curbs 99% of all potential corruption. The last 1% is the kernel getting his kmapping wrong ... Actually your patch doesn't accomplish much, since MI_UPDATE_GTT only updates the global gtt, not the aliasing ppgtt. Which means that userspace batches can still happily stomp all over your precious context objects. See comment #5 in that bug report: https://bugs.freedesktop.org/show_bug.cgi?id=75724#c5 Cheers, Daniel Quoting you from comment #5: ctx objects should only be bound in the global gtt with aliasing ppgtt. The bug was not present with full PPGTT. Oh, I see the confusion. I don't necessarily think it's userspace that's corrupting this. I just want to prove it's *not* being corrupted. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: Implement Wa4x4STCOptimizationDisable:bdw
On Wed, Mar 26, 2014 at 06:41:51PM +, Damien Lespiau wrote: Not implementing this W/A can lead to hangs. Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Rafael Barbalho rafael.barba...@intel.com Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Damien Lespiau damien.lesp...@intel.com From reading the HSD, it's not a workaround. It was a spec bug. I presume the workaround name came from the workaround database? If it did not, I'd just drop any mention of workaround. If it's in the workaround database, maybe just leave it so we can check later. Either way: Reviewed-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_reg.h | 3 ++- drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0ebc20d..927a7c1 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1060,7 +1060,8 @@ enum punit_power_well { #define CACHE_MODE_0_GEN70x7000 /* IVB+ */ #define HIZ_RAW_STALL_OPT_DISABLE (12) #define CACHE_MODE_1 0x7004 /* IVB+ */ -#define PIXEL_SUBSPAN_COLLECT_OPT_DISABLE (16) +#define PIXEL_SUBSPAN_COLLECT_OPT_DISABLE (16) +#define GEN8_4x4_STC_OPTIMIZATION_DISABLE (16) #define GEN6_BLITTER_ECOSKPD 0x221d0 #define GEN6_BLITTER_LOCK_SHIFT16 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c1e2d75..b66a43b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4882,6 +4882,10 @@ static void gen8_init_clock_gating(struct drm_device *dev) /* WaDisableSDEUnitClockGating:bdw */ I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) | GEN8_SDEUNIT_CLOCK_GATE_DISABLE); + + /* Wa4x4STCOptimizationDisable:bdw */ + I915_WRITE(CACHE_MODE_1, +_MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE)); } static void haswell_init_clock_gating(struct drm_device *dev) -- 1.8.3.1 -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/bdw: Implement Wa4x4STCOptimizationDisable:bdw
On Wed, Mar 26, 2014 at 12:08:44PM -0700, Ben Widawsky wrote: On Wed, Mar 26, 2014 at 06:41:51PM +, Damien Lespiau wrote: Not implementing this W/A can lead to hangs. Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Rafael Barbalho rafael.barba...@intel.com Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Damien Lespiau damien.lesp...@intel.com From reading the HSD, it's not a workaround. It was a spec bug. I presume the workaround name came from the workaround database? If it did not, I'd just drop any mention of workaround. If it's in the workaround database, maybe just leave it so we can check later. Either way: Reviewed-by: Ben Widawsky b...@bwidawsk.net Oh, and I think CC: stable --- drivers/gpu/drm/i915/i915_reg.h | 3 ++- drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 0ebc20d..927a7c1 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1060,7 +1060,8 @@ enum punit_power_well { #define CACHE_MODE_0_GEN7 0x7000 /* IVB+ */ #define HIZ_RAW_STALL_OPT_DISABLE (12) #define CACHE_MODE_1 0x7004 /* IVB+ */ -#define PIXEL_SUBSPAN_COLLECT_OPT_DISABLE (16) +#define PIXEL_SUBSPAN_COLLECT_OPT_DISABLE(16) +#define GEN8_4x4_STC_OPTIMIZATION_DISABLE(16) #define GEN6_BLITTER_ECOSKPD 0x221d0 #define GEN6_BLITTER_LOCK_SHIFT 16 diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c1e2d75..b66a43b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4882,6 +4882,10 @@ static void gen8_init_clock_gating(struct drm_device *dev) /* WaDisableSDEUnitClockGating:bdw */ I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) | GEN8_SDEUNIT_CLOCK_GATE_DISABLE); + + /* Wa4x4STCOptimizationDisable:bdw */ + I915_WRITE(CACHE_MODE_1, + _MASKED_BIT_ENABLE(GEN8_4x4_STC_OPTIMIZATION_DISABLE)); } static void haswell_init_clock_gating(struct drm_device *dev) -- 1.8.3.1 -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/20] ILK+ interrupt improvements, v2
On Wed, Mar 26, 2014 at 05:33:20PM -0300, Paulo Zanoni wrote: 2014-03-19 14:25 GMT-03:00 Ben Widawsky b...@bwidawsk.net: On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote: On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote: On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Hi This is basically a rebase of [PATCH 00/19] ILK+ interrupt improvements, which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new. Original cover letter: http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv-pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime! Thanks, Paulo Paulo Zanoni (20): drm/i915: add GEN5_IRQ_INIT macro drm/i915: also use GEN5_IRQ_INIT with south display interrupts drm/i915: use GEN8_IRQ_INIT on GEN5 drm/i915: add GEN5_IRQ_FINI drm/i915: don't forget to uninstall the PM IRQs drm/i915: properly clear IIR at irq_uninstall on Gen5+ drm/i915: add GEN5_IRQ_INIT drm/i915: check if IIR is still zero at postinstall on Gen5+ drm/i915: fix SERR_INT init/reset code drm/i915: fix GEN7_ERR_INT init/reset code drm/i915: fix open coded gen5_gt_irq_preinstall drm/i915: extract ibx_irq_uninstall drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall drm/i915: enable SDEIER later drm/i915: remove ibx_irq_uninstall drm/i915: add missing intel_hpd_irq_uninstall drm/i915: add ironlake_irq_reset drm/i915: add gen8_irq_reset drm/i915: only enable HWSTAM interrupts on postinstall on ILK+ drm/i915: add POSTING_READs to the IRQ init/reset macros drivers/gpu/drm/i915/i915_irq.c | 270 ++-- 1 file changed, 121 insertions(+), 149 deletions(-) Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P 1-5: Reviewed-by: Ben Widawsky b...@bwidawsk.net (with possible comment improvement on #3) 7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct: Reviewed-by: Ben Widawsky b...@bwidawsk.net 8: I'd really like to drop this one. Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm. So you don't need my reviewed-by then. I don't like it... 9-10: Reviewed-by: Ben Widawsky b...@bwidawsk.net 12-13: I wouldn't mind cpt_irq_* rename, but either way Reviewed-by: Ben Widawsky b...@bwidawsk.net 14: With the requested change in the mail: Reviewed-by: Ben Widawsky b...@bwidawsk.net 16: Reviewed-by: Ben Widawsky b...@bwidawsk.net 20: Should be squashed, but Reviewed-by: Ben Widawsky b...@bwidawsk.net 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean disable. I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're: Reviewed-by: Ben Widawsky b...@bwidawsk.net Paulo's using reset functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain disable more misleading. I cannot disagree more. Every time I read reset it confuses me. But it seems like I am the minority. I understand reset may not be the best name, I was already expecting to see suggestions on the naming here. IMHO disable is also usable, and I could rename, but Daniel just called it misleading. So how about we rename it to clear instead? (let's see if I can make Ben and Daniel agree on something!) I'll leave discussion of the other topics to the other emails. clear has a distinct definition, and in the case of the mask, you are not clearing. It's better than reset I still like, disable I can live with disable_and_mask I think you raise some good points in your review, and besides
Re: [Intel-gfx] [PATCH] drm/i915/bdw: Implement Wa4x4STCOptimizationDisable:bdw
On Wed, Mar 26, 2014 at 10:39:21PM +0100, Daniel Vetter wrote: On Wed, Mar 26, 2014 at 12:10:41PM -0700, Ben Widawsky wrote: On Wed, Mar 26, 2014 at 12:08:44PM -0700, Ben Widawsky wrote: On Wed, Mar 26, 2014 at 06:41:51PM +, Damien Lespiau wrote: Not implementing this W/A can lead to hangs. Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Rafael Barbalho rafael.barba...@intel.com Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Damien Lespiau damien.lesp...@intel.com From reading the HSD, it's not a workaround. It was a spec bug. I presume the workaround name came from the workaround database? If it did not, I'd just drop any mention of workaround. If it's in the workaround database, maybe just leave it so we can check later. Either way: Reviewed-by: Ben Widawsky b...@bwidawsk.net Oh, and I think CC: stable Queued for -next, thanks for the patch. -Daniel You want/need this in bdw-backports? -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote: As Broadwell has an increased virtual address size, it requires more than 32 bits to store offsets into its address space. This includes the debug registers to track the current HEAD of the individual rings, which may be anywhere within the per-process address spaces. In order to find the full location, we need to read the high bits from a second register. We then also need to expand our storage to keep track of the larger address. v2: Carefully read the two registers to catch wraparound between the reads. v3: Use a WARN_ON rather than loop indefinitely on an unstable register read. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Timo Aaltonen tjaal...@ubuntu.com Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 13 - drivers/gpu/drm/i915/i915_gpu_error.c |2 +- drivers/gpu/drm/i915/i915_irq.c |8 +--- drivers/gpu/drm/i915/i915_reg.h |1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 22 -- drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++--- 6 files changed, 38 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5418253..4c09fb2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -354,12 +354,12 @@ struct drm_i915_error_state { u32 ipeir; u32 ipehr; u32 instdone; - u32 acthd; u32 bbstate; u32 instpm; u32 instps; u32 seqno; u64 bbaddr; + u64 acthd; u32 fault_reg; u32 faddr; u32 rc_psmi; /* sleep state */ @@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine); #define I915_WRITE64(reg, val) dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true) #define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv, (reg), true) +#define I915_READ64_2x32(lower_reg, upper_reg) ({\ + u32 upper = I915_READ(upper_reg); \ + u32 lower = I915_READ(lower_reg); \ + u32 tmp = I915_READ(upper_reg); \ + if (upper != tmp) { \ + upper = tmp;\ + lower = I915_READ(lower_reg); \ + WARN_ON(I915_READ(upper_reg) != upper); \ + } \ + (u64)upper 32 | lower; }) + May as well get the most recent value of upper: WARN_ON((tmp = I915_READ(upper_reg)) != upper); } return (u64)tmp 32 | lower; with or without: Reviewed-by: Ben Widawsky b...@bwidawsk.net #define POSTING_READ(reg)(void)I915_READ_NOTRACE(reg) #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index b153a16..9519aa2 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -248,7 +248,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, TAIL: 0x%08x\n, ring-tail); err_printf(m, CTL: 0x%08x\n, ring-ctl); err_printf(m, HWS: 0x%08x\n, ring-hws); - err_printf(m, ACTHD: 0x%08x\n, ring-acthd); + err_printf(m, ACTHD: 0x%08llx\n, ring-acthd); err_printf(m, IPEIR: 0x%08x\n, ring-ipeir); err_printf(m, IPEHR: 0x%08x\n, ring-ipehr); err_printf(m, INSTDONE: 0x%08x\n, ring-instdone); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 77dbef6..9caae98 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2507,7 +2507,8 @@ static struct intel_ring_buffer * semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno) { struct drm_i915_private *dev_priv = ring-dev-dev_private; - u32 cmd, ipehr, acthd, acthd_min; + u64 acthd, acthd_min; + u32 cmd, ipehr; ipehr = I915_READ(RING_IPEHR(ring-mmio_base)); if ((ipehr ~(0x3 16)) != @@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv) } static enum intel_ring_hangcheck_action -ring_stuck(struct intel_ring_buffer *ring, u32 acthd) +ring_stuck(struct intel_ring_buffer *ring, u64 acthd) { struct drm_device *dev = ring-dev; struct drm_i915_private *dev_priv = dev-dev_private; @@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data) return
Re: [Intel-gfx] [PATCH 1/2] drm/i915: fix up semaphore_waits_for
On Tue, Mar 25, 2014 at 10:42:09AM +0100, Daniel Vetter wrote: On Mon, Mar 24, 2014 at 03:37:12PM -0700, Ben Widawsky wrote: On Sat, Mar 22, 2014 at 06:52:25PM +0100, Daniel Vetter wrote: On Fri, Mar 21, 2014 at 07:33:59PM +0200, Mika Kuoppala wrote: Hi, Daniel Vetter daniel.vet...@ffwll.ch writes: There's an entire pile of issues in here: - Use the main RING_HEAD register, not ACTHD. ACTHD points at the gtt offset of the batch buffer when a batch is executed. Semaphores are always emitted to the main ring, so we always want to look at that. The ipehr check should make sure that we are at the ring and acthd should not be at batch. Regardless I agree that RING_HEAD is much more safer. When I have tried to get bottom of the snb reset hang, I have noticed that after reset the acthd is sometimes 0x0c even tho head is 0x00, on snb. Hm, should we maybe mask out the lower bits, too? If you can confirm this, can you please add a follow-up patch? - Mask the obtained HEAD pointer with the actual ring size, which is much smaller. Together with the above issue this resulted us in trying to dereference a pointer way outside of the ring mmio mapping. The resulting invalid access in interrupt context (hangcheck is executed from timers) lead to a full blown kernel panic. The fbcon panic handler then tried to frob our driver harder, resulting in a full machine hang at least on my snb here where I've stumbled over this. - Handle ring wrapping correctly and be a bit more explicit about how many dwords we're scanning. We probably should also scan more than just 4 ... ipehr dont update on MI_NOOPS and the head should point to the extra MI_NOOP after semaphore sequence. So 4 should be enough. Perhaps revisit this when bdw gains semaphores. Yeah, Chris also mentioned that the HEAD should point at one dword past the end of the ring, so should even work when there are no MI_NOOPs. In any case this code is more robust in case hw engineers suddenly change the behaviour. - Space out some of teh computations for readability. This prevents hard-hangs on my snb here. Verdict from QA is still pending, but the symptoms match. Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Cc: Chris Wilson ch...@chris-wilson.co.uk Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74100 The hard hangs are not so frequent with this patch but they are still there. This patch should take care of the oops we have been seeing, but there is still something else to be found until #74100 can be marked as fixed. Very often after reset, when igt has pushed the quietence batches into rings, blitter and video ring heads gets moved properly but all updates to hardware status page and to the sync registers are missing. And result is hang by hangcheck. Repeat enough times and it is a hard hang. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Please remove the blowup comment and also update the bugzilla tag. Yeah, QA also says that it doesn't fix the hard hangs, only seems to reduce them a bit on certain setups. I've updated the commit message. btw are you testing with FBDEV=n? The lack of a fbcon panic handler should greatly increase the chances that the last few message get correctly transmitted through other means like netconsole. Patches 1-2 and the followup one are Reviewed-by: Mika Kuoppala mika.kuopp...@intel.com Thanks for the review, patches merged. -Daniel Patch 2 was trivial to implement for gen8. This functionality is a lot less trivial. I volunteered to do patch 2, are you going to port this to gen8? If you fill out the bdw pieces for patch 23 the only thing you need to change here is to adjsut the number of dwords we walk backwards to make sure that the bdw semaphore cmd still fits. Or at least that's been my idea behind all this, maybe I've overlooked something. -Daniel I don't think it's quite that easy, but I took it as a, yes. Which one is patch 3? -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Upgrade execbuffer fail after resume failure to EIO
On Tue, Mar 25, 2014 at 08:03:28AM +, Chris Wilson wrote: If we try to execute on a known ring, but it has failed to be initialised correctly, report that the GPU is hung rather than the command invalid. This leaves us reporting EINVAL only if the user requests execution on a ring that is not supported by the device. This should prevent UXA from getting stuck in a null render loop after a failed resume. Reported-by: Jiri Kosina ji...@jikos.cz References: https://bugs.freedesktop.org/show_bug.cgi?id=76554 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 1b45163e19f3..22c650490f54 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -991,6 +991,18 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev, return 0; } +static bool +intel_ring_valid(struct intel_ring_buffer *ring) +{ + switch (ring-id) { + case RCS: return true; + case VCS: return HAS_BSD(ring-dev); + case BCS: return HAS_BLT(ring-dev); intel_enable_blt()? + case VECS: return HAS_VEBOX(ring-dev); + default: return false; + } +} + static int i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct drm_file *file, @@ -1041,7 +1053,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (!intel_ring_initialized(ring)) { DRM_DEBUG(execbuf with invalid ring: %d\n, (int)(args-flags I915_EXEC_RING_MASK)); - return -EINVAL; + return intel_ring_valid(ring) ? -EIO : -EINVAL; } mode = args-flags I915_EXEC_CONSTANTS_MASK; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Allow full PPGTT with param override
On Tue, Mar 25, 2014 at 10:46:12AM +0100, Daniel Vetter wrote: On Mon, Mar 24, 2014 at 06:06:00PM -0700, Ben Widawsky wrote: When PPGTT was disabled by default, the patch also prevented the user from overriding this behavior via module parameter. Being able to test this on arbitrary kernels is extremely beneficial to track down the remaining bugs. The patch that prevented this was: commit 93a25a9e2d67765c3092bfaac9b855d95e39df97 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Thu Mar 6 09:40:43 2014 +0100 drm/i915: Disable full ppgtt by default By default PPGTT is set to -1. 0 means off, 1 means aliasing only, 2 means full, all other values are reserved. Signed-off-by: Ben Widawsky b...@bwidawsk.net My apologies for breaking this a bit harder than intended, and thanks for fixing it up. Patch merged to dinq. -Daniel No harm, no foul. FWIW QA had been reported 0 PPGTT regressions for the last week or so. Score one for their consistency. --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bbc9420..9068628 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -50,7 +50,7 @@ bool intel_enable_ppgtt(struct drm_device *dev, bool full) /* Full ppgtt disabled by default for now due to issues. */ if (full) - return false; /* HAS_PPGTT(dev) */ + return HAS_PPGTT(dev) (i915.enable_ppgtt == 2); else return HAS_ALIASING_PPGTT(dev); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] quick_dump: Fix the danvet fallout.
quick_dump built fine, but it could actually run, since a lot of the linking happens at run time. There is one hack where we redefine the environment stuff, since depending on igt_aux means we have to pull in libdrm, which I do not want to do. Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- tools/quick_dump/Makefile.am | 2 ++ tools/quick_dump/chipset.i| 5 +++-- tools/quick_dump/chipset_macro_wrap.c | 14 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tools/quick_dump/Makefile.am b/tools/quick_dump/Makefile.am index 7572ee5..ca26993 100644 --- a/tools/quick_dump/Makefile.am +++ b/tools/quick_dump/Makefile.am @@ -8,6 +8,8 @@ bin_SCRIPTS = chipset.py lib_LTLIBRARIES = I915ChipsetPython.la I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS) $(PCIACCESS_LIBS) I915ChipsetPython_la_SOURCES = chipset_wrap_python.c chipset_macro_wrap.c \ + $(top_srcdir)/lib/igt_core.c \ + $(top_srcdir)/lib/igt_debugfs.c \ $(top_srcdir)/lib/intel_os.c \ $(top_srcdir)/lib/intel_chipset.c \ $(top_srcdir)/lib/intel_reg_map.c \ diff --git a/tools/quick_dump/chipset.i b/tools/quick_dump/chipset.i index 395418e..ae176e8 100644 --- a/tools/quick_dump/chipset.i +++ b/tools/quick_dump/chipset.i @@ -4,6 +4,7 @@ #include pciaccess.h #include stdint.h #include intel_chipset.h +#include intel_io.h extern int is_sandybridge(unsigned short pciid); extern int is_ivybridge(unsigned short pciid); extern int is_valleyview(unsigned short pciid); @@ -12,7 +13,7 @@ extern int is_broadwell(unsigned short pciid); extern struct pci_device *intel_get_pci_device(); extern int intel_register_access_init(struct pci_device *pci_dev, int safe); extern uint32_t intel_register_read(uint32_t reg); -extern uint32_t intel_register_write(uint32_t reg, uint32_t val); +extern void intel_register_write(uint32_t reg, uint32_t val); extern void intel_register_access_fini(); extern int intel_register_access_needs_fakewake(); extern unsigned short pcidev_to_devid(struct pci_device *pci_dev); @@ -27,7 +28,7 @@ extern int is_broadwell(unsigned short pciid); extern struct pci_device *intel_get_pci_device(); extern int intel_register_access_init(struct pci_device *pci_dev, int safe); extern uint32_t intel_register_read(uint32_t reg); -extern uint32_t intel_register_write(uint32_t reg, uint32_t val); +extern void intel_register_write(uint32_t reg, uint32_t val); extern void intel_register_access_fini(); extern int intel_register_access_needs_fakewake(); extern unsigned short pcidev_to_devid(struct pci_device *pci_dev); diff --git a/tools/quick_dump/chipset_macro_wrap.c b/tools/quick_dump/chipset_macro_wrap.c index 392b85e..ee79777 100644 --- a/tools/quick_dump/chipset_macro_wrap.c +++ b/tools/quick_dump/chipset_macro_wrap.c @@ -1,3 +1,5 @@ +#include stdbool.h +#include stdlib.h #include pciaccess.h #include intel_chipset.h @@ -31,3 +33,15 @@ unsigned short pcidev_to_devid(struct pci_device *pdev) { return pdev-device_id; } + +bool igt_check_boolean_env_var(const char *env_var, bool default_value) +{ + char *val; + + val = getenv(env_var); + if (!val) + return default_value; + + return atoi(val) != 0; +} + -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] quick_dump: Fix the danvet fallout.
On Tue, Mar 25, 2014 at 08:47:47PM +0100, Daniel Vetter wrote: On Tue, Mar 25, 2014 at 11:38:51AM -0700, Ben Widawsky wrote: quick_dump built fine, but it could actually run, since a lot of the linking happens at run time. There is one hack where we redefine the environment stuff, since depending on igt_aux means we have to pull in libdrm, which I do not want to do. Why? libdrm is kinda something we need anyway ... -Daniel The goal is to keep the dumper's dependencies and functionality to a bare minimum. We absolutely should not need libdrm for register dumps. If you want to do fancy stuff post process that requires libdrm, that's another thing. Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- tools/quick_dump/Makefile.am | 2 ++ tools/quick_dump/chipset.i| 5 +++-- tools/quick_dump/chipset_macro_wrap.c | 14 ++ 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/tools/quick_dump/Makefile.am b/tools/quick_dump/Makefile.am index 7572ee5..ca26993 100644 --- a/tools/quick_dump/Makefile.am +++ b/tools/quick_dump/Makefile.am @@ -8,6 +8,8 @@ bin_SCRIPTS = chipset.py lib_LTLIBRARIES = I915ChipsetPython.la I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS) $(PCIACCESS_LIBS) I915ChipsetPython_la_SOURCES = chipset_wrap_python.c chipset_macro_wrap.c \ + $(top_srcdir)/lib/igt_core.c \ + $(top_srcdir)/lib/igt_debugfs.c \ $(top_srcdir)/lib/intel_os.c \ $(top_srcdir)/lib/intel_chipset.c \ $(top_srcdir)/lib/intel_reg_map.c \ diff --git a/tools/quick_dump/chipset.i b/tools/quick_dump/chipset.i index 395418e..ae176e8 100644 --- a/tools/quick_dump/chipset.i +++ b/tools/quick_dump/chipset.i @@ -4,6 +4,7 @@ #include pciaccess.h #include stdint.h #include intel_chipset.h +#include intel_io.h extern int is_sandybridge(unsigned short pciid); extern int is_ivybridge(unsigned short pciid); extern int is_valleyview(unsigned short pciid); @@ -12,7 +13,7 @@ extern int is_broadwell(unsigned short pciid); extern struct pci_device *intel_get_pci_device(); extern int intel_register_access_init(struct pci_device *pci_dev, int safe); extern uint32_t intel_register_read(uint32_t reg); -extern uint32_t intel_register_write(uint32_t reg, uint32_t val); +extern void intel_register_write(uint32_t reg, uint32_t val); extern void intel_register_access_fini(); extern int intel_register_access_needs_fakewake(); extern unsigned short pcidev_to_devid(struct pci_device *pci_dev); @@ -27,7 +28,7 @@ extern int is_broadwell(unsigned short pciid); extern struct pci_device *intel_get_pci_device(); extern int intel_register_access_init(struct pci_device *pci_dev, int safe); extern uint32_t intel_register_read(uint32_t reg); -extern uint32_t intel_register_write(uint32_t reg, uint32_t val); +extern void intel_register_write(uint32_t reg, uint32_t val); extern void intel_register_access_fini(); extern int intel_register_access_needs_fakewake(); extern unsigned short pcidev_to_devid(struct pci_device *pci_dev); diff --git a/tools/quick_dump/chipset_macro_wrap.c b/tools/quick_dump/chipset_macro_wrap.c index 392b85e..ee79777 100644 --- a/tools/quick_dump/chipset_macro_wrap.c +++ b/tools/quick_dump/chipset_macro_wrap.c @@ -1,3 +1,5 @@ +#include stdbool.h +#include stdlib.h #include pciaccess.h #include intel_chipset.h @@ -31,3 +33,15 @@ unsigned short pcidev_to_devid(struct pci_device *pdev) { return pdev-device_id; } + +bool igt_check_boolean_env_var(const char *env_var, bool default_value) +{ + char *val; + + val = getenv(env_var); + if (!val) + return default_value; + + return atoi(val) != 0; +} + -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Prevent context obj from being corrupted
While the context is not being used, we can make the PTEs invalid, so nothing can accidentally corrupt it. Systems tend to have a lot of trouble when the context gets corrupted. NOTE: This is a slightly different patch than what I posted to Bugzilla. References: https://bugs.freedesktop.org/show_bug.cgi?id=75724 Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_context.c | 56 + drivers/gpu/drm/i915/i915_reg.h | 2 +- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 6043062..4405a92 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -584,6 +584,58 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id) return ctx; } +static void +_ctx_ptes(struct intel_ring_buffer *ring, + struct i915_hw_context *ctx, + bool valid) +{ + const size_t ptes = ctx-obj-base.size PAGE_SHIFT; + const u32 base = i915_gem_obj_ggtt_offset(ctx-obj); + struct sg_page_iter sg_iter; + struct i915_address_space *vm = ctx-vm; + int i = 0; + + BUG_ON(!i915_gem_obj_is_pinned(ctx-obj)); + + if (intel_ring_begin(ring, round_up(ptes * 3, 2))) { + DRM_ERROR(Could not protect context object.\n); + return; + } + + for_each_sg_page(ctx-obj-pages-sgl, sg_iter, ctx-obj-pages-nents, 0) { + u32 pte = vm-pte_encode(sg_page_iter_dma_address(sg_iter), +ctx-obj-cache_level, +valid); + intel_ring_emit(ring, MI_UPDATE_GTT | (122)); + /* The docs contradict themselves on the offset. They say dword +* offset, yet the low 12 bits MBZ. */ + intel_ring_emit(ring, (base PAGE_MASK) + i); + intel_ring_emit(ring, pte); + i+=PAGE_SIZE; + } + + if (i PAGE_SHIFT) + intel_ring_emit(ring, MI_NOOP); + + intel_ring_advance(ring); +} + +static void +enable_ctx_ptes(struct intel_ring_buffer *ring, + struct i915_hw_context *ctx) +{ + if (INTEL_INFO(ring-dev)-gen 8) + _ctx_ptes(ring, ctx, true); +} + +static void +disable_ctx_ptes(struct intel_ring_buffer *ring, +struct i915_hw_context *ctx) +{ + if (INTEL_INFO(ring-dev)-gen 8) + _ctx_ptes(ring, ctx, false); +} + static inline int mi_set_context(struct intel_ring_buffer *ring, struct i915_hw_context *new_context, @@ -602,6 +654,8 @@ mi_set_context(struct intel_ring_buffer *ring, return ret; } + enable_ctx_ptes(ring, new_context); + ret = intel_ring_begin(ring, 6); if (ret) return ret; @@ -632,6 +686,8 @@ mi_set_context(struct intel_ring_buffer *ring, intel_ring_advance(ring); + disable_ctx_ptes(ring, new_context); + return ret; } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9f9e2b7..d536706 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -367,7 +367,7 @@ #define MI_TOPOLOGY_FILTER MI_INSTR(0x0D, 0) #define MI_LOAD_SCAN_LINES_EXCL MI_INSTR(0x13, 0) #define MI_URB_CLEARMI_INSTR(0x19, 0) -#define MI_UPDATE_GTT MI_INSTR(0x23, 0) +#define MI_UPDATE_GTT MI_INSTR(0x23, 1) #define MI_CLFLUSH MI_INSTR(0x27, 0) #define MI_REPORT_PERF_COUNTMI_INSTR(0x28, 0) #define MI_REPORT_PERF_COUNT_GGTT (10) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/12] drm/i915/bdw: Set initial rps freq to RP0
On Sat, Mar 22, 2014 at 09:06:00PM +, Chris Wilson wrote: On Sat, Mar 22, 2014 at 11:42:17AM -0700, Ben Widawsky wrote: On Thu, Mar 20, 2014 at 07:24:38AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky b...@bwidawsk.net Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea what that controls, nor its valid range. -Chris I have no reference for the video freq other than the brief mention in the programming guide, and know nothing more than you do about it. It's there because the original spec I had said to program it to 600MHz. The reason for /this/ patch was that I noticed the default values happened to be a *really* close to our max freq. and figured someone, somewhere might get a part whose lower, or upper bound precludes setting the constant provided in the programming guide. Interestingly, the programming guide has been updated since I originally wrote this patch to clearly indicate both of these registers need to be programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the valid range. Therefore, I think we should either proceed with this patch, or create a new patch to avoid writing it at all. The current code seems like the worst solution of all. If you want to argue we can drop the write to GEN6_RPNSWREQ since we do the correct thing after step 6: gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) 0xff00) 8); I wouldn't be too opposed. I was just trying to follow the spec as closely as possible, and it says to write the register value in this sequence, so I did. Let's mark that as Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk and move on. Though I may double check to see if I can find some information on the video frequency. -Chris Danvet if/when this is merged, can you please reword the subject: s/RP0/RP1 I'd say it was originally a typo, but that seems unlikely. -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/12] drm/i915/bdw: Implement a basic PM interrupt handler
Deepak, this patch can use review. If you have time, can you please take a look. There were some rebase conflicts from the last version, so please make sure to recheck carefully (since I think you did look before). Thanks. On Wed, Mar 19, 2014 at 06:31:17PM -0700, Ben Widawsky wrote: Almost all of it is reusable from the existing code. The primary difference is we need to do even less in the interrupt handler, since interrupts are not shared in the same way. The patch is mostly a copy-paste of the existing snb+ code, with updates to the relevant parts requiring changes to the interrupt handling. As such it /should/ be relatively trivial. It's highly likely that I missed some places where I need a gen8 version of the PM interrupts, but it has become invisible to me by now. This patch could probably be split into adding the new functions, followed by actually handling the interrupts. Since the code is currently disabled (and broken) I think the patch stands better by itself. v2: Move the commit about not touching the ringbuffer interrupt to the snb_* function where it belongs (Rodrigo) v3: Rebased on Paulo's runtime PM changes Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_irq.c | 84 +--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_pm.c | 39 ++- 4 files changed, 119 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4b4aeb3..2f9ec6e 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -175,6 +175,7 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv, return; } + /* Make sure not to corrupt PMIMR state used by ringbuffer code */ new_val = dev_priv-pm_irq_mask; new_val = ~interrupt_mask; new_val |= (~enabled_irq_mask interrupt_mask); @@ -214,6 +215,53 @@ static bool ivb_can_enable_err_int(struct drm_device *dev) return true; } +/** + * bdw_update_pm_irq - update GT interrupt 2 + * @dev_priv: driver private + * @interrupt_mask: mask of interrupt bits to update + * @enabled_irq_mask: mask of interrupt bits to enable + * + * Copied from the snb function, updated with relevant register offsets + */ +static void bdw_update_pm_irq(struct drm_i915_private *dev_priv, + uint32_t interrupt_mask, + uint32_t enabled_irq_mask) +{ + uint32_t new_val; + + assert_spin_locked(dev_priv-irq_lock); + + if (dev_priv-pm.irqs_disabled) { + WARN(1, IRQs disabled\n); + dev_priv-pm.regsave.gen6_pmimr = ~interrupt_mask; + dev_priv-pm.regsave.gen6_pmimr |= (~enabled_irq_mask + interrupt_mask); + return; + } + + new_val = dev_priv-pm_irq_mask; + new_val = ~interrupt_mask; + new_val |= (~enabled_irq_mask interrupt_mask); + + if (new_val != dev_priv-pm_irq_mask) { + dev_priv-pm_irq_mask = new_val; + I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) | +dev_priv-pm_irq_mask); + POSTING_READ(GEN8_GT_IMR(2)); + } +} + +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) +{ + bdw_update_pm_irq(dev_priv, mask, mask); +} + +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) +{ + bdw_update_pm_irq(dev_priv, mask, 0); +} + + static bool cpt_can_enable_serr_int(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; @@ -1131,13 +1179,16 @@ static void gen6_pm_rps_work(struct work_struct *work) spin_lock_irq(dev_priv-irq_lock); pm_iir = dev_priv-rps.pm_iir; dev_priv-rps.pm_iir = 0; - /* Make sure not to corrupt PMIMR state used by ringbuffer code */ - snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS); + if (IS_BROADWELL(dev_priv-dev)) + bdw_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS); + else { + snb_enable_pm_irq(dev_priv, GEN6_PM_RPS_EVENTS); + /* Make sure we didn't queue anything we're not going to + * process. */ + WARN_ON(pm_iir ~GEN6_PM_RPS_EVENTS); + } spin_unlock_irq(dev_priv-irq_lock); - /* Make sure we didn't queue anything we're not going to process. */ - WARN_ON(pm_iir ~GEN6_PM_RPS_EVENTS); - if ((pm_iir GEN6_PM_RPS_EVENTS) == 0) return; @@ -1330,6 +1381,19 @@ static void snb_gt_irq_handler(struct drm_device *dev, ivybridge_parity_error_irq_handler(dev, gt_iir); } +static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) +{ + if ((pm_iir GEN6_PM_RPS_EVENTS) == 0
Re: [Intel-gfx] [PATCH 22/48] drm/i915: Use drm_mm for PPGTT PDEs
On Thu, Mar 20, 2014 at 11:10:13AM +, Chris Wilson wrote: On Fri, Dec 06, 2013 at 02:11:55PM -0800, Ben Widawsky wrote: static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) { +#define GEN6_PD_ALIGN (PAGE_SIZE * 16) +#define GEN6_PD_SIZE (GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE) struct drm_device *dev = ppgtt-base.dev; struct drm_i915_private *dev_priv = dev-dev_private; - unsigned first_pd_entry_in_global_pt; - int i; - int ret = -ENOMEM; + int i, ret; - /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 -* entries. For aliasing ppgtt support we just steal them at the end for -* now. */ - first_pd_entry_in_global_pt = gtt_total_entries(dev_priv-gtt); + /* PPGTT PDEs reside in the GGTT and consists of 512 entries. The +* allocator works in address space sizes, so it's multiplied by page +* size. We allocate at the top of the GTT to avoid fragmentation. +*/ + BUG_ON(!drm_mm_initialized(dev_priv-gtt.base.mm)); + ret = drm_mm_insert_node_in_range_generic(dev_priv-gtt.base.mm, + ppgtt-node, GEN6_PD_SIZE, + GEN6_PD_ALIGN, 0, + 0, dev_priv-gtt.base.total, + DRM_MM_SEARCH_DEFAULT); This could use the simpler drm_mm_insert_node_generic(). -Chris Not with my [simple] workaround to not use offset 0, which Daniel reverted. I think he has some hope that we'll actually be able to figure out why we can't use offset 0 instead of just using the workaround. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 22/48] drm/i915: Use drm_mm for PPGTT PDEs
On Mon, Mar 24, 2014 at 07:45:56PM +, Chris Wilson wrote: On Mon, Mar 24, 2014 at 12:36:23PM -0700, Ben Widawsky wrote: On Thu, Mar 20, 2014 at 11:10:13AM +, Chris Wilson wrote: On Fri, Dec 06, 2013 at 02:11:55PM -0800, Ben Widawsky wrote: static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) { +#define GEN6_PD_ALIGN (PAGE_SIZE * 16) +#define GEN6_PD_SIZE (GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE) struct drm_device *dev = ppgtt-base.dev; struct drm_i915_private *dev_priv = dev-dev_private; - unsigned first_pd_entry_in_global_pt; - int i; - int ret = -ENOMEM; + int i, ret; - /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 -* entries. For aliasing ppgtt support we just steal them at the end for -* now. */ - first_pd_entry_in_global_pt = gtt_total_entries(dev_priv-gtt); + /* PPGTT PDEs reside in the GGTT and consists of 512 entries. The +* allocator works in address space sizes, so it's multiplied by page +* size. We allocate at the top of the GTT to avoid fragmentation. +*/ + BUG_ON(!drm_mm_initialized(dev_priv-gtt.base.mm)); + ret = drm_mm_insert_node_in_range_generic(dev_priv-gtt.base.mm, + ppgtt-node, GEN6_PD_SIZE, + GEN6_PD_ALIGN, 0, + 0, dev_priv-gtt.base.total, + DRM_MM_SEARCH_DEFAULT); This could use the simpler drm_mm_insert_node_generic(). -Chris Not with my [simple] workaround to not use offset 0, which Daniel reverted. I think he has some hope that we'll actually be able to figure out why we can't use offset 0 instead of just using the workaround. You can simply reduce the drm_mm range... -Chris Yeah, that's a better solution. Patches welcome? -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: fix up semaphore_waits_for
On Sat, Mar 22, 2014 at 06:52:25PM +0100, Daniel Vetter wrote: On Fri, Mar 21, 2014 at 07:33:59PM +0200, Mika Kuoppala wrote: Hi, Daniel Vetter daniel.vet...@ffwll.ch writes: There's an entire pile of issues in here: - Use the main RING_HEAD register, not ACTHD. ACTHD points at the gtt offset of the batch buffer when a batch is executed. Semaphores are always emitted to the main ring, so we always want to look at that. The ipehr check should make sure that we are at the ring and acthd should not be at batch. Regardless I agree that RING_HEAD is much more safer. When I have tried to get bottom of the snb reset hang, I have noticed that after reset the acthd is sometimes 0x0c even tho head is 0x00, on snb. Hm, should we maybe mask out the lower bits, too? If you can confirm this, can you please add a follow-up patch? - Mask the obtained HEAD pointer with the actual ring size, which is much smaller. Together with the above issue this resulted us in trying to dereference a pointer way outside of the ring mmio mapping. The resulting invalid access in interrupt context (hangcheck is executed from timers) lead to a full blown kernel panic. The fbcon panic handler then tried to frob our driver harder, resulting in a full machine hang at least on my snb here where I've stumbled over this. - Handle ring wrapping correctly and be a bit more explicit about how many dwords we're scanning. We probably should also scan more than just 4 ... ipehr dont update on MI_NOOPS and the head should point to the extra MI_NOOP after semaphore sequence. So 4 should be enough. Perhaps revisit this when bdw gains semaphores. Yeah, Chris also mentioned that the HEAD should point at one dword past the end of the ring, so should even work when there are no MI_NOOPs. In any case this code is more robust in case hw engineers suddenly change the behaviour. - Space out some of teh computations for readability. This prevents hard-hangs on my snb here. Verdict from QA is still pending, but the symptoms match. Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Cc: Chris Wilson ch...@chris-wilson.co.uk Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74100 The hard hangs are not so frequent with this patch but they are still there. This patch should take care of the oops we have been seeing, but there is still something else to be found until #74100 can be marked as fixed. Very often after reset, when igt has pushed the quietence batches into rings, blitter and video ring heads gets moved properly but all updates to hardware status page and to the sync registers are missing. And result is hang by hangcheck. Repeat enough times and it is a hard hang. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Please remove the blowup comment and also update the bugzilla tag. Yeah, QA also says that it doesn't fix the hard hangs, only seems to reduce them a bit on certain setups. I've updated the commit message. btw are you testing with FBDEV=n? The lack of a fbcon panic handler should greatly increase the chances that the last few message get correctly transmitted through other means like netconsole. Patches 1-2 and the followup one are Reviewed-by: Mika Kuoppala mika.kuopp...@intel.com Thanks for the review, patches merged. -Daniel Patch 2 was trivial to implement for gen8. This functionality is a lot less trivial. I volunteered to do patch 2, are you going to port this to gen8? -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Mon, Mar 24, 2014 at 04:14:32PM -0700, Ausmus, James wrote: On Sat, Mar 22, 2014 at 4:34 AM, Daniel Vetter dan...@ffwll.ch wrote: On Fri, Mar 21, 2014 at 05:51:01PM -0700, Ben Widawsky wrote: Let's try this again. I've pushed a branch here: http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=bdw-backports I need to re-review some of the merge conflicts for 4g GGTT, which I will try to do before Monday. Daniel: please make sure this is what you had in mind. I don't know where you want Cc: stable tags. We don't need cc: stable, we just need to submit it (since it has the upstream sha1s already, which is the requirement for stable patches). Cc: stable is only for when you want it to get backport anyway. Otherwise looks good. I dunno whether git cherry-pick can be told to use the sha1 reference layout Greg prefers or not, he uses This is sha1 in upstream. between the commit header and the actual commit message. But ime his scripts reformat your commit messages anyway. James: please test this as soon as possible. Once this is tested and we conclude it's sufficient to get bdw going on 3.14 without hilarity I think we should do a quick review on intel-gfx to check that the impact outside of bdw is indeed minimal. Then once drm-next has landed with all the referenced commits we can submit it to Greg with a small cover letter why we want this and that plan B would be to kill bdw in 3.14. This seems to be working well for me, with the one caveat that on boot and once per resume I'm hitting the WARN(!i915_preliminary_hw_support, GEN8_CENTROID_PIXEL_OPT_DIS not be needed for production) code in gen8_init_clock_gating - can that WARN be dropped via drm/i915: Don't use i915_preliminary_hw_support to mean pre-production ? Both with and without that patch added, the series is: Tested-by: James Ausmus james.aus...@intel.com Thanks. Daniel, the patch is added to my backports branch. I think given that that it removes a WARN which we know to be bogus, it's a good patch for stable. But it's your call. [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Allow full PPGTT with param override
When PPGTT was disabled by default, the patch also prevented the user from overriding this behavior via module parameter. Being able to test this on arbitrary kernels is extremely beneficial to track down the remaining bugs. The patch that prevented this was: commit 93a25a9e2d67765c3092bfaac9b855d95e39df97 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Thu Mar 6 09:40:43 2014 +0100 drm/i915: Disable full ppgtt by default By default PPGTT is set to -1. 0 means off, 1 means aliasing only, 2 means full, all other values are reserved. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bbc9420..9068628 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -50,7 +50,7 @@ bool intel_enable_ppgtt(struct drm_device *dev, bool full) /* Full ppgtt disabled by default for now due to issues. */ if (full) - return false; /* HAS_PPGTT(dev) */ + return HAS_PPGTT(dev) (i915.enable_ppgtt == 2); else return HAS_ALIASING_PPGTT(dev); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Enable FBC on GEN7 by default
On Mon, Mar 24, 2014 at 06:41:47PM -0700, Stéphane Marchesin wrote: On Mon, Mar 24, 2014 at 6:21 PM, Ben Widawsky benjamin.widaw...@linux.intel.com wrote: I am not clear why we've never enabled it by default for GEN7. Looking at the git hostiry, it seems Rodrigo disabled it by default, and it's never been turned on. Quite a few fixes have gone in over the past year, and I think many of us are running this successfully. Did you try with a high resolution panel? I could never get IVB FBC to be completely stable with a 2560x1700 panel... Stéphane 1600x900 is working fine. At my current laziness, that's the best I can test. I can try on a higher resolution tomorrow. Perhaps the solution would be to disable if the resolution goes above a certain point. Honestly, I assumed there was a reason it was disabled, I just couldn't find it. If there is some reason we know of why we don't enable this by default on GEN7, then please ignore the patch, and forgive my laziness. Cc: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a7da962..a9890df 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev) adjusted_mode = intel_crtc-config.adjusted_mode; if (i915.enable_fbc 0 - INTEL_INFO(dev)-gen = 7 !IS_HASWELL(dev)) { + INTEL_INFO(dev)-gen = 6) { if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT)) DRM_DEBUG_KMS(disabled per chip default\n); goto out_disable; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] [v2] drm/i915: Enable FBC on GEN7 by default
I am not clear why we've never enabled it by default for GEN7. Looking at the git hostiry, it seems Rodrigo disabled it by default, and it's never been turned on. Quite a few fixes have gone in over the past year, and I think many of us are running this successfully. If there is some reason we know of why we don't enable this by default on GEN7, then please ignore the patch, and forgive my laziness. v2: Disable FBC if width is greater than 2k, and user doesn't override. This is due to issues seen by Stéphane, and possibly others. I wasn't sure what to do with max_height. v2 was only compile tested. Cc: Stéphane Marchesin marc...@chromium.org Cc: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a7da962..2529cb8 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -512,7 +512,7 @@ void intel_update_fbc(struct drm_device *dev) adjusted_mode = intel_crtc-config.adjusted_mode; if (i915.enable_fbc 0 - INTEL_INFO(dev)-gen = 7 !IS_HASWELL(dev)) { + INTEL_INFO(dev)-gen = 6) { if (set_no_fbc_reason(dev_priv, FBC_CHIP_DEFAULT)) DRM_DEBUG_KMS(disabled per chip default\n); goto out_disable; @@ -530,7 +530,14 @@ void intel_update_fbc(struct drm_device *dev) goto out_disable; } - if (IS_G4X(dev) || INTEL_INFO(dev)-gen = 5) { + /* If the user hasn't overridden the defaults, try to get them working +* FBC on GEN7. If they have overridden the defaults, then assume they +* know what they're doing and allow foot shooting. +*/ + if (i915.enable_fbc 0 IS_GEN7(dev) !IS_HASWELL(dev)) { + max_width = 2048; + max_height = -1; + } else if (IS_G4X(dev) || INTEL_INFO(dev)-gen = 5) { max_width = 4096; max_height = 2048; } else { -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] rendercopy/gen8: Remove a hole in struct gen8_blend_state
On Mon, Mar 24, 2014 at 06:53:47PM +, Damien Lespiau wrote: Using uint64_t in that second member makes it aligned to 64bits, while the first member is only 32bits. We then had a 32bits hole in there! Found-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Rafael Barbalho rafael.barba...@intel.com Signed-off-by: Damien Lespiau damien.lesp...@intel.com I sent a mail 9 hours ago then never appeared to arrive. Hopefully it won't show up later. Recapping: lgtm - maybe a compile time assert on the struct would be a nice patch on top of this? Reviewed-by: Ben Widawsky b...@bwidawsk.net --- lib/gen8_render.h | 38 +++--- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/gen8_render.h b/lib/gen8_render.h index ca53d64..fffc100 100644 --- a/lib/gen8_render.h +++ b/lib/gen8_render.h @@ -273,25 +273,25 @@ struct gen8_blend_state { } bs0; struct { - uint64_t write_disable_blue:1; - uint64_t write_disable_green:1; - uint64_t write_disable_red:1; - uint64_t write_disable_alpha:1; - uint64_t pad1:1; - uint64_t alpha_blend_func:3; - uint64_t dest_alpha_blend_factor:5; - uint64_t source_alpha_blend_factor:5; - uint64_t color_blend_func:3; - uint64_t dest_blend_factor:5; - uint64_t source_blend_factor:5; - uint64_t color_buffer_blend:1; - uint64_t post_blend_color_clamp:1; - uint64_t pre_blend_color_clamp:1; - uint64_t color_clamp_range:2; - uint64_t pre_blend_source_only_clamp:1; - uint64_t pad0:22; - uint64_t logic_op_func:4; - uint64_t logic_op_enable:1; + uint32_t write_disable_blue:1; + uint32_t write_disable_green:1; + uint32_t write_disable_red:1; + uint32_t write_disable_alpha:1; + uint32_t pad1:1; + uint32_t alpha_blend_func:3; + uint32_t dest_alpha_blend_factor:5; + uint32_t source_alpha_blend_factor:5; + uint32_t color_blend_func:3; + uint32_t dest_blend_factor:5; + uint32_t source_blend_factor:5; + uint32_t color_buffer_blend:1; + uint32_t post_blend_color_clamp:1; + uint32_t pre_blend_color_clamp:1; + uint32_t color_clamp_range:2; + uint32_t pre_blend_source_only_clamp:1; + uint32_t pad0:22; + uint32_t logic_op_func:4; + uint32_t logic_op_enable:1; } bs[16]; }; -- 1.8.3.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote: As Broadwell has an increased virtual address size, it requires more than 32 bits to store offsets into its address space. This includes the debug registers to track the current HEAD of the individual rings, which may be anywhere within the per-process address spaces. In order to find the full location, we need to read the high bits from a second register. We then also need to expand our storage to keep track of the larger address. v2: Carefully read the two registers to catch wraparound between the reads. v3: Use a WARN_ON rather than loop indefinitely on an unstable register read. I truly feel bad for saying this at v3, but we can probably simplify this. Unless I am missing something, we only actually care about the value of acthd in: if (ring-hangcheck.acthd != acthd) return HANGCHECK_ACTIVE; I think therefore it would be safe to make hangcheck.acthd a u64. Compare the lower dword. If they're not equal, then we're done. If they are equal, compare the UDW. If UDW doesn't match, we're done. If UDW does match, do another read of the LDW and call it good: ring_stuck(u32 acthd) { if (lower_32_bits(ring-hangcheck.acthd) != acthd) return HANGCHECK_ACTIVE; else if (upper_32_bits(ring-hangcheck.acthd) != I915_READ(ACTHD_UDW)) return HANGCHECK_ACTIVE else if (acthd != I915_READ(RING_ACTHD)) return HANGCHECK_ACTIVE; } After writing that, I'm not really sure how much less complex it is, but I think it gets the same job done. Potentially there is some MMIO savings, but I'd guess it to be negligible. Jesse, please request ACTHD is expanded to a proper 64b register for gen1. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Timo Aaltonen tjaal...@ubuntu.com Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 13 - drivers/gpu/drm/i915/i915_gpu_error.c |2 +- drivers/gpu/drm/i915/i915_irq.c |8 +--- drivers/gpu/drm/i915/i915_reg.h |1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 22 -- drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++--- 6 files changed, 38 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5418253..4c09fb2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -354,12 +354,12 @@ struct drm_i915_error_state { u32 ipeir; u32 ipehr; u32 instdone; - u32 acthd; u32 bbstate; u32 instpm; u32 instps; u32 seqno; u64 bbaddr; + u64 acthd; u32 fault_reg; u32 faddr; u32 rc_psmi; /* sleep state */ @@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine); #define I915_WRITE64(reg, val) dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true) #define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv, (reg), true) +#define I915_READ64_2x32(lower_reg, upper_reg) ({\ + u32 upper = I915_READ(upper_reg); \ + u32 lower = I915_READ(lower_reg); \ + u32 tmp = I915_READ(upper_reg); \ + if (upper != tmp) { \ + upper = tmp;\ + lower = I915_READ(lower_reg); \ + WARN_ON(I915_READ(upper_reg) != upper); \ + } \ + (u64)upper 32 | lower; }) + #define POSTING_READ(reg)(void)I915_READ_NOTRACE(reg) #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index b153a16..9519aa2 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -248,7 +248,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, TAIL: 0x%08x\n, ring-tail); err_printf(m, CTL: 0x%08x\n, ring-ctl); err_printf(m, HWS: 0x%08x\n, ring-hws); - err_printf(m, ACTHD: 0x%08x\n, ring-acthd); + err_printf(m, ACTHD: 0x%08llx\n, ring-acthd); err_printf(m, IPEIR: 0x%08x\n, ring-ipeir); err_printf(m, IPEHR: 0x%08x\n, ring-ipehr); err_printf(m, INSTDONE: 0x%08x\n, ring-instdone); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 77dbef6..9caae98 100644 --- a/drivers/gpu/drm
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Mon, Mar 24, 2014 at 07:41:17PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote: As Broadwell has an increased virtual address size, it requires more than 32 bits to store offsets into its address space. This includes the debug registers to track the current HEAD of the individual rings, which may be anywhere within the per-process address spaces. In order to find the full location, we need to read the high bits from a second register. We then also need to expand our storage to keep track of the larger address. v2: Carefully read the two registers to catch wraparound between the reads. v3: Use a WARN_ON rather than loop indefinitely on an unstable register read. I truly feel bad for saying this at v3, but we can probably simplify this. Unless I am missing something, we only actually care about the value of acthd in: if (ring-hangcheck.acthd != acthd) return HANGCHECK_ACTIVE; I think therefore it would be safe to make hangcheck.acthd a u64. Compare the lower dword. If they're not equal, then we're done. If they are equal, compare the UDW. If UDW doesn't match, we're done. If UDW does match, do another read of the LDW and call it good: ring_stuck(u32 acthd) { if (lower_32_bits(ring-hangcheck.acthd) != acthd) return HANGCHECK_ACTIVE; else if (upper_32_bits(ring-hangcheck.acthd) != I915_READ(ACTHD_UDW)) return HANGCHECK_ACTIVE else if (acthd != I915_READ(RING_ACTHD)) return HANGCHECK_ACTIVE; } After writing that, I'm not really sure how much less complex it is, but I think it gets the same job done. Potentially there is some MMIO savings, but I'd guess it to be negligible. Jesse, please request ACTHD is expanded to a proper 64b register for gen1. Right after I sent that, I realized that doesn't provide for potentially corrupting ring-hangcheck.acthd. So I am back to thinking this method is better. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Timo Aaltonen tjaal...@ubuntu.com Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.h | 13 - drivers/gpu/drm/i915/i915_gpu_error.c |2 +- drivers/gpu/drm/i915/i915_irq.c |8 +--- drivers/gpu/drm/i915/i915_reg.h |1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 22 -- drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++--- 6 files changed, 38 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 5418253..4c09fb2 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -354,12 +354,12 @@ struct drm_i915_error_state { u32 ipeir; u32 ipehr; u32 instdone; - u32 acthd; u32 bbstate; u32 instpm; u32 instps; u32 seqno; u64 bbaddr; + u64 acthd; u32 fault_reg; u32 faddr; u32 rc_psmi; /* sleep state */ @@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine); #define I915_WRITE64(reg, val) dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true) #define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv, (reg), true) +#define I915_READ64_2x32(lower_reg, upper_reg) ({ \ + u32 upper = I915_READ(upper_reg); \ + u32 lower = I915_READ(lower_reg); \ + u32 tmp = I915_READ(upper_reg); \ + if (upper != tmp) { \ + upper = tmp;\ + lower = I915_READ(lower_reg); \ + WARN_ON(I915_READ(upper_reg) != upper); \ + } \ + (u64)upper 32 | lower; }) + #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg) #define POSTING_READ16(reg)(void)I915_READ16_NOTRACE(reg) diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index b153a16..9519aa2 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -248,7 +248,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, TAIL: 0x%08x\n, ring-tail); err_printf(m, CTL: 0x%08x\n, ring-ctl); err_printf(m, HWS: 0x%08x\n, ring-hws); - err_printf(m, ACTHD: 0x%08x\n, ring-acthd); + err_printf(m, ACTHD: 0x%08llx\n, ring-acthd); err_printf(m, IPEIR: 0x%08x\n
Re: [Intel-gfx] [PATCH 07/26] drm/i915: clean up PPGTT init error path
On Sat, Mar 22, 2014 at 08:58:29PM +, Chris Wilson wrote: On Sat, Mar 22, 2014 at 12:43:28PM -0700, Ben Widawsky wrote: On Tue, Mar 18, 2014 at 08:44:28AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:39PM -0700, Ben Widawsky wrote: The old code (I'm having trouble finding the commit) had a reason for doing things when there was an error, and would continue on, thus the !ret. For the newer code however, this looks completely silly. Follow the normal idiom of if (ret) return ret. Also, put the pde wiring in the gen specific init, now that GEN8 exists. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 1620211..5f73284 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1202,6 +1202,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt-pd_offset = ppgtt-node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t); + gen6_write_pdes(ppgtt); + ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); DRM_DEBUG_DRIVER(Allocated pde space (%ldM) at GTT entry: %lx\n, @@ -1226,20 +1228,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) else BUG(); - if (!ret) { - struct drm_i915_private *dev_priv = dev-dev_private; - kref_init(ppgtt-ref); - drm_mm_init(ppgtt-base.mm, ppgtt-base.start, - ppgtt-base.total); - i915_init_vm(dev_priv, ppgtt-base); - if (INTEL_INFO(dev)-gen 8) { - gen6_write_pdes(ppgtt); - DRM_DEBUG(Adding PPGTT at offset %x\n, - ppgtt-pd_offset 10); - } - } + if (ret) + return ret; - return ret; + kref_init(ppgtt-ref); + drm_mm_init(ppgtt-base.mm, ppgtt-base.start, ppgtt-base.total); + i915_init_vm(dev_priv, ppgtt-base); Didn't we just delete the dev_priv local variable? -Chris The important part is that the pde writes moved. (The DRM debug is also dropped). As for this code, I just wanted to get rid of the if (!ret) block. It looks weird. Maybe I didn't get what you're asking though. I was wondering if this patch compiles because of the removal of the dev_priv local variable. (Or if the original was a shadow.) -Chris Ah, of course. Yes, there was a shadowed dev_priv. I think it was merge/rebase fail either by myself or Daniel when the original patches were merged. -- Chris Wilson, Intel Open Source Technology Centre -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Split out GTT specific header file
On Sun, Mar 23, 2014 at 04:22:35PM +0100, Daniel Vetter wrote: On Sat, Mar 22, 2014 at 10:47:21PM -0700, Ben Widawsky wrote: This file contains all necessary defines, prototypes and typesdefs for manipulating GEN graphics address translation (this does not include the legacy AGP driver) Reiterating the comment in the header, Please try to maintain the following order within this file unless it makes sense to do otherwise. From top to bottom: 1. typedefs 2. #defines, and macros 3. structure definitions 4. function prototypes Within each section, please try to order by generation in ascending order, from top to bottom (ie. GEN6 on the top, GEN8 on the bottom). I've made some minor cleanups, and fixed a couple of typos while here - but there should be no functional changes. The purpose of the patch is to reduce clutter in our main header file, making room for new growth, and make documentation of our interfaces easier by splitting things out. With a little more work, like making i915_gtt a pointer, we could potentially completely isolate this header from i915_drv.h. At the moment however, I don't think it's worth the effort. Personally, I would have liked to put the PTE encoding functions in this file too, but I didn't want to rock the boat too much. Why would we split those out? They shouldn't be used outside of i915_gem_gtt.c, so a forward decl (if needed at all) should be all we need. iirc it was in the x86 header file, which I was trying to emulate as much as possible. In there case though, they have less platform specificity. Note that i915_gem_gtt.c is a pretty big mess with hw spcific stuff and generic code confusingly interleaved. I've had patches around to fix this up and remove all the forward decls and stuff back in Oct last year, but nuked them again. Yeah. It's actually not *that* bad. It's mostly hw specific stuff. We could potentially move a few things over to gem.c, and feel okay about it I think. A similar patch has been in use on my machine for some time. This exact patch though has only been compile tested. Signed-off-by: Ben Widawsky b...@bwidawsk.net Queued for -next, thanks for the patch. -Daniel Awesome, thanks. I'll rebase my stuff now. [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/28] more i-g-t docs and api polish
| 3 +- tools/intel_reg_snapshot.c | 5 +- tools/intel_reg_write.c| 3 +- tools/intel_stepping.c | 3 +- tools/intel_vga_read.c | 3 +- tools/intel_vga_write.c| 3 +- tools/quick_dump/Makefile.am | 6 +- tools/quick_dump/chipset_macro_wrap.c | 33 ++ tools/quick_dump/intel_chipset.c | 33 -- 202 files changed, 2401 insertions(+), 1398 deletions(-) create mode 100644 lib/igt_aux.c create mode 100644 lib/igt_aux.h create mode 100644 lib/intel_chipset.c delete mode 100644 lib/intel_drm.c delete mode 100644 lib/intel_gpu_tools.h create mode 100644 lib/intel_io.h create mode 100644 lib/intel_os.c delete mode 100644 lib/intel_pci.c delete mode 100644 lib/media_fill.c create mode 100644 tools/quick_dump/chipset_macro_wrap.c delete mode 100644 tools/quick_dump/intel_chipset.c -- 1.8.5.2 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Drop WARN_ON(flags) from ppgtt_bind_vma()
On Wed, Mar 12, 2014 at 07:32:26PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com We will call ppgtt_bind_vma() with flags != 0, so the WARN_ON(flags) is bogus. Kill it. This is not an appropriate commit message to change an invariant. The case was true, and it apparently no longer holds. At the very least the commit should have the SHA which changed the invariant, and preferably an explanation as to why the invariant no longer holds (is bogus). I The reason you have given to remove this WARN_ON can be used for any assertion we ever hit and simply reiterates what the patch does. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7727103..0dce6fc 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1243,8 +1243,6 @@ ppgtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags) { - WARN_ON(flags); - vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start, cache_level); } -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] lib: allow igt_skip_on_simulation outside of fixtures.
On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote: Thomas noticed that in simulation mode a lot of the tests fall over instead of skipping properly. This is due to recently added self-checks which ensure that any call to igt_skip happens either within a fixture or subtest block (or it's a simple test without subtests). This is to catch bugs since pretty much always not wrapping up hardware setup and checks into these blocks is a bug. Bug simulation skipping is a bit different, so allow that exception. But Otherwise we'd need to fix up piles of tests (and likely need to play a game of whack-a-mole). Also add a library testcase for all the different variants to make sure it really works. Cc: Thomas Wood thomas.w...@intel.com Cc: Ben Widawsky benjamin.widaw...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch You do realize you've just written a test for your test suite? Yo dawg, I heard you like tests I honestly don't know enough about the internals here to do a proper review in a minute, but sounds fine from the commit msg. Just one comment below and it's: Acked-by: Ben Widawsky b...@bwidawsk.net --- lib/igt_core.c | 12 - tests/.gitignore | 1 + tests/Makefile.sources | 1 + tests/igt_simulation.c | 139 + 4 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 tests/igt_simulation.c diff --git a/lib/igt_core.c b/lib/igt_core.c index bdace83ab382..dbfcd74f9160 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -1117,13 +1117,23 @@ bool igt_run_in_simulation(void) * * Skip tests when INTEL_SIMULATION environment variable is set. It uses * igt_skip() internally and hence is fully subtest aware. + * + * Note that in contrast to all other functions which use igt_skip() internally + * it is allowed to use this outside of an #igt_fixture block in a test with + * subtests. This is because in contrast to most other test requirements, + * checking for simulation mode doesn't depend upon the present hardware and it + * so makes a lot of sense to have this check in the outermost #igt_main block. */ void igt_skip_on_simulation(void) { if (igt_only_list_subtests()) return; - igt_require(!igt_run_in_simulation()); + if (!in_fixture) { + igt_fixture + igt_require(!igt_run_in_simulation()); + } else + igt_require(!igt_run_in_simulation()); } /* structured logging */ diff --git a/tests/.gitignore b/tests/.gitignore index 623a621ccace..60aa3b497f88 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -107,6 +107,7 @@ igt_list_only igt_no_exit igt_no_exit_list_only igt_no_subtest +igt_simulation kms_addfb kms_cursor_crc kms_fbc_crc diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 88866ac7ea75..8aeaac0ed15f 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -179,6 +179,7 @@ TESTS_testsuite = \ igt_fork_helper \ igt_list_only \ igt_no_subtest \ + igt_simulation \ $(NULL) TESTS = \ diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c new file mode 100644 index ..b9c6241d12e4 --- /dev/null +++ b/tests/igt_simulation.c @@ -0,0 +1,139 @@ +/* + * Copyright © 2014 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the Software), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Daniel Vetter daniel.vet...@ffwll.ch + * + */ + +#include stdlib.h +#include sys/wait.h +#include sys/types.h + +#include drmtest.h +#include igt_core.h + +bool simple; +bool list_subtests; +bool in_fixture; + +char test[] = test; +char list[] = --list-subtests; +char *argv_list[] = { test, list }; +char *argv_run[] = { test }; + +static int do_fork(void) +{ + int pid, status
Re: [Intel-gfx] [PATCH 01/26] drm/i915: Split out verbose PPGTT dumping
On Thu, Mar 20, 2014 at 12:08:00PM +, Chris Wilson wrote: On Thu, Mar 20, 2014 at 11:57:42AM +, Chris Wilson wrote: static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev, bool verbose) @@ -1838,14 +1841,11 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev, bool ver list_for_each_entry_reverse(file, dev-filelist, lhead) { struct drm_i915_file_private *file_priv = file-driver_priv; - struct i915_hw_ppgtt *pvt_ppgtt; - pvt_ppgtt = ctx_to_ppgtt(file_priv-private_default_ctx); seq_printf(m, proc: %s\n, get_pid_task(file-pid, PIDTYPE_PID)-comm); And seq_printf(m, \nproc: %s\n, for good measure - print_ppgtt(m, pvt_ppgtt, Default context); - if (verbose) - idr_for_each(file_priv-context_idr, per_file_ctx, m); + idr_for_each(file_priv-context_idr, per_file_ctx, +(void *)((unsigned long)m | verbose)); } } -- Thanks, I like it. I'm assuming you didn't want the count_pt_pages stuck in at this point (your diff was based on the end result)? I can do that if you prefer it. It seems pointless to me though. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/12] drm/i915/bdw: Set initial rps freq to RP0
On Thu, Mar 20, 2014 at 07:24:38AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:14PM -0700, Ben Widawsky wrote: Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky b...@bwidawsk.net Do you have a reference for GEN6_RC_VIDEO_FREQ? I still have no idea what that controls, nor its valid range. -Chris I have no reference for the video freq other than the brief mention in the programming guide, and know nothing more than you do about it. It's there because the original spec I had said to program it to 600MHz. The reason for /this/ patch was that I noticed the default values happened to be a *really* close to our max freq. and figured someone, somewhere might get a part whose lower, or upper bound precludes setting the constant provided in the programming guide. Interestingly, the programming guide has been updated since I originally wrote this patch to clearly indicate both of these registers need to be programmed between Rp1-Rp0. So I guess that means that Rp1-Rp0 is the valid range. Therefore, I think we should either proceed with this patch, or create a new patch to avoid writing it at all. The current code seems like the worst solution of all. If you want to argue we can drop the write to GEN6_RPNSWREQ since we do the correct thing after step 6: gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) 0xff00) 8); I wouldn't be too opposed. I was just trying to follow the spec as closely as possible, and it says to write the register value in this sequence, so I did. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915/bdw: Extract rp_state_caps logic
On Thu, Mar 20, 2014 at 07:28:29AM +, Chris Wilson wrote: On Wed, Mar 19, 2014 at 06:31:15PM -0700, Ben Widawsky wrote: We have a need for duplicated parsing of the RP_STATE_CAPS register (and the setting of the associated fields). To reuse some code, we can extract the function into a simple helper. This patch also addresses the fact that we missed doing this for gen8, something we should have done anyway. This could be two patches, one to extract, and one to add gen8, but it's trivial enough that I think one is fine. I will accept a request to split it. Please notice the fix addressed by v2 below. Valleyview is left untouched because it is different. v2: Logically rebased on top of commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac Author: Jeff McGee jeff.mc...@intel.com Date: Tue Feb 4 11:32:31 2014 -0600 drm/i915: Restore rps/rc6 on reset Note with the above change the fix for gen8 is also handled (which was not the case in Jeff's original patch). Signed-off-by: Ben Widawsky b...@bwidawsk.net Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk By setting max_freq_soft before querying overclocking frequencies, we force the user to have to manually raise the max freq through sysfs, right? Hasn't the user already explicitly asked for overclocking through the BIOS setting in the first place, so isn't that a needless burden upon the user? -Chris It's debatable, and if my memory serves, we've debated it before. Overclocking has a range. BIOS enables the user to select a value within that range. Selecting the highest possible value for the user is a policy decision (IMO). If BIOS/punit wanted to control this, it should set rp0 equal to the max overclock frequency, and not even bother letting the driver deal with it. By not selecting anything, we're not making any decision. Daniel, please notice he did put the r-b tag on it. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/26] drm/i915: clean up PPGTT init error path
On Tue, Mar 18, 2014 at 08:44:28AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:39PM -0700, Ben Widawsky wrote: The old code (I'm having trouble finding the commit) had a reason for doing things when there was an error, and would continue on, thus the !ret. For the newer code however, this looks completely silly. Follow the normal idiom of if (ret) return ret. Also, put the pde wiring in the gen specific init, now that GEN8 exists. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 22 +- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 1620211..5f73284 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1202,6 +1202,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt-pd_offset = ppgtt-node.start / PAGE_SIZE * sizeof(gen6_gtt_pte_t); + gen6_write_pdes(ppgtt); + ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); DRM_DEBUG_DRIVER(Allocated pde space (%ldM) at GTT entry: %lx\n, @@ -1226,20 +1228,14 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) else BUG(); - if (!ret) { - struct drm_i915_private *dev_priv = dev-dev_private; - kref_init(ppgtt-ref); - drm_mm_init(ppgtt-base.mm, ppgtt-base.start, - ppgtt-base.total); - i915_init_vm(dev_priv, ppgtt-base); - if (INTEL_INFO(dev)-gen 8) { - gen6_write_pdes(ppgtt); - DRM_DEBUG(Adding PPGTT at offset %x\n, - ppgtt-pd_offset 10); - } - } + if (ret) + return ret; - return ret; + kref_init(ppgtt-ref); + drm_mm_init(ppgtt-base.mm, ppgtt-base.start, ppgtt-base.total); + i915_init_vm(dev_priv, ppgtt-base); Didn't we just delete the dev_priv local variable? -Chris The important part is that the pde writes moved. (The DRM debug is also dropped). As for this code, I just wanted to get rid of the if (!ret) block. It looks weird. Maybe I didn't get what you're asking though. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/26] drm/i915: Split out gtt specific header file
On Tue, Mar 18, 2014 at 10:15:56AM +0100, Daniel Vetter wrote: On Mon, Mar 17, 2014 at 10:48:41PM -0700, Ben Widawsky wrote: TODO: Do header files need a copyright? Yup ;-) I like this though, especially since finer-grained files will make kerneldoc inclusion (well, grouped into sensible chapters at least) much simpler. -Daniel If I re-submit just this patch (with the copyright), will you merge it? It will make my life so much easier. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 162 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 57 - drivers/gpu/drm/i915/i915_gem_gtt.h | 225 3 files changed, 227 insertions(+), 217 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_gtt.h diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 084e82f..b19442c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -44,6 +44,8 @@ #include linux/kref.h #include linux/pm_qos.h +#include i915_gem_gtt.h + /* General customization: */ @@ -572,166 +574,6 @@ enum i915_cache_level { I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ }; -typedef uint32_t gen6_gtt_pte_t; - -/** - * A VMA represents a GEM BO that is bound into an address space. Therefore, a - * VMA's presence cannot be guaranteed before binding, or after unbinding the - * object into/from the address space. - * - * To make things as simple as possible (ie. no refcounting), a VMA's lifetime - * will always be = an objects lifetime. So object refcounting should cover us. - */ -struct i915_vma { - struct drm_mm_node node; - struct drm_i915_gem_object *obj; - struct i915_address_space *vm; - - /** This object's place on the active/inactive lists */ - struct list_head mm_list; - - struct list_head vma_link; /* Link in the object's VMA list */ - - /** This vma's place in the batchbuffer or on the eviction list */ - struct list_head exec_list; - - /** -* Used for performing relocations during execbuffer insertion. -*/ - struct hlist_node exec_node; - unsigned long exec_handle; - struct drm_i915_gem_exec_object2 *exec_entry; - - /** -* How many users have pinned this object in GTT space. The following -* users can each hold at most one reference: pwrite/pread, pin_ioctl -* (via user_pin_count), execbuffer (objects are not allowed multiple -* times for the same batchbuffer), and the framebuffer code. When -* switching/pageflipping, the framebuffer code has at most two buffers -* pinned per crtc. -* -* In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3 -* bits with absolutely no headroom. So use 4 bits. */ - unsigned int pin_count:4; -#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf - - /** Unmap an object from an address space. This usually consists of -* setting the valid PTE entries to a reserved scratch page. */ - void (*unbind_vma)(struct i915_vma *vma); - /* Map an object into an address space with the given cache flags. */ -#define GLOBAL_BIND (10) - void (*bind_vma)(struct i915_vma *vma, -enum i915_cache_level cache_level, -u32 flags); -}; - -struct i915_address_space { - struct drm_mm mm; - struct drm_device *dev; - struct list_head global_link; - unsigned long start;/* Start offset always 0 for dri2 */ - size_t total; /* size addr space maps (ex. 2GB for ggtt) */ - - struct { - dma_addr_t addr; - struct page *page; - } scratch; - - /** -* List of objects currently involved in rendering. -* -* Includes buffers having the contents of their GPU caches -* flushed, not necessarily primitives. last_rendering_seqno -* represents when the rendering involved will be completed. -* -* A reference is held on the buffer while on this list. -*/ - struct list_head active_list; - - /** -* LRU list of objects which are not in the ringbuffer and -* are ready to unbind, but are still in the GTT. -* -* last_rendering_seqno is 0 while an object is in this list. -* -* A reference is not held on the buffer while on this list, -* as merely being GTT-bound shouldn't prevent its being -* freed, and we'll pull it off the list in the free path. -*/ - struct list_head inactive_list; - - /* FIXME: Need a more generic return type */ - gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr, -enum i915_cache_level level, -bool valid); /* Create a valid PTE */ - void (*clear_range)(struct i915_address_space
Re: [Intel-gfx] [PATCH 14/26] drm/i915: Complete page table structures
On Tue, Mar 18, 2014 at 09:09:45AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:46PM -0700, Ben Widawsky wrote: Move the remaining members over to the new page table structures. This can be squashed with the previous commit if desire. The reasoning is the same as that patch. I simply felt it is easier to review if split. I'm not liking the shorter names much. Is there precedence elsewhere (e.g. daddr)? -Chris I'm not particularly attached to daddr. It was fun to say in my head. A lot of code does use daddr but it seems to vary between dma, device, data, destination Not exactly precedence. Initially I had the prefix p[td]_daddr, but I thought you might complain about it because it's implicit. dma_addr seemed kinda redundant to me. Recommendation? -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/26] drm/i915: Create page table allocators
On Tue, Mar 18, 2014 at 09:14:09AM +, Chris Wilson wrote: On Mon, Mar 17, 2014 at 10:48:47PM -0700, Ben Widawsky wrote: As we move toward dynamic page table allocation, it becomes much easier to manage our data structures if break do things less coarsely by breaking up all of our actions into individual tasks. This makes the code easier to write, read, and verify. Aside from the dissection of the allocation functions, the patch statically allocates the page table structures without a page directory. This remains the same for all platforms, The patch itself should not have much functional difference. The primary noticeable difference is the fact that page tables are no longer allocated, but rather statically declared as part of the page directory. This has non-zero overhead, but things gain non-trivial complexity as a result. We increase overhead for increased complexity. What's the selling point of this patch then? I'd argue about the complexity. Personally, I think the result is easier to read. I'll add this all to the commit message, but hopefully you agree: 1. Splitting out the functions allows easily combining GEN6 and GEN8 code. Page tables have no difference based on GEN8. As we'll see in a future patch when we add the dma mappings to the allocations, it requires only one small change to make work, and error handling should just fall into place. 2. Unless we always want to allocate all page tables under a given PDE, we'll have to eventually break this up into an array of pointers (or pointer to pointer). 3. Having the discrete functions is easier to review, and understand. All allocations and frees now take place in just a couple of locations. Reviewing, and cathcing leaks should be easy. 4. Less important: the gfp flags are confined to one location, which makes playing around with such things trivial.o Hopefully you're more convinced after you went through more of the patch series. If you want to try to optimize the overhead of managing the page tables, I think this is a worthy thing to do (for instance, not statically declaring the array). It takes a little more work though, and I'd prefer to do it after the code is doing what it's supposed to do. Otherwise, patch does as you say. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Split out GTT specific header file
This file contains all necessary defines, prototypes and typesdefs for manipulating GEN graphics address translation (this does not include the legacy AGP driver) Reiterating the comment in the header, Please try to maintain the following order within this file unless it makes sense to do otherwise. From top to bottom: 1. typedefs 2. #defines, and macros 3. structure definitions 4. function prototypes Within each section, please try to order by generation in ascending order, from top to bottom (ie. GEN6 on the top, GEN8 on the bottom). I've made some minor cleanups, and fixed a couple of typos while here - but there should be no functional changes. The purpose of the patch is to reduce clutter in our main header file, making room for new growth, and make documentation of our interfaces easier by splitting things out. With a little more work, like making i915_gtt a pointer, we could potentially completely isolate this header from i915_drv.h. At the moment however, I don't think it's worth the effort. Personally, I would have liked to put the PTE encoding functions in this file too, but I didn't want to rock the boat too much. A similar patch has been in use on my machine for some time. This exact patch though has only been compile tested. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 178 +-- drivers/gpu/drm/i915/i915_gem_gtt.c | 69 - drivers/gpu/drm/i915/i915_gem_gtt.h | 283 3 files changed, 286 insertions(+), 244 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_gem_gtt.h diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3f62be0..ef7e0ff 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -35,6 +35,7 @@ #include i915_reg.h #include intel_bios.h #include intel_ringbuffer.h +#include i915_gem_gtt.h #include linux/io-mapping.h #include linux/i2c.h #include linux/i2c-algo-bit.h @@ -572,168 +573,6 @@ enum i915_cache_level { I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ }; -typedef uint32_t gen6_gtt_pte_t; - -/** - * A VMA represents a GEM BO that is bound into an address space. Therefore, a - * VMA's presence cannot be guaranteed before binding, or after unbinding the - * object into/from the address space. - * - * To make things as simple as possible (ie. no refcounting), a VMA's lifetime - * will always be = an objects lifetime. So object refcounting should cover us. - */ -struct i915_vma { - struct drm_mm_node node; - struct drm_i915_gem_object *obj; - struct i915_address_space *vm; - - /** This object's place on the active/inactive lists */ - struct list_head mm_list; - - struct list_head vma_link; /* Link in the object's VMA list */ - - /** This vma's place in the batchbuffer or on the eviction list */ - struct list_head exec_list; - - /** -* Used for performing relocations during execbuffer insertion. -*/ - struct hlist_node exec_node; - unsigned long exec_handle; - struct drm_i915_gem_exec_object2 *exec_entry; - - /** -* How many users have pinned this object in GTT space. The following -* users can each hold at most one reference: pwrite/pread, pin_ioctl -* (via user_pin_count), execbuffer (objects are not allowed multiple -* times for the same batchbuffer), and the framebuffer code. When -* switching/pageflipping, the framebuffer code has at most two buffers -* pinned per crtc. -* -* In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3 -* bits with absolutely no headroom. So use 4 bits. */ - unsigned int pin_count:4; -#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf - - /** Unmap an object from an address space. This usually consists of -* setting the valid PTE entries to a reserved scratch page. */ - void (*unbind_vma)(struct i915_vma *vma); - /* Map an object into an address space with the given cache flags. */ -#define GLOBAL_BIND (10) - void (*bind_vma)(struct i915_vma *vma, -enum i915_cache_level cache_level, -u32 flags); -}; - -struct i915_address_space { - struct drm_mm mm; - struct drm_device *dev; - struct list_head global_link; - unsigned long start;/* Start offset always 0 for dri2 */ - size_t total; /* size addr space maps (ex. 2GB for ggtt) */ - - struct { - dma_addr_t addr; - struct page *page; - } scratch; - - /** -* List of objects currently involved in rendering. -* -* Includes buffers having the contents of their GPU caches -* flushed, not necessarily primitives. last_rendering_seqno -* represents when the rendering involved will be completed
Re: [Intel-gfx] [PATCH] drm/i915: release mutex in i915_gem_init()'s error path
On Tue, Feb 04, 2014 at 12:11:03PM +0100, Daniel Vetter wrote: On Fri, Jan 31, 2014 at 05:14:02PM +0200, Mika Kuoppala wrote: Found with smatch. Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com Both smatch patches merged to dinq, thanks. -Daniel CC stable? -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/12] drm/i915: Fix forcewake counts for gen8
From: Mika Kuoppala mika.kuopp...@linux.intel.com Sometimes generic driver code gets forcewake explicitly by gen6_gt_force_wake_get(), which check forcewake_count before accessing hardware. However the register access with gen8_write function access low level hw accessors directly, ignoring the forcewake_count. This leads to nested forcewake get from hardware, in ring init and possibly elsewhere, causing forcewake ack clear errors and/or hangs. Fix this by checking the forcewake count also in gen8_write v2: Read side doesn't care about shadowed registers, Remove __needs_put funkiness from gen8_write. (Ville) Improved commit message. References: https://bugs.freedesktop.org/show_bug.cgi?id=74007 Cc: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_uncore.c | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 87df68f..6df5ec4 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -634,16 +634,17 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg) #define __gen8_write(x) \ static void \ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \ - bool __needs_put = reg 0x4 !is_gen8_shadowed(dev_priv, reg); \ REG_WRITE_HEADER; \ - if (__needs_put) { \ - dev_priv-uncore.funcs.force_wake_get(dev_priv, \ - FORCEWAKE_ALL); \ - } \ - __raw_i915_write##x(dev_priv, reg, val); \ - if (__needs_put) { \ - dev_priv-uncore.funcs.force_wake_put(dev_priv, \ - FORCEWAKE_ALL); \ + if (reg 0x4 !is_gen8_shadowed(dev_priv, reg)) { \ + if (dev_priv-uncore.forcewake_count == 0) \ + dev_priv-uncore.funcs.force_wake_get(dev_priv, \ + FORCEWAKE_ALL); \ + __raw_i915_write##x(dev_priv, reg, val); \ + if (dev_priv-uncore.forcewake_count == 0) \ + dev_priv-uncore.funcs.force_wake_put(dev_priv, \ + FORCEWAKE_ALL); \ + } else { \ + __raw_i915_write##x(dev_priv, reg, val); \ } \ REG_WRITE_FOOTER; \ } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm/i915: Implement WaDisableSDEUnitClockGating:bdw
From: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 1240250..db70031 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4868,6 +4868,9 @@ #define GEN7_UCGCTL4 0x940c #define GEN7_L3BANK2X_CLOCK_GATE_DISABLE (125) +#define GEN8_UCGCTL6 0x9430 +#define GEN8_SDEUNIT_CLOCK_GATE_DISABLE (114) + #define GEN6_RPNSWREQ 0xA008 #define GEN6_TURBO_DISABLE (131) #define GEN6_FREQUENCY(x)((x)25) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 7b79c18..a0c6add 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4758,6 +4758,10 @@ static void gen8_init_clock_gating(struct drm_device *dev) I915_WRITE(GEN6_RC_SLEEP_PSMI_CONTROL, _MASKED_BIT_ENABLE(GEN8_RC_SEMA_IDLE_MSG_DISABLE)); + + /* WaDisableSDEUnitClockGating:bdw */ + I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) | + GEN8_SDEUNIT_CLOCK_GATE_DISABLE); } static void haswell_init_clock_gating(struct drm_device *dev) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/12] drm/i915/bdw: Use scratch page table for GEN8 PPGTT
From: Ben Widawsky benjamin.widaw...@intel.com I'm not clear if the hardware is still subject to the same prefetching issues that made us use a scratch page in the first place. In either case, we're using garbage with the current code (we will end up using offset 0). This may be the cause of our current gem_cpu_reloc regression with PPGTT. I cannot test it at the moment. Signed-off-by: Ben Widawsky b...@bwidawsk.net Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 40a2b36..8cc9398 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -359,6 +359,7 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm) **/ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) { + struct drm_i915_private *dev_priv = ppgtt-base.dev-dev_private; struct page *pt_pages; int i, j, ret = -ENOMEM; const int max_pdp = DIV_ROUND_UP(size, 1 30); @@ -389,6 +390,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) ppgtt-base.clear_range = gen8_ppgtt_clear_range; ppgtt-base.insert_entries = gen8_ppgtt_insert_entries; ppgtt-base.cleanup = gen8_ppgtt_cleanup; + ppgtt-base.scratch = dev_priv-gtt.base.scratch; ppgtt-base.start = 0; ppgtt-base.total = ppgtt-num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] drm/i915: don't flood the logs about bdw semaphores
From: Jani Nikula jani.nik...@intel.com BDW is no longer flagged as preliminary hw, but without i915.preliminary_hw_support module param set the logs are filled with WARNs about it. Just make semaphores off the BDW per-chip default for now. CC: Ben Widawsky b...@bwidawsk.net Reported-by: Sebastien Dufour sebastien.duf...@intel.com Signed-off-by: Jani Nikula jani.nik...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/i915_drv.c --- drivers/gpu/drm/i915/i915_drv.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ec7bb0f..8b5c305 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -477,13 +477,16 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) /* Until we get further testing... */ if (IS_GEN8(dev)) { - WARN_ON(!i915_preliminary_hw_support); return false; } if (i915_semaphores = 0) return i915_semaphores; + /* Until we get further testing... */ + if (IS_GEN8(dev)) + return false; + #ifdef CONFIG_INTEL_IOMMU /* Enable semaphores on SNB when IO remapping is off */ if (INTEL_INFO(dev)-gen == 6 intel_iommu_gfx_mapped) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/12] drm/i915: Add a partial instruction shootdown workaround on Broadwell.
From: Kenneth Graunke kenn...@whitecape.org I believe this will be necessary on production hardware. Signed-off-by: Kenneth Graunke kenn...@whitecape.org Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net [danvet: Fix whitespace fail spotted by checkpatch. Also add missing :bdw w/a tag that Ville spotted.] Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/intel_pm.c --- drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index db70031..cb9ff13 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5012,6 +5012,9 @@ #define GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE (110) #define GEN7_PSD_SINGLE_PORT_DISPATCH_ENABLE (13) +#define GEN8_ROW_CHICKEN 0xe4f0 +#define PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE(18) + #define GEN7_ROW_CHICKEN2 0xe4f4 #define GEN7_ROW_CHICKEN2_GT2 0xf4f4 #define DOP_CLOCK_GATING_DISABLE (10) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a0c6add..a7e5669 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4713,6 +4713,10 @@ static void gen8_init_clock_gating(struct drm_device *dev) WARN(!i915_preliminary_hw_support, GEN8_CENTROID_PIXEL_OPT_DIS not be needed for production\n); + /* WaDisablePartialInstShootdown:bdw */ + I915_WRITE(GEN8_ROW_CHICKEN, + _MASKED_BIT_ENABLE(PARTIAL_INSTRUCTION_SHOOTDOWN_DISABLE)); + I915_WRITE(HALF_SLICE_CHICKEN3, _MASKED_BIT_ENABLE(GEN8_CENTROID_PIXEL_OPT_DIS)); I915_WRITE(HALF_SLICE_CHICKEN3, -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/12] drm/i915: Do forcewake reset on gen8
From: Mika Kuoppala mika.kuopp...@intel.com When we get control from BIOS there might be mt forcewake bits already set. This causes us to do double mt get without proper clear/ack sequence. Fix this by clearing mt forcewake register on init, like we do with older gens. Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_uncore.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 6df5ec4..5f1762b 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -305,13 +305,13 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; - if (IS_VALLEYVIEW(dev)) { + if (IS_VALLEYVIEW(dev)) vlv_force_wake_reset(dev_priv); - } else if (INTEL_INFO(dev)-gen = 6) { + else if (IS_GEN6(dev) || IS_GEN7(dev)) __gen6_gt_force_wake_reset(dev_priv); - if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) - __gen6_gt_force_wake_mt_reset(dev_priv); - } + + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_GEN8(dev)) + __gen6_gt_force_wake_mt_reset(dev_priv); } void intel_uncore_early_sanitize(struct drm_device *dev) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/12] drm/i915/bdw: Restore PPAT on thaw
From: Ben Widawsky benjamin.widaw...@intel.com Apparently it is wiped out from under us, and we get some really fun caching artifacts upon resume (it seems to be WB for all types by default). Reported-by: James Ausmus james.aus...@intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net Tested-by: James Ausmus james.aus...@intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76113 Tested-by: Timo Aaltonen timo.aalto...@canonical.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/i915_gem_gtt.c --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 8cc9398..268e0d3 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -28,6 +28,8 @@ #include i915_trace.h #include intel_drv.h +static void gen8_setup_private_ppat(struct drm_i915_private *dev_priv); + #define GEN6_PPGTT_PD_ENTRIES 512 #define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t)) typedef uint64_t gen8_gtt_pte_t; @@ -865,6 +867,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) i915_gem_gtt_bind_object(obj, obj-cache_level); } + if (INTEL_INFO(dev)-gen = 8) + gen8_setup_private_ppat(dev_priv); + i915_gem_chipset_flush(dev); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/12] drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw
From: Ville Syrjälä ville.syrj...@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 31b36c5..2ac8f94 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -569,7 +569,7 @@ static int init_render_ring(struct intel_ring_buffer *ring) * to use MI_WAIT_FOR_EVENT within the CS. It should already be * programmed to '1' on all products. * -* WaDisableAsyncFlipPerfMode:snb,ivb,hsw,vlv +* WaDisableAsyncFlipPerfMode:snb,ivb,hsw,vlv,bdw */ if (INTEL_INFO(dev)-gen = 6) I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(ASYNC_FLIP_PERF_DISABLE)); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/12] drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM
From: Damien Lespiau damien.lesp...@intel.com While wandering in the spec, I noticed that BDW removes those 2 bits from INSTPM. I couldn't find any direct way to invalidate the TLB (ie without the ring working already). Maybe someone will be more lucky. At least, we now know we may be a problem. Signed-off-by: Damien Lespiau damien.lesp...@intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_ringbuffer.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 2ac8f94..c1ee5ef 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -977,8 +977,14 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring) I915_WRITE(mmio, (u32)ring-status_page.gfx_addr); POSTING_READ(mmio); - /* Flush the TLB for this page */ - if (INTEL_INFO(dev)-gen = 6) { + /* +* Flush the TLB for this page +* +* FIXME: These two bits have disappeared on gen8, so a question +* arises: do we still need this and if so how should we go about +* invalidating the TLB? +*/ + if (INTEL_INFO(dev)-gen = 6 INTEL_INFO(dev)-gen 8) { u32 reg = RING_INSTPM(ring-mmio_base); I915_WRITE(reg, _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE | -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/3] drm/i915: Use RMW to update chicken bits in gen7_enable_fbc()
On Wed, Mar 05, 2014 at 01:05:46PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com gen7_enable_fbc() may write to some registers which we've already touched, so use RMW so that we don't undo any previous updates. Also note that we implemnt WaFbcAsynchFlipDisableFbcQueue:bdw. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com CC stable? --- drivers/gpu/drm/i915/intel_pm.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 245d3ae..3411ad7 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -294,10 +294,13 @@ static void gen7_enable_fbc(struct drm_crtc *crtc) if (IS_IVYBRIDGE(dev)) { /* WaFbcAsynchFlipDisableFbcQueue:ivb */ - I915_WRITE(ILK_DISPLAY_CHICKEN1, ILK_FBCQ_DIS); + I915_WRITE(ILK_DISPLAY_CHICKEN1, +I915_READ(ILK_DISPLAY_CHICKEN1) | +ILK_FBCQ_DIS); } else { - /* WaFbcAsynchFlipDisableFbcQueue:hsw */ + /* WaFbcAsynchFlipDisableFbcQueue:hsw,bdw */ I915_WRITE(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc-pipe), + I915_READ(HSW_PIPE_SLICE_CHICKEN_1(intel_crtc-pipe)) | HSW_BYPASS_FBC_QUEUE); } -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] [v2] drm/i915: don't flood the logs about bdw semaphores
From: Jani Nikula jani.nik...@intel.com BDW is no longer flagged as preliminary hw, but without i915.preliminary_hw_support module param set the logs are filled with WARNs about it. Just make semaphores off the BDW per-chip default for now. v2: Spurious merge hunk leftover in v1 removed CC: Ben Widawsky b...@bwidawsk.net Reported-by: Sebastien Dufour sebastien.duf...@intel.com Signed-off-by: Jani Nikula jani.nik...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch [BDW 3.14 backport] Cc: sta...@vger.kernel.org Signed-off-by: Ben Widawsky b...@bwidawsk.net Conflicts: drivers/gpu/drm/i915/i915_drv.c --- drivers/gpu/drm/i915/i915_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ec7bb0f..8f16e11 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -476,10 +476,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev) return false; /* Until we get further testing... */ - if (IS_GEN8(dev)) { - WARN_ON(!i915_preliminary_hw_support); + if (IS_GEN8(dev)) return false; - } if (i915_semaphores = 0) return i915_semaphores; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Fri, Mar 21, 2014 at 08:49:35PM +0100, Daniel Vetter wrote: On Fri, Mar 21, 2014 at 7:48 PM, Ben Widawsky benjamin.widaw...@linux.intel.com wrote: The following patches are the backported simple fixes for 3.14. Some of these already had Cc: stable on them, but required conflict resolution which I've provided (presumably they canbe dropped if it's easier for upstream). There will be another series of backports which has fixes that require more than a single patch. I will not have a machine to test these on until Monday, but I am mailing them out now in case our QA can get it tested sooner. Ben Widawsky (2): drm/i915/bdw: Use scratch page table for GEN8 PPGTT drm/i915/bdw: Restore PPAT on thaw Damien Lespiau (1): drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Jani Nikula (1): drm/i915: don't flood the logs about bdw semaphores Kenneth Graunke (2): drm/i915: Add a partial instruction shootdown workaround on Broadwell. drm/i915: Add thread stall DOP clock gating workaround on Broadwell. Mika Kuoppala (2): drm/i915: Fix forcewake counts for gen8 drm/i915: Do forcewake reset on gen8 Ville Syrjälä (4): drm/i915: Disable semaphore wait event idle message on BDW drm/i915: Implement WaDisableSDEUnitClockGating:bdw drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW The stable team requires a reference to the sha1 of the upstream commit, which your patches seem to lack. git cherry-pick -x automatically adds that for you. I decided not to do this because in the git help it says, This is done only for cherry picks without conflicts. I believe only one of these patches didn't actually have a conflict (so I should have done it for that). So I will assume I should ignore this recommendation from the git help. I didn't want to make it seem like these patches did not have conflicts. Also please don't send out backports to stable if we still want to do some testing on them. Adding Greg and stable so he knows that he can bin this series for now. Of course all the patches in here which already have cc: stable in upstream should still go through the normal process (presuming they don't conflict ofc). But since most of these patches are from drm-intel-next we must wait anyway until drm-next has been merged into Linus' tree. Since you added Greg, I am curious - as noted in the cover letter, I've done the merge conflict resolution on the patches which already had Cc: stable. I didn't intentionally include any patches which already had Cc: stable and didn't require conflict resolution. Are those interesting/useful, should I drop them from the series? Thanks, Daniel drivers/gpu/drm/i915/i915_drv.c | 5 - drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++ drivers/gpu/drm/i915/i915_reg.h | 10 ++ drivers/gpu/drm/i915/intel_pm.c | 18 -- drivers/gpu/drm/i915/intel_ringbuffer.c | 12 +--- drivers/gpu/drm/i915/intel_uncore.c | 29 +++-- 6 files changed, 61 insertions(+), 20 deletions(-) -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Fri, Mar 21, 2014 at 04:47:05PM -0700, Greg KH wrote: On Fri, Mar 21, 2014 at 03:14:48PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 08:49:35PM +0100, Daniel Vetter wrote: On Fri, Mar 21, 2014 at 7:48 PM, Ben Widawsky benjamin.widaw...@linux.intel.com wrote: The following patches are the backported simple fixes for 3.14. Some of these already had Cc: stable on them, but required conflict resolution which I've provided (presumably they canbe dropped if it's easier for upstream). There will be another series of backports which has fixes that require more than a single patch. I will not have a machine to test these on until Monday, but I am mailing them out now in case our QA can get it tested sooner. Ben Widawsky (2): drm/i915/bdw: Use scratch page table for GEN8 PPGTT drm/i915/bdw: Restore PPAT on thaw Damien Lespiau (1): drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Jani Nikula (1): drm/i915: don't flood the logs about bdw semaphores Kenneth Graunke (2): drm/i915: Add a partial instruction shootdown workaround on Broadwell. drm/i915: Add thread stall DOP clock gating workaround on Broadwell. Mika Kuoppala (2): drm/i915: Fix forcewake counts for gen8 drm/i915: Do forcewake reset on gen8 Ville Syrjälä (4): drm/i915: Disable semaphore wait event idle message on BDW drm/i915: Implement WaDisableSDEUnitClockGating:bdw drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW The stable team requires a reference to the sha1 of the upstream commit, which your patches seem to lack. git cherry-pick -x automatically adds that for you. I decided not to do this because in the git help it says, This is done only for cherry picks without conflicts. I believe only one of these patches didn't actually have a conflict (so I should have done it for that). So I will assume I should ignore this recommendation from the git help. I didn't want to make it seem like these patches did not have conflicts. Also please don't send out backports to stable if we still want to do some testing on them. Adding Greg and stable so he knows that he can bin this series for now. Of course all the patches in here which already have cc: stable in upstream should still go through the normal process (presuming they don't conflict ofc). But since most of these patches are from drm-intel-next we must wait anyway until drm-next has been merged into Linus' tree. Since you added Greg, I am curious - as noted in the cover letter, I've done the merge conflict resolution on the patches which already had Cc: stable. I didn't intentionally include any patches which already had Cc: stable and didn't require conflict resolution. Are those interesting/useful, should I drop them from the series? I have no idea what is going on here, what this original email was from / about, or what I am supposed to do here... The stable patch process is pretty well defined, and documented, is that lacking somehow, and if so, in what? greg k-h My apologies, I didn't understand what Daniel had originally wanted from me, and I think the plan changed a bit in flight. I'm sorry you got dragged into it. The stable process documentation is perfectly adequate. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
On Fri, Mar 21, 2014 at 05:06:06PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 04:47:05PM -0700, Greg KH wrote: On Fri, Mar 21, 2014 at 03:14:48PM -0700, Ben Widawsky wrote: On Fri, Mar 21, 2014 at 08:49:35PM +0100, Daniel Vetter wrote: On Fri, Mar 21, 2014 at 7:48 PM, Ben Widawsky benjamin.widaw...@linux.intel.com wrote: The following patches are the backported simple fixes for 3.14. Some of these already had Cc: stable on them, but required conflict resolution which I've provided (presumably they canbe dropped if it's easier for upstream). There will be another series of backports which has fixes that require more than a single patch. I will not have a machine to test these on until Monday, but I am mailing them out now in case our QA can get it tested sooner. Ben Widawsky (2): drm/i915/bdw: Use scratch page table for GEN8 PPGTT drm/i915/bdw: Restore PPAT on thaw Damien Lespiau (1): drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Jani Nikula (1): drm/i915: don't flood the logs about bdw semaphores Kenneth Graunke (2): drm/i915: Add a partial instruction shootdown workaround on Broadwell. drm/i915: Add thread stall DOP clock gating workaround on Broadwell. Mika Kuoppala (2): drm/i915: Fix forcewake counts for gen8 drm/i915: Do forcewake reset on gen8 Ville Syrjälä (4): drm/i915: Disable semaphore wait event idle message on BDW drm/i915: Implement WaDisableSDEUnitClockGating:bdw drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW The stable team requires a reference to the sha1 of the upstream commit, which your patches seem to lack. git cherry-pick -x automatically adds that for you. I decided not to do this because in the git help it says, This is done only for cherry picks without conflicts. I believe only one of these patches didn't actually have a conflict (so I should have done it for that). So I will assume I should ignore this recommendation from the git help. I didn't want to make it seem like these patches did not have conflicts. Also please don't send out backports to stable if we still want to do some testing on them. Adding Greg and stable so he knows that he can bin this series for now. Of course all the patches in here which already have cc: stable in upstream should still go through the normal process (presuming they don't conflict ofc). But since most of these patches are from drm-intel-next we must wait anyway until drm-next has been merged into Linus' tree. Since you added Greg, I am curious - as noted in the cover letter, I've done the merge conflict resolution on the patches which already had Cc: stable. I didn't intentionally include any patches which already had Cc: stable and didn't require conflict resolution. Are those interesting/useful, should I drop them from the series? I have no idea what is going on here, what this original email was from / about, or what I am supposed to do here... The stable patch process is pretty well defined, and documented, is that lacking somehow, and if so, in what? greg k-h My apologies, I didn't understand what Daniel had originally wanted from me, and I think the plan changed a bit in flight. I'm sorry you got dragged into it. The stable process documentation is perfectly adequate. And if it wasn't clear, like Daniel said, please ignore these 12 patches for now. Sorry again. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/12] Broadwell 3.14 backports
Let's try this again. I've pushed a branch here: http://cgit.freedesktop.org/~bwidawsk/drm-intel/log/?h=bdw-backports I need to re-review some of the merge conflicts for 4g GGTT, which I will try to do before Monday. Daniel: please make sure this is what you had in mind. I don't know where you want Cc: stable tags. James: please test this as soon as possible. Thanks. On Fri, Mar 21, 2014 at 11:48:09AM -0700, Ben Widawsky wrote: The following patches are the backported simple fixes for 3.14. Some of these already had Cc: stable on them, but required conflict resolution which I've provided (presumably they canbe dropped if it's easier for upstream). There will be another series of backports which has fixes that require more than a single patch. I will not have a machine to test these on until Monday, but I am mailing them out now in case our QA can get it tested sooner. Ben Widawsky (2): drm/i915/bdw: Use scratch page table for GEN8 PPGTT drm/i915/bdw: Restore PPAT on thaw Damien Lespiau (1): drm/i915/bdw: The TLB invalidation mechanism has been removed from INSTPM Jani Nikula (1): drm/i915: don't flood the logs about bdw semaphores Kenneth Graunke (2): drm/i915: Add a partial instruction shootdown workaround on Broadwell. drm/i915: Add thread stall DOP clock gating workaround on Broadwell. Mika Kuoppala (2): drm/i915: Fix forcewake counts for gen8 drm/i915: Do forcewake reset on gen8 Ville Syrjälä (4): drm/i915: Disable semaphore wait event idle message on BDW drm/i915: Implement WaDisableSDEUnitClockGating:bdw drm/i915: We implement WaDisableAsyncFlipPerfMode:bdw drm/i915: Don't clobber CHICKEN_PIPESL_1 on BDW drivers/gpu/drm/i915/i915_drv.c | 5 - drivers/gpu/drm/i915/i915_gem_gtt.c | 7 +++ drivers/gpu/drm/i915/i915_reg.h | 10 ++ drivers/gpu/drm/i915/intel_pm.c | 18 -- drivers/gpu/drm/i915/intel_ringbuffer.c | 12 +--- drivers/gpu/drm/i915/intel_uncore.c | 29 +++-- 6 files changed, 61 insertions(+), 20 deletions(-) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Don't save/restore RS when not used
Cc: Kenneth Graunke kenn...@whitecape.org Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_context.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 185c926..ae4597c 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -588,6 +588,7 @@ mi_set_context(struct intel_ring_buffer *ring, struct i915_hw_context *new_context, u32 hw_flags) { + u32 flags = hw_flags | MI_MM_SPACE_GTT; int ret; /* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB @@ -601,6 +602,10 @@ mi_set_context(struct intel_ring_buffer *ring, return ret; } + /* These flags are for resource streamer on HSW+ */ + if (!IS_HASWELL(ring-dev) INTEL_INFO(ring-dev)-gen 8) + flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN); + ret = intel_ring_begin(ring, 6); if (ret) return ret; @@ -613,11 +618,8 @@ mi_set_context(struct intel_ring_buffer *ring, intel_ring_emit(ring, MI_NOOP); intel_ring_emit(ring, MI_SET_CONTEXT); - intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context-obj) | - MI_MM_SPACE_GTT | - MI_SAVE_EXT_STATE_EN | - MI_RESTORE_EXT_STATE_EN | - hw_flags); + intel_ring_emit(ring, + i915_gem_obj_ggtt_offset(new_context-obj) | flags); /* * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP * WaMiSetContext_Hang:snb,ivb,vlv -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Per-process stats work better when evaluated per-process
On Wed, Mar 19, 2014 at 01:45:45PM +, Chris Wilson wrote: The idea of printing objects used by each process is to judge how each process is using them. This means that we need to evaluate whether the object is bound for that particular process, rather than just whether it is bound into the global GTT. v2: Restore the non-full-ppgtt path for simplicity as we may not even create vma with older hardware. v3: Tweak handling of global entries and default context entries. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Reviewed-by: Ben Widawsky b...@bwidawsk.net [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/20] ILK+ interrupt improvements, v2
On Wed, Mar 19, 2014 at 09:36:04AM +0100, Daniel Vetter wrote: On Tue, Mar 18, 2014 at 01:53:53PM -0700, Ben Widawsky wrote: On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Hi This is basically a rebase of [PATCH 00/19] ILK+ interrupt improvements, which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new. Original cover letter: http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv-pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime! Thanks, Paulo Paulo Zanoni (20): drm/i915: add GEN5_IRQ_INIT macro drm/i915: also use GEN5_IRQ_INIT with south display interrupts drm/i915: use GEN8_IRQ_INIT on GEN5 drm/i915: add GEN5_IRQ_FINI drm/i915: don't forget to uninstall the PM IRQs drm/i915: properly clear IIR at irq_uninstall on Gen5+ drm/i915: add GEN5_IRQ_INIT drm/i915: check if IIR is still zero at postinstall on Gen5+ drm/i915: fix SERR_INT init/reset code drm/i915: fix GEN7_ERR_INT init/reset code drm/i915: fix open coded gen5_gt_irq_preinstall drm/i915: extract ibx_irq_uninstall drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall drm/i915: enable SDEIER later drm/i915: remove ibx_irq_uninstall drm/i915: add missing intel_hpd_irq_uninstall drm/i915: add ironlake_irq_reset drm/i915: add gen8_irq_reset drm/i915: only enable HWSTAM interrupts on postinstall on ILK+ drm/i915: add POSTING_READs to the IRQ init/reset macros drivers/gpu/drm/i915/i915_irq.c | 270 ++-- 1 file changed, 121 insertions(+), 149 deletions(-) Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P 1-5: Reviewed-by: Ben Widawsky b...@bwidawsk.net (with possible comment improvement on #3) 7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct: Reviewed-by: Ben Widawsky b...@bwidawsk.net 8: I'd really like to drop this one. Comment on this and I think with a pimped commit message this is good to go imo. I really think the added self-checks are required to start using this code for runtime pm. So you don't need my reviewed-by then. I don't like it... 9-10: Reviewed-by: Ben Widawsky b...@bwidawsk.net 12-13: I wouldn't mind cpt_irq_* rename, but either way Reviewed-by: Ben Widawsky b...@bwidawsk.net 14: With the requested change in the mail: Reviewed-by: Ben Widawsky b...@bwidawsk.net 16: Reviewed-by: Ben Widawsky b...@bwidawsk.net 20: Should be squashed, but Reviewed-by: Ben Widawsky b...@bwidawsk.net 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean disable. I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're: Reviewed-by: Ben Widawsky b...@bwidawsk.net Paulo's using reset functions/macros both in the preinstall hooks and in the uninstall/disable code. We already use reset for stuff run before init/enable code to get the hw in a state we expect it to, so I think Paulo's naming choice is accurate and a plain disable more misleading. I cannot disagree more. Every time I read reset it confuses me. But it seems like I am the minority. I think you raise some good points in your review, and besides the 3 cases I commented on I lack the detailed knowledge to avoid looking like a fool ;-) So I think I'll wait for Paulo's comments before pulling this all in. Thanks, Daniel Once Paulo responds, I'll make it a top priority to re-review whatever is needed. Sorry for the original delay. -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+
On Wed, Mar 19, 2014 at 09:28:32AM +0100, Daniel Vetter wrote: On Tue, Mar 18, 2014 at 11:20:09AM -0700, Ben Widawsky wrote: On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Instead of trying to clear it again. It should already be masked and disabled and zeroed at preinstall/uninstall. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 32 +++- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6d4daf2..4d0a8b1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ I915_WRITE(type##IIR, 0x); \ } while (0) +/* + * We should clear IMR at preinstall/uninstall, and just check at postinstall. + */ +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \ + u32 val = I915_READ(reg); \ + if (val) \ + DRM_ERROR(Interrupt register 0x%x is not zero: 0x%08x\n, \ + (reg), val); \ +} while (0) + #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \ + GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \ I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \ I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \ } while (0) #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \ + GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \ I915_WRITE(type##IMR, (imr_val)); \ I915_WRITE(type##IER, (ier_val)); \ } while (0) Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ which gives an error that we can do nothing about other than clear it anyway. I'd be in favor of dropping this patch. The point of the assert is to make sure that the new IIR clearing logic with blocking everything+clearing in the preinstall hook actually does what it's supposed to do. Since the point of this exercise is to reuse this code for runtime suspend/resume where races are much easier to hit I think this is a good self-check of the code. -Daniel Okay, I am feeling somewhat pressured to stick a reviewed-by on this since Daniel likes it. Change the macro to WARN instead of DRM_ERROR, and, clear the IIR if it's non-zero. With that change, it's: Reviewed-by-with-reservations: Ben Widawsky b...@bwidawsk.net -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: bdw expands ACTHD to 64bit
On Wed, Mar 19, 2014 at 09:54:48PM +, Chris Wilson wrote: As Broadwell has an increased virtual address size, it requires more than 32 bits to store offsets into its address space. This includes the debug registers to track the current HEAD of the individual rings, which may be anywhere within the per-process address spaces. In order to find the full location, we need to read the high bits from a second register. We then also need to expand our storage to keep track of the larger address. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Cc: Timo Aaltonen tjaal...@ubuntu.com --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gpu_error.c | 2 +- drivers/gpu/drm/i915/i915_irq.c | 8 +--- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 21 +++-- drivers/gpu/drm/i915/intel_ringbuffer.h | 6 +++--- 6 files changed, 26 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ed67b4abf9e3..ee913b63a945 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -354,12 +354,12 @@ struct drm_i915_error_state { u32 ipeir; u32 ipehr; u32 instdone; - u32 acthd; u32 bbstate; u32 instpm; u32 instps; u32 seqno; u64 bbaddr; + u64 acthd; u32 fault_reg; u32 faddr; u32 rc_psmi; /* sleep state */ diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index b153a16ead0a..9519aa240614 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -248,7 +248,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m, err_printf(m, TAIL: 0x%08x\n, ring-tail); err_printf(m, CTL: 0x%08x\n, ring-ctl); err_printf(m, HWS: 0x%08x\n, ring-hws); - err_printf(m, ACTHD: 0x%08x\n, ring-acthd); + err_printf(m, ACTHD: 0x%08llx\n, ring-acthd); %016x? if (gen8) %016x ? err_printf(m, IPEIR: 0x%08x\n, ring-ipeir); err_printf(m, IPEHR: 0x%08x\n, ring-ipehr); err_printf(m, INSTDONE: 0x%08x\n, ring-instdone); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 1dd9d3628919..b79792317f39 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2507,7 +2507,8 @@ static struct intel_ring_buffer * semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno) { struct drm_i915_private *dev_priv = ring-dev-dev_private; - u32 cmd, ipehr, acthd, acthd_min; + u64 acthd, acthd_min; + u32 cmd, ipehr; ipehr = I915_READ(RING_IPEHR(ring-mmio_base)); if ((ipehr ~(0x3 16)) != @@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv) } static enum intel_ring_hangcheck_action -ring_stuck(struct intel_ring_buffer *ring, u32 acthd) +ring_stuck(struct intel_ring_buffer *ring, u64 acthd) { struct drm_device *dev = ring-dev; struct drm_i915_private *dev_priv = dev-dev_private; @@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data) return; for_each_ring(ring, dev_priv, i) { - u32 seqno, acthd; + u64 acthd; + u32 seqno; bool busy = true; semaphore_clear_deadlocks(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f010ff7e7e2a..3c464d307a2b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -708,6 +708,7 @@ enum punit_power_well { #define BLT_HWS_PGA_GEN7 (0x04280) #define VEBOX_HWS_PGA_GEN7 (0x04380) #define RING_ACTHD(base) ((base)+0x74) +#define RING_ACTHD_UDW(base) ((base)+0x5c) #define RING_NOPID(base) ((base)+0x94) #define RING_IMR(base) ((base)+0xa8) #define RING_TIMESTAMP(base) ((base)+0x358) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 7a01911c16f8..a6ceb2c6f36d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -417,13 +417,19 @@ static void ring_write_tail(struct intel_ring_buffer *ring, I915_WRITE_TAIL(ring, value); } -u32 intel_ring_get_active_head(struct intel_ring_buffer *ring) +u64 intel_ring_get_active_head(struct intel_ring_buffer *ring) { drm_i915_private_t *dev_priv = ring-dev-dev_private; - u32 acthd_reg = INTEL_INFO(ring-dev)-gen = 4 ? - RING_ACTHD(ring-mmio_base) : ACTHD; - return I915_READ(acthd_reg); + u32 reg = (INTEL_INFO(ring-dev
Re: [Intel-gfx] [PATCH 11/11] [v4] drm/i915/bdw: Ensure a context is loaded before RC6
On Tue, Mar 04, 2014 at 03:30:14PM +0100, Daniel Vetter wrote: On Wed, Feb 19, 2014 at 10:27:20PM -0800, Ben Widawsky wrote: RC6 works a lot like HW contexts in that when the GPU enters RC6 it saves away the state to a context, and loads it upon wake. It's to be somewhat expected that BIOS will not set up valid GPU state. As a result, if loading bad state can cause the GPU to get angry, it would make sense then that we need to load state first. There are two ways in which we can do this: 1. Create 3d state in the driver, load it up, then enable RC6. 1b. Reuse a known good state, [and if needed,] just bind objects where needed. Then enable RC6. 2. Hold off enabling RC6 until userspace has had a chance to complete batches. There has been discussions in the past with #1 as it has been recommended for fixes elsewhere. I'm not opposed to it, I'd just like to do the easy thing now to enable the platform. This patch is a hack that implement option #2. It will defer enabling rc6 until the first batch from userspace has been retired. It suffers two flaws. The first is, if the driver is loaded, but a batch is not submitted/completed, we'll never enter rc6. The other is, it expects userspace to submit a batch with 3d state first. Both of these things are not actual flaws for most users because most users will boot to a graphical composited desktop. Both mesa, and X will always emit the necessary 3d state. Once a context is loaded and we enable rc6, the default context should inherit the proper state because we always inhibit the restore for the default context. This assumes certain things about the workaround/issue itself to which I am not privy (primarily that the indirect state objects don't actually need to exist). With that, there are currently 4 options for BDW: 1. Don't use RC6. 2. Use RC6 and expect a hang on the first batch submitted for every context. 3. Use RC6 and use this patch. 4. Wait for another workaround implementation. NOTE: This patch could be used against other platforms as well. v2: Re-add accidentally dropped hunk (Ben) v3: Now more compilable (Ben) v4: Use the existing enable flag for rc6. This will also make the suspend/resume case work properly, which is broken in v3. Disable rc6 on reset, and defer re-enabling until the first batch. The fact that RC6 residency continues to increment, and that this patch prevents a hang on BDW silicon has been: Tested-by: Kenneth Graunke kenn...@whitecape.org (v1) Cc: David E. Box david.e@intel.com Cc: Kristen Carlson Accardi kris...@linux.intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net squash! drm/i915/bdw: Ensure a context is loaded before RC6 --- drivers/gpu/drm/i915/i915_drv.c | 4 +++- drivers/gpu/drm/i915/i915_gem.c | 10 ++ drivers/gpu/drm/i915/intel_display.c | 5 + 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2d05d7c..7fdfc0e 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -679,6 +679,8 @@ int i915_reset(struct drm_device *dev) mutex_lock(dev-struct_mutex); i915_gem_reset(dev); + if (IS_BROADWELL(dev)) + intel_disable_gt_powersave(dev); simulated = dev_priv-gpu_error.stop_rings != 0; @@ -733,7 +735,7 @@ int i915_reset(struct drm_device *dev) * reset and the re-install of drm irq. Skip for ironlake per * previous concerns that it doesn't respond well to some forms * of re-init after reset. */ - if (INTEL_INFO(dev)-gen 5) { + if (INTEL_INFO(dev)-gen 5 !IS_BROADWELL(dev)) { mutex_lock(dev-struct_mutex); intel_enable_gt_powersave(dev); mutex_unlock(dev-struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 3618bb0..25a97a6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2420,6 +2420,7 @@ void i915_gem_reset(struct drm_device *dev) void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring) { + struct drm_i915_private *dev_priv = ring-dev-dev_private; uint32_t seqno; if (list_empty(ring-request_list)) @@ -2443,6 +2444,15 @@ i915_gem_retire_requests_ring(struct intel_ring_buffer *ring) if (!i915_seqno_passed(seqno, obj-last_read_seqno)) break; + /* Wa: can't find the w/a name. +* This doesn't actually implement the w/a, but it a workaround +* for the workaround. It defers using rc6 until we know valid +* state exists. +*/ + if (IS_BROADWELL(ring-dev) intel_enable_rc6(ring-dev
[Intel-gfx] [PATCH 05/12] drm/i915: Remove extraneous MMIO for RPS
The values created at initialization must always exist to use the interface. Reading them again is confusing, and pointless. More cleanups are coming in the next patch. Since I am not 100% certain, moreover on BYT, (though I am extremely close to that) that there is no need to leave the MMIO here, I wanted to make it a separate patch for the bisectable 'just-in-case' Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_sysfs.c | 21 + 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index e3fa8cd..49554d9 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -313,7 +313,7 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; - u32 val, rp_state_cap, hw_max, hw_min, non_oc_max; + u32 val, hw_max, hw_min, non_oc_max; ssize_t ret; ret = kstrtou32(buf, 0, val); @@ -327,16 +327,14 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, if (IS_VALLEYVIEW(dev_priv-dev)) { val = vlv_freq_opcode(dev_priv, val); - hw_max = valleyview_rps_max_freq(dev_priv); - hw_min = valleyview_rps_min_freq(dev_priv); - non_oc_max = hw_max; + non_oc_max = hw_max = dev_priv-rps.max_freq; + hw_min = dev_priv-rps.min_freq; } else { val /= GT_FREQUENCY_MULTIPLIER; - rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); hw_max = dev_priv-rps.max_freq; - non_oc_max = (rp_state_cap 0xff); - hw_min = ((rp_state_cap 0xff) 16); + non_oc_max = dev_priv-rps.rp0_freq; + hw_min = dev_priv-rps.min_freq; } if (val hw_min || val hw_max || @@ -394,7 +392,7 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; - u32 val, rp_state_cap, hw_max, hw_min; + u32 val, hw_max, hw_min; ssize_t ret; ret = kstrtou32(buf, 0, val); @@ -408,14 +406,13 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, if (IS_VALLEYVIEW(dev)) { val = vlv_freq_opcode(dev_priv, val); - hw_max = valleyview_rps_max_freq(dev_priv); - hw_min = valleyview_rps_min_freq(dev_priv); + hw_max = dev_priv-rps.max_freq; + hw_min = dev_priv-rps.min_freq; } else { val /= GT_FREQUENCY_MULTIPLIER; - rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); hw_max = dev_priv-rps.max_freq; - hw_min = ((rp_state_cap 0xff) 16); + hw_min = dev_priv-rps.min_freq; } if (val hw_min || val hw_max || val dev_priv-rps.max_freq_softlimit) { -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/12] drm/i915/bdw: RPS frequency bits are the same as HSW
Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index ab9e992..9486396 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3028,7 +3028,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val) gen6_set_rps_thresholds(dev_priv, val); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) I915_WRITE(GEN6_RPNSWREQ, HSW_FREQUENCY(val)); else -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/12] drm/i915: Reorganize the overclock code
The existing code (which I changed last) was very convoluted. I believe it was attempting to skip the overclock portion if the previous pcode write failed. When I last touched the code, I was preserving this behavior. There is some benefit to doing it that way in that if the first pcode access fails, the later is likely invalid. Having a bit more confidence in my understanding of how things work, I now feel it's better to have clear, readable, code than to try to skip over this one operation in an unusual case. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 20 +--- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 39f3238..dd3a121 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3326,7 +3326,7 @@ static void gen6_enable_rps(struct drm_device *dev) struct intel_ring_buffer *ring; u32 rp_state_cap, hw_max, hw_min; u32 gt_perf_status; - u32 rc6vids, pcu_mbox, rc6_mask = 0; + u32 rc6vids, pcu_mbox = 0, rc6_mask = 0; u32 gtfifodbg; int rc6_mode; int i, ret; @@ -3414,17 +3414,15 @@ static void gen6_enable_rps(struct drm_device *dev) I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10); ret = sandybridge_pcode_write(dev_priv, GEN6_PCODE_WRITE_MIN_FREQ_TABLE, 0); - if (!ret) { - pcu_mbox = 0; - ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, pcu_mbox); - if (!ret (pcu_mbox (131))) { /* OC supported */ - DRM_DEBUG_DRIVER(Overclocking supported. Max: %dMHz, Overclock max: %dMHz\n, -(dev_priv-rps.max_delay 0xff) * 50, -(pcu_mbox 0xff) * 50); - dev_priv-rps.hw_max = pcu_mbox 0xff; - } - } else { + if (ret) DRM_DEBUG_DRIVER(Failed to set the min frequency\n); + + ret = sandybridge_pcode_read(dev_priv, GEN6_READ_OC_PARAMS, pcu_mbox); + if (!ret (pcu_mbox (131))) { /* OC supported */ + DRM_DEBUG_DRIVER(Overclocking supported. Max: %dMHz, Overclock max: %dMHz\n, +(dev_priv-rps.max_delay 0xff) * 50, +(pcu_mbox 0xff) * 50); + dev_priv-rps.hw_max = pcu_mbox 0xff; } dev_priv-rps.power = HIGH_POWER; /* force a reset */ -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/12] drm/i915: remove rps local variables
With the renamed RPS struct members, it's easier to skip the local variables which no longer clarify anything, and if anything just make the code harder to read. The real motivation for this patch is actually the next patch, which attempts to consolidate some of the functionality. Cc: Jeff McGee jeff.mc...@intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_sysfs.c | 36 --- drivers/gpu/drm/i915/intel_pm.c | 40 --- 2 files changed, 33 insertions(+), 43 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 49554d9..9c57029 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -313,7 +313,7 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; - u32 val, hw_max, hw_min, non_oc_max; + u32 val; ssize_t ret; ret = kstrtou32(buf, 0, val); @@ -324,26 +324,19 @@ static ssize_t gt_max_freq_mhz_store(struct device *kdev, mutex_lock(dev_priv-rps.hw_lock); - if (IS_VALLEYVIEW(dev_priv-dev)) { + if (IS_VALLEYVIEW(dev_priv-dev)) val = vlv_freq_opcode(dev_priv, val); - - non_oc_max = hw_max = dev_priv-rps.max_freq; - hw_min = dev_priv-rps.min_freq; - } else { + else val /= GT_FREQUENCY_MULTIPLIER; - hw_max = dev_priv-rps.max_freq; - non_oc_max = dev_priv-rps.rp0_freq; - hw_min = dev_priv-rps.min_freq; - } - - if (val hw_min || val hw_max || + if (val dev_priv-rps.min_freq || + val dev_priv-rps.max_freq || val dev_priv-rps.min_freq_softlimit) { mutex_unlock(dev_priv-rps.hw_lock); return -EINVAL; } - if (val non_oc_max) + if (val dev_priv-rps.rp0_freq) DRM_DEBUG(User requested overclocking to %d\n, val * GT_FREQUENCY_MULTIPLIER); @@ -392,7 +385,7 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, struct drm_minor *minor = dev_to_drm_minor(kdev); struct drm_device *dev = minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; - u32 val, hw_max, hw_min; + u32 val; ssize_t ret; ret = kstrtou32(buf, 0, val); @@ -403,19 +396,14 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, mutex_lock(dev_priv-rps.hw_lock); - if (IS_VALLEYVIEW(dev)) { + if (IS_VALLEYVIEW(dev)) val = vlv_freq_opcode(dev_priv, val); - - hw_max = dev_priv-rps.max_freq; - hw_min = dev_priv-rps.min_freq; - } else { + else val /= GT_FREQUENCY_MULTIPLIER; - hw_max = dev_priv-rps.max_freq; - hw_min = dev_priv-rps.min_freq; - } - - if (val hw_min || val hw_max || val dev_priv-rps.max_freq_softlimit) { + if (val dev_priv-rps.min_freq || + val dev_priv-rps.max_freq || + val dev_priv-rps.max_freq_softlimit) { mutex_unlock(dev_priv-rps.hw_lock); return -EINVAL; } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3db7c40..fd68f93 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3324,7 +3324,7 @@ static void gen6_enable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; struct intel_ring_buffer *ring; - u32 rp_state_cap, hw_max, hw_min; + u32 rp_state_cap; u32 gt_perf_status; u32 rc6vids, pcu_mbox = 0, rc6_mask = 0; u32 gtfifodbg; @@ -3353,21 +3353,22 @@ static void gen6_enable_rps(struct drm_device *dev) gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS); /* All of these values are in units of 50MHz */ - dev_priv-rps.cur_freq = 0; - /* hw_max = RP0 until we check for overclocking */ - dev_priv-rps.max_freq = hw_max = rp_state_cap 0xff; + dev_priv-rps.cur_freq = 0; /* static values from HW: RP0 RPe RP1 RPn (min_freq) */ - dev_priv-rps.rp1_freq = (rp_state_cap 8) 0xff; - dev_priv-rps.rp0_freq = (rp_state_cap 0) 0xff; - dev_priv-rps.efficient_freq = dev_priv-rps.rp1_freq; - dev_priv-rps.min_freq = hw_min = (rp_state_cap 16) 0xff; + dev_priv-rps.rp1_freq = (rp_state_cap 8) 0xff; + dev_priv-rps.rp0_freq = (rp_state_cap 0) 0xff; + dev_priv-rps.min_freq = (rp_state_cap 16) 0xff; + /* XXX: only BYT has a special efficient freq */ + dev_priv-rps.efficient_freq= dev_priv-rps.rp1_freq; + /* hw_max = RP0 until
[Intel-gfx] [PATCH 04/12] drm/i915: Rename and comment all the RPS *stuff*
The names of the struct members for RPS are stupid. Every time I need to do anything in this code I have to spend a significant amount of time to remember what it all means. By renaming the variables (and adding the comments) I hope to clear up the situation. Indeed doing this make some upcoming patches more readable. I've avoided ILK because it's possible that the naming used for Ironlake matches what is in the docs. I believe the ILK power docs were never published, and I am too lazy to dig them up. v2: leave rp0, and rp1 in the names. It is useful to have these limits available at times. min_freq and max_freq (which may be equal to rp0, or rp1 depending on the platform) represent the actual HW min and max. Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_debugfs.c | 26 drivers/gpu/drm/i915/i915_drv.h | 26 +--- drivers/gpu/drm/i915/i915_irq.c | 25 drivers/gpu/drm/i915/i915_sysfs.c | 32 +- drivers/gpu/drm/i915/intel_pm.c | 118 ++-- 5 files changed, 120 insertions(+), 107 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 6037913..d1e0a36 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1026,7 +1026,7 @@ static int i915_cur_delayinfo(struct seq_file *m, void *unused) max_freq * GT_FREQUENCY_MULTIPLIER); seq_printf(m, Max overclocked frequency: %dMHz\n, - dev_priv-rps.hw_max * GT_FREQUENCY_MULTIPLIER); + dev_priv-rps.max_freq * GT_FREQUENCY_MULTIPLIER); } else if (IS_VALLEYVIEW(dev)) { u32 freq_sts, val; @@ -1498,8 +1498,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused) seq_puts(m, GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n); - for (gpu_freq = dev_priv-rps.min_delay; -gpu_freq = dev_priv-rps.max_delay; + for (gpu_freq = dev_priv-rps.min_freq_softlimit; +gpu_freq = dev_priv-rps.max_freq_softlimit; gpu_freq++) { ia_freq = gpu_freq; sandybridge_pcode_read(dev_priv, @@ -3449,9 +3449,9 @@ i915_max_freq_get(void *data, u64 *val) return ret; if (IS_VALLEYVIEW(dev)) - *val = vlv_gpu_freq(dev_priv, dev_priv-rps.max_delay); + *val = vlv_gpu_freq(dev_priv, dev_priv-rps.max_freq_softlimit); else - *val = dev_priv-rps.max_delay * GT_FREQUENCY_MULTIPLIER; + *val = dev_priv-rps.max_freq_softlimit * GT_FREQUENCY_MULTIPLIER; mutex_unlock(dev_priv-rps.hw_lock); return 0; @@ -3488,16 +3488,16 @@ i915_max_freq_set(void *data, u64 val) do_div(val, GT_FREQUENCY_MULTIPLIER); rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); - hw_max = dev_priv-rps.hw_max; + hw_max = dev_priv-rps.max_freq; hw_min = (rp_state_cap 16) 0xff; } - if (val hw_min || val hw_max || val dev_priv-rps.min_delay) { + if (val hw_min || val hw_max || val dev_priv-rps.min_freq_softlimit) { mutex_unlock(dev_priv-rps.hw_lock); return -EINVAL; } - dev_priv-rps.max_delay = val; + dev_priv-rps.max_freq_softlimit = val; if (IS_VALLEYVIEW(dev)) valleyview_set_rps(dev, val); @@ -3530,9 +3530,9 @@ i915_min_freq_get(void *data, u64 *val) return ret; if (IS_VALLEYVIEW(dev)) - *val = vlv_gpu_freq(dev_priv, dev_priv-rps.min_delay); + *val = vlv_gpu_freq(dev_priv, dev_priv-rps.min_freq_softlimit); else - *val = dev_priv-rps.min_delay * GT_FREQUENCY_MULTIPLIER; + *val = dev_priv-rps.min_freq_softlimit * GT_FREQUENCY_MULTIPLIER; mutex_unlock(dev_priv-rps.hw_lock); return 0; @@ -3569,16 +3569,16 @@ i915_min_freq_set(void *data, u64 val) do_div(val, GT_FREQUENCY_MULTIPLIER); rp_state_cap = I915_READ(GEN6_RP_STATE_CAP); - hw_max = dev_priv-rps.hw_max; + hw_max = dev_priv-rps.max_freq; hw_min = (rp_state_cap 16) 0xff; } - if (val hw_min || val hw_max || val dev_priv-rps.max_delay) { + if (val hw_min || val hw_max || val dev_priv-rps.max_freq_softlimit) { mutex_unlock(dev_priv-rps.hw_lock); return -EINVAL; } - dev_priv-rps.min_delay = val; + dev_priv-rps.min_freq_softlimit = val; if (IS_VALLEYVIEW(dev)) valleyview_set_rps(dev, val); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 241f5e1..c5c5760 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b
[Intel-gfx] [PATCH 07/12] drm/i915/bdw: Set initial rps freq to RP0
Programming it outside of the rp0-rp1 range is considered a programming error. Since we do not know that the previous value would actually be in the range, program something we've read from the hardware, and therefore know will work. This is potentially an issue for platforms whose ranges are outside the norms given in the programming guide (ie. early silicon) v2: Use RP1 instead of RPn Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_pm.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fd68f93..8a64ecc 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3285,8 +3285,10 @@ static void gen8_enable_rps(struct drm_device *dev) rc6_mask); /* 4 Program defaults and thresholds for RPS*/ - I915_WRITE(GEN6_RPNSWREQ, HSW_FREQUENCY(10)); /* Request 500 MHz */ - I915_WRITE(GEN6_RC_VIDEO_FREQ, HSW_FREQUENCY(12)); /* Request 600 MHz */ + I915_WRITE(GEN6_RPNSWREQ, + HSW_FREQUENCY(dev_priv-rps.rp1_freq)); + I915_WRITE(GEN6_RC_VIDEO_FREQ, + HSW_FREQUENCY(dev_priv-rps.rp1_freq)); /* NB: Docs say 1s, and 100 - which aren't equivalent */ I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1 / 128); /* 1 second timeout */ -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/12] drm/i915: Store the HW min frequency as min_freq
this leaves a temporarily awkward min_delay (the soft limit) with the new min_freq (the hardware limit). It's fixed in the next patch. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9cd870f..241f5e1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -987,6 +987,7 @@ struct intel_gen6_power_mgmt { u8 rp1_delay; u8 rp0_delay; u8 hw_max; + u8 min_freq; bool rp_up_masked; bool rp_down_masked; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index dd3a121..dd631d1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3354,7 +3354,7 @@ static void gen6_enable_rps(struct drm_device *dev) /* In units of 50MHz */ dev_priv-rps.hw_max = hw_max = rp_state_cap 0xff; - hw_min = (rp_state_cap 16) 0xff; + dev_priv-rps.min_freq = hw_min = (rp_state_cap 16) 0xff; dev_priv-rps.rp1_delay = (rp_state_cap 8) 0xff; dev_priv-rps.rp0_delay = (rp_state_cap 0) 0xff; dev_priv-rps.rpe_delay = dev_priv-rps.rp1_delay; -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] prime_mmap: Add new test for calling mmap() on dma-buf fds
the sum of which mappable aperture */ + uint64_t size1 = (gem_mappable_aperture_size() * 7) / 8; + uint64_t size2 = (gem_mappable_aperture_size() * 3) / 8; + + handle1 = gem_create(fd, size1); + fill_bo(handle1, BO_SIZE); + + dma_buf_fd1 = prime_handle_to_fd(fd, handle1); + igt_assert(errno == 0); + ptr1 = mmap(NULL, size1, PROT_READ, MAP_SHARED, dma_buf_fd1, 0); + igt_assert(ptr1 != MAP_FAILED); + igt_assert(pattern_check(ptr1, BO_SIZE) == 0); + + handle2 = gem_create(fd, size1); + fill_bo(handle2, BO_SIZE); + dma_buf_fd2 = prime_handle_to_fd(fd, handle2); + igt_assert(errno == 0); + ptr2 = mmap(NULL, size2, PROT_READ, MAP_SHARED, dma_buf_fd2, 0); + igt_assert(ptr2 != MAP_FAILED); + igt_assert(pattern_check(ptr2, BO_SIZE) == 0); + + igt_assert(memcmp(ptr1, ptr2, BO_SIZE) == 0); + + munmap(ptr1, size1); + munmap(ptr2, size2); + close(dma_buf_fd1); + close(dma_buf_fd2); + gem_close(fd, handle1); + gem_close(fd, handle2); +} + +static int +check_for_dma_buf_mmap(void) +{ + int dma_buf_fd; + char *ptr; + uint32_t handle; + int ret = 1; + + handle = gem_create(fd, BO_SIZE); + dma_buf_fd = prime_handle_to_fd(fd, handle); + ptr = mmap(NULL, BO_SIZE, PROT_READ, MAP_SHARED, dma_buf_fd, 0); + if (ptr != MAP_FAILED) + ret = 0; + munmap(ptr, BO_SIZE); + gem_close(fd, handle); + close (dma_buf_fd); + return ret; +} + +igt_main +{ + struct { + const char *name; + void (*fn)(void); + } tests[] = { + { test_correct, test_correct }, + { test_map_unmap, test_map_unmap }, + { test_reprime, test_reprime }, + { test_forked, test_forked }, + { test_refcounting, test_refcounting }, + { test_dup, test_dup }, + { test_errors, test_errors }, + { test_aperture_limit, test_aperture_limit }, + }; + int i; + + igt_fixture { + fd = drm_open_any(); + errno = 0; + } + + igt_skip_on((check_for_dma_buf_mmap() != 0)); + + for (i = 0; i ARRAY_SIZE(tests); i++) { + igt_subtest(tests[i].name) + tests[i].fn(); + } + + igt_fixture + close(fd); +} -- lgtm. Put the commit message in the test description and call it Reviewed-by: Ben Widawsky b...@bwidawsk.net I'm pretty impressed you used all the igt_*_foo. -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Print how many objects are shared in per-process statsg
On Wed, Mar 19, 2014 at 01:45:46PM +, Chris Wilson wrote: Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Any clue how you intend to use this for a commit message (I'm actually curious)? Also, the subject is wrong, you're counting size, not quantity. Anyhoo, looks correct. Reviewed-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_debugfs.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 4e1787ee8f37..9cc1c9360238 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -301,7 +301,9 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data) struct file_stats { struct drm_i915_file_private *file_priv; int count; - size_t total, global, active, inactive, unbound; + size_t total, unbound; + size_t global, shared; + size_t active, inactive; }; static int per_file_stats(int id, void *ptr, void *data) @@ -313,6 +315,9 @@ static int per_file_stats(int id, void *ptr, void *data) stats-count++; stats-total += obj-base.size; + if (obj-base.name || obj-base.dma_buf) + stats-shared += obj-base.size; + if (USES_FULL_PPGTT(obj-base.dev)) { list_for_each_entry(vma, obj-vma_list, vma_link) { struct i915_hw_ppgtt *ppgtt; @@ -450,13 +455,14 @@ static int i915_gem_object_info(struct seq_file *m, void* data) */ rcu_read_lock(); task = pid_task(file-pid, PIDTYPE_PID); - seq_printf(m, %s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu unbound)\n, + seq_printf(m, %s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu shared, %zu unbound)\n, task ? task-comm : unknown, stats.count, stats.total, stats.active, stats.inactive, stats.global, +stats.shared, stats.unbound); rcu_read_unlock(); } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/20] drm/i915: use GEN8_IRQ_INIT on GEN5
On Fri, Mar 07, 2014 at 08:10:19PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com And rename is to GEN5_IRQ_INIT. We have discussed doing equivalent changes on July 2013, and I even sent a patch series for this: [PATCH 00/15] Unify interrupt register init/reset. Now that the BDW code was merged, I have one more argument in favor of these changes. Here's what really changes with the Gen 5 IRQ init code: - We now clear the IIR registers at preinstall (they are also cleared at postinstall, but we will change that later). - We have an additional POSTING_READ at the IMR register. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 49 +++-- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 852844d..7be7da1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -80,12 +80,30 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS }; +/* + * IIR can theoretically queue up two events. Be paranoid. + * Also, make sure callers of these macros have something equivalent to a + * POSTING_READ on the IIR register. + * */ I don't understand what you mean in this comment. If you're always going to sending a posting read after the second IIR write, why not just put it in the macro? The reason it wasn't in my original macro is because we've done the posting read on IER, and IMR - so we're not going to get new interrupts. When the second IIR write lands is irrelevant. The POSTING_READ in between is to prevent the [probably impossible] case of the writes getting collapsed into one write. +#define GEN8_IRQ_INIT_NDX(type, which) do { \ + I915_WRITE(GEN8_##type##_IMR(which), 0x); \ + POSTING_READ(GEN8_##type##_IMR(which)); \ + I915_WRITE(GEN8_##type##_IER(which), 0); \ + I915_WRITE(GEN8_##type##_IIR(which), 0x); \ + POSTING_READ(GEN8_##type##_IIR(which)); \ + I915_WRITE(GEN8_##type##_IIR(which), 0x); \ +} while (0) + #define GEN5_IRQ_INIT(type) do { \ I915_WRITE(type##IMR, 0x); \ + POSTING_READ(type##IMR); \ I915_WRITE(type##IER, 0); \ - POSTING_READ(type##IER); \ + I915_WRITE(type##IIR, 0x); \ + POSTING_READ(type##IIR); \ + I915_WRITE(type##IIR, 0x); \ } while (0) + /* For display hotplug interrupt */ static void ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask) @@ -2789,6 +2807,7 @@ static void gen5_gt_irq_preinstall(struct drm_device *dev) GEN5_IRQ_INIT(GT); if (INTEL_INFO(dev)-gen = 6) GEN5_IRQ_INIT(GEN6_PM); + POSTING_READ(GTIIR); } /* drm_dma.h hooks @@ -2843,25 +2862,6 @@ static void gen8_irq_preinstall(struct drm_device *dev) I915_WRITE(GEN8_MASTER_IRQ, 0); POSTING_READ(GEN8_MASTER_IRQ); - /* IIR can theoretically queue up two events. Be paranoid */ -#define GEN8_IRQ_INIT_NDX(type, which) do { \ - I915_WRITE(GEN8_##type##_IMR(which), 0x); \ - POSTING_READ(GEN8_##type##_IMR(which)); \ - I915_WRITE(GEN8_##type##_IER(which), 0); \ - I915_WRITE(GEN8_##type##_IIR(which), 0x); \ - POSTING_READ(GEN8_##type##_IIR(which)); \ - I915_WRITE(GEN8_##type##_IIR(which), 0x); \ - } while (0) - -#define GEN8_IRQ_INIT(type) do { \ - I915_WRITE(GEN8_##type##_IMR, 0x); \ - POSTING_READ(GEN8_##type##_IMR); \ - I915_WRITE(GEN8_##type##_IER, 0); \ - I915_WRITE(GEN8_##type##_IIR, 0x); \ - POSTING_READ(GEN8_##type##_IIR); \ - I915_WRITE(GEN8_##type##_IIR, 0x); \ - } while (0) - GEN8_IRQ_INIT_NDX(GT, 0); GEN8_IRQ_INIT_NDX(GT, 1); GEN8_IRQ_INIT_NDX(GT, 2); @@ -2871,12 +2871,9 @@ static void gen8_irq_preinstall(struct drm_device *dev) GEN8_IRQ_INIT_NDX(DE_PIPE, pipe); } - GEN8_IRQ_INIT(DE_PORT); - GEN8_IRQ_INIT(DE_MISC); - GEN8_IRQ_INIT(PCU); -#undef GEN8_IRQ_INIT -#undef GEN8_IRQ_INIT_NDX - + GEN5_IRQ_INIT(GEN8_DE_PORT_); + GEN5_IRQ_INIT(GEN8_DE_MISC_); + GEN5_IRQ_INIT(GEN8_PCU_); POSTING_READ(GEN8_PCU_IIR); ibx_irq_preinstall(dev); -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/20] drm/i915: properly clear IIR at irq_uninstall on Gen5+
); - } + for_each_pipe(pipe) + GEN8_IRQ_RESET_NDX(DE_PIPE, pipe); - GEN8_IRQ_FINI(DE_PORT); - GEN8_IRQ_FINI(DE_MISC); - GEN8_IRQ_FINI(PCU); -#undef GEN8_IRQ_FINI -#undef GEN8_IRQ_FINI_NDX + GEN5_IRQ_RESET(GEN8_DE_PORT_); + GEN5_IRQ_RESET(GEN8_DE_MISC_); + GEN5_IRQ_RESET(GEN8_PCU_); POSTING_READ(GEN8_PCU_IIR); } @@ -3309,18 +3288,19 @@ static void ironlake_irq_uninstall(struct drm_device *dev) I915_WRITE(HWSTAM, 0x); - GEN5_IRQ_FINI(DE); + GEN5_IRQ_RESET(DE); if (IS_GEN7(dev)) I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT)); - GEN5_IRQ_FINI(GT); + GEN5_IRQ_RESET(GT); if (INTEL_INFO(dev)-gen = 6) - GEN5_IRQ_FINI(GEN6_PM); + GEN5_IRQ_RESET(GEN6_PM); + POSTING_READ(GTIIR); if (HAS_PCH_NOP(dev)) return; - GEN5_IRQ_FINI(SDE); + GEN5_IRQ_RESET(SDE); if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev)) I915_WRITE(SERR_INT, I915_READ(SERR_INT)); } -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/20] drm/i915: don't forget to uninstall the PM IRQs
On Fri, Mar 07, 2014 at 08:10:21PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com It's the only thihg missing, apparently. s/thihg/thing There is a potential fixup in patch 3, but with or without, everything up through here is: Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index a9f173c..f681462 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3314,6 +3314,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev) I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT)); GEN5_IRQ_FINI(GT); + if (INTEL_INFO(dev)-gen = 6) + GEN5_IRQ_FINI(GEN6_PM); if (HAS_PCH_NOP(dev)) return; -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/20] drm/i915: add GEN5_IRQ_INIT
On Fri, Mar 07, 2014 at 08:10:23PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com And the equivalent GEN8_IRQ_INIT_NDX macro. These macros are for the postinstall functions. The next patch will improve this macro. Notice that I could have included POSTING_READ calls to the macro, but that would mean the code would do a few more POSTING_READs than necessary. OTOH it would be more fail-proof. I can change that if needed. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 33 ++--- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 73f1125..6d4daf2 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -103,6 +103,16 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ I915_WRITE(type##IIR, 0x); \ } while (0) +#define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \ + I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \ + I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \ +} while (0) + +#define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \ + I915_WRITE(type##IMR, (imr_val)); \ + I915_WRITE(type##IER, (ier_val)); \ +} while (0) + I don't like these macros. IMO they make the code less readable, and only save a couple LOC. They don't prevent any programmer errors either, since all the logic is still contained in the values you pass in. I'll read on ahead to see if they're required in your grand scheme. /* For display hotplug interrupt */ static void ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask) @@ -2957,9 +2967,7 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev) } I915_WRITE(GTIIR, I915_READ(GTIIR)); - I915_WRITE(GTIMR, dev_priv-gt_irq_mask); - I915_WRITE(GTIER, gt_irqs); - POSTING_READ(GTIER); + GEN5_IRQ_INIT(GT, dev_priv-gt_irq_mask, gt_irqs); if (INTEL_INFO(dev)-gen = 6) { pm_irqs |= GEN6_PM_RPS_EVENTS; @@ -2969,10 +2977,9 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev) dev_priv-pm_irq_mask = 0x; I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); - I915_WRITE(GEN6_PMIMR, dev_priv-pm_irq_mask); - I915_WRITE(GEN6_PMIER, pm_irqs); - POSTING_READ(GEN6_PMIER); + GEN5_IRQ_INIT(GEN6_PM, dev_priv-pm_irq_mask, pm_irqs); } + POSTING_READ(GTIER); } static int ironlake_irq_postinstall(struct drm_device *dev) @@ -3005,9 +3012,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) /* should always can generate irq */ I915_WRITE(DEIIR, I915_READ(DEIIR)); - I915_WRITE(DEIMR, dev_priv-irq_mask); - I915_WRITE(DEIER, display_mask | extra_mask); - POSTING_READ(DEIER); + GEN5_IRQ_INIT(DE, dev_priv-irq_mask, display_mask | extra_mask); gen5_gt_irq_postinstall(dev); @@ -3172,8 +3177,7 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv) if (tmp) DRM_ERROR(Interrupt (%d) should have been masked in pre-install 0x%08x\n, i, tmp); - I915_WRITE(GEN8_GT_IMR(i), ~gt_interrupts[i]); - I915_WRITE(GEN8_GT_IER(i), gt_interrupts[i]); + GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]); } POSTING_READ(GEN8_GT_IER(0)); } @@ -3196,13 +3200,12 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) if (tmp) DRM_ERROR(Interrupt (%d) should have been masked in pre-install 0x%08x\n, pipe, tmp); - I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv-de_irq_mask[pipe]); - I915_WRITE(GEN8_DE_PIPE_IER(pipe), de_pipe_enables); + GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv-de_irq_mask[pipe], + de_pipe_enables); } POSTING_READ(GEN8_DE_PIPE_ISR(0)); - I915_WRITE(GEN8_DE_PORT_IMR, ~GEN8_AUX_CHANNEL_A); - I915_WRITE(GEN8_DE_PORT_IER, GEN8_AUX_CHANNEL_A); + GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A); POSTING_READ(GEN8_DE_PORT_IER); } -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/20] drm/i915: check if IIR is still zero at postinstall on Gen5+
On Fri, Mar 07, 2014 at 08:10:24PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Instead of trying to clear it again. It should already be masked and disabled and zeroed at preinstall/uninstall. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 32 +++- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 6d4daf2..4d0a8b1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -103,12 +103,24 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ I915_WRITE(type##IIR, 0x); \ } while (0) +/* + * We should clear IMR at preinstall/uninstall, and just check at postinstall. + */ +#define GEN5_ASSERT_IIR_IS_ZERO(reg) do { \ + u32 val = I915_READ(reg); \ + if (val) \ + DRM_ERROR(Interrupt register 0x%x is not zero: 0x%08x\n, \ + (reg), val); \ +} while (0) + #define GEN8_IRQ_INIT_NDX(type, which, imr_val, ier_val) do { \ + GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \ I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \ I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \ } while (0) #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \ + GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \ I915_WRITE(type##IMR, (imr_val)); \ I915_WRITE(type##IER, (ier_val)); \ } while (0) Okay, this is replacing a POSTED_WRITE, with a (slower) POSTING_READ which gives an error that we can do nothing about other than clear it anyway. I'd be in favor of dropping this patch. @@ -2940,7 +2952,7 @@ static void ibx_irq_postinstall(struct drm_device *dev) I915_WRITE(SERR_INT, I915_READ(SERR_INT)); } - I915_WRITE(SDEIIR, I915_READ(SDEIIR)); + GEN5_ASSERT_IIR_IS_ZERO(SDEIIR); I915_WRITE(SDEIMR, ~mask); } @@ -2966,7 +2978,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev) gt_irqs |= GT_BLT_USER_INTERRUPT | GT_BSD_USER_INTERRUPT; } - I915_WRITE(GTIIR, I915_READ(GTIIR)); GEN5_IRQ_INIT(GT, dev_priv-gt_irq_mask, gt_irqs); if (INTEL_INFO(dev)-gen = 6) { @@ -2976,7 +2987,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev) pm_irqs |= PM_VEBOX_USER_INTERRUPT; dev_priv-pm_irq_mask = 0x; - I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); GEN5_IRQ_INIT(GEN6_PM, dev_priv-pm_irq_mask, pm_irqs); } POSTING_READ(GTIER); @@ -3010,8 +3020,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev) dev_priv-irq_mask = ~display_mask; - /* should always can generate irq */ - I915_WRITE(DEIIR, I915_READ(DEIIR)); GEN5_IRQ_INIT(DE, dev_priv-irq_mask, display_mask | extra_mask); gen5_gt_irq_postinstall(dev); @@ -3172,13 +3180,8 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv) GT_RENDER_USER_INTERRUPT GEN8_VECS_IRQ_SHIFT }; - for (i = 0; i ARRAY_SIZE(gt_interrupts); i++) { - u32 tmp = I915_READ(GEN8_GT_IIR(i)); - if (tmp) - DRM_ERROR(Interrupt (%d) should have been masked in pre-install 0x%08x\n, - i, tmp); + for (i = 0; i ARRAY_SIZE(gt_interrupts); i++) GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]); - } POSTING_READ(GEN8_GT_IER(0)); } @@ -3195,14 +3198,9 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) dev_priv-de_irq_mask[PIPE_B] = ~de_pipe_masked; dev_priv-de_irq_mask[PIPE_C] = ~de_pipe_masked; - for_each_pipe(pipe) { - u32 tmp = I915_READ(GEN8_DE_PIPE_IIR(pipe)); - if (tmp) - DRM_ERROR(Interrupt (%d) should have been masked in pre-install 0x%08x\n, - pipe, tmp); + for_each_pipe(pipe) GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv-de_irq_mask[pipe], de_pipe_enables); - } POSTING_READ(GEN8_DE_PIPE_ISR(0)); GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A); -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 09/20] drm/i915: fix SERR_INT init/reset code
On Fri, Mar 07, 2014 at 08:10:25PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com The SERR_INT register is very similar to the other IIR registers, so let's zero it at preinstall/uninstall and WARN for a non-zero value at postinstall, just like we do with the other IIR registers. For this one, there's no need to double-clear since it can't store more than one interrupt. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com Without the assert that I don't like, this is Reviewed-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_irq.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4d0a8b1..d295624 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2811,6 +2811,10 @@ static void ibx_irq_preinstall(struct drm_device *dev) return; GEN5_IRQ_RESET(SDE); + + if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev)) + I915_WRITE(SERR_INT, 0x); + /* * SDEIER is also touched by the interrupt handler to work around missed * PCH interrupts. Hence we can't update it after the interrupt handler @@ -2949,7 +2953,7 @@ static void ibx_irq_postinstall(struct drm_device *dev) } else { mask = SDE_GMBUS_CPT | SDE_AUX_MASK_CPT | SDE_ERROR_CPT; - I915_WRITE(SERR_INT, I915_READ(SERR_INT)); + GEN5_ASSERT_IIR_IS_ZERO(SERR_INT); } GEN5_ASSERT_IIR_IS_ZERO(SDEIIR); @@ -3303,7 +3307,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev) GEN5_IRQ_RESET(SDE); if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev)) - I915_WRITE(SERR_INT, I915_READ(SERR_INT)); + I915_WRITE(SERR_INT, 0x); } static void i8xx_irq_preinstall(struct drm_device * dev) -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/20] drm/i915: fix GEN7_ERR_INT init/reset code
On Fri, Mar 07, 2014 at 08:10:26PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Same as SERR_INT and the other IIR registers: reset on preinstall/uninstall and WARN for non-zero values at postinstall. This one also doesn't need double-clear. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com This one just like patch 9 is: Reviewed-by: Ben Widawsky b...@bwidawsk.net Like that, I'd prefer to get rid of the IIR assertion --- drivers/gpu/drm/i915/i915_irq.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index d295624..02eb493 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2845,6 +2845,9 @@ static void ironlake_irq_preinstall(struct drm_device *dev) GEN5_IRQ_RESET(DE); + if (IS_GEN7(dev)) + I915_WRITE(GEN7_ERR_INT, 0x); + gen5_gt_irq_preinstall(dev); ibx_irq_preinstall(dev); @@ -3011,7 +3014,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) extra_mask = (DE_PIPEC_VBLANK_IVB | DE_PIPEB_VBLANK_IVB | DE_PIPEA_VBLANK_IVB); - I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT)); + GEN5_ASSERT_IIR_IS_ZERO(GEN7_ERR_INT); } else { display_mask = (DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT | DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE | @@ -3295,7 +3298,7 @@ static void ironlake_irq_uninstall(struct drm_device *dev) GEN5_IRQ_RESET(DE); if (IS_GEN7(dev)) - I915_WRITE(GEN7_ERR_INT, I915_READ(GEN7_ERR_INT)); + I915_WRITE(GEN7_ERR_INT, 0x); GEN5_IRQ_RESET(GT); if (INTEL_INFO(dev)-gen = 6) -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/20] drm/i915: enable SDEIER later
On Fri, Mar 07, 2014 at 08:10:30PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com On the preinstall stage we should just disable all the interrupts, but we currently enable all the south display interrupts due to the way we touch SDEIER at the IRQ handlers (note: they are still masked and our IRQ handler is disabled). I think this statement is false. The interrupt is enabled right after preinstall(). For the nomodeset case, this actually seems to make some difference. It still looks fine to me though. Instead of doing that, let's make the preinstall stage just disable all the south interrupts, and do the proper interrupt dance/ordering at the postinstall stage, including an assert to check if everything is behaving as expected. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 27 +-- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 95f535b..4479e29 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2814,13 +2814,24 @@ static void ibx_irq_preinstall(struct drm_device *dev) if (HAS_PCH_CPT(dev) || HAS_PCH_LPT(dev)) I915_WRITE(SERR_INT, 0x); +} - /* - * SDEIER is also touched by the interrupt handler to work around missed - * PCH interrupts. Hence we can't update it after the interrupt handler - * is enabled - instead we unconditionally enable all PCH interrupt - * sources here, but then only unmask them as needed with SDEIMR. - */ +/* + * SDEIER is also touched by the interrupt handler to work around missed PCH + * interrupts. Hence we can't update it after the interrupt handler is enabled - + * instead we unconditionally enable all PCH interrupt sources here, but then + * only unmask them as needed with SDEIMR. + * + * This function needs to be called before interrupts are enabled. + */ +static void ibx_irq_pre_postinstall(struct drm_device *dev) sde_irq_postinstall()? +{ + struct drm_i915_private *dev_priv = dev-dev_private; + + if (HAS_PCH_NOP(dev)) + return; + + WARN_ON(I915_READ(SDEIER) != 0); I915_WRITE(SDEIER, 0x); POSTING_READ(SDEIER); } @@ -3026,6 +3037,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev) dev_priv-irq_mask = ~display_mask; + ibx_irq_pre_postinstall(dev); + GEN5_IRQ_INIT(DE, dev_priv-irq_mask, display_mask | extra_mask); gen5_gt_irq_postinstall(dev); @@ -3217,6 +3230,8 @@ static int gen8_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; + ibx_irq_pre_postinstall(dev); + gen8_gt_irq_postinstall(dev_priv); gen8_de_irq_postinstall(dev_priv); -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 18/20] drm/i915: add gen8_irq_reset
On Fri, Mar 07, 2014 at 08:10:34PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com So we can merge all the common code from postinstall and uninstall. Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 26 +++--- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4917a8c..d6723ab 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2899,7 +2899,7 @@ static void valleyview_irq_preinstall(struct drm_device *dev) POSTING_READ(VLV_IER); } -static void gen8_irq_preinstall(struct drm_device *dev) +static void gen8_irq_reset(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; int pipe; @@ -2924,6 +2924,11 @@ static void gen8_irq_preinstall(struct drm_device *dev) ibx_irq_reset(dev); } +static void gen8_irq_preinstall(struct drm_device *dev) +{ + gen8_irq_reset(dev); +} + static void ibx_hpd_irq_setup(struct drm_device *dev) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev-dev_private; @@ -3253,30 +3258,13 @@ static int gen8_irq_postinstall(struct drm_device *dev) static void gen8_irq_uninstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; - int pipe; if (!dev_priv) return; intel_hpd_irq_uninstall(dev_priv); - I915_WRITE(GEN8_MASTER_IRQ, 0); - - GEN8_IRQ_RESET_NDX(GT, 0); - GEN8_IRQ_RESET_NDX(GT, 1); - GEN8_IRQ_RESET_NDX(GT, 2); - GEN8_IRQ_RESET_NDX(GT, 3); - - for_each_pipe(pipe) - GEN8_IRQ_RESET_NDX(DE_PIPE, pipe); - - GEN5_IRQ_RESET(GEN8_DE_PORT_); - GEN5_IRQ_RESET(GEN8_DE_MISC_); - GEN5_IRQ_RESET(GEN8_PCU_); - - POSTING_READ(GEN8_PCU_IIR); - - ibx_irq_reset(dev); + gen8_irq_reset(dev); BTW: This looks like a bad hunk. I've merged up to this point, and I do not have ibx_irq_reset(). } static void valleyview_irq_uninstall(struct drm_device *dev) -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 20/20] drm/i915: add POSTING_READs to the IRQ init/reset macros
On Fri, Mar 07, 2014 at 08:10:36PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com I previously chose to keep the POSTING_READ calls as something to be done by the macro callers, but the conclusion after discussing this on the mailing list is that leaving the POSTING_READ calls to the macros makes the code safer, and the additional useless register reads shouldn't be noticeable. So move the POSTING_READ calls to the callers. Can you just squash this into the earlier patch? Either way, Reviewed-by: Ben Widawsky b...@bwidawsk.net Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 79a8196..dee3a3b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -80,11 +80,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ [HPD_PORT_D] = PORTD_HOTPLUG_INT_STATUS }; -/* - * IIR can theoretically queue up two events. Be paranoid. - * Also, make sure callers of these macros have something equivalent to a - * POSTING_READ on the IIR register. - * */ +/* IIR can theoretically queue up two events. Be paranoid. */ #define GEN8_IRQ_RESET_NDX(type, which) do { \ I915_WRITE(GEN8_##type##_IMR(which), 0x); \ POSTING_READ(GEN8_##type##_IMR(which)); \ @@ -92,6 +88,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ I915_WRITE(GEN8_##type##_IIR(which), 0x); \ POSTING_READ(GEN8_##type##_IIR(which)); \ I915_WRITE(GEN8_##type##_IIR(which), 0x); \ + POSTING_READ(GEN8_##type##_IIR(which)); \ } while (0) #define GEN5_IRQ_RESET(type) do { \ @@ -101,6 +98,7 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ I915_WRITE(type##IIR, 0x); \ POSTING_READ(type##IIR); \ I915_WRITE(type##IIR, 0x); \ + POSTING_READ(type##IIR); \ } while (0) /* @@ -117,12 +115,14 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ GEN5_ASSERT_IIR_IS_ZERO(GEN8_##type##_IIR(which)); \ I915_WRITE(GEN8_##type##_IMR(which), (imr_val)); \ I915_WRITE(GEN8_##type##_IER(which), (ier_val)); \ + POSTING_READ(GEN8_##type##_IER(which)); \ } while (0) #define GEN5_IRQ_INIT(type, imr_val, ier_val) do { \ GEN5_ASSERT_IIR_IS_ZERO(type##IIR); \ I915_WRITE(type##IMR, (imr_val)); \ I915_WRITE(type##IER, (ier_val)); \ + POSTING_READ(type##IER); \ } while (0) /* For display hotplug interrupt */ @@ -2843,7 +2843,6 @@ static void gen5_gt_irq_reset(struct drm_device *dev) GEN5_IRQ_RESET(GT); if (INTEL_INFO(dev)-gen = 6) GEN5_IRQ_RESET(GEN6_PM); - POSTING_READ(GTIIR); } /* drm_dma.h hooks @@ -2917,7 +2916,6 @@ static void gen8_irq_reset(struct drm_device *dev) GEN5_IRQ_RESET(GEN8_DE_PORT_); GEN5_IRQ_RESET(GEN8_DE_MISC_); GEN5_IRQ_RESET(GEN8_PCU_); - POSTING_READ(GEN8_PCU_IIR); ibx_irq_reset(dev); } @@ -3016,7 +3014,6 @@ static void gen5_gt_irq_postinstall(struct drm_device *dev) dev_priv-pm_irq_mask = 0x; GEN5_IRQ_INIT(GEN6_PM, dev_priv-pm_irq_mask, pm_irqs); } - POSTING_READ(GTIER); } static int ironlake_irq_postinstall(struct drm_device *dev) @@ -3213,7 +3210,6 @@ static void gen8_gt_irq_postinstall(struct drm_i915_private *dev_priv) for (i = 0; i ARRAY_SIZE(gt_interrupts); i++) GEN8_IRQ_INIT_NDX(GT, i, ~gt_interrupts[i], gt_interrupts[i]); - POSTING_READ(GEN8_GT_IER(0)); } static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) @@ -3232,10 +3228,8 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv) for_each_pipe(pipe) GEN8_IRQ_INIT_NDX(DE_PIPE, pipe, dev_priv-de_irq_mask[pipe], de_pipe_enables); - POSTING_READ(GEN8_DE_PIPE_ISR(0)); GEN5_IRQ_INIT(GEN8_DE_PORT_, ~GEN8_AUX_CHANNEL_A, GEN8_AUX_CHANNEL_A); - POSTING_READ(GEN8_DE_PORT_IER); } static int gen8_irq_postinstall(struct drm_device *dev) -- 1.8.5.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/20] ILK+ interrupt improvements, v2
On Fri, Mar 07, 2014 at 08:10:16PM -0300, Paulo Zanoni wrote: From: Paulo Zanoni paulo.r.zan...@intel.com Hi This is basically a rebase of [PATCH 00/19] ILK+ interrupt improvements, which was sent to the mailing list on January 22. There are no real differences, except for the last patch, which is new. Original cover letter: http://lists.freedesktop.org/archives/intel-gfx/2014-January/038679.html The idea behind this series is that at some point our runtime PM code will just call our irq_preinstall, irq_postinstall and irq_uninstall functions instead of using dev_priv-pc8.regsave, so I decided to audit, cleanup and add a few WARNs to our code before we do that change. We gotta be in shape if we want to be exposed to runtime! Thanks, Paulo Paulo Zanoni (20): drm/i915: add GEN5_IRQ_INIT macro drm/i915: also use GEN5_IRQ_INIT with south display interrupts drm/i915: use GEN8_IRQ_INIT on GEN5 drm/i915: add GEN5_IRQ_FINI drm/i915: don't forget to uninstall the PM IRQs drm/i915: properly clear IIR at irq_uninstall on Gen5+ drm/i915: add GEN5_IRQ_INIT drm/i915: check if IIR is still zero at postinstall on Gen5+ drm/i915: fix SERR_INT init/reset code drm/i915: fix GEN7_ERR_INT init/reset code drm/i915: fix open coded gen5_gt_irq_preinstall drm/i915: extract ibx_irq_uninstall drm/i915: call ibx_irq_uninstall from gen8_irq_uninstall drm/i915: enable SDEIER later drm/i915: remove ibx_irq_uninstall drm/i915: add missing intel_hpd_irq_uninstall drm/i915: add ironlake_irq_reset drm/i915: add gen8_irq_reset drm/i915: only enable HWSTAM interrupts on postinstall on ILK+ drm/i915: add POSTING_READs to the IRQ init/reset macros drivers/gpu/drm/i915/i915_irq.c | 270 ++-- 1 file changed, 121 insertions(+), 149 deletions(-) Okay, here is the summary of my review. At first I was complaining to myself about how many patches you used to do a simple thing. But, I must admit it made reviewing the thing a lot easier, and when I look back at how much stuff you combined, I'm really glad you did it this way. I'm sure I've missed something silly though, since every patch looks so similar :P 1-5: Reviewed-by: Ben Widawsky b...@bwidawsk.net (with possible comment improvement on #3) 7: I don't like. Can we drop? I guess doing this would make a decent amount of churn, so if you don't want to drop it, that's fine, and it's functionally correct: Reviewed-by: Ben Widawsky b...@bwidawsk.net 8: I'd really like to drop this one. 9-10: Reviewed-by: Ben Widawsky b...@bwidawsk.net 12-13: I wouldn't mind cpt_irq_* rename, but either way Reviewed-by: Ben Widawsky b...@bwidawsk.net 14: With the requested change in the mail: Reviewed-by: Ben Widawsky b...@bwidawsk.net 16: Reviewed-by: Ben Widawsky b...@bwidawsk.net 20: Should be squashed, but Reviewed-by: Ben Widawsky b...@bwidawsk.net 6, 11, 15, 17, 18, 19: You introduce the term _reset as a verb which seems to always mean disable. I think disable makes the code so much clearer, and would really love if you can apply this simple rename. With the rename, they're: Reviewed-by: Ben Widawsky b...@bwidawsk.net -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/bdw: Restore PPAT on thaw
Apparently it is wiped out from under us, and we get some really fun caching artifacts upon resume (it seems to be WB for all types by default). Reported-by: James Ausmus james.aus...@intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bd016e2..1b45a04 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -30,6 +30,8 @@ #include i915_trace.h #include intel_drv.h +static void gen8_setup_private_ppat(struct drm_i915_private *dev_priv); + bool intel_enable_ppgtt(struct drm_device *dev, bool full) { if (i915.enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev)) @@ -1371,8 +1373,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) } - if (INTEL_INFO(dev)-gen = 8) + if (INTEL_INFO(dev)-gen = 8) { + gen8_setup_private_ppat(dev_priv); return; + } list_for_each_entry(vm, dev_priv-vm_list, global_link) { /* TODO: Perhaps it shouldn't be gen6 specific */ -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Per-process stats work better when evaluated per-process
On Thu, Mar 13, 2014 at 11:57:00AM -0300, Rodrigo Vivi wrote: From: Chris Wilson ch...@chris-wilson.co.uk The idea of printing objects used by each process is to judge how each process is using them. This means that we need to evaluate whether the object is bound for that particular process, rather than just whether it is bound into the global GTT. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ben Widawsky benjamin.widaw...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@gmail.com --- drivers/gpu/drm/i915/i915_debugfs.c | 34 ++--- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem_context.c | 1 + 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index a90d31c..ed3965f 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -299,28 +299,46 @@ static int i915_gem_stolen_list_info(struct seq_file *m, void *data) } while (0) struct file_stats { + struct drm_i915_file_private *file_priv; int count; - size_t total, active, inactive, unbound; + size_t total, global, active, inactive, unbound; }; static int per_file_stats(int id, void *ptr, void *data) { struct drm_i915_gem_object *obj = ptr; struct file_stats *stats = data; + struct i915_vma *vma; stats-count++; stats-total += obj-base.size; - if (i915_gem_obj_ggtt_bound(obj)) { - if (!list_empty(obj-ring_list)) + list_for_each_entry(vma, obj-vma_list, vma_link) { + struct i915_hw_ppgtt *ppgtt; + + if (!drm_mm_node_allocated(vma-node)) + continue; + + ppgtt = container_of(vma-vm, struct i915_hw_ppgtt, base); + if (ppgtt-ctx == NULL) { + stats-global += obj-base.size; + continue; + } I'm not really clear how this is supposed to work for global. Can you make me happy and change it to: if (i915_is_ggtt(vma-vm)) + + if (ppgtt-ctx-file_priv != stats-file_priv) + continue; + + if (obj-ring) /* XXX per-vma statistic */ stats-active += obj-base.size; Doesn't active get counted too many times if multiple VMAs exist for the same active object (not a new problem to this patch)? else stats-inactive += obj-base.size; - } else { - if (!list_empty(obj-global_list)) - stats-unbound += obj-base.size; + + return 0; } + if (!list_empty(obj-global_list)) + stats-unbound += obj-base.size; + return 0; } @@ -411,6 +429,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data) struct task_struct *task; memset(stats, 0, sizeof(stats)); + stats.file_priv = file-driver_priv; idr_for_each(file-object_idr, per_file_stats, stats); /* * Although we have a valid reference on file-pid, that does @@ -420,12 +439,13 @@ static int i915_gem_object_info(struct seq_file *m, void* data) */ rcu_read_lock(); task = pid_task(file-pid, PIDTYPE_PID); - seq_printf(m, %s: %u objects, %zu bytes (%zu active, %zu inactive, %zu unbound)\n, + seq_printf(m, %s: %u objects, %zu bytes (%zu active, %zu inactive, %zu global, %zu unbound)\n, task ? task-comm : unknown, stats.count, stats.total, stats.active, stats.inactive, +stats.global, stats.unbound); rcu_read_unlock(); } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2a319ba..b76c6de 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -721,6 +721,8 @@ struct i915_hw_ppgtt { dma_addr_t *gen8_pt_dma_addr[4]; }; + struct i915_hw_context *ctx; + int (*enable)(struct i915_hw_ppgtt *ppgtt); int (*switch_mm)(struct i915_hw_ppgtt *ppgtt, struct intel_ring_buffer *ring, diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ce41cff..1a94b07 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -215,6 +215,7 @@ create_vm_for_ctx(struct drm_device *dev, struct i915_hw_context *ctx) return ERR_PTR(ret); } + ppgtt-ctx = ctx; return ppgtt; } -- 1.8.5.3 -- Ben Widawsky, Intel Open Source Technology Center
Re: [Intel-gfx] [PATCH] drm/i915: Fix up the forcewake timer initialization
On Tue, Mar 18, 2014 at 04:31:03PM +0100, Daniel Vetter wrote: This is a regression introduced in commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd Author: Chris Wilson ch...@chris-wilson.co.uk Date: Thu Mar 13 12:00:29 2014 + drm/i915: Consolidate forcewake resetting to a single function The reordered setup sequence ended up calling del_timer_sync before the timer was set up correctly, resulting in endless hilarity when loading the driver. Compared to Ben's patch (which moved around the setup_timer call to sanitize_early) this moves the sanitize_early call around in the driver load call. This way we avoid calling setup_timer again in the resume code (where we also call sanitize_early). Cc: Chris Wilson ch...@chris-wilson.co.uk Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky benjamin.widaw...@intel.com Tested-by: Rodrigo Vivi rodrigo.v...@gmail.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76242 Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_dma.c | 2 -- drivers/gpu/drm/i915/intel_uncore.c | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index e4d2b9f15ae2..9faee49f210d 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1608,8 +1608,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) goto put_bridge; } - intel_uncore_early_sanitize(dev); - /* This must be called before any calls to HAS_PCH_* */ intel_detect_pch(dev); diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e2e328d86aff..c3832d9270a6 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -736,6 +736,8 @@ void intel_uncore_init(struct drm_device *dev) setup_timer(dev_priv-uncore.force_wake_timer, gen6_force_wake_timer, (unsigned long)dev_priv); + intel_uncore_early_sanitize(dev); + if (IS_VALLEYVIEW(dev)) { dev_priv-uncore.funcs.force_wake_get = __vlv_force_wake_get; dev_priv-uncore.funcs.force_wake_put = __vlv_force_wake_put; If you only want to setup_timer once, the setup_timer call should be in intel_uncore_init() which is the only one called only at load time. And of course, this is where the bug is. Otherwise, thaw calls uncore_early_sanitize, which will setup_timer again (which I thought was your complaint with my original patch). How about this, (only minimally tested): diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index e2e328d..7ef5aa3 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -387,8 +387,6 @@ void intel_uncore_early_sanitize(struct drm_device *dev) if (IS_GEN6(dev) || IS_GEN7(dev)) __raw_i915_write32(dev_priv, GTFIFODBG, __raw_i915_read32(dev_priv, GTFIFODBG)); - - intel_uncore_forcewake_reset(dev, false); } void intel_uncore_sanitize(struct drm_device *dev) @@ -413,6 +411,8 @@ void intel_uncore_sanitize(struct drm_device *dev) mutex_unlock(dev_priv-rps.hw_lock); } + + intel_uncore_forcewake_reset(dev, false); } /* @@ -846,7 +846,6 @@ void intel_uncore_fini(struct drm_device *dev) { /* Paranoia: make sure we have disabled everything before we exit. */ intel_uncore_sanitize(dev); - intel_uncore_forcewake_reset(dev, false); } -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 12/26] drm/i915: Page table helpers, and define renames
On Tue, Mar 18, 2014 at 11:29:58AM -0700, Jesse Barnes wrote: On Tue, 18 Mar 2014 09:05:58 + Chris Wilson ch...@chris-wilson.co.uk wrote: On Mon, Mar 17, 2014 at 10:48:44PM -0700, Ben Widawsky wrote: --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -1,8 +1,11 @@ #ifndef _I915_GEM_GTT_H #define _I915_GEM_GTT_H -#define GEN6_PPGTT_PD_ENTRIES 512 -#define I915_PPGTT_PT_ENTRIES (PAGE_SIZE / sizeof(gen6_gtt_pte_t)) +/* GEN Agnostic defines */ +#define I915_PDES_PER_PD 512 +#define I915_PTE_MASK(PAGE_SHIFT-1) That looks decidely fishy. PAGE_SHIFT is 12 - PTE_MASK = 0xb Thanks for catching this. I'll presume the define isn't even used. +#define I915_PDE_MASK(I915_PDES_PER_PD-1) + typedef uint32_t gen6_gtt_pte_t; typedef uint64_t gen8_gtt_pte_t; typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; @@ -23,6 +26,98 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t; #define GEN6_PTE_ADDR_ENCODE(addr) GEN6_GTT_ADDR_ENCODE(addr) #define HSW_PTE_ADDR_ENCODE(addr)HSW_GTT_ADDR_ENCODE(addr) + +/* GEN6 PPGTT resembles a 2 level page table: + * 31:22 | 21:12 | 11:0 + * PDE | PTE | offset + */ +#define GEN6_PDE_SHIFT 22 +#define GEN6_PTES_PER_PT (PAGE_SIZE / sizeof(gen6_gtt_pte_t)) + +static inline uint32_t i915_pte_index(uint64_t address, uint32_t pde_shift) +{ + const uint32_t mask = (1 (pde_shift - PAGE_SHIFT)) - 1; + return (address PAGE_SHIFT) mask; +} + +/* Helper to counts the number of PTEs within the given length. This count does + * not cross a page table boundary, so the max value would be + * GEN6_PTES_PER_PT for GEN6, and GEN8_PTES_PER_PT for GEN8. + */ +static inline size_t i915_pte_count(uint64_t addr, size_t length, + uint32_t pde_shift) +{ + const uint64_t pd_mask = ~((1 pde_shift) - 1); + uint64_t end; + + if (WARN_ON(!length)) + return 0; + + if (WARN_ON(addr % PAGE_SIZE)) + addr = round_down(addr, PAGE_SIZE); + + if (WARN_ON(length % PAGE_SIZE)) + length = round_up(length, PAGE_SIZE); Oh oh. I think these fixups are very suspect, so just BUG_ON(length == 0); BUG_ON(offset_in_page(addr|length)); I thought someone might have an issue with the BUG_ON. But I prefer it as well. + + end = addr + length; + + if ((addr pd_mask) != (end pd_mask)) + return (1 (pde_shift - PAGE_SHIFT)) - #define NUM_PTE(pde_shift) (1 (pde_shift - PAGE_SHIFT)) here and for computing the pd_mask. + i915_pte_index(addr, pde_shift); + + return i915_pte_index(end, pde_shift) - i915_pte_index(addr, pde_shift); +} Otherwise the helpers look a useful improvement in readability. -Chris Can we use GTT_PAGE_SIZE here too? I'm worried the kernel PAGE_SIZE will change at some point and blow us up. At least in places where we're doing our own thing rather than using the x86 bits... That's fine with me. We have quite a few other places in our code which depend on PAGE_SIZE being 4k though. It's likely I'll be maintaining this branch myself for a while, but I'll modify these both locally. -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/11] drm/i915: Rename and comment all the RPS *stuff*
On Sat, Feb 22, 2014 at 01:37:16PM +, Chris Wilson wrote: On Mon, Feb 17, 2014 at 07:01:44PM -0800, Ben Widawsky wrote: The names of the struct members for RPS are stupid. Every time I need to do anything in this code I have to spend a significant amount of time to remember what it all means. By renaming the variables (and adding the comments) I hope to clear up the situation. Indeed doing this make some upcoming patches more readable. I've avoided ILK because it's possible that the naming used for Ironlake matches what is in the docs. I believe the ILK power docs were never published, and I am too lazy to dig them up. While there may be mistakes, this patch was mostly done via sed. The renaming of hw_max required a bit of interactivity. It lost the distinction between RPe and RPn. I am in favour of keeping RP0, RP1, RPe, RPn for the hardware/spec values and adding the set of soft values used for actual interaction. -Chris Okay, as stated before, you are correct - I need to bring back RPe/RPn distinction. I think using the mix of values (basically s/_delay/_freq) doesn't fully relize what I was hoping to achieve. I don't think there is ever a case, except when debugging where it's easier to refer to the RP mnemonic. How strongly do you feel about this one? I'd really prefer to just fix RPe/RPn. Does anyone else have an opinion on: max_freq_hardlimit vs. rp0 Does anyone else want to review this one? -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] linux-next: build failure after merge of the drm-intel tree
On Tue, Mar 18, 2014 at 09:18:42PM -0400, Steven Rostedt wrote: On Wed, 19 Mar 2014 11:53:50 +1100 Stephen Rothwell s...@canb.auug.org.au wrote: Caused by commit a25ca17c1eac (drm/i915: Do not dereference pointers from ring buffer in evict event). I have used the drm-intel tree from next-20140318 for today. Bah! I'm still suffering from jet lag (just came back from Linux-Tage in Chemnitz). The next time I compile test a patch for a module, I'll make sure I have that module's config option set :-( The woe of using localmodconfig. I should have picked the box with the i915. :-/ Below is the fix. I'll repost a v2 of the original patch. Sorry about that. I was about to send out the same fix when I saw this. Reviewed-by: Ben Widawsky b...@bwidawsk.net -- Steve diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h index f3e8a90..783ae08 100644 --- a/drivers/gpu/drm/i915/i915_trace.h +++ b/drivers/gpu/drm/i915/i915_trace.h @@ -243,7 +243,7 @@ TRACE_EVENT(i915_gem_evict_vm, ), TP_fast_assign( -__entry-dev = dev-primary-index; +__entry-dev = vm-dev-primary-index; __entry-vm = vm; ), ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/11] drm/i915: Rename and comment all the RPS *stuff*
On Tue, Mar 18, 2014 at 06:27:03PM -0700, Ben Widawsky wrote: On Sat, Feb 22, 2014 at 01:37:16PM +, Chris Wilson wrote: On Mon, Feb 17, 2014 at 07:01:44PM -0800, Ben Widawsky wrote: The names of the struct members for RPS are stupid. Every time I need to do anything in this code I have to spend a significant amount of time to remember what it all means. By renaming the variables (and adding the comments) I hope to clear up the situation. Indeed doing this make some upcoming patches more readable. I've avoided ILK because it's possible that the naming used for Ironlake matches what is in the docs. I believe the ILK power docs were never published, and I am too lazy to dig them up. While there may be mistakes, this patch was mostly done via sed. The renaming of hw_max required a bit of interactivity. It lost the distinction between RPe and RPn. I am in favour of keeping RP0, RP1, RPe, RPn for the hardware/spec values and adding the set of soft values used for actual interaction. -Chris Okay, as stated before, you are correct - I need to bring back RPe/RPn distinction. I think using the mix of values (basically s/_delay/_freq) doesn't fully relize what I was hoping to achieve. I don't think there is ever a case, except when debugging where it's easier to refer to the RP mnemonic. How strongly do you feel about this one? I'd really prefer to just fix RPe/RPn. Does anyone else have an opinion on: max_freq_hardlimit vs. rp0 Does anyone else want to review this one? Okay, I started on this, and I somewhat agree. How about: u8 cur_freq;/* Current frequency (cached, may not == HW) */ u8 min_freq_softlimit; /* Minimum frequency permitted by the driver */ u8 max_freq_softlimit; /* Max frequency permitted by the driver */ u8 max_freq;/* Maximum frequency, RP0 if not overclocking */ u8 min_freq;/* AKA RPn. Minimum frequency */ u8 efficient_freq; /* AKA RPe. Pre-determined balanced frequency */ u8 rp1_freq;/* less than RP0 power/freqency */ u8 rp0_freq;/* Non-overclocked max frequency. */ Conveniently, this matches sysfs, minus the efficiency one, and I don't think there's a point in explicitly storing RPn, since it's always == min_freq. -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/26] drm/i915: Range clearing is PPGTT agnostic
Therefore we can do it from our general init function. Eventually, I hope to have a lot more commonality like this. It won't arrive yet, but this was a nice easy one. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index d89054d..77556ac 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -584,8 +584,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) ppgtt-base.start = 0; ppgtt-base.total = ppgtt-num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE; - ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); - DRM_DEBUG_DRIVER(Allocated %d pages for page directories (%d wasted)\n, ppgtt-num_pd_pages, ppgtt-num_pd_pages - max_pdp); DRM_DEBUG_DRIVER(Allocated %d pages for page tables (%lld wasted)\n, @@ -1154,8 +1152,6 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) gen6_map_page_tables(ppgtt); - ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); - DRM_DEBUG_DRIVER(Allocated pde space (%ldM) at GTT entry: %lx\n, ppgtt-node.size 20, ppgtt-node.start / PAGE_SIZE); @@ -1183,6 +1179,7 @@ int i915_gem_init_ppgtt(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) kref_init(ppgtt-ref); drm_mm_init(ppgtt-base.mm, ppgtt-base.start, ppgtt-base.total); + ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); i915_init_vm(dev_priv, ppgtt-base); return 0; -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/26] drm/i915: Un-hardcode number of page directories
trivial. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b3e31fd..084e82f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -722,7 +722,7 @@ struct i915_hw_ppgtt { }; union { dma_addr_t *pt_dma_addr; - dma_addr_t *gen8_pt_dma_addr[4]; + dma_addr_t *gen8_pt_dma_addr[GEN8_LEGACY_PDPS]; }; int (*enable)(struct i915_hw_ppgtt *ppgtt); -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/26] drm/i915: Split out verbose PPGTT dumping
There often is not enough memory to dump the full contents of the PPGTT. As a temporary bandage, to continue getting valuable basic PPGTT info, wrap the dangerous, memory hungry part inside of a new verbose version of the debugfs file. Also while here we can split out the ppgtt print function so it's more reusable. I'd really like to get ppgtt info into our error state, but I found it too difficult to make work in the limited time I have. Maybe Mika can find a way. Cc: Mika Kuoppala mika.kuopp...@linux.intel.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_debugfs.c | 28 ++-- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 1031c43..b226788 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1760,7 +1760,7 @@ static int per_file_ctx(int id, void *ptr, void *data) return 0; } -static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev) +static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev, int verbose) { struct drm_i915_private *dev_priv = dev-dev_private; struct intel_ring_buffer *ring; @@ -1785,7 +1785,13 @@ static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev) } } -static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev) +static void print_ppgtt(struct seq_file *m, struct i915_hw_ppgtt *ppgtt, const char *name) +{ + seq_printf(m, %s:\n, name); + seq_printf(m, pd gtt offset: 0x%08x\n, ppgtt-pd_offset); +} + +static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev, bool verbose) { struct drm_i915_private *dev_priv = dev-dev_private; struct intel_ring_buffer *ring; @@ -1806,10 +1812,9 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev) if (dev_priv-mm.aliasing_ppgtt) { struct i915_hw_ppgtt *ppgtt = dev_priv-mm.aliasing_ppgtt; - seq_puts(m, aliasing PPGTT:\n); - seq_printf(m, pd gtt offset: 0x%08x\n, ppgtt-pd_offset); - - ppgtt-debug_dump(ppgtt, m); + print_ppgtt(m, ppgtt, Aliasing PPGTT); + if (verbose) + ppgtt-debug_dump(ppgtt, m); } else return; @@ -1820,8 +1825,9 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev) pvt_ppgtt = ctx_to_ppgtt(file_priv-private_default_ctx); seq_printf(m, proc: %s\n, get_pid_task(file-pid, PIDTYPE_PID)-comm); - seq_puts(m, default context:\n); - idr_for_each(file_priv-context_idr, per_file_ctx, m); + print_ppgtt(m, pvt_ppgtt, Default context); + if (verbose) + idr_for_each(file_priv-context_idr, per_file_ctx, m); } seq_printf(m, ECOCHK: 0x%08x\n, I915_READ(GAM_ECOCHK)); } @@ -1831,6 +1837,7 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) struct drm_info_node *node = (struct drm_info_node *) m-private; struct drm_device *dev = node-minor-dev; struct drm_i915_private *dev_priv = dev-dev_private; + bool verbose = node-info_ent-data ? true : false; int ret = mutex_lock_interruptible(dev-struct_mutex); if (ret) @@ -1838,9 +1845,9 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) intel_runtime_pm_get(dev_priv); if (INTEL_INFO(dev)-gen = 8) - gen8_ppgtt_info(m, dev); + gen8_ppgtt_info(m, dev, verbose); else if (INTEL_INFO(dev)-gen = 6) - gen6_ppgtt_info(m, dev); + gen6_ppgtt_info(m, dev, verbose); intel_runtime_pm_put(dev_priv); mutex_unlock(dev-struct_mutex); @@ -3826,6 +3833,7 @@ static const struct drm_info_list i915_debugfs_list[] = { {i915_gen6_forcewake_count, i915_gen6_forcewake_count_info, 0}, {i915_swizzle_info, i915_swizzle_info, 0}, {i915_ppgtt_info, i915_ppgtt_info, 0}, + {i915_ppgtt_verbose_info, i915_ppgtt_info, 0, (void *)1}, {i915_dpio, i915_dpio_info, 0}, {i915_llc, i915_llc, 0}, {i915_edp_psr_status, i915_edp_psr_status, 0}, -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/26] drm/i915: Extract switch to default context
This patch existed for another reason which no longer exists. I liked it, so I kept it in the series. It can skipped if undesirable. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_gem.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 35f9a37..c59b707 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2476,6 +2476,8 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv); void i915_gem_context_close(struct drm_device *dev, struct drm_file *file); int i915_switch_context(struct intel_ring_buffer *ring, struct drm_file *file, struct i915_hw_context *to); +#define i915_switch_to_default(ring) \ + i915_switch_context(ring, NULL, ring-default_context) struct i915_hw_context * i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id); void i915_gem_context_free(struct kref *ctx_ref); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b2565d2..ed09dda 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2799,7 +2799,7 @@ int i915_gpu_idle(struct drm_device *dev) /* Flush everything onto the inactive list. */ for_each_ring(ring, dev_priv, i) { - ret = i915_switch_context(ring, NULL, ring-default_context); + ret = i915_switch_to_default(ring); if (ret) return ret; -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/26] drm/i915: Setup less PPGTT on failed pagedir
The current code will both potentially print a WARN, and setup part of the PPGTT structure. Neither of these harm the current code, it is simply for clarity, and to perhaps prevent later bugs, or weird debug messages. Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem_gtt.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 08a1e1c..09556d1 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1085,11 +1085,14 @@ alloc: goto alloc; } + if (ret) + return ret; + if (ppgtt-node.start dev_priv-gtt.mappable_end) DRM_DEBUG(Forced to use aperture for PDEs\n); ppgtt-num_pd_entries = GEN6_PPGTT_PD_ENTRIES; - return ret; + return 0; } static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt) -- 1.9.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx