Re: [PATCH v2 07/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_detect()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 09, 2019 at 09:19:50AM +0300, Imre Deak wrote:
> We don't need the AUX power for the whole duration of the detect, only
> when we're doing AUX transfers. The AUX transfer function takes its own
> reference on the AUX power domain already. The two places during detect
> which access display core registers (not specific to a
> pipe/port/transcoder) only need the power domain that is required for
> that access. That power domain is equivalent to the device global power
> domain on most platforms (enabled whenever we hold a runtime PM
> reference) except on CHV/VLV where it's equivalent to the display power
> well.
> 
> Add a new power domain that reflects the above, and use this at the two
> spots accessing registers. With that we can avoid taking the AUX
> reference for the whole duration of the detect function.
> 
> Put the domains asynchronously to avoid the unneeded on-off-on toggling.
> 
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h    |  1 +
>  drivers/gpu/drm/i915/intel_dp.c         | 32 +++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  4 ++++
>  3 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 1b6f5a71184d..7f3fafdfbd5f 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -220,6 +220,7 @@ enum aux_ch {
>  #define aux_ch_name(a) ((a) + 'A')
>  
>  enum intel_display_power_domain {
> +	POWER_DOMAIN_DISPLAY_CORE,
>  	POWER_DOMAIN_PIPE_A,
>  	POWER_DOMAIN_PIPE_B,
>  	POWER_DOMAIN_PIPE_C,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1e011998e9d5..24cca12a9a3e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -216,15 +216,21 @@ static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	intel_wakeref_t wakeref;
>  	u32 lane_info;
>  
>  	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
>  		return 4;
>  
> +	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> +
>  	lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
>  		     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
>  		    DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>  
> +	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> +				      wakeref);
> +

Future idea: maybe we want a with_something() {} for this sort of thing?

>  	switch (lane_info) {
>  	default:
>  		MISSING_CASE(lane_info);

Unrelated but this thing here looks like a hand rolled hweight(). And
for some reason it's using decimal for bitmasks.

> @@ -5294,7 +5300,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
>   *
>   * Return %true if port is connected, %false otherwise.
>   */
> -bool intel_digital_port_connected(struct intel_encoder *encoder)
> +static bool __intel_digital_port_connected(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
> @@ -5324,6 +5330,20 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	return false;
>  }
>  
> +bool intel_digital_port_connected(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	intel_wakeref_t wakeref;
> +	bool res;

bool is_connected perhaps?

> +
> +	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> +	res = __intel_digital_port_connected(encoder);
> +	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> +				      wakeref);
> +
> +	return res;
> +}
> +
>  static struct edid *
>  intel_dp_get_edid(struct intel_dp *intel_dp)
>  {
> @@ -5377,16 +5397,11 @@ intel_dp_detect(struct drm_connector *connector,
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *encoder = &dig_port->base;
>  	enum drm_connector_status status;
> -	enum intel_display_power_domain aux_domain =
> -		intel_aux_power_domain(dig_port);
> -	intel_wakeref_t wakeref;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
>  
> -	wakeref = intel_display_power_get(dev_priv, aux_domain);
> -
>  	/* Can't disconnect eDP */
>  	if (intel_dp_is_edp(intel_dp))
>  		status = edp_detect(intel_dp);
> @@ -5450,10 +5465,8 @@ intel_dp_detect(struct drm_connector *connector,
>  		int ret;
>  
>  		ret = intel_dp_retrain_link(encoder, ctx);
> -		if (ret) {
> -			intel_display_power_put(dev_priv, aux_domain, wakeref);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	/*
> @@ -5475,7 +5488,6 @@ intel_dp_detect(struct drm_connector *connector,
>  	if (status != connector_status_connected && !intel_dp->is_mst)
>  		intel_dp_unset_edid(intel_dp);
>  
> -	intel_display_power_put(dev_priv, aux_domain, wakeref);
>  	return status;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 829d483d27e9..618dd42f1b2d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -406,6 +406,8 @@ const char *
>  intel_display_power_domain_str(enum intel_display_power_domain domain)
>  {
>  	switch (domain) {
> +	case POWER_DOMAIN_DISPLAY_CORE:
> +		return "DISPLAY_CORE";
>  	case POWER_DOMAIN_PIPE_A:
>  		return "PIPE_A";
>  	case POWER_DOMAIN_PIPE_B:
> @@ -2383,6 +2385,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  #define VLV_DISPLAY_POWER_DOMAINS (		\
> +	BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) |	\
>  	BIT_ULL(POWER_DOMAIN_PIPE_A) |		\
>  	BIT_ULL(POWER_DOMAIN_PIPE_B) |		\
>  	BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) |	\
> @@ -2429,6 +2432,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>  	BIT_ULL(POWER_DOMAIN_INIT))
>  
>  #define CHV_DISPLAY_POWER_DOMAINS (		\
> +	BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) |	\
>  	BIT_ULL(POWER_DOMAIN_PIPE_A) |		\
>  	BIT_ULL(POWER_DOMAIN_PIPE_B) |		\
>  	BIT_ULL(POWER_DOMAIN_PIPE_C) |		\
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx