[Intel-gfx] [PATCH v2 1/4] drm/i915/uc: Add ops to intel_uc

[Michal Wajdeczko]

On Fri, 10 Jan 2020 17:57:22 +0100, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2020-01-10 16:29:27)
>> Instead of spreading multiple conditionals across the uC code
>> to find out current mode of uC operation, start using predefined
>> set of function pointers that reflect that mode.
>>
>> Begin with pair of init_hw/fini_hw functions that are responsible
>> for uC hardware initialization and cleanup.
>>
>> v2: drop ops_none, use macro to generate ops helpers
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.c | 45 ++++++++++++++++++++++-----
>>  drivers/gpu/drm/i915/gt/uc/intel_uc.h | 21 +++++++++++--
>>  2 files changed, 57 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 3ffc6267f96e..da401e97bba3 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -12,6 +12,19 @@
>>
>>  #include "i915_drv.h"
>>
>> +static int __uc_check_hw(struct intel_uc *uc);
>> +static int __uc_init_hw(struct intel_uc *uc);
>> +static void __uc_fini_hw(struct intel_uc *uc);
>> +
>> +static const struct intel_uc_ops uc_ops_off = {
>> +       .init_hw = __uc_check_hw,
>> +};
>> +
>> +static const struct intel_uc_ops uc_ops_on = {
>> +       .init_hw = __uc_init_hw,
>> +       .fini_hw = __uc_fini_hw,
>> +};
>> +
>>  /* Reset GuC providing us with fresh state for both GuC and HuC.
>>   */
>>  static int __intel_uc_reset_hw(struct intel_uc *uc)
>> @@ -89,6 +102,11 @@ void intel_uc_init_early(struct intel_uc *uc)
>>         intel_huc_init_early(&uc->huc);
>>
>>         __confirm_options(uc);
>> +
>> +       if (intel_uc_uses_guc(uc))
>> +               uc->ops = &uc_ops_on;
>> +       else
>> +               uc->ops = &uc_ops_off;
>>  }
>>
>>  void intel_uc_driver_late_release(struct intel_uc *uc)
>> @@ -380,24 +398,37 @@ static bool uc_is_wopcm_locked(struct intel_uc  
>> *uc)
>>                (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) &  
>> GUC_WOPCM_OFFSET_VALID);
>>  }
>>
>> -int intel_uc_init_hw(struct intel_uc *uc)
>> +static int __uc_check_hw(struct intel_uc *uc)
>> +{
>> +       if (!intel_uc_supports_guc(uc))
>> +               return 0;
>> +
>> +       /*
>> +        * We can silently continue without GuC only if it was never  
>> enabled
>> +        * before on this system after reboot, otherwise we risk GPU  
>> hangs.
>> +        * To check if GuC was loaded before we look at WOPCM registers.
>> +        */
>> +       if (uc_is_wopcm_locked(uc))
>> +               return -EIO;
>> +
>> +       return 0;
>> +}
>> +
>> +static int __uc_init_hw(struct intel_uc *uc)
>>  {
>>         struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>>         struct intel_guc *guc = &uc->guc;
>>         struct intel_huc *huc = &uc->huc;
>>         int ret, attempts;
>>
>> -       if (!intel_uc_supports_guc(uc))
>> -               return 0;
>> +       GEM_BUG_ON(!intel_uc_supports_guc(uc));
>> +       GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>
>>         /*
>>          * We can silently continue without GuC only if it was never  
>> enabled
>>          * before on this system after reboot, otherwise we risk GPU  
>> hangs.
>>          * To check if GuC was loaded before we look at WOPCM registers.
>>          */
>
> This comment still required?
> Shouldn't there be a call here to __uc_check_hw() instead?

ok, will replace below call to uc_is_wopcm_locked() with __uc_check_hw()
to avoid redundant (but otherwise still valid) comment

>
>> -       if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc))
>> -               return 0;
>> -
>>         if (!intel_uc_fw_is_available(&guc->fw)) {
>>                 ret = uc_is_wopcm_locked(uc) ||

                          ^^^^^^^^^^^^^^^^^^

>>                       intel_uc_fw_is_overridden(&guc->fw) ||
>> @@ -495,7 +526,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>         return -EIO;
>>  }