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

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

On Tue, 01 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.
>
> 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      |   29 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c  |   19 ++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c   |    1 +
>  drivers/gpu/drm/i915/intel_opregion.c |    3 ++-
>  5 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fd1de45..78d31c5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -142,6 +142,7 @@ enum plane {
>  #define sprite_name(p, s) ((p) * INTEL_INFO(dev)->num_sprites[(p)] + (s) + 'A')
>  
>  enum port {
> +	PORT_INVALID = -1,
>  	PORT_A = 0,
>  	PORT_B,
>  	PORT_C,
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cacb07b..5d5aad2 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -227,6 +227,10 @@ static void ddi_get_encoder_port(struct intel_encoder *intel_encoder,
>  	} else if (type == INTEL_OUTPUT_ANALOG) {
>  		*dig_port = NULL;
>  		*port = PORT_E;
> +	} else if (type == INTEL_OUTPUT_DSI) {
> +		*dig_port = NULL;
> +		*port = PORT_INVALID;
> +		DRM_DEBUG_KMS("Encoder type: DSI. Returning...\n");

Please remind me again what are the legitimate paths to get here with
DSI?

With all the changes and warns across the driver, I'm beginning to think
we should have a version of this function that accepts DSI, and another
one that (calls the other one) and WARNS on DSI, and that should be
called on all paths that should never encounter a DSI encoder.

The proliferation of WARNS all over the place is not very nice.

I'm sorry, I know this is not the review I gave previously on this.


BR,
Jani.


>  	} else {
>  		DRM_ERROR("Invalid DDI encoder type %d\n", type);
>  		BUG();
> @@ -392,6 +396,11 @@ void intel_prepare_ddi(struct drm_device *dev)
>  
>  		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
>  
> +		if (port == PORT_INVALID) {
> +			WARN_ON(1);
> +			continue;
> +		}
> +
>  		if (visited[port])
>  			continue;
>  
> @@ -980,6 +989,8 @@ static void bxt_ddi_clock_get(struct intel_encoder *encoder,
>  	enum port port = intel_ddi_get_encoder_port(encoder);
>  	uint32_t dpll = port;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	pipe_config->port_clock =
>  		bxt_calc_pll_link(dev_priv, dpll);
>  
> @@ -1572,6 +1583,8 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
>  	int type = intel_encoder->type;
>  	uint32_t temp;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	/* Enable TRANS_DDI_FUNC_CTL for the pipe to work in HDMI mode */
>  	temp = TRANS_DDI_FUNC_ENABLE;
>  	temp |= TRANS_DDI_SELECT_PORT(port);
> @@ -1684,6 +1697,8 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
>  	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	if (!intel_display_power_is_enabled(dev_priv, power_domain))
>  		return false;
> @@ -1730,6 +1745,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder,
>  	u32 tmp;
>  	int i;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	power_domain = intel_display_port_power_domain(encoder);
>  	if (!intel_display_power_is_enabled(dev_priv, power_domain))
>  		return false;
> @@ -1779,11 +1796,14 @@ 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 transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	if (cpu_transcoder != TRANSCODER_EDP)
>  		I915_WRITE(TRANS_CLK_SEL(cpu_transcoder),
>  			   TRANS_CLK_SEL_PORT(port));
> @@ -1870,6 +1890,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	int type = intel_encoder->type;
>  	int hdmi_level;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  		intel_edp_panel_on(intel_dp);
> @@ -1947,6 +1969,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	uint32_t val;
>  	bool wait = false;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	val = I915_READ(DDI_BUF_CTL(port));
>  	if (val & DDI_BUF_CTL_ENABLE) {
>  		val &= ~DDI_BUF_CTL_ENABLE;
> @@ -1986,6 +2010,8 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
>  	int type = intel_encoder->type;
>  
> +	WARN_ON(port == PORT_INVALID);
> +
>  	if (type == INTEL_OUTPUT_HDMI) {
>  		struct intel_digital_port *intel_dig_port =
>  			enc_to_dig_port(encoder);
> @@ -2732,6 +2758,7 @@ static bool intel_ddi_compute_config(struct intel_encoder *encoder,
>  	int port = intel_ddi_get_encoder_port(encoder);
>  
>  	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> +	WARN_ON(port == PORT_INVALID);
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b8e0310..7f39cc9 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);
> @@ -5033,7 +5034,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 +5051,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,13 +5060,16 @@ 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);
>  	drm_crtc_vblank_on(crtc);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
> +		if (encoder->pre_pll_enable)
> +			encoder->pre_pll_enable(encoder);
> +
>  		encoder->enable(encoder);
>  		intel_opregion_notify_encoder(encoder, true);
>  	}
> @@ -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..d69186c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -173,6 +173,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  
>  	if (intel_dp->active_mst_links == 0) {
>  		enum port port = intel_ddi_get_encoder_port(encoder);
> +		WARN_ON(port == PORT_INVALID);
>  
>  		/* FIXME: add support for SKL */
>  		if (INTEL_INFO(dev)->gen < 9)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 4813374..326aa6b 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -335,7 +335,7 @@ int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  		return 0;
>  
>  	port = intel_ddi_get_encoder_port(intel_encoder);
> -	if (port == PORT_E) {
> +	if ((port == PORT_E) || (port == PORT_INVALID)) {
>  		port = 0;
>  	} else {
>  		parm |= 1 << port;
> @@ -356,6 +356,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