Re: [Intel-gfx] debugging Haswell eDP black screen after S3
On Thu, May 16, 2013 at 9:24 AM, Ben Guthro b...@guthro.net wrote: On Wed, May 15, 2013 at 4:42 PM, Ben Guthro b...@guthro.net wrote: On Tue, May 14, 2013 at 5:01 PM, Ben Guthro b...@guthro.net wrote: I am attempting to debug an issue with some Haswell laptop systems which do not restore their screen after resuming from S3 when running on the stable 3.8 kernel (3.8.13) The backlight is OK, but the screen is just black. In trying to determine what was going wrong, I tried looking at the output of intel_reg_dumper, in a good, and bad case: diff -u good_reg.txt bad_reg.txt --- good_reg.txt2013-05-14 15:08:44.361997000 + +++ bad_reg.txt 2013-05-14 15:09:20.48000 + @@ -1,5 +1,4 @@ - DCC: 0x (0xf340 0xf37f 0x�� -� ) + DCC: 0x (0xf340 0xf37f 0x��= � ) CHDECMISC: 0x (none, ch2 enh disabled, ch1 enh disabled, ch0 enh disabled, flex disabled, ep not present) C0DRB0: 0x (0x) C0DRB1: 0x (0x) @@ -63,17 +62,17 @@ PIPEA_DP_LINK_N: 0x CURSOR_A_BASE: 0x01061000 CURSOR_A_CONTROL: 0x0427 - CURSOR_A_POSITION: 0x03a3032f + CURSOR_A_POSITION: 0x01bb03fb FPA0: 0x (n = 0, m1 = 0, m2 = 0) FPA1: 0x (n = 0, m1 = 0, m2 = 0) DPLL_A: 0x (disabled, non-dvo, VGA, default clock, unknown mode, p1 = 0, p2 = 0) DPLL_A_MD: 0x -HTOTAL_A: 0x0821077f (1920 active, 2082 total) -HBLANK_A: 0x0821077f (1920 start, 2082 end) - HSYNC_A: 0x081307af (1968 start, 2068 end) -VTOTAL_A: 0x045f0437 (1080 active, 1120 total) -VBLANK_A: 0x045f0437 (1080 start, 1120 end) - VSYNC_A: 0x044b0441 (1090 start, 1100 end) +HTOTAL_A: 0x (1 active, 1 total) +HBLANK_A: 0x (1 start, 1 end) + HSYNC_A: 0x (1 start, 1 end) +VTOTAL_A: 0x (1 active, 1 total) +VBLANK_A: 0x (1 start, 1 end) + VSYNC_A: 0x (1 start, 1 end) BCLRPAT_A: 0x VSYNCSHIFT_A: 0x DSPBCNTR: 0x4000 (disabled, pipe A) It appears the registers that are saved, and restored in i915_save_modeset_reg / i915_restore_modeset_reg is not working properly. When I put some debug in, I discovered that it was bailing out of i915_save_modeset_reg early since the DRIVER_MODESET bit was cleared. However, it was set at the end of i915_init() This, of course, confuses me. Am I seeing memory corruption here? It looks like I misread the code here, inversing an if statement state. That said, I don't really have any clues as to why the display is black after resuming from S3 It appears that S3 is not necessary. I can reproduce the black screen with just vbetool: vbetool dpms off vbetool dpms on Does this suggest a bios issue? Is this an eDP training issue? Are there any changesets I can try backporting? I tried this, but it didn't seem to help: https://patchwork.kernel.org/patch/2516601/ Below is a serial dump with drm.debug=4, after resuming from S3 If anyone sees anything awry, being pointed in the right direction would be appreciated: ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] debugging Haswell eDP black screen after S3
On Fri, May 17, 2013 at 7:52 AM, Ben Guthro b...@guthro.net wrote: On Thu, May 16, 2013 at 9:24 AM, Ben Guthro b...@guthro.net wrote: On Wed, May 15, 2013 at 4:42 PM, Ben Guthro b...@guthro.net wrote: On Tue, May 14, 2013 at 5:01 PM, Ben Guthro b...@guthro.net wrote: I am attempting to debug an issue with some Haswell laptop systems which do not restore their screen after resuming from S3 when running on the stable 3.8 kernel (3.8.13) The backlight is OK, but the screen is just black. In trying to determine what was going wrong, I tried looking at the output of intel_reg_dumper, in a good, and bad case: diff -u good_reg.txt bad_reg.txt --- good_reg.txt2013-05-14 15:08:44.361997000 + +++ bad_reg.txt 2013-05-14 15:09:20.48000 + @@ -1,5 +1,4 @@ - DCC: 0x (0xf340 0xf37f 0x�� -� ) + DCC: 0x (0xf340 0xf37f 0x��= � ) CHDECMISC: 0x (none, ch2 enh disabled, ch1 enh disabled, ch0 enh disabled, flex disabled, ep not present) C0DRB0: 0x (0x) C0DRB1: 0x (0x) @@ -63,17 +62,17 @@ PIPEA_DP_LINK_N: 0x CURSOR_A_BASE: 0x01061000 CURSOR_A_CONTROL: 0x0427 - CURSOR_A_POSITION: 0x03a3032f + CURSOR_A_POSITION: 0x01bb03fb FPA0: 0x (n = 0, m1 = 0, m2 = 0) FPA1: 0x (n = 0, m1 = 0, m2 = 0) DPLL_A: 0x (disabled, non-dvo, VGA, default clock, unknown mode, p1 = 0, p2 = 0) DPLL_A_MD: 0x -HTOTAL_A: 0x0821077f (1920 active, 2082 total) -HBLANK_A: 0x0821077f (1920 start, 2082 end) - HSYNC_A: 0x081307af (1968 start, 2068 end) -VTOTAL_A: 0x045f0437 (1080 active, 1120 total) -VBLANK_A: 0x045f0437 (1080 start, 1120 end) - VSYNC_A: 0x044b0441 (1090 start, 1100 end) +HTOTAL_A: 0x (1 active, 1 total) +HBLANK_A: 0x (1 start, 1 end) + HSYNC_A: 0x (1 start, 1 end) +VTOTAL_A: 0x (1 active, 1 total) +VBLANK_A: 0x (1 start, 1 end) + VSYNC_A: 0x (1 start, 1 end) BCLRPAT_A: 0x VSYNCSHIFT_A: 0x DSPBCNTR: 0x4000 (disabled, pipe A) It appears the registers that are saved, and restored in i915_save_modeset_reg / i915_restore_modeset_reg is not working properly. When I put some debug in, I discovered that it was bailing out of i915_save_modeset_reg early since the DRIVER_MODESET bit was cleared. However, it was set at the end of i915_init() This, of course, confuses me. Am I seeing memory corruption here? It looks like I misread the code here, inversing an if statement state. That said, I don't really have any clues as to why the display is black after resuming from S3 It appears that S3 is not necessary. I can reproduce the black screen with just vbetool: vbetool dpms off vbetool dpms on Does this suggest a bios issue? This can be reliably reproduced on this machine, and worked around by saving the vbestate, and restoring it after the fact: (in a working state) vbetool vbestate save vbe.save break the system: vbetool dpms off vbetool dpms on The following brings the screen back, but in a low resolution corner of X: vbetool vbestate restore vbe.save And then we can get the full resolution back with the following: xrandr --output eDP1 --off xrandr --output eDP1 --auto This is clearly not an ideal solution to make a product out of. Does this point to a BIOS issue? Is anyone out there? Is this an eDP training issue? Are there any changesets I can try backporting? I tried this, but it didn't seem to help: https://patchwork.kernel.org/patch/2516601/ Below is a serial dump with drm.debug=4, after resuming from S3 If anyone sees anything awry, being pointed in the right direction would be appreciated: ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2 V3] drm/915: Add private api for power well usage
A few comments and questions below.
On Thu, 16 May 2013 15:52:36 +0800
Wang Xingchao xingchao.w...@linux.intel.com wrote:
Haswell Display audio depends on power well in graphic side, it should
request power well before use it and release power well after use.
I915 will not shutdown power well if it detects audio is using.
This patch protects display audio crash for Intel Haswell C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com
---
drivers/gpu/drm/i915/intel_pm.c | 75
+++
include/drm/i915_powerwell.h| 36 +++
2 files changed, 104 insertions(+), 7 deletions(-)
create mode 100644 include/drm/i915_powerwell.h
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0f4b46e..88820e1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4344,18 +4344,12 @@ bool intel_using_power_well(struct drm_device *dev)
return true;
}
-void intel_set_power_well(struct drm_device *dev, bool enable)
+static void enable_power_well(struct drm_device *dev, bool enable)
We can leave the name of this function alone; even for static stuff we
tend to use the intel_ prefix. Plus it's a set function, not an enable
function... so maybe just put a __ in front of it to indicate it's for
internal use only.
{
struct drm_i915_private *dev_priv = dev-dev_private;
bool is_enabled, enable_requested;
uint32_t tmp;
+/* Global drm_device for display audio drvier usage */
+static struct drm_device *power_well_device;
+/* Lock protecting power well setting */
+static DEFINE_SPINLOCK(powerwell_lock);
+static bool i915_power_well_using;
What does this mean? If it's just for making sure we don't use bogus
power_well_device, it seems like we can just use a NULL check against
power_well_device for that instead.
+static int hsw_power_count;
+
+void i915_request_power_well(void)
+{
+ if (!power_well_device)
+ return;
+
+ if (!IS_HASWELL(power_well_device))
+ return;
+
+ spin_lock_irq(powerwell_lock);
+ if (!hsw_power_count++ !i915_power_well_using)
+ enable_power_well(power_well_device, true);
+ spin_unlock_irq(powerwell_lock);
+}
+EXPORT_SYMBOL_GPL(i915_request_power_well);
+
+void i915_release_power_well(void)
+{
+ if (!power_well_device)
+ return;
+
+ if (!IS_HASWELL(power_well_device))
+ return;
+
+ spin_lock_irq(powerwell_lock);
+ WARN_ON(!hsw_power_count);
+ if (!--hsw_power_count
+ !i915_power_well_using)
+ enable_power_well(power_well_device, false);
+ spin_unlock_irq(powerwell_lock);
+}
+EXPORT_SYMBOL_GPL(i915_release_power_well);
+
+/* TODO: Call this when i915 module unload */
+void i915_remove_power_well(void)
+{
+ i915_power_well_using = false;
+ power_well_device = NULL;
+}
+
+void intel_set_power_well(struct drm_device *dev, bool enable)
+{
+ if (!HAS_POWER_WELL(dev))
+ return;
+
+ power_well_device = dev;
+ spin_lock_irq(powerwell_lock);
+ i915_power_well_using = enable;
+ if (!enable hsw_power_count) {
+ DRM_DEBUG_KMS(Display audio power well busy using now\n);
+ goto out;
+ }
+
+ if (!i915_disable_power_well !enable)
+ goto out;
+
+ enable_power_well(dev, enable);
+out:
+ spin_unlock_irq(powerwell_lock);
+}
I think we should just set the power_well_device at module init time,
then ou wouldn't need to check/set it here.
Also, the existing i915 code could just use the request/release
functions too (internal versions taking a drm_device *), then you
wouldn't need this special case.
+/* For use by hda_i915 driver */
+extern void i915_request_power_well(void);
+extern void i915_release_power_well(void);
For future proofing, it might be good if these took an enum for the
power well being requested. Then we could track an array of refcounts
later when we need the additional controls.
But I suppose that could be added later when we have a better idea of
what future chips will look like.
Jesse
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915: track ring progression using seqnos
On Mon, May 13, 2013 at 04:32:10PM +0300, Mika Kuoppala wrote:
Instead of relying in acthd, track ring seqno progression
to detect if ring has hung.
v2: put hangcheck stuff inside struct (Chris Wilson)
Signed-off-by: Mika Kuoppala mika.kuopp...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h |2 --
drivers/gpu/drm/i915/i915_irq.c | 30 +-
drivers/gpu/drm/i915/intel_ringbuffer.h |6 ++
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14817de..db7cda9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -834,8 +834,6 @@ struct i915_gpu_error {
#define DRM_I915_HANGCHECK_JIFFIES
msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD)
struct timer_list hangcheck_timer;
int hangcheck_count;
- uint32_t last_acthd[I915_NUM_RINGS];
- uint32_t prev_instdone[I915_NUM_INSTDONE_REG];
/* For reset and error_state handling. */
spinlock_t lock;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 0e5c9b0..004ad34 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2384,22 +2384,19 @@ void i915_hangcheck_elapsed(unsigned long data)
{
struct drm_device *dev = (struct drm_device *)data;
drm_i915_private_t *dev_priv = dev-dev_private;
- uint32_t acthd[I915_NUM_RINGS], instdone[I915_NUM_INSTDONE_REG];
struct intel_ring_buffer *ring;
bool err = false, idle;
int i;
+ u32 seqno[I915_NUM_RINGS];
+ bool work_done;
if (!i915_enable_hangcheck)
return;
- memset(acthd, 0, sizeof(acthd));
idle = true;
for_each_ring(ring, dev_priv, i) {
- u32 seqno;
-
- seqno = ring-get_seqno(ring, false);
- idle = i915_hangcheck_ring_idle(ring, seqno, err);
- acthd[i] = intel_ring_get_active_head(ring);
+ seqno[i] = ring-get_seqno(ring, false);
+ idle = i915_hangcheck_ring_idle(ring, seqno[i], err);
}
/* If all work is done then ACTHD clearly hasn't advanced. */
@@ -2415,20 +2412,19 @@ void i915_hangcheck_elapsed(unsigned long data)
return;
}
- i915_get_extra_instdone(dev, instdone);
- if (memcmp(dev_priv-gpu_error.last_acthd, acthd,
-sizeof(acthd)) == 0
- memcmp(dev_priv-gpu_error.prev_instdone, instdone,
-sizeof(instdone)) == 0) {
+ work_done = false;
+ for_each_ring(ring, dev_priv, i) {
+ if (ring-hangcheck.seqno != seqno[i]) {
+ work_done = true;
+ ring-hangcheck.seqno = seqno[i];
+ }
+ }
+
+ if (!work_done) {
if (i915_hangcheck_hung(dev))
return;
} else {
dev_priv-gpu_error.hangcheck_count = 0;
-
- memcpy(dev_priv-gpu_error.last_acthd, acthd,
-sizeof(acthd));
- memcpy(dev_priv-gpu_error.prev_instdone, instdone,
-sizeof(instdone));
}
repeat:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index dac1614..ef374a8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -37,6 +37,10 @@ struct intel_hw_status_page {
#define I915_READ_SYNC_0(ring) I915_READ(RING_SYNC_0((ring)-mmio_base))
#define I915_READ_SYNC_1(ring) I915_READ(RING_SYNC_1((ring)-mmio_base))
+struct intel_ring_hangcheck {
+ u32 seqno;
+};
+
Shouldn't you initialize this thing in i915_gem_init_seqno()?
struct intel_ring_buffer {
const char *name;
enum intel_ring_id {
@@ -137,6 +141,8 @@ struct intel_ring_buffer {
struct i915_hw_context *default_context;
struct i915_hw_context *last_context;
+ struct intel_ring_hangcheck hangcheck;
+
void *private;
};
--
1.7.9.5
--
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/3] drm/i915: implement IPS feature
Reviewed-by: Rodrigo Vivi rodrigo.v...@gmail.com
On Thu, May 16, 2013 at 4:54 PM, Paulo Zanoni przan...@gmail.com wrote:
From: Paulo Zanoni paulo.r.zan...@intel.com
Intermediate Pixel Storage is a feature that should reduce the number
of times the display engine wakes up memory to read pixels, so it
should allow deeper PC states. IPS can only be enabled on ULT pipe A
with 8:8:8 pipe pixel formats.
With eDP 1920x1080 and correct watermarks but without FBC this moves
my PC7 residency from 2.5% to around 38%.
v2: - It's tied to pipe A, not port A
- Add pipe_config support (Chris)
- Add some assertions (Chris)
- Rebase against latest dinq
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 11 +
drivers/gpu/drm/i915/intel_display.c | 79
+++-
drivers/gpu/drm/i915/intel_drv.h | 2 +
3 files changed, 90 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 32eb97c..65339a7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -977,6 +977,8 @@
/* Framebuffer compression for Ivybridge */
#define IVB_FBC_RT_BASE0x7020
+#define IPS_CTL0x43408
+#define IPS_ENABLE (1 31)
#define _HSW_PIPE_SLICE_CHICKEN_1_A0x420B0
#define _HSW_PIPE_SLICE_CHICKEN_1_B0x420B4
@@ -3621,6 +3623,15 @@
#define _LGC_PALETTE_B 0x4a800
#define LGC_PALETTE(pipe) _PIPE(pipe, _LGC_PALETTE_A, _LGC_PALETTE_B)
+#define _GAMMA_MODE_A 0x4a480
+#define _GAMMA_MODE_B 0x4ac80
+#define GAMMA_MODE(pipe) _PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
+#define GAMMA_MODE_MODE_MASK (3 0)
+#define GAMMA_MODE_MODE_8bit (0 0)
+#define GAMMA_MODE_MODE_10bit (1 0)
+#define GAMMA_MODE_MODE_12bit (2 0)
+#define GAMMA_MODE_MODE_SPLIT (3 0)
+
/* interrupts */
#define DE_MASTER_IRQ_CONTROL (1 31)
#define DE_SPRITEB_FLIP_DONE(1 29)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index d588ff6..5b41cf3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3340,6 +3340,53 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
intel_wait_for_vblank(dev, intel_crtc-pipe);
}
+/* IPS only exists on ULT machines and is tied to pipe A. */
+static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
+{
+ return (IS_ULT(crtc-base.dev) crtc-pipe == PIPE_A);
+}
+
+static void hsw_enable_ips(struct intel_crtc *crtc)
+{
+ struct drm_i915_private *dev_priv = crtc-base.dev-dev_private;
+
+ if (!hsw_crtc_supports_ips(crtc))
+ return;
+
+ if (crtc-config.pipe_bpp != 24)
+ return;
+
+ DRM_DEBUG_KMS(Enabling IPS\n);
+
+ crtc-config.ips_enabled = true;
+
+ /* We can only enable IPS after we enable a plane and wait for a
vblank.
+* We guarantee that the plane is enabled by calling intel_enable_ips
+* only after intel_enable_plane. And intel_enable_plane already waits
+* for a vblank, so all we need to do here is to enable the IPS bit.
*/
+ assert_plane_enabled(dev_priv, crtc-plane);
+ I915_WRITE(IPS_CTL, IPS_ENABLE);
+}
+
+static void hsw_disable_ips(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+
+ if (!hsw_crtc_supports_ips(crtc))
+ return;
+
+ DRM_DEBUG_KMS(Disabling IPS\n);
+
+ crtc-config.ips_enabled = false;
+
+ assert_plane_enabled(dev_priv, crtc-plane);
+ I915_WRITE(IPS_CTL, 0);
+
+ /* We need to wait for a vblank before we can disable the plane. */
+ intel_wait_for_vblank(dev, crtc-pipe);
+}
+
static void haswell_crtc_enable(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc-dev;
@@ -3387,6 +3434,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
intel_crtc-config.has_pch_encoder);
intel_enable_plane(dev_priv, plane, pipe);
+ hsw_enable_ips(intel_crtc);
+
if (intel_crtc-config.has_pch_encoder)
lpt_pch_enable(crtc);
@@ -3529,6 +3578,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
if (dev_priv-cfb_plane == plane)
intel_disable_fbc(dev);
+ hsw_disable_ips(intel_crtc);
+
intel_disable_plane(dev_priv, plane, pipe);
if (intel_crtc-config.has_pch_encoder)
@@ -5021,6 +5072,8 @@ static bool i9xx_get_pipe_config(struct intel_crtc
*crtc,
i9xx_get_pfit_config(crtc, pipe_config);
+ pipe_config-ips_enabled = false;
+
return true;
}
@@ -5890,6 +5943,8 @@ static bool ironlake_get_pipe_config(struct intel_crtc
*crtc,
