Re: [PATCH 03/42] drm/i915: Only update required power domains.

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ander Conselvan de Oliveira
	<ander.conselvan.de.oliveira@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/42] drm/i915: Only update required power domains.
Date: Tue, 12 May 2015 15:13:03 +0200	[thread overview]
Message-ID: <20150512131303.GY15256@phenom.ffwll.local> (raw)
In-Reply-To: <5551ECA0.3070208@linux.intel.com>

On Tue, May 12, 2015 at 02:05:52PM +0200, Maarten Lankhorst wrote:
> Op 11-05-15 om 19:00 schreef Daniel Vetter:
> > On Mon, May 11, 2015 at 04:24:39PM +0200, Maarten Lankhorst wrote:
> >> 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 af96d686aae2..42d0cc329b37 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5298,36 +5298,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> >>  	return mask;
> >>  }
> >>  
> >> +static bool
> >> +needs_modeset(struct drm_crtc_state *state)
> > I think we should extract this from drm_atomic_helper.c. But that can be
> > done as a follow-up - if you track it ;-)
> >> +{
> >> +	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)
> > 			continue;
> >
> > while you're optimizing this?
> >
> >> +
> >> +		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);
> >>  	}
> >>  
> >> -	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;
> > Same shortcut here?
> It's not an optimization, the same path can be used for modeset and updating planes.
> If it's a plane update the code might run with irqs disabled, in which
> case we can't call power_get or power_put because that requires a mutex.

Plane updates should never ever change the power domains needed, hence for
a plane update we should short-circuit out the entire thing. And if the
pipe is off (state->active == false) then we need to delay the plane
updates until we enable the pipe again. Thus far the code has done that
with a mix of plane_state->visible checking (gated on intel_crtc->active)
and checking intel_crtc->active directly. In the future we just need to
gate plane updates on crtc_state->active. When enabling the pipe we need
to go through all planes anyway, so this should fall out naturally.

Or do I miss something?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx