Re: [PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 16 Oct 2013 17:25:52 +0300
Imre Deak <imre.deak@intel.com> wrote:

> So far the modeset code enabled all power domains if it needed any. It
> wasn't a problem since HW generations so far only had one always-on
> power well and one dynamic power well that can be enabled/disabled. For
> domains powered by always-on power wells (panel fitter on pipe A and the
> eDP transcoder) we didn't do anything, for all other domains we just
> enabled the single dynamic power well.
> 
> Future HW generations will change this, as they add multiple dynamic
> power wells. Support for these will be added later, this patch prepares
> for those by making sure we only enable the required domains.
> 
> Note that after this change on HSW we'll enable all power domains even
> if it was the domain for the panel fitter on pipe A or the eDP
> transcoder. This isn't a problem since the power domain framework
> already checks if the domain is on an always-on power well and doesn't
> do anything in this case.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e734f2..e2a4f3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> +#define for_each_power_domain(domain, mask)				\
> +	for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)	\
> +		if ((1 << (domain)) & (mask))
> +
> +static unsigned long get_pipe_power_domains(struct drm_device *dev,
> +					    enum pipe pipe, bool pfit_enabled)
> +{
> +	unsigned long mask;
> +	enum transcoder transcoder;
> +
> +	transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
> +
> +	mask = BIT(POWER_DOMAIN_PIPE(pipe));
> +	mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> +	if (pfit_enabled)
> +		mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
> +
> +	return mask;
> +}
> +
>  static void modeset_update_power_wells(struct drm_device *dev)
>  {
> -	bool enable = false;
> +	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
>  	struct intel_crtc *crtc;
>  
> +	/*
> +	 * First get all needed power domains, then put all unneeded, to avoid
> +	 * any unnecessary toggling of the power wells.
> +	 */
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +		enum intel_display_power_domain domain;
> +
>  		if (!crtc->base.enabled)
>  			continue;
>  
> -		if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
> -		    crtc->config.cpu_transcoder != TRANSCODER_EDP)
> -			enable = true;
> +		pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
> +						crtc->pipe,
> +						crtc->config.pch_pfit.enabled);
> +
> +		for_each_power_domain(domain, pipe_domains[crtc->pipe])
> +			intel_display_power_get(dev, domain);
>  	}
>  
> -	intel_set_power_well(dev, enable);
> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> +		enum intel_display_power_domain domain;
> +
> +		for_each_power_domain(domain, crtc->enabled_power_domains)
> +			intel_display_power_put(dev, domain);
> +
> +		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> +	}
>  }
>  
>  static void haswell_modeset_global_resources(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 189257d..63a5bfd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -320,6 +320,7 @@ struct intel_crtc {
>  	 * some outputs connected to this crtc.
>  	 */
>  	bool active;
> +	unsigned long enabled_power_domains;
>  	bool eld_vld;
>  	bool primary_enabled; /* is the primary plane (partially) visible? */
>  	bool lowfreq_avail;

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Hope we can get rid of the set_power_well() function soon...

-- 
Jesse Barnes, Intel Open Source Technology Center