Re: [PATCH 5/6] drm/i915: enable only the needed power domains during modeset

[Imre Deak <imre.deak@intel.com>]

On Tue, 2013-10-22 at 18:07 -0200, Paulo Zanoni wrote:
> 2013/10/18 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Wed, 16 Oct 2013 17:25:52 +0300
> > Imre Deak <imre.deak@intel.com> wrote:
> >
> >> So far the modeset code enabled all power domains if it needed any. It
> >> wasn't a problem since HW generations so far only had one always-on
> >> power well and one dynamic power well that can be enabled/disabled. For
> >> domains powered by always-on power wells (panel fitter on pipe A and the
> >> eDP transcoder) we didn't do anything, for all other domains we just
> >> enabled the single dynamic power well.
> >>
> >> Future HW generations will change this, as they add multiple dynamic
> >> power wells. Support for these will be added later, this patch prepares
> >> for those by making sure we only enable the required domains.
> >>
> >> Note that after this change on HSW we'll enable all power domains even
> >> if it was the domain for the panel fitter on pipe A or the eDP
> >> transcoder. This isn't a problem since the power domain framework
> >> already checks if the domain is on an always-on power well and doesn't
> >> do anything in this case.
> >>
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> I updated drm-intel-nightly and now when I boot Haswell with eDP-only,
> the power well is enabled, where it should be disabled. Reverting this
> patch fixes the problem for me.

As discussed on IRC, this broke b/c of a missing call to
intel_display_set_init_power() in modeset_update_power_wells(). I added
it only in patch 6/6, but that isn't applied yet since I had to rework
it. I've posted a v2 for this patch already, it should fix the issue.

Thanks for catching this.

--Imre

> 
> 
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++----
> >>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
> >>  2 files changed, 42 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 8e734f2..e2a4f3b 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -6561,21 +6561,57 @@ static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
> >>       }
> >>  }
> >>
> >> +#define for_each_power_domain(domain, mask)                          \
> >> +     for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)     \
> >> +             if ((1 << (domain)) & (mask))
> >> +
> >> +static unsigned long get_pipe_power_domains(struct drm_device *dev,
> >> +                                         enum pipe pipe, bool pfit_enabled)
> >> +{
> >> +     unsigned long mask;
> >> +     enum transcoder transcoder;
> >> +
> >> +     transcoder = intel_pipe_to_cpu_transcoder(dev->dev_private, pipe);
> >> +
> >> +     mask = BIT(POWER_DOMAIN_PIPE(pipe));
> >> +     mask |= BIT(POWER_DOMAIN_TRANSCODER(transcoder));
> >> +     if (pfit_enabled)
> >> +             mask |= BIT(POWER_DOMAIN_PIPE_PANEL_FITTER(pipe));
> >> +
> >> +     return mask;
> >> +}
> >> +
> >>  static void modeset_update_power_wells(struct drm_device *dev)
> >>  {
> >> -     bool enable = false;
> >> +     unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
> >>       struct intel_crtc *crtc;
> >>
> >> +     /*
> >> +      * First get all needed power domains, then put all unneeded, to avoid
> >> +      * any unnecessary toggling of the power wells.
> >> +      */
> >>       list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> >> +             enum intel_display_power_domain domain;
> >> +
> >>               if (!crtc->base.enabled)
> >>                       continue;
> >>
> >> -             if (crtc->pipe != PIPE_A || crtc->config.pch_pfit.enabled ||
> >> -                 crtc->config.cpu_transcoder != TRANSCODER_EDP)
> >> -                     enable = true;
> >> +             pipe_domains[crtc->pipe] = get_pipe_power_domains(dev,
> >> +                                             crtc->pipe,
> >> +                                             crtc->config.pch_pfit.enabled);
> >> +
> >> +             for_each_power_domain(domain, pipe_domains[crtc->pipe])
> >> +                     intel_display_power_get(dev, domain);
> >>       }
> >>
> >> -     intel_set_power_well(dev, enable);
> >> +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
> >> +             enum intel_display_power_domain domain;
> >> +
> >> +             for_each_power_domain(domain, crtc->enabled_power_domains)
> >> +                     intel_display_power_put(dev, domain);
> >> +
> >> +             crtc->enabled_power_domains = pipe_domains[crtc->pipe];
> >> +     }
> >>  }
> >>
> >>  static void haswell_modeset_global_resources(struct drm_device *dev)
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 189257d..63a5bfd 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -320,6 +320,7 @@ struct intel_crtc {
> >>        * some outputs connected to this crtc.
> >>        */
> >>       bool active;
> >> +     unsigned long enabled_power_domains;
> >>       bool eld_vld;
> >>       bool primary_enabled; /* is the primary plane (partially) visible? */
> >>       bool lowfreq_avail;
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > Hope we can get rid of the set_power_well() function soon...
> >
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
>