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

Shankar, Uma uma.shankar at intel.com
Thu Oct 1 09:22:57 PDT 2015



>-----Original Message-----
>From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
>Sent: Thursday, October 1, 2015 3:24 PM
>To: Shankar, Uma; intel-gfx at lists.freedesktop.org
>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma
>Subject: Re: [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in
>CRTC modeset
>
>On Wed, 30 Sep 2015, Uma Shankar <uma.shankar at intel.com> wrote:
>> From: Shashank Sharma <shashank.sharma at 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 at intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar at 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.
>

Ok got it. Will do the same.

Regards,
Uma Shankar
>>
>>  	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


More information about the Intel-gfx mailing list