[PATCH 1/2] drm/i915: Hold pc8 lock around toggling pc8.gpu_idle

[PATCH 1/2] drm/i915: Hold pc8 lock around toggling pc8.gpu_idle

[Chris Wilson]

We need to hold the pc8 lock around toggling the value of gpu_idle.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5ce1558..4fa1fd5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6546,18 +6546,22 @@ done:
 
 static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
 {
+	mutex_lock(&dev_priv->pc8.lock);
 	if (!dev_priv->pc8.gpu_idle) {
 		dev_priv->pc8.gpu_idle = true;
-		hsw_enable_package_c8(dev_priv);
+		__hsw_enable_package_c8(dev_priv);
 	}
+	mutex_unlock(&dev_priv->pc8.lock);
 }
 
 static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
 {
+	mutex_lock(&dev_priv->pc8.lock);
 	if (dev_priv->pc8.gpu_idle) {
 		dev_priv->pc8.gpu_idle = false;
-		hsw_disable_package_c8(dev_priv);
+		__hsw_disable_package_c8(dev_priv);
 	}
+	mutex_unlock(&dev_priv->pc8.lock);
 }
 
 static void haswell_modeset_global_resources(struct drm_device *dev)
-- 
1.7.9.5

[PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Chris Wilson]

If the hardware does not support package C8, then do not even schedule
work to enable it. Thereby we can eliminate a bunch of dangerous work.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7ba49d1..640bff2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1750,6 +1750,7 @@ struct drm_i915_file_private {
 #define HAS_POWER_WELL(dev)	(IS_HASWELL(dev))
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev))
+#define HAS_PC8(dev)		(IS_HASWELL(dev)) /* XXX HSW:ULX */
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4fa1fd5..2e9d75d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6479,6 +6479,9 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 
 void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_PC8(dev_priv->dev))
+		return;
+
 	mutex_lock(&dev_priv->pc8.lock);
 	__hsw_enable_package_c8(dev_priv);
 	mutex_unlock(&dev_priv->pc8.lock);
@@ -6486,6 +6489,9 @@ void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
 
 void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_PC8(dev_priv->dev))
+		return;
+
 	mutex_lock(&dev_priv->pc8.lock);
 	__hsw_disable_package_c8(dev_priv);
 	mutex_unlock(&dev_priv->pc8.lock);
@@ -6523,6 +6529,9 @@ static void hsw_update_package_c8(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool allow;
 
+	if (!HAS_PC8(dev_priv->dev))
+		return;
+
 	if (!i915_enable_pc8)
 		return;
 
@@ -6546,6 +6555,9 @@ done:
 
 static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_PC8(dev_priv->dev))
+		return;
+
 	mutex_lock(&dev_priv->pc8.lock);
 	if (!dev_priv->pc8.gpu_idle) {
 		dev_priv->pc8.gpu_idle = true;
@@ -6556,6 +6568,9 @@ static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
 
 static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
 {
+	if (!HAS_PC8(dev_priv->dev))
+		return;
+
 	mutex_lock(&dev_priv->pc8.lock);
 	if (dev_priv->pc8.gpu_idle) {
 		dev_priv->pc8.gpu_idle = false;
-- 
1.7.9.5

Re: [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Paulo Zanoni]

2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> If the hardware does not support package C8, then do not even schedule
> work to enable it. Thereby we can eliminate a bunch of dangerous work.

As I already explained, this should not be a problem since non-Haswell
platforms don't have a way to make the refcount become zero (unless we
have a bug). I also asked people's opinions about this specific
decision in one of my cover letters, but no one said anything:
http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html.

Quoting the email: "Another thing worth mentioning is that all this
code doesn't have IS_HASWELL checks, and on non-Haswell platforms the
refcount will never reach 0, so we won't ever try to enable PC8. I'm
not sure if that's what we want, so please comment on that.".

That said, I'm not against your changes.

But for completeness, you should probably add a WARN(!HAS_PC8()) at
haswell_enable_pc8_work().

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/intel_display.c |   15 +++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7ba49d1..640bff2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1750,6 +1750,7 @@ struct drm_i915_file_private {
>  #define HAS_POWER_WELL(dev)    (IS_HASWELL(dev))
>  #define HAS_FPGA_DBG_UNCLAIMED(dev)    (INTEL_INFO(dev)->has_fpga_dbg)
>  #define HAS_PSR(dev)           (IS_HASWELL(dev))
> +#define HAS_PC8(dev)           (IS_HASWELL(dev)) /* XXX HSW:ULX */

What exactly do you mean with this comment? Did you actually mean
"IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems
to work fine with this code, so I thought it wouldn't be a problem.


>
>  #define INTEL_PCH_DEVICE_ID_MASK               0xff00
>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE           0x3b00
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4fa1fd5..2e9d75d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6479,6 +6479,9 @@ static void __hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>
>  void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>  {
> +       if (!HAS_PC8(dev_priv->dev))
> +               return;
> +
>         mutex_lock(&dev_priv->pc8.lock);
>         __hsw_enable_package_c8(dev_priv);
>         mutex_unlock(&dev_priv->pc8.lock);
> @@ -6486,6 +6489,9 @@ void hsw_enable_package_c8(struct drm_i915_private *dev_priv)
>
>  void hsw_disable_package_c8(struct drm_i915_private *dev_priv)
>  {
> +       if (!HAS_PC8(dev_priv->dev))
> +               return;
> +
>         mutex_lock(&dev_priv->pc8.lock);
>         __hsw_disable_package_c8(dev_priv);
>         mutex_unlock(&dev_priv->pc8.lock);
> @@ -6523,6 +6529,9 @@ static void hsw_update_package_c8(struct drm_device *dev)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         bool allow;
>
> +       if (!HAS_PC8(dev_priv->dev))
> +               return;
> +
>         if (!i915_enable_pc8)
>                 return;
>
> @@ -6546,6 +6555,9 @@ done:
>
>  static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
>  {
> +       if (!HAS_PC8(dev_priv->dev))
> +               return;
> +
>         mutex_lock(&dev_priv->pc8.lock);
>         if (!dev_priv->pc8.gpu_idle) {
>                 dev_priv->pc8.gpu_idle = true;
> @@ -6556,6 +6568,9 @@ static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
>
>  static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>  {
> +       if (!HAS_PC8(dev_priv->dev))
> +               return;
> +
>         mutex_lock(&dev_priv->pc8.lock);
>         if (dev_priv->pc8.gpu_idle) {
>                 dev_priv->pc8.gpu_idle = false;
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Chris Wilson]

On Thu, Oct 10, 2013 at 05:17:31PM -0300, Paulo Zanoni wrote:
> 2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> > If the hardware does not support package C8, then do not even schedule
> > work to enable it. Thereby we can eliminate a bunch of dangerous work.
> 
> As I already explained, this should not be a problem since non-Haswell
> platforms don't have a way to make the refcount become zero (unless we
> have a bug). I also asked people's opinions about this specific
> decision in one of my cover letters, but no one said anything:
> http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html.
> 
> Quoting the email: "Another thing worth mentioning is that all this
> code doesn't have IS_HASWELL checks, and on non-Haswell platforms the
> refcount will never reach 0, so we won't ever try to enable PC8. I'm
> not sure if that's what we want, so please comment on that.".
> 
> That said, I'm not against your changes.

If they don't actually fix anything, they are low priority as they only
remove a mutex lock at most 10Hz. Maybe a comment would be good to remind
the next person that nothing gets enabled except on hsw.

> > +#define HAS_PC8(dev)           (IS_HASWELL(dev)) /* XXX HSW:ULX */
> 
> What exactly do you mean with this comment? Did you actually mean
> "IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems
> to work fine with this code, so I thought it wouldn't be a problem.

It means I didn't actually check the valid restrictions :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Paulo Zanoni]

2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> On Thu, Oct 10, 2013 at 05:17:31PM -0300, Paulo Zanoni wrote:
>> 2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
>> > If the hardware does not support package C8, then do not even schedule
>> > work to enable it. Thereby we can eliminate a bunch of dangerous work.
>>
>> As I already explained, this should not be a problem since non-Haswell
>> platforms don't have a way to make the refcount become zero (unless we
>> have a bug). I also asked people's opinions about this specific
>> decision in one of my cover letters, but no one said anything:
>> http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html.
>>
>> Quoting the email: "Another thing worth mentioning is that all this
>> code doesn't have IS_HASWELL checks, and on non-Haswell platforms the
>> refcount will never reach 0, so we won't ever try to enable PC8. I'm
>> not sure if that's what we want, so please comment on that.".
>>
>> That said, I'm not against your changes.
>
> If they don't actually fix anything, they are low priority as they only
> remove a mutex lock at most 10Hz. Maybe a comment would be good to remind
> the next person that nothing gets enabled except on hsw.
>
>> > +#define HAS_PC8(dev)           (IS_HASWELL(dev)) /* XXX HSW:ULX */
>>
>> What exactly do you mean with this comment? Did you actually mean
>> "IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems
>> to work fine with this code, so I thought it wouldn't be a problem.
>
> It means I didn't actually check the valid restrictions :)

Do we have plans for a V2 based on the comments? I wanted to use the
macro on a bug fix and discovered we didn't merge this yet.

Anyway, I can write follow-up patches for my own suggestions, so if we
merge V1 it's fine for me. Reviewed-by: Paulo Zanoni
<paulo.r.zanoni@intel.com>


> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni

Re: [PATCH 2/2] drm/i915: Do not enable package C8 on unsupported hardware

[Chris Wilson]

On Mon, Oct 28, 2013 at 02:55:02PM -0200, Paulo Zanoni wrote:
> 2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Thu, Oct 10, 2013 at 05:17:31PM -0300, Paulo Zanoni wrote:
> >> 2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> >> > If the hardware does not support package C8, then do not even schedule
> >> > work to enable it. Thereby we can eliminate a bunch of dangerous work.
> >>
> >> As I already explained, this should not be a problem since non-Haswell
> >> platforms don't have a way to make the refcount become zero (unless we
> >> have a bug). I also asked people's opinions about this specific
> >> decision in one of my cover letters, but no one said anything:
> >> http://lists.freedesktop.org/archives/intel-gfx/2013-August/031440.html.
> >>
> >> Quoting the email: "Another thing worth mentioning is that all this
> >> code doesn't have IS_HASWELL checks, and on non-Haswell platforms the
> >> refcount will never reach 0, so we won't ever try to enable PC8. I'm
> >> not sure if that's what we want, so please comment on that.".
> >>
> >> That said, I'm not against your changes.
> >
> > If they don't actually fix anything, they are low priority as they only
> > remove a mutex lock at most 10Hz. Maybe a comment would be good to remind
> > the next person that nothing gets enabled except on hsw.
> >
> >> > +#define HAS_PC8(dev)           (IS_HASWELL(dev)) /* XXX HSW:ULX */
> >>
> >> What exactly do you mean with this comment? Did you actually mean
> >> "IS_ULT()"? Even though only ULT has PC8-10 residencies, non-ULT seems
> >> to work fine with this code, so I thought it wouldn't be a problem.
> >
> > It means I didn't actually check the valid restrictions :)
> 
> Do we have plans for a V2 based on the comments? I wanted to use the
> macro on a bug fix and discovered we didn't merge this yet.

I was hoping you would take the hint and fix it up in an authorative
manner...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Hold pc8 lock around toggling pc8.gpu_idle

[Paulo Zanoni]

2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> We need to hold the pc8 lock around toggling the value of gpu_idle.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

Should we Cc:stable ?

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

I wonder if we need to rename hsw_enable_package_c8 and
__hsw_enable_package_c8 since we're spreading the usage of the "__"
function. Suggestions/patches welcome :)

> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5ce1558..4fa1fd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6546,18 +6546,22 @@ done:
>
>  static void hsw_package_c8_gpu_idle(struct drm_i915_private *dev_priv)
>  {
> +       mutex_lock(&dev_priv->pc8.lock);
>         if (!dev_priv->pc8.gpu_idle) {
>                 dev_priv->pc8.gpu_idle = true;
> -               hsw_enable_package_c8(dev_priv);
> +               __hsw_enable_package_c8(dev_priv);
>         }
> +       mutex_unlock(&dev_priv->pc8.lock);
>  }
>
>  static void hsw_package_c8_gpu_busy(struct drm_i915_private *dev_priv)
>  {
> +       mutex_lock(&dev_priv->pc8.lock);
>         if (dev_priv->pc8.gpu_idle) {
>                 dev_priv->pc8.gpu_idle = false;
> -               hsw_disable_package_c8(dev_priv);
> +               __hsw_disable_package_c8(dev_priv);
>         }
> +       mutex_unlock(&dev_priv->pc8.lock);
>  }
>
>  static void haswell_modeset_global_resources(struct drm_device *dev)
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/2] drm/i915: Hold pc8 lock around toggling pc8.gpu_idle

[Chris Wilson]

On Thu, Oct 10, 2013 at 05:04:27PM -0300, Paulo Zanoni wrote:
> 2013/10/10 Chris Wilson <chris@chris-wilson.co.uk>:
> > We need to hold the pc8 lock around toggling the value of gpu_idle.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Should we Cc:stable ?

Perhaps. Maybe this is a race condition that results in the enable_work
being kicked off...
 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I wonder if we need to rename hsw_enable_package_c8 and
> __hsw_enable_package_c8 since we're spreading the usage of the "__"
> function. Suggestions/patches welcome :)

__hsw_enable_package_c8 follows the idiom of being the inner locked
variant that one should only call if they know they meet the
preconditions. It keeps the longer name as a deterrent against use.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre