[3/8] drm/i915/huc: Add HuC fw loading support
diff mbox

Message ID 1481238139-9509-4-git-send-email-anusha.srivatsa@intel.com
State New
Headers show

Commit Message

Srivatsa, Anusha Dec. 8, 2016, 11:02 p.m. UTC
From: Anusha Srivatsa <anusha.srivatsa@intel.com>

The HuC loading process is similar to GuC. The intel_uc_fw_fetch()
is used for both cases.

HuC loading needs to be before GuC loading. The WOPCM setting must
be done early before loading any of them.

v2: rebased on-top of drm-intel-nightly.
    removed if(HAS_GUC()) before the guc call. (D.Gordon)
    update huc_version number of format.
v3: rebased to drm-intel-nightly, changed the file name format to
    match the one in the huc package.
    Changed dev->dev_private to to_i915()
v4: moved function back to where it was.
    change wait_for_atomic to wait_for.
v5: rebased + comment changes.
v7: rebased.
v8: rebased.
v9: rebased. Changed the year in the copyright message to reflect
the right year.Correct the comments,remove the unwanted WARN message,
replace drm_gem_object_unreference() with i915_gem_object_put().Make the
prototypes in intel_huc.h non-extern.
v10: rebased. Update the file construction done by HuC. It is similar to
GuC.Adopted the approach used in-
https://patchwork.freedesktop.org/patch/104355/ <Tvrtko Ursulin>
v11: Fix warnings remove old declaration
v12: Change dev to dev_priv in macro definition.
Corrected comments.
v13: rebased.
v14: rebased on top of drm-tip
v15: rebased. Updated functions intel_huc_load(),intel_huc_init() and
intel_uc_fw_fetch() to accept dev_priv instead of dev. Moved contents
of intel_huc.h to intel_uc.h

Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Tested-by: Xiang Haihao <haihao.xiang@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
Signed-off-by: Alex Dai <yu.dai@intel.com>
Signed-off-by: Peter Antoine <peter.antoine@intel.com>
---
 drivers/gpu/drm/i915/Makefile           |   1 +
 drivers/gpu/drm/i915/i915_drv.c         |   4 +-
 drivers/gpu/drm/i915/i915_drv.h         |   3 +-
 drivers/gpu/drm/i915/i915_guc_reg.h     |   3 +
 drivers/gpu/drm/i915/intel_guc_loader.c |   7 +-
 drivers/gpu/drm/i915/intel_huc_loader.c | 264 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h         |  22 +++
 7 files changed, 299 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_huc_loader.c

Comments

Arkadiusz Hiler Dec. 9, 2016, 10:56 a.m. UTC | #1
On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> The HuC loading process is similar to GuC. The intel_uc_fw_fetch()
> is used for both cases.
> 
> HuC loading needs to be before GuC loading. The WOPCM setting must
> be done early before loading any of them.
> 
> v2: rebased on-top of drm-intel-nightly.
>     removed if(HAS_GUC()) before the guc call. (D.Gordon)
>     update huc_version number of format.
> v3: rebased to drm-intel-nightly, changed the file name format to
>     match the one in the huc package.
>     Changed dev->dev_private to to_i915()
> v4: moved function back to where it was.
>     change wait_for_atomic to wait_for.
> v5: rebased + comment changes.
> v7: rebased.
> v8: rebased.
> v9: rebased. Changed the year in the copyright message to reflect
> the right year.Correct the comments,remove the unwanted WARN message,
> replace drm_gem_object_unreference() with i915_gem_object_put().Make the
> prototypes in intel_huc.h non-extern.
> v10: rebased. Update the file construction done by HuC. It is similar to
> GuC.Adopted the approach used in-
> https://patchwork.freedesktop.org/patch/104355/ <Tvrtko Ursulin>
> v11: Fix warnings remove old declaration
> v12: Change dev to dev_priv in macro definition.
> Corrected comments.
> v13: rebased.
> v14: rebased on top of drm-tip
> v15: rebased. Updated functions intel_huc_load(),intel_huc_init() and
> intel_uc_fw_fetch() to accept dev_priv instead of dev. Moved contents
> of intel_huc.h to intel_uc.h
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Tested-by: Xiang Haihao <haihao.xiang@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile           |   1 +
>  drivers/gpu/drm/i915/i915_drv.c         |   4 +-
>  drivers/gpu/drm/i915/i915_drv.h         |   3 +-
>  drivers/gpu/drm/i915/i915_guc_reg.h     |   3 +
>  drivers/gpu/drm/i915/intel_guc_loader.c |   7 +-
>  drivers/gpu/drm/i915/intel_huc_loader.c | 264 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |  22 +++
>  7 files changed, 299 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_huc_loader.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 3c30916..01d4f4b 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -57,6 +57,7 @@ i915-y += i915_cmd_parser.o \
>  # general-purpose microcontroller (GuC) support
>  i915-y += intel_uc.o \
>  	  intel_guc_loader.o \
> +	  intel_huc_loader.o \
>  	  i915_guc_submission.o
>  
>  # autogenerated null render state
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6428588..85a47c2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -600,6 +600,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> +	intel_huc_init(dev_priv);
>  	intel_guc_init(dev_priv);
>  
>  	ret = i915_gem_init(dev_priv);
> @@ -627,6 +628,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  	i915_gem_fini(dev_priv);
>  cleanup_irq:
> +	intel_huc_fini(dev);
>  	intel_guc_fini(dev_priv);
>  	drm_irq_uninstall(dev);
>  	intel_teardown_gmbus(dev_priv);
> @@ -1313,7 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	/* Flush any outstanding unpin_work. */
>  	drain_workqueue(dev_priv->wq);
> -
> +	intel_huc_fini(dev);
>  	intel_guc_fini(dev_priv);
>  	i915_gem_fini(dev_priv);
>  	intel_fbc_cleanup_cfb(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1480e73..0371ca4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2036,6 +2036,7 @@ struct drm_i915_private {
>  
>  	struct intel_gvt *gvt;
>  
> +	struct intel_huc huc;
>  	struct intel_guc guc;
>  
>  	struct intel_csr csr;
> @@ -2810,7 +2811,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>  #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -
> +#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
>  
>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 5e638fc..f9829f6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -61,9 +61,12 @@
>  #define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
>  #define DMA_COPY_SIZE			_MMIO(0xc310)
>  #define DMA_CTRL			_MMIO(0xc314)
> +#define   HUC_UKERNEL			  (1<<9)
>  #define   UOS_MOVE			  (1<<4)
>  #define   START_DMA			  (1<<0)
>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
> +#define   HUC_LOADING_AGENT_VCR		  (0<<1)
> +#define   HUC_LOADING_AGENT_GUC		  (1<<1)
>  #define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 26a184f..b971351 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -309,8 +309,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
>  
>  	/* Finally start the DMA */
> -	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
> -
> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA) |
> +		_MASKED_BIT_DISABLE(HUC_UKERNEL));
>  	/*
>  	 * Wait for the DMA to complete & the GuC to start up.
>  	 * NB: Docs recommend not using the interrupt for completion.
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>  
> @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		if (err)
>  			goto fail;
>  
> +		intel_huc_load(dev_priv);
>  		err = guc_ucode_xfer(dev_priv);
>  		if (!err)
>  			break;
> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
> new file mode 100644
> index 0000000..e0efd1c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> @@ -0,0 +1,264 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#include <linux/firmware.h>
> +#include "i915_drv.h"
> +#include "intel_uc.h"
> +
> +/**
> + * DOC: HuC Firmware
> + *
> + * Motivation:
> + * GEN9 introduces a new dedicated firmware for usage in media HEVC (High
> + * Efficiency Video Coding) operations. Userspace can use the firmware
> + * capabilities by adding HuC specific commands to batch buffers.
> + *
> + * Implementation:
> + * The same firmware loader is used as the GuC. However, the actual
> + * loading to HW is deferred until GEM initialization is done.
> + *
> + * Note that HuC firmware loading must be done before GuC loading.
> + */
> +
> +#define SKL_FW_MAJOR 01
> +#define SKL_FW_MINOR 07
> +#define SKL_BLD_NUM 1398
> +
> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
> +

Generally the patch looks well, but I'll withold my r-b till we have a
consensus on what parts of the huc version should we lock the kernel.

> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
> +	SKL_FW_MINOR, SKL_BLD_NUM)
> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
> +
> +/**
> + * huc_ucode_xfer() - DMA's the firmware
> + * @dev_priv: the drm device
> + *
> + * This function takes the gem object containing the firmware, sets up the DMA
> + * engine MMIO, triggers the DMA operation and waits for it to finish.
> + *
> + * Transfer the firmware image to RAM for execution by the microcontroller.
> + *
> + * Return: 0 on success, non-zero on failure
> + */
> +
> +static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +	struct i915_vma *vma;
> +	unsigned long offset = 0;
> +	u32 size;
> +	int ret;
> +
> +	ret = i915_gem_object_set_to_gtt_domain(huc_fw->uc_fw_obj, false);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	vma = i915_gem_object_ggtt_pin(huc_fw->uc_fw_obj, NULL, 0, 0, 0);
> +	if (IS_ERR(vma)) {
> +		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> +		return PTR_ERR(vma);
> +	}
> +
> +	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> +
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	/* init WOPCM */
> +	I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
> +	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE |
> +			HUC_LOADING_AGENT_GUC);
> +
> +	/* Set the source address for the uCode */
> +	offset = i915_ggtt_offset(vma) + huc_fw->header_offset;
> +	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> +	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
> +
> +	/* Hardware doesn't look at destination address for HuC. Set it to 0,
> +	 * but still program the correct address space.
> +	 */
> +	I915_WRITE(DMA_ADDR_1_LOW, 0);
> +	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> +	size = huc_fw->header_size + huc_fw->ucode_size;
> +	I915_WRITE(DMA_COPY_SIZE, size);
> +
> +	/* Start the DMA */
> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(HUC_UKERNEL | START_DMA));
> +
> +	/* Wait for DMA to finish */
> +	ret = wait_for((I915_READ(DMA_CTRL) & START_DMA) == 0, 100);
> +
> +	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
> +
> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	/*
> +	 * We keep the object pages for reuse during resume. But we can unpin it
> +	 * now that DMA has completed, so it doesn't continue to take up space.
> +	 */
> +	i915_vma_unpin(vma);
> +
> +	return ret;
> +}
> +
> +/**
> + * intel_huc_init() - initiate HuC firmware loading request
> + * @dev: the drm device
> + *
> + * Called early during driver load, but after GEM is initialised. The loading
> + * will continue only when driver explicitly specify firmware name and version.
> + * All other cases are considered as UC_FIRMWARE_NONE either because HW is not
> + * capable or driver yet support it. And there will be no error message for
> + * UC_FIRMWARE_NONE cases.
> + *
> + * The DMA-copying to HW is done later when intel_huc_load() is called.
> + */
> +void intel_huc_init(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_huc *huc = &dev_priv->huc;
> +	struct intel_uc_fw *huc_fw = &huc->huc_fw;
> +	const char *fw_path = NULL;
> +
> +	huc_fw->uc_fw_path = NULL;
> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
> +	huc_fw->load_status = UC_FIRMWARE_NONE;
> +	huc_fw->fw_type = UC_FW_TYPE_HUC;
> +
> +	if (!HAS_HUC_UCODE(dev_priv))
> +		return;
> +
> +	if (IS_SKYLAKE(dev_priv)) {
> +		fw_path = I915_SKL_HUC_UCODE;
> +		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
> +		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
> +	}
> +
> +	huc_fw->uc_fw_path = fw_path;
> +	huc_fw->fetch_status = UC_FIRMWARE_PENDING;
> +
> +	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
> +
> +	intel_uc_fw_fetch(dev_priv, huc_fw);
> +}
> +
> +/**
> + * intel_huc_load() - load HuC uCode to device
> + * @dev: the drm device
> + *
> + * Called from gem_init_hw() during driver loading and also after a GPU reset.
> + * Be note that HuC loading must be done before GuC loading.
> + *
> + * The firmware image should have already been fetched into memory by the
> + * earlier call to intel_huc_init(), so here we need only check that
> + * is succeeded, and then transfer the image to the h/w.
> + *
> + * Return:	non-zero code on error
> + */
> +int intel_huc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +	int err;
> +
> +	if (huc_fw->fetch_status == UC_FIRMWARE_NONE)
> +		return 0;
> +
> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> +		huc_fw->uc_fw_path,
> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> +		intel_uc_fw_status_repr(huc_fw->load_status));
> +
> +	if (huc_fw->fetch_status == UC_FIRMWARE_SUCCESS &&
> +	    huc_fw->load_status == UC_FIRMWARE_FAIL)
> +		return -ENOEXEC;
> +
> +	huc_fw->load_status = UC_FIRMWARE_PENDING;
> +
> +	switch (huc_fw->fetch_status) {
> +	case UC_FIRMWARE_FAIL:
> +		/* something went wrong :( */
> +		err = -EIO;
> +		goto fail;
> +
> +	case UC_FIRMWARE_NONE:
> +	case UC_FIRMWARE_PENDING:
> +	default:
> +		/* "can't happen" */
> +		WARN_ONCE(1, "HuC fw %s invalid fetch_status %s [%d]\n",
> +			huc_fw->uc_fw_path,
> +			intel_uc_fw_status_repr(huc_fw->fetch_status),
> +			huc_fw->fetch_status);
> +		err = -ENXIO;
> +		goto fail;
> +
> +	case UC_FIRMWARE_SUCCESS:
> +		break;
> +	}
> +
> +	err = huc_ucode_xfer(dev_priv);
> +	if (err)
> +		goto fail;
> +
> +	huc_fw->load_status = UC_FIRMWARE_SUCCESS;
> +
> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> +		huc_fw->uc_fw_path,
> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> +		intel_uc_fw_status_repr(huc_fw->load_status));
> +
> +	return 0;
> +
> +fail:
> +	if (huc_fw->load_status == UC_FIRMWARE_PENDING)
> +		huc_fw->load_status = UC_FIRMWARE_FAIL;
> +
> +	DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
> +
> +	return err;
> +}
> +
> +/**
> + * intel_huc_fini() - clean up resources allocated for HuC
> + * @dev: the drm device
> + *
> + * Cleans up by releasing the huc firmware GEM obj.
> + */
> +void intel_huc_fini(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	if (huc_fw->uc_fw_obj)
> +		i915_gem_object_put(huc_fw->uc_fw_obj);
> +	huc_fw->uc_fw_obj = NULL;
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
> +}
> +
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index be89f0b..ac92946 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -24,6 +24,12 @@
>  #ifndef _INTEL_UC_H_
>  #define _INTEL_UC_H_
>  
> +#ifndef _INTEL_HUC_H_
> +#define _INTEL_HUC_H_
> +
> +#define HUC_STATUS2             _MMIO(0xD3B0)
> +#define   HUC_FW_VERIFIED       (1<<7)
> +
>  #include "intel_guc_fwif.h"
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
> @@ -175,6 +181,13 @@ struct intel_guc {
>  	struct mutex send_mutex;
>  };
>  
> +struct intel_huc {
> +	/* Generic uC firmware management */
> +	struct intel_uc_fw huc_fw;
> +
> +	/* HuC-specific additions */
> +};
> +
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
> @@ -191,6 +204,9 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> +	struct intel_uc_fw *uc_fw);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>  
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> @@ -205,4 +221,10 @@ void i915_guc_register(struct drm_i915_private *dev_priv);
>  void i915_guc_unregister(struct drm_i915_private *dev_priv);
>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>  
> +/* intel_huc_loader.c */
> +void intel_huc_init(struct drm_i915_private *dev_priv);
> +void intel_huc_fini(struct drm_device *dev);
> +int intel_huc_load(struct drm_i915_private *dev_priv);
> +
> +#endif
>  #endif
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Dec. 9, 2016, 11:10 a.m. UTC | #2
On Fri, Dec 09, 2016 at 11:56:10AM +0100, Arkadiusz Hiler wrote:
> On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 wopcm_size = GUC_WOPCM_TOP;
> >  
> > @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >  		if (err)
> >  			goto fail;
> >  
> > +		intel_huc_load(dev_priv);

We don't need error handling? That would simplify a lot of our code!
-Chris
Arkadiusz Hiler Dec. 9, 2016, 11:34 a.m. UTC | #3
On Fri, Dec 09, 2016 at 11:10:03AM +0000, Chris Wilson wrote:
> On Fri, Dec 09, 2016 at 11:56:10AM +0100, Arkadiusz Hiler wrote:
> > On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > >  {
> > >  	u32 wopcm_size = GUC_WOPCM_TOP;
> > >  
> > > @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > >  		if (err)
> > >  			goto fail;
> > >  
> > > +		intel_huc_load(dev_priv);
> 
> We don't need error handling? That would simplify a lot of our code!
> -Chris

With this patch series on this specific piece of code - not really.
HuC support it intorduce is _best-eforrty_.

If the function would report error we would not act on it in anyway
other than logging the fail (which the function already does for us).

As Anusha discussed here, there will be some code reorganization due to
introduction of i915.enable_huc and deprecation of enable_guc_loading.
Once we want to have enable_huc=2, there is a reason to change the
signature and report errors.
Michal Wajdeczko Dec. 9, 2016, 12:17 p.m. UTC | #4
On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> 
> The HuC loading process is similar to GuC. The intel_uc_fw_fetch()
> is used for both cases.
> 
> HuC loading needs to be before GuC loading. The WOPCM setting must
> be done early before loading any of them.
> 
> v2: rebased on-top of drm-intel-nightly.
>     removed if(HAS_GUC()) before the guc call. (D.Gordon)
>     update huc_version number of format.
> v3: rebased to drm-intel-nightly, changed the file name format to
>     match the one in the huc package.
>     Changed dev->dev_private to to_i915()
> v4: moved function back to where it was.
>     change wait_for_atomic to wait_for.
> v5: rebased + comment changes.
> v7: rebased.
> v8: rebased.
> v9: rebased. Changed the year in the copyright message to reflect
> the right year.Correct the comments,remove the unwanted WARN message,
> replace drm_gem_object_unreference() with i915_gem_object_put().Make the
> prototypes in intel_huc.h non-extern.
> v10: rebased. Update the file construction done by HuC. It is similar to
> GuC.Adopted the approach used in-
> https://patchwork.freedesktop.org/patch/104355/ <Tvrtko Ursulin>
> v11: Fix warnings remove old declaration
> v12: Change dev to dev_priv in macro definition.
> Corrected comments.
> v13: rebased.
> v14: rebased on top of drm-tip
> v15: rebased. Updated functions intel_huc_load(),intel_huc_init() and
> intel_uc_fw_fetch() to accept dev_priv instead of dev. Moved contents
> of intel_huc.h to intel_uc.h
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Tested-by: Xiang Haihao <haihao.xiang@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile           |   1 +
>  drivers/gpu/drm/i915/i915_drv.c         |   4 +-
>  drivers/gpu/drm/i915/i915_drv.h         |   3 +-
>  drivers/gpu/drm/i915/i915_guc_reg.h     |   3 +
>  drivers/gpu/drm/i915/intel_guc_loader.c |   7 +-
>  drivers/gpu/drm/i915/intel_huc_loader.c | 264 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uc.h         |  22 +++
>  7 files changed, 299 insertions(+), 5 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_huc_loader.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 3c30916..01d4f4b 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -57,6 +57,7 @@ i915-y += i915_cmd_parser.o \
>  # general-purpose microcontroller (GuC) support
>  i915-y += intel_uc.o \
>  	  intel_guc_loader.o \
> +	  intel_huc_loader.o \
>  	  i915_guc_submission.o
>  
>  # autogenerated null render state
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6428588..85a47c2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -600,6 +600,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> +	intel_huc_init(dev_priv);
>  	intel_guc_init(dev_priv);
>  
>  	ret = i915_gem_init(dev_priv);
> @@ -627,6 +628,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>  	i915_gem_fini(dev_priv);
>  cleanup_irq:
> +	intel_huc_fini(dev);
>  	intel_guc_fini(dev_priv);
>  	drm_irq_uninstall(dev);
>  	intel_teardown_gmbus(dev_priv);
> @@ -1313,7 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
>  
>  	/* Flush any outstanding unpin_work. */
>  	drain_workqueue(dev_priv->wq);
> -
> +	intel_huc_fini(dev);
>  	intel_guc_fini(dev_priv);
>  	i915_gem_fini(dev_priv);
>  	intel_fbc_cleanup_cfb(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1480e73..0371ca4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2036,6 +2036,7 @@ struct drm_i915_private {
>  
>  	struct intel_gvt *gvt;
>  
> +	struct intel_huc huc;
>  	struct intel_guc guc;
>  
>  	struct intel_csr csr;
> @@ -2810,7 +2811,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>  #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -
> +#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
>  
>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
> index 5e638fc..f9829f6 100644
> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> @@ -61,9 +61,12 @@
>  #define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
>  #define DMA_COPY_SIZE			_MMIO(0xc310)
>  #define DMA_CTRL			_MMIO(0xc314)
> +#define   HUC_UKERNEL			  (1<<9)
>  #define   UOS_MOVE			  (1<<4)
>  #define   START_DMA			  (1<<0)
>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
> +#define   HUC_LOADING_AGENT_VCR		  (0<<1)
> +#define   HUC_LOADING_AGENT_GUC		  (1<<1)
>  #define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 26a184f..b971351 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -309,8 +309,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
>  
>  	/* Finally start the DMA */
> -	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
> -
> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA) |
> +		_MASKED_BIT_DISABLE(HUC_UKERNEL));
>  	/*
>  	 * Wait for the DMA to complete & the GuC to start up.
>  	 * NB: Docs recommend not using the interrupt for completion.
> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
>  	return ret;
>  }
>  
> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
>  	u32 wopcm_size = GUC_WOPCM_TOP;
>  
> @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>  		if (err)
>  			goto fail;
>  
> +		intel_huc_load(dev_priv);
>  		err = guc_ucode_xfer(dev_priv);
>  		if (!err)
>  			break;
> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
> new file mode 100644
> index 0000000..e0efd1c
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> @@ -0,0 +1,264 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +#include <linux/firmware.h>
> +#include "i915_drv.h"
> +#include "intel_uc.h"
> +
> +/**
> + * DOC: HuC Firmware
> + *
> + * Motivation:
> + * GEN9 introduces a new dedicated firmware for usage in media HEVC (High
> + * Efficiency Video Coding) operations. Userspace can use the firmware
> + * capabilities by adding HuC specific commands to batch buffers.
> + *
> + * Implementation:
> + * The same firmware loader is used as the GuC. However, the actual
> + * loading to HW is deferred until GEM initialization is done.
> + *
> + * Note that HuC firmware loading must be done before GuC loading.
> + */
> +
> +#define SKL_FW_MAJOR 01
> +#define SKL_FW_MINOR 07

Can we use SKL_HUC_FW_ prefix to distinguish these macros from similar
defined in intel_guc_loader.c ?


> +#define SKL_BLD_NUM 1398
> +
> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
> +
> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
> +	SKL_FW_MINOR, SKL_BLD_NUM)
> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
> +
> +/**
> + * huc_ucode_xfer() - DMA's the firmware
> + * @dev_priv: the drm device
> + *
> + * This function takes the gem object containing the firmware, sets up the DMA
> + * engine MMIO, triggers the DMA operation and waits for it to finish.
> + *
> + * Transfer the firmware image to RAM for execution by the microcontroller.
> + *
> + * Return: 0 on success, non-zero on failure
> + */
> +
> +static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +	struct i915_vma *vma;
> +	unsigned long offset = 0;
> +	u32 size;
> +	int ret;
> +
> +	ret = i915_gem_object_set_to_gtt_domain(huc_fw->uc_fw_obj, false);
> +	if (ret) {
> +		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	vma = i915_gem_object_ggtt_pin(huc_fw->uc_fw_obj, NULL, 0, 0, 0);
> +	if (IS_ERR(vma)) {
> +		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> +		return PTR_ERR(vma);
> +	}
> +
> +	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> +
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	/* init WOPCM */
> +	I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
> +	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE |
> +			HUC_LOADING_AGENT_GUC);
> +
> +	/* Set the source address for the uCode */
> +	offset = i915_ggtt_offset(vma) + huc_fw->header_offset;
> +	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> +	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
> +
> +	/* Hardware doesn't look at destination address for HuC. Set it to 0,
> +	 * but still program the correct address space.
> +	 */
> +	I915_WRITE(DMA_ADDR_1_LOW, 0);
> +	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> +
> +	size = huc_fw->header_size + huc_fw->ucode_size;
> +	I915_WRITE(DMA_COPY_SIZE, size);
> +
> +	/* Start the DMA */
> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(HUC_UKERNEL | START_DMA));
> +
> +	/* Wait for DMA to finish */
> +	ret = wait_for((I915_READ(DMA_CTRL) & START_DMA) == 0, 100);
> +
> +	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
> +
> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
> +	/*
> +	 * We keep the object pages for reuse during resume. But we can unpin it
> +	 * now that DMA has completed, so it doesn't continue to take up space.
> +	 */
> +	i915_vma_unpin(vma);
> +
> +	return ret;
> +}
> +
> +/**
> + * intel_huc_init() - initiate HuC firmware loading request
> + * @dev: the drm device

Mismatched param name.

> + *
> + * Called early during driver load, but after GEM is initialised. The loading
> + * will continue only when driver explicitly specify firmware name and version.
> + * All other cases are considered as UC_FIRMWARE_NONE either because HW is not
> + * capable or driver yet support it. And there will be no error message for
> + * UC_FIRMWARE_NONE cases.
> + *
> + * The DMA-copying to HW is done later when intel_huc_load() is called.
> + */
> +void intel_huc_init(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_huc *huc = &dev_priv->huc;

Maybe *huc shall be passed as only param (to match intel_huc function name).
Then dev_priv could be recreated from huc_to_i915().


> +	struct intel_uc_fw *huc_fw = &huc->huc_fw;
> +	const char *fw_path = NULL;
> +
> +	huc_fw->uc_fw_path = NULL;
> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
> +	huc_fw->load_status = UC_FIRMWARE_NONE;
> +	huc_fw->fw_type = UC_FW_TYPE_HUC;
> +
> +	if (!HAS_HUC_UCODE(dev_priv))
> +		return;
> +
> +	if (IS_SKYLAKE(dev_priv)) {
> +		fw_path = I915_SKL_HUC_UCODE;
> +		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
> +		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
> +	}
> +
> +	huc_fw->uc_fw_path = fw_path;
> +	huc_fw->fetch_status = UC_FIRMWARE_PENDING;
> +
> +	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
> +
> +	intel_uc_fw_fetch(dev_priv, huc_fw);
> +}
> +
> +/**
> + * intel_huc_load() - load HuC uCode to device
> + * @dev: the drm device

Mismatched param name.


> + *
> + * Called from gem_init_hw() during driver loading and also after a GPU reset.
> + * Be note that HuC loading must be done before GuC loading.
> + *
> + * The firmware image should have already been fetched into memory by the
> + * earlier call to intel_huc_init(), so here we need only check that
> + * is succeeded, and then transfer the image to the h/w.
> + *
> + * Return:	non-zero code on error
> + */
> +int intel_huc_load(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +	int err;
> +
> +	if (huc_fw->fetch_status == UC_FIRMWARE_NONE)
> +		return 0;
> +
> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> +		huc_fw->uc_fw_path,
> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> +		intel_uc_fw_status_repr(huc_fw->load_status));
> +
> +	if (huc_fw->fetch_status == UC_FIRMWARE_SUCCESS &&
> +	    huc_fw->load_status == UC_FIRMWARE_FAIL)
> +		return -ENOEXEC;
> +
> +	huc_fw->load_status = UC_FIRMWARE_PENDING;
> +
> +	switch (huc_fw->fetch_status) {
> +	case UC_FIRMWARE_FAIL:
> +		/* something went wrong :( */
> +		err = -EIO;
> +		goto fail;
> +
> +	case UC_FIRMWARE_NONE:
> +	case UC_FIRMWARE_PENDING:
> +	default:
> +		/* "can't happen" */
> +		WARN_ONCE(1, "HuC fw %s invalid fetch_status %s [%d]\n",
> +			huc_fw->uc_fw_path,
> +			intel_uc_fw_status_repr(huc_fw->fetch_status),
> +			huc_fw->fetch_status);
> +		err = -ENXIO;
> +		goto fail;
> +
> +	case UC_FIRMWARE_SUCCESS:
> +		break;
> +	}
> +
> +	err = huc_ucode_xfer(dev_priv);
> +	if (err)
> +		goto fail;
> +
> +	huc_fw->load_status = UC_FIRMWARE_SUCCESS;
> +
> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> +		huc_fw->uc_fw_path,
> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> +		intel_uc_fw_status_repr(huc_fw->load_status));
> +
> +	return 0;
> +
> +fail:
> +	if (huc_fw->load_status == UC_FIRMWARE_PENDING)
> +		huc_fw->load_status = UC_FIRMWARE_FAIL;
> +
> +	DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
> +
> +	return err;
> +}
> +
> +/**
> + * intel_huc_fini() - clean up resources allocated for HuC
> + * @dev: the drm device
> + *
> + * Cleans up by releasing the huc firmware GEM obj.
> + */
> +void intel_huc_fini(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	if (huc_fw->uc_fw_obj)
> +		i915_gem_object_put(huc_fw->uc_fw_obj);
> +	huc_fw->uc_fw_obj = NULL;
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
> +}
> +
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index be89f0b..ac92946 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -24,6 +24,12 @@
>  #ifndef _INTEL_UC_H_
>  #define _INTEL_UC_H_
>  
> +#ifndef _INTEL_HUC_H_
> +#define _INTEL_HUC_H_

Typo ? This is still intel_uc.h file, right ?


> +
> +#define HUC_STATUS2             _MMIO(0xD3B0)
> +#define   HUC_FW_VERIFIED       (1<<7)
> +

Is it correct place for these defs? 
What about i915_guc_reg.h ?


>  #include "intel_guc_fwif.h"
>  #include "i915_guc_reg.h"
>  #include "intel_ringbuffer.h"
> @@ -175,6 +181,13 @@ struct intel_guc {
>  	struct mutex send_mutex;
>  };
>  
> +struct intel_huc {
> +	/* Generic uC firmware management */
> +	struct intel_uc_fw huc_fw;
> +
> +	/* HuC-specific additions */
> +};
> +
>  /* intel_uc.c */
>  void intel_uc_init_early(struct drm_i915_private *dev_priv);
>  bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
> @@ -191,6 +204,9 @@ extern void intel_guc_fini(struct drm_i915_private *dev_priv);
>  extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
>  extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
>  extern int intel_guc_resume(struct drm_i915_private *dev_priv);
> +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> +	struct intel_uc_fw *uc_fw);
> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);

All other public functions have intel_ prefix.


>  
>  /* i915_guc_submission.c */
>  int i915_guc_submission_init(struct drm_i915_private *dev_priv);
> @@ -205,4 +221,10 @@ void i915_guc_register(struct drm_i915_private *dev_priv);
>  void i915_guc_unregister(struct drm_i915_private *dev_priv);
>  int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>  
> +/* intel_huc_loader.c */
> +void intel_huc_init(struct drm_i915_private *dev_priv);
> +void intel_huc_fini(struct drm_device *dev);
> +int intel_huc_load(struct drm_i915_private *dev_priv);
> +
> +#endif
>  #endif
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Arkadiusz Hiler Dec. 9, 2016, 12:19 p.m. UTC | #5
On Fri, Dec 09, 2016 at 12:34:55PM +0100, Arkadiusz Hiler wrote:
> On Fri, Dec 09, 2016 at 11:10:03AM +0000, Chris Wilson wrote:
> > On Fri, Dec 09, 2016 at 11:56:10AM +0100, Arkadiusz Hiler wrote:
> > > On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> > > > -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > > > +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> > > >  {
> > > >  	u32 wopcm_size = GUC_WOPCM_TOP;
> > > >  
> > > > @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> > > >  		if (err)
> > > >  			goto fail;
> > > >  
> > > > +		intel_huc_load(dev_priv);
> > 
> > We don't need error handling? That would simplify a lot of our code!
> > -Chris
> 
> With this patch series on this specific piece of code - not really.
> HuC support it intorduce is _best-eforrty_.
> 
> If the function would report error we would not act on it in anyway
> other than logging the fail (which the function already does for us).
> 
> As Anusha discussed here, there will be some code reorganization due to
> introduction of i915.enable_huc and deprecation of enable_guc_loading.
> Once we want to have enable_huc=2, there is a reason to change the
> signature and report errors.

Ergh, I've got signatures of auth and load mixed up. But most of the
statement above still semi-holds.
Srivatsa, Anusha Dec. 9, 2016, 11:56 p.m. UTC | #6
>-----Original Message-----
>From: Michal Wajdeczko [mailto:michal.wajdeczko@linux.intel.com]
>Sent: Friday, December 9, 2016 4:18 AM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Alex Dai <yu.dai@intel.com>; Peter Antoine
><peter.antoine@intel.com>
>Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915/huc: Add HuC fw loading support
>
>On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
>> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
>>
>> The HuC loading process is similar to GuC. The intel_uc_fw_fetch() is
>> used for both cases.
>>
>> HuC loading needs to be before GuC loading. The WOPCM setting must be
>> done early before loading any of them.
>>
>> v2: rebased on-top of drm-intel-nightly.
>>     removed if(HAS_GUC()) before the guc call. (D.Gordon)
>>     update huc_version number of format.
>> v3: rebased to drm-intel-nightly, changed the file name format to
>>     match the one in the huc package.
>>     Changed dev->dev_private to to_i915()
>> v4: moved function back to where it was.
>>     change wait_for_atomic to wait_for.
>> v5: rebased + comment changes.
>> v7: rebased.
>> v8: rebased.
>> v9: rebased. Changed the year in the copyright message to reflect the
>> right year.Correct the comments,remove the unwanted WARN message,
>> replace drm_gem_object_unreference() with i915_gem_object_put().Make
>> the prototypes in intel_huc.h non-extern.
>> v10: rebased. Update the file construction done by HuC. It is similar
>> to GuC.Adopted the approach used in-
>> https://patchwork.freedesktop.org/patch/104355/ <Tvrtko Ursulin>
>> v11: Fix warnings remove old declaration
>> v12: Change dev to dev_priv in macro definition.
>> Corrected comments.
>> v13: rebased.
>> v14: rebased on top of drm-tip
>> v15: rebased. Updated functions intel_huc_load(),intel_huc_init() and
>> intel_uc_fw_fetch() to accept dev_priv instead of dev. Moved contents
>> of intel_huc.h to intel_uc.h
>>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Tested-by: Xiang Haihao <haihao.xiang@intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> Signed-off-by: Alex Dai <yu.dai@intel.com>
>> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile           |   1 +
>>  drivers/gpu/drm/i915/i915_drv.c         |   4 +-
>>  drivers/gpu/drm/i915/i915_drv.h         |   3 +-
>>  drivers/gpu/drm/i915/i915_guc_reg.h     |   3 +
>>  drivers/gpu/drm/i915/intel_guc_loader.c |   7 +-
>>  drivers/gpu/drm/i915/intel_huc_loader.c | 264
>++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uc.h         |  22 +++
>>  7 files changed, 299 insertions(+), 5 deletions(-)  create mode
>> 100644 drivers/gpu/drm/i915/intel_huc_loader.c
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile
>> b/drivers/gpu/drm/i915/Makefile index 3c30916..01d4f4b 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -57,6 +57,7 @@ i915-y += i915_cmd_parser.o \  # general-purpose
>> microcontroller (GuC) support  i915-y += intel_uc.o \
>>  	  intel_guc_loader.o \
>> +	  intel_huc_loader.o \
>>  	  i915_guc_submission.o
>>
>>  # autogenerated null render state
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c index 6428588..85a47c2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -600,6 +600,7 @@ static int i915_load_modeset_init(struct drm_device
>*dev)
>>  	if (ret)
>>  		goto cleanup_irq;
>>
>> +	intel_huc_init(dev_priv);
>>  	intel_guc_init(dev_priv);
>>
>>  	ret = i915_gem_init(dev_priv);
>> @@ -627,6 +628,7 @@ static int i915_load_modeset_init(struct drm_device
>*dev)
>>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>>  	i915_gem_fini(dev_priv);
>>  cleanup_irq:
>> +	intel_huc_fini(dev);
>>  	intel_guc_fini(dev_priv);
>>  	drm_irq_uninstall(dev);
>>  	intel_teardown_gmbus(dev_priv);
>> @@ -1313,7 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
>>
>>  	/* Flush any outstanding unpin_work. */
>>  	drain_workqueue(dev_priv->wq);
>> -
>> +	intel_huc_fini(dev);
>>  	intel_guc_fini(dev_priv);
>>  	i915_gem_fini(dev_priv);
>>  	intel_fbc_cleanup_cfb(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h index 1480e73..0371ca4 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2036,6 +2036,7 @@ struct drm_i915_private {
>>
>>  	struct intel_gvt *gvt;
>>
>> +	struct intel_huc huc;
>>  	struct intel_guc guc;
>>
>>  	struct intel_csr csr;
>> @@ -2810,7 +2811,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
>>  #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>>  #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
>> -
>> +#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
>>  #define HAS_RESOURCE_STREAMER(dev_priv)
>> ((dev_priv)->info.has_resource_streamer)
>>
>>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>> index 5e638fc..f9829f6 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
>> @@ -61,9 +61,12 @@
>>  #define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
>>  #define DMA_COPY_SIZE			_MMIO(0xc310)
>>  #define DMA_CTRL			_MMIO(0xc314)
>> +#define   HUC_UKERNEL			  (1<<9)
>>  #define   UOS_MOVE			  (1<<4)
>>  #define   START_DMA			  (1<<0)
>>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
>> +#define   HUC_LOADING_AGENT_VCR		  (0<<1)
>> +#define   HUC_LOADING_AGENT_GUC		  (1<<1)
>>  #define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
>>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>> index 26a184f..b971351 100644
>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>> @@ -309,8 +309,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>>  	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
>>
>>  	/* Finally start the DMA */
>> -	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
>START_DMA));
>> -
>> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
>START_DMA) |
>> +		_MASKED_BIT_DISABLE(HUC_UKERNEL));
>>  	/*
>>  	 * Wait for the DMA to complete & the GuC to start up.
>>  	 * NB: Docs recommend not using the interrupt for completion.
>> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
>*dev_priv,
>>  	return ret;
>>  }
>>
>> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
>>  {
>>  	u32 wopcm_size = GUC_WOPCM_TOP;
>>
>> @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>>  		if (err)
>>  			goto fail;
>>
>> +		intel_huc_load(dev_priv);
>>  		err = guc_ucode_xfer(dev_priv);
>>  		if (!err)
>>  			break;
>> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c
>> b/drivers/gpu/drm/i915/intel_huc_loader.c
>> new file mode 100644
>> index 0000000..e0efd1c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Copyright (c) 2016 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person
>> +obtaining a
>> + * copy of this software and associated documentation files (the
>> +"Software"),
>> + * to deal in the Software without restriction, including without
>> +limitation
>> + * the rights to use, copy, modify, merge, publish, distribute,
>> +sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom
>> +the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including
>> +the next
>> + * paragraph) shall be included in all copies or substantial portions
>> +of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> +EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> +MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
>EVENT
>> +SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
>DAMAGES
>> +OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> +ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> +OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +#include <linux/firmware.h>
>> +#include "i915_drv.h"
>> +#include "intel_uc.h"
>> +
>> +/**
>> + * DOC: HuC Firmware
>> + *
>> + * Motivation:
>> + * GEN9 introduces a new dedicated firmware for usage in media HEVC
>> +(High
>> + * Efficiency Video Coding) operations. Userspace can use the
>> +firmware
>> + * capabilities by adding HuC specific commands to batch buffers.
>> + *
>> + * Implementation:
>> + * The same firmware loader is used as the GuC. However, the actual
>> + * loading to HW is deferred until GEM initialization is done.
>> + *
>> + * Note that HuC firmware loading must be done before GuC loading.
>> + */
>> +
>> +#define SKL_FW_MAJOR 01
>> +#define SKL_FW_MINOR 07
>
>Can we use SKL_HUC_FW_ prefix to distinguish these macros from similar defined
>in intel_guc_loader.c ?

Sure
>
>> +#define SKL_BLD_NUM 1398
>> +
>> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
>> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
>> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
>> +
>> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
>> +	SKL_FW_MINOR, SKL_BLD_NUM)
>> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>> +
>> +/**
>> + * huc_ucode_xfer() - DMA's the firmware
>> + * @dev_priv: the drm device
>> + *
>> + * This function takes the gem object containing the firmware, sets
>> +up the DMA
>> + * engine MMIO, triggers the DMA operation and waits for it to finish.
>> + *
>> + * Transfer the firmware image to RAM for execution by the microcontroller.
>> + *
>> + * Return: 0 on success, non-zero on failure  */
>> +
>> +static int huc_ucode_xfer(struct drm_i915_private *dev_priv) {
>> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
>> +	struct i915_vma *vma;
>> +	unsigned long offset = 0;
>> +	u32 size;
>> +	int ret;
>> +
>> +	ret = i915_gem_object_set_to_gtt_domain(huc_fw->uc_fw_obj, false);
>> +	if (ret) {
>> +		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	vma = i915_gem_object_ggtt_pin(huc_fw->uc_fw_obj, NULL, 0, 0, 0);
>> +	if (IS_ERR(vma)) {
>> +		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
>> +		return PTR_ERR(vma);
>> +	}
>> +
>> +	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
>> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>> +
>> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>> +
>> +	/* init WOPCM */
>> +	I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
>> +	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
>GUC_WOPCM_OFFSET_VALUE |
>> +			HUC_LOADING_AGENT_GUC);
>> +
>> +	/* Set the source address for the uCode */
>> +	offset = i915_ggtt_offset(vma) + huc_fw->header_offset;
>> +	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
>> +	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
>> +
>> +	/* Hardware doesn't look at destination address for HuC. Set it to 0,
>> +	 * but still program the correct address space.
>> +	 */
>> +	I915_WRITE(DMA_ADDR_1_LOW, 0);
>> +	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
>> +
>> +	size = huc_fw->header_size + huc_fw->ucode_size;
>> +	I915_WRITE(DMA_COPY_SIZE, size);
>> +
>> +	/* Start the DMA */
>> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(HUC_UKERNEL |
>START_DMA));
>> +
>> +	/* Wait for DMA to finish */
>> +	ret = wait_for((I915_READ(DMA_CTRL) & START_DMA) == 0, 100);
>> +
>> +	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
>> +
>> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>> +
>> +	/*
>> +	 * We keep the object pages for reuse during resume. But we can unpin it
>> +	 * now that DMA has completed, so it doesn't continue to take up space.
>> +	 */
>> +	i915_vma_unpin(vma);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * intel_huc_init() - initiate HuC firmware loading request
>> + * @dev: the drm device
>
>Mismatched param name.
>
>> + *
>> + * Called early during driver load, but after GEM is initialised. The
>> +loading
>> + * will continue only when driver explicitly specify firmware name and version.
>> + * All other cases are considered as UC_FIRMWARE_NONE either because
>> +HW is not
>> + * capable or driver yet support it. And there will be no error
>> +message for
>> + * UC_FIRMWARE_NONE cases.
>> + *
>> + * The DMA-copying to HW is done later when intel_huc_load() is called.
>> + */
>> +void intel_huc_init(struct drm_i915_private *dev_priv) {
>> +	struct intel_huc *huc = &dev_priv->huc;
>
>Maybe *huc shall be passed as only param (to match intel_huc function name).
>Then dev_priv could be recreated from huc_to_i915().

Why? Can you elaborate?
-anusha
>
>> +	struct intel_uc_fw *huc_fw = &huc->huc_fw;
>> +	const char *fw_path = NULL;
>> +
>> +	huc_fw->uc_fw_path = NULL;
>> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
>> +	huc_fw->load_status = UC_FIRMWARE_NONE;
>> +	huc_fw->fw_type = UC_FW_TYPE_HUC;
>> +
>> +	if (!HAS_HUC_UCODE(dev_priv))
>> +		return;
>> +
>> +	if (IS_SKYLAKE(dev_priv)) {
>> +		fw_path = I915_SKL_HUC_UCODE;
>> +		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
>> +		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
>> +	}
>> +
>> +	huc_fw->uc_fw_path = fw_path;
>> +	huc_fw->fetch_status = UC_FIRMWARE_PENDING;
>> +
>> +	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
>> +
>> +	intel_uc_fw_fetch(dev_priv, huc_fw); }
>> +
>> +/**
>> + * intel_huc_load() - load HuC uCode to device
>> + * @dev: the drm device
>
>Mismatched param name.
>
>
>> + *
>> + * Called from gem_init_hw() during driver loading and also after a GPU reset.
>> + * Be note that HuC loading must be done before GuC loading.
>> + *
>> + * The firmware image should have already been fetched into memory by
>> +the
>> + * earlier call to intel_huc_init(), so here we need only check that
>> + * is succeeded, and then transfer the image to the h/w.
>> + *
>> + * Return:	non-zero code on error
>> + */
>> +int intel_huc_load(struct drm_i915_private *dev_priv) {
>> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
>> +	int err;
>> +
>> +	if (huc_fw->fetch_status == UC_FIRMWARE_NONE)
>> +		return 0;
>> +
>> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
>> +		huc_fw->uc_fw_path,
>> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
>> +		intel_uc_fw_status_repr(huc_fw->load_status));
>> +
>> +	if (huc_fw->fetch_status == UC_FIRMWARE_SUCCESS &&
>> +	    huc_fw->load_status == UC_FIRMWARE_FAIL)
>> +		return -ENOEXEC;
>> +
>> +	huc_fw->load_status = UC_FIRMWARE_PENDING;
>> +
>> +	switch (huc_fw->fetch_status) {
>> +	case UC_FIRMWARE_FAIL:
>> +		/* something went wrong :( */
>> +		err = -EIO;
>> +		goto fail;
>> +
>> +	case UC_FIRMWARE_NONE:
>> +	case UC_FIRMWARE_PENDING:
>> +	default:
>> +		/* "can't happen" */
>> +		WARN_ONCE(1, "HuC fw %s invalid fetch_status %s [%d]\n",
>> +			huc_fw->uc_fw_path,
>> +			intel_uc_fw_status_repr(huc_fw->fetch_status),
>> +			huc_fw->fetch_status);
>> +		err = -ENXIO;
>> +		goto fail;
>> +
>> +	case UC_FIRMWARE_SUCCESS:
>> +		break;
>> +	}
>> +
>> +	err = huc_ucode_xfer(dev_priv);
>> +	if (err)
>> +		goto fail;
>> +
>> +	huc_fw->load_status = UC_FIRMWARE_SUCCESS;
>> +
>> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
>> +		huc_fw->uc_fw_path,
>> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
>> +		intel_uc_fw_status_repr(huc_fw->load_status));
>> +
>> +	return 0;
>> +
>> +fail:
>> +	if (huc_fw->load_status == UC_FIRMWARE_PENDING)
>> +		huc_fw->load_status = UC_FIRMWARE_FAIL;
>> +
>> +	DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
>> +
>> +	return err;
>> +}
>> +
>> +/**
>> + * intel_huc_fini() - clean up resources allocated for HuC
>> + * @dev: the drm device
>> + *
>> + * Cleans up by releasing the huc firmware GEM obj.
>> + */
>> +void intel_huc_fini(struct drm_device *dev) {
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
>> +
>> +	mutex_lock(&dev->struct_mutex);
>> +	if (huc_fw->uc_fw_obj)
>> +		i915_gem_object_put(huc_fw->uc_fw_obj);
>> +	huc_fw->uc_fw_obj = NULL;
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	huc_fw->fetch_status = UC_FIRMWARE_NONE; }
>> +
>> diff --git a/drivers/gpu/drm/i915/intel_uc.h
>> b/drivers/gpu/drm/i915/intel_uc.h index be89f0b..ac92946 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/intel_uc.h
>> @@ -24,6 +24,12 @@
>>  #ifndef _INTEL_UC_H_
>>  #define _INTEL_UC_H_
>>
>> +#ifndef _INTEL_HUC_H_
>> +#define _INTEL_HUC_H_
>
>Typo ? This is still intel_uc.h file, right ?

Yes it is intel_uc.h. Initially the above two initializations were in intel_huc.h. But now its contents are moved to intel_uc.h. One common file for guc and huc declarations.....
>
>> +
>> +#define HUC_STATUS2             _MMIO(0xD3B0)
>> +#define   HUC_FW_VERIFIED       (1<<7)
>> +
>
>Is it correct place for these defs?
>What about i915_guc_reg.h ?
This was also initially in intel_huc.h. 
>
>>  #include "intel_guc_fwif.h"
>>  #include "i915_guc_reg.h"
>>  #include "intel_ringbuffer.h"
>> @@ -175,6 +181,13 @@ struct intel_guc {
>>  	struct mutex send_mutex;
>>  };
>>
>> +struct intel_huc {
>> +	/* Generic uC firmware management */
>> +	struct intel_uc_fw huc_fw;
>> +
>> +	/* HuC-specific additions */
>> +};
>> +
>>  /* intel_uc.c */
>>  void intel_uc_init_early(struct drm_i915_private *dev_priv);  bool
>> intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); @@
>> -191,6 +204,9 @@ extern void intel_guc_fini(struct drm_i915_private
>> *dev_priv);  extern const char *intel_uc_fw_status_repr(enum
>> intel_uc_fw_status status);  extern int intel_guc_suspend(struct
>> drm_i915_private *dev_priv);  extern int intel_guc_resume(struct
>> drm_i915_private *dev_priv);
>> +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>> +	struct intel_uc_fw *uc_fw);
>> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
>
>All other public functions have intel_ prefix.
Got it. Will change. Thanks

Cheers,
Anusha
>
>>
>>  /* i915_guc_submission.c */
>>  int i915_guc_submission_init(struct drm_i915_private *dev_priv); @@
>> -205,4 +221,10 @@ void i915_guc_register(struct drm_i915_private
>> *dev_priv);  void i915_guc_unregister(struct drm_i915_private
>> *dev_priv);  int i915_guc_log_control(struct drm_i915_private
>> *dev_priv, u64 control_val);
>>
>> +/* intel_huc_loader.c */
>> +void intel_huc_init(struct drm_i915_private *dev_priv); void
>> +intel_huc_fini(struct drm_device *dev); int intel_huc_load(struct
>> +drm_i915_private *dev_priv);
>> +
>> +#endif
>>  #endif
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Arkadiusz Hiler Dec. 12, 2016, 11:50 a.m. UTC | #7
On Fri, Dec 09, 2016 at 11:56:20PM +0000, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Michal Wajdeczko [mailto:michal.wajdeczko@linux.intel.com]
> >Sent: Friday, December 9, 2016 4:18 AM
> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Alex Dai <yu.dai@intel.com>; Peter Antoine
> ><peter.antoine@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915/huc: Add HuC fw loading support
> >
> >On Thu, Dec 08, 2016 at 03:02:14PM -0800, anushasr wrote:
> >> From: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >>
> >> The HuC loading process is similar to GuC. The intel_uc_fw_fetch() is
> >> used for both cases.
> >>
> >> HuC loading needs to be before GuC loading. The WOPCM setting must be
> >> done early before loading any of them.
> >>
> >> v2: rebased on-top of drm-intel-nightly.
> >>     removed if(HAS_GUC()) before the guc call. (D.Gordon)
> >>     update huc_version number of format.
> >> v3: rebased to drm-intel-nightly, changed the file name format to
> >>     match the one in the huc package.
> >>     Changed dev->dev_private to to_i915()
> >> v4: moved function back to where it was.
> >>     change wait_for_atomic to wait_for.
> >> v5: rebased + comment changes.
> >> v7: rebased.
> >> v8: rebased.
> >> v9: rebased. Changed the year in the copyright message to reflect the
> >> right year.Correct the comments,remove the unwanted WARN message,
> >> replace drm_gem_object_unreference() with i915_gem_object_put().Make
> >> the prototypes in intel_huc.h non-extern.
> >> v10: rebased. Update the file construction done by HuC. It is similar
> >> to GuC.Adopted the approach used in-
> >> https://patchwork.freedesktop.org/patch/104355/ <Tvrtko Ursulin>
> >> v11: Fix warnings remove old declaration
> >> v12: Change dev to dev_priv in macro definition.
> >> Corrected comments.
> >> v13: rebased.
> >> v14: rebased on top of drm-tip
> >> v15: rebased. Updated functions intel_huc_load(),intel_huc_init() and
> >> intel_uc_fw_fetch() to accept dev_priv instead of dev. Moved contents
> >> of intel_huc.h to intel_uc.h
> >>
> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> Tested-by: Xiang Haihao <haihao.xiang@intel.com>
> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> Signed-off-by: Alex Dai <yu.dai@intel.com>
> >> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/Makefile           |   1 +
> >>  drivers/gpu/drm/i915/i915_drv.c         |   4 +-
> >>  drivers/gpu/drm/i915/i915_drv.h         |   3 +-
> >>  drivers/gpu/drm/i915/i915_guc_reg.h     |   3 +
> >>  drivers/gpu/drm/i915/intel_guc_loader.c |   7 +-
> >>  drivers/gpu/drm/i915/intel_huc_loader.c | 264
> >++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_uc.h         |  22 +++
> >>  7 files changed, 299 insertions(+), 5 deletions(-)  create mode
> >> 100644 drivers/gpu/drm/i915/intel_huc_loader.c
> >>
> >> diff --git a/drivers/gpu/drm/i915/Makefile
> >> b/drivers/gpu/drm/i915/Makefile index 3c30916..01d4f4b 100644
> >> --- a/drivers/gpu/drm/i915/Makefile
> >> +++ b/drivers/gpu/drm/i915/Makefile
> >> @@ -57,6 +57,7 @@ i915-y += i915_cmd_parser.o \  # general-purpose
> >> microcontroller (GuC) support  i915-y += intel_uc.o \
> >>  	  intel_guc_loader.o \
> >> +	  intel_huc_loader.o \
> >>  	  i915_guc_submission.o
> >>
> >>  # autogenerated null render state
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> >> b/drivers/gpu/drm/i915/i915_drv.c index 6428588..85a47c2 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -600,6 +600,7 @@ static int i915_load_modeset_init(struct drm_device
> >*dev)
> >>  	if (ret)
> >>  		goto cleanup_irq;
> >>
> >> +	intel_huc_init(dev_priv);
> >>  	intel_guc_init(dev_priv);
> >>
> >>  	ret = i915_gem_init(dev_priv);
> >> @@ -627,6 +628,7 @@ static int i915_load_modeset_init(struct drm_device
> >*dev)
> >>  		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
> >>  	i915_gem_fini(dev_priv);
> >>  cleanup_irq:
> >> +	intel_huc_fini(dev);
> >>  	intel_guc_fini(dev_priv);
> >>  	drm_irq_uninstall(dev);
> >>  	intel_teardown_gmbus(dev_priv);
> >> @@ -1313,7 +1315,7 @@ void i915_driver_unload(struct drm_device *dev)
> >>
> >>  	/* Flush any outstanding unpin_work. */
> >>  	drain_workqueue(dev_priv->wq);
> >> -
> >> +	intel_huc_fini(dev);
> >>  	intel_guc_fini(dev_priv);
> >>  	i915_gem_fini(dev_priv);
> >>  	intel_fbc_cleanup_cfb(dev_priv);
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h index 1480e73..0371ca4 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2036,6 +2036,7 @@ struct drm_i915_private {
> >>
> >>  	struct intel_gvt *gvt;
> >>
> >> +	struct intel_huc huc;
> >>  	struct intel_guc guc;
> >>
> >>  	struct intel_csr csr;
> >> @@ -2810,7 +2811,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> >>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> >>  #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> >>  #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> >> -
> >> +#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> >>  #define HAS_RESOURCE_STREAMER(dev_priv)
> >> ((dev_priv)->info.has_resource_streamer)
> >>
> >>  #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
> >> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
> >> b/drivers/gpu/drm/i915/i915_guc_reg.h
> >> index 5e638fc..f9829f6 100644
> >> --- a/drivers/gpu/drm/i915/i915_guc_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_guc_reg.h
> >> @@ -61,9 +61,12 @@
> >>  #define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
> >>  #define DMA_COPY_SIZE			_MMIO(0xc310)
> >>  #define DMA_CTRL			_MMIO(0xc314)
> >> +#define   HUC_UKERNEL			  (1<<9)
> >>  #define   UOS_MOVE			  (1<<4)
> >>  #define   START_DMA			  (1<<0)
> >>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
> >> +#define   HUC_LOADING_AGENT_VCR		  (0<<1)
> >> +#define   HUC_LOADING_AGENT_GUC		  (1<<1)
> >>  #define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
> >>  #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
> >> b/drivers/gpu/drm/i915/intel_guc_loader.c
> >> index 26a184f..b971351 100644
> >> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> >> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> >> @@ -309,8 +309,8 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
> >*dev_priv,
> >>  	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> >>
> >>  	/* Finally start the DMA */
> >> -	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
> >START_DMA));
> >> -
> >> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE |
> >START_DMA) |
> >> +		_MASKED_BIT_DISABLE(HUC_UKERNEL));
> >>  	/*
> >>  	 * Wait for the DMA to complete & the GuC to start up.
> >>  	 * NB: Docs recommend not using the interrupt for completion.
> >> @@ -334,7 +334,7 @@ static int guc_ucode_xfer_dma(struct drm_i915_private
> >*dev_priv,
> >>  	return ret;
> >>  }
> >>
> >> -static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> >> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
> >>  {
> >>  	u32 wopcm_size = GUC_WOPCM_TOP;
> >>
> >> @@ -511,6 +511,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> >>  		if (err)
> >>  			goto fail;
> >>
> >> +		intel_huc_load(dev_priv);
> >>  		err = guc_ucode_xfer(dev_priv);
> >>  		if (!err)
> >>  			break;
> >> diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c
> >> b/drivers/gpu/drm/i915/intel_huc_loader.c
> >> new file mode 100644
> >> index 0000000..e0efd1c
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
> >> @@ -0,0 +1,264 @@
> >> +/*
> >> + * Copyright (c) 2016 Intel Corporation
> >> + *
> >> + * Permission is hereby granted, free of charge, to any person
> >> +obtaining a
> >> + * copy of this software and associated documentation files (the
> >> +"Software"),
> >> + * to deal in the Software without restriction, including without
> >> +limitation
> >> + * the rights to use, copy, modify, merge, publish, distribute,
> >> +sublicense,
> >> + * and/or sell copies of the Software, and to permit persons to whom
> >> +the
> >> + * Software is furnished to do so, subject to the following conditions:
> >> + *
> >> + * The above copyright notice and this permission notice (including
> >> +the next
> >> + * paragraph) shall be included in all copies or substantial portions
> >> +of the
> >> + * Software.
> >> + *
> >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >> +EXPRESS OR
> >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> >> +MERCHANTABILITY,
> >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> >EVENT
> >> +SHALL
> >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> >DAMAGES
> >> +OR OTHER
> >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> >> +ARISING
> >> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> >> +OTHER DEALINGS
> >> + * IN THE SOFTWARE.
> >> + *
> >> + */
> >> +#include <linux/firmware.h>
> >> +#include "i915_drv.h"
> >> +#include "intel_uc.h"
> >> +
> >> +/**
> >> + * DOC: HuC Firmware
> >> + *
> >> + * Motivation:
> >> + * GEN9 introduces a new dedicated firmware for usage in media HEVC
> >> +(High
> >> + * Efficiency Video Coding) operations. Userspace can use the
> >> +firmware
> >> + * capabilities by adding HuC specific commands to batch buffers.
> >> + *
> >> + * Implementation:
> >> + * The same firmware loader is used as the GuC. However, the actual
> >> + * loading to HW is deferred until GEM initialization is done.
> >> + *
> >> + * Note that HuC firmware loading must be done before GuC loading.
> >> + */
> >> +
> >> +#define SKL_FW_MAJOR 01
> >> +#define SKL_FW_MINOR 07
> >
> >Can we use SKL_HUC_FW_ prefix to distinguish these macros from similar defined
> >in intel_guc_loader.c ?
> 
> Sure
> >
> >> +#define SKL_BLD_NUM 1398
> >> +
> >> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
> >> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
> >> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
> >> +
> >> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
> >> +	SKL_FW_MINOR, SKL_BLD_NUM)
> >> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
> >> +
> >> +/**
> >> + * huc_ucode_xfer() - DMA's the firmware
> >> + * @dev_priv: the drm device
> >> + *
> >> + * This function takes the gem object containing the firmware, sets
> >> +up the DMA
> >> + * engine MMIO, triggers the DMA operation and waits for it to finish.
> >> + *
> >> + * Transfer the firmware image to RAM for execution by the microcontroller.
> >> + *
> >> + * Return: 0 on success, non-zero on failure  */
> >> +
> >> +static int huc_ucode_xfer(struct drm_i915_private *dev_priv) {
> >> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> >> +	struct i915_vma *vma;
> >> +	unsigned long offset = 0;
> >> +	u32 size;
> >> +	int ret;
> >> +
> >> +	ret = i915_gem_object_set_to_gtt_domain(huc_fw->uc_fw_obj, false);
> >> +	if (ret) {
> >> +		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	vma = i915_gem_object_ggtt_pin(huc_fw->uc_fw_obj, NULL, 0, 0, 0);
> >> +	if (IS_ERR(vma)) {
> >> +		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
> >> +		return PTR_ERR(vma);
> >> +	}
> >> +
> >> +	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
> >> +	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> >> +
> >> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> >> +
> >> +	/* init WOPCM */
> >> +	I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
> >> +	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
> >GUC_WOPCM_OFFSET_VALUE |
> >> +			HUC_LOADING_AGENT_GUC);
> >> +
> >> +	/* Set the source address for the uCode */
> >> +	offset = i915_ggtt_offset(vma) + huc_fw->header_offset;
> >> +	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
> >> +	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
> >> +
> >> +	/* Hardware doesn't look at destination address for HuC. Set it to 0,
> >> +	 * but still program the correct address space.
> >> +	 */
> >> +	I915_WRITE(DMA_ADDR_1_LOW, 0);
> >> +	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
> >> +
> >> +	size = huc_fw->header_size + huc_fw->ucode_size;
> >> +	I915_WRITE(DMA_COPY_SIZE, size);
> >> +
> >> +	/* Start the DMA */
> >> +	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(HUC_UKERNEL |
> >START_DMA));
> >> +
> >> +	/* Wait for DMA to finish */
> >> +	ret = wait_for((I915_READ(DMA_CTRL) & START_DMA) == 0, 100);
> >> +
> >> +	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
> >> +
> >> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >> +
> >> +	/*
> >> +	 * We keep the object pages for reuse during resume. But we can unpin it
> >> +	 * now that DMA has completed, so it doesn't continue to take up space.
> >> +	 */
> >> +	i915_vma_unpin(vma);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +/**
> >> + * intel_huc_init() - initiate HuC firmware loading request
> >> + * @dev: the drm device
> >
> >Mismatched param name.
> >
> >> + *
> >> + * Called early during driver load, but after GEM is initialised. The
> >> +loading
> >> + * will continue only when driver explicitly specify firmware name and version.
> >> + * All other cases are considered as UC_FIRMWARE_NONE either because
> >> +HW is not
> >> + * capable or driver yet support it. And there will be no error
> >> +message for
> >> + * UC_FIRMWARE_NONE cases.
> >> + *
> >> + * The DMA-copying to HW is done later when intel_huc_load() is called.
> >> + */
> >> +void intel_huc_init(struct drm_i915_private *dev_priv) {
> >> +	struct intel_huc *huc = &dev_priv->huc;
> >
> >Maybe *huc shall be passed as only param (to match intel_huc function name).
> >Then dev_priv could be recreated from huc_to_i915().
> 
> Why? Can you elaborate?
> -anusha
> >
> >> +	struct intel_uc_fw *huc_fw = &huc->huc_fw;
> >> +	const char *fw_path = NULL;
> >> +
> >> +	huc_fw->uc_fw_path = NULL;
> >> +	huc_fw->fetch_status = UC_FIRMWARE_NONE;
> >> +	huc_fw->load_status = UC_FIRMWARE_NONE;
> >> +	huc_fw->fw_type = UC_FW_TYPE_HUC;
> >> +
> >> +	if (!HAS_HUC_UCODE(dev_priv))
> >> +		return;
> >> +
> >> +	if (IS_SKYLAKE(dev_priv)) {
> >> +		fw_path = I915_SKL_HUC_UCODE;
> >> +		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
> >> +		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
> >> +	}
> >> +
> >> +	huc_fw->uc_fw_path = fw_path;
> >> +	huc_fw->fetch_status = UC_FIRMWARE_PENDING;
> >> +
> >> +	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
> >> +
> >> +	intel_uc_fw_fetch(dev_priv, huc_fw); }
> >> +
> >> +/**
> >> + * intel_huc_load() - load HuC uCode to device
> >> + * @dev: the drm device
> >
> >Mismatched param name.
> >
> >
> >> + *
> >> + * Called from gem_init_hw() during driver loading and also after a GPU reset.
> >> + * Be note that HuC loading must be done before GuC loading.
> >> + *
> >> + * The firmware image should have already been fetched into memory by
> >> +the
> >> + * earlier call to intel_huc_init(), so here we need only check that
> >> + * is succeeded, and then transfer the image to the h/w.
> >> + *
> >> + * Return:	non-zero code on error
> >> + */
> >> +int intel_huc_load(struct drm_i915_private *dev_priv) {
> >> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> >> +	int err;
> >> +
> >> +	if (huc_fw->fetch_status == UC_FIRMWARE_NONE)
> >> +		return 0;
> >> +
> >> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> >> +		huc_fw->uc_fw_path,
> >> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> >> +		intel_uc_fw_status_repr(huc_fw->load_status));
> >> +
> >> +	if (huc_fw->fetch_status == UC_FIRMWARE_SUCCESS &&
> >> +	    huc_fw->load_status == UC_FIRMWARE_FAIL)
> >> +		return -ENOEXEC;
> >> +
> >> +	huc_fw->load_status = UC_FIRMWARE_PENDING;
> >> +
> >> +	switch (huc_fw->fetch_status) {
> >> +	case UC_FIRMWARE_FAIL:
> >> +		/* something went wrong :( */
> >> +		err = -EIO;
> >> +		goto fail;
> >> +
> >> +	case UC_FIRMWARE_NONE:
> >> +	case UC_FIRMWARE_PENDING:
> >> +	default:
> >> +		/* "can't happen" */
> >> +		WARN_ONCE(1, "HuC fw %s invalid fetch_status %s [%d]\n",
> >> +			huc_fw->uc_fw_path,
> >> +			intel_uc_fw_status_repr(huc_fw->fetch_status),
> >> +			huc_fw->fetch_status);
> >> +		err = -ENXIO;
> >> +		goto fail;
> >> +
> >> +	case UC_FIRMWARE_SUCCESS:
> >> +		break;
> >> +	}
> >> +
> >> +	err = huc_ucode_xfer(dev_priv);
> >> +	if (err)
> >> +		goto fail;
> >> +
> >> +	huc_fw->load_status = UC_FIRMWARE_SUCCESS;
> >> +
> >> +	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
> >> +		huc_fw->uc_fw_path,
> >> +		intel_uc_fw_status_repr(huc_fw->fetch_status),
> >> +		intel_uc_fw_status_repr(huc_fw->load_status));
> >> +
> >> +	return 0;
> >> +
> >> +fail:
> >> +	if (huc_fw->load_status == UC_FIRMWARE_PENDING)
> >> +		huc_fw->load_status = UC_FIRMWARE_FAIL;
> >> +
> >> +	DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
> >> +
> >> +	return err;
> >> +}
> >> +
> >> +/**
> >> + * intel_huc_fini() - clean up resources allocated for HuC
> >> + * @dev: the drm device
> >> + *
> >> + * Cleans up by releasing the huc firmware GEM obj.
> >> + */
> >> +void intel_huc_fini(struct drm_device *dev) {
> >> +	struct drm_i915_private *dev_priv = to_i915(dev);
> >> +	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
> >> +
> >> +	mutex_lock(&dev->struct_mutex);
> >> +	if (huc_fw->uc_fw_obj)
> >> +		i915_gem_object_put(huc_fw->uc_fw_obj);
> >> +	huc_fw->uc_fw_obj = NULL;
> >> +	mutex_unlock(&dev->struct_mutex);
> >> +
> >> +	huc_fw->fetch_status = UC_FIRMWARE_NONE; }
> >> +
> >> diff --git a/drivers/gpu/drm/i915/intel_uc.h
> >> b/drivers/gpu/drm/i915/intel_uc.h index be89f0b..ac92946 100644
> >> --- a/drivers/gpu/drm/i915/intel_uc.h
> >> +++ b/drivers/gpu/drm/i915/intel_uc.h
> >> @@ -24,6 +24,12 @@
> >>  #ifndef _INTEL_UC_H_
> >>  #define _INTEL_UC_H_
> >>
> >> +#ifndef _INTEL_HUC_H_
> >> +#define _INTEL_HUC_H_
> >
> >Typo ? This is still intel_uc.h file, right ?
> 
> Yes it is intel_uc.h. Initially the above two initializations were in intel_huc.h. But now its contents are moved to intel_uc.h. One common file for guc and huc declarations.....

But those are guards that assure us that the content of header file is
included one. The name should come strictly from the file name. One
guard is enough. So drop the _INTEL_HUC_H_ and just leave the
_INTEL_UC_H_

> >
> >> +
> >> +#define HUC_STATUS2             _MMIO(0xD3B0)
> >> +#define   HUC_FW_VERIFIED       (1<<7)
> >> +
> >
> >Is it correct place for these defs?
> >What about i915_guc_reg.h ?
> This was also initially in intel_huc.h. 
> >
> >>  #include "intel_guc_fwif.h"
> >>  #include "i915_guc_reg.h"
> >>  #include "intel_ringbuffer.h"
> >> @@ -175,6 +181,13 @@ struct intel_guc {
> >>  	struct mutex send_mutex;
> >>  };
> >>
> >> +struct intel_huc {
> >> +	/* Generic uC firmware management */
> >> +	struct intel_uc_fw huc_fw;
> >> +
> >> +	/* HuC-specific additions */
> >> +};
> >> +
> >>  /* intel_uc.c */
> >>  void intel_uc_init_early(struct drm_i915_private *dev_priv);  bool
> >> intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status); @@
> >> -191,6 +204,9 @@ extern void intel_guc_fini(struct drm_i915_private
> >> *dev_priv);  extern const char *intel_uc_fw_status_repr(enum
> >> intel_uc_fw_status status);  extern int intel_guc_suspend(struct
> >> drm_i915_private *dev_priv);  extern int intel_guc_resume(struct
> >> drm_i915_private *dev_priv);
> >> +void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> >> +	struct intel_uc_fw *uc_fw);
> >> +u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
> >
> >All other public functions have intel_ prefix.
> Got it. Will change. Thanks
> 
> Cheers,
> Anusha
> >
> >>
> >>  /* i915_guc_submission.c */
> >>  int i915_guc_submission_init(struct drm_i915_private *dev_priv); @@
> >> -205,4 +221,10 @@ void i915_guc_register(struct drm_i915_private
> >> *dev_priv);  void i915_guc_unregister(struct drm_i915_private
> >> *dev_priv);  int i915_guc_log_control(struct drm_i915_private
> >> *dev_priv, u64 control_val);
> >>
> >> +/* intel_huc_loader.c */
> >> +void intel_huc_init(struct drm_i915_private *dev_priv); void
> >> +intel_huc_fini(struct drm_device *dev); int intel_huc_load(struct
> >> +drm_i915_private *dev_priv);
> >> +
> >> +#endif
> >>  #endif
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tvrtko Ursulin Dec. 12, 2016, 6:52 p.m. UTC | #8
Hi all,

Executive decision required below:

On 08/12/2016 23:02, anushasr wrote:

[snip]

> +
> +/**
> + * DOC: HuC Firmware
> + *
> + * Motivation:
> + * GEN9 introduces a new dedicated firmware for usage in media HEVC (High
> + * Efficiency Video Coding) operations. Userspace can use the firmware
> + * capabilities by adding HuC specific commands to batch buffers.
> + *
> + * Implementation:
> + * The same firmware loader is used as the GuC. However, the actual
> + * loading to HW is deferred until GEM initialization is done.
> + *
> + * Note that HuC firmware loading must be done before GuC loading.
> + */
> +
> +#define SKL_FW_MAJOR 01
> +#define SKL_FW_MINOR 07
> +#define SKL_BLD_NUM 1398
> +
> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
> +
> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
> +	SKL_FW_MINOR, SKL_BLD_NUM)
> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);

Daniel & Jani, I understand in the GuC firmware discussions you were 
very much in the favour of encoding the full name (major and minor 
included) as the fw firmware?

Argument was that we want to in effect claim support only for one 
validated firmware binary with one version of i915.

In the case of the HuC we have a very similar situation with two key 
differences.

First, there is a build number in the file name as provided by the 
firmware team. We know that it will happen that only the build number 
changes with some fixes and the minor stays the same. And we know that 
the major indicates the interface compatibility.

Secondly, from all I can see, there are no interactions between the 
driver and the HuC firmware apart from driver loading it and thats it.

In the light of that I was advocating only using the major in the driver 
request fw name in order to improve usability both for developers 
(easier to test with different firmwares), and for users (be it 
distributions or end users - easier to upgrade the HuC firmware in case 
of codec issues by not having to patch and recompile the kernel).

Since I understand this topic has been beaten to death in the past and 
there are strong opinions on it, could you just okay (or not) the 
current proposal (as in posted patches) which encodes major, minor and 
build number in the fw name?

Regards,

Tvrtko
Jani Nikula Dec. 14, 2016, 3:19 p.m. UTC | #9
On Mon, 12 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:
> Hi all,
>
> Executive decision required below:
>
> On 08/12/2016 23:02, anushasr wrote:
>
> [snip]
>
>> +
>> +/**
>> + * DOC: HuC Firmware
>> + *
>> + * Motivation:
>> + * GEN9 introduces a new dedicated firmware for usage in media HEVC (High
>> + * Efficiency Video Coding) operations. Userspace can use the firmware
>> + * capabilities by adding HuC specific commands to batch buffers.
>> + *
>> + * Implementation:
>> + * The same firmware loader is used as the GuC. However, the actual
>> + * loading to HW is deferred until GEM initialization is done.
>> + *
>> + * Note that HuC firmware loading must be done before GuC loading.
>> + */
>> +
>> +#define SKL_FW_MAJOR 01
>> +#define SKL_FW_MINOR 07
>> +#define SKL_BLD_NUM 1398
>> +
>> +#define HUC_FW_PATH(platform, major, minor, bld_num) \
>> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
>> +	__stringify(minor) "_" __stringify(bld_num) ".bin"
>> +
>> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
>> +	SKL_FW_MINOR, SKL_BLD_NUM)
>> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>
> Daniel & Jani, I understand in the GuC firmware discussions you were 
> very much in the favour of encoding the full name (major and minor 
> included) as the fw firmware?
>
> Argument was that we want to in effect claim support only for one 
> validated firmware binary with one version of i915.
>
> In the case of the HuC we have a very similar situation with two key 
> differences.
>
> First, there is a build number in the file name as provided by the 
> firmware team. We know that it will happen that only the build number 
> changes with some fixes and the minor stays the same. And we know that 
> the major indicates the interface compatibility.
>
> Secondly, from all I can see, there are no interactions between the 
> driver and the HuC firmware apart from driver loading it and thats it.
>
> In the light of that I was advocating only using the major in the driver 
> request fw name in order to improve usability both for developers 
> (easier to test with different firmwares), and for users (be it 
> distributions or end users - easier to upgrade the HuC firmware in case 
> of codec issues by not having to patch and recompile the kernel).
>
> Since I understand this topic has been beaten to death in the past and 
> there are strong opinions on it, could you just okay (or not) the 
> current proposal (as in posted patches) which encodes major, minor and 
> build number in the fw name?

The question is the same as it ever was, can you absolutely guarantee a
new firmware version (even if just a build number bump) will not cause
regressions? Note that we'll probably only have resources to test the
latest kernel against the latest firmware, but accepting future firmware
versions means even stable kernels will start using the new firmware
versions after linux-firmware updates. We also can't easily
retroactively prevent this from happening even if we find out that there
will be breakage.

I still think we should only accept one firmware version. If we (or
someone else) has the resources to test against older kernels, commits
to enable newer firmware versions can be backported to stable.

I would love to be able to look at the firmware sources and say, oh,
there are no interactions with the kernel whatsoever, but you know how
that is. I don't know if there are interactions. I don't know if future
blobs will have interactions, and break everything if the kernel doesn't
do something the black box expects.

If you want to help developers test various firmware versions, I suggest
the same thing I suggested the last time: add an _unsafe module
parameter to specify the firmware filename/version to load.

BR,
Jani.
Parenteau, Paul A Dec. 14, 2016, 3:24 p.m. UTC | #10
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

>Jani Nikula

>Sent: Wednesday, December 14, 2016 7:20 AM

>To: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>; Srivatsa, Anusha

><anusha.srivatsa@intel.com>; intel-gfx@lists.freedesktop.org

>Subject: Re: [Intel-gfx] [PATCH 3/8] drm/i915/huc: Add HuC fw loading support

>

>On Mon, 12 Dec 2016, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote:

>> Hi all,

>>

>> Executive decision required below:

>>

>> On 08/12/2016 23:02, anushasr wrote:

>>

>> [snip]

>>

>>> +

>>> +/**

>>> + * DOC: HuC Firmware

>>> + *

>>> + * Motivation:

>>> + * GEN9 introduces a new dedicated firmware for usage in media HEVC

>>> +(High

>>> + * Efficiency Video Coding) operations. Userspace can use the

>>> +firmware

>>> + * capabilities by adding HuC specific commands to batch buffers.

>>> + *

>>> + * Implementation:

>>> + * The same firmware loader is used as the GuC. However, the actual

>>> + * loading to HW is deferred until GEM initialization is done.

>>> + *

>>> + * Note that HuC firmware loading must be done before GuC loading.

>>> + */

>>> +

>>> +#define SKL_FW_MAJOR 01

>>> +#define SKL_FW_MINOR 07

>>> +#define SKL_BLD_NUM 1398

>>> +

>>> +#define HUC_FW_PATH(platform, major, minor, bld_num) \

>>> +	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \

>>> +	__stringify(minor) "_" __stringify(bld_num) ".bin"

>>> +

>>> +#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \

>>> +	SKL_FW_MINOR, SKL_BLD_NUM)

>>> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);

>>

>> Daniel & Jani, I understand in the GuC firmware discussions you were

>> very much in the favour of encoding the full name (major and minor

>> included) as the fw firmware?

>>

>> Argument was that we want to in effect claim support only for one

>> validated firmware binary with one version of i915.

>>

>> In the case of the HuC we have a very similar situation with two key

>> differences.

>>

>> First, there is a build number in the file name as provided by the

>> firmware team. We know that it will happen that only the build number

>> changes with some fixes and the minor stays the same. And we know that

>> the major indicates the interface compatibility.

>>

>> Secondly, from all I can see, there are no interactions between the

>> driver and the HuC firmware apart from driver loading it and thats it.

>>

>> In the light of that I was advocating only using the major in the

>> driver request fw name in order to improve usability both for

>> developers (easier to test with different firmwares), and for users

>> (be it distributions or end users - easier to upgrade the HuC firmware

>> in case of codec issues by not having to patch and recompile the kernel).

>>

>> Since I understand this topic has been beaten to death in the past and

>> there are strong opinions on it, could you just okay (or not) the

>> current proposal (as in posted patches) which encodes major, minor and

>> build number in the fw name?

>

>The question is the same as it ever was, can you absolutely guarantee a new

>firmware version (even if just a build number bump) will not cause regressions?

>Note that we'll probably only have resources to test the latest kernel against the

>latest firmware, but accepting future firmware versions means even stable

>kernels will start using the new firmware versions after linux-firmware updates.

>We also can't easily retroactively prevent this from happening even if we find

>out that there will be breakage.

>

>I still think we should only accept one firmware version. If we (or someone else)

>has the resources to test against older kernels, commits to enable newer

>firmware versions can be backported to stable.

>

I agree with Jani's position here.  We have to keep some controls to the combinations of FW and kernels so we know what works.  At least until FW matures in the future.
>

>I would love to be able to look at the firmware sources and say, oh, there are no

>interactions with the kernel whatsoever, but you know how that is. I don't know

>if there are interactions. I don't know if future blobs will have interactions, and

>break everything if the kernel doesn't do something the black box expects.

>

>If you want to help developers test various firmware versions, I suggest the same

>thing I suggested the last time: add an _unsafe module parameter to specify the

>firmware filename/version to load.

>

>BR,

>Jani.

>

>

>--

>Jani Nikula, Intel Open Source Technology Center

>_______________________________________________

>Intel-gfx mailing list

>Intel-gfx@lists.freedesktop.org

>https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 3c30916..01d4f4b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -57,6 +57,7 @@  i915-y += i915_cmd_parser.o \
 # general-purpose microcontroller (GuC) support
 i915-y += intel_uc.o \
 	  intel_guc_loader.o \
+	  intel_huc_loader.o \
 	  i915_guc_submission.o
 
 # autogenerated null render state
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6428588..85a47c2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -600,6 +600,7 @@  static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_irq;
 
+	intel_huc_init(dev_priv);
 	intel_guc_init(dev_priv);
 
 	ret = i915_gem_init(dev_priv);
@@ -627,6 +628,7 @@  static int i915_load_modeset_init(struct drm_device *dev)
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
 cleanup_irq:
+	intel_huc_fini(dev);
 	intel_guc_fini(dev_priv);
 	drm_irq_uninstall(dev);
 	intel_teardown_gmbus(dev_priv);
@@ -1313,7 +1315,7 @@  void i915_driver_unload(struct drm_device *dev)
 
 	/* Flush any outstanding unpin_work. */
 	drain_workqueue(dev_priv->wq);
-
+	intel_huc_fini(dev);
 	intel_guc_fini(dev_priv);
 	i915_gem_fini(dev_priv);
 	intel_fbc_cleanup_cfb(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1480e73..0371ca4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2036,6 +2036,7 @@  struct drm_i915_private {
 
 	struct intel_gvt *gvt;
 
+	struct intel_huc huc;
 	struct intel_guc guc;
 
 	struct intel_csr csr;
@@ -2810,7 +2811,7 @@  intel_info(const struct drm_i915_private *dev_priv)
 #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
 #define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
-
+#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
 #define HAS_RESOURCE_STREAMER(dev_priv) ((dev_priv)->info.has_resource_streamer)
 
 #define HAS_POOLED_EU(dev_priv)	((dev_priv)->info.has_pooled_eu)
diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h b/drivers/gpu/drm/i915/i915_guc_reg.h
index 5e638fc..f9829f6 100644
--- a/drivers/gpu/drm/i915/i915_guc_reg.h
+++ b/drivers/gpu/drm/i915/i915_guc_reg.h
@@ -61,9 +61,12 @@ 
 #define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
 #define DMA_COPY_SIZE			_MMIO(0xc310)
 #define DMA_CTRL			_MMIO(0xc314)
+#define   HUC_UKERNEL			  (1<<9)
 #define   UOS_MOVE			  (1<<4)
 #define   START_DMA			  (1<<0)
 #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
+#define   HUC_LOADING_AGENT_VCR		  (0<<1)
+#define   HUC_LOADING_AGENT_GUC		  (1<<1)
 #define   GUC_WOPCM_OFFSET_VALUE	  0x80000	/* 512KB */
 #define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 26a184f..b971351 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -309,8 +309,8 @@  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
 
 	/* Finally start the DMA */
-	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
-
+	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA) |
+		_MASKED_BIT_DISABLE(HUC_UKERNEL));
 	/*
 	 * Wait for the DMA to complete & the GuC to start up.
 	 * NB: Docs recommend not using the interrupt for completion.
@@ -334,7 +334,7 @@  static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv,
 	return ret;
 }
 
-static u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
+u32 guc_wopcm_size(struct drm_i915_private *dev_priv)
 {
 	u32 wopcm_size = GUC_WOPCM_TOP;
 
@@ -511,6 +511,7 @@  int intel_guc_setup(struct drm_i915_private *dev_priv)
 		if (err)
 			goto fail;
 
+		intel_huc_load(dev_priv);
 		err = guc_ucode_xfer(dev_priv);
 		if (!err)
 			break;
diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
new file mode 100644
index 0000000..e0efd1c
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_huc_loader.c
@@ -0,0 +1,264 @@ 
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+#include <linux/firmware.h>
+#include "i915_drv.h"
+#include "intel_uc.h"
+
+/**
+ * DOC: HuC Firmware
+ *
+ * Motivation:
+ * GEN9 introduces a new dedicated firmware for usage in media HEVC (High
+ * Efficiency Video Coding) operations. Userspace can use the firmware
+ * capabilities by adding HuC specific commands to batch buffers.
+ *
+ * Implementation:
+ * The same firmware loader is used as the GuC. However, the actual
+ * loading to HW is deferred until GEM initialization is done.
+ *
+ * Note that HuC firmware loading must be done before GuC loading.
+ */
+
+#define SKL_FW_MAJOR 01
+#define SKL_FW_MINOR 07
+#define SKL_BLD_NUM 1398
+
+#define HUC_FW_PATH(platform, major, minor, bld_num) \
+	"i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
+	__stringify(minor) "_" __stringify(bld_num) ".bin"
+
+#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_FW_MAJOR, \
+	SKL_FW_MINOR, SKL_BLD_NUM)
+MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
+
+/**
+ * huc_ucode_xfer() - DMA's the firmware
+ * @dev_priv: the drm device
+ *
+ * This function takes the gem object containing the firmware, sets up the DMA
+ * engine MMIO, triggers the DMA operation and waits for it to finish.
+ *
+ * Transfer the firmware image to RAM for execution by the microcontroller.
+ *
+ * Return: 0 on success, non-zero on failure
+ */
+
+static int huc_ucode_xfer(struct drm_i915_private *dev_priv)
+{
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
+	struct i915_vma *vma;
+	unsigned long offset = 0;
+	u32 size;
+	int ret;
+
+	ret = i915_gem_object_set_to_gtt_domain(huc_fw->uc_fw_obj, false);
+	if (ret) {
+		DRM_DEBUG_DRIVER("set-domain failed %d\n", ret);
+		return ret;
+	}
+
+	vma = i915_gem_object_ggtt_pin(huc_fw->uc_fw_obj, NULL, 0, 0, 0);
+	if (IS_ERR(vma)) {
+		DRM_DEBUG_DRIVER("pin failed %d\n", (int)PTR_ERR(vma));
+		return PTR_ERR(vma);
+	}
+
+	/* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
+	I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
+
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
+	/* init WOPCM */
+	I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
+	I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE |
+			HUC_LOADING_AGENT_GUC);
+
+	/* Set the source address for the uCode */
+	offset = i915_ggtt_offset(vma) + huc_fw->header_offset;
+	I915_WRITE(DMA_ADDR_0_LOW, lower_32_bits(offset));
+	I915_WRITE(DMA_ADDR_0_HIGH, upper_32_bits(offset) & 0xFFFF);
+
+	/* Hardware doesn't look at destination address for HuC. Set it to 0,
+	 * but still program the correct address space.
+	 */
+	I915_WRITE(DMA_ADDR_1_LOW, 0);
+	I915_WRITE(DMA_ADDR_1_HIGH, DMA_ADDRESS_SPACE_WOPCM);
+
+	size = huc_fw->header_size + huc_fw->ucode_size;
+	I915_WRITE(DMA_COPY_SIZE, size);
+
+	/* Start the DMA */
+	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(HUC_UKERNEL | START_DMA));
+
+	/* Wait for DMA to finish */
+	ret = wait_for((I915_READ(DMA_CTRL) & START_DMA) == 0, 100);
+
+	DRM_DEBUG_DRIVER("HuC DMA transfer wait over with ret %d\n", ret);
+
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
+	/*
+	 * We keep the object pages for reuse during resume. But we can unpin it
+	 * now that DMA has completed, so it doesn't continue to take up space.
+	 */
+	i915_vma_unpin(vma);
+
+	return ret;
+}
+
+/**
+ * intel_huc_init() - initiate HuC firmware loading request
+ * @dev: the drm device
+ *
+ * Called early during driver load, but after GEM is initialised. The loading
+ * will continue only when driver explicitly specify firmware name and version.
+ * All other cases are considered as UC_FIRMWARE_NONE either because HW is not
+ * capable or driver yet support it. And there will be no error message for
+ * UC_FIRMWARE_NONE cases.
+ *
+ * The DMA-copying to HW is done later when intel_huc_load() is called.
+ */
+void intel_huc_init(struct drm_i915_private *dev_priv)
+{
+	struct intel_huc *huc = &dev_priv->huc;
+	struct intel_uc_fw *huc_fw = &huc->huc_fw;
+	const char *fw_path = NULL;
+
+	huc_fw->uc_fw_path = NULL;
+	huc_fw->fetch_status = UC_FIRMWARE_NONE;
+	huc_fw->load_status = UC_FIRMWARE_NONE;
+	huc_fw->fw_type = UC_FW_TYPE_HUC;
+
+	if (!HAS_HUC_UCODE(dev_priv))
+		return;
+
+	if (IS_SKYLAKE(dev_priv)) {
+		fw_path = I915_SKL_HUC_UCODE;
+		huc_fw->major_ver_wanted = SKL_FW_MAJOR;
+		huc_fw->minor_ver_wanted = SKL_FW_MINOR;
+	}
+
+	huc_fw->uc_fw_path = fw_path;
+	huc_fw->fetch_status = UC_FIRMWARE_PENDING;
+
+	DRM_DEBUG_DRIVER("HuC firmware pending, path %s\n", fw_path);
+
+	intel_uc_fw_fetch(dev_priv, huc_fw);
+}
+
+/**
+ * intel_huc_load() - load HuC uCode to device
+ * @dev: the drm device
+ *
+ * Called from gem_init_hw() during driver loading and also after a GPU reset.
+ * Be note that HuC loading must be done before GuC loading.
+ *
+ * The firmware image should have already been fetched into memory by the
+ * earlier call to intel_huc_init(), so here we need only check that
+ * is succeeded, and then transfer the image to the h/w.
+ *
+ * Return:	non-zero code on error
+ */
+int intel_huc_load(struct drm_i915_private *dev_priv)
+{
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
+	int err;
+
+	if (huc_fw->fetch_status == UC_FIRMWARE_NONE)
+		return 0;
+
+	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
+		huc_fw->uc_fw_path,
+		intel_uc_fw_status_repr(huc_fw->fetch_status),
+		intel_uc_fw_status_repr(huc_fw->load_status));
+
+	if (huc_fw->fetch_status == UC_FIRMWARE_SUCCESS &&
+	    huc_fw->load_status == UC_FIRMWARE_FAIL)
+		return -ENOEXEC;
+
+	huc_fw->load_status = UC_FIRMWARE_PENDING;
+
+	switch (huc_fw->fetch_status) {
+	case UC_FIRMWARE_FAIL:
+		/* something went wrong :( */
+		err = -EIO;
+		goto fail;
+
+	case UC_FIRMWARE_NONE:
+	case UC_FIRMWARE_PENDING:
+	default:
+		/* "can't happen" */
+		WARN_ONCE(1, "HuC fw %s invalid fetch_status %s [%d]\n",
+			huc_fw->uc_fw_path,
+			intel_uc_fw_status_repr(huc_fw->fetch_status),
+			huc_fw->fetch_status);
+		err = -ENXIO;
+		goto fail;
+
+	case UC_FIRMWARE_SUCCESS:
+		break;
+	}
+
+	err = huc_ucode_xfer(dev_priv);
+	if (err)
+		goto fail;
+
+	huc_fw->load_status = UC_FIRMWARE_SUCCESS;
+
+	DRM_DEBUG_DRIVER("%s fw status: fetch %s, load %s\n",
+		huc_fw->uc_fw_path,
+		intel_uc_fw_status_repr(huc_fw->fetch_status),
+		intel_uc_fw_status_repr(huc_fw->load_status));
+
+	return 0;
+
+fail:
+	if (huc_fw->load_status == UC_FIRMWARE_PENDING)
+		huc_fw->load_status = UC_FIRMWARE_FAIL;
+
+	DRM_ERROR("Failed to complete HuC uCode load with ret %d\n", err);
+
+	return err;
+}
+
+/**
+ * intel_huc_fini() - clean up resources allocated for HuC
+ * @dev: the drm device
+ *
+ * Cleans up by releasing the huc firmware GEM obj.
+ */
+void intel_huc_fini(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_uc_fw *huc_fw = &dev_priv->huc.huc_fw;
+
+	mutex_lock(&dev->struct_mutex);
+	if (huc_fw->uc_fw_obj)
+		i915_gem_object_put(huc_fw->uc_fw_obj);
+	huc_fw->uc_fw_obj = NULL;
+	mutex_unlock(&dev->struct_mutex);
+
+	huc_fw->fetch_status = UC_FIRMWARE_NONE;
+}
+
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index be89f0b..ac92946 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -24,6 +24,12 @@ 
 #ifndef _INTEL_UC_H_
 #define _INTEL_UC_H_
 
+#ifndef _INTEL_HUC_H_
+#define _INTEL_HUC_H_
+
+#define HUC_STATUS2             _MMIO(0xD3B0)
+#define   HUC_FW_VERIFIED       (1<<7)
+
 #include "intel_guc_fwif.h"
 #include "i915_guc_reg.h"
 #include "intel_ringbuffer.h"
@@ -175,6 +181,13 @@  struct intel_guc {
 	struct mutex send_mutex;
 };
 
+struct intel_huc {
+	/* Generic uC firmware management */
+	struct intel_uc_fw huc_fw;
+
+	/* HuC-specific additions */
+};
+
 /* intel_uc.c */
 void intel_uc_init_early(struct drm_i915_private *dev_priv);
 bool intel_guc_recv(struct drm_i915_private *dev_priv, u32 *status);
@@ -191,6 +204,9 @@  extern void intel_guc_fini(struct drm_i915_private *dev_priv);
 extern const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status);
 extern int intel_guc_suspend(struct drm_i915_private *dev_priv);
 extern int intel_guc_resume(struct drm_i915_private *dev_priv);
+void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
+	struct intel_uc_fw *uc_fw);
+u32 guc_wopcm_size(struct drm_i915_private *dev_priv);
 
 /* i915_guc_submission.c */
 int i915_guc_submission_init(struct drm_i915_private *dev_priv);
@@ -205,4 +221,10 @@  void i915_guc_register(struct drm_i915_private *dev_priv);
 void i915_guc_unregister(struct drm_i915_private *dev_priv);
 int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
 
+/* intel_huc_loader.c */
+void intel_huc_init(struct drm_i915_private *dev_priv);
+void intel_huc_fini(struct drm_device *dev);
+int intel_huc_load(struct drm_i915_private *dev_priv);
+
+#endif
 #endif