On Fri, Apr 19, 2013 at 06:44:47AM +0100, Damien Lespiau wrote:
> On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >
> > This should replace intel_using_power_well. The idea is that we're
> > adding the requested power domain as an argument, so this might enable
> > the code to look less platform-specific and also allows us to easily
> > add new domains in case we need.
> >
> > Requested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++-----
> > drivers/gpu/drm/i915/intel_drv.h | 10 +++++++++-
> > drivers/gpu/drm/i915/intel_pm.c | 23 ++++++++++++++++++-----
> > 3 files changed, 38 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b86023d..b492eaa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> > if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> > state = true;
> >
> > - if (!intel_using_power_well(dev_priv->dev) &&
> > - cpu_transcoder != TRANSCODER_EDP) {
> > + if (cpu_transcoder != TRANSCODER_EDP &&
> > + !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
>
> I'm wondering if we could simplify the look of this and try to avoid
> using POWER_DOMAIN_OTHER, for instance having a:
>
> if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> return false;
>
> playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
> the cpu_transcoder in them.
Well I have my own bikeshed - imo POWER_DOMAIN_TRANSCODER_EDP should be
called just POWER_DOMAIN_EDP since it also includes the eDP port. So the
transcoder part in the name is a bit misleading. And I suspect that we
still miss some DDI A vs everything else checks in the hw state readout
code ...
My fear with using more fine-grained domains than what we currently (and
for the next few generations of hw, hopefully) need is that we spill check
complexity into code where it's simply not relevant. So I think that the
code clarity we gain by pushing the "is this thing in one of the special
domains" into intel_display_power_enabled we'll waste again with
inconsistency in checking - it's not relevant for most things after all.
-Daniel
>
> > cur_state = false;
> > } else {
> > reg = PIPECONF(cpu_transcoder);
> > @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> > int pipe = intel_crtc->pipe;
> > int plane = intel_crtc->plane;
> > enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> > + enum intel_display_power_domain domain;
> >
> > if (!intel_crtc->active)
> > return;
> > @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> > /* XXX: Once we have proper panel fitter state tracking implemented with
> > * hardware state read/check support we should switch to only disable
> > * the panel fitter when we know it's used. */
> > - if (intel_using_power_well(dev)) {
> > + if (pipe == PIPE_A)
> > + domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
> > + else
> > + domain = POWER_DOMAIN_OTHER;
> > +
>
> Of course, if the above is found useful, same thing for here for the
> panel fitter function.
>
> > + if (intel_display_power_enabled(dev, domain)) {
> > I915_WRITE(PF_CTL(pipe), 0);
> > I915_WRITE(PF_WIN_SZ(pipe), 0);
> > }
> > @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> > enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
> > uint32_t tmp;
> >
>
> [...]
>
> > +enum intel_display_power_domain {
> > + POWER_DOMAIN_PIPE_A,
> > + POWER_DOMAIN_PIPE_A_PANEL_FITTER,
> > + POWER_DOMAIN_TRANSCODER_EDP,
> > + POWER_DOMAIN_OTHER,
> > +};
> > +
> > int intel_pch_rawclk(struct drm_device *dev);
> >
> > int intel_connector_update_modes(struct drm_connector *connector,
> > @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
> > extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
> > extern void intel_gpu_ips_teardown(void);
> >
> > -extern bool intel_using_power_well(struct drm_device *dev);
> > +extern bool intel_display_power_enabled(struct drm_device *dev,
> > + enum intel_display_power_domain domain);
> > extern void intel_init_power_well(struct drm_device *dev);
> > extern void intel_set_power_well(struct drm_device *dev, bool enable);
> > extern void intel_enable_gt_powersave(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 2557926..1c472d7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
> > * enable it, so check if it's enabled and also check if we've requested it to
> > * be enabled.
> > */
> > -bool intel_using_power_well(struct drm_device *dev)
> > +bool intel_display_power_enabled(struct drm_device *dev,
> > + enum intel_display_power_domain domain)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + bool power_well_enabled;
> >
> > - if (IS_HASWELL(dev))
> > - return I915_READ(HSW_PWR_WELL_DRIVER) ==
> > - (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> > - else
> > + if (!HAS_POWER_WELL(dev))
> > + return true;
> > +
> > + power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
> > + (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> > +
> > + switch (domain) {
> > + case POWER_DOMAIN_PIPE_A:
> > + case POWER_DOMAIN_TRANSCODER_EDP:
> > return true;
> > + case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > + case POWER_DOMAIN_OTHER:
> > + return power_well_enabled;
> > + default:
> > + BUG();
> > + }
> > }
>
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
