On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote:
> From: "A.Sunil Kamath" <sunil.kamath@xxxxxxxxx>
>
> Display Context Save and Restore support is needed for
> various SKL Display C states like DC5, DC6.
>
> This implementation is added based on first version of DMC CSR program
> that we received from h/w team.
>
> Here we are using request_firmware based design.
> Finally this firmware should end up in linux-firmware tree.
>
> For SKL platform its mandatory to ensure that we load this
> csr program before enabling DC states like DC5/DC6.
>
> As CSR program gets reset on various conditions, we should ensure
> to load it during boot and in future change to be added to load
> this system resume sequence too.
>
> v1: Initial relese as RFC patch
>
> v2: Design change as per Daniel, Damien and Shobit's review comments
> request firmware method followed.
>
> v3: Some optimization and functional changes.
> Pulled register defines into drivers/gpu/drm/i915/i915_reg.h
> Used kmemdup to allocate and duplicate firmware content.
> Ensured to free allocated buffer.
>
> v4: Modified as per review comments from Satheesh and Daniel
> Removed temporary buffer.
> Optimized number of writes by replacing I915_WRITE with I915_WRITE64.
>
> v5:
> Modified as per review comemnts from Damien.
> - Changed name for functions and firmware.
> - Introduced HAS_CSR.
> - Reverted back previous change and used csr_buf with u8 size.
> - Using cpu_to_be64 for endianness change.
>
> Modified as per review comments from Imre.
> - Modified registers and macro names to be a bit closer to bspec terminology
> and the existing register naming in the driver.
> - Early return for non SKL platforms in intel_load_csr_program function.
> - Added locking around CSR program load function as it may be called
> concurrently during system/runtime resume.
> - Releasing the fw before loading the program for consistency
> - Handled error path during f/w load.
>
> v6: Modified as per review comments from Imre.
> - Corrected out_freecsr sequence.
>
> v7: Modified as per review comments from Imre.
> Fail loading fw if fw->size%8!=0.
>
> v8: Rebase to latest.
>
> v9: Rebase on top of -nightly (Damien)
>
> v10: Enabled support for dmc firmware ver 1.0.
> According to ver 1.0 in a single binary package all the firmware's that are
> required for different stepping's of the product will be stored. The package
> contains the css header, followed by the package header and the actual dmc
> firmwares. Package header contains the firmware/stepping mapping table and
> the corresponding firmware offsets to the individual binaries, within the
> package. Each individual program binary contains the header and the payload
> sections whose size is specified in the header section. This changes are done
> to extract the specific firmaware from the package. (Animesh)
>
> v11: Modified as per review comemnts from Imre.
> - Added code comment from bpec for header structure elements.
> - Added __packed to avoid structure padding.
> - Added helper functions for stepping and substepping info.
> - Added code comment for CSR_MAX_FW_SIZE.
> - Disabled BXT firmware loading, will be enabled with dmc 1.0 support.
> - Changed skl_stepping_info based on bspec, earlier used from config DB.
> - Removed duplicate call of cpu_to_be* from intel_csr_load_program function.
> - Used cpu_to_be32 instead of cpu_to_be64 as firmware binary in dword aligned.
> - Added sanity check for header length.
> - Added sanity check for mmio address got from firmware binary.
> - kmalloc done separately for dmc header and dmc firmware. (Animesh)
>
> v12: Modified as per review comemnts from Imre.
> - Corrected the typo error in skl stepping info structure.
> - Added out-of-bound access for skl_stepping_info.
> - Sanity check for mmio address modified.
> - Sanity check added for stepping and substeppig.
> - Modified the intel_dmc_info structure, cache only the required header info. (Animesh)
>
> v13: clarify firmware load error message.
> The reason for a firmware loading failure can be obscure if the driver
> is built-in. Provide an explanation to the user about the likely reason for
> the failure and how to resolve it. (Imre)
>
> v14: Suggested by Jani.
> - fix s/I915/CONFIG_DRM_I915/ typo
> - add fw_path to the firmware object instead of using a static ptr (Jani)
>
> Issue: VIZ-2569
> Signed-off-by: A.Sunil Kamath <sunil.kamath@xxxxxxxxx>
> Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/Makefile | 3 +-
> drivers/gpu/drm/i915/i915_dma.c | 12 +-
> drivers/gpu/drm/i915/i915_drv.c | 20 +++
> drivers/gpu/drm/i915/i915_drv.h | 132 ++++++++++++++++++++
> drivers/gpu/drm/i915/i915_reg.h | 18 +++
> drivers/gpu/drm/i915/intel_csr.c | 262 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 5 +
> 7 files changed, 450 insertions(+), 2 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/intel_csr.c
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index a69002e..5238deb 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -12,7 +12,8 @@ i915-y := i915_drv.o \
> i915_suspend.o \
> i915_sysfs.o \
> intel_pm.o \
> - intel_runtime_pm.o
> + intel_runtime_pm.o \
> + intel_csr.o
>
> i915-$(CONFIG_COMPAT) += i915_ioc32.o
> i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 68e0c85..4d92429 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -783,6 +783,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> spin_lock_init(&dev_priv->mmio_flip_lock);
> mutex_init(&dev_priv->dpio_lock);
> mutex_init(&dev_priv->modeset_restore_lock);
> + mutex_init(&dev_priv->csr_lock);
>
> intel_pm_setup(dev);
>
> @@ -828,10 +829,15 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
> intel_uncore_init(dev);
>
> - ret = i915_gem_gtt_init(dev);
> + /* Load CSR Firmware for SKL */
> + ret = intel_csr_ucode_init(dev);
> if (ret)
> goto out_regs;
I missed this earlier, but we should just continue here in case of error
and don't fail loading the driver.
>
> + ret = i915_gem_gtt_init(dev);
> + if (ret)
> + goto out_freecsr;
> +
> /* WARNING: Apparently we must kick fbdev drivers before vgacon,
> * otherwise the vga fbdev driver falls over. */
> ret = i915_kick_out_firmware_fb(dev_priv);
> @@ -1000,6 +1006,8 @@ out_mtrrfree:
> io_mapping_free(dev_priv->gtt.mappable);
> out_gtt:
> i915_global_gtt_cleanup(dev);
> +out_freecsr:
> + intel_csr_ucode_fini(dev);
> out_regs:
> intel_uncore_fini(dev);
> pci_iounmap(dev->pdev, dev_priv->regs);
> @@ -1077,6 +1085,8 @@ int i915_driver_unload(struct drm_device *dev)
> mutex_unlock(&dev->struct_mutex);
> i915_gem_cleanup_stolen(dev);
>
> + intel_csr_ucode_fini(dev);
> +
> intel_teardown_gmbus(dev);
> intel_teardown_mchbar(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 82f8be4..489caa6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -543,6 +543,26 @@ void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
> cancel_delayed_work_sync(&dev_priv->hotplug_reenable_work);
> }
>
> +void i915_firmware_load_error_print(const char *fw_path, int err)
> +{
> + DRM_ERROR("failed to load firmware %s (%d)\n", fw_path, err);
> +
> + /*
> + * If the reason is not known assume -ENOENT since that's the most
> + * usual failure mode.
> + */
> + if (!err)
> + err = -ENOENT;
> +
> + if (!(IS_BUILTIN(CONFIG_DRM_I915) && err == -ENOENT))
> + return;
> +
> + DRM_ERROR(
> + "The driver is built-in, so to load the firmware you need to\n"
> + "include it either in the kernel (see CONFIG_EXTRA_FIRMWARE) or\n"
> + "in your initrd/initramfs image.\n");
> +}
> +
> static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
> {
> struct drm_device *dev = dev_priv->dev;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ef320c..dd572a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -666,6 +666,12 @@ struct intel_uncore {
> #define for_each_fw_domain(domain__, dev_priv__, i__) \
> for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>
> +struct intel_csr {
> + int csr_size;
> + u8 *csr_buf;
csr_size isn't used and csr_buf is only used locally, so they can be
removed from here.
> + const char *fw_path;
> +};
> +
> #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> func(is_mobile) sep \
> func(is_i85x) sep \
> @@ -1560,6 +1566,123 @@ struct i915_virtual_gpu {
> bool active;
> };
>
> +struct intel_css_header {
> + /* 0x09 for DMC */
> + uint32_t module_type;
> +
> + /* Includes the DMC specific header in dwords */
> + uint32_t header_len;
> +
> + /* always value would be 0x10000 */
> + uint32_t header_ver;
> +
> + /* Not used */
> + uint32_t module_id;
> +
> + /* Not used */
> + uint32_t module_vendor;
> +
> + /* in YYYYMMDD format */
> + uint32_t date;
> +
> + /* Size in dwords (CSS_Headerlen + PackageHeaderLen + dmc FWsLen)/4 */
> + uint32_t size;
> +
> + /* Not used */
> + uint32_t key_size;
> +
> + /* Not used */
> + uint32_t modulus_size;
> +
> + /* Not used */
> + uint32_t exponent_size;
> +
> + /* Not used */
> + uint32_t reserved1[12];
> +
> + /* Major Minor */
> + uint32_t version;
> +
> + /* Not used */
> + uint32_t reserved2[8];
> +
> + /* Not used */
> + uint32_t kernel_header_info;
> +} __packed;
> +
> +struct intel_fw_info {
> + uint16_t reserved1;
> +
> + /* Stepping (A, B, C, ..., *). * is a wildcard */
> + char stepping;
> +
> + /* Sub-stepping (0, 1, ..., *). * is a wildcard */
> + char substepping;
> +
> + uint32_t offset;
> + uint32_t reserved2;
> +} __packed;
> +
> +struct intel_package_header {
> + /* DMC container header length in dwords */
> + unsigned char header_len;
> +
> + /* always value would be 0x01 */
> + unsigned char header_ver;
> +
> + unsigned char reserved[10];
> +
> + /* Number of valid entries in the FWInfo array below */
> + uint32_t num_entries;
> +
> + struct intel_fw_info fw_info[20];
> +} __packed;
> +
> +struct intel_dmc_header {
> + /* always value would be 0x40403E3E */
> + uint32_t signature;
> +
> + /* DMC binary header length */
> + unsigned char header_len;
> +
> + /* 0x01 */
> + unsigned char header_ver;
> +
> + /* Reserved */
> + uint16_t dmcc_ver;
> +
> + /* Major, Minor */
> + uint32_t project;
> +
> + /* Firmware program size (excluding header) in dwords */
> + uint32_t fw_size;
> +
> + /* Major Minor version */
> + uint32_t fw_version;
> +
> + /* Number of valid MMIO cycles present. */
> + uint32_t mmio_count;
> +
> + /* MMIO address */
> + uint32_t mmioaddr[8];
> +
> + /* MMIO data */
> + uint32_t mmiodata[8];
> +
> + /* FW filename */
> + unsigned char dfile[32];
> +
> + uint32_t reserved1[2];
> +} __packed;
All the above __packed structs are used only in intel_csr.c, so they can
be defined there without export them.
> +
> +struct intel_dmc_info {
> + __be32 *dmc_payload;
> + uint32_t dmc_fw_size;
> + uint32_t mmio_count;
> + uint32_t mmioaddr[8];
> + uint32_t mmiodata[8];
> +};
> +
> struct drm_i915_private {
> struct drm_device *dev;
> struct kmem_cache *slab;
> @@ -1574,6 +1697,11 @@ struct drm_i915_private {
>
> struct i915_virtual_gpu vgpu;
>
> + struct intel_csr csr;
> +
> + /* Display CSR-related protection */
> + struct mutex csr_lock;
> +
> struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>
>
> @@ -1832,6 +1960,7 @@ struct drm_i915_private {
> * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
> * will be rejected. Instead look for a better place.
> */
> + struct intel_dmc_info dmc_info;
This should be part of struct intel_csr.
> };
>
> static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> @@ -2418,6 +2547,8 @@ struct drm_i915_cmd_table {
> #define HAS_RC6(dev) (INTEL_INFO(dev)->gen >= 6)
> #define HAS_RC6p(dev) (INTEL_INFO(dev)->gen == 6 || IS_IVYBRIDGE(dev))
>
> +#define HAS_CSR(dev) (IS_SKYLAKE(dev))
> +
> #define INTEL_PCH_DEVICE_ID_MASK 0xff00
> #define INTEL_PCH_IBX_DEVICE_ID_TYPE 0x3b00
> #define INTEL_PCH_CPT_DEVICE_ID_TYPE 0x1c00
> @@ -2508,6 +2639,7 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
> extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
> int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
> void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
> +void i915_firmware_load_error_print(const char *fw_path, int err);
>
> /* i915_irq.c */
> void i915_queue_hangcheck(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b522eb6..77faa2b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6776,6 +6776,24 @@ enum skl_disp_power_wells {
> #define GET_CFG_CR1_REG(id) (DPLL1_CFGCR1 + (id - SKL_DPLL1) * 8)
> #define GET_CFG_CR2_REG(id) (DPLL1_CFGCR2 + (id - SKL_DPLL1) * 8)
>
> +/*
> +* SKL CSR registers for DC5 and DC6
> +*/
> +#define CSR_PROGRAM_BASE 0x80000
> +#define CSR_HEADER_OFFSET 128
> +#define CSR_SSP_BASE_ADDR_GEN9 0x00002FC0
> +#define CSR_HTP_ADDR_SKL 0x00500034
> +#define CSR_SSP_BASE 0x8F074
> +#define CSR_HTP_SKL 0x8F004
> +#define CSR_LAST_WRITE 0x8F034
> +#define CSR_LAST_WRITE_VALUE 0xc003b400
> +/* MMIO address range for CSR program (0x80000 - 0x82FFF) */
> +#define CSR_MAX_FW_SIZE 0x2FFF
> +#define CSR_DEFAULT_FW_OFFSET 0xFFFFFFFF
> +#define CSR_MMIO_START_RANGE 0x80000
> +#define CSR_MMIO_END_RANGE 0x8FFFF
All of the above are used only intel_csr.c, so no need to export them.
> +#define CSR_MAX_MMIO_COUNT 8
I wouldn't define this, just use ARRAY_SIZE for the corresponding sanity
check.
> +
> /* Please see hsw_read_dcomp() and hsw_write_dcomp() before using this register,
> * since on HSW we can't write to it using I915_WRITE. */
> #define D_COMP_HSW (MCHBAR_MIRROR_BASE_SNB + 0x5F0C)
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> new file mode 100644
> index 0000000..f44f1cd
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_csr.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 "i915_reg.h"
> +
> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
> +
> +MODULE_FIRMWARE(I915_CSR_SKL);
> +
> +#define num_array_elements(a) (sizeof(a)/sizeof(a[0]))
You can use ARRAY_SIZE.
> +
> +struct stepping_info {
> + char stepping;
> + char substepping;
> + uint16_t reserved;
reserved is not used.
> +};
> +
> +struct stepping_info skl_stepping_info[] = {
> + {'A', '0', 0}, {'B', '0', 0}, {'C', '0', 0},
> + {'D', '0', 0}, {'E', '0', 0}, {'F', '0', 0},
> + {'G', '0', 0}, {'H', '0', 0}, {'I', '0', 0}
> +};
Should be static.
> +
> +char intel_get_stepping(struct drm_device *dev)
> +{
> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> + num_array_elements(skl_stepping_info)))
> + return skl_stepping_info[dev->pdev->revision].stepping;
> + else
> + return -ENODATA;
> +}
> +
> +char intel_get_substepping(struct drm_device *dev)
> +{
> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> + num_array_elements(skl_stepping_info)))
> + return skl_stepping_info[dev->pdev->revision].substepping;
> + else
> + return -ENODATA;
> +}
> +void intel_csr_load_program(struct drm_device *dev)
The above 3 functions should be static.
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + unsigned char *buf = (unsigned char *)dev_priv->dmc_info.dmc_payload;
> + uint32_t i, num_bytes;
> +
> + if (!IS_GEN9(dev)) {
> + DRM_ERROR("No CSR support available for this platform\n");
> + return;
> + }
> +
> + mutex_lock(&dev_priv->csr_lock);
> + /* fw_size is dwords, converting it to bytes and nearest 8 multiple. */
> + num_bytes = dev_priv->dmc_info.dmc_fw_size * 4;
> + for (i = 0; i < (num_bytes & ~7); i = i + 8) {
> + uint64_t tmp = *(uint64_t *)(buf + i);
> +
> + I915_WRITE64((CSR_PROGRAM_BASE + i), tmp);
> + }
> +
> + /* fw_size is in dwords, for odd size need to write last 4 bytes */
> + if (num_bytes & 7) {
> + uint32_t tmp = *(uint32_t *)(buf + (num_bytes & ~7));
> +
> + I915_WRITE((CSR_PROGRAM_BASE + i), tmp);
> + }
I wouldn't optimize at this point, just use a simple loop:
for (i = 0; i < dmc_fw_size; i++)
I915_WRITE(CSR_PROGRAM_BASE + i * 4, (u32 __force)dmc_payload + i);
If it needs to be optimized I'd use instead:
/* No forcewake needed for the CSR program MMIO range */
memcpy_toio(dev_priv->regs + CSR_PROGRAM_BASE, dmc_payload, dmc_fw_size * 4);
> +
> + for (i = 0; i < dev_priv->dmc_info.mmio_count; i++) {
> + I915_WRITE(dev_priv->dmc_info.mmioaddr[i],
> + dev_priv->dmc_info.mmiodata[i]);
> + }
> + mutex_unlock(&dev_priv->csr_lock);
> +}
> +
> +static void finish_csr_load(const struct firmware *fw, void *context)
> +{
> + struct drm_i915_private *dev_priv = context;
> + struct drm_device *dev = dev_priv->dev;
> + struct intel_css_header *css_header;
> + struct intel_package_header *package_header;
> + struct intel_dmc_header *dmc_header;
> + struct intel_csr *csr = &dev_priv->csr;
> + char stepping = intel_get_stepping(dev);
> + char substepping = intel_get_substepping(dev);
> + uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> + uint32_t i, j;
> + __be32 *dmc_payload;
> +
> + if (!fw) {
> + i915_firmware_load_error_print(csr->fw_path, 0);
> + goto out;
> + }
> +
> + if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
> + DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> + goto out;
> + }
> +
> + /* save csr f/w as it will be needed during resume */
> + csr->csr_buf = kmemdup(fw->data, fw->size, GFP_KERNEL);
Since you will copy everything relevant out of csr_buf and free it at
the end of the function, you could just use fw->data directly (and move
release_firmware later accordingly).
> +
> + csr->csr_size = fw->size;
This is unused.
> +
> + release_firmware(fw);
> +
> + /* Extract CSS Header information*/
> + css_header = (struct intel_css_header *)csr->csr_buf;
> + if (sizeof(struct intel_css_header) !=
> + (css_header->header_len * 4)) {
> + DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
> + (css_header->header_len * 4));
> + goto out;
> + }
> + readcount += sizeof(struct intel_css_header);
> +
> + /* Extract Package Header information*/
> + package_header = (struct intel_package_header *)
> + &csr->csr_buf[readcount];
> + if (sizeof(struct intel_package_header) !=
> + (package_header->header_len * 4)) {
> + DRM_ERROR("Firmware has wrong package header length %u bytes\n",
> + (package_header->header_len * 4));
> + goto out;
> + }
> + readcount += sizeof(struct intel_package_header);
> +
> + /* Search for dmc_offset to find firware binary. */
> + for (i = 0; i < package_header->num_entries; i++) {
> + if (package_header->fw_info[i].substepping == '*' &&
> + stepping == package_header->fw_info[i].stepping) {
> + dmc_offset = package_header->fw_info[i].offset;
> + break;
> + } else if (stepping == package_header->fw_info[i].stepping &&
> + substepping == package_header->fw_info[i].substepping) {
> + dmc_offset = package_header->fw_info[i].offset;
> + break;
> + } else if (package_header->fw_info[i].stepping == '*' &&
> + package_header->fw_info[i].substepping == '*')
> + dmc_offset = package_header->fw_info[i].offset;
> + }
> + if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
> + DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
> + goto out;
> + }
> + readcount += dmc_offset;
> +
> + /* Extract dmc_header information. */
> + dmc_header = (struct intel_dmc_header *)&csr->csr_buf[readcount];
> + if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
> + DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
> + (dmc_header->header_len));
> + goto out;
> + }
> + readcount += sizeof(struct intel_dmc_header);
> +
> + /* Cache the dmc header info. */
> + if (dmc_header->mmio_count > CSR_MAX_MMIO_COUNT) {
> + DRM_ERROR("Firmware has wrong mmio count %u\n",
> + dmc_header->mmio_count);
> + goto out;
> + }
> + dev_priv->dmc_info.mmio_count = dmc_header->mmio_count;
> + for (i = 0; i < dmc_header->mmio_count; i++) {
> + if (dmc_header->mmioaddr[i] >= CSR_MMIO_START_RANGE &&
> + dmc_header->mmioaddr[i] <= CSR_MMIO_END_RANGE) {
> + DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
> + dmc_header->mmioaddr[i]);
> + goto out;
> + }
> + dev_priv->dmc_info.mmioaddr[i] = dmc_header->mmioaddr[i];
> + dev_priv->dmc_info.mmiodata[i] = dmc_header->mmiodata[i];
> + }
> +
> + /* fw_size is in dwords, so multiplied by 4 to convert into bytes. */
> + nbytes = dmc_header->fw_size * 4;
> + if (nbytes > CSR_MAX_FW_SIZE) {
> + DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
> + goto out;
> + }
> + dev_priv->dmc_info.dmc_payload = kmalloc(nbytes, GFP_KERNEL);
> + if (!dev_priv->dmc_info.dmc_payload) {
> + DRM_ERROR("Memory allocation failed for dmc payload\n");
> + goto out;
> + }
> +
> + dmc_payload = dev_priv->dmc_info.dmc_payload;
> + for (i = 0, j = 0; i < nbytes; i = i + 4, j++) {
I'd just use a single iterator here.
> + uint32_t *tmp = (u32 *)&csr->csr_buf[readcount + i];
> + /*
> + * The firmware payload is an array of 32 bit words stored in
> + * little-endian format in the firmware image and programmed
> + * as 32 bit big-endian format to memory.
> + */
> + dmc_payload[j] = cpu_to_be32(*tmp);
> + }
> +
> + /* load csr program during system boot, as needed for DC states */
> + intel_csr_load_program(dev);
> +out:
> + kfree(csr->csr_buf);
> + csr->csr_buf = NULL;
> +}
> +
> +int intel_csr_ucode_init(struct drm_device *dev)
This could be void, since we won't use the returned error.
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_csr *csr = &dev_priv->csr;
> + int ret;
> +
> + if (!HAS_CSR(dev))
> + return 0;
> +
> + if (IS_SKYLAKE(dev))
> + csr->fw_path = I915_CSR_SKL;
> + else {
> + DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> + return 0;
> + }
> +
> + /* CSR supported for platform, load firmware */
> + ret = request_firmware_nowait(THIS_MODULE, true, csr->fw_path,
> + &dev_priv->dev->pdev->dev,
> + GFP_KERNEL, dev_priv,
> + finish_csr_load);
> + if (ret)
> + i915_firmware_load_error_print(csr->fw_path, ret);
> +
> + return ret;
> +}
> +
> +void intel_csr_ucode_fini(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + if (!HAS_CSR(dev))
> + return;
> +
> + kfree(dev_priv->dmc_info.dmc_payload);
> + memset(&dev_priv->dmc_info, 0, sizeof(struct intel_dmc_info));
No need for the memset, dmc_info won't be used afterwards.
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fca7b9f..cd20b6a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1059,6 +1059,11 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
> unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
> struct drm_i915_gem_object *obj);
>
> +/* intel_csr.c */
> +int intel_csr_ucode_init(struct drm_device *dev);
> +void intel_csr_load_program(struct drm_device *dev);
> +void intel_csr_ucode_fini(struct drm_device *dev);
> +
> /* intel_dp.c */
> void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
> bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
