Re: [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

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

On Wed, 30 Sep 2015, Uma Shankar <uma.shankar@intel.com> wrote:
> From: Shashank Sharma <shashank.sharma@intel.com>
>
> SKL and BXT qualifies the HAS_DDI() check, and hence haswell
> modeset functions are re-used for modeset sequence. But DDI
> interface doesn't include support for DSI.
> This patch adds:
> 1. cases for DSI encoder, in those modeset functions and allows
>    a CRTC modeset
> 2. Adds call to pre_pll enabled from CRTC modeset function. Nothing
>    needs to be done as such in CRTC for DSI encoder, as PLL, clock
>    and and transcoder programming will be taken care in encoder's
>    pre_enable and pre_pll_enable function.
>
> v2: Fixed Jani's review comments. Added INVALID_PORT for non DDI
>     encoder like DSI for platforms having HAS_DDI as true.
>
> v3: Rebased on latest drm-nightly branch. Added a WARN_ON for invalid
>     encoder.
>
> v4: WARN_ON for invalid encoder is refactored as per Jani's suggestion.
>     Fixed the sequence for pre_pll_enable.
>
> v5: Protected DDI code paths in case of DSI encoder calls.
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |    1 +
>  drivers/gpu/drm/i915/intel_ddi.c      |   83 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_display.c  |   21 ++++++---
>  drivers/gpu/drm/i915/intel_dp_mst.c   |    8 +++-
>  drivers/gpu/drm/i915/intel_opregion.c |    9 +++-
>  5 files changed, 101 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fd1de45..f97a2a2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2482,6 +2482,7 @@ struct drm_i915_cmd_table {
>  				 INTEL_INFO(dev)->gen >= 9)
>  
>  #define HAS_DDI(dev)		(INTEL_INFO(dev)->has_ddi)
> +#define has_encoder_ddi(type)  ((type) == (INTEL_OUTPUT_DSI) ?  0 : 1)

Drop this change.

>  #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev) || \
>  				 IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cacb07b..4abc13d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -390,8 +390,10 @@ void intel_prepare_ddi(struct drm_device *dev)
>  		enum port port;
>  		bool supports_hdmi;
>  
> -		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
> +		if (intel_encoder->type == INTEL_OUTPUT_DSI)
> +			continue;
>  

This is good.

> +		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
>  		if (visited[port])
>  			continue;
>  
> @@ -977,8 +979,16 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
>  				struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> -	uint32_t dpll = port;
> +	enum port port;
> +	uint32_t dpll;
> +
> +	if (!has_encoder_ddi(encoder->type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(encoder);
> +	dpll = port;

No. Drop this change.

Ask yourself, is it okay to call this function (bxt_ddi_clock_get) if
the encoder type is DSI.

No, it's not. We should not be here, at all, if encoder is DSI.

We have modified ddi_get_encoder_port (and therefore
intel_ddi_get_encoder_port) to emit a big warning if that function is
called on a DSI encoder.

So don't make those extra checks. We'll catch the bugs with the warning
in ddi_get_encoder_port. We'll fix the code paths, and move on.

We don't care if the function goes on and does something stupid because
we called it with the wrong preconditions. Similarly, we don't check
for, say, IS_GEN2() in DDI code, because that should not have
happened. The bug is somewhere else.

Now, there *are* valid code paths which will be called if encoder is
DSI. In those cases, we need to handle DSI gracefully and not call
ddi_get_encoder_port. For example intel_prepare_ddi above and
intel_opregion_notify_encoder below.

>  
>  	pipe_config->port_clock =
>  		bxt_calc_pll_link(dev_priv, dpll);
> @@ -1568,10 +1578,16 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum pipe pipe = intel_crtc->pipe;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	int type = intel_encoder->type;
>  	uint32_t temp;
>  
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	/* Enable TRANS_DDI_FUNC_CTL for the pipe to work in HDMI mode */
>  	temp = TRANS_DDI_FUNC_ENABLE;
>  	temp |= TRANS_DDI_SELECT_PORT(port);
> @@ -1678,12 +1694,18 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder = intel_connector->encoder;
>  	int type = intel_connector->base.connector_type;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	enum pipe pipe = 0;
>  	enum transcoder cpu_transcoder;
>  	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
>  
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return false;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	if (!intel_display_power_is_enabled(dev_priv, power_domain))
>  		return false;
> @@ -1725,11 +1747,17 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum port port;
>  	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  	int i;
>  
> +	if (!has_encoder_ddi(encoder->type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return false;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(encoder);

Drop this change.

>  	power_domain = intel_display_port_power_domain(encoder);
>  	if (!intel_display_power_is_enabled(dev_priv, power_domain))
>  		return false;
> @@ -1779,11 +1807,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
> +	if (!has_encoder_ddi(intel_encoder->type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	if (cpu_transcoder != TRANSCODER_EDP)
>  		I915_WRITE(TRANS_CLK_SEL(cpu_transcoder),
>  			   TRANS_CLK_SEL_PORT(port));
> @@ -1866,10 +1901,16 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	int type = intel_encoder->type;
>  	int hdmi_level;
>  
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  		intel_edp_panel_on(intel_dp);
> @@ -1942,11 +1983,17 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	int type = intel_encoder->type;
>  	uint32_t val;
>  	bool wait = false;
>  
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	val = I915_READ(DDI_BUF_CTL(port));
>  	if (val & DDI_BUF_CTL_ENABLE) {
>  		val &= ~DDI_BUF_CTL_ENABLE;
> @@ -1983,9 +2030,15 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum port port = intel_ddi_get_encoder_port(intel_encoder);
> +	enum port port;
>  	int type = intel_encoder->type;
>  
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return;
> +	}
> +
> +	port = intel_ddi_get_encoder_port(intel_encoder);

Drop this change.

>  	if (type == INTEL_OUTPUT_HDMI) {
>  		struct intel_digital_port *intel_dig_port =
>  			enc_to_dig_port(encoder);
> @@ -2729,8 +2782,14 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  				     struct intel_crtc_state *pipe_config)
>  {
>  	int type = encoder->type;
> -	int port = intel_ddi_get_encoder_port(encoder);
> +	int port;
> +
> +	if (!has_encoder_ddi(type)) {
> +		DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +		return false;
> +	}
>  
> +	port = intel_ddi_get_encoder_port(encoder);

Drop this change.

>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>  
>  	if (port == PORT_A)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b8e0310..ea0f533 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4991,6 +4991,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
> +	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>  	int pipe = intel_crtc->pipe;
>  
>  	WARN_ON(!crtc->state->enable);
> @@ -5023,9 +5024,12 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc->active = true;
>  
>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> -	for_each_encoder_on_crtc(dev, crtc, encoder)
> +	for_each_encoder_on_crtc(dev, crtc, encoder) {
> +		if (encoder->pre_pll_enable)
> +			encoder->pre_pll_enable(encoder);
>  		if (encoder->pre_enable)
>  			encoder->pre_enable(encoder);
> +	}
>  
>  	if (intel_crtc->config->has_pch_encoder) {
>  		intel_set_pch_fifo_underrun_reporting(dev_priv, TRANSCODER_A,
> @@ -5033,7 +5037,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		dev_priv->display.fdi_link_train(crtc);
>  	}
>  
> -	intel_ddi_enable_pipe_clock(intel_crtc);
> +	if (!is_dsi)
> +		intel_ddi_enable_pipe_clock(intel_crtc);
>  
>  	if (INTEL_INFO(dev)->gen == 9)
>  		skylake_pfit_update(intel_crtc, 1);
> @@ -5049,7 +5054,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_load_lut(crtc);
>  
>  	intel_ddi_set_pipe_settings(crtc);
> -	intel_ddi_enable_transcoder_func(crtc);
> +	if (!is_dsi)
> +		intel_ddi_enable_transcoder_func(crtc);
>  
>  	intel_update_watermarks(crtc);
>  	intel_enable_pipe(intel_crtc);
> @@ -5057,7 +5063,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	if (intel_crtc->config->has_pch_encoder)
>  		lpt_pch_enable(crtc);
>  
> -	if (intel_crtc->config->dp_encoder_is_mst)
> +	if (intel_crtc->config->dp_encoder_is_mst && !is_dsi)
>  		intel_ddi_set_vc_payload_alloc(crtc, true);
>  
>  	assert_vblank_disabled(crtc);
> @@ -5159,6 +5165,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
> +	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -5179,7 +5186,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	if (intel_crtc->config->dp_encoder_is_mst)
>  		intel_ddi_set_vc_payload_alloc(crtc, false);
>  
> -	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
> +	if (!is_dsi)
> +		intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>  
>  	if (INTEL_INFO(dev)->gen == 9)
>  		skylake_pfit_update(intel_crtc, 0);
> @@ -5188,7 +5196,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	else
>  		MISSING_CASE(INTEL_INFO(dev)->gen);
>  
> -	intel_ddi_disable_pipe_clock(intel_crtc);
> +	if (!is_dsi)
> +		intel_ddi_disable_pipe_clock(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder) {
>  		lpt_disable_pch_transcoder(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 600afdb..00327bc 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -172,8 +172,14 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  	intel_mst->port = found->port;
>  
>  	if (intel_dp->active_mst_links == 0) {
> -		enum port port = intel_ddi_get_encoder_port(encoder);
> +		enum port port;
>  
> +		if (!has_encoder_ddi(encoder->type)) {
> +			DRM_DEBUG_KMS("DDI func getting called for DSI?\n");
> +			return;
> +		}
> +
> +		port = intel_ddi_get_encoder_port(encoder);

Drop this change.

>  		/* FIXME: add support for SKL */
>  		if (INTEL_INFO(dev)->gen < 9)
>  			I915_WRITE(PORT_CLK_SEL(port),
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 4813374..64b8bcd 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -334,8 +334,12 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  	if (!HAS_DDI(dev))
>  		return 0;
>  
> -	port = intel_ddi_get_encoder_port(intel_encoder);
> -	if (port == PORT_E) {
> +	if (!has_encoder_ddi(type))

This is good, but don't add extra wrapper (has_encoder_ddi) for the
encode type check.

> +		port = 0;
> +	else
> +		port = intel_ddi_get_encoder_port(intel_encoder);
> +
> +	if (port == PORT_E)  {
>  		port = 0;
>  	} else {
>  		parm |= 1 << port;
> @@ -356,6 +360,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  		type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
>  		break;
>  	case INTEL_OUTPUT_EDP:
> +	case INTEL_OUTPUT_DSI:
>  		type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
>  		break;
>  	default:
> -- 
> 1.7.9.5
>

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