Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
> From: Deepak S <deepak.s@linux.intel.com>
> 
> In chv, we have two power wells Render & Media. We need to use
> corresponsing forcewake count. If we dont follow this we are getting
> error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
> multiple entry into __vlv_force_wake_get.
> 
> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index bd1b28d..bafd38b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>  	 */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (dev_priv->uncore.forcewake_count++ == 0)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +		if (dev_priv->uncore.fw_rendercount++ == 0)
> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> +							      FORCEWAKE_RENDER);
> +		if (dev_priv->uncore.fw_mediacount++ == 0)
> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> +							      FORCEWAKE_MEDIA);

This will wake both wells. Is that needed or should we just pick one
based on the ring?

> +	} else {
> +		if (dev_priv->uncore.forcewake_count++ == 0)
> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> +							      FORCEWAKE_ALL);
> +	}
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
>  
>  	I915_WRITE(RING_ELSP(ring), desc[1]);
> @@ -315,8 +325,19 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>  
>  	/* Release Force Wakeup (see the big comment above). */
>  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> +		if (--dev_priv->uncore.fw_rendercount == 0)
> +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> +							      FORCEWAKE_RENDER);
> +		if (--dev_priv->uncore.fw_mediacount == 0)
> +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> +							      FORCEWAKE_MEDIA);
> +	} else {
> +		if (--dev_priv->uncore.forcewake_count == 0)
> +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> +							      FORCEWAKE_ALL);
> +	}
> +
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
>  }
>  
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Ville Syrjälä]

On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
> > From: Deepak S <deepak.s@linux.intel.com>
> > 
> > In chv, we have two power wells Render & Media. We need to use
> > corresponsing forcewake count. If we dont follow this we are getting
> > error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
> > multiple entry into __vlv_force_wake_get.
> > 
> > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> >  1 file changed, 25 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index bd1b28d..bafd38b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> >  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> >  	 */
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -	if (dev_priv->uncore.forcewake_count++ == 0)
> > -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > +		if (dev_priv->uncore.fw_rendercount++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_RENDER);
> > +		if (dev_priv->uncore.fw_mediacount++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_MEDIA);
> 
> This will wake both wells. Is that needed or should we just pick one
> based on the ring?

Also unlike the comment says runtime_pm_get() can't sleep since someone
must already be holding a reference, othwewise we surely can't go
writing any registers. So in theory we should be able to call
gen6_gt_force_wake_get() here, but maybe that would trigger a
might_sleep() warning. the current force wake code duplication (esp.
outside intel_uncore.c) is rather unfortunate and I'd like to see it
killed off. Maybe we just need to pull the rpm get/put outside
gen6_gt_force_wake_get()? I never really liked hiding it there anyway.

> 
> > +	} else {
> > +		if (dev_priv->uncore.forcewake_count++ == 0)
> > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +							      FORCEWAKE_ALL);
> > +	}
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> >  
> >  	I915_WRITE(RING_ELSP(ring), desc[1]);
> > @@ -315,8 +325,19 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> >  
> >  	/* Release Force Wakeup (see the big comment above). */
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -	if (--dev_priv->uncore.forcewake_count == 0)
> > -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > +		if (--dev_priv->uncore.fw_rendercount == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_RENDER);
> > +		if (--dev_priv->uncore.fw_mediacount == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_MEDIA);
> > +	} else {
> > +		if (--dev_priv->uncore.forcewake_count == 0)
> > +			dev_priv->uncore.funcs.force_wake_put(dev_priv,
> > +							      FORCEWAKE_ALL);
> > +	}
> > +
> >  	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> >  }
> >  
> > -- 
> > 1.9.1
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Daniel Vetter]

On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
> > > From: Deepak S <deepak.s@linux.intel.com>
> > > 
> > > In chv, we have two power wells Render & Media. We need to use
> > > corresponsing forcewake count. If we dont follow this we are getting
> > > error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
> > > multiple entry into __vlv_force_wake_get.
> > > 
> > > Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
> > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > > index bd1b28d..bafd38b 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
> > >  	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> > >  	 */
> > >  	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > > -	if (dev_priv->uncore.forcewake_count++ == 0)
> > > -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > > +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > > +		if (dev_priv->uncore.fw_rendercount++ == 0)
> > > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > +							      FORCEWAKE_RENDER);
> > > +		if (dev_priv->uncore.fw_mediacount++ == 0)
> > > +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > +							      FORCEWAKE_MEDIA);
> > 
> > This will wake both wells. Is that needed or should we just pick one
> > based on the ring?
> 
> Also unlike the comment says runtime_pm_get() can't sleep since someone
> must already be holding a reference, othwewise we surely can't go
> writing any registers. So in theory we should be able to call
> gen6_gt_force_wake_get() here, but maybe that would trigger a
> might_sleep() warning. the current force wake code duplication (esp.
> outside intel_uncore.c) is rather unfortunate and I'd like to see it
> killed off. Maybe we just need to pull the rpm get/put outside
> gen6_gt_force_wake_get()? I never really liked hiding it there anyway.

Yeah this is just broken design. And if you look at the other wheel to
track outstanding gpu work (requests) then it's not needed at all.

But I'm not sure what's the priority of the "rework execlists to use
requests" task is and when (if ever that will happen). Jesse is the
arbiter for this stuff anyway, so adding him.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[deepak.s]

From: Deepak S <deepak.s@linux.intel.com>

In chv, we have two power wells Render & Media. We need to use
corresponsing forcewake count. If we dont follow this we are getting
error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
multiple entry into __vlv_force_wake_get.

Signed-off-by: Deepak S <deepak.s@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index bd1b28d..bafd38b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
 	 */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (dev_priv->uncore.forcewake_count++ == 0)
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
+	if (IS_CHERRYVIEW(dev_priv->dev)) {
+		if (dev_priv->uncore.fw_rendercount++ == 0)
+			dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							      FORCEWAKE_RENDER);
+		if (dev_priv->uncore.fw_mediacount++ == 0)
+			dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							      FORCEWAKE_MEDIA);
+	} else {
+		if (dev_priv->uncore.forcewake_count++ == 0)
+			dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							      FORCEWAKE_ALL);
+	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
 
 	I915_WRITE(RING_ELSP(ring), desc[1]);
@@ -315,8 +325,19 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
 
 	/* Release Force Wakeup (see the big comment above). */
 	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (--dev_priv->uncore.forcewake_count == 0)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
+	if (IS_CHERRYVIEW(dev_priv->dev)) {
+		if (--dev_priv->uncore.fw_rendercount == 0)
+			dev_priv->uncore.funcs.force_wake_put(dev_priv,
+							      FORCEWAKE_RENDER);
+		if (--dev_priv->uncore.fw_mediacount == 0)
+			dev_priv->uncore.funcs.force_wake_put(dev_priv,
+							      FORCEWAKE_MEDIA);
+	} else {
+		if (--dev_priv->uncore.forcewake_count == 0)
+			dev_priv->uncore.funcs.force_wake_put(dev_priv,
+							      FORCEWAKE_ALL);
+	}
+
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
 }
 
-- 
1.9.1

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Deepak S]

On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
> On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
>> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
>>> On Tue, Sep 09, 2014 at 07:14:16PM +0530, deepak.s@linux.intel.com wrote:
>>>> From: Deepak S <deepak.s@linux.intel.com>
>>>>
>>>> In chv, we have two power wells Render & Media. We need to use
>>>> corresponsing forcewake count. If we dont follow this we are getting
>>>> error "*ERROR*: Timed out waiting for forcewake old ack to clear" due to
>>>> multiple entry into __vlv_force_wake_get.
>>>>
>>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/intel_lrc.c | 29 +++++++++++++++++++++++++----
>>>>   1 file changed, 25 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>>>> index bd1b28d..bafd38b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>>>> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct intel_engine_cs *ring,
>>>>   	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
>>>>   	 */
>>>>   	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
>>>> -	if (dev_priv->uncore.forcewake_count++ == 0)
>>>> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
>>>> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
>>>> +		if (dev_priv->uncore.fw_rendercount++ == 0)
>>>> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>>>> +							      FORCEWAKE_RENDER);
>>>> +		if (dev_priv->uncore.fw_mediacount++ == 0)
>>>> +			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>>>> +							      FORCEWAKE_MEDIA);
>>> This will wake both wells. Is that needed or should we just pick one
>>> based on the ring?
>> Also unlike the comment says runtime_pm_get() can't sleep since someone
>> must already be holding a reference, othwewise we surely can't go
>> writing any registers. So in theory we should be able to call
>> gen6_gt_force_wake_get() here, but maybe that would trigger a
>> might_sleep() warning. the current force wake code duplication (esp.
>> outside intel_uncore.c) is rather unfortunate and I'd like to see it
>> killed off. Maybe we just need to pull the rpm get/put outside
>> gen6_gt_force_wake_get()? I never really liked hiding it there anyway.
> Yeah this is just broken design. And if you look at the other wheel to
> track outstanding gpu work (requests) then it's not needed at all.
>
> But I'm not sure what's the priority of the "rework execlists to use
> requests" task is and when (if ever that will happen). Jesse is the
> arbiter for this stuff anyway, so adding him.
> -Daniel

hmm , agreed do we have a reworked execlist? The reason why added this, on chv when i enable execlist, due to incorrect forcewake count
is causing multiple entries to forcewake_get resulting in "*ERROR*: Timed out waiting for forcewake old ack to clear" "and Hang.

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Jesse Barnes]

On Tue, 09 Sep 2014 21:45:08 +0530
Deepak S <deepak.s@intel.com> wrote:

> 
> On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
> > On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
> >> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> >>> On Tue, Sep 09, 2014 at 07:14:16PM +0530,
> >>> deepak.s@linux.intel.com wrote:
> >>>> From: Deepak S <deepak.s@linux.intel.com>
> >>>>
> >>>> In chv, we have two power wells Render & Media. We need to use
> >>>> corresponsing forcewake count. If we dont follow this we are
> >>>> getting error "*ERROR*: Timed out waiting for forcewake old ack
> >>>> to clear" due to multiple entry into __vlv_force_wake_get.
> >>>>
> >>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> >>>> ---
> >>>>   drivers/gpu/drm/i915/intel_lrc.c | 29
> >>>> +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+),
> >>>> 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> >>>> b/drivers/gpu/drm/i915/intel_lrc.c index bd1b28d..bafd38b 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >>>> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct
> >>>> intel_engine_cs *ring,
> >>>>   	 * Instead, we do the runtime_pm_get/put when
> >>>> creating/destroying requests. */
> >>>>   	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> >>>> -	if (dev_priv->uncore.forcewake_count++ == 0)
> >>>> -		dev_priv->uncore.funcs.force_wake_get(dev_priv,
> >>>> FORCEWAKE_ALL);
> >>>> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> >>>> +		if (dev_priv->uncore.fw_rendercount++ == 0)
> >>>> +
> >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> >>>> +
> >>>> FORCEWAKE_RENDER);
> >>>> +		if (dev_priv->uncore.fw_mediacount++ == 0)
> >>>> +
> >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> >>>> +
> >>>> FORCEWAKE_MEDIA);
> >>> This will wake both wells. Is that needed or should we just pick
> >>> one based on the ring?
> >> Also unlike the comment says runtime_pm_get() can't sleep since
> >> someone must already be holding a reference, othwewise we surely
> >> can't go writing any registers. So in theory we should be able to
> >> call gen6_gt_force_wake_get() here, but maybe that would trigger a
> >> might_sleep() warning. the current force wake code duplication
> >> (esp. outside intel_uncore.c) is rather unfortunate and I'd like
> >> to see it killed off. Maybe we just need to pull the rpm get/put
> >> outside gen6_gt_force_wake_get()? I never really liked hiding it
> >> there anyway.
> > Yeah this is just broken design. And if you look at the other wheel
> > to track outstanding gpu work (requests) then it's not needed at
> > all.
> >
> > But I'm not sure what's the priority of the "rework execlists to use
> > requests" task is and when (if ever that will happen). Jesse is the
> > arbiter for this stuff anyway, so adding him.
> > -Daniel
> 
> hmm , agreed do we have a reworked execlist? The reason why added
> this, on chv when i enable execlist, due to incorrect forcewake count
> is causing multiple entries to forcewake_get resulting in "*ERROR*:
> Timed out waiting for forcewake old ack to clear" "and Hang.

I'm hoping we can get execlists reworked on top of the request/seqno
stuff shortly after it lands, but I don't think that's a reason to
block this fix, since Chris is still busy fixing up the request
changes.

Jesse 

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

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Daniel Vetter]

On Tue, Sep 09, 2014 at 02:25:50PM -0700, Jesse Barnes wrote:
> On Tue, 09 Sep 2014 21:45:08 +0530
> Deepak S <deepak.s@intel.com> wrote:
> 
> > 
> > On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
> > > On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
> > >> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> > >>> On Tue, Sep 09, 2014 at 07:14:16PM +0530,
> > >>> deepak.s@linux.intel.com wrote:
> > >>>> From: Deepak S <deepak.s@linux.intel.com>
> > >>>>
> > >>>> In chv, we have two power wells Render & Media. We need to use
> > >>>> corresponsing forcewake count. If we dont follow this we are
> > >>>> getting error "*ERROR*: Timed out waiting for forcewake old ack
> > >>>> to clear" due to multiple entry into __vlv_force_wake_get.
> > >>>>
> > >>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > >>>> ---
> > >>>>   drivers/gpu/drm/i915/intel_lrc.c | 29
> > >>>> +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+),
> > >>>> 4 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > >>>> b/drivers/gpu/drm/i915/intel_lrc.c index bd1b28d..bafd38b 100644
> > >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> > >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > >>>> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct
> > >>>> intel_engine_cs *ring,
> > >>>>   	 * Instead, we do the runtime_pm_get/put when
> > >>>> creating/destroying requests. */
> > >>>>   	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > >>>> -	if (dev_priv->uncore.forcewake_count++ == 0)
> > >>>> -		dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > >>>> FORCEWAKE_ALL);
> > >>>> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > >>>> +		if (dev_priv->uncore.fw_rendercount++ == 0)
> > >>>> +
> > >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > >>>> +
> > >>>> FORCEWAKE_RENDER);
> > >>>> +		if (dev_priv->uncore.fw_mediacount++ == 0)
> > >>>> +
> > >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > >>>> +
> > >>>> FORCEWAKE_MEDIA);
> > >>> This will wake both wells. Is that needed or should we just pick
> > >>> one based on the ring?
> > >> Also unlike the comment says runtime_pm_get() can't sleep since
> > >> someone must already be holding a reference, othwewise we surely
> > >> can't go writing any registers. So in theory we should be able to
> > >> call gen6_gt_force_wake_get() here, but maybe that would trigger a
> > >> might_sleep() warning. the current force wake code duplication
> > >> (esp. outside intel_uncore.c) is rather unfortunate and I'd like
> > >> to see it killed off. Maybe we just need to pull the rpm get/put
> > >> outside gen6_gt_force_wake_get()? I never really liked hiding it
> > >> there anyway.
> > > Yeah this is just broken design. And if you look at the other wheel
> > > to track outstanding gpu work (requests) then it's not needed at
> > > all.
> > >
> > > But I'm not sure what's the priority of the "rework execlists to use
> > > requests" task is and when (if ever that will happen). Jesse is the
> > > arbiter for this stuff anyway, so adding him.
> > > -Daniel
> > 
> > hmm , agreed do we have a reworked execlist? The reason why added
> > this, on chv when i enable execlist, due to incorrect forcewake count
> > is causing multiple entries to forcewake_get resulting in "*ERROR*:
> > Timed out waiting for forcewake old ack to clear" "and Hang.
> 
> I'm hoping we can get execlists reworked on top of the request/seqno
> stuff shortly after it lands, but I don't think that's a reason to
> block this fix, since Chris is still busy fixing up the request
> changes.

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Daniel Vetter]

On Wed, Sep 10, 2014 at 09:16:45AM +0200, Daniel Vetter wrote:
> On Tue, Sep 09, 2014 at 02:25:50PM -0700, Jesse Barnes wrote:
> > On Tue, 09 Sep 2014 21:45:08 +0530
> > Deepak S <deepak.s@intel.com> wrote:
> > 
> > > 
> > > On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
> > > > On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
> > > >> On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
> > > >>> On Tue, Sep 09, 2014 at 07:14:16PM +0530,
> > > >>> deepak.s@linux.intel.com wrote:
> > > >>>> From: Deepak S <deepak.s@linux.intel.com>
> > > >>>>
> > > >>>> In chv, we have two power wells Render & Media. We need to use
> > > >>>> corresponsing forcewake count. If we dont follow this we are
> > > >>>> getting error "*ERROR*: Timed out waiting for forcewake old ack
> > > >>>> to clear" due to multiple entry into __vlv_force_wake_get.
> > > >>>>
> > > >>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
> > > >>>> ---
> > > >>>>   drivers/gpu/drm/i915/intel_lrc.c | 29
> > > >>>> +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+),
> > > >>>> 4 deletions(-)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > >>>> b/drivers/gpu/drm/i915/intel_lrc.c index bd1b28d..bafd38b 100644
> > > >>>> --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > >>>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > >>>> @@ -300,8 +300,18 @@ static void execlists_elsp_write(struct
> > > >>>> intel_engine_cs *ring,
> > > >>>>   	 * Instead, we do the runtime_pm_get/put when
> > > >>>> creating/destroying requests. */
> > > >>>>   	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > > >>>> -	if (dev_priv->uncore.forcewake_count++ == 0)
> > > >>>> -		dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > >>>> FORCEWAKE_ALL);
> > > >>>> +	if (IS_CHERRYVIEW(dev_priv->dev)) {
> > > >>>> +		if (dev_priv->uncore.fw_rendercount++ == 0)
> > > >>>> +
> > > >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > >>>> +
> > > >>>> FORCEWAKE_RENDER);
> > > >>>> +		if (dev_priv->uncore.fw_mediacount++ == 0)
> > > >>>> +
> > > >>>> dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > > >>>> +
> > > >>>> FORCEWAKE_MEDIA);
> > > >>> This will wake both wells. Is that needed or should we just pick
> > > >>> one based on the ring?
> > > >> Also unlike the comment says runtime_pm_get() can't sleep since
> > > >> someone must already be holding a reference, othwewise we surely
> > > >> can't go writing any registers. So in theory we should be able to
> > > >> call gen6_gt_force_wake_get() here, but maybe that would trigger a
> > > >> might_sleep() warning. the current force wake code duplication
> > > >> (esp. outside intel_uncore.c) is rather unfortunate and I'd like
> > > >> to see it killed off. Maybe we just need to pull the rpm get/put
> > > >> outside gen6_gt_force_wake_get()? I never really liked hiding it
> > > >> there anyway.
> > > > Yeah this is just broken design. And if you look at the other wheel
> > > > to track outstanding gpu work (requests) then it's not needed at
> > > > all.
> > > >
> > > > But I'm not sure what's the priority of the "rework execlists to use
> > > > requests" task is and when (if ever that will happen). Jesse is the
> > > > arbiter for this stuff anyway, so adding him.
> > > > -Daniel
> > > 
> > > hmm , agreed do we have a reworked execlist? The reason why added
> > > this, on chv when i enable execlist, due to incorrect forcewake count
> > > is causing multiple entries to forcewake_get resulting in "*ERROR*:
> > > Timed out waiting for forcewake old ack to clear" "and Hang.
> > 
> > I'm hoping we can get execlists reworked on top of the request/seqno
> > stuff shortly after it lands, but I don't think that's a reason to
> > block this fix, since Chris is still busy fixing up the request
> > changes.
> 
> Queued for -next, thanks for the patch.

Aside if someone gets bored and wants to apply some polish: And __ variant
of force_wake_get/put which only asserts that the device isn't runtime
suspended instead of doing the runtime_pm_get/put would be cute - we have
one other user in the hsw/bdw rpm code already.

The current force_wake_get/put functions would then just wrap those raw
versions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[Chris Wilson]

On Wed, Sep 10, 2014 at 05:47:39PM +0200, Daniel Vetter wrote:
> Aside if someone gets bored and wants to apply some polish: And __ variant
> of force_wake_get/put which only asserts that the device isn't runtime
> suspended instead of doing the runtime_pm_get/put would be cute - we have
> one other user in the hsw/bdw rpm code already.
> 
> The current force_wake_get/put functions would then just wrap those raw
> versions.

I fail to see the reason why they take it at all. Conceptually,
gen6_gt_force_wake_get() is just as lowlevel as the register access they
wrap. The only one that breaks the rule is i915_debugfs.c and that seems
easy enough to wrap with its own intel_runtime_pm_get/_put.

And once you've finish with that you can then sort out the mess that is
the vlv variants. And now skl continues in the cut'n'paste'n'paste vein.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

[S, Deepak]

Hmm Ok Daniel. Let me try and clean up the forcewake. :)

Chris, most of the debugfs is already covered with runtime_get/put. If anything is missed we can add that

Thanks
Deepak

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Wednesday, September 10, 2014 9:21 PM
To: Daniel Vetter
Cc: Jesse Barnes; S, Deepak; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write

On Wed, Sep 10, 2014 at 05:47:39PM +0200, Daniel Vetter wrote:
> Aside if someone gets bored and wants to apply some polish: And __ 
> variant of force_wake_get/put which only asserts that the device isn't 
> runtime suspended instead of doing the runtime_pm_get/put would be 
> cute - we have one other user in the hsw/bdw rpm code already.
> 
> The current force_wake_get/put functions would then just wrap those 
> raw versions.

I fail to see the reason why they take it at all. Conceptually,
gen6_gt_force_wake_get() is just as lowlevel as the register access they wrap. The only one that breaks the rule is i915_debugfs.c and that seems easy enough to wrap with its own intel_runtime_pm_get/_put.

And once you've finish with that you can then sort out the mess that is the vlv variants. And now skl continues in the cut'n'paste'n'paste vein.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre

[PATCH] rpm

[Chris Wilson]

---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 20 ++------------
 drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
 drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
 4 files changed, 27 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 5f35048..a72d8b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
 	if (INTEL_INFO(dev)->gen < 6)
 		return 0;
 
+	intel_runtime_pm_get(dev_priv);
 	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
 
 	return 0;
@@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
 		return 0;
 
 	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+	intel_runtime_pm_put(dev_priv);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 794ad8f..fafd202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
 static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 {
 	uint32_t val;
-	unsigned long irqflags;
 
 	val = I915_READ(LCPLL_CTL);
 
@@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 	/*
 	 * Make sure we're not on PC8 state before disabling PC8, otherwise
 	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
-	 *
-	 * The other problem is that hsw_restore_lcpll() is called as part of
-	 * the runtime PM resume sequence, so we can't just call
-	 * gen6_gt_force_wake_get() because that function calls
-	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
-	 * while we are on the resume sequence. So to solve this problem we have
-	 * to call special forcewake code that doesn't touch runtime PM and
-	 * doesn't enable the forcewake delayed work.
 	 */
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-	if (dev_priv->uncore.forcewake_count++ == 0)
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
 
 	if (val & LCPLL_POWER_DOWN_ALLOW) {
 		val &= ~LCPLL_POWER_DOWN_ALLOW;
@@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
 			DRM_ERROR("Switching back to LCPLL failed\n");
 	}
 
-	/* See the big comment above. */
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-	if (--dev_priv->uncore.forcewake_count == 0)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
 }
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6f1dd00..aeaa1bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
 	struct drm_i915_private *dev_priv = engine->i915;
 	uint64_t tmp;
 	uint32_t desc[4];
-	unsigned long flags;
 
 	/* XXX: You must always write both descriptors in the order below. */
 
@@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
 	desc[1] = upper_32_bits(tmp);
 	desc[0] = lower_32_bits(tmp);
 
-	/* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
-	 * are in progress.
-	 *
-	 * The other problem is that we can't just call gen6_gt_force_wake_get()
-	 * because that function calls intel_runtime_pm_get(), which might sleep.
-	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
-	 */
-	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (dev_priv->uncore.forcewake_count++ == 0)
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
-
+	gen6_gt_force_wake_get(dev_priv, engine->power_domains);
 	I915_WRITE(RING_ELSP(engine), desc[1]);
 	I915_WRITE(RING_ELSP(engine), desc[0]);
 	I915_WRITE(RING_ELSP(engine), desc[3]);
@@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
 
 	/* ELSP is a wo register, so use another nearby reg for posting instead */
 	POSTING_READ(RING_EXECLIST_STATUS(engine));
-
-	/* Release Force Wakeup (see the big comment above). */
-	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
-	if (--dev_priv->uncore.forcewake_count == 0)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
+	gen6_gt_force_wake_put(dev_priv, engine->power_domains);
 }
 
 static u16 next_tag(struct intel_engine_cs *engine)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c99d5ef..3b3d3e0 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -24,6 +24,8 @@
 #include "i915_drv.h"
 #include "intel_drv.h"
 
+#include <linux/pm_runtime.h>
+
 #define FORCEWAKE_ACK_TIMEOUT_MS 2
 
 #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__))
@@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
 
 static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 {
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (fw_engine & FORCEWAKE_RENDER &&
 	    dev_priv->uncore.fw_rendercount++ != 0)
 		fw_engine &= ~FORCEWAKE_RENDER;
@@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 
 	if (fw_engine)
 		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 {
-	unsigned long irqflags;
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-
 	if (fw_engine & FORCEWAKE_RENDER) {
 		WARN_ON(!dev_priv->uncore.fw_rendercount);
 		if (--dev_priv->uncore.fw_rendercount != 0)
@@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 
 	if (fw_engine)
 		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
-
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 static void gen6_force_wake_timer(unsigned long arg)
@@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
 	if (!dev_priv->uncore.funcs.force_wake_get)
 		return;
 
-	intel_runtime_pm_get(dev_priv);
-
-	/* Redirect to VLV specific routine */
-	if (IS_VALLEYVIEW(dev_priv->dev))
-		return vlv_force_wake_get(dev_priv, fw_engine);
+	WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-	if (dev_priv->uncore.forcewake_count++ == 0)
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
+
+	/* Redirect to VLV specific routine */
+	if (IS_VALLEYVIEW(dev_priv->dev)) {
+		vlv_force_wake_get(dev_priv, fw_engine);
+	} else {
+		if (dev_priv->uncore.forcewake_count++ == 0)
+			dev_priv->uncore.funcs.force_wake_get(dev_priv,
+							      FORCEWAKE_ALL);
+	}
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
@@ -428,25 +421,22 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 	if (!dev_priv->uncore.funcs.force_wake_put)
 		return;
 
+	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
+
 	/* Redirect to VLV specific routine */
 	if (IS_VALLEYVIEW(dev_priv->dev)) {
 		vlv_force_wake_put(dev_priv, fw_engine);
-		goto out;
-	}
-
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-	WARN_ON(!dev_priv->uncore.forcewake_count);
-
-	if (--dev_priv->uncore.forcewake_count == 0) {
-		dev_priv->uncore.forcewake_count++;
-		mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
-				 jiffies + 1);
+	} else {
+		WARN_ON(!dev_priv->uncore.forcewake_count);
+		if (--dev_priv->uncore.forcewake_count == 0) {
+			dev_priv->uncore.forcewake_count++;
+			mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
+					 jiffies + 1);
+		}
 	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 
-out:
-	intel_runtime_pm_put(dev_priv);
 }
 
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
-- 
1.9.1

Re: [PATCH] rpm

[Paulo Zanoni]

2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
>  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
>  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
>  4 files changed, 27 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5f35048..a72d8b8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>         if (INTEL_INFO(dev)->gen < 6)
>                 return 0;
>
> +       intel_runtime_pm_get(dev_priv);
>         gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>
>         return 0;
> @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>                 return 0;
>
>         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +       intel_runtime_pm_put(dev_priv);
>
>         return 0;
>  }

Just a minor comment on the chunk above. I didn't look at the rest of the patch.

We used to have exactly the code that you rewrote, but we decided to
remove it because, without it, we can test that gen6_gt_force_wake_get
actually wakes up the HW and keeps it awake until someone calls
gen6_gt_force_wake_put.

If we add these get/put calls above, we won't really detect any
problems related with the actual gen6_gt_force_wake_get/put functions,
so we may hide problems.

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 794ad8f..fafd202 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  {
>         uint32_t val;
> -       unsigned long irqflags;
>
>         val = I915_READ(LCPLL_CTL);
>
> @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>         /*
>          * Make sure we're not on PC8 state before disabling PC8, otherwise
>          * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> -        *
> -        * The other problem is that hsw_restore_lcpll() is called as part of
> -        * the runtime PM resume sequence, so we can't just call
> -        * gen6_gt_force_wake_get() because that function calls
> -        * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> -        * while we are on the resume sequence. So to solve this problem we have
> -        * to call special forcewake code that doesn't touch runtime PM and
> -        * doesn't enable the forcewake delayed work.
>          */
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -       if (dev_priv->uncore.forcewake_count++ == 0)
> -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +       gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>
>         if (val & LCPLL_POWER_DOWN_ALLOW) {
>                 val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>                         DRM_ERROR("Switching back to LCPLL failed\n");
>         }
>
> -       /* See the big comment above. */
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -       if (--dev_priv->uncore.forcewake_count == 0)
> -               dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +       gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>  }
>
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6f1dd00..aeaa1bc 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>         struct drm_i915_private *dev_priv = engine->i915;
>         uint64_t tmp;
>         uint32_t desc[4];
> -       unsigned long flags;
>
>         /* XXX: You must always write both descriptors in the order below. */
>
> @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>         desc[1] = upper_32_bits(tmp);
>         desc[0] = lower_32_bits(tmp);
>
> -       /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
> -        * are in progress.
> -        *
> -        * The other problem is that we can't just call gen6_gt_force_wake_get()
> -        * because that function calls intel_runtime_pm_get(), which might sleep.
> -        * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> -        */
> -       spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -       if (dev_priv->uncore.forcewake_count++ == 0)
> -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> -
> +       gen6_gt_force_wake_get(dev_priv, engine->power_domains);
>         I915_WRITE(RING_ELSP(engine), desc[1]);
>         I915_WRITE(RING_ELSP(engine), desc[0]);
>         I915_WRITE(RING_ELSP(engine), desc[3]);
> @@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>
>         /* ELSP is a wo register, so use another nearby reg for posting instead */
>         POSTING_READ(RING_EXECLIST_STATUS(engine));
> -
> -       /* Release Force Wakeup (see the big comment above). */
> -       spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -       if (--dev_priv->uncore.forcewake_count == 0)
> -               dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> +       gen6_gt_force_wake_put(dev_priv, engine->power_domains);
>  }
>
>  static u16 next_tag(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c99d5ef..3b3d3e0 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -24,6 +24,8 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>
> +#include <linux/pm_runtime.h>
> +
>  #define FORCEWAKE_ACK_TIMEOUT_MS 2
>
>  #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__))
> @@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>
>  static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -       unsigned long irqflags;
> -
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>         if (fw_engine & FORCEWAKE_RENDER &&
>             dev_priv->uncore.fw_rendercount++ != 0)
>                 fw_engine &= ~FORCEWAKE_RENDER;
> @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>
>         if (fw_engine)
>                 dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> -
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>
>  static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -       unsigned long irqflags;
> -
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>         if (fw_engine & FORCEWAKE_RENDER) {
>                 WARN_ON(!dev_priv->uncore.fw_rendercount);
>                 if (--dev_priv->uncore.fw_rendercount != 0)
> @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>
>         if (fw_engine)
>                 dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> -
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>
>  static void gen6_force_wake_timer(unsigned long arg)
> @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>         if (!dev_priv->uncore.funcs.force_wake_get)
>                 return;
>
> -       intel_runtime_pm_get(dev_priv);
> -
> -       /* Redirect to VLV specific routine */
> -       if (IS_VALLEYVIEW(dev_priv->dev))
> -               return vlv_force_wake_get(dev_priv, fw_engine);
> +       WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
>
>         spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -       if (dev_priv->uncore.forcewake_count++ == 0)
> -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> +
> +       /* Redirect to VLV specific routine */
> +       if (IS_VALLEYVIEW(dev_priv->dev)) {
> +               vlv_force_wake_get(dev_priv, fw_engine);
> +       } else {
> +               if (dev_priv->uncore.forcewake_count++ == 0)
> +                       dev_priv->uncore.funcs.force_wake_get(dev_priv,
> +                                                             FORCEWAKE_ALL);
> +       }
>         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>
> @@ -428,25 +421,22 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>         if (!dev_priv->uncore.funcs.force_wake_put)
>                 return;
>
> +       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +
>         /* Redirect to VLV specific routine */
>         if (IS_VALLEYVIEW(dev_priv->dev)) {
>                 vlv_force_wake_put(dev_priv, fw_engine);
> -               goto out;
> -       }
> -
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -       WARN_ON(!dev_priv->uncore.forcewake_count);
> -
> -       if (--dev_priv->uncore.forcewake_count == 0) {
> -               dev_priv->uncore.forcewake_count++;
> -               mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> -                                jiffies + 1);
> +       } else {
> +               WARN_ON(!dev_priv->uncore.forcewake_count);
> +               if (--dev_priv->uncore.forcewake_count == 0) {
> +                       dev_priv->uncore.forcewake_count++;
> +                       mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> +                                        jiffies + 1);
> +               }
>         }
>
>         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>
> -out:
> -       intel_runtime_pm_put(dev_priv);
>  }
>
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] rpm

[Chris Wilson]

On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote:
> 2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
> >  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
> >  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
> >  4 files changed, 27 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5f35048..a72d8b8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
> >         if (INTEL_INFO(dev)->gen < 6)
> >                 return 0;
> >
> > +       intel_runtime_pm_get(dev_priv);
> >         gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >
> >         return 0;
> > @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
> >                 return 0;
> >
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +       intel_runtime_pm_put(dev_priv);
> >
> >         return 0;
> >  }
> 
> Just a minor comment on the chunk above. I didn't look at the rest of the patch.
> 
> We used to have exactly the code that you rewrote, but we decided to
> remove it because, without it, we can test that gen6_gt_force_wake_get
> actually wakes up the HW and keeps it awake until someone calls
> gen6_gt_force_wake_put.

But why? gen6_gt_force_wake_get() should never be called outside of a
rpm context - it is lowlevel register access.
 
> If we add these get/put calls above, we won't really detect any
> problems related with the actual gen6_gt_force_wake_get/put functions,
> so we may hide problems.

See above. That's how the design got broken in the first place and we
have very lowlevel code being copied and pasted throughout the driver.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] rpm

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote:
> 2014-09-10 13:43 GMT-03:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
> >  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
> >  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
> >  4 files changed, 27 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5f35048..a72d8b8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
> >         if (INTEL_INFO(dev)->gen < 6)
> >                 return 0;
> >
> > +       intel_runtime_pm_get(dev_priv);
> >         gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >
> >         return 0;
> > @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
> >                 return 0;
> >
> >         gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> > +       intel_runtime_pm_put(dev_priv);
> >
> >         return 0;
> >  }
> 
> Just a minor comment on the chunk above. I didn't look at the rest of the patch.
> 
> We used to have exactly the code that you rewrote, but we decided to
> remove it because, without it, we can test that gen6_gt_force_wake_get
> actually wakes up the HW and keeps it awake until someone calls
> gen6_gt_force_wake_put.
> 
> If we add these get/put calls above, we won't really detect any
> problems related with the actual gen6_gt_force_wake_get/put functions,
> so we may hide problems.

Well, I think the patch is a great. With the rpm stuff killed
from forcewake handling we could actually use the delayed forcewake put
in the register access macros, which is where it might actually do some
good. IIRC that was even the original intention when the timer got added.
In the current form I'm pretty sure the timer is practically useless.

> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 794ad8f..fafd202 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
> >  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  {
> >         uint32_t val;
> > -       unsigned long irqflags;
> >
> >         val = I915_READ(LCPLL_CTL);
> >
> > @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >         /*
> >          * Make sure we're not on PC8 state before disabling PC8, otherwise
> >          * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> > -        *
> > -        * The other problem is that hsw_restore_lcpll() is called as part of
> > -        * the runtime PM resume sequence, so we can't just call
> > -        * gen6_gt_force_wake_get() because that function calls
> > -        * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> > -        * while we are on the resume sequence. So to solve this problem we have
> > -        * to call special forcewake code that doesn't touch runtime PM and
> > -        * doesn't enable the forcewake delayed work.
> >          */
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -       if (dev_priv->uncore.forcewake_count++ == 0)
> > -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > +       gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> >
> >         if (val & LCPLL_POWER_DOWN_ALLOW) {
> >                 val &= ~LCPLL_POWER_DOWN_ALLOW;
> > @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >                         DRM_ERROR("Switching back to LCPLL failed\n");
> >         }
> >
> > -       /* See the big comment above. */
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -       if (--dev_priv->uncore.forcewake_count == 0)
> > -               dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> > +       gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >  }
> >
> >  /*
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 6f1dd00..aeaa1bc 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
> >         struct drm_i915_private *dev_priv = engine->i915;
> >         uint64_t tmp;
> >         uint32_t desc[4];
> > -       unsigned long flags;
> >
> >         /* XXX: You must always write both descriptors in the order below. */
> >
> > @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
> >         desc[1] = upper_32_bits(tmp);
> >         desc[0] = lower_32_bits(tmp);
> >
> > -       /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
> > -        * are in progress.
> > -        *
> > -        * The other problem is that we can't just call gen6_gt_force_wake_get()
> > -        * because that function calls intel_runtime_pm_get(), which might sleep.
> > -        * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> > -        */
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -       if (dev_priv->uncore.forcewake_count++ == 0)
> > -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> > -
> > +       gen6_gt_force_wake_get(dev_priv, engine->power_domains);
> >         I915_WRITE(RING_ELSP(engine), desc[1]);
> >         I915_WRITE(RING_ELSP(engine), desc[0]);
> >         I915_WRITE(RING_ELSP(engine), desc[3]);
> > @@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
> >
> >         /* ELSP is a wo register, so use another nearby reg for posting instead */
> >         POSTING_READ(RING_EXECLIST_STATUS(engine));
> > -
> > -       /* Release Force Wakeup (see the big comment above). */
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> > -       if (--dev_priv->uncore.forcewake_count == 0)
> > -               dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> > +       gen6_gt_force_wake_put(dev_priv, engine->power_domains);
> >  }
> >
> >  static u16 next_tag(struct intel_engine_cs *engine)
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index c99d5ef..3b3d3e0 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -24,6 +24,8 @@
> >  #include "i915_drv.h"
> >  #include "intel_drv.h"
> >
> > +#include <linux/pm_runtime.h>
> > +
> >  #define FORCEWAKE_ACK_TIMEOUT_MS 2
> >
> >  #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__))
> > @@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
> >
> >  static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> >  {
> > -       unsigned long irqflags;
> > -
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -
> >         if (fw_engine & FORCEWAKE_RENDER &&
> >             dev_priv->uncore.fw_rendercount++ != 0)
> >                 fw_engine &= ~FORCEWAKE_RENDER;
> > @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> >
> >         if (fw_engine)
> >                 dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> > -
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >
> >  static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> >  {
> > -       unsigned long irqflags;
> > -
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -
> >         if (fw_engine & FORCEWAKE_RENDER) {
> >                 WARN_ON(!dev_priv->uncore.fw_rendercount);
> >                 if (--dev_priv->uncore.fw_rendercount != 0)
> > @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> >
> >         if (fw_engine)
> >                 dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> > -
> > -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >
> >  static void gen6_force_wake_timer(unsigned long arg)
> > @@ -406,15 +396,18 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> >         if (!dev_priv->uncore.funcs.force_wake_get)
> >                 return;
> >
> > -       intel_runtime_pm_get(dev_priv);
> > -
> > -       /* Redirect to VLV specific routine */
> > -       if (IS_VALLEYVIEW(dev_priv->dev))
> > -               return vlv_force_wake_get(dev_priv, fw_engine);
> > +       WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
> >
> >         spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -       if (dev_priv->uncore.forcewake_count++ == 0)
> > -               dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +
> > +       /* Redirect to VLV specific routine */
> > +       if (IS_VALLEYVIEW(dev_priv->dev)) {
> > +               vlv_force_wake_get(dev_priv, fw_engine);
> > +       } else {
> > +               if (dev_priv->uncore.forcewake_count++ == 0)
> > +                       dev_priv->uncore.funcs.force_wake_get(dev_priv,
> > +                                                             FORCEWAKE_ALL);
> > +       }
> >         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >  }
> >
> > @@ -428,25 +421,22 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> >         if (!dev_priv->uncore.funcs.force_wake_put)
> >                 return;
> >
> > +       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > +
> >         /* Redirect to VLV specific routine */
> >         if (IS_VALLEYVIEW(dev_priv->dev)) {
> >                 vlv_force_wake_put(dev_priv, fw_engine);
> > -               goto out;
> > -       }
> > -
> > -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > -       WARN_ON(!dev_priv->uncore.forcewake_count);
> > -
> > -       if (--dev_priv->uncore.forcewake_count == 0) {
> > -               dev_priv->uncore.forcewake_count++;
> > -               mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> > -                                jiffies + 1);
> > +       } else {
> > +               WARN_ON(!dev_priv->uncore.forcewake_count);
> > +               if (--dev_priv->uncore.forcewake_count == 0) {
> > +                       dev_priv->uncore.forcewake_count++;
> > +                       mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> > +                                        jiffies + 1);
> > +               }
> >         }
> >
> >         spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> >
> > -out:
> > -       intel_runtime_pm_put(dev_priv);
> >  }
> >
> >  void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] rpm

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote:
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 20 ++------------
>  drivers/gpu/drm/i915/intel_lrc.c     | 21 ++-------------
>  drivers/gpu/drm/i915/intel_uncore.c  | 52 +++++++++++++++---------------------
>  4 files changed, 27 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5f35048..a72d8b8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
>  	if (INTEL_INFO(dev)->gen < 6)
>  		return 0;
>  
> +	intel_runtime_pm_get(dev_priv);
>  	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	return 0;
> @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
>  		return 0;
>  
>  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> +	intel_runtime_pm_put(dev_priv);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 794ad8f..fafd202 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private *dev_priv,
>  static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  {
>  	uint32_t val;
> -	unsigned long irqflags;
>  
>  	val = I915_READ(LCPLL_CTL);
>  
> @@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  	/*
>  	 * Make sure we're not on PC8 state before disabling PC8, otherwise
>  	 * we'll hang the machine. To prevent PC8 state, just enable force_wake.
> -	 *
> -	 * The other problem is that hsw_restore_lcpll() is called as part of
> -	 * the runtime PM resume sequence, so we can't just call
> -	 * gen6_gt_force_wake_get() because that function calls
> -	 * intel_runtime_pm_get(), and we can't change the runtime PM refcount
> -	 * while we are on the resume sequence. So to solve this problem we have
> -	 * to call special forcewake code that doesn't touch runtime PM and
> -	 * doesn't enable the forcewake delayed work.
>  	 */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	if (dev_priv->uncore.forcewake_count++ == 0)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
>  
>  	if (val & LCPLL_POWER_DOWN_ALLOW) {
>  		val &= ~LCPLL_POWER_DOWN_ALLOW;
> @@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
>  			DRM_ERROR("Switching back to LCPLL failed\n");
>  	}
>  
> -	/* See the big comment above. */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
>  }
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 6f1dd00..aeaa1bc 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  	struct drm_i915_private *dev_priv = engine->i915;
>  	uint64_t tmp;
>  	uint32_t desc[4];
> -	unsigned long flags;
>  
>  	/* XXX: You must always write both descriptors in the order below. */
>  
> @@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  	desc[1] = upper_32_bits(tmp);
>  	desc[0] = lower_32_bits(tmp);
>  
> -	/* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
> -	 * are in progress.
> -	 *
> -	 * The other problem is that we can't just call gen6_gt_force_wake_get()
> -	 * because that function calls intel_runtime_pm_get(), which might sleep.
> -	 * Instead, we do the runtime_pm_get/put when creating/destroying requests.
> -	 */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (dev_priv->uncore.forcewake_count++ == 0)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> -
> +	gen6_gt_force_wake_get(dev_priv, engine->power_domains);
>  	I915_WRITE(RING_ELSP(engine), desc[1]);
>  	I915_WRITE(RING_ELSP(engine), desc[0]);
>  	I915_WRITE(RING_ELSP(engine), desc[3]);
> @@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs *engine,
>  
>  	/* ELSP is a wo register, so use another nearby reg for posting instead */
>  	POSTING_READ(RING_EXECLIST_STATUS(engine));
> -
> -	/* Release Force Wakeup (see the big comment above). */
> -	spin_lock_irqsave(&dev_priv->uncore.lock, flags);
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, flags);
> +	gen6_gt_force_wake_put(dev_priv, engine->power_domains);
>  }
>  
>  static u16 next_tag(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c99d5ef..3b3d3e0 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -24,6 +24,8 @@
>  #include "i915_drv.h"
>  #include "intel_drv.h"
>  
> +#include <linux/pm_runtime.h>
> +
>  #define FORCEWAKE_ACK_TIMEOUT_MS 2
>  
>  #define __raw_i915_read8(dev_priv__, reg__) readb((dev_priv__)->regs + (reg__))
> @@ -258,10 +260,6 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  
>  static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -	unsigned long irqflags;
> -
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	if (fw_engine & FORCEWAKE_RENDER &&
>  	    dev_priv->uncore.fw_rendercount++ != 0)
>  		fw_engine &= ~FORCEWAKE_RENDER;
> @@ -271,16 +269,10 @@ static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>  
>  	if (fw_engine)
>  		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  {
> -	unsigned long irqflags;
> -
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -
>  	if (fw_engine & FORCEWAKE_RENDER) {
>  		WARN_ON(!dev_priv->uncore.fw_rendercount);
>  		if (--dev_priv->uncore.fw_rendercount != 0)
> @@ -295,8 +287,6 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  
>  	if (fw_engine)
>  		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> -
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  static void gen6_force_wake_timer(unsigned long arg)

Looks like you forgot to kill the rpm_put() from the timer. And then we
also need to make sure the timer is approriately cancelled when
runtime suspending the device.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] rpm

[Chris Wilson]

On Wed, Sep 10, 2014 at 08:15:47PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote:
> >  static void gen6_force_wake_timer(unsigned long arg)
> 
> Looks like you forgot to kill the rpm_put() from the timer. And then we
> also need to make sure the timer is approriately cancelled when
> runtime suspending the device.

Or maybe I have that already fixed in my tree so you don't see it in
this patch ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: Reduce duplicated forcewake logic

[Chris Wilson]

Introduce a structure to track the individual forcewake domains and use
that to eliminated duplicate logic.
---
 drivers/gpu/drm/i915/i915_debugfs.c |  41 +++---
 drivers/gpu/drm/i915/i915_drv.h     |  32 +++--
 drivers/gpu/drm/i915/intel_uncore.c | 265 ++++++++++++++++--------------------
 3 files changed, 157 insertions(+), 181 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a72d8b8..c3176f1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1317,7 +1317,6 @@ static int vlv_drpc_info(struct seq_file *m)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 rpmodectl1, rcctl1;
-	unsigned fw_rendercount = 0, fw_mediacount = 0;
 
 	intel_runtime_pm_get(dev_priv);
 
@@ -1350,22 +1349,12 @@ static int vlv_drpc_info(struct seq_file *m)
 	seq_printf(m, "Media RC6 residency since boot: %u\n",
 		   I915_READ(VLV_GT_MEDIA_RC6));
 
-	spin_lock_irq(&dev_priv->uncore.lock);
-	fw_rendercount = dev_priv->uncore.fw_rendercount;
-	fw_mediacount = dev_priv->uncore.fw_mediacount;
-	spin_unlock_irq(&dev_priv->uncore.lock);
-
-	seq_printf(m, "Forcewake Render Count = %u\n", fw_rendercount);
-	seq_printf(m, "Forcewake Media Count = %u\n", fw_mediacount);
-
-
-	return 0;
+	return i915_gen6_forcewake_count_info(m, data);
 }
 
 
 static int gen6_drpc_info(struct seq_file *m)
 {
-
 	struct drm_info_node *node = m->private;
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1379,7 +1368,7 @@ static int gen6_drpc_info(struct seq_file *m)
 	intel_runtime_pm_get(dev_priv);
 
 	spin_lock_irq(&dev_priv->uncore.lock);
-	forcewake_count = dev_priv->uncore.forcewake_count;
+	forcewake_count = dev_priv->uncore.fw_domain[FW_DOMAIN_RENDER].wake_count;
 	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	if (forcewake_count) {
@@ -1976,21 +1965,23 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
 	struct drm_info_node *node = m->private;
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	unsigned forcewake_count = 0, fw_rendercount = 0, fw_mediacount = 0;
+	const char *domain_names[] = {
+		"render",
+		"media",
+	};
+	int i;
 
 	spin_lock_irq(&dev_priv->uncore.lock);
-	if (IS_VALLEYVIEW(dev)) {
-		fw_rendercount = dev_priv->uncore.fw_rendercount;
-		fw_mediacount = dev_priv->uncore.fw_mediacount;
-	} else
-		forcewake_count = dev_priv->uncore.forcewake_count;
-	spin_unlock_irq(&dev_priv->uncore.lock);
+	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
+			continue;
 
-	if (IS_VALLEYVIEW(dev)) {
-		seq_printf(m, "fw_rendercount = %u\n", fw_rendercount);
-		seq_printf(m, "fw_mediacount = %u\n", fw_mediacount);
-	} else
-		seq_printf(m, "forcewake count = %u\n", forcewake_count);
+		seq_printf(m, "%s.wake_count = %u\n",
+			   domain_names[i],
+			   dev_priv->uncore.fw_domain[i].wake_count);
+	}
+
+	spin_unlock_irq(&dev_priv->uncore.lock);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5d0b38c..6424191 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -528,18 +528,30 @@ struct intel_uncore_funcs {
 				uint64_t val, bool trace);
 };
 
+enum {
+	FW_DOMAIN_RENDER = 0,
+	FW_DOMAIN_MEDIA,
+
+	FW_DOMAIN_COUNT
+};
+
 struct intel_uncore {
 	spinlock_t lock; /** lock is also taken in irq contexts. */
 
 	struct intel_uncore_funcs funcs;
 
 	unsigned fifo_count;
-	unsigned forcewake_count;
-
-	unsigned fw_rendercount;
-	unsigned fw_mediacount;
+	unsigned fw_domains;
 
-	struct timer_list force_wake_timer;
+	struct intel_uncore_forcewake_domain {
+		struct drm_i915_private *i915;
+		int id;
+		unsigned wake_count;
+		struct timer_list timer;
+	} fw_domain[FW_DOMAIN_COUNT];
+#define FORCEWAKE_RENDER	(1 << FW_DOMAIN_RENDER)
+#define FORCEWAKE_MEDIA		(1 << FW_DOMAIN_MEDIA)
+#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
 };
 
 #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
@@ -2948,8 +2960,10 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
  * must be set to prevent GT core from power down and stale values being
  * returned.
  */
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
-void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
+			    unsigned fw_domains);
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
+			    unsigned fw_domains);
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
 
 int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
@@ -2981,10 +2995,6 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
 int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 
-#define FORCEWAKE_RENDER	(1 << 0)
-#define FORCEWAKE_MEDIA		(1 << 1)
-#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
-
 
 #define I915_READ8(reg)		dev_priv->uncore.funcs.mmio_readb(dev_priv, (reg), true)
 #define I915_WRITE8(reg, val)	dev_priv->uncore.funcs.mmio_writeb(dev_priv, (reg), (val), true)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3b3d3e0..641950b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -73,7 +73,7 @@ static void __gen6_gt_force_wake_reset(struct drm_i915_private *dev_priv)
 }
 
 static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
-							int fw_engine)
+				     int fw_engine)
 {
 	if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK) & 1) == 0,
 			    FORCEWAKE_ACK_TIMEOUT_MS))
@@ -99,7 +99,7 @@ static void __gen7_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
 }
 
 static void __gen7_gt_force_wake_mt_get(struct drm_i915_private *dev_priv,
-							int fw_engine)
+					int fw_engine)
 {
 	u32 forcewake_ack;
 
@@ -136,7 +136,7 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
 }
 
 static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
-							int fw_engine)
+				     int fw_engine)
 {
 	__raw_i915_write32(dev_priv, FORCEWAKE, 0);
 	/* something from same cacheline, but !FORCEWAKE */
@@ -145,7 +145,7 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
 }
 
 static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
-							int fw_engine)
+					int fw_engine)
 {
 	__raw_i915_write32(dev_priv, FORCEWAKE_MT,
 			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
@@ -194,7 +194,7 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
 }
 
 static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
-						int fw_engine)
+				 int fw_engine)
 {
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine) {
@@ -238,9 +238,8 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
 }
 
 static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
-					int fw_engine)
+				 int fw_engine)
 {
-
 	/* Check for Render Engine */
 	if (FORCEWAKE_RENDER & fw_engine)
 		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
@@ -258,59 +257,35 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
 		gen6_gt_check_fifodbg(dev_priv);
 }
 
-static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
-{
-	if (fw_engine & FORCEWAKE_RENDER &&
-	    dev_priv->uncore.fw_rendercount++ != 0)
-		fw_engine &= ~FORCEWAKE_RENDER;
-	if (fw_engine & FORCEWAKE_MEDIA &&
-	    dev_priv->uncore.fw_mediacount++ != 0)
-		fw_engine &= ~FORCEWAKE_MEDIA;
-
-	if (fw_engine)
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
-}
-
-static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
-{
-	if (fw_engine & FORCEWAKE_RENDER) {
-		WARN_ON(!dev_priv->uncore.fw_rendercount);
-		if (--dev_priv->uncore.fw_rendercount != 0)
-			fw_engine &= ~FORCEWAKE_RENDER;
-	}
-
-	if (fw_engine & FORCEWAKE_MEDIA) {
-		WARN_ON(!dev_priv->uncore.fw_mediacount);
-		if (--dev_priv->uncore.fw_mediacount != 0)
-			fw_engine &= ~FORCEWAKE_MEDIA;
-	}
-
-	if (fw_engine)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
-}
-
 static void gen6_force_wake_timer(unsigned long arg)
 {
-	struct drm_i915_private *dev_priv = (void *)arg;
+	struct intel_uncore_forcewake_domain *domain = (void *)arg;
 	unsigned long irqflags;
 
-	assert_device_not_suspended(dev_priv);
+	assert_device_not_suspended(domain->i915);
 
-	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
-	WARN_ON(!dev_priv->uncore.forcewake_count);
+	spin_lock_irqsave(&domain->i915->uncore.lock, irqflags);
+	WARN_ON(!domain->wake_count);
 
-	if (--dev_priv->uncore.forcewake_count == 0)
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
-	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
+	if (--domain->wake_count == 0)
+		domain->i915->uncore.funcs.force_wake_put(domain->i915,
+							  1 << domain->id);
+	spin_unlock_irqrestore(&domain->i915->uncore.lock, irqflags);
 }
 
 void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
+	int i;
 
-	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
-		gen6_force_wake_timer((unsigned long)dev_priv);
+	for (i = 0; i < FW_DOMAIN_COUNT; i++ ) {
+		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
+			continue;
+
+		if (del_timer_sync(&dev_priv->uncore.fw_domain[i].timer))
+			gen6_force_wake_timer((unsigned long)&dev_priv->uncore.fw_domain[i]);
+	}
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
@@ -327,18 +302,12 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 
 	if (restore) { /* If reset with a user forcewake, try to restore */
 		unsigned fw = 0;
+		int i;
 
-		if (IS_VALLEYVIEW(dev)) {
-			if (dev_priv->uncore.fw_rendercount)
-				fw |= FORCEWAKE_RENDER;
-
-			if (dev_priv->uncore.fw_mediacount)
-				fw |= FORCEWAKE_MEDIA;
-		} else {
-			if (dev_priv->uncore.forcewake_count)
-				fw = FORCEWAKE_ALL;
+		for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+			if (dev_priv->uncore.fw_domain[i].wake_count)
+				fw |= 1 << i;
 		}
-
 		if (fw)
 			dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
 
@@ -389,50 +358,60 @@ void intel_uncore_sanitize(struct drm_device *dev)
  * be called at the beginning of the sequence followed by a call to
  * gen6_gt_force_wake_put() at the end of the sequence.
  */
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
+void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
+			    unsigned fw_domains)
 {
 	unsigned long irqflags;
+	int i;
 
 	if (!dev_priv->uncore.funcs.force_wake_get)
 		return;
 
 	WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
 
+	fw_domains &= dev_priv->uncore.fw_domains;
+
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	/* Redirect to VLV specific routine */
-	if (IS_VALLEYVIEW(dev_priv->dev)) {
-		vlv_force_wake_get(dev_priv, fw_engine);
-	} else {
-		if (dev_priv->uncore.forcewake_count++ == 0)
-			dev_priv->uncore.funcs.force_wake_get(dev_priv,
-							      FORCEWAKE_ALL);
+	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+		if ((fw_domains & (1 << i)) == 0)
+			continue;
+
+		if (dev_priv->uncore.fw_domain[i].wake_count++)
+			fw_domains &= ~(1 << i);
 	}
+
+	if (fw_domains)
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
+
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
 }
 
 /*
  * see gen6_gt_force_wake_get()
  */
-void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
+void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
+			    unsigned fw_domains)
 {
 	unsigned long irqflags;
+	int i;
 
 	if (!dev_priv->uncore.funcs.force_wake_put)
 		return;
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	/* Redirect to VLV specific routine */
-	if (IS_VALLEYVIEW(dev_priv->dev)) {
-		vlv_force_wake_put(dev_priv, fw_engine);
-	} else {
-		WARN_ON(!dev_priv->uncore.forcewake_count);
-		if (--dev_priv->uncore.forcewake_count == 0) {
-			dev_priv->uncore.forcewake_count++;
-			mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
-					 jiffies + 1);
-		}
+	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+		if ((fw_domains & (1 << i)) == 0)
+			continue;
+
+		WARN_ON(!dev_priv->uncore.fw_domain[i].wake_count);
+		if (--dev_priv->uncore.fw_domain[i].wake_count)
+			continue;
+
+		dev_priv->uncore.fw_domain[i].wake_count++;
+		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
+				 jiffies + 1);
 	}
 
 	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
@@ -441,10 +420,13 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
 
 void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
 {
+	int i;
+
 	if (!dev_priv->uncore.funcs.force_wake_get)
 		return;
 
-	WARN_ON(dev_priv->uncore.forcewake_count > 0);
+	for (i = 0; i < FW_DOMAIN_COUNT; i++)
+		WARN_ON(dev_priv->uncore.fw_domain[i].wake_count > 0);
 }
 
 /* We give fast paths for the really cool registers */
@@ -578,19 +560,38 @@ __gen2_read(64)
 	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
 	return val
 
+static inline void __force_wake_get(struct drm_i915_private *dev_priv,
+				    unsigned fw_domains)
+{
+	int i;
+
+	/* Ideally GCC would be constant-fold and eliminate this loop */
+
+	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+		if ((fw_domains & (1 << i)) == 0)
+			continue;
+
+		if (dev_priv->uncore.fw_domain[i].wake_count) {
+			fw_domains &= ~(1 << i);
+			continue;
+		}
+
+		dev_priv->uncore.fw_domain[i].wake_count++;
+		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
+				 jiffies + 1);
+	}
+
+	if (fw_domains)
+		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
+}
+
 #define __gen6_read(x) \
 static u##x \
 gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 	GEN6_READ_HEADER(x); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
-	if (dev_priv->uncore.forcewake_count == 0 && \
-	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
-						      FORCEWAKE_ALL); \
-		dev_priv->uncore.forcewake_count++; \
-		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
-				 jiffies + 1); \
-	} \
+	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) \
+		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
 	val = __raw_i915_read##x(dev_priv, reg); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
 	GEN6_READ_FOOTER; \
@@ -599,45 +600,27 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
 #define __vlv_read(x) \
 static u##x \
 vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
-	unsigned fwengine = 0; \
 	GEN6_READ_HEADER(x); \
-	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) { \
-		if (dev_priv->uncore.fw_rendercount == 0) \
-			fwengine = FORCEWAKE_RENDER; \
-	} else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) { \
-		if (dev_priv->uncore.fw_mediacount == 0) \
-			fwengine = FORCEWAKE_MEDIA; \
-	}  \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
+	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) \
+		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) \
+		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
 	val = __raw_i915_read##x(dev_priv, reg); \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
 	GEN6_READ_FOOTER; \
 }
 
 #define __chv_read(x) \
 static u##x \
 chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
-	unsigned fwengine = 0; \
 	GEN6_READ_HEADER(x); \
-	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
-		if (dev_priv->uncore.fw_rendercount == 0) \
-			fwengine = FORCEWAKE_RENDER; \
-	} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
-		if (dev_priv->uncore.fw_mediacount == 0) \
-			fwengine = FORCEWAKE_MEDIA; \
-	} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
-		if (dev_priv->uncore.fw_rendercount == 0) \
-			fwengine |= FORCEWAKE_RENDER; \
-		if (dev_priv->uncore.fw_mediacount == 0) \
-			fwengine |= FORCEWAKE_MEDIA; \
-	} \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
+	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
+		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
+		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
+	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
+		__force_wake_get(dev_priv, \
+				 FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
 	val = __raw_i915_read##x(dev_priv, reg); \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
 	GEN6_READ_FOOTER; \
 }
 
@@ -766,17 +749,9 @@ static void \
 gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
 	GEN6_WRITE_HEADER; \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
-	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
-							      FORCEWAKE_ALL); \
-		__raw_i915_write##x(dev_priv, reg, val); \
-		if (dev_priv->uncore.forcewake_count == 0) \
-			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
-							      FORCEWAKE_ALL); \
-	} else { \
-		__raw_i915_write##x(dev_priv, reg, val); \
-	} \
+	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) \
+		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+	__raw_i915_write##x(dev_priv, reg, val); \
 	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
 	hsw_unclaimed_reg_detect(dev_priv); \
 	GEN6_WRITE_FOOTER; \
@@ -785,28 +760,17 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
 #define __chv_write(x) \
 static void \
 chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
-	unsigned fwengine = 0; \
 	bool shadowed = is_gen8_shadowed(dev_priv, reg); \
 	GEN6_WRITE_HEADER; \
 	if (!shadowed) { \
-		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
-			if (dev_priv->uncore.fw_rendercount == 0) \
-				fwengine = FORCEWAKE_RENDER; \
-		} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
-			if (dev_priv->uncore.fw_mediacount == 0) \
-				fwengine = FORCEWAKE_MEDIA; \
-		} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
-			if (dev_priv->uncore.fw_rendercount == 0) \
-				fwengine |= FORCEWAKE_RENDER; \
-			if (dev_priv->uncore.fw_mediacount == 0) \
-				fwengine |= FORCEWAKE_MEDIA; \
-		} \
+		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
+			__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
+		else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
+			__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
+		else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
+			__force_wake_get(dev_priv, FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
 	} \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
 	__raw_i915_write##x(dev_priv, reg, val); \
-	if (fwengine) \
-		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
 	GEN6_WRITE_FOOTER; \
 }
 
@@ -837,18 +801,18 @@ __gen6_write(64)
 void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	setup_timer(&dev_priv->uncore.force_wake_timer,
-		    gen6_force_wake_timer, (unsigned long)dev_priv);
+	int i;
 
 	intel_uncore_early_sanitize(dev, false);
 
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
+		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER | FORCEWAKE_MEDIA;
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __gen7_gt_force_wake_mt_get;
 		dev_priv->uncore.funcs.force_wake_put = __gen7_gt_force_wake_mt_put;
+		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
 	} else if (IS_IVYBRIDGE(dev)) {
 		u32 ecobus;
 
@@ -880,11 +844,22 @@ void intel_uncore_init(struct drm_device *dev)
 			dev_priv->uncore.funcs.force_wake_put =
 				__gen6_gt_force_wake_put;
 		}
+		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
 	} else if (IS_GEN6(dev)) {
 		dev_priv->uncore.funcs.force_wake_get =
 			__gen6_gt_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put =
 			__gen6_gt_force_wake_put;
+		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
+	}
+
+	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
+		dev_priv->uncore.fw_domain[i].i915 = dev_priv;
+		dev_priv->uncore.fw_domain[i].id = i;
+
+		setup_timer(&dev_priv->uncore.fw_domain[i].timer,
+			    gen6_force_wake_timer,
+			    (unsigned long)&dev_priv->uncore.fw_domain[i]);
 	}
 
 	switch (INTEL_INFO(dev)->gen) {
-- 
1.9.1

Re: [PATCH] drm/i915: Reduce duplicated forcewake logic

[Tvrtko Ursulin]

On 09/10/2014 07:34 PM, Chris Wilson wrote:
> Introduce a structure to track the individual forcewake domains and use
> that to eliminated duplicate logic.
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c |  41 +++---
>   drivers/gpu/drm/i915/i915_drv.h     |  32 +++--
>   drivers/gpu/drm/i915/intel_uncore.c | 265 ++++++++++++++++--------------------
>   3 files changed, 157 insertions(+), 181 deletions(-)

As said yesterday I like the idea, some comments and questions inline.

> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a72d8b8..c3176f1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1317,7 +1317,6 @@ static int vlv_drpc_info(struct seq_file *m)
>   	struct drm_device *dev = node->minor->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	u32 rpmodectl1, rcctl1;
> -	unsigned fw_rendercount = 0, fw_mediacount = 0;
>
>   	intel_runtime_pm_get(dev_priv);
>
> @@ -1350,22 +1349,12 @@ static int vlv_drpc_info(struct seq_file *m)
>   	seq_printf(m, "Media RC6 residency since boot: %u\n",
>   		   I915_READ(VLV_GT_MEDIA_RC6));
>
> -	spin_lock_irq(&dev_priv->uncore.lock);
> -	fw_rendercount = dev_priv->uncore.fw_rendercount;
> -	fw_mediacount = dev_priv->uncore.fw_mediacount;
> -	spin_unlock_irq(&dev_priv->uncore.lock);
> -
> -	seq_printf(m, "Forcewake Render Count = %u\n", fw_rendercount);
> -	seq_printf(m, "Forcewake Media Count = %u\n", fw_mediacount);
> -
> -
> -	return 0;
> +	return i915_gen6_forcewake_count_info(m, data);
>   }
>
>
>   static int gen6_drpc_info(struct seq_file *m)
>   {
> -
>   	struct drm_info_node *node = m->private;
>   	struct drm_device *dev = node->minor->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1379,7 +1368,7 @@ static int gen6_drpc_info(struct seq_file *m)
>   	intel_runtime_pm_get(dev_priv);
>
>   	spin_lock_irq(&dev_priv->uncore.lock);
> -	forcewake_count = dev_priv->uncore.forcewake_count;
> +	forcewake_count = dev_priv->uncore.fw_domain[FW_DOMAIN_RENDER].wake_count;
>   	spin_unlock_irq(&dev_priv->uncore.lock);
>
>   	if (forcewake_count) {
> @@ -1976,21 +1965,23 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
>   	struct drm_info_node *node = m->private;
>   	struct drm_device *dev = node->minor->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned forcewake_count = 0, fw_rendercount = 0, fw_mediacount = 0;
> +	const char *domain_names[] = {
> +		"render",
> +		"media",
> +	};
> +	int i;
>
>   	spin_lock_irq(&dev_priv->uncore.lock);
> -	if (IS_VALLEYVIEW(dev)) {
> -		fw_rendercount = dev_priv->uncore.fw_rendercount;
> -		fw_mediacount = dev_priv->uncore.fw_mediacount;
> -	} else
> -		forcewake_count = dev_priv->uncore.forcewake_count;
> -	spin_unlock_irq(&dev_priv->uncore.lock);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
> +			continue;

What do you think about something like for_each_forcewake_domain (and 
perhaps for_each_possible_forcewake_domain) for readability since there 
is a fair number of these loops?

> -	if (IS_VALLEYVIEW(dev)) {
> -		seq_printf(m, "fw_rendercount = %u\n", fw_rendercount);
> -		seq_printf(m, "fw_mediacount = %u\n", fw_mediacount);
> -	} else
> -		seq_printf(m, "forcewake count = %u\n", forcewake_count);
> +		seq_printf(m, "%s.wake_count = %u\n",
> +			   domain_names[i],
> +			   dev_priv->uncore.fw_domain[i].wake_count);
> +	}
> +
> +	spin_unlock_irq(&dev_priv->uncore.lock);
>
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5d0b38c..6424191 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -528,18 +528,30 @@ struct intel_uncore_funcs {
>   				uint64_t val, bool trace);
>   };
>
> +enum {
> +	FW_DOMAIN_RENDER = 0,
> +	FW_DOMAIN_MEDIA,
> +
> +	FW_DOMAIN_COUNT
> +};

Would there be any need to attempt hiding these somehow so people don't 
try to pass these instead of FORCEWAKE_... ones into relevant functions? 
No idea how though. Maybe gcc will complain if passing in enum into int?

> +
>   struct intel_uncore {
>   	spinlock_t lock; /** lock is also taken in irq contexts. */
>
>   	struct intel_uncore_funcs funcs;
>
>   	unsigned fifo_count;
> -	unsigned forcewake_count;
> -
> -	unsigned fw_rendercount;
> -	unsigned fw_mediacount;
> +	unsigned fw_domains;
>
> -	struct timer_list force_wake_timer;
> +	struct intel_uncore_forcewake_domain {
> +		struct drm_i915_private *i915;

Should this be named dev_priv for consistency?

> +		int id;
> +		unsigned wake_count;
> +		struct timer_list timer;
> +	} fw_domain[FW_DOMAIN_COUNT];
> +#define FORCEWAKE_RENDER	(1 << FW_DOMAIN_RENDER)
> +#define FORCEWAKE_MEDIA		(1 << FW_DOMAIN_MEDIA)
> +#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
>   };
>
>   #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> @@ -2948,8 +2960,10 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
>    * must be set to prevent GT core from power down and stale values being
>    * returned.
>    */
> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
> -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains);
> +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains);
>   void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>
>   int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> @@ -2981,10 +2995,6 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>   int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
>   int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
>
> -#define FORCEWAKE_RENDER	(1 << 0)
> -#define FORCEWAKE_MEDIA		(1 << 1)
> -#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
> -
>
>   #define I915_READ8(reg)		dev_priv->uncore.funcs.mmio_readb(dev_priv, (reg), true)
>   #define I915_WRITE8(reg, val)	dev_priv->uncore.funcs.mmio_writeb(dev_priv, (reg), (val), true)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 3b3d3e0..641950b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -73,7 +73,7 @@ static void __gen6_gt_force_wake_reset(struct drm_i915_private *dev_priv)
>   }
>
>   static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +				     int fw_engine)
>   {
>   	if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK) & 1) == 0,
>   			    FORCEWAKE_ACK_TIMEOUT_MS))
> @@ -99,7 +99,7 @@ static void __gen7_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
>   }
>
>   static void __gen7_gt_force_wake_mt_get(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +					int fw_engine)
>   {
>   	u32 forcewake_ack;
>
> @@ -136,7 +136,7 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>   }
>
>   static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +				     int fw_engine)
>   {
>   	__raw_i915_write32(dev_priv, FORCEWAKE, 0);
>   	/* something from same cacheline, but !FORCEWAKE */
> @@ -145,7 +145,7 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
>   }
>
>   static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +					int fw_engine)
>   {
>   	__raw_i915_write32(dev_priv, FORCEWAKE_MT,
>   			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
> @@ -194,7 +194,7 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
>   }
>
>   static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
> -						int fw_engine)
> +				 int fw_engine)
>   {
>   	/* Check for Render Engine */
>   	if (FORCEWAKE_RENDER & fw_engine) {
> @@ -238,9 +238,8 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>   }
>
>   static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
> -					int fw_engine)
> +				 int fw_engine)
>   {

What's up with the above series of indentation changes?

> -
>   	/* Check for Render Engine */
>   	if (FORCEWAKE_RENDER & fw_engine)
>   		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
> @@ -258,59 +257,35 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>   		gen6_gt_check_fifodbg(dev_priv);
>   }
>
> -static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> -{
> -	if (fw_engine & FORCEWAKE_RENDER &&
> -	    dev_priv->uncore.fw_rendercount++ != 0)
> -		fw_engine &= ~FORCEWAKE_RENDER;
> -	if (fw_engine & FORCEWAKE_MEDIA &&
> -	    dev_priv->uncore.fw_mediacount++ != 0)
> -		fw_engine &= ~FORCEWAKE_MEDIA;
> -
> -	if (fw_engine)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> -}
> -
> -static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> -{
> -	if (fw_engine & FORCEWAKE_RENDER) {
> -		WARN_ON(!dev_priv->uncore.fw_rendercount);
> -		if (--dev_priv->uncore.fw_rendercount != 0)
> -			fw_engine &= ~FORCEWAKE_RENDER;
> -	}
> -
> -	if (fw_engine & FORCEWAKE_MEDIA) {
> -		WARN_ON(!dev_priv->uncore.fw_mediacount);
> -		if (--dev_priv->uncore.fw_mediacount != 0)
> -			fw_engine &= ~FORCEWAKE_MEDIA;
> -	}
> -
> -	if (fw_engine)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> -}
> -
>   static void gen6_force_wake_timer(unsigned long arg)
>   {
> -	struct drm_i915_private *dev_priv = (void *)arg;
> +	struct intel_uncore_forcewake_domain *domain = (void *)arg;
>   	unsigned long irqflags;
>
> -	assert_device_not_suspended(dev_priv);
> +	assert_device_not_suspended(domain->i915);
>
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	WARN_ON(!dev_priv->uncore.forcewake_count);
> +	spin_lock_irqsave(&domain->i915->uncore.lock, irqflags);
> +	WARN_ON(!domain->wake_count);
>
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	if (--domain->wake_count == 0)
> +		domain->i915->uncore.funcs.force_wake_put(domain->i915,
> +							  1 << domain->id);
> +	spin_unlock_irqrestore(&domain->i915->uncore.lock, irqflags);
>   }
>
>   void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	unsigned long irqflags;
> +	int i;
>
> -	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
> -		gen6_force_wake_timer((unsigned long)dev_priv);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++ ) {
> +		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (del_timer_sync(&dev_priv->uncore.fw_domain[i].timer))
> +			gen6_force_wake_timer((unsigned long)&dev_priv->uncore.fw_domain[i]);
> +	}
>
>   	/* Hold uncore.lock across reset to prevent any register access
>   	 * with forcewake not set correctly
> @@ -327,18 +302,12 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>
>   	if (restore) { /* If reset with a user forcewake, try to restore */
>   		unsigned fw = 0;
> +		int i;
>
> -		if (IS_VALLEYVIEW(dev)) {
> -			if (dev_priv->uncore.fw_rendercount)
> -				fw |= FORCEWAKE_RENDER;
> -
> -			if (dev_priv->uncore.fw_mediacount)
> -				fw |= FORCEWAKE_MEDIA;
> -		} else {
> -			if (dev_priv->uncore.forcewake_count)
> -				fw = FORCEWAKE_ALL;
> +		for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +			if (dev_priv->uncore.fw_domain[i].wake_count)
> +				fw |= 1 << i;
>   		}
> -
>   		if (fw)
>   			dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
>
> @@ -389,50 +358,60 @@ void intel_uncore_sanitize(struct drm_device *dev)
>    * be called at the beginning of the sequence followed by a call to
>    * gen6_gt_force_wake_put() at the end of the sequence.
>    */
> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains)

Is gen6_ still the best prefix for these two?

Should you also use the opportunity to add kerneldoc now that Daniel is 
making a bit push in that direction?

>   {
>   	unsigned long irqflags;
> +	int i;

Not sure if it makes any difference nowadays to use unsigned for loops 
like this.

>
>   	if (!dev_priv->uncore.funcs.force_wake_get)
>   		return;
>
>   	WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
>
> +	fw_domains &= dev_priv->uncore.fw_domains;
> +
>   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> -	/* Redirect to VLV specific routine */
> -	if (IS_VALLEYVIEW(dev_priv->dev)) {
> -		vlv_force_wake_get(dev_priv, fw_engine);
> -	} else {
> -		if (dev_priv->uncore.forcewake_count++ == 0)
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> -							      FORCEWAKE_ALL);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (dev_priv->uncore.fw_domain[i].wake_count++)
> +			fw_domains &= ~(1 << i);
>   	}
> +
> +	if (fw_domains)
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +
>   	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>   }
>
>   /*
>    * see gen6_gt_force_wake_get()
>    */
> -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains)
>   {
>   	unsigned long irqflags;
> +	int i;
>
>   	if (!dev_priv->uncore.funcs.force_wake_put)
>   		return;
>
>   	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> -	/* Redirect to VLV specific routine */
> -	if (IS_VALLEYVIEW(dev_priv->dev)) {
> -		vlv_force_wake_put(dev_priv, fw_engine);
> -	} else {
> -		WARN_ON(!dev_priv->uncore.forcewake_count);
> -		if (--dev_priv->uncore.forcewake_count == 0) {
> -			dev_priv->uncore.forcewake_count++;
> -			mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> -					 jiffies + 1);
> -		}
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		WARN_ON(!dev_priv->uncore.fw_domain[i].wake_count);
> +		if (--dev_priv->uncore.fw_domain[i].wake_count)
> +			continue;
> +
> +		dev_priv->uncore.fw_domain[i].wake_count++;

I would put a comment here about why we are incrementing after decrementing.

Possibly also what is the purpose of the timer if not already documented 
somewhere.

> +		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
> +				 jiffies + 1);
>   	}
>
>   	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> @@ -441,10 +420,13 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>
>   void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>   {
> +	int i;
> +
>   	if (!dev_priv->uncore.funcs.force_wake_get)
>   		return;
>
> -	WARN_ON(dev_priv->uncore.forcewake_count > 0);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++)
> +		WARN_ON(dev_priv->uncore.fw_domain[i].wake_count > 0);
>   }
>
>   /* We give fast paths for the really cool registers */
> @@ -578,19 +560,38 @@ __gen2_read(64)
>   	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
>   	return val
>
> +static inline void __force_wake_get(struct drm_i915_private *dev_priv,
> +				    unsigned fw_domains)

This function is to be used by what type of callers compared to 
gen6_gt_force_wake_get?

> +{
> +	int i;
> +
> +	/* Ideally GCC would be constant-fold and eliminate this loop */
> +
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (dev_priv->uncore.fw_domain[i].wake_count) {
> +			fw_domains &= ~(1 << i);
> +			continue;
> +		}
> +
> +		dev_priv->uncore.fw_domain[i].wake_count++;
> +		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
> +				 jiffies + 1);
> +	}
> +
> +	if (fw_domains)
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +}
> +
>   #define __gen6_read(x) \
>   static u##x \
>   gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>   	GEN6_READ_HEADER(x); \
>   	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
> -	if (dev_priv->uncore.forcewake_count == 0 && \
> -	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> -						      FORCEWAKE_ALL); \
> -		dev_priv->uncore.forcewake_count++; \
> -		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
> -				 jiffies + 1); \
> -	} \
> +	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
>   	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
>   	GEN6_READ_FOOTER; \
> @@ -599,45 +600,27 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>   #define __vlv_read(x) \
>   static u##x \
>   vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> -	unsigned fwengine = 0; \
>   	GEN6_READ_HEADER(x); \
> -	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine = FORCEWAKE_RENDER; \
> -	} else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine = FORCEWAKE_MEDIA; \
> -	}  \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
> +	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \

This changes from immediate to delayed put - would it make sense to move 
it into a follow up patch?

>   	GEN6_READ_FOOTER; \
>   }
>
>   #define __chv_read(x) \
>   static u##x \
>   chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> -	unsigned fwengine = 0; \
>   	GEN6_READ_HEADER(x); \
> -	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine = FORCEWAKE_RENDER; \
> -	} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine = FORCEWAKE_MEDIA; \
> -	} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine |= FORCEWAKE_RENDER; \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine |= FORCEWAKE_MEDIA; \
> -	} \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, \
> +				 FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
>   	val = __raw_i915_read##x(dev_priv, reg); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>   	GEN6_READ_FOOTER; \
>   }
>
> @@ -766,17 +749,9 @@ static void \
>   gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
>   	GEN6_WRITE_HEADER; \
>   	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
> -	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
> -							      FORCEWAKE_ALL); \
> -		__raw_i915_write##x(dev_priv, reg, val); \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> -							      FORCEWAKE_ALL); \
> -	} else { \
> -		__raw_i915_write##x(dev_priv, reg, val); \
> -	} \
> +	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	__raw_i915_write##x(dev_priv, reg, val); \
>   	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
>   	hsw_unclaimed_reg_detect(dev_priv); \
>   	GEN6_WRITE_FOOTER; \
> @@ -785,28 +760,17 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>   #define __chv_write(x) \
>   static void \
>   chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> -	unsigned fwengine = 0; \
>   	bool shadowed = is_gen8_shadowed(dev_priv, reg); \
>   	GEN6_WRITE_HEADER; \
>   	if (!shadowed) { \
> -		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_rendercount == 0) \
> -				fwengine = FORCEWAKE_RENDER; \
> -		} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_mediacount == 0) \
> -				fwengine = FORCEWAKE_MEDIA; \
> -		} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_rendercount == 0) \
> -				fwengine |= FORCEWAKE_RENDER; \
> -			if (dev_priv->uncore.fw_mediacount == 0) \
> -				fwengine |= FORCEWAKE_MEDIA; \
> -		} \
> +		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +		else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
> +		else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
>   	} \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
>   	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>   	GEN6_WRITE_FOOTER; \
>   }
>
> @@ -837,18 +801,18 @@ __gen6_write(64)
>   void intel_uncore_init(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	setup_timer(&dev_priv->uncore.force_wake_timer,
> -		    gen6_force_wake_timer, (unsigned long)dev_priv);
> +	int i;
>
>   	intel_uncore_early_sanitize(dev, false);
>
>   	if (IS_VALLEYVIEW(dev)) {
>   		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
>   		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER | FORCEWAKE_MEDIA;
>   	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>   		dev_priv->uncore.funcs.force_wake_get = __gen7_gt_force_wake_mt_get;
>   		dev_priv->uncore.funcs.force_wake_put = __gen7_gt_force_wake_mt_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
>   	} else if (IS_IVYBRIDGE(dev)) {
>   		u32 ecobus;
>
> @@ -880,11 +844,22 @@ void intel_uncore_init(struct drm_device *dev)
>   			dev_priv->uncore.funcs.force_wake_put =
>   				__gen6_gt_force_wake_put;
>   		}
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
>   	} else if (IS_GEN6(dev)) {
>   		dev_priv->uncore.funcs.force_wake_get =
>   			__gen6_gt_force_wake_get;
>   		dev_priv->uncore.funcs.force_wake_put =
>   			__gen6_gt_force_wake_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
> +	}
> +
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		dev_priv->uncore.fw_domain[i].i915 = dev_priv;
> +		dev_priv->uncore.fw_domain[i].id = i;
> +
> +		setup_timer(&dev_priv->uncore.fw_domain[i].timer,
> +			    gen6_force_wake_timer,
> +			    (unsigned long)&dev_priv->uncore.fw_domain[i]);
>   	}
>
>   	switch (INTEL_INFO(dev)->gen) {
>

Regards,

Tvrtko

Re: [PATCH] drm/i915: Reduce duplicated forcewake logic

[Tvrtko Ursulin]

On 10/01/2014 04:53 PM, Tvrtko Ursulin wrote:
>
> On 09/10/2014 07:34 PM, Chris Wilson wrote:
>> Introduce a structure to track the individual forcewake domains and use
>> that to eliminated duplicate logic.
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c |  41 +++---
>>   drivers/gpu/drm/i915/i915_drv.h     |  32 +++--
>>   drivers/gpu/drm/i915/intel_uncore.c | 265
>> ++++++++++++++++--------------------
>>   3 files changed, 157 insertions(+), 181 deletions(-)
>
> As said yesterday I like the idea, some comments and questions inline.

Forgot about intel_lrc.c which looks like wouldn't compile after this 
patch. :)

Regards,

Tvrtko

Re: [PATCH] drm/i915: Reduce duplicated forcewake logic

[Chris Wilson]

On Wed, Oct 01, 2014 at 04:53:07PM +0100, Tvrtko Ursulin wrote:
> >-	struct timer_list force_wake_timer;
> >+	struct intel_uncore_forcewake_domain {
> >+		struct drm_i915_private *i915;
> 
> Should this be named dev_priv for consistency?

Depends on whose tree you are looking at. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Reduce duplicated forcewake logic

[Ville Syrjälä]

On Wed, Sep 10, 2014 at 07:34:54PM +0100, Chris Wilson wrote:
> Introduce a structure to track the individual forcewake domains and use
> that to eliminated duplicate logic.
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  41 +++---
>  drivers/gpu/drm/i915/i915_drv.h     |  32 +++--
>  drivers/gpu/drm/i915/intel_uncore.c | 265 ++++++++++++++++--------------------
>  3 files changed, 157 insertions(+), 181 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index a72d8b8..c3176f1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1317,7 +1317,6 @@ static int vlv_drpc_info(struct seq_file *m)
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 rpmodectl1, rcctl1;
> -	unsigned fw_rendercount = 0, fw_mediacount = 0;
>  
>  	intel_runtime_pm_get(dev_priv);
>  
> @@ -1350,22 +1349,12 @@ static int vlv_drpc_info(struct seq_file *m)
>  	seq_printf(m, "Media RC6 residency since boot: %u\n",
>  		   I915_READ(VLV_GT_MEDIA_RC6));
>  
> -	spin_lock_irq(&dev_priv->uncore.lock);
> -	fw_rendercount = dev_priv->uncore.fw_rendercount;
> -	fw_mediacount = dev_priv->uncore.fw_mediacount;
> -	spin_unlock_irq(&dev_priv->uncore.lock);
> -
> -	seq_printf(m, "Forcewake Render Count = %u\n", fw_rendercount);
> -	seq_printf(m, "Forcewake Media Count = %u\n", fw_mediacount);
> -
> -
> -	return 0;
> +	return i915_gen6_forcewake_count_info(m, data);
>  }
>  
>  
>  static int gen6_drpc_info(struct seq_file *m)
>  {
> -
>  	struct drm_info_node *node = m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1379,7 +1368,7 @@ static int gen6_drpc_info(struct seq_file *m)
>  	intel_runtime_pm_get(dev_priv);
>  
>  	spin_lock_irq(&dev_priv->uncore.lock);
> -	forcewake_count = dev_priv->uncore.forcewake_count;
> +	forcewake_count = dev_priv->uncore.fw_domain[FW_DOMAIN_RENDER].wake_count;
>  	spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  	if (forcewake_count) {
> @@ -1976,21 +1965,23 @@ static int i915_gen6_forcewake_count_info(struct seq_file *m, void *data)
>  	struct drm_info_node *node = m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	unsigned forcewake_count = 0, fw_rendercount = 0, fw_mediacount = 0;
> +	const char *domain_names[] = {

static const char * const domain_names[] ?

Hmm. Maybe someone should fire up coccinelle to go through the entire
codebase with this...

> +		"render",
> +		"media",
> +	};
> +	int i;
>  
>  	spin_lock_irq(&dev_priv->uncore.lock);
> -	if (IS_VALLEYVIEW(dev)) {
> -		fw_rendercount = dev_priv->uncore.fw_rendercount;
> -		fw_mediacount = dev_priv->uncore.fw_mediacount;
> -	} else
> -		forcewake_count = dev_priv->uncore.forcewake_count;
> -	spin_unlock_irq(&dev_priv->uncore.lock);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
> +			continue;
>  
> -	if (IS_VALLEYVIEW(dev)) {
> -		seq_printf(m, "fw_rendercount = %u\n", fw_rendercount);
> -		seq_printf(m, "fw_mediacount = %u\n", fw_mediacount);
> -	} else
> -		seq_printf(m, "forcewake count = %u\n", forcewake_count);
> +		seq_printf(m, "%s.wake_count = %u\n",
> +			   domain_names[i],
> +			   dev_priv->uncore.fw_domain[i].wake_count);
> +	}
> +
> +	spin_unlock_irq(&dev_priv->uncore.lock);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5d0b38c..6424191 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -528,18 +528,30 @@ struct intel_uncore_funcs {
>  				uint64_t val, bool trace);
>  };
>  
> +enum {
> +	FW_DOMAIN_RENDER = 0,
> +	FW_DOMAIN_MEDIA,
> +
> +	FW_DOMAIN_COUNT
> +};
> +
>  struct intel_uncore {
>  	spinlock_t lock; /** lock is also taken in irq contexts. */
>  
>  	struct intel_uncore_funcs funcs;
>  
>  	unsigned fifo_count;
> -	unsigned forcewake_count;
> -
> -	unsigned fw_rendercount;
> -	unsigned fw_mediacount;
> +	unsigned fw_domains;
>  
> -	struct timer_list force_wake_timer;
> +	struct intel_uncore_forcewake_domain {
> +		struct drm_i915_private *i915;
> +		int id;
> +		unsigned wake_count;
> +		struct timer_list timer;
> +	} fw_domain[FW_DOMAIN_COUNT];
> +#define FORCEWAKE_RENDER	(1 << FW_DOMAIN_RENDER)
> +#define FORCEWAKE_MEDIA		(1 << FW_DOMAIN_MEDIA)
> +#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
>  };
>  
>  #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> @@ -2948,8 +2960,10 @@ extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
>   * must be set to prevent GT core from power down and stale values being
>   * returned.
>   */
> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
> -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains);
> +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains);
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv);
>  
>  int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u8 mbox, u32 *val);
> @@ -2981,10 +2995,6 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
>  int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
>  
> -#define FORCEWAKE_RENDER	(1 << 0)
> -#define FORCEWAKE_MEDIA		(1 << 1)
> -#define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
> -
>  
>  #define I915_READ8(reg)		dev_priv->uncore.funcs.mmio_readb(dev_priv, (reg), true)
>  #define I915_WRITE8(reg, val)	dev_priv->uncore.funcs.mmio_writeb(dev_priv, (reg), (val), true)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 3b3d3e0..641950b 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -73,7 +73,7 @@ static void __gen6_gt_force_wake_reset(struct drm_i915_private *dev_priv)
>  }
>  
>  static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +				     int fw_engine)
>  {
>  	if (wait_for_atomic((__raw_i915_read32(dev_priv, FORCEWAKE_ACK) & 1) == 0,
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
> @@ -99,7 +99,7 @@ static void __gen7_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
>  }
>  
>  static void __gen7_gt_force_wake_mt_get(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +					int fw_engine)
>  {
>  	u32 forcewake_ack;
>  
> @@ -136,7 +136,7 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>  }
>  
>  static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +				     int fw_engine)
>  {
>  	__raw_i915_write32(dev_priv, FORCEWAKE, 0);
>  	/* something from same cacheline, but !FORCEWAKE */
> @@ -145,7 +145,7 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
>  }
>  
>  static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
> -							int fw_engine)
> +					int fw_engine)
>  {
>  	__raw_i915_write32(dev_priv, FORCEWAKE_MT,
>  			   _MASKED_BIT_DISABLE(FORCEWAKE_KERNEL));
> @@ -194,7 +194,7 @@ static void vlv_force_wake_reset(struct drm_i915_private *dev_priv)
>  }
>  
>  static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
> -						int fw_engine)
> +				 int fw_engine)
>  {
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine) {
> @@ -238,9 +238,8 @@ static void __vlv_force_wake_get(struct drm_i915_private *dev_priv,
>  }
>  
>  static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
> -					int fw_engine)
> +				 int fw_engine)
>  {
> -
>  	/* Check for Render Engine */
>  	if (FORCEWAKE_RENDER & fw_engine)
>  		__raw_i915_write32(dev_priv, FORCEWAKE_VLV,
> @@ -258,59 +257,35 @@ static void __vlv_force_wake_put(struct drm_i915_private *dev_priv,
>  		gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -static void vlv_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> -{
> -	if (fw_engine & FORCEWAKE_RENDER &&
> -	    dev_priv->uncore.fw_rendercount++ != 0)
> -		fw_engine &= ~FORCEWAKE_RENDER;
> -	if (fw_engine & FORCEWAKE_MEDIA &&
> -	    dev_priv->uncore.fw_mediacount++ != 0)
> -		fw_engine &= ~FORCEWAKE_MEDIA;
> -
> -	if (fw_engine)
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_engine);
> -}
> -
> -static void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> -{
> -	if (fw_engine & FORCEWAKE_RENDER) {
> -		WARN_ON(!dev_priv->uncore.fw_rendercount);
> -		if (--dev_priv->uncore.fw_rendercount != 0)
> -			fw_engine &= ~FORCEWAKE_RENDER;
> -	}
> -
> -	if (fw_engine & FORCEWAKE_MEDIA) {
> -		WARN_ON(!dev_priv->uncore.fw_mediacount);
> -		if (--dev_priv->uncore.fw_mediacount != 0)
> -			fw_engine &= ~FORCEWAKE_MEDIA;
> -	}
> -
> -	if (fw_engine)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fw_engine);
> -}
> -
>  static void gen6_force_wake_timer(unsigned long arg)
>  {
> -	struct drm_i915_private *dev_priv = (void *)arg;
> +	struct intel_uncore_forcewake_domain *domain = (void *)arg;
>  	unsigned long irqflags;
>  
> -	assert_device_not_suspended(dev_priv);
> +	assert_device_not_suspended(domain->i915);
>  
> -	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> -	WARN_ON(!dev_priv->uncore.forcewake_count);
> +	spin_lock_irqsave(&domain->i915->uncore.lock, irqflags);
> +	WARN_ON(!domain->wake_count);
>  
> -	if (--dev_priv->uncore.forcewake_count == 0)
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
> -	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +	if (--domain->wake_count == 0)
> +		domain->i915->uncore.funcs.force_wake_put(domain->i915,
> +							  1 << domain->id);
> +	spin_unlock_irqrestore(&domain->i915->uncore.lock, irqflags);
>  }
>  
>  void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	unsigned long irqflags;
> +	int i;
>  
> -	if (del_timer_sync(&dev_priv->uncore.force_wake_timer))
> -		gen6_force_wake_timer((unsigned long)dev_priv);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++ ) {
> +		if ((dev_priv->uncore.fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (del_timer_sync(&dev_priv->uncore.fw_domain[i].timer))
> +			gen6_force_wake_timer((unsigned long)&dev_priv->uncore.fw_domain[i]);
> +	}
>  
>  	/* Hold uncore.lock across reset to prevent any register access
>  	 * with forcewake not set correctly
> @@ -327,18 +302,12 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
>  
>  	if (restore) { /* If reset with a user forcewake, try to restore */
>  		unsigned fw = 0;
> +		int i;
>  
> -		if (IS_VALLEYVIEW(dev)) {
> -			if (dev_priv->uncore.fw_rendercount)
> -				fw |= FORCEWAKE_RENDER;
> -
> -			if (dev_priv->uncore.fw_mediacount)
> -				fw |= FORCEWAKE_MEDIA;
> -		} else {
> -			if (dev_priv->uncore.forcewake_count)
> -				fw = FORCEWAKE_ALL;
> +		for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +			if (dev_priv->uncore.fw_domain[i].wake_count)
> +				fw |= 1 << i;
>  		}
> -
>  		if (fw)
>  			dev_priv->uncore.funcs.force_wake_get(dev_priv, fw);
>  
> @@ -389,50 +358,60 @@ void intel_uncore_sanitize(struct drm_device *dev)
>   * be called at the beginning of the sequence followed by a call to
>   * gen6_gt_force_wake_put() at the end of the sequence.
>   */
> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains)
>  {
>  	unsigned long irqflags;
> +	int i;
>  
>  	if (!dev_priv->uncore.funcs.force_wake_get)
>  		return;
>  
>  	WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
>  
> +	fw_domains &= dev_priv->uncore.fw_domains;
> +
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	/* Redirect to VLV specific routine */
> -	if (IS_VALLEYVIEW(dev_priv->dev)) {
> -		vlv_force_wake_get(dev_priv, fw_engine);
> -	} else {
> -		if (dev_priv->uncore.forcewake_count++ == 0)
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv,
> -							      FORCEWAKE_ALL);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (dev_priv->uncore.fw_domain[i].wake_count++)
> +			fw_domains &= ~(1 << i);
>  	}
> +
> +	if (fw_domains)
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>  }
>  
>  /*
>   * see gen6_gt_force_wake_get()
>   */
> -void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
> +void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
> +			    unsigned fw_domains)
>  {
>  	unsigned long irqflags;
> +	int i;
>  
>  	if (!dev_priv->uncore.funcs.force_wake_put)
>  		return;

Aren't we missing this

fw_domains &= dev_priv->uncore.fw_domains;

here? I would expect to see the WARN triggering below due to
wake_count==0 when the domain doesn't exist.

>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	/* Redirect to VLV specific routine */
> -	if (IS_VALLEYVIEW(dev_priv->dev)) {
> -		vlv_force_wake_put(dev_priv, fw_engine);
> -	} else {
> -		WARN_ON(!dev_priv->uncore.forcewake_count);
> -		if (--dev_priv->uncore.forcewake_count == 0) {
> -			dev_priv->uncore.forcewake_count++;
> -			mod_timer_pinned(&dev_priv->uncore.force_wake_timer,
> -					 jiffies + 1);
> -		}
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		WARN_ON(!dev_priv->uncore.fw_domain[i].wake_count);
> +		if (--dev_priv->uncore.fw_domain[i].wake_count)
> +			continue;
> +

Maybe WARN_ON(timer_pending()) here?

> +		dev_priv->uncore.fw_domain[i].wake_count++;
> +		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
> +				 jiffies + 1);
>  	}
>  
>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> @@ -441,10 +420,13 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine)
>  
>  void assert_force_wake_inactive(struct drm_i915_private *dev_priv)
>  {
> +	int i;
> +
>  	if (!dev_priv->uncore.funcs.force_wake_get)
>  		return;
>  
> -	WARN_ON(dev_priv->uncore.forcewake_count > 0);
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++)
> +		WARN_ON(dev_priv->uncore.fw_domain[i].wake_count > 0);
>  }
>  
>  /* We give fast paths for the really cool registers */
> @@ -578,19 +560,38 @@ __gen2_read(64)
>  	trace_i915_reg_rw(false, reg, val, sizeof(val), trace); \
>  	return val
>  
> +static inline void __force_wake_get(struct drm_i915_private *dev_priv,
> +				    unsigned fw_domains)
> +{
> +	int i;
> +
> +	/* Ideally GCC would be constant-fold and eliminate this loop */
> +
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		if ((fw_domains & (1 << i)) == 0)
> +			continue;
> +
> +		if (dev_priv->uncore.fw_domain[i].wake_count) {
> +			fw_domains &= ~(1 << i);
> +			continue;
> +		}
> +

And here?

I think I had some concerns about the wake_count vs. timer handling when
I saw the first version of this patch, but now that I look at this I
can't see anything wrong with it.

> +		dev_priv->uncore.fw_domain[i].wake_count++;
> +		mod_timer_pinned(&dev_priv->uncore.fw_domain[i].timer,
> +				 jiffies + 1);
> +	}
> +
> +	if (fw_domains)
> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
> +}
> +
>  #define __gen6_read(x) \
>  static u##x \
>  gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  	GEN6_READ_HEADER(x); \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \
> -	if (dev_priv->uncore.forcewake_count == 0 && \
> -	    NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> -						      FORCEWAKE_ALL); \
> -		dev_priv->uncore.forcewake_count++; \
> -		mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
> -				 jiffies + 1); \

This seems to be against some other tree where the timer is already used
for register accesses. That's not the case in nightly AFAICS.

I'm a bit worried about making the timer change at the same time as the
code refactoring. I would suggest refactoring first, and tossing in another
patch on top which expands the use of the timer to include register
access. That way we could at least revert the timer part if necessary
without losing the much cleaner code.

Also the code in nightly still has the runtime pm stuff in the forcewake
code. I guess you cleaned those out in your tree, but I don't remeber
seeing a recent patch on that front.

Otherwise this lookd very nice IMO, so would be great to get it in
finally.

> -	} \
> +	if (NEEDS_FORCE_WAKE((dev_priv), (reg))) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
>  	val = __raw_i915_read##x(dev_priv, reg); \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \
>  	GEN6_READ_FOOTER; \
> @@ -599,45 +600,27 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
>  #define __vlv_read(x) \
>  static u##x \
>  vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> -	unsigned fwengine = 0; \
>  	GEN6_READ_HEADER(x); \
> -	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine = FORCEWAKE_RENDER; \
> -	} else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine = FORCEWAKE_MEDIA; \
> -	}  \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
> +	if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
>  	val = __raw_i915_read##x(dev_priv, reg); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>  	GEN6_READ_FOOTER; \
>  }
>  
>  #define __chv_read(x) \
>  static u##x \
>  chv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> -	unsigned fwengine = 0; \
>  	GEN6_READ_HEADER(x); \
> -	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine = FORCEWAKE_RENDER; \
> -	} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine = FORCEWAKE_MEDIA; \
> -	} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -		if (dev_priv->uncore.fw_rendercount == 0) \
> -			fwengine |= FORCEWAKE_RENDER; \
> -		if (dev_priv->uncore.fw_mediacount == 0) \
> -			fwengine |= FORCEWAKE_MEDIA; \
> -	} \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
> +	if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
> +	else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
> +		__force_wake_get(dev_priv, \
> +				 FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
>  	val = __raw_i915_read##x(dev_priv, reg); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>  	GEN6_READ_FOOTER; \
>  }
>  
> @@ -766,17 +749,9 @@ static void \
>  gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
>  	GEN6_WRITE_HEADER; \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \
> -	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_get(dev_priv,	\
> -							      FORCEWAKE_ALL); \
> -		__raw_i915_write##x(dev_priv, reg, val); \
> -		if (dev_priv->uncore.forcewake_count == 0) \
> -			dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> -							      FORCEWAKE_ALL); \
> -	} else { \
> -		__raw_i915_write##x(dev_priv, reg, val); \
> -	} \
> +	if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) \
> +		__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +	__raw_i915_write##x(dev_priv, reg, val); \
>  	hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \
>  	hsw_unclaimed_reg_detect(dev_priv); \
>  	GEN6_WRITE_FOOTER; \
> @@ -785,28 +760,17 @@ gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>  #define __chv_write(x) \
>  static void \
>  chv_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> -	unsigned fwengine = 0; \
>  	bool shadowed = is_gen8_shadowed(dev_priv, reg); \
>  	GEN6_WRITE_HEADER; \
>  	if (!shadowed) { \
> -		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_rendercount == 0) \
> -				fwengine = FORCEWAKE_RENDER; \
> -		} else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_mediacount == 0) \
> -				fwengine = FORCEWAKE_MEDIA; \
> -		} else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) { \
> -			if (dev_priv->uncore.fw_rendercount == 0) \
> -				fwengine |= FORCEWAKE_RENDER; \
> -			if (dev_priv->uncore.fw_mediacount == 0) \
> -				fwengine |= FORCEWAKE_MEDIA; \
> -		} \
> +		if (FORCEWAKE_CHV_RENDER_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_RENDER); \
> +		else if (FORCEWAKE_CHV_MEDIA_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_MEDIA); \
> +		else if (FORCEWAKE_CHV_COMMON_RANGE_OFFSET(reg)) \
> +			__force_wake_get(dev_priv, FORCEWAKE_RENDER | FORCEWAKE_MEDIA); \
>  	} \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_get(dev_priv, fwengine); \
>  	__raw_i915_write##x(dev_priv, reg, val); \
> -	if (fwengine) \
> -		dev_priv->uncore.funcs.force_wake_put(dev_priv, fwengine); \
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> @@ -837,18 +801,18 @@ __gen6_write(64)
>  void intel_uncore_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	setup_timer(&dev_priv->uncore.force_wake_timer,
> -		    gen6_force_wake_timer, (unsigned long)dev_priv);
> +	int i;
>  
>  	intel_uncore_early_sanitize(dev, false);
>  
>  	if (IS_VALLEYVIEW(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
>  		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER | FORCEWAKE_MEDIA;
>  	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get = __gen7_gt_force_wake_mt_get;
>  		dev_priv->uncore.funcs.force_wake_put = __gen7_gt_force_wake_mt_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
>  	} else if (IS_IVYBRIDGE(dev)) {
>  		u32 ecobus;
>  
> @@ -880,11 +844,22 @@ void intel_uncore_init(struct drm_device *dev)
>  			dev_priv->uncore.funcs.force_wake_put =
>  				__gen6_gt_force_wake_put;
>  		}
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
>  	} else if (IS_GEN6(dev)) {
>  		dev_priv->uncore.funcs.force_wake_get =
>  			__gen6_gt_force_wake_get;
>  		dev_priv->uncore.funcs.force_wake_put =
>  			__gen6_gt_force_wake_put;
> +		dev_priv->uncore.fw_domains = FORCEWAKE_RENDER;
> +	}
> +
> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
> +		dev_priv->uncore.fw_domain[i].i915 = dev_priv;
> +		dev_priv->uncore.fw_domain[i].id = i;
> +
> +		setup_timer(&dev_priv->uncore.fw_domain[i].timer,
> +			    gen6_force_wake_timer,
> +			    (unsigned long)&dev_priv->uncore.fw_domain[i]);
>  	}
>  
>  	switch (INTEL_INFO(dev)->gen) {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reduce duplicated forcewake logic

[Dave Gordon]

On 07/11/14 15:46, Ville Syrjälä wrote:
> On Wed, Sep 10, 2014 at 07:34:54PM +0100, Chris Wilson wrote:
>> Introduce a structure to track the individual forcewake domains and use
>> that to eliminated duplicate logic.
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c |  41 +++---
>>  drivers/gpu/drm/i915/i915_drv.h     |  32 +++--
>>  drivers/gpu/drm/i915/intel_uncore.c | 265 ++++++++++++++++--------------------
>>  3 files changed, 157 insertions(+), 181 deletions(-)

Hi Chris,

this looks useful -- I was looking at refactoring the VLV vs GEN9
versions of forcewake too. A few comments below:

>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index 3b3d3e0..641950b 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c

>> @@ -145,7 +145,7 @@ static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv,
>>  }
>>  
>>  static void __gen7_gt_force_wake_mt_put(struct drm_i915_private *dev_priv,
>> -							int fw_engine)
>> +					int fw_engine)

Here and in all the other versions: how about renaming the parameter to
"fw_domains" in line with your new enum and struct naming? Thus making
it clearer that it can have multiple bits set ...

>> @@ -389,50 +358,60 @@ void intel_uncore_sanitize(struct drm_device *dev)
>>   * be called at the beginning of the sequence followed by a call to
>>   * gen6_gt_force_wake_put() at the end of the sequence.
>>   */
>> -void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine)
>> +void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv,
>> +			    unsigned fw_domains)
>>  {
>>  	unsigned long irqflags;
>> +	int i;
>>  
>>  	if (!dev_priv->uncore.funcs.force_wake_get)
>>  		return;
>>  
>>  	WARN_ON(!pm_runtime_active(&dev_priv->dev->pdev->dev));
>>  
>> +	fw_domains &= dev_priv->uncore.fw_domains;
>> +
>>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>>  
>> -	/* Redirect to VLV specific routine */
>> -	if (IS_VALLEYVIEW(dev_priv->dev)) {
>> -		vlv_force_wake_get(dev_priv, fw_engine);
>> -	} else {
>> -		if (dev_priv->uncore.forcewake_count++ == 0)
>> -			dev_priv->uncore.funcs.force_wake_get(dev_priv,
>> -							      FORCEWAKE_ALL);
>> +	for (i = 0; i < FW_DOMAIN_COUNT; i++) {
>> +		if ((fw_domains & (1 << i)) == 0)
>> +			continue;
>> +
>> +		if (dev_priv->uncore.fw_domain[i].wake_count++)
>> +			fw_domains &= ~(1 << i);
>>  	}
>> +
>> +	if (fw_domains)
>> +		dev_priv->uncore.funcs.force_wake_get(dev_priv, fw_domains);
>> +
>>  	spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>>  }

Nice to get rid of the "Redirect to XXX specific routine" stuff -- one
of the things I don't like about the current forcewake code is that the
VLV and GEN9 versions of the outer wrapper are inconsistent w.r.t. how
(and how many times) to call the ->force_wait_get() vfunc -- vlv
collects all the bits and makes one call, gen9 makes one call for each
bit separately. This code would handle all gens consistently, which is
much nicer. And I prefer a single call with multiple bits, so that the
low-level code, which knows where the corresponding h/w bits are, can
deal efficiently with multiple bits mapping to the same register, or
(for example) issue all the forcewake writes first and then poll them
all for completion, rather than doing the write/poll pairs sequentially.

[snip]
> 
> Also the code in nightly still has the runtime pm stuff in the forcewake
> code. I guess you cleaned those out in your tree, but I don't remeber
> seeing a recent patch on that front.

Yes, this has been another area we wanted clarified and/or tidied up too
- I posted a message earlier today asking about whether the PM calls
were necessary and a patch that allowed them to be avoided. But if they
can be removed completely that would be better still :)

> Otherwise this lookd very nice IMO, so would be great to get it in
> finally.

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