| Message ID | 1396900911-12415-2-git-send-email-rodrigo.vivi@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: > From: Deepak S <deepak.s@intel.com> > > We need do forcewake before Disabling RC6, This is what the BIOS > expects while going into suspend. > > v2: updated commit message. (Daniel) > > Signed-off-by: Deepak S <deepak.s@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 04af065..ad2ff99 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + /* we're doing forcewake before Disabling RC6, > + * This what the BIOS expects when going into suspend */ > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > + > I915_WRITE(GEN6_RC_CONTROL, 0); > > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > + > gen6_disable_rps_interrupts(dev); > } > Isn't the forcewake done as part of I915_WRITE sufficient?
On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: > On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: > > From: Deepak S <deepak.s@intel.com> > > > > We need do forcewake before Disabling RC6, This is what the BIOS > > expects while going into suspend. > > > > v2: updated commit message. (Daniel) > > > > Signed-off-by: Deepak S <deepak.s@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 04af065..ad2ff99 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + /* we're doing forcewake before Disabling RC6, > > + * This what the BIOS expects when going into suspend */ > > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > + > > I915_WRITE(GEN6_RC_CONTROL, 0); > > > > + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > > + > > gen6_disable_rps_interrupts(dev); > > } > > > > Isn't the forcewake done as part of I915_WRITE sufficient? Writes don't do forcewake, nor is the register even part of the VLV forcewake ranges. I guess the rationale for this patche is still a bit vague. But if it's really needed, I wonder whether we should do this same dance for !VLV too? Do we have any "GPU stuck in wrong power state after suspend" type of bugs still around?
On 4/8/2014 6:13 PM, Ville Syrjälä wrote: > On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: >> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: >>> From: Deepak S <deepak.s@intel.com> >>> >>> We need do forcewake before Disabling RC6, This is what the BIOS >>> expects while going into suspend. >>> >>> v2: updated commit message. (Daniel) >>> >>> Signed-off-by: Deepak S <deepak.s@intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >>> --- >>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>> index 04af065..ad2ff99 100644 >>> --- a/drivers/gpu/drm/i915/intel_pm.c >>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) >>> { >>> struct drm_i915_private *dev_priv = dev->dev_private; >>> >>> + /* we're doing forcewake before Disabling RC6, >>> + * This what the BIOS expects when going into suspend */ >>> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); >>> + >>> I915_WRITE(GEN6_RC_CONTROL, 0); >>> >>> + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); >>> + >>> gen6_disable_rps_interrupts(dev); >>> } >>> >> >> Isn't the forcewake done as part of I915_WRITE sufficient? > > Writes don't do forcewake, nor is the register even part of the > VLV forcewake ranges. > > I guess the rationale for this patche is still a bit vague. But if it's > really needed, I wonder whether we should do this same dance for !VLV > too? Do we have any "GPU stuck in wrong power state after suspend" type of > bugs still around? One of suggestion form the HW team was to Bring the wells up before we disable RC6 at run-time. We did see some issue when we enabled D0ix. I think the is a good practice to make sure we bring-up the wells before we disable RC6. At least this avoids the cases where wells are not up before we can access the Next register after disable.
On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote: > > > On 4/8/2014 6:13 PM, Ville Syrjälä wrote: > >On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: > >>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: > >>>From: Deepak S <deepak.s@intel.com> > >>> > >>>We need do forcewake before Disabling RC6, This is what the BIOS > >>>expects while going into suspend. > >>> > >>>v2: updated commit message. (Daniel) > >>> > >>>Signed-off-by: Deepak S <deepak.s@intel.com> > >>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >>>--- > >>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > >>> 1 file changed, 6 insertions(+) > >>> > >>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>index 04af065..ad2ff99 100644 > >>>--- a/drivers/gpu/drm/i915/intel_pm.c > >>>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) > >>> { > >>> struct drm_i915_private *dev_priv = dev->dev_private; > >>> > >>>+ /* we're doing forcewake before Disabling RC6, > >>>+ * This what the BIOS expects when going into suspend */ > >>>+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > >>>+ > >>> I915_WRITE(GEN6_RC_CONTROL, 0); > >>> > >>>+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > >>>+ > >>> gen6_disable_rps_interrupts(dev); > >>> } > >>> > >> > >>Isn't the forcewake done as part of I915_WRITE sufficient? > > > >Writes don't do forcewake, nor is the register even part of the > >VLV forcewake ranges. > > > >I guess the rationale for this patche is still a bit vague. But if it's > >really needed, I wonder whether we should do this same dance for !VLV > >too? Do we have any "GPU stuck in wrong power state after suspend" type of > >bugs still around? > > One of suggestion form the HW team was to Bring the wells up before we > disable RC6 at run-time. We did see some issue when we enabled D0ix. > > I think the is a good practice to make sure we bring-up the wells before we > disable RC6. At least this avoids the cases where wells are not up before we > can access the Next register after disable. Ville was totally right. I do think a POSTING_READ is still sufficient. Don't care much either way.
On 4/9/2014 9:43 AM, Ben Widawsky wrote: > On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote: >> >> >> On 4/8/2014 6:13 PM, Ville Syrjälä wrote: >>> On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: >>>> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: >>>>> From: Deepak S <deepak.s@intel.com> >>>>> >>>>> We need do forcewake before Disabling RC6, This is what the BIOS >>>>> expects while going into suspend. >>>>> >>>>> v2: updated commit message. (Daniel) >>>>> >>>>> Signed-off-by: Deepak S <deepak.s@intel.com> >>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >>>>> --- >>>>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>> index 04af065..ad2ff99 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) >>>>> { >>>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>>> >>>>> + /* we're doing forcewake before Disabling RC6, >>>>> + * This what the BIOS expects when going into suspend */ >>>>> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); >>>>> + >>>>> I915_WRITE(GEN6_RC_CONTROL, 0); >>>>> >>>>> + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); >>>>> + >>>>> gen6_disable_rps_interrupts(dev); >>>>> } >>>>> >>>> >>>> Isn't the forcewake done as part of I915_WRITE sufficient? >>> >>> Writes don't do forcewake, nor is the register even part of the >>> VLV forcewake ranges. >>> >>> I guess the rationale for this patche is still a bit vague. But if it's >>> really needed, I wonder whether we should do this same dance for !VLV >>> too? Do we have any "GPU stuck in wrong power state after suspend" type of >>> bugs still around? >> >> One of suggestion form the HW team was to Bring the wells up before we >> disable RC6 at run-time. We did see some issue when we enabled D0ix. >> >> I think the is a good practice to make sure we bring-up the wells before we >> disable RC6. At least this avoids the cases where wells are not up before we >> can access the Next register after disable. > > Ville was totally right. I do think a POSTING_READ is still sufficient. > Don't care much either way. > If feel this patch is not adding any value. I OK dropping this patch.
On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote: > > > On 4/9/2014 9:43 AM, Ben Widawsky wrote: > >On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote: > >> > >> > >>On 4/8/2014 6:13 PM, Ville Syrjälä wrote: > >>>On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: > >>>>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: > >>>>>From: Deepak S <deepak.s@intel.com> > >>>>> > >>>>>We need do forcewake before Disabling RC6, This is what the BIOS > >>>>>expects while going into suspend. > >>>>> > >>>>>v2: updated commit message. (Daniel) > >>>>> > >>>>>Signed-off-by: Deepak S <deepak.s@intel.com> > >>>>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >>>>>--- > >>>>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > >>>>> 1 file changed, 6 insertions(+) > >>>>> > >>>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>>>index 04af065..ad2ff99 100644 > >>>>>--- a/drivers/gpu/drm/i915/intel_pm.c > >>>>>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>>>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) > >>>>> { > >>>>> struct drm_i915_private *dev_priv = dev->dev_private; > >>>>> > >>>>>+ /* we're doing forcewake before Disabling RC6, > >>>>>+ * This what the BIOS expects when going into suspend */ > >>>>>+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > >>>>>+ > >>>>> I915_WRITE(GEN6_RC_CONTROL, 0); > >>>>> > >>>>>+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > >>>>>+ > >>>>> gen6_disable_rps_interrupts(dev); > >>>>> } > >>>>> > >>>> > >>>>Isn't the forcewake done as part of I915_WRITE sufficient? > >>> > >>>Writes don't do forcewake, nor is the register even part of the > >>>VLV forcewake ranges. > >>> > >>>I guess the rationale for this patche is still a bit vague. But if it's > >>>really needed, I wonder whether we should do this same dance for !VLV > >>>too? Do we have any "GPU stuck in wrong power state after suspend" type of > >>>bugs still around? > >> > >>One of suggestion form the HW team was to Bring the wells up before we > >>disable RC6 at run-time. We did see some issue when we enabled D0ix. > >> > >>I think the is a good practice to make sure we bring-up the wells before we > >>disable RC6. At least this avoids the cases where wells are not up before we > >>can access the Next register after disable. > > > >Ville was totally right. I do think a POSTING_READ is still sufficient. > >Don't care much either way. > > > > If feel this patch is not adding any value. I OK dropping this patch. I think it makes a lot of sense - on the enable side we also grab the wells (in case the bios has enabled rc6 already) to make sure we change the rc6/rps state while everything is around. Can you please update your patch to also roll this out for gen6/8 rps disable functions? Thanks, Daniel
On Wednesday 09 April 2014 06:32 PM, Daniel Vetter wrote: > On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote: >> >> On 4/9/2014 9:43 AM, Ben Widawsky wrote: >>> On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote: >>>> >>>> On 4/8/2014 6:13 PM, Ville Syrjälä wrote: >>>>> On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: >>>>>> On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: >>>>>>> From: Deepak S <deepak.s@intel.com> >>>>>>> >>>>>>> We need do forcewake before Disabling RC6, This is what the BIOS >>>>>>> expects while going into suspend. >>>>>>> >>>>>>> v2: updated commit message. (Daniel) >>>>>>> >>>>>>> Signed-off-by: Deepak S <deepak.s@intel.com> >>>>>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ >>>>>>> 1 file changed, 6 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c >>>>>>> index 04af065..ad2ff99 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>>>>>> @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) >>>>>>> { >>>>>>> struct drm_i915_private *dev_priv = dev->dev_private; >>>>>>> >>>>>>> + /* we're doing forcewake before Disabling RC6, >>>>>>> + * This what the BIOS expects when going into suspend */ >>>>>>> + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); >>>>>>> + >>>>>>> I915_WRITE(GEN6_RC_CONTROL, 0); >>>>>>> >>>>>>> + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); >>>>>>> + >>>>>>> gen6_disable_rps_interrupts(dev); >>>>>>> } >>>>>>> >>>>>> Isn't the forcewake done as part of I915_WRITE sufficient? >>>>> Writes don't do forcewake, nor is the register even part of the >>>>> VLV forcewake ranges. >>>>> >>>>> I guess the rationale for this patche is still a bit vague. But if it's >>>>> really needed, I wonder whether we should do this same dance for !VLV >>>>> too? Do we have any "GPU stuck in wrong power state after suspend" type of >>>>> bugs still around? >>>> One of suggestion form the HW team was to Bring the wells up before we >>>> disable RC6 at run-time. We did see some issue when we enabled D0ix. >>>> >>>> I think the is a good practice to make sure we bring-up the wells before we >>>> disable RC6. At least this avoids the cases where wells are not up before we >>>> can access the Next register after disable. >>> Ville was totally right. I do think a POSTING_READ is still sufficient. >>> Don't care much either way. >>> >> If feel this patch is not adding any value. I OK dropping this patch. > I think it makes a lot of sense - on the enable side we also grab the > wells (in case the bios has enabled rc6 already) to make sure we change > the rc6/rps state while everything is around. > > Can you please update your patch to also roll this out for gen6/8 rps > disable functions? > > Thanks, Daniel Sure Daniel, I will add this to gen6/8 and submit the patch
On Wed, Apr 09, 2014 at 09:51:38AM +0530, S, Deepak wrote: > > > On 4/9/2014 9:43 AM, Ben Widawsky wrote: > >On Tue, Apr 08, 2014 at 06:22:52PM +0530, S, Deepak wrote: > >> > >> > >>On 4/8/2014 6:13 PM, Ville Syrjälä wrote: > >>>On Mon, Apr 07, 2014 at 02:36:20PM -0700, Ben Widawsky wrote: > >>>>On Mon, Apr 07, 2014 at 05:01:46PM -0300, Rodrigo Vivi wrote: > >>>>>From: Deepak S <deepak.s@intel.com> > >>>>> > >>>>>We need do forcewake before Disabling RC6, This is what the BIOS > >>>>>expects while going into suspend. > >>>>> > >>>>>v2: updated commit message. (Daniel) > >>>>> > >>>>>Signed-off-by: Deepak S <deepak.s@intel.com> > >>>>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> > >>>>>--- > >>>>> drivers/gpu/drm/i915/intel_pm.c | 6 ++++++ > >>>>> 1 file changed, 6 insertions(+) > >>>>> > >>>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > >>>>>index 04af065..ad2ff99 100644 > >>>>>--- a/drivers/gpu/drm/i915/intel_pm.c > >>>>>+++ b/drivers/gpu/drm/i915/intel_pm.c > >>>>>@@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) > >>>>> { > >>>>> struct drm_i915_private *dev_priv = dev->dev_private; > >>>>> > >>>>>+ /* we're doing forcewake before Disabling RC6, > >>>>>+ * This what the BIOS expects when going into suspend */ > >>>>>+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > >>>>>+ > >>>>> I915_WRITE(GEN6_RC_CONTROL, 0); > >>>>> > >>>>>+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); > >>>>>+ > >>>>> gen6_disable_rps_interrupts(dev); > >>>>> } > >>>>> > >>>> > >>>>Isn't the forcewake done as part of I915_WRITE sufficient? > >>> > >>>Writes don't do forcewake, nor is the register even part of the > >>>VLV forcewake ranges. > >>> > >>>I guess the rationale for this patche is still a bit vague. But if it's > >>>really needed, I wonder whether we should do this same dance for !VLV > >>>too? Do we have any "GPU stuck in wrong power state after suspend" type of > >>>bugs still around? > >> > >>One of suggestion form the HW team was to Bring the wells up before we > >>disable RC6 at run-time. We did see some issue when we enabled D0ix. > >> > >>I think the is a good practice to make sure we bring-up the wells before we > >>disable RC6. At least this avoids the cases where wells are not up before we > >>can access the Next register after disable. > > > >Ville was totally right. I do think a POSTING_READ is still sufficient. > >Don't care much either way. > > > > If feel this patch is not adding any value. I OK dropping this patch. I didn't mean it's not adding value. In general I like to address what the spec is trying to tell us to do, instead of following the letter of the spec. Here is another example of a very much missing, "why" as Ville was saying. If there is a bug it currently fixes, then I have no question about how to proceed.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 04af065..ad2ff99 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3198,8 +3198,14 @@ static void valleyview_disable_rps(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + /* we're doing forcewake before Disabling RC6, + * This what the BIOS expects when going into suspend */ + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); + I915_WRITE(GEN6_RC_CONTROL, 0); + gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); + gen6_disable_rps_interrupts(dev); }