Re: [Intel-gfx] [PATCH 03/42] drm/i915: Only update required power domains.
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
> >> ---
> >> 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
Re: [Intel-gfx] [PATCH 03/42] drm/i915: Only update required power domains.
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
>> ---
>> 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/42] drm/i915: Only update required power domains.
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
> ---
> 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
--
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
