[PATCH 1/2] drm/i915: Tune done rc6 enabling output

[PATCH 1/2] drm/i915: Tune done rc6 enabling output

[Daniel Vetter]

Power users spot this and then get adventurous and try to adjust
module driver options. Nothing good ever came out of that, so
hide it better.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 338a80b6773e..4f879494e0c5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3425,10 +3425,10 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
 		else
 			mode = 0;
 	}
-	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
-		 (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
-		 (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
-		 (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
+	DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
+		      (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
+		      (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
+		      (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
 }
 
 static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
@@ -3452,8 +3452,8 @@ static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
 			mask = INTEL_RC6_ENABLE;
 
 		if ((enable_rc6 & mask) != enable_rc6)
-			DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid %d)\n",
-				 enable_rc6 & mask, enable_rc6, mask);
+			DRM_DEBUG_KMS("Adjusting RC6 mask to %d (requested %d, valid %d)\n",
+				      enable_rc6 & mask, enable_rc6, mask);
 
 		return enable_rc6 & mask;
 	}
-- 
2.0.1

[PATCH 2/2] drm/i915: Tune down MCH_SSKPD values warning

[Daniel Vetter]

Users often can't do anything about this since their vendors stopped
providing BIOS updates. Also we seem to be able to hack around it
with increased latency values, and thus far the only reports have
been for screens with really high resolutions. So tune it down to a
level where only developers can see it.

Also drop some of the end-user fluff.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f879494e0c5..684dc5f60544 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5235,11 +5235,9 @@ static void gen6_check_mch_setup(struct drm_device *dev)
 	uint32_t tmp;
 
 	tmp = I915_READ(MCH_SSKPD);
-	if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL) {
-		DRM_INFO("Wrong MCH_SSKPD value: 0x%08x\n", tmp);
-		DRM_INFO("This can cause pipe underruns and display issues.\n");
-		DRM_INFO("Please upgrade your BIOS to fix this.\n");
-	}
+	if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL)
+		DRM_DEBUG_KMS("Wrong MCH_SSKPD value: 0x%08x This can cause underruns.\n",
+			      tmp);
 }
 
 static void gen6_init_clock_gating(struct drm_device *dev)
-- 
2.0.1

Re: [PATCH 2/2] drm/i915: Tune down MCH_SSKPD values warning

[Ville Syrjälä]

On Mon, Aug 04, 2014 at 11:18:28AM +0200, Daniel Vetter wrote:
> Users often can't do anything about this since their vendors stopped
> providing BIOS updates. Also we seem to be able to hack around it
> with increased latency values, and thus far the only reports have
> been for screens with really high resolutions. So tune it down to a
> level where only developers can see it.
> 
> Also drop some of the end-user fluff.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Both patches make sense to me so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4f879494e0c5..684dc5f60544 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5235,11 +5235,9 @@ static void gen6_check_mch_setup(struct drm_device *dev)
>  	uint32_t tmp;
>  
>  	tmp = I915_READ(MCH_SSKPD);
> -	if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL) {
> -		DRM_INFO("Wrong MCH_SSKPD value: 0x%08x\n", tmp);
> -		DRM_INFO("This can cause pipe underruns and display issues.\n");
> -		DRM_INFO("Please upgrade your BIOS to fix this.\n");
> -	}
> +	if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL)
> +		DRM_DEBUG_KMS("Wrong MCH_SSKPD value: 0x%08x This can cause underruns.\n",
> +			      tmp);
>  }
>  
>  static void gen6_init_clock_gating(struct drm_device *dev)
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: Tune down MCH_SSKPD values warning

[Paulo Zanoni]

2014-08-04 12:03 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Mon, Aug 04, 2014 at 11:18:28AM +0200, Daniel Vetter wrote:
>> Users often can't do anything about this since their vendors stopped
>> providing BIOS updates. Also we seem to be able to hack around it
>> with increased latency values, and thus far the only reports have
>> been for screens with really high resolutions. So tune it down to a
>> level where only developers can see it.
>>
>> Also drop some of the end-user fluff.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Both patches make sense to me so:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I never really understood why we expect a specific value for a
specific field of the SSKPD on SNB. Isn't this value based on a lot of
different machine-dependent variables? On HSW this doesn't make sense
at all (and we don't have this check there, so the code is already
fine).

>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 4f879494e0c5..684dc5f60544 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5235,11 +5235,9 @@ static void gen6_check_mch_setup(struct drm_device *dev)
>>       uint32_t tmp;
>>
>>       tmp = I915_READ(MCH_SSKPD);
>> -     if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL) {
>> -             DRM_INFO("Wrong MCH_SSKPD value: 0x%08x\n", tmp);
>> -             DRM_INFO("This can cause pipe underruns and display issues.\n");
>> -             DRM_INFO("Please upgrade your BIOS to fix this.\n");
>> -     }
>> +     if ((tmp & MCH_SSKPD_WM0_MASK) != MCH_SSKPD_WM0_VAL)
>> +             DRM_DEBUG_KMS("Wrong MCH_SSKPD value: 0x%08x This can cause underruns.\n",
>> +                           tmp);
>>  }
>>
>>  static void gen6_init_clock_gating(struct drm_device *dev)
>> --
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 1/2] drm/i915: Tune done rc6 enabling output

[Paulo Zanoni]

2014-08-04 6:18 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Power users spot this and then get adventurous and try to adjust
> module driver options. Nothing good ever came out of that, so
> hide it better.

Agreed. Back when we were toggling the default behavior of RC6 at
every new -rc release, this was useful.

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

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 338a80b6773e..4f879494e0c5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3425,10 +3425,10 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
>                 else
>                         mode = 0;
>         }
> -       DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> -                (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> -                (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> -                (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> +       DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> +                     (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> +                     (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> +                     (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
>  }
>
>  static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
> @@ -3452,8 +3452,8 @@ static int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6)
>                         mask = INTEL_RC6_ENABLE;
>
>                 if ((enable_rc6 & mask) != enable_rc6)
> -                       DRM_INFO("Adjusting RC6 mask to %d (requested %d, valid %d)\n",
> -                                enable_rc6 & mask, enable_rc6, mask);
> +                       DRM_DEBUG_KMS("Adjusting RC6 mask to %d (requested %d, valid %d)\n",
> +                                     enable_rc6 & mask, enable_rc6, mask);
>
>                 return enable_rc6 & mask;
>         }
> --
> 2.0.1
>
> _______________________________________________
> 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: Tune done rc6 enabling output

[Chris Wilson]

On Mon, Aug 04, 2014 at 11:18:27AM +0200, Daniel Vetter wrote:
> Power users spot this and then get adventurous and try to adjust
> module driver options. Nothing good ever came out of that, so
> hide it better.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 338a80b6773e..4f879494e0c5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3425,10 +3425,10 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
>  		else
>  			mode = 0;
>  	}
> -	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> -		 (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> -		 (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> -		 (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> +	DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> +		      (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> +		      (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> +		      (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
>  }

I disagree. This is a good informative message about the capabilities
enabled for their GPU. If you want to make the statement that the user
shouldn't touch the values, make that adjustment emit a WARNING and
implement the kernel tainting you suggested. Also fixing [drm] to be
more specific would be useful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Tune done rc6 enabling output

[Daniel Vetter]

On Mon, Aug 04, 2014 at 04:34:28PM +0100, Chris Wilson wrote:
> On Mon, Aug 04, 2014 at 11:18:27AM +0200, Daniel Vetter wrote:
> > Power users spot this and then get adventurous and try to adjust
> > module driver options. Nothing good ever came out of that, so
> > hide it better.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 338a80b6773e..4f879494e0c5 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3425,10 +3425,10 @@ static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
> >  		else
> >  			mode = 0;
> >  	}
> > -	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> > -		 (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> > -		 (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> > -		 (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> > +	DRM_DEBUG_KMS("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
> > +		      (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
> > +		      (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
> > +		      (mode & GEN6_RC_CTL_RC6pp_ENABLE) ? "on" : "off");
> >  }
> 
> I disagree. This is a good informative message about the capabilities
> enabled for their GPU. If you want to make the statement that the user
> shouldn't touch the values, make that adjustment emit a WARNING and
> implement the kernel tainting you suggested. Also fixing [drm] to be
> more specific would be useful.

Well I'd agree if we'd generally report this, but we don't. This really is
the only feature where we tell the user in dmesg what we've done with it,
for a lot of other stuff (psr, fbc, rps ...) we just silently set stuff
up. rc6 really is the only thing and I don't see why it's special. Imo
users should use tools like powertop to assess the effectivenes or our pm
code anyway, not look at dmesg.

So this is orthogonal to the module param tainting which unfortunately
missed 3.17 since Jani (who took over) went on vacatino before completing
the revised patches.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch