| Message ID | 1407143908-19196-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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; }
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(-)