Re: [PATCH RFC 2/5] drm/i915: Only update required power domains.

Ander Conselvan De Oliveira
2015-04-24T08:47:35Z

On Wed, 2015-04-22 at 13:24 +0200, maarten.lankhorst@linux.intel.com
wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> This prevents unnecessarily updating power domains, while still
> enabling all power domains on initial setup.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++--------
> 1 file changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 92d54dd30d7e..438d8e213748 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5163,36 +5163,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> return mask;
> }
>
> +static bool
> +needs_modeset(struct drm_crtc_state *state)
> +{
> + return state->mode_changed || state->active_changed;
> +}
> +
> static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> {
> 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;
> + bool init_power = dev_priv->power_domains.init_power_on;
> + bool any_power = init_power, any_modeset = false;
> + unsigned long domains;
>
> /*
> * 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) {
> + int idx = drm_crtc_index(&crtc->base);
> + struct drm_crtc_state *crtc_state = state->crtc_states[idx];
> enum intel_display_power_domain domain;
>
> - if (!crtc->base.state->enable)
> + if (!init_power && !crtc_state)
> + continue;
> +
> + if (needs_modeset(crtc->base.state))
> + any_modeset = true;
> +
> + if (crtc->base.state->enable)
> + pipe_domains[crtc->pipe] =
> + get_crtc_power_domains(&crtc->base);
> +
> + if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains)
> continue;
>
> - pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
> + WARN_ON(!init_power && !needs_modeset(crtc->base.state));
> +
> + any_power = true;
> + domains = pipe_domains[crtc->pipe] &
> + ~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);

Isn't intel_display_power_get() already a no-op if the power domain is
already active, except for the reference counting? Or did I miss
something?

Ander

> }
>
> - if (dev_priv->display.modeset_global_resources)
> + if (any_modeset && dev_priv->display.modeset_global_resources)
> dev_priv->display.modeset_global_resources(state);
>
> + if (!any_power)
> + return;
> +
> for_each_intel_crtc(dev, crtc) {
> + int idx = drm_crtc_index(&crtc->base);
> + struct drm_crtc_state *crtc_state = state->crtc_states[idx];
> enum intel_display_power_domain domain;
>
> - for_each_power_domain(domain, crtc->enabled_power_domains)
> + if (!init_power && !crtc_state)
> + continue;
> +
> + domains = crtc->enabled_power_domains &
> + ~pipe_domains[crtc->pipe];
> +
> + for_each_power_domain(domain, domains)
> intel_display_power_put(dev_priv, domain);
>
> crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> @@ -11144,12 +11180,6 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
> return false;
> }
>
> -static bool
> -needs_modeset(struct drm_crtc_state *state)
> -{
> - return state->mode_changed || state->active_changed;
> -}
> -
> static void
> intel_modeset_update_state(struct drm_atomic_state *state)
> {