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

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

On Wed, 23 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.
>
> 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      |   21 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c  |   21 +++++++++++++++------
>  drivers/gpu/drm/i915/intel_opregion.c |    3 ++-
>  4 files changed, 38 insertions(+), 8 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,

My idea was that you wouldn't add this. Maybe I wasn't clear enough.

>  	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..8edb632 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");

My idea was that you'd only call this function on DDI (i.e. non-DSI)
encoders. So you could do a warn here. Doesn't matter what you set *port
to, it's going to be wrong anyway, and this is only slightly better than
not oopsing on the BUG below.

>  	} else {
>  		DRM_ERROR("Invalid DDI encoder type %d\n", type);
>  		BUG();
> @@ -237,6 +241,15 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
>  {
>  	struct intel_digital_port *dig_port;
>  	enum port port;
> +	int type = intel_encoder->type;
> +
> +	if (type == INTEL_OUTPUT_DSI) {
> +		port = PORT_INVALID;
> +		DRM_DEBUG_KMS("Encoder type: DSI. Returning...\n");
> +		WARN_ON(1);
> +
> +		return port;
> +	}

Remove these.

>  
>  	ddi_get_encoder_port(intel_encoder, &dig_port, &port);
>  
> @@ -392,6 +405,11 @@ void intel_prepare_ddi(struct drm_device *dev)
>  
>  		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
>  

My idea was that you'd only call this function on DDI (i.e. non-DSI)
encoders. So you'd have to add a check for DSI here.

> +		if (port == PORT_INVALID) {
> +			WARN_ON(1);

But this warn now makes me think we don't ever get here on with
DSI. Don't warn for normal cases.

> +			continue;
> +		}
> +
>  		if (visited[port])
>  			continue;
>  
> @@ -1779,7 +1797,8 @@ 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;
> 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_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);

My idea was that you'd only call this function on DDI (i.e. non-DSI)
encoders. So you'd have to add a check for DSI here.


BR,
Jani.

> -	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