[PATCH 0/5] drm/i915: VGA vs. power well

[PATCH 0/5] drm/i915: VGA vs. power well

[ville.syrjala]

Match of a lifetime! In the red corner VGA, the underdog. In the blue corner,
the killer of registers, the bane of memory access, the POWER WELL! Who will
win?

This series eliminates the unclaimed register access warning introduced by
the delayed VGA memory disable. And now we actually disable VGA memory decode
on machines where we turn the power well off during init, making it slightly
more probable that vga arbitration will work.

Fixing pc8 entry when the last power well reference disappears is left as
an exercise for the reader.

[PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add APIs to get/put power well references for specific purposes.

Also reorganize the internal i915_request power well handling to use the
reference count just like everyone else. This way all we need to do is
check the reference count and we know whether the power well needs to be
enabled of disabled.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  4 ++
 drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 87 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 774ebb6..2ecd3d2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
 
 extern bool intel_display_power_enabled(struct drm_device *dev,
 					enum intel_display_power_domain domain);
+extern void intel_display_power_get(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
+extern void intel_display_power_put(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8cffef4..4962303 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
+void intel_display_power_get(struct drm_device *dev,
+			     enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		if (!power_well->count++)
+			__intel_set_power_well(power_well->device, true);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
+void intel_display_power_put(struct drm_device *dev,
+			     enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_well *power_well = &dev_priv->power_well;
+
+	if (!HAS_POWER_WELL(dev))
+		return;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return;
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		spin_lock_irq(&power_well->lock);
+		WARN_ON(!power_well->count);
+		if (!--power_well->count)
+			__intel_set_power_well(power_well->device, false);
+		spin_unlock_irq(&power_well->lock);
+		return;
+	default:
+		BUG();
+	}
+}
+
 static struct i915_power_well *hsw_pwr;
 
 /* Display audio driver power well request */
@@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	if (!hsw_pwr->count++ &&
-			!hsw_pwr->i915_request)
+	if (!hsw_pwr->count++)
 		__intel_set_power_well(hsw_pwr->device, true);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
 
 	spin_lock_irq(&hsw_pwr->lock);
 	WARN_ON(!hsw_pwr->count);
-	if (!--hsw_pwr->count &&
-		       !hsw_pwr->i915_request)
+	if (!--hsw_pwr->count)
 		__intel_set_power_well(hsw_pwr->device, false);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
@@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 		return;
 
 	spin_lock_irq(&power_well->lock);
+
+	/*
+	 * This function will only ever contribute one
+	 * to the power well reference count. i915_request
+	 * is what tracks whether we have or have not
+	 * added the one to the reference count.
+	 */
+	if (power_well->i915_request == enable)
+		goto out;
+
 	power_well->i915_request = enable;
 
-	/* only reject "disable" power well request */
-	if (power_well->count && !enable) {
-		spin_unlock_irq(&power_well->lock);
-		return;
+	if (enable) {
+		if (!power_well->count++)
+			__intel_set_power_well(dev, true);
+	} else {
+		WARN_ON(!power_well->count);
+		if (!--power_well->count)
+			__intel_set_power_well(dev, false);
 	}
 
-	__intel_set_power_well(dev, enable);
+ out:
 	spin_unlock_irq(&power_well->lock);
 }
 
-- 
1.8.1.5

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

Re: [PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

[Paulo Zanoni]

Hi

2013/9/12  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add APIs to get/put power well references for specific purposes.
>
> Also reorganize the internal i915_request power well handling to use the
> reference count just like everyone else. This way all we need to do is
> check the reference count and we know whether the power well needs to be
> enabled of disabled.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++++++++++++----
>  2 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 774ebb6..2ecd3d2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
>
>  extern bool intel_display_power_enabled(struct drm_device *dev,
>                                         enum intel_display_power_domain domain);
> +extern void intel_display_power_get(struct drm_device *dev,
> +                                   enum intel_display_power_domain domain);
> +extern void intel_display_power_put(struct drm_device *dev,
> +                                   enum intel_display_power_domain domain);
>  extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8cffef4..4962303 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>         }
>  }
>
> +void intel_display_power_get(struct drm_device *dev,
> +                            enum intel_display_power_domain domain)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_well *power_well = &dev_priv->power_well;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return;
> +
> +       switch (domain) {
> +       case POWER_DOMAIN_PIPE_A:
> +       case POWER_DOMAIN_TRANSCODER_EDP:
> +               return;
> +       case POWER_DOMAIN_PIPE_B:
> +       case POWER_DOMAIN_PIPE_C:
> +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> +       case POWER_DOMAIN_TRANSCODER_A:
> +       case POWER_DOMAIN_TRANSCODER_B:
> +       case POWER_DOMAIN_TRANSCODER_C:

I know I'm the one who added all these domains, but I have to say I
only did this because of the reviewers, I don't really like the
interface. With your addition there's a new problem: you can get the
POWER_DOMAIN_PIPE_B interface and then put the
POWER_COMAIN_PIPE_C_PANEL_FITTER and no one will notice. I really
think the power well itself should be the domain. Also, in cases like
the suspend/resume code we don't have any domain that makes sense. But
what's *not* ugly about the power well code?

I'm not suggesting you to fix that, I'm more kinda asking for ideas, I
may want to reorganize this code yet again when doing the D3 feature.
(Just because every single time we touch the power well code we have
to refactor it!)


> +               spin_lock_irq(&power_well->lock);
> +               if (!power_well->count++)
> +                       __intel_set_power_well(power_well->device, true);
> +               spin_unlock_irq(&power_well->lock);
> +               return;
> +       default:
> +               BUG();
> +       }
> +}
> +
> +void intel_display_power_put(struct drm_device *dev,
> +                            enum intel_display_power_domain domain)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_well *power_well = &dev_priv->power_well;
> +
> +       if (!HAS_POWER_WELL(dev))
> +               return;
> +
> +       switch (domain) {
> +       case POWER_DOMAIN_PIPE_A:
> +       case POWER_DOMAIN_TRANSCODER_EDP:
> +               return;
> +       case POWER_DOMAIN_PIPE_B:
> +       case POWER_DOMAIN_PIPE_C:
> +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> +       case POWER_DOMAIN_TRANSCODER_A:
> +       case POWER_DOMAIN_TRANSCODER_B:
> +       case POWER_DOMAIN_TRANSCODER_C:
> +               spin_lock_irq(&power_well->lock);
> +               WARN_ON(!power_well->count);
> +               if (!--power_well->count)
> +                       __intel_set_power_well(power_well->device, false);
> +               spin_unlock_irq(&power_well->lock);
> +               return;
> +       default:
> +               BUG();
> +       }
> +}
> +
>  static struct i915_power_well *hsw_pwr;
>
>  /* Display audio driver power well request */
> @@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
>                 return;
>
>         spin_lock_irq(&hsw_pwr->lock);
> -       if (!hsw_pwr->count++ &&
> -                       !hsw_pwr->i915_request)
> +       if (!hsw_pwr->count++)
>                 __intel_set_power_well(hsw_pwr->device, true);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
>
>         spin_lock_irq(&hsw_pwr->lock);
>         WARN_ON(!hsw_pwr->count);
> -       if (!--hsw_pwr->count &&
> -                      !hsw_pwr->i915_request)
> +       if (!--hsw_pwr->count)
>                 __intel_set_power_well(hsw_pwr->device, false);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
> @@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>                 return;
>
>         spin_lock_irq(&power_well->lock);
> +
> +       /*
> +        * This function will only ever contribute one
> +        * to the power well reference count. i915_request
> +        * is what tracks whether we have or have not
> +        * added the one to the reference count.
> +        */
> +       if (power_well->i915_request == enable)
> +               goto out;
> +
>         power_well->i915_request = enable;
>
> -       /* only reject "disable" power well request */
> -       if (power_well->count && !enable) {
> -               spin_unlock_irq(&power_well->lock);
> -               return;

And now to the real problem of the patch: previously whenever we got a
call to "enable" we'd call __intel_set_power_well and certainly write
the register. Now with this patch we may not do this due to
i915_request and the count. This breaks suspend/resume where just
after we resume we call intel_set_power_well(dev, true) but then the
new code doesn't really writes the register since i915_request is
already true. As a consequence, we see "unclaimed register" messages
complaining about registers 70008, 71008 and 72008. Perhaps in the
resume path we should fix our tracking and force the "enable" somehow.


> +       if (enable) {
> +               if (!power_well->count++)
> +                       __intel_set_power_well(dev, true);
> +       } else {
> +               WARN_ON(!power_well->count);
> +               if (!--power_well->count)
> +                       __intel_set_power_well(dev, false);
>         }
>
> -       __intel_set_power_well(dev, enable);
> + out:
>         spin_unlock_irq(&power_well->lock);
>  }
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 05:05:59PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Add APIs to get/put power well references for specific purposes.
> >
> > Also reorganize the internal i915_request power well handling to use the
> > reference count just like everyone else. This way all we need to do is
> > check the reference count and we know whether the power well needs to be
> > enabled of disabled.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  4 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 87 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 774ebb6..2ecd3d2 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
> >
> >  extern bool intel_display_power_enabled(struct drm_device *dev,
> >                                         enum intel_display_power_domain domain);
> > +extern void intel_display_power_get(struct drm_device *dev,
> > +                                   enum intel_display_power_domain domain);
> > +extern void intel_display_power_put(struct drm_device *dev,
> > +                                   enum intel_display_power_domain domain);
> >  extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8cffef4..4962303 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
> >         }
> >  }
> >
> > +void intel_display_power_get(struct drm_device *dev,
> > +                            enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return;
> > +
> > +       switch (domain) {
> > +       case POWER_DOMAIN_PIPE_A:
> > +       case POWER_DOMAIN_TRANSCODER_EDP:
> > +               return;
> > +       case POWER_DOMAIN_PIPE_B:
> > +       case POWER_DOMAIN_PIPE_C:
> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> > +       case POWER_DOMAIN_TRANSCODER_A:
> > +       case POWER_DOMAIN_TRANSCODER_B:
> > +       case POWER_DOMAIN_TRANSCODER_C:
> 
> I know I'm the one who added all these domains, but I have to say I
> only did this because of the reviewers, I don't really like the
> interface. With your addition there's a new problem: you can get the
> POWER_DOMAIN_PIPE_B interface and then put the
> POWER_COMAIN_PIPE_C_PANEL_FITTER and no one will notice. I really
> think the power well itself should be the domain. Also, in cases like
> the suspend/resume code we don't have any domain that makes sense. But
> what's *not* ugly about the power well code?
> 
> I'm not suggesting you to fix that, I'm more kinda asking for ideas, I
> may want to reorganize this code yet again when doing the D3 feature.
> (Just because every single time we touch the power well code we have
> to refactor it!)

In other platforms we're going to have totally different mix of
functional blocks vs. power wells. So assuming we want to deal with those
using a unified API we do need something like this. But maybe there's a
better way to go, haven't really thought about it.

> 
> 
> > +               spin_lock_irq(&power_well->lock);
> > +               if (!power_well->count++)
> > +                       __intel_set_power_well(power_well->device, true);
> > +               spin_unlock_irq(&power_well->lock);
> > +               return;
> > +       default:
> > +               BUG();
> > +       }
> > +}
> > +
> > +void intel_display_power_put(struct drm_device *dev,
> > +                            enum intel_display_power_domain domain)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       struct i915_power_well *power_well = &dev_priv->power_well;
> > +
> > +       if (!HAS_POWER_WELL(dev))
> > +               return;
> > +
> > +       switch (domain) {
> > +       case POWER_DOMAIN_PIPE_A:
> > +       case POWER_DOMAIN_TRANSCODER_EDP:
> > +               return;
> > +       case POWER_DOMAIN_PIPE_B:
> > +       case POWER_DOMAIN_PIPE_C:
> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
> > +       case POWER_DOMAIN_TRANSCODER_A:
> > +       case POWER_DOMAIN_TRANSCODER_B:
> > +       case POWER_DOMAIN_TRANSCODER_C:
> > +               spin_lock_irq(&power_well->lock);
> > +               WARN_ON(!power_well->count);
> > +               if (!--power_well->count)
> > +                       __intel_set_power_well(power_well->device, false);
> > +               spin_unlock_irq(&power_well->lock);
> > +               return;
> > +       default:
> > +               BUG();
> > +       }
> > +}
> > +
> >  static struct i915_power_well *hsw_pwr;
> >
> >  /* Display audio driver power well request */
> > @@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
> >                 return;
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> > -       if (!hsw_pwr->count++ &&
> > -                       !hsw_pwr->i915_request)
> > +       if (!hsw_pwr->count++)
> >                 __intel_set_power_well(hsw_pwr->device, true);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
> >
> >         spin_lock_irq(&hsw_pwr->lock);
> >         WARN_ON(!hsw_pwr->count);
> > -       if (!--hsw_pwr->count &&
> > -                      !hsw_pwr->i915_request)
> > +       if (!--hsw_pwr->count)
> >                 __intel_set_power_well(hsw_pwr->device, false);
> >         spin_unlock_irq(&hsw_pwr->lock);
> >  }
> > @@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
> >                 return;
> >
> >         spin_lock_irq(&power_well->lock);
> > +
> > +       /*
> > +        * This function will only ever contribute one
> > +        * to the power well reference count. i915_request
> > +        * is what tracks whether we have or have not
> > +        * added the one to the reference count.
> > +        */
> > +       if (power_well->i915_request == enable)
> > +               goto out;
> > +
> >         power_well->i915_request = enable;
> >
> > -       /* only reject "disable" power well request */
> > -       if (power_well->count && !enable) {
> > -               spin_unlock_irq(&power_well->lock);
> > -               return;
> 
> And now to the real problem of the patch: previously whenever we got a
> call to "enable" we'd call __intel_set_power_well and certainly write
> the register. Now with this patch we may not do this due to
> i915_request and the count. This breaks suspend/resume where just
> after we resume we call intel_set_power_well(dev, true) but then the
> new code doesn't really writes the register since i915_request is
> already true. As a consequence, we see "unclaimed register" messages
> complaining about registers 70008, 71008 and 72008. Perhaps in the
> resume path we should fix our tracking and force the "enable" somehow.

Hmm. I guess we anyway want to force the power well to be active during
resume regardless of where the refcount was left.

So maybe just a resume power well func or something:

intel_resume_power_well()
{
	if (!i915_request) {
		i915_request = true;
		count++;
	}
	__set_power_well(true);
}

> 
> 
> > +       if (enable) {
> > +               if (!power_well->count++)
> > +                       __intel_set_power_well(dev, true);
> > +       } else {
> > +               WARN_ON(!power_well->count);
> > +               if (!--power_well->count)
> > +                       __intel_set_power_well(dev, false);
> >         }
> >
> > -       __intel_set_power_well(dev, enable);
> > + out:
> >         spin_unlock_irq(&power_well->lock);
> >  }
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/5] drm/i915: Add intel_display_power_{get, put} to request power for specific domains

[Paulo Zanoni]

2013/9/13 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Fri, Sep 13, 2013 at 05:05:59PM -0300, Paulo Zanoni wrote:
>> Hi
>>
>> 2013/9/12  <ville.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Add APIs to get/put power well references for specific purposes.
>> >
>> > Also reorganize the internal i915_request power well handling to use the
>> > reference count just like everyone else. This way all we need to do is
>> > check the reference count and we know whether the power well needs to be
>> > enabled of disabled.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_drv.h |  4 ++
>> >  drivers/gpu/drm/i915/intel_pm.c  | 92 ++++++++++++++++++++++++++++++++++++----
>> >  2 files changed, 87 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 774ebb6..2ecd3d2 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -763,6 +763,10 @@ extern void i915_remove_power_well(struct drm_device *dev);
>> >
>> >  extern bool intel_display_power_enabled(struct drm_device *dev,
>> >                                         enum intel_display_power_domain domain);
>> > +extern void intel_display_power_get(struct drm_device *dev,
>> > +                                   enum intel_display_power_domain domain);
>> > +extern void intel_display_power_put(struct drm_device *dev,
>> > +                                   enum intel_display_power_domain domain);
>> >  extern void intel_init_power_well(struct drm_device *dev);
>> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index 8cffef4..4962303 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -5333,6 +5333,69 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>> >         }
>> >  }
>> >
>> > +void intel_display_power_get(struct drm_device *dev,
>> > +                            enum intel_display_power_domain domain)
>> > +{
>> > +       struct drm_i915_private *dev_priv = dev->dev_private;
>> > +       struct i915_power_well *power_well = &dev_priv->power_well;
>> > +
>> > +       if (!HAS_POWER_WELL(dev))
>> > +               return;
>> > +
>> > +       switch (domain) {
>> > +       case POWER_DOMAIN_PIPE_A:
>> > +       case POWER_DOMAIN_TRANSCODER_EDP:
>> > +               return;
>> > +       case POWER_DOMAIN_PIPE_B:
>> > +       case POWER_DOMAIN_PIPE_C:
>> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
>> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
>> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
>> > +       case POWER_DOMAIN_TRANSCODER_A:
>> > +       case POWER_DOMAIN_TRANSCODER_B:
>> > +       case POWER_DOMAIN_TRANSCODER_C:
>>
>> I know I'm the one who added all these domains, but I have to say I
>> only did this because of the reviewers, I don't really like the
>> interface. With your addition there's a new problem: you can get the
>> POWER_DOMAIN_PIPE_B interface and then put the
>> POWER_COMAIN_PIPE_C_PANEL_FITTER and no one will notice. I really
>> think the power well itself should be the domain. Also, in cases like
>> the suspend/resume code we don't have any domain that makes sense. But
>> what's *not* ugly about the power well code?
>>
>> I'm not suggesting you to fix that, I'm more kinda asking for ideas, I
>> may want to reorganize this code yet again when doing the D3 feature.
>> (Just because every single time we touch the power well code we have
>> to refactor it!)
>
> In other platforms we're going to have totally different mix of
> functional blocks vs. power wells. So assuming we want to deal with those
> using a unified API we do need something like this. But maybe there's a
> better way to go, haven't really thought about it.
>
>>
>>
>> > +               spin_lock_irq(&power_well->lock);
>> > +               if (!power_well->count++)
>> > +                       __intel_set_power_well(power_well->device, true);
>> > +               spin_unlock_irq(&power_well->lock);
>> > +               return;
>> > +       default:
>> > +               BUG();
>> > +       }
>> > +}
>> > +
>> > +void intel_display_power_put(struct drm_device *dev,
>> > +                            enum intel_display_power_domain domain)
>> > +{
>> > +       struct drm_i915_private *dev_priv = dev->dev_private;
>> > +       struct i915_power_well *power_well = &dev_priv->power_well;
>> > +
>> > +       if (!HAS_POWER_WELL(dev))
>> > +               return;
>> > +
>> > +       switch (domain) {
>> > +       case POWER_DOMAIN_PIPE_A:
>> > +       case POWER_DOMAIN_TRANSCODER_EDP:
>> > +               return;
>> > +       case POWER_DOMAIN_PIPE_B:
>> > +       case POWER_DOMAIN_PIPE_C:
>> > +       case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
>> > +       case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
>> > +       case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
>> > +       case POWER_DOMAIN_TRANSCODER_A:
>> > +       case POWER_DOMAIN_TRANSCODER_B:
>> > +       case POWER_DOMAIN_TRANSCODER_C:
>> > +               spin_lock_irq(&power_well->lock);
>> > +               WARN_ON(!power_well->count);
>> > +               if (!--power_well->count)
>> > +                       __intel_set_power_well(power_well->device, false);
>> > +               spin_unlock_irq(&power_well->lock);
>> > +               return;
>> > +       default:
>> > +               BUG();
>> > +       }
>> > +}
>> > +
>> >  static struct i915_power_well *hsw_pwr;
>> >
>> >  /* Display audio driver power well request */
>> > @@ -5342,8 +5405,7 @@ void i915_request_power_well(void)
>> >                 return;
>> >
>> >         spin_lock_irq(&hsw_pwr->lock);
>> > -       if (!hsw_pwr->count++ &&
>> > -                       !hsw_pwr->i915_request)
>> > +       if (!hsw_pwr->count++)
>> >                 __intel_set_power_well(hsw_pwr->device, true);
>> >         spin_unlock_irq(&hsw_pwr->lock);
>> >  }
>> > @@ -5357,8 +5419,7 @@ void i915_release_power_well(void)
>> >
>> >         spin_lock_irq(&hsw_pwr->lock);
>> >         WARN_ON(!hsw_pwr->count);
>> > -       if (!--hsw_pwr->count &&
>> > -                      !hsw_pwr->i915_request)
>> > +       if (!--hsw_pwr->count)
>> >                 __intel_set_power_well(hsw_pwr->device, false);
>> >         spin_unlock_irq(&hsw_pwr->lock);
>> >  }
>> > @@ -5394,15 +5455,28 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>> >                 return;
>> >
>> >         spin_lock_irq(&power_well->lock);
>> > +
>> > +       /*
>> > +        * This function will only ever contribute one
>> > +        * to the power well reference count. i915_request
>> > +        * is what tracks whether we have or have not
>> > +        * added the one to the reference count.
>> > +        */
>> > +       if (power_well->i915_request == enable)
>> > +               goto out;
>> > +
>> >         power_well->i915_request = enable;
>> >
>> > -       /* only reject "disable" power well request */
>> > -       if (power_well->count && !enable) {
>> > -               spin_unlock_irq(&power_well->lock);
>> > -               return;
>>
>> And now to the real problem of the patch: previously whenever we got a
>> call to "enable" we'd call __intel_set_power_well and certainly write
>> the register. Now with this patch we may not do this due to
>> i915_request and the count. This breaks suspend/resume where just
>> after we resume we call intel_set_power_well(dev, true) but then the
>> new code doesn't really writes the register since i915_request is
>> already true. As a consequence, we see "unclaimed register" messages
>> complaining about registers 70008, 71008 and 72008. Perhaps in the
>> resume path we should fix our tracking and force the "enable" somehow.
>
> Hmm. I guess we anyway want to force the power well to be active during
> resume regardless of where the refcount was left.
>
> So maybe just a resume power well func or something:
>
> intel_resume_power_well()
> {
>         if (!i915_request) {
>                 i915_request = true;
>                 count++;
>         }
>         __set_power_well(true);
> }

Yes, looks good.

>
>>
>>
>> > +       if (enable) {
>> > +               if (!power_well->count++)
>> > +                       __intel_set_power_well(dev, true);
>> > +       } else {
>> > +               WARN_ON(!power_well->count);
>> > +               if (!--power_well->count)
>> > +                       __intel_set_power_well(dev, false);
>> >         }
>> >
>> > -       __intel_set_power_well(dev, enable);
>> > + out:
>> >         spin_unlock_irq(&power_well->lock);
>> >  }
>> >
>> > --
>> > 1.8.1.5
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni

[PATCH 2/5] drm/i915: Refactor power well refcount inc/dec operations

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We increase/decrease the power well refcount in several places now, and
all of those places need to do the same thing, so pull that code into
a few small helper functions.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4962303..1aa604c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5333,6 +5333,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
 	}
 }
 
+static void __intel_inc_power_well_refcount(struct i915_power_well *power_well)
+{
+	if (!power_well->count++)
+		__intel_set_power_well(power_well->device, true);
+}
+
+static void __intel_dec_power_well_refcount(struct i915_power_well *power_well)
+{
+	WARN_ON(!power_well->count);
+	if (!--power_well->count)
+		__intel_set_power_well(power_well->device, false);
+}
+
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain)
 {
@@ -5355,8 +5368,7 @@ void intel_display_power_get(struct drm_device *dev,
 	case POWER_DOMAIN_TRANSCODER_B:
 	case POWER_DOMAIN_TRANSCODER_C:
 		spin_lock_irq(&power_well->lock);
-		if (!power_well->count++)
-			__intel_set_power_well(power_well->device, true);
+		__intel_inc_power_well_refcount(power_well);
 		spin_unlock_irq(&power_well->lock);
 		return;
 	default:
@@ -5386,9 +5398,7 @@ void intel_display_power_put(struct drm_device *dev,
 	case POWER_DOMAIN_TRANSCODER_B:
 	case POWER_DOMAIN_TRANSCODER_C:
 		spin_lock_irq(&power_well->lock);
-		WARN_ON(!power_well->count);
-		if (!--power_well->count)
-			__intel_set_power_well(power_well->device, false);
+		__intel_dec_power_well_refcount(power_well);
 		spin_unlock_irq(&power_well->lock);
 		return;
 	default:
@@ -5405,8 +5415,7 @@ void i915_request_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	if (!hsw_pwr->count++)
-		__intel_set_power_well(hsw_pwr->device, true);
+	__intel_inc_power_well_refcount(hsw_pwr);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_request_power_well);
@@ -5418,9 +5427,7 @@ void i915_release_power_well(void)
 		return;
 
 	spin_lock_irq(&hsw_pwr->lock);
-	WARN_ON(!hsw_pwr->count);
-	if (!--hsw_pwr->count)
-		__intel_set_power_well(hsw_pwr->device, false);
+	__intel_dec_power_well_refcount(hsw_pwr);
 	spin_unlock_irq(&hsw_pwr->lock);
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
@@ -5467,14 +5474,10 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 
 	power_well->i915_request = enable;
 
-	if (enable) {
-		if (!power_well->count++)
-			__intel_set_power_well(dev, true);
-	} else {
-		WARN_ON(!power_well->count);
-		if (!--power_well->count)
-			__intel_set_power_well(dev, false);
-	}
+	if (enable)
+		__intel_inc_power_well_refcount(power_well);
+	else
+		__intel_dec_power_well_refcount(power_well);
 
  out:
 	spin_unlock_irq(&power_well->lock);
-- 
1.8.1.5

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

Re: [PATCH 2/5] drm/i915: Refactor power well refcount inc/dec operations

[Paulo Zanoni]

2013/9/12  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We increase/decrease the power well refcount in several places now, and
> all of those places need to do the same thing, so pull that code into
> a few small helper functions.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

<bikeshed> I don't think we need those "__" in the beginning. </bikeshed>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4962303..1aa604c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5333,6 +5333,19 @@ static void __intel_set_power_well(struct drm_device *dev, bool enable)
>         }
>  }
>
> +static void __intel_inc_power_well_refcount(struct i915_power_well *power_well)
> +{
> +       if (!power_well->count++)
> +               __intel_set_power_well(power_well->device, true);
> +}
> +
> +static void __intel_dec_power_well_refcount(struct i915_power_well *power_well)
> +{
> +       WARN_ON(!power_well->count);
> +       if (!--power_well->count)
> +               __intel_set_power_well(power_well->device, false);
> +}
> +
>  void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain)
>  {
> @@ -5355,8 +5368,7 @@ void intel_display_power_get(struct drm_device *dev,
>         case POWER_DOMAIN_TRANSCODER_B:
>         case POWER_DOMAIN_TRANSCODER_C:
>                 spin_lock_irq(&power_well->lock);
> -               if (!power_well->count++)
> -                       __intel_set_power_well(power_well->device, true);
> +               __intel_inc_power_well_refcount(power_well);
>                 spin_unlock_irq(&power_well->lock);
>                 return;
>         default:
> @@ -5386,9 +5398,7 @@ void intel_display_power_put(struct drm_device *dev,
>         case POWER_DOMAIN_TRANSCODER_B:
>         case POWER_DOMAIN_TRANSCODER_C:
>                 spin_lock_irq(&power_well->lock);
> -               WARN_ON(!power_well->count);
> -               if (!--power_well->count)
> -                       __intel_set_power_well(power_well->device, false);
> +               __intel_dec_power_well_refcount(power_well);
>                 spin_unlock_irq(&power_well->lock);
>                 return;
>         default:
> @@ -5405,8 +5415,7 @@ void i915_request_power_well(void)
>                 return;
>
>         spin_lock_irq(&hsw_pwr->lock);
> -       if (!hsw_pwr->count++)
> -               __intel_set_power_well(hsw_pwr->device, true);
> +       __intel_inc_power_well_refcount(hsw_pwr);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_request_power_well);
> @@ -5418,9 +5427,7 @@ void i915_release_power_well(void)
>                 return;
>
>         spin_lock_irq(&hsw_pwr->lock);
> -       WARN_ON(!hsw_pwr->count);
> -       if (!--hsw_pwr->count)
> -               __intel_set_power_well(hsw_pwr->device, false);
> +       __intel_dec_power_well_refcount(hsw_pwr);
>         spin_unlock_irq(&hsw_pwr->lock);
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
> @@ -5467,14 +5474,10 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
>
>         power_well->i915_request = enable;
>
> -       if (enable) {
> -               if (!power_well->count++)
> -                       __intel_set_power_well(dev, true);
> -       } else {
> -               WARN_ON(!power_well->count);
> -               if (!--power_well->count)
> -                       __intel_set_power_well(dev, false);
> -       }
> +       if (enable)
> +               __intel_inc_power_well_refcount(power_well);
> +       else
> +               __intel_dec_power_well_refcount(power_well);
>
>   out:
>         spin_unlock_irq(&power_well->lock);
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/5] drm/i915: Refactor power well refcount inc/dec operations

[Chris Wilson]

On Fri, Sep 13, 2013 at 05:09:09PM -0300, Paulo Zanoni wrote:
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We increase/decrease the power well refcount in several places now, and
> > all of those places need to do the same thing, so pull that code into
> > a few small helper functions.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> <bikeshed> I don't think we need those "__" in the beginning. </bikeshed>

It would work better as __intel_power_well_get/put. Then it follows the
convention that you have a wrapper that does some sanity checks, takes
a locks and calls the core locked (unsafe) function, before unwrapping.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/5] drm/i915: Add POWER_DOMAIN_VGA

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VGA registers/memory live inside the the display power well. Add a power
domain for VGA.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/intel_pm.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 152291c..54b1966 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -99,6 +99,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_TRANSCODER_B,
 	POWER_DOMAIN_TRANSCODER_C,
 	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+	POWER_DOMAIN_VGA,
 };
 
 #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1aa604c..f08fe52 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5271,6 +5271,7 @@ bool intel_display_power_enabled(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return true;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5359,6 +5360,7 @@ void intel_display_power_get(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
@@ -5389,6 +5391,7 @@ void intel_display_power_put(struct drm_device *dev,
 	case POWER_DOMAIN_PIPE_A:
 	case POWER_DOMAIN_TRANSCODER_EDP:
 		return;
+	case POWER_DOMAIN_VGA:
 	case POWER_DOMAIN_PIPE_B:
 	case POWER_DOMAIN_PIPE_C:
 	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
-- 
1.8.1.5

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

Re: [PATCH 3/5] drm/i915: Add POWER_DOMAIN_VGA

[Paulo Zanoni]

2013/9/12  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VGA registers/memory live inside the the display power well. Add a power
> domain for VGA.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

(see comments in patch 1/5)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 1 +
>  drivers/gpu/drm/i915/intel_pm.c | 3 +++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 152291c..54b1966 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -99,6 +99,7 @@ enum intel_display_power_domain {
>         POWER_DOMAIN_TRANSCODER_B,
>         POWER_DOMAIN_TRANSCODER_C,
>         POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
> +       POWER_DOMAIN_VGA,
>  };
>
>  #define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1aa604c..f08fe52 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5271,6 +5271,7 @@ bool intel_display_power_enabled(struct drm_device *dev,
>         case POWER_DOMAIN_PIPE_A:
>         case POWER_DOMAIN_TRANSCODER_EDP:
>                 return true;
> +       case POWER_DOMAIN_VGA:
>         case POWER_DOMAIN_PIPE_B:
>         case POWER_DOMAIN_PIPE_C:
>         case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> @@ -5359,6 +5360,7 @@ void intel_display_power_get(struct drm_device *dev,
>         case POWER_DOMAIN_PIPE_A:
>         case POWER_DOMAIN_TRANSCODER_EDP:
>                 return;
> +       case POWER_DOMAIN_VGA:
>         case POWER_DOMAIN_PIPE_B:
>         case POWER_DOMAIN_PIPE_C:
>         case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> @@ -5389,6 +5391,7 @@ void intel_display_power_put(struct drm_device *dev,
>         case POWER_DOMAIN_PIPE_A:
>         case POWER_DOMAIN_TRANSCODER_EDP:
>                 return;
> +       case POWER_DOMAIN_VGA:
>         case POWER_DOMAIN_PIPE_B:
>         case POWER_DOMAIN_PIPE_C:
>         case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 4/5] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The init and resume codepaths want to handel the power well in slightly
different ways, so pull the power well init out from
intel_modeset_init_hw() which gets called in both cases.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      | 2 ++
 drivers/gpu/drm/i915/i915_drv.c      | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 2 --
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 9b265a4..e5c7b10 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
 
+	intel_init_power_well(dev);
+
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec690ca..cd5a66d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
 		/* We need working interrupts for modeset enabling ... */
 		drm_irq_install(dev);
 
+		intel_init_power_well(dev);
+
 		intel_modeset_init_hw(dev);
 
 		drm_modeset_lock_all(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d06e3b4..53f5a1f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10343,8 +10343,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
 
 void intel_modeset_init_hw(struct drm_device *dev)
 {
-	intel_init_power_well(dev);
-
 	intel_prepare_ddi(dev);
 
 	intel_init_clock_gating(dev);
-- 
1.8.1.5

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

Re: [PATCH 4/5] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[Paulo Zanoni]

2013/9/12  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The init and resume codepaths want to handel the power well in slightly
> different ways, so pull the power well init out from
> intel_modeset_init_hw() which gets called in both cases.

Can you please explain more? Where's the slight difference?

(also, I'm not sure if this code will change due to my comment in patch 1/5)

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      | 2 ++
>  drivers/gpu/drm/i915/i915_drv.c      | 2 ++
>  drivers/gpu/drm/i915/intel_display.c | 2 --
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 9b265a4..e5c7b10 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
>
> +       intel_init_power_well(dev);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index ec690ca..cd5a66d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
>                 /* We need working interrupts for modeset enabling ... */
>                 drm_irq_install(dev);
>
> +               intel_init_power_well(dev);
> +
>                 intel_modeset_init_hw(dev);
>
>                 drm_modeset_lock_all(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d06e3b4..53f5a1f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10343,8 +10343,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
>
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
> -       intel_init_power_well(dev);
> -
>         intel_prepare_ddi(dev);
>
>         intel_init_clock_gating(dev);
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: Pull intel_init_power_well() out of intel_modeset_init_hw()

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 05:19:34PM -0300, Paulo Zanoni wrote:
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The init and resume codepaths want to handel the power well in slightly
> > different ways, so pull the power well init out from
> > intel_modeset_init_hw() which gets called in both cases.
> 
> Can you please explain more? Where's the slight difference?

See patch 5. We want to keep the power well powered until we've finished
the vgacon->fbcon handoff, so we need to grab an extra power well
reference between intel_init_power_well() and first ->global_resources()
which would otherwise disable the power well.

> 
> (also, I'm not sure if this code will change due to my comment in patch 1/5)
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      | 2 ++
> >  drivers/gpu/drm/i915/i915_drv.c      | 2 ++
> >  drivers/gpu/drm/i915/intel_display.c | 2 --
> >  3 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index 9b265a4..e5c7b10 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1324,6 +1324,8 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >
> >         INIT_WORK(&dev_priv->console_resume_work, intel_console_resume);
> >
> > +       intel_init_power_well(dev);
> > +
> >         intel_modeset_gem_init(dev);
> >
> >         /* Always safe in the mode setting case. */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index ec690ca..cd5a66d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -596,6 +596,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
> >                 /* We need working interrupts for modeset enabling ... */
> >                 drm_irq_install(dev);
> >
> > +               intel_init_power_well(dev);
> > +
> >                 intel_modeset_init_hw(dev);
> >
> >                 drm_modeset_lock_all(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index d06e3b4..53f5a1f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10343,8 +10343,6 @@ void i915_disable_vga_mem(struct drm_device *dev)
> >
> >  void intel_modeset_init_hw(struct drm_device *dev)
> >  {
> > -       intel_init_power_well(dev);
> > -
> >         intel_prepare_ddi(dev);
> >
> >         intel_init_clock_gating(dev);
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

[PATCH 5/5] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

VGA registers live inside the power well on HSW, so in order to write
the VGA MSR register we need the power well to be on.

We really must write to the register to properly clear the
VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
the power well is down. It seems that the implicit zeroing done by
the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
whomever is actually responsible for the memory decode ranges.

If we leave VGA memory decode enabled, and then turn off the power well,
all VGA memory reads will return zeroes. But if we first disable VGA
memory deocde and then turn off the power well, VGA memory reads
return all ones, indicating that the access wasn't claimed by anyone.
For the vga arbiter to function correctly the IGD must not claim the
VGA memory accesses.

Previously we were doing the VGA_MSR register access while the power well
was excplicitly powered up during driver init. But ever since

 commit 6e1b4fdad5157bb9e88777d525704aba24389bee
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Thu Sep 5 20:40:52 2013 +0300

    drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done

we delay the VGA memory disable until fbcon has initialized, and so
there's a possibility that the power well got turned off during the
fbcon modeset. Also vgacon_save_screen() will need the power well to be
on to be able to read the VGA memory.

So immediately after enabling the power well during init grab a refence
for VGA purposes, and after all the VGA handling is done, release it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e5c7b10..23f965d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
 
 	intel_init_power_well(dev);
 
+	/* Keep VGA alive until i915_disable_vga_mem() */
+	intel_display_power_get(dev, POWER_DOMAIN_VGA);
+
 	intel_modeset_gem_init(dev);
 
 	/* Always safe in the mode setting case. */
@@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	 * vgacon_save_screen() works during the handover.
 	 */
 	i915_disable_vga_mem(dev);
+	intel_display_power_put(dev, POWER_DOMAIN_VGA);
 
 	/* Only enable hotplug handling once the fbdev is fully set up. */
 	dev_priv->enable_hotplug_processing = true;
-- 
1.8.1.5

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

Re: [PATCH 5/5] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Paulo Zanoni]

2013/9/12  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> VGA registers live inside the power well on HSW, so in order to write
> the VGA MSR register we need the power well to be on.
>
> We really must write to the register to properly clear the
> VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> the power well is down. It seems that the implicit zeroing done by
> the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> whomever is actually responsible for the memory decode ranges.

Due to the paragraph above, don't we also need to fix
i915_redisable_vga() ? If not, please explain why on the commit
message.

Don't we also need to do something about i915_enable_vga_mem()?


>
> If we leave VGA memory decode enabled, and then turn off the power well,
> all VGA memory reads will return zeroes. But if we first disable VGA
> memory deocde and then turn off the power well, VGA memory reads
> return all ones, indicating that the access wasn't claimed by anyone.
> For the vga arbiter to function correctly the IGD must not claim the
> VGA memory accesses.
>
> Previously we were doing the VGA_MSR register access while the power well
> was excplicitly powered up during driver init. But ever since
>
>  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Thu Sep 5 20:40:52 2013 +0300
>
>     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
>
> we delay the VGA memory disable until fbcon has initialized, and so
> there's a possibility that the power well got turned off during the
> fbcon modeset. Also vgacon_save_screen() will need the power well to be
> on to be able to read the VGA memory.
>
> So immediately after enabling the power well during init grab a refence
> for VGA purposes, and after all the VGA handling is done, release it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index e5c7b10..23f965d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>
>         intel_init_power_well(dev);
>
> +       /* Keep VGA alive until i915_disable_vga_mem() */

I'd write more here. Maybe copy the paragraph in the commit message.
This seems like an important thing and people won't really get the
idea if they don't git-blame.


> +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> +
>         intel_modeset_gem_init(dev);
>
>         /* Always safe in the mode setting case. */
> @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>          * vgacon_save_screen() works during the handover.
>          */
>         i915_disable_vga_mem(dev);
> +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
>
>         /* Only enable hotplug handling once the fbdev is fully set up. */
>         dev_priv->enable_hotplug_processing = true;
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/5] drm/i915: Fix unclaimed register access due to delayed VGA memroy disable

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 05:27:59PM -0300, Paulo Zanoni wrote:
> 2013/9/12  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > VGA registers live inside the power well on HSW, so in order to write
> > the VGA MSR register we need the power well to be on.
> >
> > We really must write to the register to properly clear the
> > VGA_MSR_MEM_EN enable bit, even if all VGA registers get zeroed when
> > the power well is down. It seems that the implicit zeroing done by
> > the power well is not enough to propagate the VGA_MSR_MEM_EN bit to
> > whomever is actually responsible for the memory decode ranges.
> 
> Due to the paragraph above, don't we also need to fix
> i915_redisable_vga() ? If not, please explain why on the commit
> message.
> 
> Don't we also need to do something about i915_enable_vga_mem()?

Yeah probably we should. For i915_enable_vga_mem() we could just power
the well back around the access if it's not already up.

The rediable_vga case is somewhat more problematic since we call it
during lid events. We'd always need to power the well back up just to
check if the BIOS was an idiot and re-enabled VGA. Maybe we can assume
that on HSW the BIOS isn't that stupid. For resume I guess we may want
to power it back up and do the redisable_vga thing. I'll take a closer
look at it next week.

> 
> 
> >
> > If we leave VGA memory decode enabled, and then turn off the power well,
> > all VGA memory reads will return zeroes. But if we first disable VGA
> > memory deocde and then turn off the power well, VGA memory reads
> > return all ones, indicating that the access wasn't claimed by anyone.
> > For the vga arbiter to function correctly the IGD must not claim the
> > VGA memory accesses.
> >
> > Previously we were doing the VGA_MSR register access while the power well
> > was excplicitly powered up during driver init. But ever since
> >
> >  commit 6e1b4fdad5157bb9e88777d525704aba24389bee
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Thu Sep 5 20:40:52 2013 +0300
> >
> >     drm/i915: Delay disabling of VGA memory until vgacon->fbcon handoff is done
> >
> > we delay the VGA memory disable until fbcon has initialized, and so
> > there's a possibility that the power well got turned off during the
> > fbcon modeset. Also vgacon_save_screen() will need the power well to be
> > on to be able to read the VGA memory.
> >
> > So immediately after enabling the power well during init grab a refence
> > for VGA purposes, and after all the VGA handling is done, release it.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e5c7b10..23f965d 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1326,6 +1326,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >
> >         intel_init_power_well(dev);
> >
> > +       /* Keep VGA alive until i915_disable_vga_mem() */
> 
> I'd write more here. Maybe copy the paragraph in the commit message.
> This seems like an important thing and people won't really get the
> idea if they don't git-blame.
> 
> 
> > +       intel_display_power_get(dev, POWER_DOMAIN_VGA);
> > +
> >         intel_modeset_gem_init(dev);
> >
> >         /* Always safe in the mode setting case. */
> > @@ -1358,6 +1361,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
> >          * vgacon_save_screen() works during the handover.
> >          */
> >         i915_disable_vga_mem(dev);
> > +       intel_display_power_put(dev, POWER_DOMAIN_VGA);
> >
> >         /* Only enable hotplug handling once the fbdev is fully set up. */
> >         dev_priv->enable_hotplug_processing = true;
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC