Re: [PATCH v4 2/8] drm/i915: Unify power domain handling.

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu:
> Right now there's separate power domain handling for update_pipe and
> modesets. Unify this and only grab POWER_DOMAIN_MODESET once.

I would have split this in two separate commits: one for
POWER_DOMAIN_MODESET, and the other for killing
intel_modeset_update_crtc_power_domains. Let's hope no one bisects
anything to this patch :)

With or without the split:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 69 +++++++++++++-------------
> ----------
>  1 file changed, 24 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index b479ba6238d7..804f2c6f260d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5359,32 +5359,6 @@ static void modeset_put_power_domains(struct
> drm_i915_private *dev_priv,
>  		intel_display_power_put(dev_priv, domain);
>  }
>  
> -static void modeset_update_crtc_power_domains(struct
> drm_atomic_state *state)
> -{
> -	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> -	struct drm_device *dev = state->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned long put_domains[I915_MAX_PIPES] = {};
> -	struct drm_crtc_state *crtc_state;
> -	struct drm_crtc *crtc;
> -	int i;
> -
> -	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> -		if (needs_modeset(crtc->state))
> -			put_domains[to_intel_crtc(crtc)->pipe] =
> -				modeset_get_crtc_power_domains(crtc,
> -					to_intel_crtc_state(crtc-
> >state));
> -	}
> -
> -	if (dev_priv->display.modeset_commit_cdclk &&
> -	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> -		dev_priv->display.modeset_commit_cdclk(state);
> -
> -	for (i = 0; i < I915_MAX_PIPES; i++)
> -		if (put_domains[i])
> -			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> -}
> -
>  static int intel_compute_max_dotclk(struct drm_i915_private
> *dev_priv)
>  {
>  	int max_cdclk_freq = dev_priv->max_cdclk_freq;
> @@ -13422,6 +13396,7 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  	struct drm_crtc *crtc;
>  	int ret = 0, i;
>  	bool hw_check = intel_state->modeset;
> +	unsigned long put_domains[I915_MAX_PIPES] = {};
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13437,11 +13412,22 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		       sizeof(intel_state->min_pixclk));
>  		dev_priv->active_crtcs = intel_state->active_crtcs;
>  		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> +
> +		intel_display_power_get(dev_priv,
> POWER_DOMAIN_MODESET);
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
> +		if (needs_modeset(crtc->state) ||
> +		    to_intel_crtc_state(crtc->state)->update_pipe) {
> +			hw_check = true;
> +
> +			put_domains[to_intel_crtc(crtc)->pipe] =
> +				modeset_get_crtc_power_domains(crtc,
> +					to_intel_crtc_state(crtc-
> >state));
> +		}
> +
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> @@ -13474,7 +13460,10 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		intel_shared_dpll_commit(state);
>  
>  		drm_atomic_helper_update_legacy_modeset_state(state-
> >dev, state);
> -		modeset_update_crtc_power_domains(state);
> +
> +		if (dev_priv->display.modeset_commit_cdclk &&
> +		    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> +			dev_priv-
> >display.modeset_commit_cdclk(state);
>  	}
>  
>  	/* Now enable the clocks, plane, pipe, and connectors that
> we set up. */
> @@ -13483,24 +13472,12 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		bool modeset = needs_modeset(crtc->state);
>  		bool update_pipe = !modeset &&
>  			to_intel_crtc_state(crtc->state)-
> >update_pipe;
> -		unsigned long put_domains = 0;
> -
> -		if (modeset)
> -			intel_display_power_get(dev_priv,
> POWER_DOMAIN_MODESET);
>  
>  		if (modeset && crtc->state->active) {
>  			update_scanline_offset(to_intel_crtc(crtc));
>  			dev_priv->display.crtc_enable(crtc);
>  		}
>  
> -		if (update_pipe) {
> -			put_domains =
> modeset_get_crtc_power_domains(crtc,
> -					      to_intel_crtc_state(cr
> tc->state));
> -
> -			/* make sure intel_modeset_check_state runs
> */
> -			hw_check = true;
> -		}
> -
>  		if (!modeset)
>  			intel_pre_plane_update(to_intel_crtc_state(c
> rtc_state));
>  
> @@ -13511,19 +13488,21 @@ static int intel_atomic_commit(struct
> drm_device *dev,
>  		    (crtc->state->planes_changed || update_pipe))
>  			drm_atomic_helper_commit_planes_on_crtc(crtc
> _state);
>  
> -		if (put_domains)
> -			modeset_put_power_domains(dev_priv,
> put_domains);
> -
>  		intel_post_plane_update(intel_crtc);
> -
> -		if (modeset)
> -			intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (put_domains[i])
> +			modeset_put_power_domains(dev_priv,
> put_domains[i]);
> +	}
> +
> +	if (intel_state->modeset)
> +		intel_display_power_put(dev_priv,
> POWER_DOMAIN_MODESET);
> +
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx