[Intel-gfx] [PATCH 3/7] drm/i915: add intel_display_power_enabled

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> 
> This should replace intel_using_power_well. The idea is that we're
> adding the requested power domain as an argument, so this might enable
> the code to look less platform-specific and also allows us to easily
> add new domains in case we need.
> 
> Requested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
>  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b86023d..b492eaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>  		state = true;
>  
> -	if (!intel_using_power_well(dev_priv->dev) &&
> -	    cpu_transcoder != TRANSCODER_EDP) {
> +	if (cpu_transcoder != TRANSCODER_EDP &&
> +	    !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {

I'm wondering if we could simplify the look of this and try to avoid
using POWER_DOMAIN_OTHER, for instance having a:

if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
	return false;

playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
the cpu_transcoder in them.

>  		cur_state = false;
>  	} else {
>  		reg = PIPECONF(cpu_transcoder);
> @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> +	enum intel_display_power_domain domain;
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	/* XXX: Once we have proper panel fitter state tracking implemented with
>  	 * hardware state read/check support we should switch to only disable
>  	 * the panel fitter when we know it's used. */
> -	if (intel_using_power_well(dev)) {
> +	if (pipe == PIPE_A)
> +		domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
> +	else
> +		domain = POWER_DOMAIN_OTHER;
> +

Of course, if the above is found useful, same thing for here for the
panel fitter function.

> +	if (intel_display_power_enabled(dev, domain)) {
>  		I915_WRITE(PF_CTL(pipe), 0);
>  		I915_WRITE(PF_WIN_SZ(pipe), 0);
>  	}
> @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>  	uint32_t tmp;
>  

[...]

> +enum intel_display_power_domain {
> +	POWER_DOMAIN_PIPE_A,
> +	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
> +	POWER_DOMAIN_TRANSCODER_EDP,
> +	POWER_DOMAIN_OTHER,
> +};
> +
>  int intel_pch_rawclk(struct drm_device *dev);
>  
>  int intel_connector_update_modes(struct drm_connector *connector,
> @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern bool intel_using_power_well(struct drm_device *dev);
> +extern bool intel_display_power_enabled(struct drm_device *dev,
> +					enum intel_display_power_domain domain);
>  extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2557926..1c472d7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
>   * enable it, so check if it's enabled and also check if we've requested it to
>   * be enabled.
>   */
> -bool intel_using_power_well(struct drm_device *dev)
> +bool intel_display_power_enabled(struct drm_device *dev,
> +				 enum intel_display_power_domain domain)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool power_well_enabled;
>  
> -	if (IS_HASWELL(dev))
> -		return I915_READ(HSW_PWR_WELL_DRIVER) ==
> -		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> -	else
> +	if (!HAS_POWER_WELL(dev))
> +		return true;
> +
> +	power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
> +			     (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> +
> +	switch (domain) {
> +	case POWER_DOMAIN_PIPE_A:
> +	case POWER_DOMAIN_TRANSCODER_EDP:
>  		return true;
> +	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> +	case POWER_DOMAIN_OTHER:
> +		return power_well_enabled;
> +	default:
> +		BUG();
> +	}
>  }
  
-- 
Damien