Re: [Intel-gfx] [PATCH 1/2] drm/i915: remove unexplained vblank wait in the DP off code
On Fri, Apr 11, 2014 at 02:25:41PM -0700, Jesse Barnes wrote: I don't think this is necessary; at least it doesn't appear to be on my BYT. Dropping it speeds up our shutdown code a little, in some cases resulting in faster init times. Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org Also not required on my IVB. -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
Re: [Intel-gfx] [PATCH] drm/i915/bdw: cs-stall before state cache invld w/a
On Fri, Apr 11, 2014 at 06:46:08PM -0700, Ben Widawsky wrote: We do this already for previous GENs. I guess we must do it for BDW too according to DOCS. Pipe_control with CS-stall bit set must be issued before a pipe-control command that has the State Cache Invalidate bit set. This does not solve the problem I have unfortunately. I didn't check if this was in Ville's CHV series. If it was, I apologize. No, but it was posted by Ken some time ago already. I picked up Ken's patches and was going to repost them as part of my next workarounds series. NOTE: I tried to use smaller lengths for the command, but nothing made it happy except 6. Cc: Kenneth Graunke kenn...@whitecape.org Cc: Jordan Justen jljus...@gmail.com Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/intel_ringbuffer.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a9b04d1..092dea0 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -266,17 +266,25 @@ gen6_render_ring_flush(struct intel_ring_buffer *ring, static int gen7_render_ring_cs_stall_wa(struct intel_ring_buffer *ring) { - int ret; + int ret, size = 4; - ret = intel_ring_begin(ring, 4); + if (IS_BROADWELL(ring-dev)) + size += 2; + + ret = intel_ring_begin(ring, size); if (ret) return ret; - intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(4)); + intel_ring_emit(ring, GFX_OP_PIPE_CONTROL(size)); intel_ring_emit(ring, PIPE_CONTROL_CS_STALL | PIPE_CONTROL_STALL_AT_SCOREBOARD); intel_ring_emit(ring, 0); intel_ring_emit(ring, 0); + if (IS_BROADWELL(ring-dev)) { + intel_ring_emit(ring, 0); + intel_ring_emit(ring, 0); + } + intel_ring_advance(ring); return 0; @@ -389,6 +397,11 @@ gen8_render_ring_flush(struct intel_ring_buffer *ring, flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE; flags |= PIPE_CONTROL_QW_WRITE; flags |= PIPE_CONTROL_GLOBAL_GTT_IVB; + + /* Workaround: we must issue a pipe_control with CS-stall bit + * set before a pipe_control command that has the state cache + * invalidate bit set. */ + gen7_render_ring_cs_stall_wa(ring); } ret = intel_ring_begin(ring, 6); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Always apply cursor width changes
It is possible for userspace to create a big object large enough for a 256x256, and then switch over to using it as a 64x64 cursor. This requires the cursor update routines to check for a change in width on every update, rather than just when the cursor is originally enabled. This also fixes an issue with 845g/865g which cannot change the base address of the cursor whilst it is active. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 106 +-- drivers/gpu/drm/i915/intel_drv.h | 3 +- 3 files changed, 53 insertions(+), 58 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 4c5de60e5757..7799b1394741 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2450,7 +2450,7 @@ static int i915_display_info(struct seq_file *m, void *unused) active = cursor_position(dev, crtc-pipe, x, y); seq_printf(m, \tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n, - yesno(crtc-cursor_visible), + yesno(crtc-cursor_base), x, y, crtc-cursor_addr, yesno(active)); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5e9cb9383f05..92d152694464 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7564,29 +7564,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) struct drm_device *dev = crtc-dev; struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - bool visible = base != 0; - u32 cntl; + uint32_t cntl; - if (intel_crtc-cursor_visible == visible) - return; - - cntl = I915_READ(_CURACNTR); - if (visible) { + if (base != intel_crtc-cursor_base) { /* On these chipsets we can only modify the base whilst * the cursor is disabled. */ + if (intel_crtc-cursor_cntl) { + I915_WRITE(_CURACNTR, 0); + POSTING_READ(_CURACNTR); + intel_crtc-cursor_cntl = 0; + } + I915_WRITE(_CURABASE, base); + POSTING_READ(_CURABASE); + } - cntl = ~(CURSOR_FORMAT_MASK); - /* XXX width must be 64, stride 256 = 0x00 28 */ - cntl |= CURSOR_ENABLE | + /* XXX width must be 64, stride 256 = 0x00 28 */ + cntl = 0; + if (base) + cntl = (CURSOR_ENABLE | CURSOR_GAMMA_ENABLE | - CURSOR_FORMAT_ARGB; - } else - cntl = ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE); - I915_WRITE(_CURACNTR, cntl); - - intel_crtc-cursor_visible = visible; + CURSOR_FORMAT_ARGB); + if (intel_crtc-cursor_cntl != cntl) { + I915_WRITE(_CURACNTR, cntl); + POSTING_READ(_CURACNTR); + intel_crtc-cursor_cntl = cntl; + } } static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) @@ -7595,16 +7599,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int pipe = intel_crtc-pipe; - bool visible = base != 0; - - if (intel_crtc-cursor_visible != visible) { - int16_t width = intel_crtc-cursor_width; - uint32_t cntl = I915_READ(CURCNTR(pipe)); - if (base) { - cntl = ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); - cntl |= MCURSOR_GAMMA_ENABLE; + uint32_t cntl; - switch (width) { + cntl = 0; + if (base) { + cntl = MCURSOR_GAMMA_ENABLE; + switch (intel_crtc-cursor_width) { case 64: cntl |= CURSOR_MODE_64_ARGB_AX; break; @@ -7617,18 +7617,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) default: WARN_ON(1); return; - } - cntl |= pipe 28; /* Connect to correct pipe */ - } else { - cntl = ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE); - cntl |= CURSOR_MODE_DISABLE; } + cntl |= pipe 28; /* Connect to correct pipe */ + } + if (intel_crtc-cursor_cntl != cntl) { I915_WRITE(CURCNTR(pipe),
[Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
The status bits are unconditionally set, the control bits only enable the actual interrupt generation. Which means if we get some random other interrupts we'll bogusly complain about them. So restrict the WARN to platforms with a sane hotplug interrupt handling scheme. This WARN has been introduced in commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b Author: Egbert Eich e...@suse.de Date: Fri Jul 26 14:14:24 2013 +0200 drm/i915: Add messages useful for HPD storm detection debugging (v2) Cc: Egbert Eich e...@suse.de Cc: bitlord bitlord0...@gmail.com Reported-by: bitlord bitlord0...@gmail.com Cc: sta...@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_irq.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 7753249b3a95..f98ba4e6e70b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev, spin_lock(dev_priv-irq_lock); for (i = 1; i HPD_NUM_PINS; i++) { - WARN_ONCE(hpd[i] hotplug_trigger - dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED, - Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, - hotplug_trigger, i, hpd[i]); + if (hpd[i] hotplug_trigger + dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED) { + /* +* On GMCH platforms the interrupt mask bits only +* prevent irq generation, not the setting of the +* hotplug bits itself. So only WARN about unexpected +* interrupts on saner platforms. +*/ + WARN_ONCE(INTEL_INFO(dev)-gen = 5 !IS_VALLEYVIEW(dev), + Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, + hotplug_trigger, i, hpd[i]); + + continue; + } if (!(hpd[i] hotplug_trigger) || dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED) -- 1.8.5.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Always apply cursor width changes
On Sat, Apr 12, 2014 at 04:20:49PM +0100, Chris Wilson wrote: It is possible for userspace to create a big object large enough for a 256x256, and then switch over to using it as a 64x64 cursor. This requires the cursor update routines to check for a change in width on every update, rather than just when the cursor is originally enabled. This also fixes an issue with 845g/865g which cannot change the base address of the cursor whilst it is active. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Hm, can we have a testcase for this please? I'm thinking of a black cursor on black fb at full size, then using the same cursor at smaller size and filling out the oversize with white. If the CRCs don't match we know that we scan out a bit of white. -Daniel --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 106 +-- drivers/gpu/drm/i915/intel_drv.h | 3 +- 3 files changed, 53 insertions(+), 58 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 4c5de60e5757..7799b1394741 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -2450,7 +2450,7 @@ static int i915_display_info(struct seq_file *m, void *unused) active = cursor_position(dev, crtc-pipe, x, y); seq_printf(m, \tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n, -yesno(crtc-cursor_visible), +yesno(crtc-cursor_base), x, y, crtc-cursor_addr, yesno(active)); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5e9cb9383f05..92d152694464 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7564,29 +7564,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base) struct drm_device *dev = crtc-dev; struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - bool visible = base != 0; - u32 cntl; + uint32_t cntl; - if (intel_crtc-cursor_visible == visible) - return; - - cntl = I915_READ(_CURACNTR); - if (visible) { + if (base != intel_crtc-cursor_base) { /* On these chipsets we can only modify the base whilst * the cursor is disabled. */ + if (intel_crtc-cursor_cntl) { + I915_WRITE(_CURACNTR, 0); + POSTING_READ(_CURACNTR); + intel_crtc-cursor_cntl = 0; + } + I915_WRITE(_CURABASE, base); + POSTING_READ(_CURABASE); + } - cntl = ~(CURSOR_FORMAT_MASK); - /* XXX width must be 64, stride 256 = 0x00 28 */ - cntl |= CURSOR_ENABLE | + /* XXX width must be 64, stride 256 = 0x00 28 */ + cntl = 0; + if (base) + cntl = (CURSOR_ENABLE | CURSOR_GAMMA_ENABLE | - CURSOR_FORMAT_ARGB; - } else - cntl = ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE); - I915_WRITE(_CURACNTR, cntl); - - intel_crtc-cursor_visible = visible; + CURSOR_FORMAT_ARGB); + if (intel_crtc-cursor_cntl != cntl) { + I915_WRITE(_CURACNTR, cntl); + POSTING_READ(_CURACNTR); + intel_crtc-cursor_cntl = cntl; + } } static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) @@ -7595,16 +7599,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) struct drm_i915_private *dev_priv = dev-dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int pipe = intel_crtc-pipe; - bool visible = base != 0; - - if (intel_crtc-cursor_visible != visible) { - int16_t width = intel_crtc-cursor_width; - uint32_t cntl = I915_READ(CURCNTR(pipe)); - if (base) { - cntl = ~(CURSOR_MODE | MCURSOR_PIPE_SELECT); - cntl |= MCURSOR_GAMMA_ENABLE; + uint32_t cntl; - switch (width) { + cntl = 0; + if (base) { + cntl = MCURSOR_GAMMA_ENABLE; + switch (intel_crtc-cursor_width) { case 64: cntl |= CURSOR_MODE_64_ARGB_AX; break; @@ -7617,18 +7617,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base) default: WARN_ON(1); return; - } - cntl |= pipe 28; /* Connect to correct pipe */ - } else { -
Re: [Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
Daniel Vetter writes: The status bits are unconditionally set, the control bits only enable the actual interrupt generation. Which means if we get some random other interrupts we'll bogusly complain about them. So restrict the WARN to platforms with a sane hotplug interrupt handling scheme. This WARN has been introduced in commit b8f102e8bf71cacf33326360fdf9dcfd1a63925b Author: Egbert Eich e...@suse.de Date: Fri Jul 26 14:14:24 2013 +0200 drm/i915: Add messages useful for HPD storm detection debugging (v2) Cc: Egbert Eich e...@suse.de Cc: bitlord bitlord0...@gmail.com Reported-by: bitlord bitlord0...@gmail.com Cc: sta...@vger.kernel.org Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_irq.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 7753249b3a95..f98ba4e6e70b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev, spin_lock(dev_priv-irq_lock); for (i = 1; i HPD_NUM_PINS; i++) { -WARN_ONCE(hpd[i] hotplug_trigger - dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED, - Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, - hotplug_trigger, i, hpd[i]); +if (hpd[i] hotplug_trigger +dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED) { +/* + * On GMCH platforms the interrupt mask bits only + * prevent irq generation, not the setting of the + * hotplug bits itself. So only WARN about unexpected + * interrupts on saner platforms. + */ +WARN_ONCE(INTEL_INFO(dev)-gen = 5 !IS_VALLEYVIEW(dev), + Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, + hotplug_trigger, i, hpd[i]); Personally I'd prefer the condition in the WARN..() macro to be the unexpected condition you want to warn about. This makes it easier for anybody not up to speed with the details of hotplug handling to understand the code. Of course the way you structure this avoids a lot of unnecessary tests. But if this is a concern maybe the entire for loop should be restructured with if (!(hpd[i] hotplug_trigger)) continue; right at the beginning. + +continue; +} if (!(hpd[i] hotplug_trigger) || dev_priv-hpd_stats[i].hpd_mark != HPD_ENABLED) -- 1.8.5.2 Cheers, Egbert. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't WARN about unexpected hpd interrupts on gmch platforms
On Sat, Apr 12, 2014 at 8:12 PM, Egbert Eich e...@freedesktop.org wrote: diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 7753249b3a95..f98ba4e6e70b 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1362,10 +1362,20 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev, spin_lock(dev_priv-irq_lock); for (i = 1; i HPD_NUM_PINS; i++) { -WARN_ONCE(hpd[i] hotplug_trigger - dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED, - Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, - hotplug_trigger, i, hpd[i]); +if (hpd[i] hotplug_trigger +dev_priv-hpd_stats[i].hpd_mark == HPD_DISABLED) { +/* + * On GMCH platforms the interrupt mask bits only + * prevent irq generation, not the setting of the + * hotplug bits itself. So only WARN about unexpected + * interrupts on saner platforms. + */ +WARN_ONCE(INTEL_INFO(dev)-gen = 5 !IS_VALLEYVIEW(dev), + Received HPD interrupt (0x%08x) on pin %d (0x%08x) although disabled\n, + hotplug_trigger, i, hpd[i]); Personally I'd prefer the condition in the WARN..() macro to be the unexpected condition you want to warn about. This makes it easier for anybody not up to speed with the details of hotplug handling to understand the code. Of course the way you structure this avoids a lot of unnecessary tests. But if this is a concern maybe the entire for loop should be restructured with if (!(hpd[i] hotplug_trigger)) continue; right at the beginning. + +continue; +} We want to skip the hpd handling in any case if the interrupt is blocked, otherwise every time we have some unrelated interrupt on gmch platforms while a hpd storm is ongoing we'll fire off the hotplug work. Which we obviously don't want since that defeats the point of the storm handling code. But we only want to WARN on platforms where we can reliably stop the status bits too, hence why I've nested the checks like this. -Daniel -- 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