drm/i915: Don't query PCODE RC6VIDS on platforms not supporting it

Submitted by Imre Deak on Feb. 8, 2018, 5:41 p.m.

Details

Message ID 20180208174102.10240-1-imre.deak@intel.com
State Accepted
Commit 51cc9adef064c8cf3df6d24e8662e9151529decf
Headers show
Series "drm/i915: Don't query PCODE RC6VIDS on platforms not supporting it" ( rev: 1 ) in Intel GFX

Browsing this patch as part of:
"drm/i915: Don't query PCODE RC6VIDS on platforms not supporting it" rev 1 in Intel GFX
<< prev patch [1/1] next patch >>

Commit Message

Imre Deak Feb. 8, 2018, 5:41 p.m.
On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
platforms. Do it only on SNB, IVB and HSW, where we use this command
anyway for RC6 enabling.

Based on my tests the command also succeeds on all LLC platforms, but
it's not clear if it's really supported on those (it returns 0 aka 245mv
for all RC6 states everywhere except on SNB). BSpec lists the command as
supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
incorrect, since on SKL/KBL the same command ID is used for
SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
command list is also incorrect for those platforms (see
P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).

I filed a request to update that info in Bspec, but for now let's
assume a minimal set of platforms where the command is supported.

Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2bdce9fea671..8f9045fae3cf 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1484,9 +1484,12 @@  static int gen6_drpc_info(struct seq_file *m)
 		gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
 	}
 
-	mutex_lock(&dev_priv->pcu_lock);
-	sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
-	mutex_unlock(&dev_priv->pcu_lock);
+	if (INTEL_GEN(dev_priv) <= 7) {
+		mutex_lock(&dev_priv->pcu_lock);
+		sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
+				       &rc6vids);
+		mutex_unlock(&dev_priv->pcu_lock);
+	}
 
 	seq_printf(m, "RC1e Enabled: %s\n",
 		   yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
@@ -1542,12 +1545,15 @@  static int gen6_drpc_info(struct seq_file *m)
 	print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
 	print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
 
-	seq_printf(m, "RC6   voltage: %dmV\n",
-		   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
-	seq_printf(m, "RC6+  voltage: %dmV\n",
-		   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
-	seq_printf(m, "RC6++ voltage: %dmV\n",
-		   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
+	if (INTEL_GEN(dev_priv) <= 7) {
+		seq_printf(m, "RC6   voltage: %dmV\n",
+			   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
+		seq_printf(m, "RC6+  voltage: %dmV\n",
+			   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
+		seq_printf(m, "RC6++ voltage: %dmV\n",
+			   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
+	}
+
 	return i915_forcewake_domains(m, NULL);
 }
 

Comments

Quoting Imre Deak (2018-02-08 17:41:02)
> On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> platforms. Do it only on SNB, IVB and HSW, where we use this command
> anyway for RC6 enabling.
> 
> Based on my tests the command also succeeds on all LLC platforms, but
> it's not clear if it's really supported on those (it returns 0 aka 245mv
> for all RC6 states everywhere except on SNB). BSpec lists the command as
> supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> incorrect, since on SKL/KBL the same command ID is used for
> SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> command list is also incorrect for those platforms (see
> P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
> 
> I filed a request to update that info in Bspec, but for now let's
> assume a minimal set of platforms where the command is supported.
> 
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2bdce9fea671..8f9045fae3cf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
>                 gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
>         }
>  
> -       mutex_lock(&dev_priv->pcu_lock);
> -       sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> -       mutex_unlock(&dev_priv->pcu_lock);
> +       if (INTEL_GEN(dev_priv) <= 7) {
> +               mutex_lock(&dev_priv->pcu_lock);
> +               sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> +                                      &rc6vids);
> +               mutex_unlock(&dev_priv->pcu_lock);
> +       }
>  
>         seq_printf(m, "RC1e Enabled: %s\n",
>                    yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
>         print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
>         print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
>  
> -       seq_printf(m, "RC6   voltage: %dmV\n",
> -                  GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> -       seq_printf(m, "RC6+  voltage: %dmV\n",
> -                  GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> -       seq_printf(m, "RC6++ voltage: %dmV\n",
> -                  GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> +       if (INTEL_GEN(dev_priv) <= 7) {

I expect this will confuse gcc-4.4 and it will emit a
maybe-used-uninitialised. I would set rc6vids=0 and then test if
(rc6vids) here.
-Chris
On Thu, Feb 08, 2018 at 06:33:03PM +0000, Chris Wilson wrote:
> Quoting Imre Deak (2018-02-08 17:41:02)
> > On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> > MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> > platforms. Do it only on SNB, IVB and HSW, where we use this command
> > anyway for RC6 enabling.
> > 
> > Based on my tests the command also succeeds on all LLC platforms, but
> > it's not clear if it's really supported on those (it returns 0 aka 245mv
> > for all RC6 states everywhere except on SNB). BSpec lists the command as
> > supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> > incorrect, since on SKL/KBL the same command ID is used for
> > SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> > command list is also incorrect for those platforms (see
> > P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
> > 
> > I filed a request to update that info in Bspec, but for now let's
> > assume a minimal set of platforms where the command is supported.
> > 
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 2bdce9fea671..8f9045fae3cf 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
> >                 gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
> >         }
> >  
> > -       mutex_lock(&dev_priv->pcu_lock);
> > -       sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > -       mutex_unlock(&dev_priv->pcu_lock);
> > +       if (INTEL_GEN(dev_priv) <= 7) {
> > +               mutex_lock(&dev_priv->pcu_lock);
> > +               sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> > +                                      &rc6vids);
> > +               mutex_unlock(&dev_priv->pcu_lock);
> > +       }
> >  
> >         seq_printf(m, "RC1e Enabled: %s\n",
> >                    yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> > @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
> >         print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
> >         print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
> >  
> > -       seq_printf(m, "RC6   voltage: %dmV\n",
> > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> > -       seq_printf(m, "RC6+  voltage: %dmV\n",
> > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> > -       seq_printf(m, "RC6++ voltage: %dmV\n",
> > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> > +       if (INTEL_GEN(dev_priv) <= 7) {
> 
> I expect this will confuse gcc-4.4 and it will emit a
> maybe-used-uninitialised. I would set rc6vids=0 and then test if
> (rc6vids) here.

It's inited already to 0, so shouldn't warn. (0 is a valid value
otherwise).

--Imre

> -Chris
On Thu, 08 Feb 2018, Imre Deak <imre.deak@intel.com> wrote:
> On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> platforms. Do it only on SNB, IVB and HSW, where we use this command
> anyway for RC6 enabling.
>
> Based on my tests the command also succeeds on all LLC platforms, but
> it's not clear if it's really supported on those (it returns 0 aka 245mv
> for all RC6 states everywhere except on SNB). BSpec lists the command as
> supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> incorrect, since on SKL/KBL the same command ID is used for
> SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> command list is also incorrect for those platforms (see
> P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
>
> I filed a request to update that info in Bspec, but for now let's
> assume a minimal set of platforms where the command is supported.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337

If the patch fixes the bug, please use Bugzilla:.

If the patch is related to the bug, please use References:.


BR,
Jani.


> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2bdce9fea671..8f9045fae3cf 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
>  		gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
>  	}
>  
> -	mutex_lock(&dev_priv->pcu_lock);
> -	sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> -	mutex_unlock(&dev_priv->pcu_lock);
> +	if (INTEL_GEN(dev_priv) <= 7) {
> +		mutex_lock(&dev_priv->pcu_lock);
> +		sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> +				       &rc6vids);
> +		mutex_unlock(&dev_priv->pcu_lock);
> +	}
>  
>  	seq_printf(m, "RC1e Enabled: %s\n",
>  		   yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
>  	print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
>  	print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
>  
> -	seq_printf(m, "RC6   voltage: %dmV\n",
> -		   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> -	seq_printf(m, "RC6+  voltage: %dmV\n",
> -		   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> -	seq_printf(m, "RC6++ voltage: %dmV\n",
> -		   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> +	if (INTEL_GEN(dev_priv) <= 7) {
> +		seq_printf(m, "RC6   voltage: %dmV\n",
> +			   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> +		seq_printf(m, "RC6+  voltage: %dmV\n",
> +			   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> +		seq_printf(m, "RC6++ voltage: %dmV\n",
> +			   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> +	}
> +
>  	return i915_forcewake_domains(m, NULL);
>  }
Quoting Imre Deak (2018-02-08 18:44:41)
> On Thu, Feb 08, 2018 at 06:33:03PM +0000, Chris Wilson wrote:
> > Quoting Imre Deak (2018-02-08 17:41:02)
> > > On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> > > MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> > > platforms. Do it only on SNB, IVB and HSW, where we use this command
> > > anyway for RC6 enabling.
> > > 
> > > Based on my tests the command also succeeds on all LLC platforms, but
> > > it's not clear if it's really supported on those (it returns 0 aka 245mv
> > > for all RC6 states everywhere except on SNB). BSpec lists the command as
> > > supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> > > incorrect, since on SKL/KBL the same command ID is used for
> > > SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> > > command list is also incorrect for those platforms (see
> > > P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
> > > 
> > > I filed a request to update that info in Bspec, but for now let's
> > > assume a minimal set of platforms where the command is supported.
> > > 
> > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 2bdce9fea671..8f9045fae3cf 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
> > >                 gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
> > >         }
> > >  
> > > -       mutex_lock(&dev_priv->pcu_lock);
> > > -       sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > > -       mutex_unlock(&dev_priv->pcu_lock);
> > > +       if (INTEL_GEN(dev_priv) <= 7) {
> > > +               mutex_lock(&dev_priv->pcu_lock);
> > > +               sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> > > +                                      &rc6vids);
> > > +               mutex_unlock(&dev_priv->pcu_lock);
> > > +       }
> > >  
> > >         seq_printf(m, "RC1e Enabled: %s\n",
> > >                    yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> > > @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
> > >         print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
> > >         print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
> > >  
> > > -       seq_printf(m, "RC6   voltage: %dmV\n",
> > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> > > -       seq_printf(m, "RC6+  voltage: %dmV\n",
> > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> > > -       seq_printf(m, "RC6++ voltage: %dmV\n",
> > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> > > +       if (INTEL_GEN(dev_priv) <= 7) {
> > 
> > I expect this will confuse gcc-4.4 and it will emit a
> > maybe-used-uninitialised. I would set rc6vids=0 and then test if
> > (rc6vids) here.
> 
> It's inited already to 0, so shouldn't warn. (0 is a valid value
> otherwise).

(Valid but not validated ;)

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
On Mon, Feb 12, 2018 at 11:19:09AM +0000, Chris Wilson wrote:
> Quoting Imre Deak (2018-02-08 18:44:41)
> > On Thu, Feb 08, 2018 at 06:33:03PM +0000, Chris Wilson wrote:
> > > Quoting Imre Deak (2018-02-08 17:41:02)
> > > > On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> > > > MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> > > > platforms. Do it only on SNB, IVB and HSW, where we use this command
> > > > anyway for RC6 enabling.
> > > > 
> > > > Based on my tests the command also succeeds on all LLC platforms, but
> > > > it's not clear if it's really supported on those (it returns 0 aka 245mv
> > > > for all RC6 states everywhere except on SNB). BSpec lists the command as
> > > > supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> > > > incorrect, since on SKL/KBL the same command ID is used for
> > > > SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> > > > command list is also incorrect for those platforms (see
> > > > P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
> > > > 
> > > > I filed a request to update that info in Bspec, but for now let's
> > > > assume a minimal set of platforms where the command is supported.
> > > > 
> > > > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
> > > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 2bdce9fea671..8f9045fae3cf 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
> > > >                 gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
> > > >         }
> > > >  
> > > > -       mutex_lock(&dev_priv->pcu_lock);
> > > > -       sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > > > -       mutex_unlock(&dev_priv->pcu_lock);
> > > > +       if (INTEL_GEN(dev_priv) <= 7) {
> > > > +               mutex_lock(&dev_priv->pcu_lock);
> > > > +               sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> > > > +                                      &rc6vids);
> > > > +               mutex_unlock(&dev_priv->pcu_lock);
> > > > +       }
> > > >  
> > > >         seq_printf(m, "RC1e Enabled: %s\n",
> > > >                    yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> > > > @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
> > > >         print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
> > > >         print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
> > > >  
> > > > -       seq_printf(m, "RC6   voltage: %dmV\n",
> > > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> > > > -       seq_printf(m, "RC6+  voltage: %dmV\n",
> > > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> > > > -       seq_printf(m, "RC6++ voltage: %dmV\n",
> > > > -                  GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> > > > +       if (INTEL_GEN(dev_priv) <= 7) {
> > > 
> > > I expect this will confuse gcc-4.4 and it will emit a
> > > maybe-used-uninitialised. I would set rc6vids=0 and then test if
> > > (rc6vids) here.
> > 
> > It's inited already to 0, so shouldn't warn. (0 is a valid value
> > otherwise).
> 
> (Valid but not validated ;)

True, so we don't know if the platforms returning 0 (with the PCODE
command otherwise succeeding) return an actually correct value. Checking
again it's only SNB where we use the value (so not even IVB/HSW) and
that's the only one returning a non-zero value. I hope someone will
follow-up on my bspec update request and clarify this.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
On Mon, Feb 12, 2018 at 12:33:57PM +0200, Jani Nikula wrote:
> On Thu, 08 Feb 2018, Imre Deak <imre.deak@intel.com> wrote:
> > On BXT/GLK GEN6_PCODE_READ_RC6VIDS fails with
> > MAILBOX_P24C_CC_ILLEGAL_CMD, so don't try to do the query on these
> > platforms. Do it only on SNB, IVB and HSW, where we use this command
> > anyway for RC6 enabling.
> >
> > Based on my tests the command also succeeds on all LLC platforms, but
> > it's not clear if it's really supported on those (it returns 0 aka 245mv
> > for all RC6 states everywhere except on SNB). BSpec lists the command as
> > supported on SKL+ (see P24C_PCODE_MAILBOX_INTERFACE) but that's clearly
> > incorrect, since on SKL/KBL the same command ID is used for
> > SKL_PCODE_LOAD_HDCP_KEYS. Since the command fails on BXT/GLK, the BSpec
> > command list is also incorrect for those platforms (see
> > P_CR_P24C_PCODE_MAILBOX_INTERFACE_0_2_0_GTTMMADR).
> >
> > I filed a request to update that info in Bspec, but for now let's
> > assume a minimal set of platforms where the command is supported.
> >
> > Reference: https://bugs.freedesktop.org/show_bug.cgi?id=103337
> 
> If the patch fixes the bug, please use Bugzilla:.
> 
> If the patch is related to the bug, please use References:.

It fixes a bug reported in that bug, but there are multiple issues
included in the report. So I suppose References: is the correct tag
then. 

> 
> 
> BR,
> Jani.
> 
> 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 2bdce9fea671..8f9045fae3cf 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1484,9 +1484,12 @@ static int gen6_drpc_info(struct seq_file *m)
> >  		gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
> >  	}
> >  
> > -	mutex_lock(&dev_priv->pcu_lock);
> > -	sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS, &rc6vids);
> > -	mutex_unlock(&dev_priv->pcu_lock);
> > +	if (INTEL_GEN(dev_priv) <= 7) {
> > +		mutex_lock(&dev_priv->pcu_lock);
> > +		sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
> > +				       &rc6vids);
> > +		mutex_unlock(&dev_priv->pcu_lock);
> > +	}
> >  
> >  	seq_printf(m, "RC1e Enabled: %s\n",
> >  		   yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> > @@ -1542,12 +1545,15 @@ static int gen6_drpc_info(struct seq_file *m)
> >  	print_rc6_res(m, "RC6+ residency since boot:", GEN6_GT_GFX_RC6p);
> >  	print_rc6_res(m, "RC6++ residency since boot:", GEN6_GT_GFX_RC6pp);
> >  
> > -	seq_printf(m, "RC6   voltage: %dmV\n",
> > -		   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> > -	seq_printf(m, "RC6+  voltage: %dmV\n",
> > -		   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> > -	seq_printf(m, "RC6++ voltage: %dmV\n",
> > -		   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> > +	if (INTEL_GEN(dev_priv) <= 7) {
> > +		seq_printf(m, "RC6   voltage: %dmV\n",
> > +			   GEN6_DECODE_RC6_VID(((rc6vids >> 0) & 0xff)));
> > +		seq_printf(m, "RC6+  voltage: %dmV\n",
> > +			   GEN6_DECODE_RC6_VID(((rc6vids >> 8) & 0xff)));
> > +		seq_printf(m, "RC6++ voltage: %dmV\n",
> > +			   GEN6_DECODE_RC6_VID(((rc6vids >> 16) & 0xff)));
> > +	}
> > +
> >  	return i915_forcewake_domains(m, NULL);
> >  }
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
On Fri, Feb 09, 2018 at 03:24:28AM +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Don't query PCODE RC6VIDS on platforms not supporting it
> URL   : https://patchwork.freedesktop.org/series/37936/
> State : success

Pushed it to -dinq with the s/Reference:/References:/ change. Thanks for
the review.

> 
> == Summary ==
> 
> Test kms_cursor_crc:
>         Subgroup cursor-64x64-offscreen:
>                 skip       -> PASS       (shard-apl)
> Test gem_exec_suspend:
>         Subgroup basic-s4:
>                 skip       -> FAIL       (shard-snb) fdo#103375
> Test drv_suspend:
>         Subgroup debugfs-reader:
>                 skip       -> PASS       (shard-snb) fdo#102365 +1
> Test gem_eio:
>         Subgroup in-flight-contexts:
>                 pass       -> FAIL       (shard-hsw) fdo#104676 +1
> Test kms_sysfs_edid_timing:
>                 warn       -> PASS       (shard-apl) fdo#100047
> Test kms_flip:
>         Subgroup 2x-plain-flip-fb-recreate-interruptible:
>                 fail       -> PASS       (shard-hsw)
> Test kms_frontbuffer_tracking:
>         Subgroup fbc-1p-primscrn-cur-indfb-draw-mmap-gtt:
>                 pass       -> FAIL       (shard-apl) fdo#101623
> Test perf_pmu:
>         Subgroup semaphore-wait-idle-bcs0:
>                 fail       -> PASS       (shard-apl) fdo#105011
> Test kms_cursor_legacy:
>         Subgroup cursor-vs-flip-legacy:
>                 fail       -> PASS       (shard-apl) fdo#103355
> Test kms_plane:
>         Subgroup plane-panning-bottom-right-suspend-pipe-b-planes:
>                 pass       -> SKIP       (shard-hsw) fdo#103540
> 
> fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
> fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
> fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676
> fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
> fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
> fdo#105011 https://bugs.freedesktop.org/show_bug.cgi?id=105011
> fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
> fdo#103540 https://bugs.freedesktop.org/show_bug.cgi?id=103540
> 
> shard-apl        total:3354 pass:1738 dwarn:1   dfail:0   fail:20  skip:1594 time:12341s
> shard-hsw        total:3444 pass:1759 dwarn:1   dfail:0   fail:11  skip:1672 time:11773s
> shard-snb        total:3444 pass:1350 dwarn:1   dfail:0   fail:11  skip:2082 time:6581s
> Blacklisted hosts:
> shard-kbl        total:3380 pass:1876 dwarn:1   dfail:0   fail:21  skip:1481 time:9422s
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7959/shards.html