On to, 2016-03-24 at 11:15 +0200, Jani Nikula wrote:
> On Wed, 23 Mar 2016, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > Currently the machine hangs during booting while accessing the
> > BXT_MIPI_PORT_CTRL register during pipe HW state readout. After
> > some
> > experimentation I found that the hang is caused by the DSI PLL
> > being
> > disabled, or it being enabled but with an incorrect divider
> > configuration. Enabling the PLL got rid of the boot problem, so fix
> > this by checking the PLL enabled state/configuration before
> > attempting
> > to read out the HW state.
> >
> > The DSI_PLL_ENABLE register is in the always-on power well, while
> > the
> > BXT_DSI_PLL_CTL is in power well 0. This isn't exactly matched by
> > the
> > transcoder power domain, but what we really need is just a runtime
> > PM
> > reference, which is provided by any power domain.
> >
> > Ville also found this dependency specified in BSpec, so I added a
> > reference to that too.
> >
> > v2:
> > - Make sure we hold a power reference while accessing the PLL
> > registers.
> >
> > CC: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > CC: Uma Shankar <uma.shankar@xxxxxxxxx>
> > CC: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Fixes: c6c794a2fc5e ("drm/i915/bxt: Initialize MIPI DSI for BXT")
>
> I figured this might blow something up now, but then again we needed
> to
> do it to find out all the ways it would blow up. Apologies and
> thanks. ;)
>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
> > drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++++
> > drivers/gpu/drm/i915/intel_dsi.h | 1 +
> > drivers/gpu/drm/i915/intel_dsi_pll.c | 37
> > ++++++++++++++++++++++++++++++++++++
> > 5 files changed, 62 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f3ba43c..c839ce9 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7811,9 +7811,11 @@ enum skl_disp_power_wells {
> > #define BXT_DSIC_16X_BY2 (1 << 10)
> > #define BXT_DSIC_16X_BY3 (2 << 10)
> > #define BXT_DSIC_16X_BY4 (3 << 10)
> > +#define BXT_DSIC_16X_MASK (3 << 10)
> > #define BXT_DSIA_16X_BY2 (1 << 8)
> > #define BXT_DSIA_16X_BY3 (2 << 8)
> > #define BXT_DSIA_16X_BY4 (3 << 8)
> > +#define BXT_DSIA_16X_MASK (3 << 8)
> > #define BXT_DSI_FREQ_SEL_SHIFT 8
> > #define BXT_DSI_FREQ_SEL_MASK (0xF <<
> > BXT_DSI_FREQ_SEL_SHIFT)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 009b03b..6fd94c4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -36,6 +36,7 @@
> > #include "intel_drv.h"
> > #include <drm/i915_drm.h>
> > #include "i915_drv.h"
> > +#include "intel_dsi.h"
> > #include "i915_trace.h"
> > #include <drm/drm_atomic.h>
> > #include <drm/drm_atomic_helper.h>
> > @@ -9852,10 +9853,12 @@ static bool
> > bxt_get_dsi_transcoder_state(struct intel_crtc *crtc,
> > enum intel_display_power_domain power_domain;
> > enum port port;
> > enum transcoder cpu_transcoder;
> > + bool pll_enabled;
> > u32 tmp;
> >
> > pipe_config->has_dsi_encoder = false;
> >
> > + pll_enabled = false;
>
> It seems to me you could just bail out early here if the dsi pll is
> disabled.
>
> I'm guessing you put the check in the loop because of the power
> domain reference.
Yes.
> Two thoughts on that:
>
> a) BXT_DSI_PLL_ENABLE is in always on power.
Well, that means always-on, whenever the PCI device is on, that is we
still need to hold a runtime PM reference, which is provided by any
display power domain reference.
> If you dropped the paranoia
> in bxt_dsi_pll_is_enabled() about valid BXT_DSI_PLL_CTL values, you
> could simplify code here and in intel_dsi_get_hw_state().
I think it's better to check the dividers too. If the dividers are
incorrect that machine will hang even though the PLL is enabled. Ville
told me that he saw a similar misconfiguration by BIOS on CHV/VLV, so
it's not unimaginable that it could also happen on BXT.
> b) BXT_DSI_PLL_CTL is in PG0. Here, you implicitly trust the
> transcoder
> power domain to match that. In intel_dsi_get_hw_state() you
> implicitly
> trust the DSI port power domain to match that. They both do, but this
> seems a somewhat coincidental choice.
I'm actually not sure if power well 0 is simply the same as the always-
on well, there is no separate always-on well shown in BSpec "Broxton
Display Connections". In any case we don't handle either of these
wells manually (on-demand), it's only during runtime suspend when they
are powered off. So to keep them on we need a runtime PM reference and
that is guaranteed to be provided by any power domain ref.
> > for_each_port_masked(port, BIT(PORT_A) | BIT(PORT_C)) {
> > if (port == PORT_A)
> > cpu_transcoder = TRANSCODER_DSI_A;
> > @@ -9867,6 +9870,16 @@ static bool
> > bxt_get_dsi_transcoder_state(struct intel_crtc *crtc,
> > continue;
> > *power_domain_mask |= BIT(power_domain);
> >
> > + /*
> > + * The PLL needs to be enabled with a valid
> > divider
> > + * configuration, otherwise accessing DSI
> > registers will hang
> > + * the machine. See BSpec North Display Engine
> > + * registers/MIPI[BXT].
> > + */
> > + pll_enabled = pll_enabled ||
> > intel_dsi_pll_is_enabled(dev_priv);
> > + if (!pll_enabled)
> > + break;
> > +
>
> I guess if we want to keep the paranoia and we are happy with how the
> power domain references map to the power well of BXT_DSI_PLL_CTL, the
> simplest you could do is throw away the local variable and just do
> this:
>
> if (!intel_dsi_pll_is_enabled(dev_priv))
> continue;
>
> This potentially leads to an extra loop here, but at least the code
> gets
> simpler and easier to grasp.
Yea, it will result in an extra HW access, but I can simplify things as
you suggested.
> > /* XXX: this works for video mode only */
> > tmp = I915_READ(BXT_MIPI_PORT_CTRL(port));
> > if (!(tmp & DPI_ENABLE))
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> > b/drivers/gpu/drm/i915/intel_dsi.c
> > index 96ea3f7..0de74e1 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -684,6 +684,14 @@ static bool intel_dsi_get_hw_state(struct
> > intel_encoder *encoder,
> > if (!intel_display_power_get_if_enabled(dev_priv,
> > power_domain))
> > return false;
> >
> > + /*
> > + * On Broxton the PLL needs to be enabled with a valid
> > divider
> > + * configuration, otherwise accessing DSI registers will
> > hang the
> > + * machine. See BSpec North Display Engine
> > registers/MIPI[BXT].
> > + */
> > + if (IS_BROXTON(dev_priv) &&
> > !intel_dsi_pll_is_enabled(dev_priv))
> > + goto out_put_power;
> > +
> > /* XXX: this only works for one DSI output */
> > for_each_dsi_port(port, intel_dsi->ports) {
> > i915_reg_t ctrl_reg = IS_BROXTON(dev) ?
> > @@ -726,6 +734,7 @@ static bool intel_dsi_get_hw_state(struct
> > intel_encoder *encoder,
> > break;
> > }
> >
> > +out_put_power:
> > intel_display_power_put(dev_priv, power_domain);
> >
> > return active;
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.h
> > b/drivers/gpu/drm/i915/intel_dsi.h
> > index e582ef8..ec58ead 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.h
> > +++ b/drivers/gpu/drm/i915/intel_dsi.h
> > @@ -126,6 +126,7 @@ static inline struct intel_dsi
> > *enc_to_intel_dsi(struct drm_encoder *encoder)
> > return container_of(encoder, struct intel_dsi, base.base);
> > }
> >
> > +bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv);
> > extern void intel_enable_dsi_pll(struct intel_encoder *encoder);
> > extern void intel_disable_dsi_pll(struct intel_encoder *encoder);
> > extern u32 intel_dsi_get_pclk(struct intel_encoder *encoder, int
> > pipe_bpp);
> > diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c
> > b/drivers/gpu/drm/i915/intel_dsi_pll.c
> > index e3e343c..e5b0625 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> > @@ -192,6 +192,33 @@ static void vlv_disable_dsi_pll(struct
> > intel_encoder *encoder)
> > mutex_unlock(&dev_priv->sb_lock);
> > }
> >
> > +static bool bxt_dsi_pll_is_enabled(struct drm_i915_private
> > *dev_priv)
> > +{
> > + bool enabled;
> > + u32 val;
> > + u32 mask;
> > +
> > + mask = BXT_DSI_PLL_DO_ENABLE | BXT_DSI_PLL_LOCKED;
> > + val = I915_READ(BXT_DSI_PLL_ENABLE);
> > + enabled = (val & mask) == mask;
> > +
> > + if (!enabled)
> > + return false;
> > +
> > + /*
> > + * Both dividers must be programmed with valid values even
> > if only one
> > + * of the PLL is used. See BSpec/Broxton Clocks.
> > + */
> > + val = I915_READ(BXT_DSI_PLL_CTL);
> > + if (!(val & BXT_DSIA_16X_MASK) || !(val &
> > BXT_DSIC_16X_MASK)) {
> > + DRM_DEBUG_DRIVER("PLL is enabled with invalid
> > divider settings "
> > + "(%08x)\n", val);
> > + enabled = false;
> > + }
>
> Do we really need this level of paranoia?
Yes, based on my test I think so, see above.
> > +
> > + return enabled;
> > +}
> > +
> > static void bxt_disable_dsi_pll(struct intel_encoder *encoder)
> > {
> > struct drm_i915_private *dev_priv = encoder->base.dev-
> > >dev_private;
> > @@ -486,6 +513,16 @@ static void bxt_enable_dsi_pll(struct
> > intel_encoder *encoder)
> > DRM_DEBUG_KMS("DSI PLL locked\n");
> > }
> >
> > +bool intel_dsi_pll_is_enabled(struct drm_i915_private *dev_priv)
> > +{
> > + if (IS_BROXTON(dev_priv))
> > + return bxt_dsi_pll_is_enabled(dev_priv);
> > +
> > + MISSING_CASE(INTEL_DEVID(dev_priv));
> > +
> > + return false;
> > +}
> > +
> > void intel_enable_dsi_pll(struct intel_encoder *encoder)
> > {
> > struct drm_device *dev = encoder->base.dev;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
