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 <[email protected]> > >> --- > >> 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 [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
