Re: [PATCH 3/6] drm/i915/huc: Add HuC fw loading support

Peter Antoine
2016-06-23T13:52:44Z

[-- Attachment #1: Type: TEXT/PLAIN, Size: 21101 bytes --]

On Thu, 23 Jun 2016, Dave Gordon wrote:

> On 22/06/16 09:31, Daniel Vetter wrote:
>> On Tue, Jun 21, 2016 at 07:11:22PM +0100, Peter Antoine wrote:
>>> From: Alex Dai <yu.dai@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.
>>>
>>> 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_dma.c | 3 +
>>> drivers/gpu/drm/i915/i915_drv.h | 3 +
>>> drivers/gpu/drm/i915/i915_gem.c | 13 +-
>>> drivers/gpu/drm/i915/i915_guc_reg.h | 3 +
>>> drivers/gpu/drm/i915/intel_guc.h | 1 +
>>> drivers/gpu/drm/i915/intel_guc_loader.c | 11 +-
>>> drivers/gpu/drm/i915/intel_huc.h | 44 ++++++
>>> drivers/gpu/drm/i915/intel_huc_loader.c | 262
>>> ++++++++++++++++++++++++++++++++
>>> 9 files changed, 331 insertions(+), 10 deletions(-)
>>> create mode 100644 drivers/gpu/drm/i915/intel_huc.h
>>> 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 276abf1..7384db0 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -44,6 +44,7 @@ i915-y += i915_cmd_parser.o \
>>>
>>> # general-purpose microcontroller (GuC) support
>>> i915-y += intel_guc_loader.o \
>>> + intel_huc_loader.o \
>>> i915_guc_submission.o
>>>
>>> # autogenerated null render state
>>> diff --git a/drivers/gpu/drm/i915/i915_dma.c
>>> b/drivers/gpu/drm/i915/i915_dma.c
>>> index e28c0dd..a449a59 100644
>>> --- a/drivers/gpu/drm/i915/i915_dma.c
>>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>>> @@ -507,6 +507,7 @@ static int i915_load_modeset_init(struct drm_device
>>> *dev)
>>> * working irqs for e.g. gmbus and dp aux transfers. */
>>> intel_modeset_init(dev);
>>>
>>> + intel_huc_init(dev);
>>> intel_guc_init(dev);
>>>
>>> ret = i915_gem_init(dev);
>>> @@ -532,6 +533,7 @@ static int i915_load_modeset_init(struct drm_device
>>> *dev)
>>> cleanup_gem:
>>> i915_gem_fini(dev);
>>> cleanup_irq:
>>> + intel_huc_fini(dev);
>>> intel_guc_fini(dev);
>>> drm_irq_uninstall(dev);
>>> intel_teardown_gmbus(dev);
>>> @@ -1563,6 +1565,7 @@ int i915_driver_unload(struct drm_device *dev)
>>> /* Flush any outstanding unpin_work. */
>>> flush_workqueue(dev_priv->wq);
>>>
>>> + intel_huc_fini(dev);
>>> intel_guc_fini(dev);
>>> i915_gem_fini(dev);
>>> 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 7c81757..8b88227 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -54,6 +54,7 @@
>>> #include "intel_bios.h"
>>> #include "intel_dpll_mgr.h"
>>> #include "intel_guc.h"
>>> +#include "intel_huc.h"
>>> #include "intel_lrc.h"
>>> #include "intel_ringbuffer.h"
>>>
>>> @@ -1749,6 +1750,7 @@ struct drm_i915_private {
>>>
>>> struct intel_gvt gvt;
>>>
>>> + struct intel_huc huc;
>>> struct intel_guc guc;
>>>
>>> struct intel_csr csr;
>>> @@ -2841,6 +2843,7 @@ struct drm_i915_cmd_table {
>>> #define HAS_GUC(dev) (IS_GEN9(dev) && !IS_KABYLAKE(dev))
>>> #define HAS_GUC_UCODE(dev) (HAS_GUC(dev))
>>> #define HAS_GUC_SCHED(dev) (HAS_GUC(dev))
>>> +#define HAS_HUC_UCODE(dev) (HAS_GUC(dev))
>>>
>>> #define HAS_RESOURCE_STREAMER(dev) (IS_HASWELL(dev) || \
>>> INTEL_INFO(dev)->gen >= 8)
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
>>> b/drivers/gpu/drm/i915/i915_gem.c
>>> index 6abd5e5..549dd3f 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -5143,9 +5143,16 @@ i915_gem_init_hw(struct drm_device *dev)
>>> intel_mocs_init_l3cc_table(dev);
>>>
>>> /* We can't enable contexts until all firmware is loaded */
>>> - ret = intel_guc_setup(dev);
>>> - if (ret)
>>> - goto out;
>>> + if (HAS_GUC(dev)) {
>>> + /* init WOPCM */
>>> + I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev));
>>> + I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
>>> +
>>> + intel_huc_load(dev);
>>> + ret = intel_guc_setup(dev);
>>> + if (ret)
>>> + goto out;
>>> + }
>>>
>>> /*
>>> * Increment the next seqno by 0x100 so we have a visible break
>>> diff --git a/drivers/gpu/drm/i915/i915_guc_reg.h
>>> b/drivers/gpu/drm/i915/i915_guc_reg.h
>>> index cf5a65b..51533f1 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.h
>>> b/drivers/gpu/drm/i915/intel_guc.h
>>> index 0b4ed88..c4b92f3 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc.h
>>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>>> @@ -160,6 +160,7 @@ extern const char *intel_uc_fw_status_repr(enum
>>> intel_uc_fw_status status);
>>> extern int intel_guc_suspend(struct drm_device *dev);
>>> extern int intel_guc_resume(struct drm_device *dev);
>>> void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw
>>> *uc_fw);
>>> +u32 guc_wopcm_size(struct drm_device *dev);
>>>
>>> /* i915_guc_submission.c */
>>> int i915_guc_submission_init(struct drm_i915_private *dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> index 70575bd..c4a210d 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> @@ -270,7 +270,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.
>>> @@ -295,12 +296,12 @@ 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_device *dev)
>>> {
>>> u32 wopcm_size = GUC_WOPCM_TOP;
>>>
>>> /* On BXT, the top of WOPCM is reserved for RC6 context */
>>> - if (IS_BROXTON(dev_priv))
>>> + if (IS_BROXTON(dev))
>>> wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
>>>
>>> return wopcm_size;
>>> @@ -332,10 +333,6 @@ static int guc_ucode_xfer(struct drm_i915_private
>>> *dev_priv)
>>>
>>> intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>>
>>> - /* init WOPCM */
>>> - I915_WRITE(GUC_WOPCM_SIZE, guc_wopcm_size(dev_priv));
>>> - I915_WRITE(DMA_GUC_WOPCM_OFFSET, GUC_WOPCM_OFFSET_VALUE);
>>> -
>>> /* Enable MIA caching. GuC clock gating is disabled. */
>>> I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_huc.h
>>> b/drivers/gpu/drm/i915/intel_huc.h
>>> new file mode 100644
>>> index 0000000..946caa7
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>>> @@ -0,0 +1,44 @@
>>> +/*
>>> + * Copyright © 2014 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.
>>> + *
>>> + */
>>> +#ifndef _INTEL_HUC_H_
>>> +#define _INTEL_HUC_H_
>>> +
>>> +#include "intel_guc.h"
>>> +
>>> +#define HUC_STATUS2 _MMIO(0xD3B0)
>>> +#define HUC_FW_VERIFIED (1<<7)
>>> +
>>> +struct intel_huc {
>>> + /* Generic uC firmware management */
>>> + struct intel_uc_fw huc_fw;
>>> +
>>> + /* HuC-specific additions */
>>> +};
>>> +
>>> +extern void intel_huc_init(struct drm_device *dev);
>>> +extern int intel_huc_load(struct drm_device *dev);
>>> +extern void intel_huc_auth(struct drm_device *dev);
>>> +extern void intel_huc_fini(struct drm_device *dev);
>>> +
>>> +#endif
>>> 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..472fabe
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/intel_huc_loader.c
>>> @@ -0,0 +1,262 @@
>>> +/*
>>> + * Copyright © 2014 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_huc.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:
>>> + * On supported platforms, i915's job is to load the firmware stored on
>>> the
>>> + * file system and assist with authentication. It is up to userspace to
>>> + * detect the presence of HuC support on a platform, on their own.
>>> + * For debugging, i915 provides a debugfs file, i915_huc_load_status_info
>>> + * which displays the firmware load status.
>>> + *
>>> + * The unified uC firmware loader is used. Firmware binary is fetched by
>>> the
>>> + * loader asynchronously from the driver init process. However, the
>>> actual
>>> + * loading to HW is deferred until GEM initialization is done. Be note
>>> that HuC
>>> + * firmware loading must be done before GuC loading.
>>> + */
>>> +
>>> +#define I915_SKL_HUC_UCODE "i915/skl_huc_ver1.bin"
>>> +MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
>>> +
>>> +/**
>>> + * intel_huc_load_ucode() - DMA's the firmware
>>> + * @dev: 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;
>>> + 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;
>>> + }
>>> +
>>> + ret = i915_gem_obj_ggtt_pin(huc_fw->uc_fw_obj, 0, 0);
>>> + if (ret) {
>>> + DRM_DEBUG_DRIVER("pin failed %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + /* Invalidate GuC TLB to let GuC take the latest updates to GTT. */
>>> + I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>>> +
>>> + WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>>> + intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
>>> +
>>> + /* Set the source address for the uCode */
>>> + offset = i915_gem_obj_ggtt_offset(huc_fw->uc_fw_obj) +
>>> + 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_atomic((I915_READ(DMA_CTRL) & START_DMA) == 0, 50);
>>> +
>>> + 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_gem_object_ggtt_unpin(huc_fw->uc_fw_obj);
>>> +
>>> + 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_ucode_load() is
>>> called.
>>> + */
>>> +void intel_huc_init(struct drm_device *dev)
>>> +{
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + struct intel_huc *huc = &dev_priv->huc;
>>> + struct intel_uc_fw *huc_fw = &huc->huc_fw;
>>> + const char *fw_path = NULL;
>>> +
>>> + huc_fw->uc_dev = dev;
>>> + 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))
>>> + return;
>>> +
>>> + if (IS_SKYLAKE(dev)) {
>>> + fw_path = I915_SKL_HUC_UCODE;
>>> + huc_fw->major_ver_wanted = 1;
>>> + huc_fw->minor_ver_wanted = 5;
>>> + }
>>> +
>>> + if (fw_path == NULL)
>>> + return;
>>> +
>>> + 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, 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_ucode_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_device *dev)
>>> +{
>>> + struct drm_i915_private *dev_priv = dev->dev_private;
>>> + 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;
>>> + }
>>
>> Can we please remove all that indirection through fetch_status? This
>> really shouldn't be that complicated that you need pages of code to
>> generate/decode status. I know it's copypaste from guc, but I didn't like
>> it there, and I definitely don't want 2 copies of this. Please simplify
>> both places.
>>
>> The correct fix is to give the uc_fw_fetch function an error code, and rip
>> out all that cargo culted status handling. All of it.
>> -Daniel
>
> No, the *correct* fix is to unify all the firmware loaders we have.
> There should just be ONE piece of code that can be used to fetch and load ANy
> firmware into ANY auxiliary microcontroller. NOT one per microcontroller, all
> different -- that way lies madness.
>
> We already had a unified loader for the HuC and GuC a year ago, but IIRC the
> party line then was "just make it (GuC) specific, then copypaste it for the
> second uC, and when we've got three versions we'll have learnt how we really
> want a unified loader to behave."
>
> Well. here's the copypaste, and we already have a different loader for the
> DMC/CSR, so it must be time for (re-)unification.
>
> .Dave.
>

Just to add, if you uc_fw_fetch() has an error code you will still have to
remember the state of the fetch or at each reset/resume/etc... or you will
have to try the firmware load again and that can take a long time. So the
state will have to be re-instated.

Seeing this code was written with the given goals and were written in the
same vane as code that was deemed acceptable, it seems weird at this late
stage to change the design goals.

Note: this is the third time that these patches have been posted and were
only rejected (as far as I know) due to no open-source user. Which there
is now, and is why I have reposted these patches.

Peter.

https://patchwork.freedesktop.org/series/9011/ 22/6/2016
https://patchwork.freedesktop.org/series/3188/ 9/2/2016
https://patchwork.freedesktop.org/series/2355/ 11/1/2016

--
Peter Antoine (Android Graphics Driver Software Engineer)
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]