Re: [Intel-gfx] [PATCH] drm/i915/gt: Reset twice

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

On 12/22/22 3:47 PM, Andi Shyti wrote:
> Hi GG,
> 
>>>>>>>>    drivers/gpu/drm/i915/gt/intel_reset.c | 34
>>>>>>>> ++++++++++++++++++++++-----
>>>>>>>>    1 file changed, 28 insertions(+), 6 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>>>> index ffde89c5835a4..88dfc0c5316ff 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>>>>>>>> @@ -268,6 +268,7 @@ static int ilk_do_reset(struct intel_gt *gt,
>>>>>>>> intel_engine_mask_t engine_mask,
>>>>>>>>    static int gen6_hw_domain_reset(struct intel_gt *gt, u32
>>>>>>>> hw_domain_mask)
>>>>>>>>    {
>>>>>>>>           struct intel_uncore *uncore = gt->uncore;
>>>>>>>> +       int loops = 2;
>>>>>>>>           int err;
>>>>>>>>           /*
>>>>>>>> @@ -275,18 +276,39 @@ static int gen6_hw_domain_reset(struct
>>>>>>>> intel_gt *gt, u32 hw_domain_mask)
>>>>>>>>            * for fifo space for the write or forcewake the chip for
>>>>>>>>            * the read
>>>>>>>>            */
>>>>>>>> -       intel_uncore_write_fw(uncore, GEN6_GDRST,
>>>>>>>> hw_domain_mask);
>>>>>>>> +       do {
>>>>>>>> +               intel_uncore_write_fw(uncore, GEN6_GDRST,
>>>>>>>> hw_domain_mask);
>>>>>>>> -       /* Wait for the device to ack the reset requests */
>>>>>>>> -       err = __intel_wait_for_register_fw(uncore,
>>>>>>>> -                                          GEN6_GDRST,
>>>>>>>> hw_domain_mask, 0,
>>>>>>>> -                                          500, 0,
>>>>>>>> -                                          NULL);
>>>>>>>> +               /*
>>>>>>>> +                * Wait for the device to ack the reset requests.
>>>>>>>> +                *
>>>>>>>> +                * On some platforms, e.g. Jasperlake, we see see
>>>>>>>> that the
>>>>>>>> +                * engine register state is not cleared until
>>>>>>>> shortly after
>>>>>>>> +                * GDRST reports completion, causing a failure as
>>>>>>>> we try
>>>>>>>> +                * to immediately resume while the internal state
>>>>>>>> is still
>>>>>>>> +                * in flux. If we immediately repeat the reset,
>>>>>>>> the second
>>>>>>>> +                * reset appears to serialise with the first, and
>>>>>>>> since
>>>>>>>> +                * it is a no-op, the registers should retain
>>>>>>>> their reset
>>>>>>>> +                * value. However, there is still a concern that
>>>>>>>> upon
>>>>>>>> +                * leaving the second reset, the internal engine
>>>>>>>> state
>>>>>>>> +                * is still in flux and not ready for resuming.
>>>>>>>> +                */
>>>>>>>> +               err = __intel_wait_for_register_fw(uncore,
>>>>>>>> GEN6_GDRST,
>>>>>>>> +
>>>>>>>> hw_domain_mask, 0,
>>>>>>>> +                                                  2000, 0,
>>>>>>>> +                                                  NULL);
> 
>> Andi, fast_timeout_us is increased from 500 to 2000, and if it fails, it
>> tries to reset it once more. How was this value of 2000 calculated?
> 
> No real reason, it's just an empiric choice to make the call a
> bit more robust and suffer less from delayed feedback.
> 
>>>>>>>> +       } while (err == 0 && --loops);
>>>>>>>>           if (err)
>>>>>>>>                   GT_TRACE(gt,
>>>>>>>>                            "Wait for 0x%08x engines reset
>>>>>>>> failed\n",
>>>>>>>>                            hw_domain_mask);
> 
>> Did GT_TRACE report an error in a situation where the problem was reported?
> 
> I guess so, in Jasperlake.
> 
>>>>>>>> +       /*
>>>>>>>> +        * As we have observed that the engine state is still
>>>>>>>> volatile
>>>>>>>> +        * after GDRST is acked, impose a small delay to let
>>>>>>>> everything settle.
>>>>>>>> +        */
>>>>>>>> +       udelay(50);
> 
>> udelay(50) affects all platforms that can call gen6_hw_domain_reset(), is
>> that intended?
> 
> Yes, that's intended as apparently we need to give it a bit more
> time for the engines to recover from the reset. We are here in
> atomic context and we need udelay to wait atomically, thus
> udelay().
> 
Hi Andi,

In scenarios/platforms where GSC Firmware is not used, reset through FLR 
is not possible and this reset function is used.
Therefore if this problem cannot be avoided by other WAs and this method 
is the only one, we might have to apply this patch as a temporal fix.
But we also ask the person who is in charge of this HW Platform 
(Jasperlake or all of GEN11?)to analyze the problem and you need to sure 
get proper WA guidance as a next step.

Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

Br,
G.G.
> Thank you,
> Andi