| Message ID | 1506661116-12106-1-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 29 Sep 2017 06:58:36 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > i915_gem_restore_fences is GEM resumption task hence it is moved to > i915_gem_resume from i915_restore_state. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
On Fri, 2017-09-29 at 10:28 +0530, Sagar Arun Kamble wrote: > i915_gem_restore_fences is GEM resumption task hence it is moved to > i915_gem_resume from i915_restore_state. > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Yep, this is a more appropriate place. Locality to the display resume was probably reason for the odd placement previously (it was added to fix a bug of fences not being there for display resume). Do we want to comment this requirement between i915_gem_resume and i915_restore_state? Chris? Anyway, Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas PS. My guess seems to be correct according to git blame (skipping one rename in between) info. > --- > drivers/gpu/drm/i915/i915_gem.c | 1 + > drivers/gpu/drm/i915/i915_suspend.c | 2 -- > 2 files changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 73eeb6b..ab8c694 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4595,6 +4595,7 @@ void i915_gem_resume(struct drm_i915_private *dev_priv) > > mutex_lock(&dev->struct_mutex); > i915_gem_restore_gtt_mappings(dev_priv); > + i915_gem_restore_fences(dev_priv); > > /* As we didn't flush the kernel context before suspend, we cannot > * guarantee that the context image is complete. So let's just reset > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c > index 5c86925a..8f3aa4d 100644 > --- a/drivers/gpu/drm/i915/i915_suspend.c > +++ b/drivers/gpu/drm/i915/i915_suspend.c > @@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private *dev_priv) > > mutex_lock(&dev_priv->drm.struct_mutex); > > - i915_gem_restore_fences(dev_priv); > - > if (IS_GEN4(dev_priv)) > pci_write_config_word(pdev, GCDGMBUS, > dev_priv->regfile.saveGCDGMBUS);
Quoting Joonas Lahtinen (2017-09-29 08:08:31) > On Fri, 2017-09-29 at 10:28 +0530, Sagar Arun Kamble wrote: > > i915_gem_restore_fences is GEM resumption task hence it is moved to > > i915_gem_resume from i915_restore_state. > > > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Yep, this is a more appropriate place. Locality to the display resume > was probably reason for the odd placement previously (it was added to > fix a bug of fences not being there for display resume). > > Do we want to comment this requirement between i915_gem_resume and > i915_restore_state? Chris? I'd forgotten about restore_state. That's old school save-the-world, restore-the-world for UMS code. Keep whittling it down and eventually it will go away, everything we do now should be independent of i915_suspend.c... > Anyway, > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Added to my queue for pushing, thanks for the patch and review. > Regards, Joonas > > PS. My guess seems to be correct according to git blame (skipping one > rename in between) info. Yes, debugging fences across resume has always been a case of fixing display corruption. :| -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 73eeb6b..ab8c694 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4595,6 +4595,7 @@ void i915_gem_resume(struct drm_i915_private *dev_priv) mutex_lock(&dev->struct_mutex); i915_gem_restore_gtt_mappings(dev_priv); + i915_gem_restore_fences(dev_priv); /* As we didn't flush the kernel context before suspend, we cannot * guarantee that the context image is complete. So let's just reset diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 5c86925a..8f3aa4d 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private *dev_priv) mutex_lock(&dev_priv->drm.struct_mutex); - i915_gem_restore_fences(dev_priv); - if (IS_GEN4(dev_priv)) pci_write_config_word(pdev, GCDGMBUS, dev_priv->regfile.saveGCDGMBUS);
i915_gem_restore_fences is GEM resumption task hence it is moved to i915_gem_resume from i915_restore_state. Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 1 + drivers/gpu/drm/i915/i915_suspend.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)