On Sat, 11 Feb 2012 10:34:14 -0200, Eugeni Dodonov <eugeni.dodo...@intel.com>
wrote:
> This allows to select which rc6 modes are to be used via kernel parameter,
> via a bitmask parameter. E.g.:
>
> - to enable rc6, i915_enable_rc6=1
> - to enable rc6 and deep rc6, i915_enable_rc6=3
> - to enable rc6 and deepest rc6, use i915_enable_rc6=5
> - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
>
> Please keep in mind that the deepest RC6 state really should NOT be used
> by default, as it could potentially worsen the issues with deep RC6. So do
> enable it only when you know what you are doing. However, having it around
> could help solving possible future rc6-related issues and their debugging
> on user machines.
>
> Note that this changes behavior - previously, value of 1 would enable both
> RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
> stages must be enabled manually.
>
> Signed-off-by: Eugeni Dodonov <eugeni.dodo...@intel.com>
A couple of nits, but the code does what it says on the tin. I was
going to give an r-b, but those nits seem to be growing...
> +/* RC6 modes */
If you're going to put a comment here, at least put a descriptive
comment. What is an rc mode, why should I care about the differences,
when should it be used, by whom and perhaps how?
> +#define INTEL_RC6_ENABLE (1<<0)
> +#define INTEL_RC6p_ENABLE (1<<1)
> +#define INTEL_RC6pp_ENABLE (1<<2)
> +
> - if (intel_enable_rc6(dev_priv->dev))
> - rc6_mask = GEN6_RC_CTL_RC6p_ENABLE |
> - GEN6_RC_CTL_RC6_ENABLE;
> + rc6_mode = intel_enable_rc6(dev_priv->dev);
> + /* Are we enabling rc6? */
/* This comment is pure fluff. Don't tell me what, tell me why, or rm! */
> + if (rc6_mode > 0) {
I'm a little uneasy mixing signs and bitmasks, and this test looks a
little pointless as intel_enable_rc6 now returns the bitmask. Kill it
and we kill one level of indentation!
> + if (rc6_mode & INTEL_RC6_ENABLE) {
> + DRM_DEBUG("i915: Enabling RC6\n");
No need to include i915 here, it will be added by DRM_DEBUG. I think
these are worthy of being INFO, if you can tidy them up into a single
string and further prettify them.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
