Re: [PATCH v2] drm/i915/bxt: Fix DSI HW state readout

[Imre Deak <imre.deak@intel.com>]

On to, 2016-03-24 at 11:15 +0200, Jani Nikula wrote:
> On Wed, 23 Mar 2016, Imre Deak <imre.deak@intel.com> 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@intel.com>
> > CC: Uma Shankar <uma.shankar@intel.com>
> > CC: Jani Nikula <jani.nikula@intel.com>
> > 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@intel.com>
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx