Re: [PATCH 02/12] drm/i915: ensure the HW is powered during display pipe HW readout

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Imre Deak <imre.deak@intel.com> writes:

> The assumption when adding the intel_display_power_is_enabled() checks
> was that if it returns success the power can't be turned off afterwards
> during the HW access, which is guaranteed by modeset locks. This isn't
> always true, so make sure we hold a dedicated reference for the time of
> the access.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>


Was concerned on domain mask overflows but there was already
BUILD_BUG_ON for it.

Revieved-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 67 ++++++++++++++++++++++++++----------
>  1 file changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 836bbdc..6abfc54 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8181,18 +8181,22 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					    POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
> +	ret = false;
> +
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	if (IS_G4X(dev) || IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
>  		switch (tmp & PIPECONF_BPC_MASK) {
> @@ -8272,7 +8276,12 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  	pipe_config->base.adjusted_mode.crtc_clock =
>  		pipe_config->port_clock / pipe_config->pixel_multiplier;
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void ironlake_init_pch_refclk(struct drm_device *dev)
> @@ -9376,18 +9385,21 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					    POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
> +	ret = false;
>  	tmp = I915_READ(PIPECONF(crtc->pipe));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	switch (tmp & PIPECONF_BPC_MASK) {
>  	case PIPECONF_6BPC:
> @@ -9450,7 +9462,12 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  	ironlake_get_pfit_config(crtc, pipe_config);
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv)
> @@ -9982,12 +9999,17 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	enum intel_display_power_domain pfit_domain;
> +	enum intel_display_power_domain power_domain;
> +	unsigned long power_domain_mask;
>  	uint32_t tmp;
> +	bool ret;
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -					 POWER_DOMAIN_PIPE(crtc->pipe)))
> +	power_domain = POWER_DOMAIN_PIPE(crtc->pipe);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
>  		return false;
> +	power_domain_mask = BIT(power_domain);
> +
> +	ret = false;
>  
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
> @@ -10014,13 +10036,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  			pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  	}
>  
> -	if (!intel_display_power_is_enabled(dev_priv,
> -			POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder)))
> -		return false;
> +	power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder);
> +	if (!intel_display_power_get_if_enabled(dev_priv, power_domain))
> +		goto out;
> +	power_domain_mask |= BIT(power_domain);
>  
>  	tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder));
>  	if (!(tmp & PIPECONF_ENABLE))
> -		return false;
> +		goto out;
>  
>  	haswell_get_ddi_port_state(crtc, pipe_config);
>  
> @@ -10030,14 +10053,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		skl_init_scalers(dev, crtc, pipe_config);
>  	}
>  
> -	pfit_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> -
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		pipe_config->scaler_state.scaler_id = -1;
>  		pipe_config->scaler_state.scaler_users &= ~(1 << SKL_CRTC_INDEX);
>  	}
>  
> -	if (intel_display_power_is_enabled(dev_priv, pfit_domain)) {
> +	power_domain = POWER_DOMAIN_PIPE_PANEL_FITTER(crtc->pipe);
> +	if (intel_display_power_get_if_enabled(dev_priv, power_domain)) {
> +		power_domain_mask |= BIT(power_domain);
>  		if (INTEL_INFO(dev)->gen >= 9)
>  			skylake_get_pfit_config(crtc, pipe_config);
>  		else
> @@ -10055,7 +10078,13 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  		pipe_config->pixel_multiplier = 1;
>  	}
>  
> -	return true;
> +	ret = true;
> +
> +out:
> +	for_each_power_domain(power_domain, power_domain_mask)
> +		intel_display_power_put(dev_priv, power_domain);
> +
> +	return ret;
>  }
>  
>  static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx