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

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

On 12/10/19 12:47 PM, Michal Wajdeczko wrote:
> 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.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++----
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++--
>   2 files changed, 63 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 c6519066a0f6..e3d1359f9719 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -12,6 +12,10 @@
>   
>   #include "i915_drv.h"
>   
> +extern const struct intel_uc_ops uc_ops_none;
> +extern const struct intel_uc_ops uc_ops_off;
> +extern const struct intel_uc_ops uc_ops_on;
> +
>   /* Reset GuC providing us with fresh state for both GuC and HuC.
>    */
>   static int __intel_uc_reset_hw(struct intel_uc *uc)
> @@ -89,6 +93,13 @@ 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 if (intel_uc_supports_guc(uc))
> +		uc->ops = &uc_ops_off;
> +	else
> +		uc->ops = &uc_ops_none;
>   }
>   
>   void intel_uc_driver_late_release(struct intel_uc *uc)
> @@ -413,24 +424,36 @@ 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)
> +{
> +	GEM_BUG_ON(!intel_uc_supports_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.
> +	 */
> +	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.
>   	 */
> -	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) ||
> @@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>   	return -EIO;
>   }
>   
> -void intel_uc_fini_hw(struct intel_uc *uc)
> +static void __uc_fini_hw(struct intel_uc *uc)
>   {
>   	struct intel_guc *guc = &uc->guc;
>   
> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>   	 */
>   	return __uc_resume(uc, true);
>   }
> +
> +const struct intel_uc_ops uc_ops_none = {
> +};
> +
> +const struct intel_uc_ops uc_ops_off = {
> +	.init_hw = __uc_check_hw,
> +};
> +

I'm not sold on having both uc_ops_off and uc_ops_none and I'd prefer 
them to be merged into one.
AFAICS, the only things you  do in uc_ops_off are __intel_uc_reset_hw 
and __uc_check_hw. __intel_uc_reset_hw shouldn't be needed, we do a GT 
reset when we come up and we're not touching the microcontrollers if 
intel_uc_uses_guc is false, so there should be no need to reset them. 
for __uc_check_hw, we can add an early return if !intel_uc_supports_guc 
so it is callable on all platforms and add it to uc_ops_none.

Daniele

> +const struct intel_uc_ops uc_ops_on = {
> +	.init_hw = __uc_init_hw,
> +	.fini_hw = __uc_fini_hw,
> +};
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 527995c21196..36643e17a09e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -10,7 +10,15 @@
>   #include "intel_huc.h"
>   #include "i915_params.h"
>   
> +struct intel_uc;
> +
> +struct intel_uc_ops {
> +	int (*init_hw)(struct intel_uc *uc);
> +	void (*fini_hw)(struct intel_uc *uc);
> +};
> +
>   struct intel_uc {
> +	struct intel_uc_ops const *ops;
>   	struct intel_guc guc;
>   	struct intel_huc huc;
>   
> @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc);
>   void intel_uc_cleanup_firmwares(struct intel_uc *uc);
>   void intel_uc_sanitize(struct intel_uc *uc);
>   void intel_uc_init(struct intel_uc *uc);
> -int intel_uc_init_hw(struct intel_uc *uc);
> -void intel_uc_fini_hw(struct intel_uc *uc);
>   void intel_uc_fini(struct intel_uc *uc);
>   void intel_uc_reset_prepare(struct intel_uc *uc);
>   void intel_uc_suspend(struct intel_uc *uc);
> @@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct intel_uc *uc)
>   	return intel_huc_is_enabled(&uc->huc);
>   }
>   
> +static inline int intel_uc_init_hw(struct intel_uc *uc)
> +{
> +	if (uc->ops->init_hw)
> +		return uc->ops->init_hw(uc);
> +	return 0;
> +}
> +
> +static inline void intel_uc_fini_hw(struct intel_uc *uc)
> +{
> +	if (uc->ops->fini_hw)
> +		uc->ops->fini_hw(uc);
> +}
> +
>   #endif
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx