[PATCH] drm/i915/rc6: Disable RPG during workload execution

[Nilawar, Badal]

On 23-10-2024 20:18, Rodrigo Vivi wrote:
> On Wed, Oct 23, 2024 at 11:03:57AM +0530, Nilawar, Badal wrote:
>>
>>
>> On 22-10-2024 22:39, Rodrigo Vivi wrote:
>>> On Tue, Oct 22, 2024 at 03:28:43PM +0200, Andi Shyti wrote:
>>>> Hi Badal,
>>>>
>>>> On Tue, Oct 22, 2024 at 06:52:26PM +0530, Badal Nilawar wrote:
>>>>> Encountering forcewake errors related to render power gating;
>>>>
>>>> Can you please expand your explanation here?
>>>
>>> yeap. More explanation please. All platforms? really?
>>
>> We are seeing Render forcewake timeouts on ADLP, ADLM, ADLN, TWL, DG1, rpl.
> 
> Is this a regression? or a new issue?

This is old issue, first reported year back.

> 
> Is this happening with Xe on these platforms? or i915 only?

i915 only. This is not reported on Xe kmd.

> 
>> Issue disappears after disabling RPG. Instead of fully disabling RPG I am
>> disabling it during active submissions i.e. during unpark.
>> For MTL and ARL RPG is already disabled permanently.
> 
> uhm. Interesting. Why that is disabled on these platforms? 

 From commit log its temporary wa to avoid fw timeouts.

perhaps we should be
> doing the same for all GuC enabled platforms?

I think so as temporary Wa.

> 
>>
>> Impact of doing this change should be performance improvement so kept for
>> all platform otherwise I will add platform check.
> 
> it could cause power consumption and battery life regressions. Better to filter
> per platform.

Sure, will filter per platform.

> 
>>
>> This is the issue https://gitlab.freedesktop.org/drm/intel/issues/9413. Will
>> add it in commit message.
> 
> Next time please include the relevant links in the commit msg.

Sure.

> 
> Thanks a lot for the info and for working on this,

Thanks,
Badal

> Rodrigo.
> 
>>
>> Regards,
>> Badal
>>
>>>
>>>>
>>>>> therefore, disable it during workload execution.
>>>>
>>>> ... and here.
>>>>
>>>>> Cc: Chris Wilson <chris.p.wilson at linux.intel.com>
>>>>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/gt/intel_rc6.c       | 18 +++++++++++++++++-
>>>>>    drivers/gpu/drm/i915/gt/intel_rc6_types.h |  1 +
>>>>>    2 files changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>>>> index c864d101faf9..459394ab5258 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>>>>> @@ -140,6 +140,7 @@ static void gen11_rc6_enable(struct intel_rc6 *rc6)
>>>>>    					      VDN_MFX_POWERGATE_ENABLE(i));
>>>>>    	}
>>>>> +	rc6->pg_enable = pg_enable;
>>>>
>>>> this looks borderline racy, it's fine only because this function
>>>> is called during resume which normally runs in atomic context.
>>>>
>>>>>    	intel_uncore_write_fw(uncore, GEN9_PG_ENABLE, pg_enable);
>>>>>    }
>>>>> @@ -572,8 +573,11 @@ static void __intel_rc6_disable(struct intel_rc6 *rc6)
>>>>>    	intel_guc_rc_disable(gt_to_guc(gt));
>>>>>    	intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
>>>>> -	if (GRAPHICS_VER(i915) >= 9)
>>>>> +	if (GRAPHICS_VER(i915) >= 9) {
>>>>> +		rc6->pg_enable = 0;
>>>>>    		intel_uncore_write_fw(uncore, GEN9_PG_ENABLE, 0);
>>>>> +	}
>>>>> +
>>>>>    	intel_uncore_write_fw(uncore, GEN6_RC_CONTROL, 0);
>>>>>    	intel_uncore_write_fw(uncore, GEN6_RC_STATE, 0);
>>>>>    	intel_uncore_forcewake_put(uncore, FORCEWAKE_ALL);
>>>>> @@ -687,6 +691,15 @@ void intel_rc6_unpark(struct intel_rc6 *rc6)
>>>>>    	/* Restore HW timers for automatic RC6 entry while busy */
>>>>>    	intel_uncore_write_fw(uncore, GEN6_RC_CONTROL, rc6->ctl_enable);
>>>>> +
>>>>> +	/*
>>>>> +	 * Seeing render forcewake timeouts during active submissions so disable render PG
>>>>> +	 * while workloads are under execution.
>>>>
>>>> Can you please improve this sentence? If I never new about the
>>>> issue I would be a bit confused.
>>>>
>>>>> +	 * FIXME Remove this change once real cause of render force wake timeout is fixed
>>>>> +	 */
>>>>> +	if (rc6->pg_enable == GEN9_RENDER_PG_ENABLE)
>>>>
>>>> is this supposed to be "pg_enable == GEN9_RENDER_PG_ENABLE" or
>>>> "pg_enable & GEN9_RENDER_PG_ENABLE" ?
>>>>
>>>> Andi
>>