Re: [PATCH] drm/i915/icl: Sanitize DDI port clock gating for DSI ports

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

On Thu, 29 Nov 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 27 Nov 2018, Imre Deak <imre.deak@intel.com> wrote:
>> The requirement for the DDI port clock gating for a port in DSI mode is
>> the opposite wrt. the case when the port is in DDI mode: the clock
>> should be gated when the port is active and ungated when the port is
>> inactive. Note that we cannot simply keep the DDI clock gated when the
>> port will be only used in DSI mode: it must be gated/ungated at a
>> specific spot in the DSI enable/disable sequence.
>>
>> Ensure the above for all ports of a DSI encoder, also adding a sanity
>> check that we haven't registered another encoder using the same port
>> (VBT should never allow this to happen).
>>
>> Cc: Madhav Chauhan <madhav.chauhan@intel.com>
>> Cc: Vandita Kulkarni <vandita.kulkarni@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Clint Taylor <clinton.a.taylor@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

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

BR,
Jani.


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


>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 65 +++++++++++++++++++++++++++++-----------
>>  drivers/gpu/drm/i915/intel_dsi.h |  5 ++++
>>  2 files changed, 53 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index ad11540ac436..6d032a6be16c 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -28,6 +28,7 @@
>>  #include <drm/drm_scdc_helper.h>
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>> +#include "intel_dsi.h"
>>  
>>  struct ddi_buf_trans {
>>  	u32 trans1;	/* balance leg enable, de-emph level */
>> @@ -2854,8 +2855,9 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>  	u32 val;
>> -	enum port port = encoder->port;
>> -	bool clk_enabled;
>> +	enum port port;
>> +	u32 port_mask;
>> +	bool ddi_clk_needed;
>>  
>>  	/*
>>  	 * In case of DP MST, we sanitize the primary encoder only, not the
>> @@ -2864,9 +2866,6 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
>>  	if (encoder->type == INTEL_OUTPUT_DP_MST)
>>  		return;
>>  
>> -	val = I915_READ(DPCLKA_CFGCR0_ICL);
>> -	clk_enabled = !(val & icl_dpclka_cfgcr0_clk_off(dev_priv, port));
>> -
>>  	if (!encoder->base.crtc && intel_encoder_is_dp(encoder)) {
>>  		u8 pipe_mask;
>>  		bool is_mst;
>> @@ -2880,20 +2879,52 @@ void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
>>  			return;
>>  	}
>>  
>> -	if (clk_enabled == !!encoder->base.crtc)
>> -		return;
>> +	port_mask = BIT(encoder->port);
>> +	ddi_clk_needed = encoder->base.crtc;
>>  
>> -	/*
>> -	 * Punt on the case now where clock is disabled, but the encoder is
>> -	 * enabled, something else is really broken then.
>> -	 */
>> -	if (WARN_ON(!clk_enabled))
>> -		return;
>> +	if (encoder->type == INTEL_OUTPUT_DSI) {
>> +		struct intel_encoder *other_encoder;
>>  
>> -	DRM_NOTE("Port %c is disabled but it has a mapped PLL, unmap it\n",
>> -		 port_name(port));
>> -	val |= icl_dpclka_cfgcr0_clk_off(dev_priv, port);
>> -	I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>> +		port_mask = intel_dsi_encoder_ports(encoder);
>> +		/*
>> +		 * Sanity check that we haven't incorrectly registered another
>> +		 * encoder using any of the ports of this DSI encoder.
>> +		 */
>> +		for_each_intel_encoder(&dev_priv->drm, other_encoder) {
>> +			if (other_encoder == encoder)
>> +				continue;
>> +
>> +			if (WARN_ON(port_mask & BIT(other_encoder->port)))
>> +				return;
>> +		}
>> +		/*
>> +		 * DSI ports should have their DDI clock ungated when disabled
>> +		 * and gated when enabled.
>> +		 */
>> +		ddi_clk_needed = !encoder->base.crtc;
>> +	}
>> +
>> +	val = I915_READ(DPCLKA_CFGCR0_ICL);
>> +	for_each_port_masked(port, port_mask) {
>> +		bool ddi_clk_ungated = !(val &
>> +					 icl_dpclka_cfgcr0_clk_off(dev_priv,
>> +								   port));
>> +
>> +		if (ddi_clk_needed == ddi_clk_ungated)
>> +			continue;
>> +
>> +		/*
>> +		 * Punt on the case now where clock is gated, but it would
>> +		 * be needed by the port. Something else is really broken then.
>> +		 */
>> +		if (WARN_ON(ddi_clk_needed))
>> +			continue;
>> +
>> +		DRM_NOTE("Port %c is disabled/in DSI mode with an ungated DDI clock, gate it\n",
>> +			 port_name(port));
>> +		val |= icl_dpclka_cfgcr0_clk_off(dev_priv, port);
>> +		I915_WRITE(DPCLKA_CFGCR0_ICL, val);
>> +	}
>>  }
>>  
>>  static void intel_ddi_clk_select(struct intel_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
>> index ee93137f4433..d968f1f13e09 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.h
>> +++ b/drivers/gpu/drm/i915/intel_dsi.h
>> @@ -146,6 +146,11 @@ static inline bool is_cmd_mode(struct intel_dsi *intel_dsi)
>>  	return intel_dsi->operation_mode == INTEL_DSI_COMMAND_MODE;
>>  }
>>  
>> +static inline u16 intel_dsi_encoder_ports(struct intel_encoder *encoder)
>> +{
>> +	return enc_to_intel_dsi(&encoder->base)->ports;
>> +}
>> +
>>  /* intel_dsi.c */
>>  int intel_dsi_bitrate(const struct intel_dsi *intel_dsi);
>>  int intel_dsi_tlpx_ns(const struct intel_dsi *intel_dsi);

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