Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sat, 11 Feb 2012 10:34:14 -0200, Eugeni Dodonov <eugeni.dodonov@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.dodonov@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