Re: [Intel-gfx] [RFC] drm/i915/rc6: Access RC6 regs with forcewake

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 22-04-2022 21:04, Matt Roper wrote:
> On Fri, Apr 22, 2022 at 07:07:52PM +0530, Badal Nilawar wrote:
>> To access RC6 related MMIO regs use intel_uncore_write(), which grabs
>> forcewake if reg requires a forcewake.
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> index b4770690e794..9edaad3f19b5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
>> @@ -55,7 +55,7 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6 *rc)
>>   
>>   static void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val)
>>   {
>> -	intel_uncore_write_fw(uncore, reg, val);
>> +	intel_uncore_write(uncore, reg, val);
> 
> The set() function is primarily used within the *_rc6_enable() functions.
> Those are all called via intel_rc6_enable() which has already
> grabbed explicit forcewake before calling them:
> 
>          intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL);
> 
> so there's no need to grab an additional reference on every register
> access.  Likewise, the call in vlv_residency_raw() doesn't need
> forcewake because its own caller (intel_rc6_residency_ns) has already
> acquired it.
> 
> I think the call in intel_rc6_unpark() is the only one that might be
> questionable from a very quick skim of the code.  So rather than
> needlessly grabbing forcewake in all the other spots, it's probably
> better to just replace that single call with a direct call to
> intel_uncore_write() if it actually is problematic.
> 
> Even better, we might just want to drop the set() wrapper completely and
> replace all instances with the appropriate intel_uncore_write[_fw]
> calls; I don't really see the slightly shorter lines the wrapper gives
> us as being worth the deviation from the rest of the driver's code
> (especially if it's causing us confusion about what the intendended
> forcewake semantics are).
> 
Thanks for clarification.
The change in set() function was done for inte_rc6_unpark/park() only 
since 0xA090 RC6_CTRL is getting written without forcewake. For now as 
you suggested in inte_rc6_unpark/park() I will replace set() with 
intel_uncore_write().

Regards,
Badal
> 
> Matt
> 
>>   }
>>   
>>   static void gen11_rc6_enable(struct intel_rc6 *rc6)
>> -- 
>> 2.25.1
>>
>