Re: [PATCH 2/2] drm/i915/bxt: Explicitly clear the Turbo control register

From: "Goel, Akash" <akash.goel@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: akash.goel@intel.com
Subject: Re: [PATCH 2/2] drm/i915/bxt: Explicitly clear the Turbo control register
Date: Thu, 21 Apr 2016 14:00:30 +0530	[thread overview]
Message-ID: <57188FA6.8070609@intel.com> (raw)
In-Reply-To: <20160421073756.GH15733@nuc-i3427.alporthouse.com>

On 4/21/2016 1:07 PM, Chris Wilson wrote:
> On Thu, Apr 21, 2016 at 12:33:41PM +0530, akash.goel@intel.com wrote:
>> From: Akash Goel <akash.goel@intel.com>
>>
>> As a part of WaGsvDisableTurbo, Driver makes an early exit from the
>> Gen9 Turbo enabling function, so doesn't program the Turbo Control register.
>> But BIOS could leave the Hw Turbo as enabled, so need to explicitly clear
>> out the Control register just to avoid inconsitency with debugfs
>> interface, which will show Turbo as enabled only and that is not expected
>> after adding the WaGsvDisableTurbo. Apart from this there is no problem
>> even if the Turbo is left enabled in the Control register, as the Up/Down
>> interrupts would remain masked.
>>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index b7c2186..41d5586 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4804,6 +4804,16 @@ static void gen9_enable_rps(struct drm_device *dev)
>>
>>   	/* WaGsvDisableTurbo: Workaround to disable turbo on BXT A* */
>>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
>> +		/*
>> +		 * BIOS could leave the Hw Turbo enabled, so need to explicitly
>> +		 * clear out the Control register just to avoid inconsitency
>> +		 * with debugfs interface, which will show  Turbo as enabled
>> +		 * only and that is not expected by the User after adding the
>> +		 * WaGsvDisableTurbo. Apart from this there is no problem even
>> +		 * if the Turbo is left enabled in the Control register, as the
>> +		 * Up/Down interrupts would remain masked.
>> +		 */
>> +		I915_WRITE(GEN6_RP_CONTROL, 0);
>
> First question. Do we not want to sanitize bios state for everybody
> during init? That is the usual procedure.
>
Sorry I am not sure. There is a sanitize for certain module parameters.
Recently for RC6 on BXT, sanitize was done depending upon the BIOS state.

> None of the *_disable_rps() actually clear GEN6_RP_CONTROL. They should
> according to the rationale above.  And then this should be a call to
> gen9_disable_rps().

Ok so will add the clearing of GEN6_RP_CONTROL in all *_disable_rps() 
and then call gen9_disable_rps().

Best regards
Akash

> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx