* [PATCH] drm/i915/bdw: don't try to check IPS state on BDW @ 2014-01-07 0:28 Jesse Barnes 2014-01-07 19:49 ` Paulo Zanoni 0 siblings, 1 reply; 5+ messages in thread From: Jesse Barnes @ 2014-01-07 0:28 UTC (permalink / raw) To: intel-gfx According to Art, we don't have a way to read back the state reliably at runtime, at least not without risking disabling it again. So drop the readout and checking on BDW. References: https://bugs.freedesktop.org/show_bug.cgi?id=71906 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_display.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4d1357a..b6dfae6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -7004,8 +7004,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, if (intel_display_power_enabled(dev, pfit_domain)) ironlake_get_pfit_config(crtc, pipe_config); - pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && - (I915_READ(IPS_CTL) & IPS_ENABLE); + if (IS_HASWELL(dev)) + pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && + (I915_READ(IPS_CTL) & IPS_ENABLE); pipe_config->pixel_multiplier = 1; @@ -9335,7 +9336,9 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_I(pch_pfit.size); } - PIPE_CONF_CHECK_I(ips_enabled); + /* BDW+ don't expose a synchronous way to read the state */ + if (IS_HASWELL(dev)) + PIPE_CONF_CHECK_I(ips_enabled); PIPE_CONF_CHECK_I(double_wide); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/bdw: don't try to check IPS state on BDW 2014-01-07 0:28 [PATCH] drm/i915/bdw: don't try to check IPS state on BDW Jesse Barnes @ 2014-01-07 19:49 ` Paulo Zanoni 2014-01-07 21:30 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 Jesse Barnes 2014-01-07 21:33 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW Jesse Barnes 0 siblings, 2 replies; 5+ messages in thread From: Paulo Zanoni @ 2014-01-07 19:49 UTC (permalink / raw) To: Jesse Barnes; +Cc: Intel Graphics Development 2014/1/6 Jesse Barnes <jbarnes@virtuousgeek.org>: > According to Art, we don't have a way to read back the state reliably at > runtime, at least not without risking disabling it again. So drop the > readout and checking on BDW. Based on that, we also need to: - Replace the TODO comment at hsw_enable_ips() with something else - Move the POSTING_READ() at hsw_disable_ips() to the IS_HASWELL() case - Print "enisabled" when someone reads i915_ips_status on BDW > > References: https://bugs.freedesktop.org/show_bug.cgi?id=71906 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_display.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4d1357a..b6dfae6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7004,8 +7004,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > if (intel_display_power_enabled(dev, pfit_domain)) > ironlake_get_pfit_config(crtc, pipe_config); > > - pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && > - (I915_READ(IPS_CTL) & IPS_ENABLE); > + if (IS_HASWELL(dev)) > + pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && > + (I915_READ(IPS_CTL) & IPS_ENABLE); > > pipe_config->pixel_multiplier = 1; > > @@ -9335,7 +9336,9 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(pch_pfit.size); > } > > - PIPE_CONF_CHECK_I(ips_enabled); > + /* BDW+ don't expose a synchronous way to read the state */ > + if (IS_HASWELL(dev)) > + PIPE_CONF_CHECK_I(ips_enabled); > > PIPE_CONF_CHECK_I(double_wide); > > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 2014-01-07 19:49 ` Paulo Zanoni @ 2014-01-07 21:30 ` Jesse Barnes 2014-01-08 14:00 ` Paulo Zanoni 2014-01-07 21:33 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW Jesse Barnes 1 sibling, 1 reply; 5+ messages in thread From: Jesse Barnes @ 2014-01-07 21:30 UTC (permalink / raw) To: intel-gfx According to Art, we don't have a way to read back the state reliably at runtime, through the control reg or the mailbox, at least not without risking disabling it again. So drop the readout and checking on BDW. v2: drop TODO comment (Paulo) move POSTING_READ of control reg under HSW branch in disable (Paulo) always report IPS as enabled on BDW (Paulo) References: https://bugs.freedesktop.org/show_bug.cgi?id=71906 Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 7252f30..75a489e 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1361,7 +1361,7 @@ static int i915_ips_status(struct seq_file *m, void *unused) return 0; } - if (I915_READ(IPS_CTL) & IPS_ENABLE) + if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) seq_puts(m, "enabled\n"); else seq_puts(m, "disabled\n"); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4d1357a..74137d5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3431,9 +3431,8 @@ void hsw_enable_ips(struct intel_crtc *crtc) mutex_unlock(&dev_priv->rps.hw_lock); /* Quoting Art Runyan: "its not safe to expect any particular * value in IPS_CTL bit 31 after enabling IPS through the - * mailbox." Therefore we need to defer waiting on the state - * change. - * TODO: need to fix this for state checker + * mailbox." Moreover, the mailbox may return a bogus state, + * so we need to just enable it and continue on. */ } else { I915_WRITE(IPS_CTL, IPS_ENABLE); @@ -3460,9 +3459,10 @@ void hsw_disable_ips(struct intel_crtc *crtc) mutex_lock(&dev_priv->rps.hw_lock); WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0)); mutex_unlock(&dev_priv->rps.hw_lock); - } else + } else { I915_WRITE(IPS_CTL, 0); - POSTING_READ(IPS_CTL); + POSTING_READ(IPS_CTL); + } /* We need to wait for a vblank before we can disable the plane. */ intel_wait_for_vblank(dev, crtc->pipe); @@ -7004,8 +7004,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, if (intel_display_power_enabled(dev, pfit_domain)) ironlake_get_pfit_config(crtc, pipe_config); - pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && - (I915_READ(IPS_CTL) & IPS_ENABLE); + if (IS_HASWELL(dev)) + pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && + (I915_READ(IPS_CTL) & IPS_ENABLE); pipe_config->pixel_multiplier = 1; @@ -9335,7 +9336,9 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_I(pch_pfit.size); } - PIPE_CONF_CHECK_I(ips_enabled); + /* BDW+ don't expose a synchronous way to read the state */ + if (IS_HASWELL(dev)) + PIPE_CONF_CHECK_I(ips_enabled); PIPE_CONF_CHECK_I(double_wide); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 2014-01-07 21:30 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 Jesse Barnes @ 2014-01-08 14:00 ` Paulo Zanoni 0 siblings, 0 replies; 5+ messages in thread From: Paulo Zanoni @ 2014-01-08 14:00 UTC (permalink / raw) To: Jesse Barnes; +Cc: Intel Graphics Development 2014/1/7 Jesse Barnes <jbarnes@virtuousgeek.org>: > According to Art, we don't have a way to read back the state reliably at > runtime, through the control reg or the mailbox, at least not without risking > disabling it again. So drop the readout and checking on BDW. > > v2: drop TODO comment (Paulo) > move POSTING_READ of control reg under HSW branch in disable (Paulo) > always report IPS as enabled on BDW (Paulo) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=71906 > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++-------- > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 7252f30..75a489e 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1361,7 +1361,7 @@ static int i915_ips_status(struct seq_file *m, void *unused) > return 0; > } > > - if (I915_READ(IPS_CTL) & IPS_ENABLE) > + if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) <bikeshedding> I'm not really sure if we need to print "enabled" here... It's more like "enabled and disabled". </bikeshedding> Anyway, your patch seems to fix the bug, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > seq_puts(m, "enabled\n"); > else > seq_puts(m, "disabled\n"); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4d1357a..74137d5 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3431,9 +3431,8 @@ void hsw_enable_ips(struct intel_crtc *crtc) > mutex_unlock(&dev_priv->rps.hw_lock); > /* Quoting Art Runyan: "its not safe to expect any particular > * value in IPS_CTL bit 31 after enabling IPS through the > - * mailbox." Therefore we need to defer waiting on the state > - * change. > - * TODO: need to fix this for state checker > + * mailbox." Moreover, the mailbox may return a bogus state, > + * so we need to just enable it and continue on. > */ > } else { > I915_WRITE(IPS_CTL, IPS_ENABLE); > @@ -3460,9 +3459,10 @@ void hsw_disable_ips(struct intel_crtc *crtc) > mutex_lock(&dev_priv->rps.hw_lock); > WARN_ON(sandybridge_pcode_write(dev_priv, DISPLAY_IPS_CONTROL, 0)); > mutex_unlock(&dev_priv->rps.hw_lock); > - } else > + } else { > I915_WRITE(IPS_CTL, 0); > - POSTING_READ(IPS_CTL); > + POSTING_READ(IPS_CTL); > + } > > /* We need to wait for a vblank before we can disable the plane. */ > intel_wait_for_vblank(dev, crtc->pipe); > @@ -7004,8 +7004,9 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, > if (intel_display_power_enabled(dev, pfit_domain)) > ironlake_get_pfit_config(crtc, pipe_config); > > - pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && > - (I915_READ(IPS_CTL) & IPS_ENABLE); > + if (IS_HASWELL(dev)) > + pipe_config->ips_enabled = hsw_crtc_supports_ips(crtc) && > + (I915_READ(IPS_CTL) & IPS_ENABLE); > > pipe_config->pixel_multiplier = 1; > > @@ -9335,7 +9336,9 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_I(pch_pfit.size); > } > > - PIPE_CONF_CHECK_I(ips_enabled); > + /* BDW+ don't expose a synchronous way to read the state */ > + if (IS_HASWELL(dev)) > + PIPE_CONF_CHECK_I(ips_enabled); > > PIPE_CONF_CHECK_I(double_wide); > > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915/bdw: don't try to check IPS state on BDW 2014-01-07 19:49 ` Paulo Zanoni 2014-01-07 21:30 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 Jesse Barnes @ 2014-01-07 21:33 ` Jesse Barnes 1 sibling, 0 replies; 5+ messages in thread From: Jesse Barnes @ 2014-01-07 21:33 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development On Tue, 7 Jan 2014 17:49:05 -0200 Paulo Zanoni <przanoni@gmail.com> wrote: > 2014/1/6 Jesse Barnes <jbarnes@virtuousgeek.org>: > > According to Art, we don't have a way to read back the state reliably at > > runtime, at least not without risking disabling it again. So drop the > > readout and checking on BDW. > > Based on that, we also need to: > - Replace the TODO comment at hsw_enable_ips() with something else > - Move the POSTING_READ() at hsw_disable_ips() to the IS_HASWELL() case > - Print "enisabled" when someone reads i915_ips_status on BDW All good changes. Just sent an updated patch. Thanks, -- Jesse Barnes, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-08 14:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-07 0:28 [PATCH] drm/i915/bdw: don't try to check IPS state on BDW Jesse Barnes 2014-01-07 19:49 ` Paulo Zanoni 2014-01-07 21:30 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW v2 Jesse Barnes 2014-01-08 14:00 ` Paulo Zanoni 2014-01-07 21:33 ` [PATCH] drm/i915/bdw: don't try to check IPS state on BDW Jesse Barnes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox