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

Commit Message

Shankar, Uma Sept. 23, 2015, 5:53 p.m. UTC

From: Shashank Sharma <[e-mail]>

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 <[e-mail]>
Signed-off-by: Uma Shankar <[e-mail]>
---
 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(-)

Comments

Jani Nikula Sept. 28, 2015, 1:28 p.m. UTC | #1

On Wed, 23 Sep 2015, Uma Shankar <[e-mail]> wrote:
> From: Shashank Sharma <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Uma Shankar <[e-mail]>
> ---
>  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
>

Shankar, Uma Sept. 28, 2015, 4:57 p.m. UTC | #2

>-----Original Message-----
>From: Jani Nikula [mailto:[e-mail]]
>Sent: Monday, September 28, 2015 6:58 PM
>To: Shankar, Uma; [e-mail]
>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>Subject: Re: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in
>CRTC modeset
>
>On Wed, 23 Sep 2015, Uma Shankar <[e-mail]> wrote:
>> From: Shashank Sharma <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Uma Shankar <[e-mail]>
>> ---
>>  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.
>

intel_ddi_get_encoder_port is the one getting called from multiple locations. This expects an enum to be returned.  We could either set the *port in 

 @@ -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");

And let this function return the PORT_INVALID with a WARN. Or we can initialize the port to PORT_INVALID and return that instead. Then I can remove these lines from here.
Also, If we try to avoid this function getting called from various locations, we will again end up to the original problem of spilled over DSI checks at multiple places in code.

Please suggest which ever looks ok. 

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

Yes, you are right. This shouldn't be Warning, just a DSI protection should be fine. Will rectify this.

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

Currently, I have implemented it as all other occurrences of this function. Even if it gets called for DSI,
it will return PORT_INVALID. That case is handled below.

But it would be better to check for DSI and avoid the call and an unnecessary Warning. Will change this.

Please comment if the overall approach looks OK. Will implement accordingly. 

Thanks for all the suggestions and comments.

Regards,
Uma Shankar

>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

Jani Nikula Sept. 29, 2015, 7:29 a.m. UTC | #3

On Mon, 28 Sep 2015, "Shankar, Uma" <[e-mail]> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:[e-mail]]
>>Sent: Monday, September 28, 2015 6:58 PM
>>To: Shankar, Uma; [e-mail]
>>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>>Subject: Re: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in
>>CRTC modeset
>>
>>On Wed, 23 Sep 2015, Uma Shankar <[e-mail]> wrote:
>>> From: Shashank Sharma <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Uma Shankar <[e-mail]>
>>> ---
>>>  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.
>>
>
> intel_ddi_get_encoder_port is the one getting called from multiple locations. This expects an enum to be returned.  We could either set the *port in 
>
>  @@ -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");
>
> And let this function return the PORT_INVALID with a WARN. Or we can initialize the port to PORT_INVALID and return that instead. Then I can remove these lines from here.
> Also, If we try to avoid this function getting called from various locations, we will again end up to the original problem of spilled over DSI checks at multiple places in code.
>
> Please suggest which ever looks ok. 

I just sent two patches [1][2] to modify ddi_get_encoder_port a
little. Rebase on top, and don't modify ddi_get_encoder_port() or
intel_ddi_get_encoder_port() at all. Just make sure you don't call the
functions for DSI. No need to add PORT_INVALID either.

BR,
Jani.


[1] http://mid.gmane.org/[e-mail]
[2] http://mid.gmane.org/[e-mail]

>
>>>
>>>  	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.
>
> Yes, you are right. This shouldn't be Warning, just a DSI protection should be fine. Will rectify this.
>
>>> +			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.
>>
>
> Currently, I have implemented it as all other occurrences of this function. Even if it gets called for DSI,
> it will return PORT_INVALID. That case is handled below.
>
> But it would be better to check for DSI and avoid the call and an unnecessary Warning. Will change this.
>
> Please comment if the overall approach looks OK. Will implement accordingly. 
>
> Thanks for all the suggestions and comments.
>
> Regards,
> Uma Shankar
>
>>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

Shankar, Uma Sept. 30, 2015, 4:33 p.m. UTC | #4

>-----Original Message-----
>From: Jani Nikula [mailto:[e-mail]]
>Sent: Tuesday, September 29, 2015 12:59 PM
>To: Shankar, Uma; [e-mail]
>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank
>Subject: RE: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in
>CRTC modeset
>
>On Mon, 28 Sep 2015, "Shankar, Uma" <[e-mail]> wrote:
>>>-----Original Message-----
>>>From: Jani Nikula [mailto:[e-mail]]
>>>Sent: Monday, September 28, 2015 6:58 PM
>>>To: Shankar, Uma; [e-mail]
>>>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>>>Subject: Re: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder
>>>support in CRTC modeset
>>>
>>>On Wed, 23 Sep 2015, Uma Shankar <[e-mail]> wrote:
>>>> From: Shashank Sharma <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Uma Shankar <[e-mail]>
>>>> ---
>>>>  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.
>>>
>>
>> intel_ddi_get_encoder_port is the one getting called from multiple
>> locations. This expects an enum to be returned.  We could either set
>> the *port in
>>
>>  @@ -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");
>>
>> And let this function return the PORT_INVALID with a WARN. Or we can
>initialize the port to PORT_INVALID and return that instead. Then I can remove
>these lines from here.
>> Also, If we try to avoid this function getting called from various locations, we
>will again end up to the original problem of spilled over DSI checks at multiple
>places in code.
>>
>> Please suggest which ever looks ok.
>
>I just sent two patches [1][2] to modify ddi_get_encoder_port a little. Rebase on
>top, and don't modify ddi_get_encoder_port() or
>intel_ddi_get_encoder_port() at all. Just make sure you don't call the functions
>for DSI. No need to add PORT_INVALID either.
>
>BR,
>Jani.
>

Thanks Jani.  I will rebase and re-submit. Personally I feel this will be best approach as it will avoid the call itself.
However, only flip side is protection at multiple places in code for DSI. 

Regards,
Uma Shankar


>[1] http://mid.gmane.org/1443511466-8017-1-git-send-email-
>[e-mail]
>[2] http://mid.gmane.org/1443511466-8017-2-git-send-email-
>[e-mail]
>
>>
>>>>
>>>>  	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.
>>
>> Yes, you are right. This shouldn't be Warning, just a DSI protection should be
>fine. Will rectify this.
>>
>>>> +			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.
>>>
>>
>> Currently, I have implemented it as all other occurrences of this
>> function. Even if it gets called for DSI, it will return PORT_INVALID. That case is
>handled below.
>>
>> But it would be better to check for DSI and avoid the call and an unnecessary
>Warning. Will change this.
>>
>> Please comment if the overall approach looks OK. Will implement accordingly.
>>
>> Thanks for all the suggestions and comments.
>>
>> Regards,
>> Uma Shankar
>>
>>>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
>
>--
>Jani Nikula, Intel Open Source Technology Center

Jani Nikula Oct. 1, 2015, 9:56 a.m. UTC | #5

On Wed, 30 Sep 2015, "Shankar, Uma" <[e-mail]> wrote:
>>-----Original Message-----
>>From: Jani Nikula [mailto:[e-mail]]
>>Sent: Tuesday, September 29, 2015 12:59 PM
>>To: Shankar, Uma; [e-mail]
>>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank
>>Subject: RE: [BXT MIPI PATCH v4 05/14] drm/i915/bxt: DSI encoder support in
>>CRTC modeset
>>
>>I just sent two patches [1][2] to modify ddi_get_encoder_port a
>>little. Rebase on top, and don't modify ddi_get_encoder_port() or
>>intel_ddi_get_encoder_port() at all. Just make sure you don't call the
>>functions for DSI. No need to add PORT_INVALID either.
>>
>>BR,
>>Jani.
>>
>
> Thanks Jani.  I will rebase and re-submit. Personally I feel this will
> be best approach as it will avoid the call itself.  However, only flip
> side is protection at multiple places in code for DSI.

As I explain in my review, it is not necessary to add those checks
everywhere.

BR,
Jani.

>>[1] http://mid.gmane.org/1443511466-8017-1-git-send-email-
>>[e-mail]
>>[2] http://mid.gmane.org/1443511466-8017-2-git-send-email-
>>[e-mail]

Patch

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..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");
 	} 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;
+	}
 
 	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);
 
+		if (port == PORT_INVALID) {
+			WARN_ON(1);
+			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);
-	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: