Re: [Intel-gfx] [PATCH 1/3] drm/i915: implement IPS feature
On Mon, May 13, 2013 at 04:00:08PM -0300, Paulo Zanoni 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 port 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%.
You need to read initial IPS state and add that to pipe-config so that
takeover from the BIOS is correct and to make the code less fiddly.
[snip]
+static bool hsw_crtc_supports_ips(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc-dev;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_encoder *encoder;
+ bool is_port_a_edp = false;
+
+ if (!IS_ULT(dev))
+ return false;
+
+ for_each_encoder_on_crtc(dev, crtc, encoder)
+ if (encoder-type == INTEL_OUTPUT_EDP)
+ if (enc_to_dig_port(encoder-base)-port == PORT_A)
+ is_port_a_edp = true;
+ if (!is_port_a_edp)
+ return false;
+
+ if (intel_crtc-config.pipe_bpp != 24)
+ return false;
+
Try:
{
if (!IS_ULT(dev)) return false;
if (intel_crtc-config.pipe_bpp != 24) return false;
for_each_encoder_on_crtc() if (encoder-type == INTEL_OUTPUT_EDP
enc_to_dig_port(encoder) == PORT_A) return true;
return false;
}
+ return true;
+}
+
+static void hsw_enable_ips(struct drm_crtc *crtc)
+{
+ struct drm_i915_private *dev_priv = crtc-dev-dev_private;
+
+ if (!hsw_crtc_supports_ips(crtc))
+ return;
+
+ DRM_DEBUG_KMS(Enabling IPS\n);
+
+ /* 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. */
Throw in an assert_pipe_enabled() for good measure.
+ I915_WRITE(IPS_CTL, IPS_ENABLE);
+}
+
+static void hsw_disable_ips(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc-dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+ if (!hsw_crtc_supports_ips(crtc))
+ return;
For disabling, it would be better to check if the IPS feature was
enabled on this pipe. As written, one can change the module parameter to
trick the system into running with IPS always enabled - presumably that
is bad.
+
+ DRM_DEBUG_KMS(Disabling IPS\n);
+
+ I915_WRITE(IPS_CTL, 0);
+
+ /* We need to wait for a vblank before we can disable the plane. */
+ intel_wait_for_vblank(dev, intel_crtc-pipe);
+}
+
static void haswell_crtc_enable(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc-dev;
@@ -3387,6 +3443,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(crtc);
+
if (intel_crtc-config.has_pch_encoder)
lpt_pch_enable(crtc);
@@ -3516,6 +3574,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
if (dev_priv-cfb_plane == plane)
intel_disable_fbc(dev);
+ hsw_disable_ips(crtc);
+
intel_disable_plane(dev_priv, plane, pipe);
if (intel_crtc-config.has_pch_encoder)
@@ -6291,8 +6351,10 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- int palreg = PALETTE(intel_crtc-pipe);
+ enum pipe pipe = intel_crtc-pipe;
+ int palreg = PALETTE(pipe);
int i;
+ bool reenable_ips = false;
/* The clocks have to be on to load the palette. */
if (!crtc-enabled || !intel_crtc-active)
@@ -6300,7 +6362,18 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
/* use legacy palette for Ironlake */
if (HAS_PCH_SPLIT(dev))
- palreg = LGC_PALETTE(intel_crtc-pipe);
+ palreg = LGC_PALETTE(pipe);
+
+ /* Workaround : Do not read or write the pipe palette/gamma data while
+ * GAMMA_MODE is configured for split gamma and IPS_CTL has IPS enabled.
+ */
+ if (hsw_crtc_supports_ips(crtc)
Bring on pipe_config IPS state - as this can provoked into a user
triggerable hang (or something)...
+ (I915_READ(IPS_CTL) IPS_ENABLE)
+ ((I915_READ(GAMMA_MODE(pipe)) GAMMA_MODE_MODE_MASK) ==
+ GAMMA_MODE_MODE_SPLIT)) {
+ hsw_disable_ips(crtc);
+ reenable_ips = true;
+ }
for (i = 0; i 256; i++) {
I915_WRITE(palreg + 4 *
[Intel-gfx] [RFC PATCH 0/3] vlv sideband refactoring
Okay, I'm stuck with this a bit, and whichever approach I choose I
expect there to be a bunch of bikeshedding. So I won't polish this
further before comments.
Both the intel_dpio_{read,write} and valleyview_{punit,nc}_{read,write}
use the IOSF sideband interface. They access the same registers and do
mostly the same stuff, but no shared code. There are even duplicate
register defines for the same registers. Both have locking, but the
former use dpio_lock and the latter rps.hw_lock. It's racy.
These patches refactor the sideband access to a single function that
expects dpio_lock to be held. The dpio_lock is only used for sideband
stuff, so it's a better match than rps.hw_lock for the purpose. The rps
stuff still needs rps.hw_lock, since it's used to protect more than just
the register access, so rps code will need to hold both locks.
The bikeshedding department:
1) Do we need to propagate sideband errors from the functions (like the
valleyview_punit_* functions do), or, since the return values are
never checked anywhere anyway, can we just convert to the
intel_dpio_* style (functions return the register value only) for all
of them?
2) There will be quite a few more ports. Add new wrappers for each of
them, or create generic read/write functions that need a port
argument?
3) Should the rps stuff take dpio_lock at a higher level than the
wrappers? This is pretty much a requirement for the generic r/w
function.
4) Does dpio really need a devfn different from the rest?
5) Your additional bikeshedding here. ;)
Cheers,
Jani.
Jani Nikula (3):
drm/i915: group vlv iosf sideband register accessors to a new file
drm/i915: refactor all vlv sideband accessors to use one helper
drm/i915: drop redundant warnings on not holding dpio_lock
drivers/gpu/drm/i915/Makefile |1 +
drivers/gpu/drm/i915/i915_reg.h | 93 -
drivers/gpu/drm/i915/intel_display.c | 37 --
drivers/gpu/drm/i915/intel_dp.c |6 --
drivers/gpu/drm/i915/intel_hdmi.c |4 --
drivers/gpu/drm/i915/intel_pm.c | 60
drivers/gpu/drm/i915/intel_sideband.c | 121 +
7 files changed, 165 insertions(+), 157 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_sideband.c
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 1/3] drm/i915: group vlv iosf sideband register accessors to a new file
No functional changes.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/Makefile |1 +
drivers/gpu/drm/i915/intel_display.c | 37 --
drivers/gpu/drm/i915/intel_pm.c | 60
drivers/gpu/drm/i915/intel_sideband.c | 123 +
4 files changed, 124 insertions(+), 97 deletions(-)
create mode 100644 drivers/gpu/drm/i915/intel_sideband.c
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 91f3ac6..40034ec 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -36,6 +36,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \
intel_overlay.o \
intel_sprite.o \
intel_opregion.o \
+ intel_sideband.o \
dvo_ch7xxx.o \
dvo_ch7017.o \
dvo_ivch.o \
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7358e4e..39af0e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -381,43 +381,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.find_pll = intel_vlv_find_best_pll,
};
-u32 intel_dpio_read(struct drm_i915_private *dev_priv, int reg)
-{
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) DPIO_BUSY) == 0, 100)) {
- DRM_ERROR(DPIO idle wait timed out\n);
- return 0;
- }
-
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_READ | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) DPIO_BUSY) == 0, 100)) {
- DRM_ERROR(DPIO read wait timed out\n);
- return 0;
- }
-
- return I915_READ(DPIO_DATA);
-}
-
-void intel_dpio_write(struct drm_i915_private *dev_priv, int reg, u32 val)
-{
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) DPIO_BUSY) == 0, 100)) {
- DRM_ERROR(DPIO idle wait timed out\n);
- return;
- }
-
- I915_WRITE(DPIO_DATA, val);
- I915_WRITE(DPIO_REG, reg);
- I915_WRITE(DPIO_PKT, DPIO_RID | DPIO_OP_WRITE | DPIO_PORTID |
- DPIO_BYTE);
- if (wait_for_atomic_us((I915_READ(DPIO_PKT) DPIO_BUSY) == 0, 100))
- DRM_ERROR(DPIO write wait timed out\n);
-}
-
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
int refclk)
{
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1a76572..a06118d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4952,66 +4952,6 @@ int sandybridge_pcode_write(struct drm_i915_private
*dev_priv, u8 mbox, u32 val)
return 0;
}
-static int vlv_punit_rw(struct drm_i915_private *dev_priv, u32 port, u8 opcode,
- u8 addr, u32 *val)
-{
- u32 cmd, devfn, be, bar;
-
- bar = 0;
- be = 0xf;
- devfn = PCI_DEVFN(2, 0);
-
- cmd = (devfn IOSF_DEVFN_SHIFT) | (opcode IOSF_OPCODE_SHIFT) |
- (port IOSF_PORT_SHIFT) | (be IOSF_BYTE_ENABLES_SHIFT) |
- (bar IOSF_BAR_SHIFT);
-
- WARN_ON(!mutex_is_locked(dev_priv-rps.hw_lock));
-
- if (I915_READ(VLV_IOSF_DOORBELL_REQ) IOSF_SB_BUSY) {
- DRM_DEBUG_DRIVER(warning: pcode (%s) mailbox access failed\n,
-opcode == PUNIT_OPCODE_REG_READ ?
-read : write);
- return -EAGAIN;
- }
-
- I915_WRITE(VLV_IOSF_ADDR, addr);
- if (opcode == PUNIT_OPCODE_REG_WRITE)
- I915_WRITE(VLV_IOSF_DATA, *val);
- I915_WRITE(VLV_IOSF_DOORBELL_REQ, cmd);
-
- if (wait_for((I915_READ(VLV_IOSF_DOORBELL_REQ) IOSF_SB_BUSY) == 0,
-5)) {
- DRM_ERROR(timeout waiting for pcode %s (%d) to finish\n,
- opcode == PUNIT_OPCODE_REG_READ ? read : write,
- addr);
- return -ETIMEDOUT;
- }
-
- if (opcode == PUNIT_OPCODE_REG_READ)
- *val = I915_READ(VLV_IOSF_DATA);
- I915_WRITE(VLV_IOSF_DATA, 0);
-
- return 0;
-}
-
-int valleyview_punit_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_READ,
- addr, val);
-}
-
-int valleyview_punit_write(struct drm_i915_private *dev_priv, u8 addr, u32 val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_PUNIT, PUNIT_OPCODE_REG_WRITE,
- addr, val);
-}
-
-int valleyview_nc_read(struct drm_i915_private *dev_priv, u8 addr, u32 *val)
-{
- return vlv_punit_rw(dev_priv, IOSF_PORT_NC, PUNIT_OPCODE_REG_READ,
- addr, val);
-}
-
int vlv_gpu_freq(int ddr_freq,
[Intel-gfx] [RFC PATCH 3/3] drm/i915: drop redundant warnings on not holding dpio_lock
The lower level sideband read/write functions already do this.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/intel_dp.c |6 --
drivers/gpu/drm/i915/intel_hdmi.c |4
2 files changed, 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2bb4009..12d5c18 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1428,8 +1428,6 @@ static void intel_pre_enable_dp(struct intel_encoder
*encoder)
int pipe = intel_crtc-pipe;
u32 val;
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
val = intel_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
if (pipe)
@@ -1456,8 +1454,6 @@ static void intel_dp_pre_pll_enable(struct intel_encoder
*encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
/* Program Tx lane resets to default */
intel_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
@@ -1608,8 +1604,6 @@ static uint32_t intel_vlv_signal_levels(struct intel_dp
*intel_dp)
uint8_t train_set = intel_dp-train_set[0];
int port = vlv_dport_to_channel(dport);
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
switch (train_set DP_TRAIN_PRE_EMPHASIS_MASK) {
case DP_TRAIN_PRE_EMPHASIS_0:
preemph_reg_value = 0x0004000;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 93de5ff..9562fe4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -988,8 +988,6 @@ static void intel_hdmi_pre_enable(struct intel_encoder
*encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
/* Enable clock channels for this port */
val = intel_dpio_read(dev_priv, DPIO_DATA_LANE_A(port));
val = 0;
@@ -1033,8 +1031,6 @@ static void intel_hdmi_pre_pll_enable(struct
intel_encoder *encoder)
if (!IS_VALLEYVIEW(dev))
return;
- WARN_ON(!mutex_is_locked(dev_priv-dpio_lock));
-
/* Program Tx lane resets to default */
intel_dpio_write(dev_priv, DPIO_PCS_TX(port),
DPIO_PCS_TX_LANE2_RESET |
--
1.7.10.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC PATCH 2/3] drm/i915: refactor all vlv sideband accessors to use one helper
Based on work by Shobhit Kumar shobhit.ku...@intel.com and Yogesh Mohan Marimuthu yogesh.mohan.marimu...@intel.com Signed-off-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/i915_reg.h | 93 ++ drivers/gpu/drm/i915/intel_sideband.c | 102 - 2 files changed, 93 insertions(+), 102 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 7af7ae6..fcee92b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -344,27 +344,54 @@ #define DEBUG_RESET_DISPLAY (19) /* - * DPIO - a special bus for various display related registers to hide behind: - * 0x800c: m1, m2, n, p1, p2, k dividers - * 0x8014: REF and SFR select - * 0x8014: N divider, VCO select - * 0x801c/3c: core clock bits - * 0x8048/68: low pass filter coefficients - * 0x8100: fast clock controls + * IOSF sideband + */ +#define VLV_IOSF_DOORBELL_REQ (VLV_DISPLAY_BASE + 0x2100) +#define IOSF_DEVFN_SHIFT 24 +#define IOSF_OPCODE_SHIFT16 +#define IOSF_PORT_SHIFT 8 +#define IOSF_BYTE_ENABLES_SHIFT 4 +#define IOSF_BAR_SHIFT 1 +#define IOSF_SB_BUSY (10) +#define IOSF_PORT_PUNIT 0x4 +#define IOSF_PORT_NC 0x11 +#define IOSF_PORT_DPIO 0x12 +#define VLV_IOSF_DATA (VLV_DISPLAY_BASE + 0x2104) +#define VLV_IOSF_ADDR (VLV_DISPLAY_BASE + 0x2108) + +#define PUNIT_OPCODE_REG_READ 6 +#define PUNIT_OPCODE_REG_WRITE 7 + +#define PUNIT_REG_GPU_LFM 0xd3 +#define PUNIT_REG_GPU_FREQ_REQ 0xd4 +#define PUNIT_REG_GPU_FREQ_STS 0xd8 +#define PUNIT_REG_MEDIA_TURBO_FREQ_REQ 0xdc + +#define PUNIT_FUSE_BUS20xf6 /* bits 47:40 */ +#define PUNIT_FUSE_BUS10xf5 /* bits 55:48 */ + +#define IOSF_NC_FB_GFX_FREQ_FUSE 0x1c +#define FB_GFX_MAX_FREQ_FUSE_SHIFT 3 +#define FB_GFX_MAX_FREQ_FUSE_MASK0x07f8 +#define FB_GFX_FGUARANTEED_FREQ_FUSE_SHIFT 11 +#define FB_GFX_FGUARANTEED_FREQ_FUSE_MASK0x0007f800 +#define IOSF_NC_FB_GFX_FMAX_FUSE_HI0x34 +#define FB_FMAX_VMIN_FREQ_HI_MASK0x0007 +#define IOSF_NC_FB_GFX_FMAX_FUSE_LO0x30 +#define FB_FMAX_VMIN_FREQ_LO_SHIFT 27 +#define FB_FMAX_VMIN_FREQ_LO_MASK0xf800 + +/* + * DPIO - a special bus for various display related registers to hide behind * * DPIO is VLV only. * * Note: digital port B is DDI0, digital pot C is DDI1 */ -#define DPIO_PKT (VLV_DISPLAY_BASE + 0x2100) -#define DPIO_RID (024) -#define DPIO_OP_WRITE (116) -#define DPIO_OP_READ (016) -#define DPIO_PORTID (0x128) -#define DPIO_BYTE (0xf4) -#define DPIO_BUSY (10) /* status only */ -#define DPIO_DATA (VLV_DISPLAY_BASE + 0x2104) -#define DPIO_REG (VLV_DISPLAY_BASE + 0x2108) +#define DPIO_DEVFN 0 +#define DPIO_OPCODE_REG_WRITE 1 +#define DPIO_OPCODE_REG_READ 0 + #define DPIO_CTL (VLV_DISPLAY_BASE + 0x2110) #define DPIO_MODSEL1 (13) /* if ref clk b == 27 */ #define DPIO_MODSEL0 (12) /* if ref clk a == 27 */ @@ -4541,40 +4568,6 @@ #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16 -#define VLV_IOSF_DOORBELL_REQ 0x182100 -#define IOSF_DEVFN_SHIFT 24 -#define IOSF_OPCODE_SHIFT16 -#define IOSF_PORT_SHIFT 8 -#define IOSF_BYTE_ENABLES_SHIFT 4 -#define IOSF_BAR_SHIFT 1 -#define IOSF_SB_BUSY (10) -#define IOSF_PORT_PUNIT 0x4 -#define IOSF_PORT_NC 0x11 -#define VLV_IOSF_DATA 0x182104 -#define VLV_IOSF_ADDR 0x182108 - -#define PUNIT_OPCODE_REG_READ 6 -#define PUNIT_OPCODE_REG_WRITE 7 - -#define PUNIT_REG_GPU_LFM 0xd3 -#define PUNIT_REG_GPU_FREQ_REQ 0xd4 -#define PUNIT_REG_GPU_FREQ_STS 0xd8 -#define PUNIT_REG_MEDIA_TURBO_FREQ_REQ 0xdc - -#define PUNIT_FUSE_BUS20xf6 /* bits 47:40 */ -#define PUNIT_FUSE_BUS10xf5 /* bits 55:48 */ - -#define IOSF_NC_FB_GFX_FREQ_FUSE 0x1c -#define FB_GFX_MAX_FREQ_FUSE_SHIFT 3 -#define
Re: [Intel-gfx] debugging Haswell eDP black screen after S3
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 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/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/6] drm/i915: add encoder get_config function v5
2013/5/14 Jesse Barnes jbar...@virtuousgeek.org:
We can use this for fetching encoder specific pipe_config state, like
mode flags, adjusted clock, etc.
Just used for mode flags atm, so we can check the pipe config state at
mode set time.
v2: get_config when checking hw state too
v3: fix DVO and LVDS mode flags (Ville)
get SDVO DTD for flag fetch (Ville)
v4: use input timings (Ville)
correct command used (Ville)
remove gen4 check (Ville)
v5: get DDI flag config too
Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
I applied this on my Haswell machine and booted it with both eDP-only
and eDP+DP and now I don't see those error messages anymore.
---
drivers/gpu/drm/i915/intel_crt.c | 23 +++
drivers/gpu/drm/i915/intel_ddi.c | 23 +++
drivers/gpu/drm/i915/intel_display.c | 20 +---
drivers/gpu/drm/i915/intel_dp.c | 23 +++
drivers/gpu/drm/i915/intel_drv.h |4
drivers/gpu/drm/i915/intel_dvo.c | 21 +
drivers/gpu/drm/i915/intel_hdmi.c| 23 +++
drivers/gpu/drm/i915/intel_lvds.c| 26 +
drivers/gpu/drm/i915/intel_sdvo.c| 42
++
9 files changed, 202 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_crt.c
b/drivers/gpu/drm/i915/intel_crt.c
index cc414f1..789c4ef 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -81,6 +81,28 @@ static bool intel_crt_get_hw_state(struct intel_encoder
*encoder,
return true;
}
+static void intel_crt_get_config(struct intel_encoder *encoder,
+struct intel_crtc_config *pipe_config)
+{
+ struct drm_i915_private *dev_priv = encoder-base.dev-dev_private;
+ struct intel_crt *crt = intel_encoder_to_crt(encoder);
+ u32 tmp, flags = 0;
+
+ tmp = I915_READ(crt-adpa_reg);
+
+ if (tmp ADPA_HSYNC_ACTIVE_HIGH)
+ flags |= DRM_MODE_FLAG_PHSYNC;
+ else
+ flags |= DRM_MODE_FLAG_NHSYNC;
+
+ if (tmp ADPA_VSYNC_ACTIVE_HIGH)
+ flags |= DRM_MODE_FLAG_PVSYNC;
+ else
+ flags |= DRM_MODE_FLAG_NVSYNC;
+
+ pipe_config-adjusted_mode.flags |= flags;
+}
+
static void intel_disable_crt(struct intel_encoder *encoder)
{
struct drm_i915_private *dev_priv = encoder-base.dev-dev_private;
@@ -784,6 +806,7 @@ void intel_crt_init(struct drm_device *dev)
crt-base.compute_config = intel_crt_compute_config;
crt-base.disable = intel_disable_crt;
crt-base.enable = intel_enable_crt;
+ crt-base.get_config = intel_crt_get_config;
if (I915_HAS_HOTPLUG(dev))
crt-base.hpd_pin = HPD_CRT;
if (HAS_DDI(dev))
diff --git a/drivers/gpu/drm/i915/intel_ddi.c
b/drivers/gpu/drm/i915/intel_ddi.c
index cddcf4a..27a74a9 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1254,6 +1254,28 @@ static void intel_ddi_hot_plug(struct intel_encoder
*intel_encoder)
intel_dp_check_link_status(intel_dp);
}
+static void intel_ddi_get_config(struct intel_encoder *encoder,
+struct intel_crtc_config *pipe_config)
+{
+ struct drm_i915_private *dev_priv = encoder-base.dev-dev_private;
+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder-base.crtc);
+ enum transcoder cpu_transcoder = intel_crtc-config.cpu_transcoder;
+ u32 temp, flags = 0;
+
+ temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
+ if (temp TRANS_DDI_PHSYNC)
+ flags |= DRM_MODE_FLAG_PHSYNC;
+ else
+ flags |= DRM_MODE_FLAG_NHSYNC;
+ if (temp TRANS_DDI_PVSYNC)
+ flags |= DRM_MODE_FLAG_PVSYNC;
+ else
+ flags |= DRM_MODE_FLAG_NVSYNC;
+
+ pipe_config-adjusted_mode.flags |= flags;
+ pipe_config-pixel_multiplier = 1;
+}
+
static void intel_ddi_destroy(struct drm_encoder *encoder)
{
/* HDMI has nothing special to destroy, so we can go with this. */
@@ -1313,6 +1335,7 @@ void intel_ddi_init(struct drm_device *dev, enum port
port)
intel_encoder-disable = intel_disable_ddi;
intel_encoder-post_disable = intel_ddi_post_disable;
intel_encoder-get_hw_state = intel_ddi_get_hw_state;
+ intel_encoder-get_config = intel_ddi_get_config;
intel_dig_port-port = port;
intel_dig_port-port_reversal = I915_READ(DDI_BUF_CTL(port))
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7358e4e..163b97e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@
Re: [Intel-gfx] [alsa-devel] [PATCH 1/2] drm/915: Add private api for power well usage
Hi Takashi,
-Original Message-
From: Takashi Iwai [mailto:ti...@suse.de]
Sent: Tuesday, May 14, 2013 8:32 PM
To: Wang Xingchao
Cc: dan...@ffwll.ch; Girdwood, Liam R; alsa-de...@alsa-project.org; Zanoni,
Paulo R; Li, Jocelyn; Lin, Mengdong; intel-gfx@lists.freedesktop.org; Wang,
Xingchao; Barnes, Jesse; david.hennings...@canonical.com
Subject: Re: [alsa-devel] [PATCH 1/2] drm/915: Add private api for power well
usage
At Tue, 14 May 2013 19:44:18 +0800,
Wang Xingchao 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 mobile
C3 stepping board.
Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com
---
drivers/gpu/drm/i915/intel_pm.c | 76
+++
include/drm/i915_powerwell.h| 36 +++
2 files changed, 105 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..cf7e352 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)
{
struct drm_i915_private *dev_priv = dev-dev_private;
bool is_enabled, enable_requested;
uint32_t tmp;
- if (!HAS_POWER_WELL(dev))
- return;
-
- if (!i915_disable_power_well !enable)
- return;
-
tmp = I915_READ(HSW_PWR_WELL_DRIVER);
is_enabled = tmp HSW_PWR_WELL_STATE;
enable_requested = tmp HSW_PWR_WELL_ENABLE; @@ -4378,6
+4372,74 @@
void intel_set_power_well(struct drm_device *dev, bool enable)
}
}
+/* 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;
+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++) {
+ enable_power_well(power_well_device, true);
+ }
Should be
if (!hsw_power_count++ !i915_power_well_using)
enable_power_well(power_well_device, true);
Fixed.
Thanks
--xingchao
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] ALSA: hda - Add power-welll support for haswell HDA
Hi Takashi,
Thanks your quick feedback, please see my comments below.
-Original Message-
From: Takashi Iwai [mailto:ti...@suse.de]
Sent: Tuesday, May 14, 2013 8:15 PM
To: Wang Xingchao
Cc: dan...@ffwll.ch; Girdwood, Liam R; david.hennings...@canonical.com; Lin,
Mengdong; Li, Jocelyn; alsa-de...@alsa-project.org;
intel-gfx@lists.freedesktop.org; Barnes, Jesse; Zanoni, Paulo R; Wang,
Xingchao
Subject: Re: [PATCH 2/2] ALSA: hda - Add power-welll support for haswell HDA
At Tue, 14 May 2013 19:44:19 +0800,
Wang Xingchao wrote:
For Intel Haswell chip, HDA controller and codec have power well
dependency from GPU side. This patch added support to request/release
power well in audio driver. Power save feature should be enabled to
get runtime power saving.
Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com
---
sound/pci/hda/Kconfig |8
sound/pci/hda/Makefile|3 ++
sound/pci/hda/hda_i915.c | 92
+
sound/pci/hda/hda_i915.h | 35 +
sound/pci/hda/hda_intel.c | 33 ++--
5 files changed, 168 insertions(+), 3 deletions(-) create mode
100644 sound/pci/hda/hda_i915.c create mode 100644
sound/pci/hda/hda_i915.h
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index
80a7d44..d9e71e4 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -152,6 +152,14 @@ config SND_HDA_CODEC_HDMI
snd-hda-codec-hdmi.
This module is automatically loaded at probing.
+config SND_HDA_I915
+ bool Build Display HD-audio controller/codec power well support for
i915
cards
+ depends on DRM_I915
+ default y
+ help
+ Say Y here to include full HDMI and DisplayPort HD-audio
controller/codec
+ support for Intel Haswell graphics cards based on the i915 driver.
Do we need to make this selectable?
If you have i915 driver, this can be always set.
Well, for Non-Haswell chips with SND_HDA_I915 compiled in, it do nothing as it
has no AZX_DCAPS_I915_POWERWELL set, so no power-well API called.
For Haswell chips, SND_HDA_I915 must be enabled otherwise display audio would
fail. Anyway I added a note message in the choice to let
user know this option is a *must* one for Haswell chips.
config SND_HDA_CODEC_CIRRUS
bool Build Cirrus Logic codec support
default y
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index
24a2514..4b0a4bc 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -6,6 +6,9 @@ snd-hda-codec-$(CONFIG_PROC_FS) += hda_proc.o
snd-hda-codec-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o
snd-hda-codec-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o
+# for haswell power well
+snd-hda-intel-$(CONFIG_SND_HDA_I915) +=hda_i915.o
+
# for trace-points
CFLAGS_hda_codec.o := -I$(src)
CFLAGS_hda_intel.o := -I$(src)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c new
file mode 100644 index 000..96ca9e7
--- /dev/null
+++ b/sound/pci/hda/hda_i915.c
@@ -0,0 +1,92 @@
+/*
+ * hda_i915.c - routines for Haswell HDA controller power well
+support
+ *
+ * This program is free software; you can redistribute it and/or
+modify it
+ * under the terms of the GNU General Public License as published by
+the Free
+ * Software Foundation; either version 2 of the License, or (at your
+option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
+License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include sound/core.h
+#include drm/i915_powerwell.h
+#include hda_i915.h
+
+static const char *module_name = i915; static struct module *i915;
+static void (*get_power)(void); static void (*put_power)(void);
+
+void hda_display_power(bool enable)
+{
+ if (!get_power || !put_power)
+ return;
+
+ snd_printk(KERN_DEBUG HDA display power %s \n,
+ enable ? Enable : Disable);
Use snd_printdd() for such a frequent debug message.
We don't want to see this message at each time you open/close the audio
stream.
Fixed.
+ if (enable)
+ get_power();
+ else
+ put_power();
+}
+
+/* in case i915 not loaded, try load i915 first */ static int
+load_i915(void) {
+ int err;
+ mutex_lock(module_mutex);
+ i915 = find_module(module_name);
+ mutex_unlock(module_mutex);
+
+ if (!i915) {
+ err =
Re: [Intel-gfx] [PATCH 2/2] ALSA: hda - Add power-welll support for haswell HDA
At Thu, 16 May 2013 03:51:16 +,
Wang, Xingchao wrote:
Hi Takashi,
Thanks your quick feedback, please see my comments below.
-Original Message-
From: Takashi Iwai [mailto:ti...@suse.de]
Sent: Tuesday, May 14, 2013 8:15 PM
To: Wang Xingchao
Cc: dan...@ffwll.ch; Girdwood, Liam R; david.hennings...@canonical.com; Lin,
Mengdong; Li, Jocelyn; alsa-de...@alsa-project.org;
intel-gfx@lists.freedesktop.org; Barnes, Jesse; Zanoni, Paulo R; Wang,
Xingchao
Subject: Re: [PATCH 2/2] ALSA: hda - Add power-welll support for haswell HDA
At Tue, 14 May 2013 19:44:19 +0800,
Wang Xingchao wrote:
For Intel Haswell chip, HDA controller and codec have power well
dependency from GPU side. This patch added support to request/release
power well in audio driver. Power save feature should be enabled to
get runtime power saving.
Signed-off-by: Wang Xingchao xingchao.w...@linux.intel.com
---
sound/pci/hda/Kconfig |8
sound/pci/hda/Makefile|3 ++
sound/pci/hda/hda_i915.c | 92
+
sound/pci/hda/hda_i915.h | 35 +
sound/pci/hda/hda_intel.c | 33 ++--
5 files changed, 168 insertions(+), 3 deletions(-) create mode
100644 sound/pci/hda/hda_i915.c create mode 100644
sound/pci/hda/hda_i915.h
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig index
80a7d44..d9e71e4 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -152,6 +152,14 @@ config SND_HDA_CODEC_HDMI
snd-hda-codec-hdmi.
This module is automatically loaded at probing.
+config SND_HDA_I915
+ bool Build Display HD-audio controller/codec power well support for
i915
cards
+ depends on DRM_I915
+ default y
+ help
+ Say Y here to include full HDMI and DisplayPort HD-audio
controller/codec
+ support for Intel Haswell graphics cards based on the i915 driver.
Do we need to make this selectable?
If you have i915 driver, this can be always set.
Well, for Non-Haswell chips with SND_HDA_I915 compiled in, it do nothing as
it has no AZX_DCAPS_I915_POWERWELL set, so no power-well API called.
For Haswell chips, SND_HDA_I915 must be enabled otherwise display audio would
fail. Anyway I added a note message in the choice to let
user know this option is a *must* one for Haswell chips.
Yes, but the problem is if the driver without CONFIG_SND_HDA_I915 is
used on Haswell. Then the driver is loaded as if everything is OK
even though you know it's broken.
If you make this selectable, you should give an error from
hda_i915_init() in the case CONFIG_SND_HDA_i915=n, so that the driver
load shall fail.
config SND_HDA_CODEC_CIRRUS
bool Build Cirrus Logic codec support
default y
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile index
24a2514..4b0a4bc 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -6,6 +6,9 @@ snd-hda-codec-$(CONFIG_PROC_FS) += hda_proc.o
snd-hda-codec-$(CONFIG_SND_HDA_HWDEP) += hda_hwdep.o
snd-hda-codec-$(CONFIG_SND_HDA_INPUT_BEEP) += hda_beep.o
+# for haswell power well
+snd-hda-intel-$(CONFIG_SND_HDA_I915) += hda_i915.o
+
# for trace-points
CFLAGS_hda_codec.o := -I$(src)
CFLAGS_hda_intel.o := -I$(src)
diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c new
file mode 100644 index 000..96ca9e7
--- /dev/null
+++ b/sound/pci/hda/hda_i915.c
@@ -0,0 +1,92 @@
+/*
+ * hda_i915.c - routines for Haswell HDA controller power well
+support
+ *
+ * This program is free software; you can redistribute it and/or
+modify it
+ * under the terms of the GNU General Public License as published by
+the Free
+ * Software Foundation; either version 2 of the License, or (at your
+option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public
+License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+Foundation,
+ * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+ */
+
+#include linux/init.h
+#include linux/module.h
+#include sound/core.h
+#include drm/i915_powerwell.h
+#include hda_i915.h
+
+static const char *module_name = i915; static struct module *i915;
+static void (*get_power)(void); static void (*put_power)(void);
+
+void hda_display_power(bool enable)
+{
+ if (!get_power || !put_power)
+ return;
+
+ snd_printk(KERN_DEBUG HDA display power %s \n,
+ enable ? Enable : Disable);
Use snd_printdd() for such
