Re: [PATCH 1/2] drm/i915/guc: fix GuC suspend/resume

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 10/16/2018 3:26 AM, Michal Wajdeczko wrote:
> On Tue, 16 Oct 2018 00:10:08 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
>
>> The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC
>> FW and then return, so waiting on the H2H is not enough to guarantee
>> GuC is done.
>> When all the processing is done, GuC writes 0 to scratch register 14,
>> so we can poll on that. Note that GuC does not ensure that the value
>> in the register is different from 0 while the action is in progress
>> so we need to take care of that ourselves as well.
>>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_guc.c      | 28 +++++++++++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_guc_fwif.h |  6 ++++++
>>  2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index 230aea69385d..f238cd7a9dcf 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -521,6 +521,30 @@ int intel_guc_auth_huc(struct intel_guc *guc, 
>> u32 rsa_offset)
>>      return intel_guc_send(guc, action, ARRAY_SIZE(action));
>>  }
>> +/*
>> + * The ENTER/EXIT_S_STATE actions queue the save/restore operation 
>> in GuC FW and
>> + * then return, so waiting on the H2H is not enough to guarantee GuC 
>> is done.
>> + * When all the processing is done, GuC writes 0 to scratch register 
>> 14, so we
>
> s/writes 0/writes INTEL_GUC_SLEEP_STATE_SUCCESS ?
>
>> + * can poll on that. Note that GuC does not ensure that the value in 
>> the
>> + * register is different from 0 while the action is in progress so 
>> we need to
>> + * take care of that ourselves as well.
>> + */
>> +static int guc_sleep_state_action(struct intel_guc *guc,
>> +                  const u32 *action, u32 len)
>> +{
>> +    struct drm_i915_private *dev_priv = guc_to_i915(guc);
>> +    int ret;
>> +
>> +    I915_WRITE(SOFT_SCRATCH(14), ~0x0);
>
> Do we want to add dedicated name for that scratch register?

As I replied on your patch, the register is used for other purposes in 
other actions so I don't think it is a good idea to rename it.

>
> Also, as GuC is using scratch[14] then we need to remove it from our send
> register pool - patch is here [1]
>
>> +
>> +    ret = intel_guc_send(guc, action, len);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), ~0x0,
>> +                       INTEL_GUC_SLEEP_STATE_SUCCESS, 10);
>
> Maybe it is worth to use __intel_wait_for_register() and print sleep
> state error code in case of failure ? And do we really need to wait
> full 10ms for SUCCESS if GuC already has reported PREEMPT_TO_IDLE_FAILED
> or ENGINE_RESET_FAILED ?
>
>     u32 state;
>
>     ret = __intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), 
> 0x80000000,
>                         0, 10, &state);
>     if (ret)
>         return ret;
>     if (status != INTEL_GUC_SLEEP_STATE_SUCCESS) {
>         DRM_ERROR("... %u\n", state);
>         return -EIO;
>     }
>     return 0;
>

It should be impossible for us to get PREEMPT_TO_IDLE_FAILED or 
ENGINE_RESET_FAILED because those only come in play if guc_suspend() is 
called while there are outstanding request inside GuC. However, no real 
downsides in going with your solution either so I'll do the changes ;)

>> +}
>> +
>>  /**
>>   * intel_guc_suspend() - notify GuC entering suspend state
>>   * @guc:    the guc
>> @@ -533,7 +557,7 @@ int intel_guc_suspend(struct intel_guc *guc)
>>          intel_guc_ggtt_offset(guc, guc->shared_data)
>>      };
>> -    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> +    return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
>>  }
>> /**
>> @@ -571,7 +595,7 @@ int intel_guc_resume(struct intel_guc *guc)
>>          intel_guc_ggtt_offset(guc, guc->shared_data)
>>      };
>> -    return intel_guc_send(guc, data, ARRAY_SIZE(data));
>> +    return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
>>  }
>> /**
>> diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h 
>> b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> index 8382d591c784..b0eb5aabe0a7 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
>> @@ -687,6 +687,12 @@ enum intel_guc_report_status {
>>      INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
>>  };
>> +enum intel_guc_sleep_state_status {
>> +    INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0,
>> +    INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1,
>> +    INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2
>
> as we waiting for state change, maybe we should explicitly define
>     INTEL_GUC_SLEEP_STATE_INVALID_MASK = 0x80000000,
> and use it in __intel_wait_for_register ?

ack

>
>> +};
>> +
>>  #define GUC_LOG_CONTROL_LOGGING_ENABLED    (1 << 0)
>>  #define GUC_LOG_CONTROL_VERBOSITY_SHIFT    4
>>  #define GUC_LOG_CONTROL_VERBOSITY_MASK    (0xF << 
>> GUC_LOG_CONTROL_VERBOSITY_SHIFT)
>

Note for you while I'm it: I've been told that the gen11 GuC FW has a 
simplified and improved flow for suspend/resume, so some changes will be 
required in your series. Not sure about the details.

Thanks,
Daniele

> Thanks,
> Michal
>
> [1] https://patchwork.freedesktop.org/patch/256921/

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