Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Tue, 21 Oct 2014 16:50:12 +0300 Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Thu, Sep 11, 2014 at 07:15:27AM -0700, Jesse Barnes wrote: On Thu, 11 Sep 2014 14:59:35 +0300 Imre Deak imre.d...@intel.com wrote: On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote: Since correctness wins over optimal code and since the optimization Optimal code is also correct ;-) s/optimal/just plain broken/ Yes, bad wording. To clarify, since the optimization is now always off anyway (and it's also broken), I would hope that we could remove it for now and concentrate on fixing the existing s/r issues. Once we find that things are stable enough we could add back this optimization. Arg, I guess we didn't test after moving to the opregion test? Or maybe it was working when it landed for S3 and not for S4? Or broke sometime after it landed? Anyway this is a really valuable optimization for resume time on some platforms, and really we shouldn't have other agents clobbering our GTT on resume, so I'd really like to fix/re-add it asap. If/when we add this back I would suggest that we also add a sanity check that can be enabled with drm.debug which would verify the GTT mapping are what we expect. That way at least most developer would have it enabled and we'd catch problems earlier, and also bug reporters would be forced to enable it when we ask for dmesg w/ debugs. Yeah we had that awhile back iirc, but I think it got bikeshedded to death. I'd be happy if someone resurrected it; I'd give it an r-b without suggesting more sophisticated checksum algorithms. :) http://lists.freedesktop.org/archives/intel-gfx/2012-September/020305.html Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Thu, Sep 11, 2014 at 07:15:27AM -0700, Jesse Barnes wrote: On Thu, 11 Sep 2014 14:59:35 +0300 Imre Deak imre.d...@intel.com wrote: On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote: Since correctness wins over optimal code and since the optimization Optimal code is also correct ;-) s/optimal/just plain broken/ Yes, bad wording. To clarify, since the optimization is now always off anyway (and it's also broken), I would hope that we could remove it for now and concentrate on fixing the existing s/r issues. Once we find that things are stable enough we could add back this optimization. Arg, I guess we didn't test after moving to the opregion test? Or maybe it was working when it landed for S3 and not for S4? Or broke sometime after it landed? Anyway this is a really valuable optimization for resume time on some platforms, and really we shouldn't have other agents clobbering our GTT on resume, so I'd really like to fix/re-add it asap. If/when we add this back I would suggest that we also add a sanity check that can be enabled with drm.debug which would verify the GTT mapping are what we expect. That way at least most developer would have it enabled and we'd catch problems earlier, and also bug reporters would be forced to enable it when we ask for dmesg w/ debugs. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote: Since correctness wins over optimal code and since the optimization Optimal code is also correct ;-) s/optimal/just plain broken/ -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote:
The logic to skip restoring GTT mappings was added to speed up
suspend/resume, but not on old GENs where not restoring them caused
problems. The check for old GENs is based on the existance of OpRegion,
but this doesn't work since opregion is initialized only after
the check. So we end up always restoring the mappings.
On my BYT - which has OpRegion - skipping restoring the mappings during
suspend doesn't work, I get a GPU hang after resume. Also the logic of
when to allow the optimization during S4 is reversed: we should allow it
during S4 thaw but not during S4 restore, but atm we have it the other
way around in the code.
Since correctness wins over optimal code and since the optimization
wasn't used anyway I decided not to try to fix it at this point, but
just remove it. This allows us to unify the S3 and S4 handlers in the
following patches.
Signed-off-by: Imre Deak imre.d...@intel.com
Adding Jesse. Also he claimed that it actually helped back in
commit 1abd02e2dd7e0bd577000301fb2fd47780637387
Author: Jesse Barnes jbar...@virtuousgeek.org
Date: Fri Nov 2 11:14:02 2012 -0700
drm/i915: don't rewrite the GTT on resume v4
dunno where exactly this broke.
-Daniel
---
drivers/gpu/drm/i915/i915_drv.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3addc2..5e25283 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
return ret;
}
-static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
+static int __i915_drm_thaw(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- if (drm_core_check_feature(dev, DRIVER_MODESET)
- restore_gtt_mappings) {
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
mutex_lock(dev-struct_mutex);
i915_gem_restore_gtt_mappings(dev);
mutex_unlock(dev-struct_mutex);
@@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET))
i915_check_and_clear_faults(dev);
- return __i915_drm_thaw(dev, true);
+ return __i915_drm_thaw(dev);
}
static int i915_resume_early(struct drm_device *dev)
@@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
static int i915_drm_resume(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
- /*
- * Platforms with opregion should have sane BIOS, older ones (gen3 and
- * earlier) need to restore the GTT mappings since the BIOS might clear
- * all our scratch PTEs.
- */
- ret = __i915_drm_thaw(dev, !dev_priv-opregion.header);
+ ret = __i915_drm_thaw(dev);
if (ret)
return ret;
--
1.8.4
___
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote: Since correctness wins over optimal code and since the optimization Optimal code is also correct ;-) s/optimal/just plain broken/ Yes, bad wording. To clarify, since the optimization is now always off anyway (and it's also broken), I would hope that we could remove it for now and concentrate on fixing the existing s/r issues. Once we find that things are stable enough we could add back this optimization. --Imre signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
On Thu, 11 Sep 2014 14:59:35 +0300 Imre Deak imre.d...@intel.com wrote: On Thu, 2014-09-11 at 08:49 +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 06:17:01PM +0300, Imre Deak wrote: Since correctness wins over optimal code and since the optimization Optimal code is also correct ;-) s/optimal/just plain broken/ Yes, bad wording. To clarify, since the optimization is now always off anyway (and it's also broken), I would hope that we could remove it for now and concentrate on fixing the existing s/r issues. Once we find that things are stable enough we could add back this optimization. Arg, I guess we didn't test after moving to the opregion test? Or maybe it was working when it landed for S3 and not for S4? Or broke sometime after it landed? Anyway this is a really valuable optimization for resume time on some platforms, and really we shouldn't have other agents clobbering our GTT on resume, so I'd really like to fix/re-add it asap. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
The logic to skip restoring GTT mappings was added to speed up
suspend/resume, but not on old GENs where not restoring them caused
problems. The check for old GENs is based on the existance of OpRegion,
but this doesn't work since opregion is initialized only after
the check. So we end up always restoring the mappings.
On my BYT - which has OpRegion - skipping restoring the mappings during
suspend doesn't work, I get a GPU hang after resume. Also the logic of
when to allow the optimization during S4 is reversed: we should allow it
during S4 thaw but not during S4 restore, but atm we have it the other
way around in the code.
Since correctness wins over optimal code and since the optimization
wasn't used anyway I decided not to try to fix it at this point, but
just remove it. This allows us to unify the S3 and S4 handlers in the
following patches.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3addc2..5e25283 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
return ret;
}
-static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
+static int __i915_drm_thaw(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- if (drm_core_check_feature(dev, DRIVER_MODESET)
- restore_gtt_mappings) {
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
mutex_lock(dev-struct_mutex);
i915_gem_restore_gtt_mappings(dev);
mutex_unlock(dev-struct_mutex);
@@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET))
i915_check_and_clear_faults(dev);
- return __i915_drm_thaw(dev, true);
+ return __i915_drm_thaw(dev);
}
static int i915_resume_early(struct drm_device *dev)
@@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
static int i915_drm_resume(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
- /*
-* Platforms with opregion should have sane BIOS, older ones (gen3 and
-* earlier) need to restore the GTT mappings since the BIOS might clear
-* all our scratch PTEs.
-*/
- ret = __i915_drm_thaw(dev, !dev_priv-opregion.header);
+ ret = __i915_drm_thaw(dev);
if (ret)
return ret;
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
