| Message ID | 1431354318-11995-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -Daniel > + > + 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]; > @@ -11539,12 +11575,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) > { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. ~Maarten
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
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) +{ + 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); } - 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]; @@ -11539,12 +11575,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) {
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(-)