>-----Original Message----- >From: Michal Wajdeczko [mailto:michal.wajdeczko@xxxxxxxxxxxxxxx] >Sent: Friday, December 9, 2016 4:18 AM >To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx> >Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Alex Dai <yu.dai@xxxxxxxxx>; Peter Antoine ><peter.antoine@xxxxxxxxx> >Subject: Re: [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@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> Tested-by: Xiang Haihao <haihao.xiang@xxxxxxxxx> >> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> >> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx> >> Signed-off-by: Peter Antoine <peter.antoine@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
