[Intel-gfx] [PATCH 1/2] drm/i915/BXT: Fixed COS blanking issue

[Daniel Vetter]

On Wed, Feb 03, 2016 at 11:44:03AM +0200, Jani Nikula wrote:
> On Tue, 02 Feb 2016, Ramalingam C <ramalingam.c at intel.com> wrote:
> > From: Uma Shankar <uma.shankar at 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.
> >
> > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  107 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 101 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a66220a..f8685f5 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7814,6 +7814,69 @@ 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;
> > +	enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > +	struct intel_encoder *encoder;
> > +	uint32_t tmp;
> > +
> > +	tmp = I915_READ(HTOTAL(cpu_transcoder));
> > +	pipe_config->base.adjusted_mode.crtc_hdisplay = (tmp & 0xffff) + 1;
> > +	pipe_config->base.adjusted_mode.crtc_htotal =
> > +						((tmp >> 16) & 0xffff) + 1;
> > +	tmp = I915_READ(HBLANK(cpu_transcoder));
> > +	pipe_config->base.adjusted_mode.crtc_hblank_start = (tmp & 0xffff) + 1;
> > +	pipe_config->base.adjusted_mode.crtc_hblank_end =
> > +						((tmp >> 16) & 0xffff) + 1;
> > +	tmp = I915_READ(HSYNC(cpu_transcoder));
> > +	pipe_config->base.adjusted_mode.crtc_hsync_start = (tmp & 0xffff) + 1;
> > +	pipe_config->base.adjusted_mode.crtc_hsync_end =
> > +						((tmp >> 16) & 0xffff) + 1;
> > +
> > +	tmp = I915_READ(VBLANK(cpu_transcoder));
> > +	pipe_config->base.adjusted_mode.crtc_vblank_start = (tmp & 0xffff) + 1;
> > +	pipe_config->base.adjusted_mode.crtc_vblank_end =
> > +						((tmp >> 16) & 0xffff) + 1;
> > +	tmp = I915_READ(VSYNC(cpu_transcoder));
> > +	pipe_config->base.adjusted_mode.crtc_vsync_start = (tmp & 0xffff) + 1;
> > +	pipe_config->base.adjusted_mode.crtc_vsync_end =
> > +						((tmp >> 16) & 0xffff) + 1;
> > +
> > +	if (I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_INTERLACE_MASK) {
> > +		pipe_config->base.adjusted_mode.flags |=
> > +						DRM_MODE_FLAG_INTERLACE;
> > +		pipe_config->base.adjusted_mode.crtc_vtotal += 1;
> > +		pipe_config->base.adjusted_mode.crtc_vblank_end += 1;
> > +	}
> > +
> > +
> > +	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
> > +		struct intel_dsi *intel_dsi =
> > +			enc_to_intel_dsi(&encoder->base);
> > +		enum port port;
> > +
> > +		pipe_config->pipe_bpp = intel_dsi->dsi_bpp;
> > +		for_each_dsi_port(port, intel_dsi->ports) {
> > +			pipe_config->base.adjusted_mode.crtc_hdisplay =
> > +				I915_READ(BXT_MIPI_TRANS_HACTIVE(port));
> > +			pipe_config->base.adjusted_mode.crtc_vdisplay =
> > +				I915_READ(BXT_MIPI_TRANS_VACTIVE(port));
> > +			pipe_config->base.adjusted_mode.crtc_vtotal =
> > +				I915_READ(BXT_MIPI_TRANS_VTOTAL(port));
> > +		}
> > +	}
> > +
> > +	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)
> >  {
> > @@ -9969,6 +10032,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	enum intel_display_power_domain pfit_domain;
> >  	uint32_t tmp;
> > +	bool is_dsi = false;
> >  
> >  	if (!intel_display_power_is_enabled(dev_priv,
> >  					 POWER_DOMAIN_PIPE(crtc->pipe)))
> > @@ -9999,17 +10063,48 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >  			pipe_config->cpu_transcoder = TRANSCODER_EDP;
> >  	}
> >  
> > +	if (dev_priv->vbt.has_mipi) {
> > +		enum port port_num = (dev_priv->vbt.dsi.port == DVO_PORT_MIPIA)
> > +					? PORT_A : PORT_C;
> 
> Just "enum port port" please.
> 
> > +		uint32_t dsi_ctrl = I915_READ(MIPI_CTRL(port_num));
> > +
> > +		tmp = I915_READ(BXT_MIPI_PORT_CTRL(port_num));
> > +		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 false;
> > +			case BXT_PIPE_SELECT_A:
> > +				trans_dsi_pipe = PIPE_A;
> > +				break;
> > +			case BXT_PIPE_SELECT_B:
> > +				trans_dsi_pipe = PIPE_B;
> > +				break;
> > +			case BXT_PIPE_SELECT_C:
> > +				trans_dsi_pipe = PIPE_C;
> > +				break;
> > +			}
> > +
> > +			if (trans_dsi_pipe == crtc->pipe)
> > +				is_dsi = true;
> > +		}
> > +	}
> > +
> 
> You need to set pipe_config->has_dsi_encoder.
> 
> This is possibly cleanest, if you abstract the above into a separate
> function, say
> 
> bxt_get_pipe_config_dsi(struct intel_crtc *crtc, struct intel_crtc_state
> *pipe_config)
> 
> and set pipe_config->has_dsi_encoder if trans_dsi_pipe == crtc->pipe.
> 
> This probably needs to take into account the different power domains as
> well?
> 
> BR,
> Jani.
> 
> 
> PS. I guess we're missing the has_dsi_encoder readout for BYT/CHV as
> well. :(

That means we really, really should have a pipe config check for
has_dsi_encoder, which we're lacking too. Plus we don't even have a
machine with DSI in CI, which is why no one noticed that either.

Please add all the missing bits here. Iron rule is that if you read out
some hw state, then you _must_ add corresponding state checks ot
intel_pipe_config_compare.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch