Re: [PATCH] drm/i915/icl: push pll to port mapping/unmapping to ddi encoder hooks

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

On Mon, 03 Dec 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 29, 2018 at 01:57:15PM +0200, Jani Nikula wrote:
>> Unclutter the haswell_crtc_enable() and haswell_crtc_disable() functions
>> a bit by moving the pll to port mapping and unmapping functions to the
>> ddi encoder hooks. This allows removal of a bunch of boilerplate code
>> from the functions.
>> 
>> Additionally, the ICL DSI encoder needs to do the clock gating and
>> ungating slightly differently, and this allows its own handling in a
>> clean fashion.
>> 
>> Cc: Madhav Chauhan <madhav.chauhan@intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, pushed to dinq as part of [1].

BR,
Jani.


[1] https://patchwork.freedesktop.org/series/51011/


>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c     | 84 +++++++++++++++---------------------
>>  drivers/gpu/drm/i915/intel_display.c |  6 ---
>>  drivers/gpu/drm/i915/intel_drv.h     |  6 ---
>>  3 files changed, 34 insertions(+), 62 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index ad11540ac436..7bad6c857b81 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2785,69 +2785,45 @@ uint32_t icl_dpclka_cfgcr0_clk_off(struct drm_i915_private *dev_priv,
>>  	return 0;
>>  }
>>  
>> -void icl_map_plls_to_ports(struct drm_crtc *crtc,
>> -			   struct intel_crtc_state *crtc_state,
>> -			   struct drm_atomic_state *old_state)
>> +static void icl_map_plls_to_ports(struct intel_encoder *encoder,
>> +				  const struct intel_crtc_state *crtc_state)
>>  {
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	struct intel_shared_dpll *pll = crtc_state->shared_dpll;
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> -	struct drm_connector_state *conn_state;
>> -	struct drm_connector *conn;
>> -	int i;
>> -
>> -	for_each_new_connector_in_state(old_state, conn, conn_state, i) {
>> -		struct intel_encoder *encoder =
>> -			to_intel_encoder(conn_state->best_encoder);
>> -		enum port port;
>> -		uint32_t val;
>> -
>> -		if (conn_state->crtc != crtc)
>> -			continue;
>> -
>> -		port = encoder->port;
>> -		mutex_lock(&dev_priv->dpll_lock);
>> +	enum port port = encoder->port;
>> +	u32 val;
>>  
>> -		val = I915_READ(DPCLKA_CFGCR0_ICL);
>> -		WARN_ON((val & icl_dpclka_cfgcr0_clk_off(dev_priv, port)) == 0);
>> +	mutex_lock(&dev_priv->dpll_lock);
>>  
>> -		if (intel_port_is_combophy(dev_priv, port)) {
>> -			val &= ~DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>> -			val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, port);
>> -			I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>> -			POSTING_READ(DPCLKA_CFGCR0_ICL);
>> -		}
>> +	val = I915_READ(DPCLKA_CFGCR0_ICL);
>> +	WARN_ON((val & icl_dpclka_cfgcr0_clk_off(dev_priv, port)) == 0);
>>  
>> -		val &= ~icl_dpclka_cfgcr0_clk_off(dev_priv, port);
>> +	if (intel_port_is_combophy(dev_priv, port)) {
>> +		val &= ~DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>> +		val |= DPCLKA_CFGCR0_DDI_CLK_SEL(pll->info->id, port);
>>  		I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>> -
>> -		mutex_unlock(&dev_priv->dpll_lock);
>> +		POSTING_READ(DPCLKA_CFGCR0_ICL);
>>  	}
>> +
>> +	val &= ~icl_dpclka_cfgcr0_clk_off(dev_priv, port);
>> +	I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>> +
>> +	mutex_unlock(&dev_priv->dpll_lock);
>>  }
>>  
>> -void icl_unmap_plls_to_ports(struct drm_crtc *crtc,
>> -			     struct intel_crtc_state *crtc_state,
>> -			     struct drm_atomic_state *old_state)
>> +static void icl_unmap_plls_to_ports(struct intel_encoder *encoder)
>>  {
>> -	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>> -	struct drm_connector_state *old_conn_state;
>> -	struct drm_connector *conn;
>> -	int i;
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +	enum port port = encoder->port;
>> +	u32 val;
>>  
>> -	for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
>> -		struct intel_encoder *encoder =
>> -			to_intel_encoder(old_conn_state->best_encoder);
>> -		enum port port;
>> +	mutex_lock(&dev_priv->dpll_lock);
>>  
>> -		if (old_conn_state->crtc != crtc)
>> -			continue;
>> +	val = I915_READ(DPCLKA_CFGCR0_ICL);
>> +	val |= icl_dpclka_cfgcr0_clk_off(dev_priv, port);
>> +	I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>>  
>> -		port = encoder->port;
>> -		mutex_lock(&dev_priv->dpll_lock);
>> -		I915_WRITE(DPCLKA_CFGCR0_ICL,
>> -			   I915_READ(DPCLKA_CFGCR0_ICL) |
>> -			   icl_dpclka_cfgcr0_clk_off(dev_priv, port));
>> -		mutex_unlock(&dev_priv->dpll_lock);
>> -	}
>> +	mutex_unlock(&dev_priv->dpll_lock);
>>  }
>>  
>>  void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
>> @@ -3208,6 +3184,9 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
>>  
>>  	WARN_ON(crtc_state->has_pch_encoder);
>>  
>> +	if (INTEL_GEN(dev_priv) >= 11)
>> +		icl_map_plls_to_ports(encoder, crtc_state);
>> +
>>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>>  
>>  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) {
>> @@ -3306,6 +3285,8 @@ static void intel_ddi_post_disable(struct intel_encoder *encoder,
>>  				   const struct intel_crtc_state *old_crtc_state,
>>  				   const struct drm_connector_state *old_conn_state)
>>  {
>> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> +
>>  	/*
>>  	 * When called from DP MST code:
>>  	 * - old_conn_state will be NULL
>> @@ -3325,6 +3306,9 @@ static void intel_ddi_post_disable(struct intel_encoder *encoder,
>>  	else
>>  		intel_ddi_post_disable_dp(encoder,
>>  					  old_crtc_state, old_conn_state);
>> +
>> +	if (INTEL_GEN(dev_priv) >= 11)
>> +		icl_unmap_plls_to_ports(encoder);
>>  }
>>  
>>  void intel_ddi_fdi_post_disable(struct intel_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index d07fa4456150..559db98b5a4a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5704,9 +5704,6 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
>>  	if (pipe_config->shared_dpll)
>>  		intel_enable_shared_dpll(pipe_config);
>>  
>> -	if (INTEL_GEN(dev_priv) >= 11)
>> -		icl_map_plls_to_ports(crtc, pipe_config, old_state);
>> -
>>  	intel_encoders_pre_enable(crtc, pipe_config, old_state);
>>  
>>  	if (intel_crtc_has_dp_encoder(pipe_config))
>> @@ -5908,9 +5905,6 @@ static void haswell_crtc_disable(struct intel_crtc_state *old_crtc_state,
>>  
>>  	intel_encoders_post_disable(crtc, old_crtc_state, old_state);
>>  
>> -	if (INTEL_GEN(dev_priv) >= 11)
>> -		icl_unmap_plls_to_ports(crtc, old_crtc_state, old_state);
>> -
>>  	intel_encoders_post_pll_disable(crtc, old_crtc_state, old_state);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 40edb21087a7..9018be2171ee 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1522,12 +1522,6 @@ u8 intel_ddi_dp_pre_emphasis_max(struct intel_encoder *encoder,
>>  				 u8 voltage_swing);
>>  int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder,
>>  				     bool enable);
>> -void icl_map_plls_to_ports(struct drm_crtc *crtc,
>> -			   struct intel_crtc_state *crtc_state,
>> -			   struct drm_atomic_state *old_state);
>> -void icl_unmap_plls_to_ports(struct drm_crtc *crtc,
>> -			     struct intel_crtc_state *crtc_state,
>> -			     struct drm_atomic_state *old_state);
>>  void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder);
>>  
>>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>> -- 
>> 2.11.0

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