Re: [PATCH 2/3 V3] drm/i915/BXT: Fixed COS blanking issue

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 11 Feb 2016, Ramalingam C <ramalingam.c@intel.com> wrote:
> From: Uma Shankar <uma.shankar@intel.com>
>
> During Charging OS mode, mipi display was blanking.This is
> because during driver load, though encoder, connector were
> active but crtc returned inactive. This caused sanitize
> function to disable the DSI panel. In AOS, this is fine
> since HWC will do a modeset and crtc, connector, encoder
> mapping will be restored. But in COS, no modeset is called,
> it just calls DPMS ON/OFF.
>
> This is fine on BYT/CHT since transcoder is common b/w
> all encoders. But for BXT, there is a separate mipi
> transcoder. Hence this needs special handling for BXT.
>
> V2: pipe_config->has_dsi_encoder is set for BXT [By Jani]
>
> V3: Timing parameters are retrieved from VBT. [By Jani]
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  106 ++++++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a66220a..e47a75c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drmP.h>
>  #include "intel_drv.h"
> +#include "intel_dsi.h"
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include "i915_trace.h"
> @@ -7814,6 +7815,53 @@ static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
>  		   (intel_crtc->config->pipe_src_h - 1));
>  }
>  
> +static void intel_get_dsi_pipe_timings(struct intel_crtc *crtc,
> +				   struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_display_mode *vbt_dsi_mode =
> +						dev_priv->vbt.lfp_lvds_vbt_mode;

The whole point of the function (and the call chain here) is to get the
information from the hardware, so that we can a) set up the internal
software state according to the hardware state at driver load, and b)
later cross check the hardware state against the software state. Reading
the information from the VBT provides no value.

> +	struct intel_encoder *encoder;
> +	uint32_t tmp;
> +
> +	pipe_config->base.adjusted_mode.crtc_hdisplay = vbt_dsi_mode->hdisplay;
> +	pipe_config->base.adjusted_mode.crtc_htotal = vbt_dsi_mode->htotal;
> +	pipe_config->base.adjusted_mode.crtc_hblank_start =
> +						vbt_dsi_mode->hdisplay;
> +	pipe_config->base.adjusted_mode.crtc_hblank_end = vbt_dsi_mode->htotal;
> +	pipe_config->base.adjusted_mode.crtc_hsync_start =
> +						vbt_dsi_mode->hsync_start;
> +	pipe_config->base.adjusted_mode.crtc_hsync_end =
> +						vbt_dsi_mode->hsync_end;
> +
> +	pipe_config->base.adjusted_mode.crtc_vdisplay = vbt_dsi_mode->vdisplay;
> +	pipe_config->base.adjusted_mode.crtc_vtotal = vbt_dsi_mode->vtotal;
> +	pipe_config->base.adjusted_mode.crtc_vblank_start =
> +							vbt_dsi_mode->vdisplay;
> +	pipe_config->base.adjusted_mode.crtc_vblank_end = vbt_dsi_mode->vtotal;
> +	pipe_config->base.adjusted_mode.crtc_vsync_start =
> +						vbt_dsi_mode->vsync_start;
> +	pipe_config->base.adjusted_mode.crtc_vsync_end =
> +						vbt_dsi_mode->vsync_end;
> +
> +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> +		if (encoder->type == INTEL_OUTPUT_DSI) {
> +			struct intel_dsi *intel_dsi =
> +				enc_to_intel_dsi(&encoder->base);
> +			pipe_config->pipe_bpp =
> +				dsi_pixel_format_bpp(intel_dsi->pixel_format);
> +		}
> +	}

Things like this should be done in the encoder->get_config hook. Maybe
*all* of this DSI timing business should be done there.

Maybe there needs to be a bxt specific get_config hook for DSI.

> +
> +	tmp = I915_READ(PIPESRC(crtc->pipe));
> +	pipe_config->pipe_src_h = (tmp & 0xffff) + 1;
> +	pipe_config->pipe_src_w = ((tmp >> 16) & 0xffff) + 1;
> +
> +	pipe_config->base.mode.vdisplay = pipe_config->pipe_src_h;
> +	pipe_config->base.mode.hdisplay = pipe_config->pipe_src_w;
> +}
> +
>  static void intel_get_pipe_timings(struct intel_crtc *crtc,
>  				   struct intel_crtc_state *pipe_config)
>  {
> @@ -9962,6 +10010,40 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  	}
>  }
>  
> +static void bxt_get_pipe_config_dsi(struct intel_crtc *crtc,
> +					struct intel_crtc_state *pipe_config)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum port port = (dev_priv->vbt.dsi.port == DVO_PORT_MIPIA)
> +				? PORT_A : PORT_C;

Again, should read the hardware, not VBT.

> +	uint32_t dsi_ctrl = I915_READ(MIPI_CTRL(port));
> +	uint32_t tmp;
> +
> +	tmp = I915_READ(BXT_MIPI_PORT_CTRL(port));
> +	if (tmp & DPI_ENABLE) {
> +		enum pipe trans_dsi_pipe;
> +
> +		switch (dsi_ctrl & BXT_PIPE_SELECT_MASK) {
> +		default:
> +			WARN(1, "unknown pipe linked to dsi transcoder\n");
> +			return;
> +		case BXT_PIPE_SELECT(PIPE_A):
> +			trans_dsi_pipe = PIPE_A;
> +			break;
> +		case BXT_PIPE_SELECT(PIPE_B):
> +			trans_dsi_pipe = PIPE_B;
> +			break;
> +		case BXT_PIPE_SELECT(PIPE_C):
> +			trans_dsi_pipe = PIPE_C;
> +			break;
> +		}
> +
> +		if (trans_dsi_pipe == crtc->pipe)
> +			pipe_config->has_dsi_encoder = true;
> +	}
> +}
> +
>  static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  				    struct intel_crtc_state *pipe_config)
>  {
> @@ -9999,17 +10081,29 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  			pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  	}
>  
> +	if (dev_priv->vbt.has_mipi && IS_BROXTON(dev))
> +		bxt_get_pipe_config_dsi(crtc, pipe_config);

We should read the hardware, not VBT. The point is, what if we have DSI
enabled even though VBT says no? Then we have a problem somewhere, and
this code is used for cross checking such problems.

But maybe this should be moved into the encoder hook no matter what.

> +
>  	if (!intel_display_power_is_enabled(dev_priv,
>  			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
>  		return false;
>  
> -	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
> -	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> -
> -	haswell_get_ddi_port_state(crtc, pipe_config);
> +	if (pipe_config->has_dsi_encoder && IS_BROXTON(dev))

No need to check for IS_BROXTON.

> +		/*
> +		 * BXT doesn't have the PIPE timing registers.
> +		 * Hence retrieved the pipe detail from PORT register.
> +		 * And timing parameters are obtained from VBT
> +		 */
> +		intel_get_dsi_pipe_timings(crtc, pipe_config);
> +	else {
> +		tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
> +		if (!(tmp & PIPECONF_ENABLE))
> +			return false;
> +		if (!IS_VALLEYVIEW(dev))
> +			haswell_get_ddi_port_state(crtc, pipe_config);
>  
> -	intel_get_pipe_timings(crtc, pipe_config);
> +		intel_get_pipe_timings(crtc, pipe_config);
> +	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_init_scalers(dev, crtc, pipe_config);

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