Re: [PATCH 6/6] drm/i915/huc: Add BXT HuC Loading Support

Dave Gordon
2016-06-28T15:24:59Z

On 21/06/16 19:11, Peter Antoine wrote:
> This patch adds the HuC Loading for the BXT.
> Version 1.x of the HuC firmware.
>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 13 +++----------
> drivers/gpu/drm/i915/intel_guc_loader.c | 29 +++++++++++++++++------------
> drivers/gpu/drm/i915/intel_huc_loader.c | 7 +++++++
> 3 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 549dd3f..6abd5e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5143,16 +5143,9 @@ i915_gem_init_hw(struct drm_device *dev)
> intel_mocs_init_l3cc_table(dev);
>
> /* We can't enable contexts until all firmware is loaded */
> - if (HAS_GUC(dev)) {
> - /* init WOPCM */
> - I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev));
> - I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
> -
> - intel_huc_load(dev);
> - ret = intel_guc_setup(dev);
> - if (ret)
> - goto out;
> - }
> + ret = intel_guc_setup(dev);
> + if (ret)
> + goto out;

This reverts one of the earlier changes that I objected to!
Please squash them together so it isn't changed and then changed back.

> /*
> * Increment the next seqno by 0x100 so we have a visible break
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index e876a23f..289b5b6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -82,6 +82,17 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
> }
> };
>
> +u32 guc_wopcm_size(struct drm_device *dev)
> +{
> + u32 wopcm_size = GUC_WOPCM_TOP;
> +
> + /* On BXT, the top of WOPCM is reserved for RC6 context */
> + if (IS_BROXTON(dev))
> + wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> +
> + return wopcm_size;
> +}
> +
> static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
> {
> struct intel_engine_cs *engine;
> @@ -296,17 +307,6 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> -u32 guc_wopcm_size(struct drm_device *dev)
> -{
> - u32 wopcm_size = GUC_WOPCM_TOP;
> -
> - /* On BXT, the top of WOPCM is reserved for RC6 context */
> - if (IS_BROXTON(dev))
> - wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> -
> - return wopcm_size;
> -}
> -

This is just being moved around? Does it need to be?

> /*
> * Load the GuC firmware blob into the MinuteIA.
> */
> @@ -333,6 +333,10 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
>
> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>
> + /* init WOPCM */
> + I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev));
> + I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
> +

Is this being moved around (again?)
I think we should end up with GUC_WOPCM_SIZE being set just once.

> /* Enable MIA caching. GuC clock gating is disabled. */
> I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
>
> @@ -483,6 +487,7 @@ int intel_guc_setup(struct drm_device *dev)
> goto fail;
> }
>
> + intel_huc_load(dev);
> err = guc_ucode_xfer(dev_priv);
> if (!err)
> break;
> @@ -638,7 +643,7 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
> size = uc_fw->header_size + uc_fw->ucode_size;
>
> /* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
> - if (size > guc_wopcm_size(dev->dev_private)) {
> + if (size > guc_wopcm_size(dev)) {
> DRM_ERROR("Firmware is too large to fit in WOPCM\n");
> goto fail;
> }
> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
> index 7205e9e..7a43d4e 100644
> --- a/drivers/gpu/drm/i915/intel_huc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> @@ -49,6 +49,9 @@
> #define I915_SKL_HUC_UCODE "i915/skl_huc_ver1.bin"
> MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>
> +#define I915_BXT_HUC_UCODE "i915/bxt_huc_ver1.bin"
> +MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
> +
> /**
> * intel_huc_load_ucode() - DMA's the firmware
> * @dev: the drm device
> @@ -157,6 +160,10 @@ void intel_huc_init(struct drm_device *dev)
> fw_path = I915_SKL_HUC_UCODE;
> huc_fw->major_ver_wanted = 1;
> huc_fw->minor_ver_wanted = 5;
> + } else if (IS_BROXTON(dev)) {
> + fw_path = I915_BXT_HUC_UCODE;
> + huc_fw->major_ver_wanted = 1;
> + huc_fw->minor_ver_wanted = 0;
> }
>
> if (fw_path == NULL)

Overall: I think the general functionality is OK, but there are a few
specific points that need to be fixed (unless they already are, by a
later patch). In addition, the series need to be reorganised so that we
don't have the same lines of code repeatedly shuffled from one place to
another, or a change made and then reverted.

.Dave.