| Message ID | 1366313746-3659-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@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@ffwll.ch> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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. > 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(); > + } > }
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@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@ffwll.ch> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi 2013/4/19 Daniel Vetter <daniel@ffwll.ch>: > 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@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@ffwll.ch> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 ... I think POWER_DOMAIN_EDP can be misleading. We can have "PIPE_B + TRANSCODER_EDP" configured (who can guarantee what the xrandr apps really do?), and we can also have the "eDP on port D" which will never use TRANSCODER_EDP, it will use PIPE_X + CPU_TRANSCODER_X. > > 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@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 -- Paulo Zanoni
On Fri, Apr 19, 2013 at 12:28:30PM -0300, Paulo Zanoni wrote: > Hi > > 2013/4/19 Daniel Vetter <daniel@ffwll.ch>: > > 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@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@ffwll.ch> > >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 ... > > I think POWER_DOMAIN_EDP can be misleading. We can have "PIPE_B + > TRANSCODER_EDP" configured (who can guarantee what the xrandr apps > really do?), and we can also have the "eDP on port D" which will never > use TRANSCODER_EDP, it will use PIPE_X + CPU_TRANSCODER_X. Yeah, I guess that makes sense. -Daniel
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)) { 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; + + 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; - if (!intel_using_power_well(dev_priv->dev) && - cpu_transcoder != TRANSCODER_EDP) + if (cpu_transcoder != TRANSCODER_EDP && + !intel_display_power_enabled(dev, POWER_DOMAIN_OTHER)) return false; tmp = I915_READ(PIPECONF(cpu_transcoder)); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b5b6d19..04b3f84 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -465,6 +465,13 @@ struct intel_fbc_work { int interval; }; +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(); + } } void intel_set_power_well(struct drm_device *dev, bool enable)