Re: [PATCH v2 4/6] drm/i915/bxt: add dsi transcoders

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 18, 2016 at 05:05:42PM +0200, Jani Nikula wrote:
> The BXT display connections have DSI transcoders A and C that can be
> muxed to any pipe, not unlike the eDP transcoder. Add the notion of DSI
> transcoders.
> 
> The "normal" transcoders A, B and C are not used with BXT DSI, so care
> must be taken to avoid accessing those registers with DSI transcoders in
> the hardware state readout, modeset, and generally everywhere.
> 
> v2: addressing comments by Ville:
>  - rename the dsi get config function to hsw_get_dsi_transcoder_state
>  - rebase onto the higher level split of pipe/transcoder functions
>  - use more has_dsi_encoder as we can now because of the above,
>    with no need to look at the transcoder so much
>  - rename IS_DSI_TRANSCODER to transcoder_is_dsi
>  - use the above a bit more instead of comparing to < TRANSCODER_EDP
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 13 +++++
>  drivers/gpu/drm/i915/intel_ddi.c        |  6 +++
>  drivers/gpu/drm/i915/intel_display.c    | 91 +++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h        |  3 +-
>  drivers/gpu/drm/i915/intel_dsi.c        |  9 ++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  6 +++
>  6 files changed, 116 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f330a53c19b9..5e700770c403 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -127,6 +127,8 @@ enum transcoder {
>  	TRANSCODER_B,
>  	TRANSCODER_C,
>  	TRANSCODER_EDP,
> +	TRANSCODER_DSI_A,
> +	TRANSCODER_DSI_C,
>  	I915_MAX_TRANSCODERS
>  };
>  
> @@ -141,11 +143,20 @@ static inline const char *transcoder_name(enum transcoder transcoder)
>  		return "C";
>  	case TRANSCODER_EDP:
>  		return "EDP";
> +	case TRANSCODER_DSI_A:
> +		return "DSI A";
> +	case TRANSCODER_DSI_C:
> +		return "DSI C";
>  	default:
>  		return "<invalid>";
>  	}
>  }
>  
> +static inline bool transcoder_is_dsi(enum transcoder transcoder)
> +{
> +	return transcoder == TRANSCODER_DSI_A || transcoder == TRANSCODER_DSI_C;
> +}
> +
>  /*
>   * I915_MAX_PLANES in the enum below is the maximum (across all platforms)
>   * number of planes per CRTC.  Not all platforms really have this many planes,
> @@ -196,6 +207,8 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_TRANSCODER_B,
>  	POWER_DOMAIN_TRANSCODER_C,
>  	POWER_DOMAIN_TRANSCODER_EDP,
> +	POWER_DOMAIN_TRANSCODER_DSI_A,
> +	POWER_DOMAIN_TRANSCODER_DSI_C,
>  	POWER_DOMAIN_PORT_DDI_A_LANES,
>  	POWER_DOMAIN_PORT_DDI_B_LANES,
>  	POWER_DOMAIN_PORT_DDI_C_LANES,
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 91654ffc3a42..e6c3a80e1360 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1061,6 +1061,8 @@ void intel_ddi_set_pipe_settings(struct drm_crtc *crtc)
>  	uint32_t temp;
>  
>  	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP || type == INTEL_OUTPUT_DP_MST) {
> +		WARN_ON(transcoder_is_dsi(cpu_transcoder));
> +
>  		temp = TRANS_MSA_SYNC_CLK;
>  		switch (intel_crtc->config->pipe_bpp) {
>  		case 18:
> @@ -1942,6 +1944,10 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi;
>  	u32 temp, flags = 0;
>  
> +	/* XXX: DSI transcoder paranoia */
> +	if (WARN_ON(transcoder_is_dsi(cpu_transcoder)))
> +		return;
> +
>  	temp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
>  	if (temp & TRANS_DDI_PHSYNC)
>  		flags |= DRM_MODE_FLAG_PHSYNC;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 98d8b563b9a1..28ead66ed987 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4900,6 +4900,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	int pipe = intel_crtc->pipe, hsw_workaround_pipe;
> +	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->state);
>  
> @@ -4916,11 +4917,14 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	if (intel_crtc->config->has_dp_encoder)
>  		intel_dp_set_m_n(intel_crtc, M1_N1);
>  
> -	intel_set_pipe_timings(intel_crtc);
> +	if (!intel_crtc->config->has_dsi_encoder)
> +		intel_set_pipe_timings(intel_crtc);
> +
>  	intel_set_pipe_src_size(intel_crtc);
>  
> -	if (intel_crtc->config->cpu_transcoder != TRANSCODER_EDP) {
> -		I915_WRITE(PIPE_MULT(intel_crtc->config->cpu_transcoder),
> +	if (cpu_transcoder != TRANSCODER_EDP &&
> +	    !transcoder_is_dsi(cpu_transcoder)) {
> +		I915_WRITE(PIPE_MULT(cpu_transcoder),
>  			   intel_crtc->config->pixel_multiplier - 1);
>  	}
>  
> @@ -4929,7 +4933,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  				     &intel_crtc->config->fdi_m_n, NULL);
>  	}
>  
> -	haswell_set_pipeconf(crtc);
> +	if (!intel_crtc->config->has_dsi_encoder)
> +		haswell_set_pipeconf(crtc);
> +
>  	haswell_set_pipe_gamma(crtc);
>  	haswell_set_pipemisc(crtc);
>  
> @@ -4972,7 +4978,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		dev_priv->display.initial_watermarks(pipe_config);
>  	else
>  		intel_update_watermarks(crtc);
> -	intel_enable_pipe(intel_crtc);
> +
> +	/* XXX: Do the pipe assertions at the right place for BXT DSI. */
> +	if (!intel_crtc->config->has_dsi_encoder)
> +		intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
>  		lpt_pch_enable(crtc);
> @@ -5105,7 +5114,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	drm_crtc_vblank_off(crtc);
>  	assert_vblank_disabled(crtc);
>  
> -	intel_disable_pipe(intel_crtc);
> +	/* XXX: Do the pipe assertions at the right place for BXT DSI. */
> +	if (!intel_crtc->config->has_dsi_encoder)
> +		intel_disable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->dp_encoder_is_mst)
>  		intel_ddi_set_vc_payload_alloc(crtc, false);
> @@ -9957,6 +9968,47 @@ static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
>  	return tmp & PIPECONF_ENABLE;
>  }
>  
> +static bool bxt_get_dsi_transcoder_state(struct intel_crtc *crtc,
> +					 struct intel_crtc_state *pipe_config,
> +					 unsigned long *power_domain_mask)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
> +	enum port port;
> +	enum transcoder cpu_transcoder;
> +	u32 tmp;
> +
> +	pipe_config->has_dsi_encoder = false;
> +
> +	for_each_port_masked(port, BIT(PORT_A) | BIT(PORT_C)) {
> +		if (port == PORT_A)
> +			cpu_transcoder = TRANSCODER_DSI_A;
> +		else
> +			cpu_transcoder = TRANSCODER_DSI_C;
> +
> +		power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder);
> +		if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> +			continue;
> +		*power_domain_mask |= BIT(power_domain);
> +
> +		/* XXX: this works for video mode only */
> +		tmp = I915_READ(BXT_MIPI_PORT_CTRL(port));
> +		if (!(tmp & DPI_ENABLE))
> +			continue;
> +
> +		tmp = I915_READ(MIPI_CTRL(port));
> +		if ((tmp & BXT_PIPE_SELECT_MASK) != BXT_PIPE_SELECT(crtc->pipe))
> +			continue;
> +
> +		pipe_config->cpu_transcoder = cpu_transcoder;
> +		pipe_config->has_dsi_encoder = true;
> +		break;
> +	}
> +
> +	return pipe_config->has_dsi_encoder;
> +}
> +
>  static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  				       struct intel_crtc_state *pipe_config)
>  {
> @@ -10018,12 +10070,22 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  
>  	active = hsw_get_transcoder_state(crtc, pipe_config, &power_domain_mask);
>  
> +	if (IS_BROXTON(dev_priv)) {
> +		bxt_get_dsi_transcoder_state(crtc, pipe_config,
> +					     &power_domain_mask);
> +		WARN_ON(active && pipe_config->has_dsi_encoder);

The warn could use a small comment perhaps, explaining that DSI and DDI
transcoders shouldn't be enabled at the same time.

Anyway, patch looks good to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		if (pipe_config->has_dsi_encoder)
> +			active = true;
> +	}
> +
>  	if (!active)
>  		goto out;
>  
> -	haswell_get_ddi_port_state(crtc, pipe_config);
> +	if (!pipe_config->has_dsi_encoder) {
> +		haswell_get_ddi_port_state(crtc, pipe_config);
> +		intel_get_pipe_timings(crtc, pipe_config);
> +	}
>  
> -	intel_get_pipe_timings(crtc, pipe_config);
>  	intel_get_pipe_src_size(crtc, pipe_config);
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> @@ -10048,7 +10110,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) &&
>  			(I915_READ(IPS_CTL) & IPS_ENABLE);
>  
> -	if (pipe_config->cpu_transcoder != TRANSCODER_EDP) {
> +	if (pipe_config->cpu_transcoder != TRANSCODER_EDP &&
> +	    !transcoder_is_dsi(pipe_config->cpu_transcoder)) {
>  		pipe_config->pixel_multiplier =
>  			I915_READ(PIPE_MULT(pipe_config->cpu_transcoder)) + 1;
>  	} else {
> @@ -15520,10 +15583,15 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	i915_reg_t reg = PIPECONF(crtc->config->cpu_transcoder);
> +	enum transcoder cpu_transcoder = crtc->config->cpu_transcoder;
>  
>  	/* Clear any frame start delays used for debugging left by the BIOS */
> -	I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
> +	if (!transcoder_is_dsi(cpu_transcoder)) {
> +		i915_reg_t reg = PIPECONF(cpu_transcoder);
> +
> +		I915_WRITE(reg,
> +			   I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);
> +	}
>  
>  	/* restore vblank interrupts to correct state */
>  	drm_crtc_vblank_reset(&crtc->base);
> @@ -16194,6 +16262,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  			error->pipe[i].stat = I915_READ(PIPESTAT(i));
>  	}
>  
> +	/* Note: this does not include DSI transcoders. */
>  	error->num_transcoders = INTEL_INFO(dev)->num_pipes;
>  	if (HAS_DDI(dev_priv->dev))
>  		error->num_transcoders++; /* Account for eDP. */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5136eeffc24e..ba45245ad6c8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -437,7 +437,8 @@ struct intel_crtc_state {
>  	bool has_infoframe;
>  
>  	/* CPU Transcoder for the pipe. Currently this can only differ from the
> -	 * pipe on Haswell (where we have a special eDP transcoder). */
> +	 * pipe on Haswell and later (where we have a special eDP transcoder)
> +	 * and Broxton (where we have special DSI transcoders). */
>  	enum transcoder cpu_transcoder;
>  
>  	/*
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 3562bf337e62..1981212ffc8d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -268,6 +268,7 @@ static inline bool is_cmd_mode(struct intel_dsi *intel_dsi)
>  static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
>  	struct intel_dsi *intel_dsi = container_of(encoder, struct intel_dsi,
>  						   base);
>  	struct intel_connector *intel_connector = intel_dsi->attached_connector;
> @@ -284,6 +285,14 @@ static bool intel_dsi_compute_config(struct intel_encoder *encoder,
>  	/* DSI uses short packets for sync events, so clear mode flags for DSI */
>  	adjusted_mode->flags = 0;
>  
> +	if (IS_BROXTON(dev_priv)) {
> +		/* Dual link goes to DSI transcoder A. */
> +		if (intel_dsi->ports == BIT(PORT_C))
> +			pipe_config->cpu_transcoder = TRANSCODER_DSI_C;
> +		else
> +			pipe_config->cpu_transcoder = TRANSCODER_DSI_A;
> +	}
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 2e88a5e06884..d189a0012277 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -89,6 +89,10 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>  		return "TRANSCODER_C";
>  	case POWER_DOMAIN_TRANSCODER_EDP:
>  		return "TRANSCODER_EDP";
> +	case POWER_DOMAIN_TRANSCODER_DSI_A:
> +		return "TRANSCODER_DSI_A";
> +	case POWER_DOMAIN_TRANSCODER_DSI_C:
> +		return "TRANSCODER_DSI_C";
>  	case POWER_DOMAIN_PORT_DDI_A_LANES:
>  		return "PORT_DDI_A_LANES";
>  	case POWER_DOMAIN_PORT_DDI_B_LANES:
> @@ -419,6 +423,8 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,
>  	BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>  	BIT(POWER_DOMAIN_PIPE_A) |			\
>  	BIT(POWER_DOMAIN_TRANSCODER_EDP) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_DSI_A) |		\
> +	BIT(POWER_DOMAIN_TRANSCODER_DSI_C) |		\
>  	BIT(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_DSI) |			\
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx