| Message ID | 1381933553-19529-6-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> > --- > 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...
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. >> --- >> 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
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 > > >
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;
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> --- drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 42 insertions(+), 5 deletions(-)