| Message ID | 1389245442-10947-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 09, 2014 at 11:00:42AM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > VLV PCTX will come from stolen memory. Upon every suspend/resume cycle, > this is being deallocated/reallocated. Given that PCTX has to be there > at a constant stolen memory offset, doing this is not required. Also > there is a chance of it getting corrupted, if this range gets used for > some other allocation(on resume), which can result in GPU hangs. > > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++ > drivers/gpu/drm/i915/intel_pm.c | 9 ++++----- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 29b3693..5cf97d6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return; > > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } This should be extracted into a intel_pm_teardown to mirror intel_pm_setup. I know that we already cleanup the fbc allocations here, but for sane driver teardown code we should move all those deallocations out eventually and just have a WARN_ON(drm_mm_not_empty) in here before tearing down the stolen mem subsystem. -Daniel > + > i915_gem_stolen_cleanup_compression(dev); > drm_mm_takedown(&dev_priv->mm.stolen); > } > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 469170c..369cc73 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3138,11 +3138,6 @@ static void valleyview_disable_rps(struct drm_device *dev) > I915_WRITE(GEN6_RC_CONTROL, 0); > > gen6_disable_rps_interrupts(dev); > - > - if (dev_priv->vlv_pctx) { > - drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > - dev_priv->vlv_pctx = NULL; > - } > } > > static void intel_print_rc6_info(struct drm_device *dev, u32 mode) > @@ -3508,6 +3503,10 @@ static void valleyview_setup_pctx(struct drm_device *dev) > u32 pcbr; > int pctx_size = 24*1024; > > + /* If PC Context is already there, then bail out*/ > + if (dev_priv->vlv_pctx) > + return; > + > pcbr = I915_READ(VLV_PCBR); > if (pcbr) { > /* BIOS set it up already, grab the pre-alloc'd space */ > -- > 1.8.5.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return; > > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } >> This should be extracted into a intel_pm_teardown to mirror intel_pm_setup. I know that we already cleanup the fbc allocations here, but for sane driver teardown code we should move all those deallocations out eventually and just >> have a WARN_ON(drm_mm_not_empty) in here before tearing down the stolen mem subsystem. Actually as of now there isn't any such function intel_pm_teardown defined to mirror intel_pm_setup. Although there is a intel_enable_gt_powersave & intel_disable_gt_powersave pair, but we couldn't use the intel_disable_gt_powersave for this, as we wanted to clean up the PCTX only at the time of driver unload. So the most appropriate place for us to do so was i915_gem_cleanup_stolen function. Best Regards Akash -----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, January 09, 2014 12:34 PM To: Goel, Akash Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 3/7] drm/i915/vlv: Not reallocating VLV PCTX upon every suspend/resume On Thu, Jan 09, 2014 at 11:00:42AM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > VLV PCTX will come from stolen memory. Upon every suspend/resume > cycle, this is being deallocated/reallocated. Given that PCTX has to > be there at a constant stolen memory offset, doing this is not > required. Also there is a chance of it getting corrupted, if this > range gets used for some other allocation(on resume), which can result in GPU hangs. > > Signed-off-by: Akash Goel <akash.goel@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_stolen.c | 5 +++++ > drivers/gpu/drm/i915/intel_pm.c | 9 ++++----- > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > b/drivers/gpu/drm/i915/i915_gem_stolen.c > index 29b3693..5cf97d6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) > if (!drm_mm_initialized(&dev_priv->mm.stolen)) > return; > > + if (dev_priv->vlv_pctx) { > + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > + dev_priv->vlv_pctx = NULL; > + } This should be extracted into a intel_pm_teardown to mirror intel_pm_setup. I know that we already cleanup the fbc allocations here, but for sane driver teardown code we should move all those deallocations out eventually and just have a WARN_ON(drm_mm_not_empty) in here before tearing down the stolen mem subsystem. -Daniel > + > i915_gem_stolen_cleanup_compression(dev); > drm_mm_takedown(&dev_priv->mm.stolen); > } > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c index 469170c..369cc73 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3138,11 +3138,6 @@ static void valleyview_disable_rps(struct drm_device *dev) > I915_WRITE(GEN6_RC_CONTROL, 0); > > gen6_disable_rps_interrupts(dev); > - > - if (dev_priv->vlv_pctx) { > - drm_gem_object_unreference(&dev_priv->vlv_pctx->base); > - dev_priv->vlv_pctx = NULL; > - } > } > > static void intel_print_rc6_info(struct drm_device *dev, u32 mode) @@ > -3508,6 +3503,10 @@ static void valleyview_setup_pctx(struct drm_device *dev) > u32 pcbr; > int pctx_size = 24*1024; > > + /* If PC Context is already there, then bail out*/ > + if (dev_priv->vlv_pctx) > + return; > + > pcbr = I915_READ(VLV_PCBR); > if (pcbr) { > /* BIOS set it up already, grab the pre-alloc'd space */ > -- > 1.8.5.2 > > _______________________________________________ > 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
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index 29b3693..5cf97d6 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -213,6 +213,11 @@ void i915_gem_cleanup_stolen(struct drm_device *dev) if (!drm_mm_initialized(&dev_priv->mm.stolen)) return; + if (dev_priv->vlv_pctx) { + drm_gem_object_unreference(&dev_priv->vlv_pctx->base); + dev_priv->vlv_pctx = NULL; + } + i915_gem_stolen_cleanup_compression(dev); drm_mm_takedown(&dev_priv->mm.stolen); } diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 469170c..369cc73 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3138,11 +3138,6 @@ static void valleyview_disable_rps(struct drm_device *dev) I915_WRITE(GEN6_RC_CONTROL, 0); gen6_disable_rps_interrupts(dev); - - if (dev_priv->vlv_pctx) { - drm_gem_object_unreference(&dev_priv->vlv_pctx->base); - dev_priv->vlv_pctx = NULL; - } } static void intel_print_rc6_info(struct drm_device *dev, u32 mode) @@ -3508,6 +3503,10 @@ static void valleyview_setup_pctx(struct drm_device *dev) u32 pcbr; int pctx_size = 24*1024; + /* If PC Context is already there, then bail out*/ + if (dev_priv->vlv_pctx) + return; + pcbr = I915_READ(VLV_PCBR); if (pcbr) { /* BIOS set it up already, grab the pre-alloc'd space */