Re: [Intel-gfx] [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
On Friday 08 May 2015 06:49 PM, Ville Syrjälä wrote:
On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
On Friday 10 April 2015 08:51 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
get corrupted. The values I've managed to read from it seem to have some
pattern but vary quite a lot. The corruption doesn't seem to just happen
when the register is accessed, but can also happen spontaneosly during
modeset. When this happens during a modeset things go south and the
display doesn't light up.
I've managed to hit the problemn when toggling HDMI on port D on and
off. When things get corrupted the display doesn't light up, but as soon
as I manually write the correct value to the register the display comes
up.
First I was suspicious that we ourselves accidentally overwrite it with
garbage, but didn't catch anything with the reg_rw tracepoint. Also I
sprinkled check all over the modeset path to see exactly when the
corruption happens, and eg. the read back value was fine just before
intel_dp_set_m(), and corrupted immediately after it. I also made my
check function repair the register value whenever it was wrong, and with
this approach the corruption repeated several times during the modeset
operation, always seeming to trigger in the same exact calls to the
check function, while other calls to the function never caught anything.
So far I've not seen this problem occurring when carefully avoiding all
read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
or an actual workaround, but we can hope it works. So let's avoid reading
the register and instead track the desired value of the register in dev_priv.
v2: Read out the power well state to determine initial register value
v3: Use DPIO_CHx names instead of raw numbers
Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
Not always. I think it somehow depends on what other register accesses
happen around it. So there is perhaps some magic sequence that might allow
reading it, but I decided that it's better to be safe and never read it.
I saw similar issues on my setup. On some platform access phy is causing system
behave inconsistently :(
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_reg.h | 5 -
drivers/gpu/drm/i915/intel_runtime_pm.c | 36
-
3 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 822f259..288c3fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1754,6 +1754,8 @@ struct drm_i915_private {
u32 fdi_rx_config;
+ u32 chv_phy_control;
+
u32 suspend_count;
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cfbd5a6..98588d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
#define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
#define DPLL_PORTD_READY_MASK (0xf)
#define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
-#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
+#define PHY_CH_SU_PSR0x1
+#define PHY_CH_DEEP_PSR 0x7
PHY_CH_DEEP_PSR defined but not used in this patch?
Just wanted to define it since it's the only other valid value, and the
doc situation is crap. I've not played around with PSR so I'm not
entirely sure how these would be used in practise. My gut is telling me
SU_PSR might be used with link standby and DEEP_PSR with link off, but
that's just a hunch at this point.
right ville, DEEP_PSR is related to link off. I will try to find the docs
related to this.
You'll see in later patches that we'll start using the override bits to
force the power state, so I think at that point these don't even matter.
But I suppose when we enter PSR we should drop the override bits to
allow the hardware to manage the power states based on the PSR mode
selected.
other than this, patch does what it says.
Reviewed-by: Deepak Sdeepa...@linux.intel.com
+#define PHY_CH_POWER_MODE(mode, phy, ch) ((mode) (6*(phy)+3*(ch)+2))
+#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
#define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
#define PHY_POWERGOOD(phy) (((phy) == DPIO_PHY0) ? (131) : (130))
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..b73671f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -666,8 +666,8 @@ static void
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
On Fri, May 08, 2015 at 04:19:13PM +0300, Ville Syrjälä wrote:
On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
On Friday 10 April 2015 08:51 PM, ville.syrj...@linux.intel.com wrote:
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index cfbd5a6..98588d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
#define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
#define DPLL_PORTD_READY_MASK (0xf)
#define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
-#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
+#define PHY_CH_SU_PSR 0x1
+#define PHY_CH_DEEP_PSR0x7
PHY_CH_DEEP_PSR defined but not used in this patch?
Just wanted to define it since it's the only other valid value, and the
doc situation is crap. I've not played around with PSR so I'm not
entirely sure how these would be used in practise. My gut is telling me
SU_PSR might be used with link standby and DEEP_PSR with link off, but
that's just a hunch at this point.
Yeah adding all #defines in the a patch even if not all used is imo good
practice. Merged the first 3 patches to dinq, thanks.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
On Fri, May 08, 2015 at 06:24:42PM +0530, Deepak S wrote:
On Friday 10 April 2015 08:51 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
get corrupted. The values I've managed to read from it seem to have some
pattern but vary quite a lot. The corruption doesn't seem to just happen
when the register is accessed, but can also happen spontaneosly during
modeset. When this happens during a modeset things go south and the
display doesn't light up.
I've managed to hit the problemn when toggling HDMI on port D on and
off. When things get corrupted the display doesn't light up, but as soon
as I manually write the correct value to the register the display comes
up.
First I was suspicious that we ourselves accidentally overwrite it with
garbage, but didn't catch anything with the reg_rw tracepoint. Also I
sprinkled check all over the modeset path to see exactly when the
corruption happens, and eg. the read back value was fine just before
intel_dp_set_m(), and corrupted immediately after it. I also made my
check function repair the register value whenever it was wrong, and with
this approach the corruption repeated several times during the modeset
operation, always seeming to trigger in the same exact calls to the
check function, while other calls to the function never caught anything.
So far I've not seen this problem occurring when carefully avoiding all
read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
or an actual workaround, but we can hope it works. So let's avoid reading
the register and instead track the desired value of the register in
dev_priv.
v2: Read out the power well state to determine initial register value
v3: Use DPIO_CHx names instead of raw numbers
Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
Not always. I think it somehow depends on what other register accesses
happen around it. So there is perhaps some magic sequence that might allow
reading it, but I decided that it's better to be safe and never read it.
I saw similar issues on my setup. On some platform access phy is causing
system behave inconsistently :(
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_reg.h | 5 -
drivers/gpu/drm/i915/intel_runtime_pm.c | 36
-
3 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 822f259..288c3fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1754,6 +1754,8 @@ struct drm_i915_private {
u32 fdi_rx_config;
+ u32 chv_phy_control;
+
u32 suspend_count;
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index cfbd5a6..98588d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
#define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
#define DPLL_PORTD_READY_MASK (0xf)
#define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
-#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
+#define PHY_CH_SU_PSR0x1
+#define PHY_CH_DEEP_PSR 0x7
PHY_CH_DEEP_PSR defined but not used in this patch?
Just wanted to define it since it's the only other valid value, and the
doc situation is crap. I've not played around with PSR so I'm not
entirely sure how these would be used in practise. My gut is telling me
SU_PSR might be used with link standby and DEEP_PSR with link off, but
that's just a hunch at this point.
You'll see in later patches that we'll start using the override bits to
force the power state, so I think at that point these don't even matter.
But I suppose when we enter PSR we should drop the override bits to
allow the hardware to manage the power states based on the PSR mode
selected.
other than this, patch does what it says.
Reviewed-by: Deepak Sdeepa...@linux.intel.com
+#define PHY_CH_POWER_MODE(mode, phy, ch) ((mode) (6*(phy)+3*(ch)+2))
+#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
#define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
#define PHY_POWERGOOD(phy) (((phy) == DPIO_PHY0) ? (131) :
(130))
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..b73671f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Work around DISPLAY_PHY_CONTROL register corruption on CHV
On Friday 10 April 2015 08:51 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes (exactly when is a bit unclear) DISPLAY_PHY_CONTROL appears to
get corrupted. The values I've managed to read from it seem to have some
pattern but vary quite a lot. The corruption doesn't seem to just happen
when the register is accessed, but can also happen spontaneosly during
modeset. When this happens during a modeset things go south and the
display doesn't light up.
I've managed to hit the problemn when toggling HDMI on port D on and
off. When things get corrupted the display doesn't light up, but as soon
as I manually write the correct value to the register the display comes
up.
First I was suspicious that we ourselves accidentally overwrite it with
garbage, but didn't catch anything with the reg_rw tracepoint. Also I
sprinkled check all over the modeset path to see exactly when the
corruption happens, and eg. the read back value was fine just before
intel_dp_set_m(), and corrupted immediately after it. I also made my
check function repair the register value whenever it was wrong, and with
this approach the corruption repeated several times during the modeset
operation, always seeming to trigger in the same exact calls to the
check function, while other calls to the function never caught anything.
So far I've not seen this problem occurring when carefully avoiding all
read accesses to DISPLAY_PHY_CONTROL. Not sure if that's just pure luck
or an actual workaround, but we can hope it works. So let's avoid reading
the register and instead track the desired value of the register in dev_priv.
v2: Read out the power well state to determine initial register value
v3: Use DPIO_CHx names instead of raw numbers
Even reading once DISPLAY_PHY_CONTROL layer is getting corrupted?
I saw similar issues on my setup. On some platform access phy is causing system
behave inconsistently :(
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 ++
drivers/gpu/drm/i915/i915_reg.h | 5 -
drivers/gpu/drm/i915/intel_runtime_pm.c | 36 -
3 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 822f259..288c3fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1754,6 +1754,8 @@ struct drm_i915_private {
u32 fdi_rx_config;
+ u32 chv_phy_control;
+
u32 suspend_count;
struct i915_suspend_saved_registers regfile;
struct vlv_s0ix_state vlv_s0ix_state;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cfbd5a6..98588d5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1887,7 +1887,10 @@ enum skl_disp_power_wells {
#define DPIO_PHY_STATUS (VLV_DISPLAY_BASE + 0x6240)
#define DPLL_PORTD_READY_MASK (0xf)
#define DISPLAY_PHY_CONTROL (VLV_DISPLAY_BASE + 0x60100)
-#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
+#define PHY_CH_SU_PSR0x1
+#define PHY_CH_DEEP_PSR 0x7
PHY_CH_DEEP_PSR defined but not used in this patch?
other than this, patch does what it says.
Reviewed-by: Deepak Sdeepa...@linux.intel.com
+#define PHY_CH_POWER_MODE(mode, phy, ch) ((mode) (6*(phy)+3*(ch)+2))
+#define PHY_COM_LANE_RESET_DEASSERT(phy) (1 (phy))
#define DISPLAY_PHY_STATUS (VLV_DISPLAY_BASE + 0x60104)
#define PHY_POWERGOOD(phy) (((phy) == DPIO_PHY0) ? (131) : (130))
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index ce00e69..b73671f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -666,8 +666,8 @@ static void chv_dpio_cmn_power_well_enable(struct
drm_i915_private *dev_priv,
if (wait_for(I915_READ(DISPLAY_PHY_STATUS) PHY_POWERGOOD(phy), 1))
DRM_ERROR(Display PHY %d is not power up\n, phy);
- I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL) |
- PHY_COM_LANE_RESET_DEASSERT(phy));
+ dev_priv-chv_phy_control |= PHY_COM_LANE_RESET_DEASSERT(phy);
+ I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv-chv_phy_control);
}
static void chv_dpio_cmn_power_well_disable(struct drm_i915_private *dev_priv,
@@ -687,8 +687,8 @@ static void chv_dpio_cmn_power_well_disable(struct
drm_i915_private *dev_priv,
assert_pll_disabled(dev_priv, PIPE_C);
}
- I915_WRITE(DISPLAY_PHY_CONTROL, I915_READ(DISPLAY_PHY_CONTROL)
- ~PHY_COM_LANE_RESET_DEASSERT(phy));
+ dev_priv-chv_phy_control = ~PHY_COM_LANE_RESET_DEASSERT(phy);
+ I915_WRITE(DISPLAY_PHY_CONTROL, dev_priv-chv_phy_control);
vlv_set_power_well(dev_priv, power_well, false);
}
@@ -1401,6 +1401,30 @@ static
