[PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Jesse Barnes]

This may take awhile (~10ms), and we don't need to make noise about it.

References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
Tested-by: huax.lu@intel.com
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee27d74..4eebfd8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
 					dev_priv->rps.min_freq_softlimit);
 
-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
-				& GENFREQSTATUS) == 0, 5))
-		DRM_ERROR("timed out waiting for Punit\n");
-
 	vlv_force_gfx_clock(dev_priv, false);
 
 	I915_WRITE(GEN6_PMINTRMSK,
-- 
1.8.3.2

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Ville Syrjälä]

On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> This may take awhile (~10ms), and we don't need to make noise about it.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> Tested-by: huax.lu@intel.com
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ee27d74..4eebfd8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
>  					dev_priv->rps.min_freq_softlimit);
>  
> -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> -				& GENFREQSTATUS) == 0, 5))
> -		DRM_ERROR("timed out waiting for Punit\n");
> -

As I recall the entire point of this was to make sure the frequency
really drops before we allow turning off the clock. I'm not sure if we
can trust that the frequency (and more importantly the voltage) will
drop if we allow turning off the clock before the transition is
complete.

>  	vlv_force_gfx_clock(dev_priv, false);
>  
>  	I915_WRITE(GEN6_PMINTRMSK,
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Jesse Barnes]

On Fri, 6 Jun 2014 11:29:24 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> > This may take awhile (~10ms), and we don't need to make noise about it.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> > Tested-by: huax.lu@intel.com
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ee27d74..4eebfd8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> >  					dev_priv->rps.min_freq_softlimit);
> >  
> > -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> > -				& GENFREQSTATUS) == 0, 5))
> > -		DRM_ERROR("timed out waiting for Punit\n");
> > -
> 
> As I recall the entire point of this was to make sure the frequency
> really drops before we allow turning off the clock. I'm not sure if we
> can trust that the frequency (and more importantly the voltage) will
> drop if we allow turning off the clock before the transition is
> complete.

well, we may need to increase the timeout then... let's see if that
helps with the bug.

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Ville Syrjälä]

On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
> On Fri, 6 Jun 2014 11:29:24 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> > > This may take awhile (~10ms), and we don't need to make noise about it.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> > > Tested-by: huax.lu@intel.com
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index ee27d74..4eebfd8 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> > >  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> > >  					dev_priv->rps.min_freq_softlimit);
> > >  
> > > -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> > > -				& GENFREQSTATUS) == 0, 5))
> > > -		DRM_ERROR("timed out waiting for Punit\n");
> > > -
> > 
> > As I recall the entire point of this was to make sure the frequency
> > really drops before we allow turning off the clock. I'm not sure if we
> > can trust that the frequency (and more importantly the voltage) will
> > drop if we allow turning off the clock before the transition is
> > complete.
> 
> well, we may need to increase the timeout then... let's see if that
> helps with the bug.

FYI I played around with my BYT a bit and it doesn't appear to require
this "force gfx clock on" part of the workaround. 

I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
sure both render and media wells and the gfx clock remain off, and I
was also monitoring vnn via svid. While that was going on I just rewrote
PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
enough it did, and svid showed me that vnn had also changed. So it
appears there's no need to have the gfx clock on to change its frequency
on this BYT.

I wonder if this part of the workaround was only needed on older parts.
Deepak, any ideas?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[S, Deepak]

On 6/11/2014 10:26 PM, Ville Syrjälä wrote:
> On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
>> On Fri, 6 Jun 2014 11:29:24 +0300
>> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>
>>> On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
>>>> This may take awhile (~10ms), and we don't need to make noise about it.
>>>>
>>>> References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
>>>> Tested-by: huax.lu@intel.com
>>>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_pm.c | 4 ----
>>>>   1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index ee27d74..4eebfd8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>>>   	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
>>>>   					dev_priv->rps.min_freq_softlimit);
>>>>
>>>> -	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
>>>> -				& GENFREQSTATUS) == 0, 5))
>>>> -		DRM_ERROR("timed out waiting for Punit\n");
>>>> -
>>>
>>> As I recall the entire point of this was to make sure the frequency
>>> really drops before we allow turning off the clock. I'm not sure if we
>>> can trust that the frequency (and more importantly the voltage) will
>>> drop if we allow turning off the clock before the transition is
>>> complete.
>>
>> well, we may need to increase the timeout then... let's see if that
>> helps with the bug.
>
> FYI I played around with my BYT a bit and it doesn't appear to require
> this "force gfx clock on" part of the workaround.
>
> I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
> sure both render and media wells and the gfx clock remain off, and I
> was also monitoring vnn via svid. While that was going on I just rewrote
> PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
> enough it did, and svid showed me that vnn had also changed. So it
> appears there's no need to have the gfx clock on to change its frequency
> on this BYT.
>
> I wonder if this part of the workaround was only needed on older parts.
> Deepak, any ideas?

Yes ville, this was added initial for older parts and force gfx clock 
was part of the workaround.
We have not verified this on newer parts. Let me check with hw guys to 
see if workaround still exits and when this was fixed.

Thanks
Deepak

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Lee, Chon Ming]

On 06/12 08:14, S, Deepak wrote:
> 
> 
> On 6/11/2014 10:26 PM, Ville Syrjälä wrote:
> >On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
> >>On Fri, 6 Jun 2014 11:29:24 +0300
> >>Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >>
> >>>On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> >>>>This may take awhile (~10ms), and we don't need to make noise about it.
> >>>>
> >>>>References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> >>>>Tested-by: huax.lu@intel.com
> >>>>Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>>>---
> >>>>  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> >>>>  1 file changed, 4 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>index ee27d74..4eebfd8 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>@@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> >>>>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> >>>>  					dev_priv->rps.min_freq_softlimit);
> >>>>
> >>>>-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> >>>>-				& GENFREQSTATUS) == 0, 5))
> >>>>-		DRM_ERROR("timed out waiting for Punit\n");
> >>>>-
> >>>
> >>>As I recall the entire point of this was to make sure the frequency
> >>>really drops before we allow turning off the clock. I'm not sure if we
> >>>can trust that the frequency (and more importantly the voltage) will
> >>>drop if we allow turning off the clock before the transition is
> >>>complete.
> >>
> >>well, we may need to increase the timeout then... let's see if that
> >>helps with the bug.
> >
> >FYI I played around with my BYT a bit and it doesn't appear to require
> >this "force gfx clock on" part of the workaround.
> >
> >I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
> >sure both render and media wells and the gfx clock remain off, and I
> >was also monitoring vnn via svid. While that was going on I just rewrote
> >PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
> >enough it did, and svid showed me that vnn had also changed. So it
> >appears there's no need to have the gfx clock on to change its frequency
> >on this BYT.
> >
> >I wonder if this part of the workaround was only needed on older parts.
> >Deepak, any ideas?
> 
> Yes ville, this was added initial for older parts and force gfx
> clock was part of the workaround.
> We have not verified this on newer parts. Let me check with hw guys
> to see if workaround still exits and when this was fixed.
> 
> Thanks
> Deepak

This is a bit side topic.  In the vlv_set_rps_idle, it try to mask the turbo
interrupt by writing to GEN6_PMINTRMSK (offset 0xA168).  But based on the bspec,
the PMIMR for BYT/GEN6 should be 0x44024 (GEN6_PMIMR).  

Not sure I miss anything.  If there offset is wrong, mean the turbo has not mask at
all.

Regards,
Chon Ming

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Daniel Vetter]

On Thu, Jun 12, 2014 at 03:37:18PM +0800, Lee, Chon Ming wrote:
> On 06/12 08:14, S, Deepak wrote:
> > 
> > 
> > On 6/11/2014 10:26 PM, Ville Syrjälä wrote:
> > >On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
> > >>On Fri, 6 Jun 2014 11:29:24 +0300
> > >>Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >>
> > >>>On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> > >>>>This may take awhile (~10ms), and we don't need to make noise about it.
> > >>>>
> > >>>>References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> > >>>>Tested-by: huax.lu@intel.com
> > >>>>Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >>>>---
> > >>>>  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> > >>>>  1 file changed, 4 deletions(-)
> > >>>>
> > >>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > >>>>index ee27d74..4eebfd8 100644
> > >>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> > >>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > >>>>@@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> > >>>>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> > >>>>  					dev_priv->rps.min_freq_softlimit);
> > >>>>
> > >>>>-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> > >>>>-				& GENFREQSTATUS) == 0, 5))
> > >>>>-		DRM_ERROR("timed out waiting for Punit\n");
> > >>>>-
> > >>>
> > >>>As I recall the entire point of this was to make sure the frequency
> > >>>really drops before we allow turning off the clock. I'm not sure if we
> > >>>can trust that the frequency (and more importantly the voltage) will
> > >>>drop if we allow turning off the clock before the transition is
> > >>>complete.
> > >>
> > >>well, we may need to increase the timeout then... let's see if that
> > >>helps with the bug.
> > >
> > >FYI I played around with my BYT a bit and it doesn't appear to require
> > >this "force gfx clock on" part of the workaround.
> > >
> > >I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
> > >sure both render and media wells and the gfx clock remain off, and I
> > >was also monitoring vnn via svid. While that was going on I just rewrote
> > >PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
> > >enough it did, and svid showed me that vnn had also changed. So it
> > >appears there's no need to have the gfx clock on to change its frequency
> > >on this BYT.
> > >
> > >I wonder if this part of the workaround was only needed on older parts.
> > >Deepak, any ideas?
> > 
> > Yes ville, this was added initial for older parts and force gfx
> > clock was part of the workaround.
> > We have not verified this on newer parts. Let me check with hw guys
> > to see if workaround still exits and when this was fixed.
> > 
> > Thanks
> > Deepak
> 
> This is a bit side topic.  In the vlv_set_rps_idle, it try to mask the turbo
> interrupt by writing to GEN6_PMINTRMSK (offset 0xA168).  But based on the bspec,
> the PMIMR for BYT/GEN6 should be 0x44024 (GEN6_PMIMR).  
> 
> Not sure I miss anything.  If there offset is wrong, mean the turbo has not mask at
> all.

Afaik PM interrupt handling has 3 levels: GEN6_PMINTRMSK is the lowest and
only provides a mask to prevent irq event forwarding, GEN6_PMI[IEMS]R are
the normal set of 4 registers and then there's the PM interrupt bit in the
DE interrupt registers.

Iirc masking in PMINTRMSK has power benefits, but I'm not sure.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Ville Syrjälä]

On Thu, Jun 12, 2014 at 03:37:18PM +0800, Lee, Chon Ming wrote:
> On 06/12 08:14, S, Deepak wrote:
> > 
> > 
> > On 6/11/2014 10:26 PM, Ville Syrjälä wrote:
> > >On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
> > >>On Fri, 6 Jun 2014 11:29:24 +0300
> > >>Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >>
> > >>>On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> > >>>>This may take awhile (~10ms), and we don't need to make noise about it.
> > >>>>
> > >>>>References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> > >>>>Tested-by: huax.lu@intel.com
> > >>>>Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >>>>---
> > >>>>  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> > >>>>  1 file changed, 4 deletions(-)
> > >>>>
> > >>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > >>>>index ee27d74..4eebfd8 100644
> > >>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> > >>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > >>>>@@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> > >>>>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> > >>>>  					dev_priv->rps.min_freq_softlimit);
> > >>>>
> > >>>>-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> > >>>>-				& GENFREQSTATUS) == 0, 5))
> > >>>>-		DRM_ERROR("timed out waiting for Punit\n");
> > >>>>-
> > >>>
> > >>>As I recall the entire point of this was to make sure the frequency
> > >>>really drops before we allow turning off the clock. I'm not sure if we
> > >>>can trust that the frequency (and more importantly the voltage) will
> > >>>drop if we allow turning off the clock before the transition is
> > >>>complete.
> > >>
> > >>well, we may need to increase the timeout then... let's see if that
> > >>helps with the bug.
> > >
> > >FYI I played around with my BYT a bit and it doesn't appear to require
> > >this "force gfx clock on" part of the workaround.
> > >
> > >I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
> > >sure both render and media wells and the gfx clock remain off, and I
> > >was also monitoring vnn via svid. While that was going on I just rewrote
> > >PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
> > >enough it did, and svid showed me that vnn had also changed. So it
> > >appears there's no need to have the gfx clock on to change its frequency
> > >on this BYT.
> > >
> > >I wonder if this part of the workaround was only needed on older parts.
> > >Deepak, any ideas?
> > 
> > Yes ville, this was added initial for older parts and force gfx
> > clock was part of the workaround.
> > We have not verified this on newer parts. Let me check with hw guys
> > to see if workaround still exits and when this was fixed.
> > 
> > Thanks
> > Deepak
> 
> This is a bit side topic.  In the vlv_set_rps_idle, it try to mask the turbo
> interrupt by writing to GEN6_PMINTRMSK (offset 0xA168).  But based on the bspec,
> the PMIMR for BYT/GEN6 should be 0x44024 (GEN6_PMIMR).  
> 
> Not sure I miss anything.  If there offset is wrong, mean the turbo has not mask at
> all.

There are two mask registers for this stuff. PMINTRMSK is some kind
of lower level mask, and GEN6_PMIMR is the regular interrupt mask.
I believe masking the interrupt in PMINTRMSK prevents it from being
visible in GEN6_PMISR even.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Lee, Chon Ming]

On 06/12 11:11, Ville Syrjälä wrote:
> On Thu, Jun 12, 2014 at 03:37:18PM +0800, Lee, Chon Ming wrote:
> > On 06/12 08:14, S, Deepak wrote:
> > > 
> > > 
> > > On 6/11/2014 10:26 PM, Ville Syrjälä wrote:
> > > >On Fri, Jun 06, 2014 at 08:03:20AM -0700, Jesse Barnes wrote:
> > > >>On Fri, 6 Jun 2014 11:29:24 +0300
> > > >>Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > >>
> > > >>>On Thu, Jun 05, 2014 at 01:49:34PM -0700, Jesse Barnes wrote:
> > > >>>>This may take awhile (~10ms), and we don't need to make noise about it.
> > > >>>>
> > > >>>>References: https://bugs.freedesktop.org/show_bug.cgi?id=75244
> > > >>>>Tested-by: huax.lu@intel.com
> > > >>>>Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > >>>>---
> > > >>>>  drivers/gpu/drm/i915/intel_pm.c | 4 ----
> > > >>>>  1 file changed, 4 deletions(-)
> > > >>>>
> > > >>>>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > >>>>index ee27d74..4eebfd8 100644
> > > >>>>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > >>>>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > >>>>@@ -3227,10 +3227,6 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
> > > >>>>  	vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> > > >>>>  					dev_priv->rps.min_freq_softlimit);
> > > >>>>
> > > >>>>-	if (wait_for(((vlv_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS))
> > > >>>>-				& GENFREQSTATUS) == 0, 5))
> > > >>>>-		DRM_ERROR("timed out waiting for Punit\n");
> > > >>>>-
> > > >>>
> > > >>>As I recall the entire point of this was to make sure the frequency
> > > >>>really drops before we allow turning off the clock. I'm not sure if we
> > > >>>can trust that the frequency (and more importantly the voltage) will
> > > >>>drop if we allow turning off the clock before the transition is
> > > >>>complete.
> > > >>
> > > >>well, we may need to increase the timeout then... let's see if that
> > > >>helps with the bug.
> > > >
> > > >FYI I played around with my BYT a bit and it doesn't appear to require
> > > >this "force gfx clock on" part of the workaround.
> > > >
> > > >I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
> > > >sure both render and media wells and the gfx clock remain off, and I
> > > >was also monitoring vnn via svid. While that was going on I just rewrote
> > > >PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
> > > >enough it did, and svid showed me that vnn had also changed. So it
> > > >appears there's no need to have the gfx clock on to change its frequency
> > > >on this BYT.
> > > >
> > > >I wonder if this part of the workaround was only needed on older parts.
> > > >Deepak, any ideas?
> > > 
> > > Yes ville, this was added initial for older parts and force gfx
> > > clock was part of the workaround.
> > > We have not verified this on newer parts. Let me check with hw guys
> > > to see if workaround still exits and when this was fixed.
> > > 
> > > Thanks
> > > Deepak
> > 
> > This is a bit side topic.  In the vlv_set_rps_idle, it try to mask the turbo
> > interrupt by writing to GEN6_PMINTRMSK (offset 0xA168).  But based on the bspec,
> > the PMIMR for BYT/GEN6 should be 0x44024 (GEN6_PMIMR).  
> > 
> > Not sure I miss anything.  If there offset is wrong, mean the turbo has not mask at
> > all.
> 
> There are two mask registers for this stuff. PMINTRMSK is some kind
> of lower level mask, and GEN6_PMIMR is the regular interrupt mask.
> I believe masking the interrupt in PMINTRMSK prevents it from being
> visible in GEN6_PMISR even.
> 

Thanks for the info.  Got that, we can't use GEN6PMIMR to mask interrupt, need
to use PMINTRMSK.

Regards,
Chon Ming

> -- 
> Ville Syrjälä
> Intel OTC

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[S, Deepak]

 >> I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
 >> sure both render and media wells and the gfx clock remain off, and I
 >> was also monitoring vnn via svid. While that was going on I just rewrote
 >> PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
 >> enough it did, and svid showed me that vnn had also changed. So it
 >> appears there's no need to have the gfx clock on to change its frequency
 >> on this BYT.
 >>
 >> I wonder if this part of the workaround was only needed on older parts.
 >> Deepak, any ideas?
 >
 > Yes ville, this was added initial for older parts and force gfx clock
 > was part of the workaround.
 > We have not verified this on newer parts. Let me check with hw guys to
 > see if workaround still exits and when this was fixed.

Hi Ville, Got the confirmation from HW team, this WA as been fixed in 
latest stepping, we can remove the force gfx clock, mask and request 
only the min freq when we are idle.

You will submit patch will fix or you want me to do it?

Thanks
Deepak

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[Ville Syrjälä]

On Thu, Jun 12, 2014 at 11:45:03PM +0530, S, Deepak wrote:
>  >> I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
>  >> sure both render and media wells and the gfx clock remain off, and I
>  >> was also monitoring vnn via svid. While that was going on I just rewrote
>  >> PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
>  >> enough it did, and svid showed me that vnn had also changed. So it
>  >> appears there's no need to have the gfx clock on to change its frequency
>  >> on this BYT.
>  >>
>  >> I wonder if this part of the workaround was only needed on older parts.
>  >> Deepak, any ideas?
>  >
>  > Yes ville, this was added initial for older parts and force gfx clock
>  > was part of the workaround.
>  > We have not verified this on newer parts. Let me check with hw guys to
>  > see if workaround still exits and when this was fixed.
> 
> Hi Ville, Got the confirmation from HW team, this WA as been fixed in 
> latest stepping,

What's latest here? Did we ever ship any of the steppings that still
need the gfx clock force?

> we can remove the force gfx clock, mask and request 
> only the min freq when we are idle.
> 
> You will submit patch will fix or you want me to do it?

Go ahead if you have time. I'm already juggling too many things :)

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915/vlv: drop punit freq staus read after setting idle

[S, Deepak]

On 6/13/2014 12:10 AM, Ville Syrjälä wrote:
> On Thu, Jun 12, 2014 at 11:45:03PM +0530, S, Deepak wrote:
>>   >> I was polling VLV_GTLC_SURVIVABILITY_REG and VLV_GTLC_PW_STATUS to make
>>   >> sure both render and media wells and the gfx clock remain off, and I
>>   >> was also monitoring vnn via svid. While that was going on I just rewrote
>>   >> PUNIT_REG_GPU_FREQ_REQ to make Punit change the frequency, and sure
>>   >> enough it did, and svid showed me that vnn had also changed. So it
>>   >> appears there's no need to have the gfx clock on to change its frequency
>>   >> on this BYT.
>>   >>
>>   >> I wonder if this part of the workaround was only needed on older parts.
>>   >> Deepak, any ideas?
>>   >
>>   > Yes ville, this was added initial for older parts and force gfx clock
>>   > was part of the workaround.
>>   > We have not verified this on newer parts. Let me check with hw guys to
>>   > see if workaround still exits and when this was fixed.
>>
>> Hi Ville, Got the confirmation from HW team, this WA as been fixed in
>> latest stepping,
>
> What's latest here? Did we ever ship any of the steppings that still
> need the gfx clock force?
>
>> we can remove the force gfx clock, mask and request
>> only the min freq when we are idle.
>>
>> You will submit patch will fix or you want me to do it?
>
> Go ahead if you have time. I'm already juggling too many things :)
Ok. I will send the patch for review :)