Re: [PATCH 04/24] drm/i915: Update power domains only on affected crtc's.

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, Jun 01, 2015 at 03:27:07PM +0200, Maarten Lankhorst wrote:
> Use for_each_crtc_state to only touch affected crtc's.
> In order to make sure that the initial power is still set
> correctly we make sure modeset_update_crtc_power_domains is called
> during the initial modeset.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  3 ---
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++-------------
>  2 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d3632c56fdf7..78ef0bb53c36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -634,9 +634,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  	intel_display_suspend(dev);
>  	drm_modeset_unlock_all(dev);
>  
> -	/* suspending displays will unsets init power */
> -	intel_display_set_init_power(dev_priv, true);
> -
>  	intel_dp_mst_suspend(dev);
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e9afc55c284..4dc07602248b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5188,42 +5188,49 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>  	return mask;
>  }
>  
> -static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> +static void modeset_update_crtc_power_domains(struct drm_atomic_state *state, bool gr)

What does 'gr' stand for and what does the parameter signify?  It seems
to just gate whether we call display.modeset_global_resources, but it's
unclear to me from the commit message above in which situations we
would/wouldn't want to do this and why.


Matt

>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> -	struct intel_crtc *crtc;
> +	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, }, domains;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i;
>  
>  	/*
>  	 * First get all needed power domains, then put all unneeded, to avoid
>  	 * any unnecessary toggling of the power wells.
>  	 */
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		enum intel_display_power_domain domain;
> +		enum pipe pipe = to_intel_crtc(crtc)->pipe;
>  
> -		if (!crtc->base.state->enable)
> +		if (!crtc->state->active)
>  			continue;
>  
> -		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
> +		domains = pipe_domains[pipe] = get_crtc_power_domains(crtc);
> +		domains &= ~to_intel_crtc(crtc)->enabled_power_domains;
>  
> -		for_each_power_domain(domain, pipe_domains[crtc->pipe])
> +		for_each_power_domain(domain, domains)
>  			intel_display_power_get(dev_priv, domain);
>  	}
>  
> -	if (dev_priv->display.modeset_global_resources)
> +	if (gr && dev_priv->display.modeset_global_resources)
>  		dev_priv->display.modeset_global_resources(state);
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +		enum pipe pipe = intel_crtc->pipe;
>  		enum intel_display_power_domain domain;
>  
> -		for_each_power_domain(domain, crtc->enabled_power_domains)
> -			intel_display_power_put(dev_priv, domain);
> +		domains = intel_crtc->enabled_power_domains;
> +		domains &= ~pipe_domains[pipe];
>  
> -		crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> -	}
> +		intel_crtc->enabled_power_domains = pipe_domains[pipe];
>  
> -	intel_display_set_init_power(dev_priv, false);
> +		for_each_power_domain(domain, domains)
> +			intel_display_power_put(dev_priv, domain);
> +	}
>  }
>  
>  void broxton_set_cdclk(struct drm_device *dev, int frequency)
> @@ -12698,7 +12705,7 @@ static int __intel_set_mode(struct drm_atomic_state *state)
>  	/* The state has been swaped above, so state actually contains the
>  	 * old state now. */
>  
> -	modeset_update_crtc_power_domains(state);
> +	modeset_update_crtc_power_domains(state, true);
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -15280,12 +15287,16 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  		ret = intel_set_mode(state);
>  		if (ret) {
>  			DRM_ERROR("Failed to restore previous mode\n");
> +			modeset_update_crtc_power_domains(state, false);
>  			drm_atomic_state_free(state);
>  		}
>  	} else {
> +		modeset_update_crtc_power_domains(state, false);
>  		drm_atomic_state_free(state);
>  	}
>  
> +	intel_display_set_init_power(dev_priv, false);
> +
>  	intel_modeset_check_state(dev);
>  }
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx