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.d...@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
> > >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
