| Message ID | 1465323873-9786-7-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote: > We can check the power state of the PHY data and common lanes as > reported by the PHY. Do this in case we need to debug problems where the > PHY gets stuck in an unexpected state. > > Note that I only check these when the lanes are expected to be powered > on purpose, since it's not clear at what point the PHY power/clock gates > things. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ > drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 81fc498..e904818 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { > #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) > #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) > > +#define _BXT_PHY_CTL_DDI_A 0x64C00 > +#define _BXT_PHY_CTL_DDI_B 0x64C10 > +#define _BXT_PHY_CTL_DDI_C 0x64C20 > +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) > +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) > +#define BXT_PHY_LANE_ENABLED (1 << 8) > +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ > + _BXT_PHY_CTL_DDI_B) > + > #define _PHY_CTL_FAMILY_EDP 0x64C80 > #define _PHY_CTL_FAMILY_DDI 0x64C90 > #define COMMON_RESET_DIS (1 << 31) > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index acf0a7a..afa28a1 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > > out: > + if (ret && IS_BROXTON(dev_priv)) { > + tmp = I915_READ(BXT_PHY_CTL(port)); > + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | > + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { > + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " > + "(PHY_CTL %08x)\n", port_name(port), tmp); > + ret = false; Hmm. I'm not sure I'd change the return value here because then we'd fail to follow the proper shutdown sequence, perhaps even messing up the next modeset? So maybe just ERROR/WARN in these cases? > + } > + } > + > intel_display_power_put(dev_priv, power_domain); > > return ret; > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > enum dpio_phy phy) > { > + enum port port; > + > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) > return false; > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > return false; > } > > + for_each_port_masked(port, > + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : > + BIT(PORT_A)) { > + u32 tmp = I915_READ(BXT_PHY_CTL(port)); > + > + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " > + "for port %c powered down " > + "(PHY_CTL %08x)\n", > + phy, port_name(port), tmp); > + > + return false; > + } > + } > + > return true; > } > > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote: > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote: > > We can check the power state of the PHY data and common lanes as > > reported by the PHY. Do this in case we need to debug problems where the > > PHY gets stuck in an unexpected state. > > > > Note that I only check these when the lanes are expected to be powered > > on purpose, since it's not clear at what point the PHY power/clock gates > > things. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ > > drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 81fc498..e904818 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { > > #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) > > #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) > > > > +#define _BXT_PHY_CTL_DDI_A 0x64C00 > > +#define _BXT_PHY_CTL_DDI_B 0x64C10 > > +#define _BXT_PHY_CTL_DDI_C 0x64C20 > > +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) > > +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) > > +#define BXT_PHY_LANE_ENABLED (1 << 8) > > +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ > > + _BXT_PHY_CTL_DDI_B) > > + > > #define _PHY_CTL_FAMILY_EDP 0x64C80 > > #define _PHY_CTL_FAMILY_DDI 0x64C90 > > #define COMMON_RESET_DIS (1 << 31) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index acf0a7a..afa28a1 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > > > > out: > > + if (ret && IS_BROXTON(dev_priv)) { > > + tmp = I915_READ(BXT_PHY_CTL(port)); > > + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | > > + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { > > + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " > > + "(PHY_CTL %08x)\n", port_name(port), tmp); > > + ret = false; > > Hmm. I'm not sure I'd change the return value here because then we'd > fail to follow the proper shutdown sequence, perhaps even messing > up the next modeset? What I wanted is to force a reprogramming in this case, but yea we would miss then the proper disable sequence. AFAICS we can't mark the state here as enabled but not valid, so for now I will just change the above to DRM_ERROR then and report an enabled state as you suggested. > So maybe just ERROR/WARN in these cases? > > > + } > > + } > > + > > intel_display_power_put(dev_priv, power_domain); > > > > return ret; > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > enum dpio_phy phy) > > { > > + enum port port; > > + > > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) > > return false; > > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > return false; > > } > > > > + for_each_port_masked(port, > > + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : > > + BIT(PORT_A)) { > > + u32 tmp = I915_READ(BXT_PHY_CTL(port)); > > + > > + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { > > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " > > + "for port %c powered down " > > + "(PHY_CTL %08x)\n", > > + phy, port_name(port), tmp); > > + > > + return false; > > + } > > + } > > + > > return true; > > } > > > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote: > On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote: > > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote: > > > We can check the power state of the PHY data and common lanes as > > > reported by the PHY. Do this in case we need to debug problems where the > > > PHY gets stuck in an unexpected state. > > > > > > Note that I only check these when the lanes are expected to be powered > > > on purpose, since it's not clear at what point the PHY power/clock gates > > > things. > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ > > > drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ > > > 2 files changed, 36 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 81fc498..e904818 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { > > > #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) > > > #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) > > > > > > +#define _BXT_PHY_CTL_DDI_A 0x64C00 > > > +#define _BXT_PHY_CTL_DDI_B 0x64C10 > > > +#define _BXT_PHY_CTL_DDI_C 0x64C20 > > > +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) > > > +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) > > > +#define BXT_PHY_LANE_ENABLED (1 << 8) > > > +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ > > > + _BXT_PHY_CTL_DDI_B) > > > + > > > #define _PHY_CTL_FAMILY_EDP 0x64C80 > > > #define _PHY_CTL_FAMILY_DDI 0x64C90 > > > #define COMMON_RESET_DIS (1 << 31) > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > index acf0a7a..afa28a1 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > > > > > > out: > > > + if (ret && IS_BROXTON(dev_priv)) { > > > + tmp = I915_READ(BXT_PHY_CTL(port)); > > > + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | > > > + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { > > > + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " > > > + "(PHY_CTL %08x)\n", port_name(port), tmp); > > > + ret = false; > > > > Hmm. I'm not sure I'd change the return value here because then we'd > > fail to follow the proper shutdown sequence, perhaps even messing > > up the next modeset? > > What I wanted is to force a reprogramming in this case, but yea we > would miss then the proper disable sequence. AFAICS we can't mark the > state here as enabled but not valid, so for now I will just change the > above to DRM_ERROR then and report an enabled state as you suggested. Yeah. So far we've done any reprogramming in the sanitize phase, but that may require splitting/duplicating some parts of state readout. Maybe we should allow .get_config() & co. to flag the state as "this needs a forced modeset to sanitize things"? > > > So maybe just ERROR/WARN in these cases? > > > > > + } > > > + } > > > + > > > intel_display_power_put(dev_priv, power_domain); > > > > > > return ret; > > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > enum dpio_phy phy) > > > { > > > + enum port port; > > > + > > > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) > > > return false; > > > > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > return false; > > > } > > > > > > + for_each_port_masked(port, > > > + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : > > > + BIT(PORT_A)) { > > > + u32 tmp = I915_READ(BXT_PHY_CTL(port)); > > > + > > > + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { > > > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " > > > + "for port %c powered down " > > > + "(PHY_CTL %08x)\n", > > > + phy, port_name(port), tmp); > > > + > > > + return false; > > > + } > > > + } > > > + > > > return true; > > > } > > > > > > -- > > > 2.5.0 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On ke, 2016-06-08 at 17:50 +0300, Ville Syrjälä wrote: > On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote: > > On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote: > > > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote: > > > > We can check the power state of the PHY data and common lanes as > > > > reported by the PHY. Do this in case we need to debug problems where the > > > > PHY gets stuck in an unexpected state. > > > > > > > > Note that I only check these when the lanes are expected to be powered > > > > on purpose, since it's not clear at what point the PHY power/clock gates > > > > things. > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ > > > > drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ > > > > 2 files changed, 36 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > index 81fc498..e904818 100644 > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { > > > > #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) > > > > #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) > > > > > > > > +#define _BXT_PHY_CTL_DDI_A 0x64C00 > > > > +#define _BXT_PHY_CTL_DDI_B 0x64C10 > > > > +#define _BXT_PHY_CTL_DDI_C 0x64C20 > > > > +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) > > > > +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) > > > > +#define BXT_PHY_LANE_ENABLED (1 << 8) > > > > +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ > > > > + _BXT_PHY_CTL_DDI_B) > > > > + > > > > #define _PHY_CTL_FAMILY_EDP 0x64C80 > > > > #define _PHY_CTL_FAMILY_DDI 0x64C90 > > > > #define COMMON_RESET_DIS (1 << 31) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > > index acf0a7a..afa28a1 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > > > > > > > > out: > > > > + if (ret && IS_BROXTON(dev_priv)) { > > > > + tmp = I915_READ(BXT_PHY_CTL(port)); > > > > + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | > > > > + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { > > > > + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " > > > > + "(PHY_CTL %08x)\n", port_name(port), tmp); > > > > + ret = false; > > > > > > Hmm. I'm not sure I'd change the return value here because then we'd > > > fail to follow the proper shutdown sequence, perhaps even messing > > > up the next modeset? > > > > What I wanted is to force a reprogramming in this case, but yea we > > would miss then the proper disable sequence. AFAICS we can't mark the > > state here as enabled but not valid, so for now I will just change the > > above to DRM_ERROR then and report an enabled state as you suggested. > > Yeah. So far we've done any reprogramming in the sanitize phase, but > that may require splitting/duplicating some parts of state readout. Maybe > we should allow .get_config() & co. to flag the state as "this needs a > forced modeset to sanitize things"? Yea, sounds good to me. Since this a new check in any case could we add this as a follow-up? > > > So maybe just ERROR/WARN in these cases? > > > > > > > + } > > > > + } > > > > + > > > > intel_display_power_put(dev_priv, power_domain); > > > > > > > > return ret; > > > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > > > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > > enum dpio_phy phy) > > > > { > > > > + enum port port; > > > > + > > > > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) > > > > return false; > > > > > > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > > return false; > > > > } > > > > > > > > + for_each_port_masked(port, > > > > + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : > > > > + BIT(PORT_A)) { > > > > + u32 tmp = I915_READ(BXT_PHY_CTL(port)); > > > > + > > > > + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { > > > > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " > > > > + "for port %c powered down " > > > > + "(PHY_CTL %08x)\n", > > > > + phy, port_name(port), tmp); > > > > + > > > > + return false; > > > > + } > > > > + } > > > > + > > > > return true; > > > > } > > > > > > > > -- > > > > 2.5.0 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
On Wed, Jun 08, 2016 at 05:55:23PM +0300, Imre Deak wrote: > On ke, 2016-06-08 at 17:50 +0300, Ville Syrjälä wrote: > > On Wed, Jun 08, 2016 at 05:41:00PM +0300, Imre Deak wrote: > > > On ke, 2016-06-08 at 17:19 +0300, Ville Syrjälä wrote: > > > > On Tue, Jun 07, 2016 at 09:24:33PM +0300, Imre Deak wrote: > > > > > We can check the power state of the PHY data and common lanes as > > > > > reported by the PHY. Do this in case we need to debug problems where the > > > > > PHY gets stuck in an unexpected state. > > > > > > > > > > Note that I only check these when the lanes are expected to be powered > > > > > on purpose, since it's not clear at what point the PHY power/clock gates > > > > > things. > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ > > > > > drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ > > > > > 2 files changed, 36 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > > > index 81fc498..e904818 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > > > @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { > > > > > #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) > > > > > #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) > > > > > > > > > > +#define _BXT_PHY_CTL_DDI_A 0x64C00 > > > > > +#define _BXT_PHY_CTL_DDI_B 0x64C10 > > > > > +#define _BXT_PHY_CTL_DDI_C 0x64C20 > > > > > +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) > > > > > +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) > > > > > +#define BXT_PHY_LANE_ENABLED (1 << 8) > > > > > +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ > > > > > + _BXT_PHY_CTL_DDI_B) > > > > > + > > > > > #define _PHY_CTL_FAMILY_EDP 0x64C80 > > > > > #define _PHY_CTL_FAMILY_DDI 0x64C90 > > > > > #define COMMON_RESET_DIS (1 << 31) > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > > > index acf0a7a..afa28a1 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > > @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > > > > DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); > > > > > > > > > > out: > > > > > + if (ret && IS_BROXTON(dev_priv)) { > > > > > + tmp = I915_READ(BXT_PHY_CTL(port)); > > > > > + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | > > > > > + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { > > > > > + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " > > > > > + "(PHY_CTL %08x)\n", port_name(port), tmp); > > > > > + ret = false; > > > > > > > > Hmm. I'm not sure I'd change the return value here because then we'd > > > > fail to follow the proper shutdown sequence, perhaps even messing > > > > up the next modeset? > > > > > > What I wanted is to force a reprogramming in this case, but yea we > > > would miss then the proper disable sequence. AFAICS we can't mark the > > > state here as enabled but not valid, so for now I will just change the > > > above to DRM_ERROR then and report an enabled state as you suggested. > > > > Yeah. So far we've done any reprogramming in the sanitize phase, but > > that may require splitting/duplicating some parts of state readout. Maybe > > we should allow .get_config() & co. to flag the state as "this needs a > > forced modeset to sanitize things"? > > Yea, sounds good to me. Since this a new check in any case could we add > this as a follow-up? Yes. This was more of a food for thought type of suggestion, so wasn't expecting that it would happen right away. > > > > > So maybe just ERROR/WARN in these cases? > > > > > > > > > + } > > > > > + } > > > > > + > > > > > intel_display_power_put(dev_priv, power_domain); > > > > > > > > > > return ret; > > > > > @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) > > > > > bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > > > enum dpio_phy phy) > > > > > { > > > > > + enum port port; > > > > > + > > > > > if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) > > > > > return false; > > > > > > > > > > @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, > > > > > return false; > > > > > } > > > > > > > > > > + for_each_port_masked(port, > > > > > + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : > > > > > + BIT(PORT_A)) { > > > > > + u32 tmp = I915_READ(BXT_PHY_CTL(port)); > > > > > + > > > > > + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { > > > > > + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " > > > > > + "for port %c powered down " > > > > > + "(PHY_CTL %08x)\n", > > > > > + phy, port_name(port), tmp); > > > > > + > > > > > + return false; > > > > > + } > > > > > + } > > > > > + > > > > > return true; > > > > > } > > > > > > > > > > -- > > > > > 2.5.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 81fc498..e904818 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1276,6 +1276,15 @@ enum skl_disp_power_wells { #define BXT_P_CR_GT_DISP_PWRON _MMIO(0x138090) #define GT_DISPLAY_POWER_ON(phy) (1 << (phy)) +#define _BXT_PHY_CTL_DDI_A 0x64C00 +#define _BXT_PHY_CTL_DDI_B 0x64C10 +#define _BXT_PHY_CTL_DDI_C 0x64C20 +#define BXT_PHY_CMNLANE_POWERDOWN_ACK (1 << 10) +#define BXT_PHY_LANE_POWERDOWN_ACK (1 << 9) +#define BXT_PHY_LANE_ENABLED (1 << 8) +#define BXT_PHY_CTL(port) _MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \ + _BXT_PHY_CTL_DDI_B) + #define _PHY_CTL_FAMILY_EDP 0x64C80 #define _PHY_CTL_FAMILY_DDI 0x64C90 #define COMMON_RESET_DIS (1 << 31) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index acf0a7a..afa28a1 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1342,6 +1342,16 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); out: + if (ret && IS_BROXTON(dev_priv)) { + tmp = I915_READ(BXT_PHY_CTL(port)); + if ((tmp & (BXT_PHY_LANE_POWERDOWN_ACK | + BXT_PHY_LANE_ENABLED)) != BXT_PHY_LANE_ENABLED) { + DRM_DEBUG_KMS("Port %c enabled but PHY powered down? " + "(PHY_CTL %08x)\n", port_name(port), tmp); + ret = false; + } + } + intel_display_power_put(dev_priv, power_domain); return ret; @@ -1745,6 +1755,8 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder) bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, enum dpio_phy phy) { + enum port port; + if (!(I915_READ(BXT_P_CR_GT_DISP_PWRON) & GT_DISPLAY_POWER_ON(phy))) return false; @@ -1770,6 +1782,21 @@ bool bxt_ddi_phy_is_enabled(struct drm_i915_private *dev_priv, return false; } + for_each_port_masked(port, + phy == DPIO_PHY0 ? BIT(PORT_B) | BIT(PORT_C) : + BIT(PORT_A)) { + u32 tmp = I915_READ(BXT_PHY_CTL(port)); + + if (tmp & BXT_PHY_CMNLANE_POWERDOWN_ACK) { + DRM_DEBUG_DRIVER("DDI PHY %d powered, but common lane " + "for port %c powered down " + "(PHY_CTL %08x)\n", + phy, port_name(port), tmp); + + return false; + } + } + return true; }
We can check the power state of the PHY data and common lanes as reported by the PHY. Do this in case we need to debug problems where the PHY gets stuck in an unexpected state. Note that I only check these when the lanes are expected to be powered on purpose, since it's not clear at what point the PHY power/clock gates things. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 9 +++++++++ drivers/gpu/drm/i915/intel_ddi.c | 27 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)