Re: [PATCH v2 2/2] drm/i915/guc: Reset GuC and retry on firmware load failure

[Arun Siluvery <arun.siluvery@linux.intel.com>]

On 10/03/2016 17:57, Yu Dai wrote:
> LGTM. Reviewed-by: Alex Dai <yu.dai@intel.com>
>
thanks for the review.
This patch is dependent on patch1 which is being reworked. I will rebase 
send this to the list again once all dependencies are resolved.

regards
Arun

> On 03/08/2016 03:38 AM, Arun Siluvery wrote:
>> Due to timing issues in the HW some of the status bits required for GuC
>> authentication doesn't get set occassionally, when that happens, GuC
>> cannot
>> be initialized and we will be left with a wedged GPU. The WA suggested is
>> to perform a soft reset of GuC and attempt to reload the fw again for few
>> times before giving up.
>>
>> As the failure is dependent on timing, tests performed by triggering
>> manual
>> full gpu reset (i915_wedged) showed that we could sometimes hit this
>> after
>> several thousand iterations but sometimes tests ran even longer
>> without any
>> issues. Reset and reload mechanism proved helpful when we indeed hit fw
>> load failure so it is better to include this to improve driver stability.
>>
>> This change implements the following WA,
>>
>> WaEnableuKernelHeaderValidFix:skl,bxt
>> WaEnableGuCBootHashCheckNotSet:skl,bxt
>>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         |  1 +
>>   drivers/gpu/drm/i915/i915_guc_reg.h     |  1 +
>>   drivers/gpu/drm/i915/i915_reg.h         |  1 +
>>   drivers/gpu/drm/i915/intel_guc_loader.c | 49
>> +++++++++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/intel_uncore.c     | 19 +++++++++++++
>>   5 files changed, 69 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index f37ac12..0df7c82 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2757,6 +2757,7 @@ extern long i915_compat_ioctl(struct file *filp,
>> unsigned int cmd,
>>   extern int intel_gpu_reset(struct drm_device *dev);
>>   extern bool intel_has_gpu_reset(struct drm_device *dev);
>>   extern int i915_reset(struct drm_device *dev);
>> +extern int intel_guc_reset(struct drm_i915_private *dev_priv);
>>   extern unsigned long i915_chipset_val(struct drm_i915_private
>> *dev_priv);
>>   extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
>>   extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index e4ba582..94ceee5 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -27,6 +27,7 @@
>>   /* Definitions of GuC H/W registers, bits, etc */
>>   #define GUC_STATUS            _MMIO(0xc000)
>> +#define   GS_MIA_IN_RESET        (1 << 0)
>>   #define   GS_BOOTROM_SHIFT        1
>>   #define   GS_BOOTROM_MASK          (0x7F << GS_BOOTROM_SHIFT)
>>   #define   GS_BOOTROM_RSA_FAILED          (0x50 << GS_BOOTROM_SHIFT)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 7dfc400..48a23de 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -164,6 +164,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t
>> reg)
>>   #define  GEN6_GRDOM_RENDER        (1 << 1)
>>   #define  GEN6_GRDOM_MEDIA        (1 << 2)
>>   #define  GEN6_GRDOM_BLT            (1 << 3)
>> +#define  GEN9_GRDOM_GUC            (1 << 5)
>>   #define RING_PP_DIR_BASE(ring)        _MMIO((ring)->mmio_base+0x228)
>>   #define RING_PP_DIR_BASE_READ(ring)    _MMIO((ring)->mmio_base+0x518)
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 82a3c03..f9cb814 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -353,6 +353,24 @@ static int guc_ucode_xfer(struct drm_i915_private
>> *dev_priv)
>>       return ret;
>>   }
>> +static int i915_reset_guc(struct drm_i915_private *dev_priv)
>> +{
>> +    int ret;
>> +    u32 guc_status;
>> +
>> +    ret = intel_guc_reset(dev_priv);
>> +    if (ret) {
>> +        DRM_ERROR("GuC reset failed, ret = %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    guc_status = I915_READ(GUC_STATUS);
>> +    WARN(!(guc_status & GS_MIA_IN_RESET),
>> +         "GuC status: 0x%x, MIA core expected to be in reset\n",
>> guc_status);
>> +
>> +    return ret;
>> +}
>> +
>>   /**
>>    * intel_guc_ucode_load() - load GuC uCode into the device
>>    * @dev:    drm device
>> @@ -417,9 +435,36 @@ int intel_guc_ucode_load(struct drm_device *dev)
>>       if (err)
>>           goto fail;
>> +    /*
>> +     * WaEnableuKernelHeaderValidFix:skl,bxt
>> +     * For BXT, this is only upto B0 but below WA is required for later
>> +     * steppings also so this is extended as well.
>> +     */
>> +    /* WaEnableGuCBootHashCheckNotSet:skl,bxt */
>>       err = guc_ucode_xfer(dev_priv);
>> -    if (err)
>> -        goto fail;
>> +    if (err) {
>> +        int retries = 3;
>> +
>> +        DRM_ERROR("GuC fw load failed, err=%d, attempting reset and
>> retry\n", err);
>> +
>> +        while (retries--) {
>> +            err = i915_reset_guc(dev_priv);
>> +            if (err)
>> +                break;
>> +
>> +            err = guc_ucode_xfer(dev_priv);
>> +            if (!err) {
>> +                DRM_DEBUG_DRIVER("GuC fw reload succeeded after
>> reset\n");
>> +                break;
>> +            }
>> +            DRM_DEBUG_DRIVER("GuC fw reload retries left: %d\n",
>> retries);
>> +        }
>> +
>> +        if (err) {
>> +            DRM_ERROR("GuC fw reload attempt failed, ret=%d\n", err);
>> +            goto fail;
>> +        }
>> +    }
>>       guc_fw->guc_fw_load_status = GUC_FIRMWARE_SUCCESS;
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
>> b/drivers/gpu/drm/i915/intel_uncore.c
>> index 80e38d5..92be76a 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -1647,6 +1647,25 @@ bool intel_has_gpu_reset(struct drm_device *dev)
>>       return intel_get_gpu_reset(dev) != NULL;
>>   }
>> +int intel_guc_reset(struct drm_i915_private *dev_priv)
>> +{
>> +    int ret;
>> +    unsigned long irqflags;
>> +
>> +    if (!i915.enable_guc_submission)
>> +        return -EINVAL;
>> +
>> +    intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> +    spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>> +
>> +    ret = gen6_domain_reset(dev_priv, GEN9_GRDOM_GUC);
>> +
>> +    spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
>> +    intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> +
>> +    return ret;
>> +}
>> +
>>   bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv)
>>   {
>>       return check_for_unclaimed_mmio(dev_priv);
>
>

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