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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 24 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.
> v3: (Jani)
> - Simplify check in bxt_get_dsi_transcoder_state()
> - Add comment explaining why we check for valid dividers in
>   bxt_dsi_pll_is_enabled()
>
> 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")
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>

Thanks for fixing this.

Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>


> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c     |  9 ++++++++
>  drivers/gpu/drm/i915/intel_dsi.h     |  1 +
>  drivers/gpu/drm/i915/intel_dsi_pll.c | 40 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 63 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..e05393e 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>
> @@ -9867,6 +9868,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]. We can break out here early, since we
> +		 * need the same DSI PLL to be enabled for both DSI ports.
> +		 */
> +		if (!intel_dsi_pll_is_enabled(dev_priv))
> +			break;
> +
>  		/* 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..ce688f9 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -192,6 +192,36 @@ 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. Check this here for
> +	 * paranoia, since BIOS is known to misconfigure PLLs in this way at
> +	 * times, and since accessing DSI registers with invalid dividers
> +	 * causes a system hang.
> +	 */
> +	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;
> +	}
> +
> +	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 +516,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;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux