Re: [PATCH 04/12] drm/i915/uc: introduce intel_uc_fw_supported

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Wed, 10 Jul 2019 02:54:29 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

> Instead of always checking in the device config is GuC and HuC are

s/is/if

> supported or not, we can save the state in the uc_fw structure and
> avoid going through i915 every time from the low-level uc management
> code. while at it FIRMWARE_NONE has been renamed to better indicate that

s/while/While

> we haven't started the fetch/load yet, but we might have already selected
> a blob.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_fw.c |  6 +++++-
>  drivers/gpu/drm/i915/intel_huc_fw.c |  6 +++++-
>  drivers/gpu/drm/i915/intel_uc.c     | 25 ++++++++++++------------
>  drivers/gpu/drm/i915/intel_uc_fw.c  |  4 +++-
>  drivers/gpu/drm/i915/intel_uc_fw.h  | 30 ++++++++++++++++++++++++-----
>  5 files changed, 51 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_fw.c  
> b/drivers/gpu/drm/i915/intel_guc_fw.c
> index db1e0daca7db..ee95d4960c5c 100644
> --- a/drivers/gpu/drm/i915/intel_guc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_guc_fw.c
> @@ -79,8 +79,12 @@ static void guc_fw_select(struct intel_uc_fw *guc_fw)
> 	GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
> -	if (!HAS_GUC(i915))
> +	if (!HAS_GUC(i915)) {
> +		guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  		return;
> +	}
> +
> +	guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> 	if (i915_modparams.guc_firmware_path) {
>  		guc_fw->path = i915_modparams.guc_firmware_path;
> diff --git a/drivers/gpu/drm/i915/intel_huc_fw.c  
> b/drivers/gpu/drm/i915/intel_huc_fw.c
> index 05cbf8338f53..06e726ba9863 100644
> --- a/drivers/gpu/drm/i915/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_huc_fw.c
> @@ -73,8 +73,12 @@ static void huc_fw_select(struct intel_uc_fw *huc_fw)
> 	GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
> -	if (!HAS_HUC(dev_priv))
> +	if (!HAS_HUC(dev_priv)) {
> +		huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  		return;
> +	}
> +
> +	huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> 	if (i915_modparams.huc_firmware_path) {
>  		huc_fw->path = i915_modparams.huc_firmware_path;
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 789b0bccfb41..ef2a864b8990 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -71,7 +71,8 @@ static int __get_default_guc_log_level(struct  
> drm_i915_private *i915)
>  {
>  	int guc_log_level;
> -	if (!HAS_GUC(i915) || !intel_uc_is_using_guc(i915))
> +	if (!intel_uc_fw_supported(&i915->guc.fw) ||

this goes too far, we should limit number of direct accesses to .fw
maybe we can have:

	inline bool intel_uc_has_guc(i915)
	{
		return intel_guc_is_present(&i915->guc);
	}

	inline bool intel_guc_is_present(guc)
	{
		return intel_uc_fw_is_defined(&guc->fw);
	}

> +	    !intel_uc_is_using_guc(i915))
>  		guc_log_level = GUC_LOG_LEVEL_DISABLED;
>  	else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
>  		 IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> @@ -119,16 +120,16 @@ static void sanitize_options_early(struct  
> drm_i915_private *i915)
>  	if (intel_uc_is_using_guc(i915) && !intel_uc_fw_is_selected(guc_fw)) {
>  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>  			 "enable_guc", i915_modparams.enable_guc,
> -			 !HAS_GUC(i915) ? "no GuC hardware" :
> -					  "no GuC firmware");
> +			 !intel_uc_fw_supported(guc_fw) ?
> +				"no GuC hardware" : "no GuC firmware");
>  	}
> 	/* Verify HuC firmware availability */
>  	if (intel_uc_is_using_huc(i915) && !intel_uc_fw_is_selected(huc_fw)) {
>  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>  			 "enable_guc", i915_modparams.enable_guc,
> -			 !HAS_HUC(i915) ? "no HuC hardware" :
> -					  "no HuC firmware");
> +			 !intel_uc_fw_supported(huc_fw) ?
> +				"no HuC hardware" : "no HuC firmware");
>  	}
> 	/* XXX: GuC submission is unavailable for now */
> @@ -148,8 +149,8 @@ static void sanitize_options_early(struct  
> drm_i915_private *i915)
>  	if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc(i915)) {
>  		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
>  			 "guc_log_level", i915_modparams.guc_log_level,
> -			 !HAS_GUC(i915) ? "no GuC hardware" :
> -					  "GuC not enabled");
> +			 !intel_uc_fw_supported(guc_fw) ?
> +				"no GuC hardware" : "GuC not enabled");
>  		i915_modparams.guc_log_level = 0;
>  	}
> @@ -376,7 +377,7 @@ int intel_uc_init(struct drm_i915_private *i915)
>  	if (!USES_GUC(i915))
>  		return 0;
> -	if (!HAS_GUC(i915))
> +	if (!intel_uc_fw_supported(&guc->fw))
>  		return -ENODEV;
> 	/* XXX: GuC submission is unavailable for now */
> @@ -419,7 +420,7 @@ void intel_uc_fini(struct drm_i915_private *i915)
>  	if (!USES_GUC(i915))
>  		return;
> -	GEM_BUG_ON(!HAS_GUC(i915));
> +	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> 	if (USES_GUC_SUBMISSION(i915))
>  		intel_guc_submission_fini(guc);
> @@ -435,7 +436,7 @@ static void __uc_sanitize(struct drm_i915_private  
> *i915)
>  	struct intel_guc *guc = &i915->guc;
>  	struct intel_huc *huc = &i915->huc;
> -	GEM_BUG_ON(!HAS_GUC(i915));
> +	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> 	intel_huc_sanitize(huc);
>  	intel_guc_sanitize(guc);
> @@ -460,7 +461,7 @@ int intel_uc_init_hw(struct drm_i915_private *i915)
>  	if (!USES_GUC(i915))
>  		return 0;
> -	GEM_BUG_ON(!HAS_GUC(i915));
> +	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> 	guc_reset_interrupts(guc);
> @@ -557,7 +558,7 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
>  	if (!intel_guc_is_loaded(guc))
>  		return;
> -	GEM_BUG_ON(!HAS_GUC(i915));
> +	GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> 	if (USES_GUC_SUBMISSION(i915))
>  		intel_guc_submission_disable(guc);
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index f342ddd47df8..8ce7210907c0 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -47,6 +47,8 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  	size_t size;
>  	int err;
> +	GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
> +
>  	if (!uc_fw->path) {
>  		dev_info(dev_priv->drm.dev,
>  			 "%s: No firmware was defined for %s!\n",
> @@ -328,7 +330,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw  
> *uc_fw)
>  	if (obj)
>  		i915_gem_object_put(obj);
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>  }
> /**
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index 24e66469153c..833d04d06576 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -26,6 +26,7 @@
>  #define _INTEL_UC_FW_H_
> #include <linux/types.h>
> +#include "i915_gem.h"
> struct drm_printer;
>  struct drm_i915_private;
> @@ -34,8 +35,10 @@ struct drm_i915_private;
>  #define INTEL_UC_FIRMWARE_URL  
> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915"
> enum intel_uc_fw_status {
> +	INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
>  	INTEL_UC_FIRMWARE_FAIL = -1,
> -	INTEL_UC_FIRMWARE_NONE = 0,
> +	INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks done too  
> early */
> +	INTEL_UC_FIRMWARE_NOT_STARTED = 1,
>  	INTEL_UC_FIRMWARE_PENDING,
>  	INTEL_UC_FIRMWARE_SUCCESS
>  };
> @@ -79,10 +82,14 @@ static inline
>  const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>  {
>  	switch (status) {
> +	case INTEL_UC_FIRMWARE_NOT_SUPPORTED:

maybe INTEL_UC_FIRMWARE_NOT_APPLICABLE, as used in string below?

> +		return "N/A - uc HW not available";

note that other states have short strings, so just "N/A" ?

>  	case INTEL_UC_FIRMWARE_FAIL:
>  		return "FAIL";
> -	case INTEL_UC_FIRMWARE_NONE:
> -		return "NONE";
> +	case INTEL_UC_FIRMWARE_UNINITIALIZED:
> +		return "UNINITIALIZED";
> +	case INTEL_UC_FIRMWARE_NOT_STARTED:
> +		return "NOT_STARTED";

NOT_STARTED and PENDING are not that much different,
see below

>  	case INTEL_UC_FIRMWARE_PENDING:
>  		return "PENDING";
>  	case INTEL_UC_FIRMWARE_SUCCESS:

maybe we should drop split on fetch/load and use
single status, something like:

      UNINITIALIZED --> N/A (no HW)
            |
         DEFINED ----> MISSING (no valid fw)
            |
      -> FETCHED/UNLOADED <-
     /       |              \
     \      LOADING         /
      \      /    \        /
       \- READY   FAILED -/


> @@ -106,9 +113,15 @@ static inline
>  void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>  			    enum intel_uc_fw_type type)
>  {
> +	/*
> +	 * we use FIRMWARE_UNINITIALIZED to detect checks against fetch_status
> +	 * before we're looked at the HW caps to see if we have uc support
> +	 */
> +	BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
> +
>  	uc_fw->path = NULL;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_NONE;
> -	uc_fw->load_status = INTEL_UC_FIRMWARE_NONE;
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
> +	uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>  	uc_fw->type = type;
>  }
> @@ -122,6 +135,13 @@ static inline bool intel_uc_fw_is_loaded(struct  
> intel_uc_fw *uc_fw)
>  	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
>  }
> +static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw)

returning "supported" in case of "no blob/no defs" might be misleading
maybe "defined" will clearer ?

> +{
> +	/* shouldn't call this before checking hw/blob availability */
> +	GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
> +	return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> +}
> +
>  static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
>  {
>  	if (intel_uc_fw_is_loaded(uc_fw))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx