[PATCH 1/4] drm/i915/huc: Move firmware selection to early_init
Sagar Arun Kamble
sagar.a.kamble at intel.com
Thu Nov 30 06:40:26 UTC 2017
Functionality wise looks fine.
Suggesting minor style changes if you feel apt.
commit subject can say "early init" or "init_early"
On 11/29/2017 10:57 PM, Michal Wajdeczko wrote:
> Doing HuC firmware path selection from sanitize_options function
> is not perfect, while there is no problem with doing so during
> early init stage as we already have all needed data.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> drivers/gpu/drm/i915/intel_huc.c | 60 +++++++++++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_huc.h | 2 +-
> drivers/gpu/drm/i915/intel_uc.c | 4 +--
> 4 files changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bddd658..4c3616b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3232,6 +3232,9 @@ intel_info(const struct drm_i915_private *dev_priv)
> #define HAS_GUC_CT(dev_priv) ((dev_priv)->info.has_guc_ct)
> #define HAS_GUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
> #define HAS_GUC_SCHED(dev_priv) (HAS_GUC(dev_priv))
> +
> +/* For now, anything with a GuC has also HuC */
> +#define HAS_HUC(dev_priv) (HAS_GUC(dev_priv))
HAS_HUC|GUC_UCODE are of no use after this series so remove them in new patch?
> #define HAS_HUC_UCODE(dev_priv) (HAS_GUC(dev_priv))
>
> #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 98d1725..6d0e050 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -77,43 +77,57 @@ MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
> #define I915_GLK_HUC_UCODE HUC_FW_PATH(glk, GLK_HUC_FW_MAJOR, \
> GLK_HUC_FW_MINOR, GLK_BLD_NUM)
>
> -/**
> - * intel_huc_select_fw() - selects HuC firmware for loading
> - * @huc: intel_huc struct
> - */
> -void intel_huc_select_fw(struct intel_huc *huc)
> +static void huc_fw_select(struct intel_uc_fw *huc_fw)
> {
> + struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
> struct drm_i915_private *dev_priv = huc_to_i915(huc);
>
> - intel_uc_fw_init(&huc->fw, INTEL_UC_FW_TYPE_HUC);
> + GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
I feel this BUG_ON should be after HAS_HUC check although we init for non-HuC too.
Similarly for next patch. Rest all looks fine.
> +
> + if (!HAS_HUC(dev_priv))
> + return;
>
> if (i915_modparams.huc_firmware_path) {
> - huc->fw.path = i915_modparams.huc_firmware_path;
> - huc->fw.major_ver_wanted = 0;
> - huc->fw.minor_ver_wanted = 0;
> + huc_fw->path = i915_modparams.huc_firmware_path;
> + huc_fw->major_ver_wanted = 0;
> + huc_fw->minor_ver_wanted = 0;
> } else if (IS_SKYLAKE(dev_priv)) {
> - huc->fw.path = I915_SKL_HUC_UCODE;
> - huc->fw.major_ver_wanted = SKL_HUC_FW_MAJOR;
> - huc->fw.minor_ver_wanted = SKL_HUC_FW_MINOR;
> + huc_fw->path = I915_SKL_HUC_UCODE;
> + huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
> + huc_fw->minor_ver_wanted = SKL_HUC_FW_MINOR;
> } else if (IS_BROXTON(dev_priv)) {
> - huc->fw.path = I915_BXT_HUC_UCODE;
> - huc->fw.major_ver_wanted = BXT_HUC_FW_MAJOR;
> - huc->fw.minor_ver_wanted = BXT_HUC_FW_MINOR;
> + huc_fw->path = I915_BXT_HUC_UCODE;
> + huc_fw->major_ver_wanted = BXT_HUC_FW_MAJOR;
> + huc_fw->minor_ver_wanted = BXT_HUC_FW_MINOR;
> } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> - huc->fw.path = I915_KBL_HUC_UCODE;
> - huc->fw.major_ver_wanted = KBL_HUC_FW_MAJOR;
> - huc->fw.minor_ver_wanted = KBL_HUC_FW_MINOR;
> + huc_fw->path = I915_KBL_HUC_UCODE;
> + huc_fw->major_ver_wanted = KBL_HUC_FW_MAJOR;
> + huc_fw->minor_ver_wanted = KBL_HUC_FW_MINOR;
> } else if (IS_GEMINILAKE(dev_priv)) {
> - huc->fw.path = I915_GLK_HUC_UCODE;
> - huc->fw.major_ver_wanted = GLK_HUC_FW_MAJOR;
> - huc->fw.minor_ver_wanted = GLK_HUC_FW_MINOR;
> + huc_fw->path = I915_GLK_HUC_UCODE;
> + huc_fw->major_ver_wanted = GLK_HUC_FW_MAJOR;
> + huc_fw->minor_ver_wanted = GLK_HUC_FW_MINOR;
> } else {
> - DRM_ERROR("No HuC firmware known for platform with HuC!\n");
> - return;
> + DRM_WARN("%s: No firmware known for this platform!\n",
> + intel_uc_fw_type_repr(huc_fw->type));
> }
> }
>
> /**
> + * intel_huc_init_early() - initializes HuC struct
> + * @huc: intel_huc struct
> + *
> + * On platforms with HuC selects firmware for uploading
> + */
> +void intel_huc_init_early(struct intel_huc *huc)
> +{
> + struct intel_uc_fw *huc_fw = &huc->fw;
> +
> + intel_uc_fw_init(huc_fw, INTEL_UC_FW_TYPE_HUC);
> + huc_fw_select(huc_fw);
> +}
> +
> +/**
> * huc_ucode_xfer() - DMA's the firmware
> * @dev_priv: the drm_i915_private device
> *
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> index aaa38b9..3d757bc 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -34,7 +34,7 @@ struct intel_huc {
> /* HuC-specific additions */
> };
>
> -void intel_huc_select_fw(struct intel_huc *huc);
> +void intel_huc_init_early(struct intel_huc *huc);
> void intel_huc_init_hw(struct intel_huc *huc);
> void intel_huc_auth(struct intel_huc *huc);
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 1e2a30a..95b524c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -65,9 +65,6 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>
> /* Verify firmware version */
> if (i915_modparams.enable_guc_loading) {
> - if (HAS_HUC_UCODE(dev_priv))
> - intel_huc_select_fw(&dev_priv->huc);
> -
> if (intel_guc_fw_select(&dev_priv->guc))
> i915_modparams.enable_guc_loading = 0;
> }
> @@ -84,6 +81,7 @@ void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
> void intel_uc_init_early(struct drm_i915_private *dev_priv)
> {
> intel_guc_init_early(&dev_priv->guc);
> + intel_huc_init_early(&dev_priv->huc);
> }
>
> void intel_uc_init_fw(struct drm_i915_private *dev_priv)
More information about the Intel-gfx-trybot
mailing list