Re: [Intel-gfx] [PATCH] drm/i915/guc: Request RP0 before loading firmware

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 12/21/2021 10:11 AM, Ewins, Jon wrote:
>
> On 12/20/2021 3:52 PM, Sundaresan, Sujaritha wrote:
>>
>> On 12/16/2021 3:30 PM, Vinay Belgaumkar wrote:
>>> By default, GT (and GuC) run at RPn. Requesting for RP0
>>> before firmware load can speed up DMA and HuC auth as well.
>>> In addition to writing to 0xA008, we also need to enable
>>> swreq in 0xA024 so that Punit will pay heed to our request.
>>>
>>> SLPC will restore the frequency back to RPn after initialization,
>>> but we need to manually do that for the non-SLPC path.
>>>
>>> We don't need a manual override in the SLPC disabled case, just
>>> use the intel_rps_set function to ensure consistent RPS state.
>>>
>>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_rps.c   | 59 
>>> +++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/gt/intel_rps.h   |  2 +
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c |  9 ++++
>>>   drivers/gpu/drm/i915/i915_reg.h       |  4 ++
>>>   4 files changed, 74 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c 
>>> b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> index 07ff7ba7b2b7..d576b34c7d6f 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
>>> @@ -2226,6 +2226,65 @@ u32 intel_rps_read_state_cap(struct intel_rps 
>>> *rps)
>>>           return intel_uncore_read(uncore, GEN6_RP_STATE_CAP);
>>>   }
>>>   +static void intel_rps_set_manual(struct intel_rps *rps, bool enable)
>>> +{
>>> +    struct intel_uncore *uncore = rps_to_uncore(rps);
>>> +    u32 state = enable ? GEN9_RPSWCTL_ENABLE : GEN9_RPSWCTL_DISABLE;
>>> +
>>> +    /* Allow punit to process software requests */
>>> +    intel_uncore_write(uncore, GEN6_RP_CONTROL, state);
>>> +}
>> Was there a specific reason to remove the set/clear timer functions ?
>
> Replying on behalf of Vinay Belguamkar:
>
> We are now using the intel_rps_set() function which handles more state 
> update in the correct rps path. We also obtain an rps lock which 
> guarantees not clobbering rps data.  The set/clear timers were being 
> done when we were modifying the frequency outside of the rps paths.  
> rps_set_manual is now only called in the SLPC path where the rps 
> timers are not even running.

Got it.

Reviewed-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>


>
>>
>>> +
>>> +void intel_rps_raise_unslice(struct intel_rps *rps)
>>> +{
>>> +    struct intel_uncore *uncore = rps_to_uncore(rps);
>>> +    u32 rp0_unslice_req;
>>> +
>>> +    mutex_lock(&rps->lock);
>>> +
>>> +    if (rps_uses_slpc(rps)) {
>>> +        /* RP limits have not been initialized yet for SLPC path */
>>> +        rp0_unslice_req = ((intel_rps_read_state_cap(rps) >> 0)
>>> +                   & 0xff) * GEN9_FREQ_SCALER;
>>> +
>>> +        intel_rps_set_manual(rps, true);
>>> +        intel_uncore_write(uncore, GEN6_RPNSWREQ,
>>> +                   ((rp0_unslice_req <<
>>> +                   GEN9_SW_REQ_UNSLICE_RATIO_SHIFT) |
>>> +                   GEN9_IGNORE_SLICE_RATIO));
>>> +        intel_rps_set_manual(rps, false);
>>> +    } else {
>>> +        intel_rps_set(rps, rps->rp0_freq);
>>> +    }
>>> +
>>> +    mutex_unlock(&rps->lock);
>>> +}
>>> +
>>> +void intel_rps_lower_unslice(struct intel_rps *rps)
>>> +{
>>> +    struct intel_uncore *uncore = rps_to_uncore(rps);
>>> +    u32 rpn_unslice_req;
>>> +
>>> +    mutex_lock(&rps->lock);
>>> +
>>> +    if (rps_uses_slpc(rps)) {
>>> +        /* RP limits have not been initialized yet for SLPC path */
>>> +        rpn_unslice_req = ((intel_rps_read_state_cap(rps) >> 16)
>>> +                   & 0xff) * GEN9_FREQ_SCALER;
>>> +
>>> +        intel_rps_set_manual(rps, true);
>>> +        intel_uncore_write(uncore, GEN6_RPNSWREQ,
>>> +                   ((rpn_unslice_req <<
>>> +                   GEN9_SW_REQ_UNSLICE_RATIO_SHIFT) |
>>> +                   GEN9_IGNORE_SLICE_RATIO));
>>> +        intel_rps_set_manual(rps, false);
>>> +    } else {
>>> +        intel_rps_set(rps, rps->min_freq);
>>> +    }
>>> +
>>> +    mutex_unlock(&rps->lock);
>>> +}
>>> +
>> Small function name nitpick maybe unslice_freq ? Just a suggestion.
>>>   /* External interface for intel_ips.ko */
>>>     static struct drm_i915_private __rcu *ips_mchdev;
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h 
>>> b/drivers/gpu/drm/i915/gt/intel_rps.h
>>> index aee12f37d38a..c6d76a3d1331 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_rps.h
>>> +++ b/drivers/gpu/drm/i915/gt/intel_rps.h
>>> @@ -45,6 +45,8 @@ u32 intel_rps_get_rpn_frequency(struct intel_rps 
>>> *rps);
>>>   u32 intel_rps_read_punit_req(struct intel_rps *rps);
>>>   u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
>>>   u32 intel_rps_read_state_cap(struct intel_rps *rps);
>>> +void intel_rps_raise_unslice(struct intel_rps *rps);
>>> +void intel_rps_lower_unslice(struct intel_rps *rps);
>>>     void gen5_rps_irq_handler(struct intel_rps *rps);
>>>   void gen6_rps_irq_handler(struct intel_rps *rps, u32 pm_iir);
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 2fef3b0bbe95..3693c4e7dad0 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -8,6 +8,7 @@
>>>   #include "intel_guc.h"
>>>   #include "intel_guc_ads.h"
>>>   #include "intel_guc_submission.h"
>>> +#include "gt/intel_rps.h"
>>>   #include "intel_uc.h"
>>>     #include "i915_drv.h"
>>> @@ -462,6 +463,8 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>       else
>>>           attempts = 1;
>>>   +    intel_rps_raise_unslice(&uc_to_gt(uc)->rps);
>>> +
>>>       while (attempts--) {
>>>           /*
>>>            * Always reset the GuC just before (re)loading, so
>>> @@ -499,6 +502,9 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>           ret = intel_guc_slpc_enable(&guc->slpc);
>>>           if (ret)
>>>               goto err_submission;
>>> +    } else {
>>> +        /* Restore GT back to RPn for non-SLPC path */
>>> +        intel_rps_lower_unslice(&uc_to_gt(uc)->rps);
>>>       }
>>>         drm_info(&i915->drm, "%s firmware %s version %u.%u %s:%s\n",
>>> @@ -529,6 +535,9 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>   err_log_capture:
>>>       __uc_capture_load_err_log(uc);
>>>   err_out:
>>> +    /* Return GT back to RPn */
>>> +    intel_rps_lower_unslice(&uc_to_gt(uc)->rps);
>>> +
>>>       __uc_sanitize(uc);
>>>         if (!ret) {
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index 1891e7fac39b..b2a86a26b843 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -9399,6 +9399,7 @@ enum {
>>>   #define   GEN6_OFFSET(x)            ((x) << 19)
>>>   #define   GEN6_AGGRESSIVE_TURBO            (0 << 15)
>>>   #define   GEN9_SW_REQ_UNSLICE_RATIO_SHIFT    23
>>> +#define   GEN9_IGNORE_SLICE_RATIO        (0 << 0)
>>>     #define GEN6_RC_VIDEO_FREQ            _MMIO(0xA00C)
>>>   #define GEN6_RC_CONTROL                _MMIO(0xA090)
>>> @@ -9434,6 +9435,9 @@ enum {
>>>   #define   GEN6_RP_UP_BUSY_CONT            (0x4 << 3)
>>>   #define   GEN6_RP_DOWN_IDLE_AVG            (0x2 << 0)
>>>   #define   GEN6_RP_DOWN_IDLE_CONT        (0x1 << 0)
>>> +#define   GEN6_RPSWCTL_SHIFT            9
>>> +#define   GEN9_RPSWCTL_ENABLE            (0x2 << GEN6_RPSWCTL_SHIFT)
>>> +#define   GEN9_RPSWCTL_DISABLE            (0x0 << GEN6_RPSWCTL_SHIFT)
>>>   #define GEN6_RP_UP_THRESHOLD            _MMIO(0xA02C)
>>>   #define GEN6_RP_DOWN_THRESHOLD            _MMIO(0xA030)
>>>   #define GEN6_RP_CUR_UP_EI            _MMIO(0xA050)
>>
>> Regards,
>>
>> Suja
>>