Re: [PATCH v3 1/8] drm/i915/skl: Add support to load SKL CSR firmware

Imre Deak
2015-04-13T12:37:51Z

On ma, 2015-04-13 at 18:37 +0530, Animesh Manna wrote:
>
> On 04/13/2015 04:33 PM, Imre Deak wrote:
> > On ma, 2015-04-13 at 15:54 +0530, Animesh Manna wrote:
> >> From: "A.Sunil Kamath" <sunil.kamath@intel.com>
> >>
> >> 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)
> >>
> >> v15:
> >> 1) Changed the firmware name as dmc_gen9.bin, everytime for a new firmware version a symbolic link
> >> with same name will help not to build kernel again.
> >> 2) Changes done as per review comments from Imre.
> >> - Error check removed for intel_csr_ucode_init.
> >> - Moved csr-specific data structure to intel_csr.h and optimization done on structure definition.
> >> - fw->data used directly for parsing the header info & memory allocation
> >> only done separately for payload. (Animesh)
> >>
> >> v16:
> >> No need for out_regs label in i915_driver_load(), so removed it. (Animesh)
> >>
> >> Issue: VIZ-2569
> >> Signed-off-by: A.Sunil Kamath <sunil.kamath@intel.com>
> >> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/Makefile | 3 +-
> >> drivers/gpu/drm/i915/i915_dma.c | 11 +-
> >> drivers/gpu/drm/i915/i915_drv.c | 20 ++++
> >> drivers/gpu/drm/i915/i915_drv.h | 17 +++
> >> drivers/gpu/drm/i915/intel_csr.c | 234 +++++++++++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/i915/intel_csr.h | 164 +++++++++++++++++++++++++++
> >> drivers/gpu/drm/i915/intel_drv.h | 5 +
> >> 7 files changed, 451 insertions(+), 3 deletions(-)
> >> create mode 100644 drivers/gpu/drm/i915/intel_csr.c
> >> create mode 100644 drivers/gpu/drm/i915/intel_csr.h
> >>
> >> 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 e44116f..a238889 100644
> >> --- a/drivers/gpu/drm/i915/i915_dma.c
> >> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >> @@ -816,6 +816,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);
> >>
> >> @@ -861,9 +862,12 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>
> >> intel_uncore_init(dev);
> >>
> >> + /* Load CSR Firmware for SKL */
> >> + intel_csr_ucode_init(dev);
> >> +
> >> ret = i915_gem_gtt_init(dev);
> >> if (ret)
> >> - goto out_regs;
> >> + goto out_freecsr;
> >>
> >> /* WARNING: Apparently we must kick fbdev drivers before vgacon,
> >> * otherwise the vga fbdev driver falls over. */
> >> @@ -1033,7 +1037,8 @@ out_mtrrfree:
> >> io_mapping_free(dev_priv->gtt.mappable);
> >> out_gtt:
> >> i915_global_gtt_cleanup(dev);
> >> -out_regs:
> >> +out_freecsr:
> >> + intel_csr_ucode_fini(dev);
> >> intel_uncore_fini(dev);
> >> pci_iounmap(dev->pdev, dev_priv->regs);
> >> put_bridge:
> >> @@ -1113,6 +1118,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 c3fdbb0..acd0e2b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -556,6 +556,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 47be4a5..90e47a9 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -667,6 +667,15 @@ 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 {
> >> + const char *fw_path;
> >> + __be32 *dmc_payload;
> >> + uint32_t dmc_fw_size;
> >> + uint32_t mmio_count;
> >> + uint32_t mmioaddr[8];
> >> + uint32_t mmiodata[8];
> >> +};
> >> +
> >> #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> >> func(is_mobile) sep \
> >> func(is_i85x) sep \
> >> @@ -1573,6 +1582,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_PINS];
> >>
> >> /** gmbus_mutex protects against concurrent usage of the single hw gmbus
> >> @@ -2425,6 +2439,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
> >> @@ -2515,6 +2531,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/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> >> new file mode 100644
> >> index 0000000..ab9b16b
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/intel_csr.c
> >> @@ -0,0 +1,234 @@
> >> +/*
> >> + * 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"
> >> +#include "intel_csr.h"
> >> +
> >> +static struct stepping_info skl_stepping_info[] = {
> > Nitpick: this should be const.
> >
> >> + {'A', '0'}, {'B', '0'}, {'C', '0'},
> >> + {'D', '0'}, {'E', '0'}, {'F', '0'},
> >> + {'G', '0'}, {'H', '0'}, {'I', '0'}
> >> +};
> >> +
> >> +static char intel_get_stepping(struct drm_device *dev)
> >> +{
> >> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> >> + ARRAY_SIZE(skl_stepping_info)))
> >> + return skl_stepping_info[dev->pdev->revision].stepping;
> > Nitpick: at this point it's just
> > if (dev->pdev->revision < 9)
> > return 'A' + dev->pdev->revision;
> >
> > w/o the need for skl_stepping_info.
>
> If we have different firmwares for different substepping for example
> A0,A1,A3 then stepping will be same but substepping will be different
> and I assume revision_id also will be different. And if we want to put
> some logic like ('A' + dev->pdev->revision) then we can do it but
> should be based on bspec, Why I am telling because if later we get some
> table and not able to derive a common logic then again may need this
> kind of lookup table.
>
> And using lookuo table comes with advantage
> - to extend support for future platform only have to add new structure or
> change existing structure to incorporate any future changes in stepping
> info which got from bspec. - time complexity will be less.
>
> Yes agree space might be an issue. Suggest me trade off between time and
> space.

It would be just simpler in the above way, it's not a space issue. I
think once we have platforms with substeppings other than 0, we will
anyway have to change the above struct stepping_info and the logic to
look up the proper entry in it. But the above works too, so I'm ok if
you keep it as-is for now. Just make the struct const please.

> >> + else
> >> + return -ENODATA;
> >> +}
> >> +
> >> +static char intel_get_substepping(struct drm_device *dev)
> >> +{
> >> + if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> >> + ARRAY_SIZE(skl_stepping_info)))
> >> + return skl_stepping_info[dev->pdev->revision].substepping;
> > and this one is simply
> > return 0;
>
> For now all values are 0 for skl, but implementation done thinking of
> supporting future changes. We may have multiple firmware for different
> substepping.

Ok, let's keep it as-is.

> >> + else
> >> + return -ENODATA;
> >> +}
> > Missing newline.
> >
> >> +void intel_csr_load_program(struct drm_device *dev)
> >> +{
> >> + struct drm_i915_private *dev_priv = dev->dev_private;
> >> + __be32 *payload = dev_priv->csr.dmc_payload;
> >> + uint32_t i, fw_size;
> >> +
> >> + 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. */
> > The above comment is stale now.
>
> Agree.
>
> >
> >> + fw_size = dev_priv->csr.dmc_fw_size;
> >> + for (i = 0; i < fw_size; i++)
> >> + I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> >> + (u32 __force)payload[i]);
> >> +
> >> + for (i = 0; i < dev_priv->csr.mmio_count; i++) {
> >> + I915_WRITE(dev_priv->csr.mmioaddr[i],
> >> + dev_priv->csr.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;
> >> + __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;
> >> + }
> >> +
> >> + /* Extract CSS Header information*/
> >> + css_header = (struct intel_css_header *)fw->data;
> >> + 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 *)
> >> + &fw->data[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 *)&fw->data[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 > ARRAY_SIZE(csr->mmioaddr)) {
> >> + DRM_ERROR("Firmware has wrong mmio count %u\n",
> >> + dmc_header->mmio_count);
> >> + goto out;
> >> + }
> >> + csr->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;
> >> + }
> >> + csr->mmioaddr[i] = dmc_header->mmioaddr[i];
> >> + csr->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;
> >> + }
> >> + csr->dmc_fw_size = dmc_header->fw_size;
> >> +
> >> + csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
> >> + if (!csr->dmc_payload) {
> >> + DRM_ERROR("Memory allocation failed for dmc payload\n");
> >> + goto out;
> >> + }
> >> +
> >> + dmc_payload = csr->dmc_payload;
> >> + for (i = 0; i < dmc_header->fw_size; i++) {
> >> + uint32_t *tmp = (u32 *)&fw->data[readcount + i * 4];
> >> + /*
> >> + * 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[i] = cpu_to_be32(*tmp);
> >> + }
> >> +
> >> + /* load csr program during system boot, as needed for DC states */
> >> + intel_csr_load_program(dev);
> >> +out:
> >> + release_firmware(fw);
> >> +}
> >> +
> >> +void intel_csr_ucode_init(struct drm_device *dev)
> >> +{
> >> + struct drm_i915_private *dev_priv = dev->dev_private;
> >> + struct intel_csr *csr = &dev_priv->csr;
> >> + int ret;
> >> +
> >> + if (!HAS_CSR(dev))
> >> + return;
> >> +
> >> + if (IS_SKYLAKE(dev))
> >> + csr->fw_path = I915_CSR_SKL;
> >> + else {
> >> + DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> >> + return;
> >> + }
> >> +
> >> + /* 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);
> >> +
> >> +}
> >> +
> >> +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->csr.dmc_payload);
> >> +}
> >> diff --git a/drivers/gpu/drm/i915/intel_csr.h b/drivers/gpu/drm/i915/intel_csr.h
> >> new file mode 100644
> >> index 0000000..c2a5a53
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/i915/intel_csr.h
> >> @@ -0,0 +1,164 @@
> >> +/*
> >> + * 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_CSR_H_
> >> +#define _INTEL_CSR_H_
> >> +
> >> +#define I915_CSR_SKL "i915/dmc_gen9.bin"
> >> +
> >> +MODULE_FIRMWARE(I915_CSR_SKL);
> >> +
> >> +/*
> >> +* 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
> >> +
> >> +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;
> >> +
> >> +struct stepping_info {
> >> + char stepping;
> >> + char substepping;
> >> +} __packed;
> > stepping_info doesn't need to be packed.
>
> To avoid structure padding used __packed which will save some memory.

No, the reason for __packed above is simply correctness for binary
interfaces like what we have with the DMC firmware. For anything else we
can trust the compiler to do the right thing wrt. to alignment/padding.

> > What I meant is to add all these to intel_csr.c. I don't think
> > there is any reason for a new header file unless you need to share
> > somethinh among multiple .c modules.
>
> intel_runtime_pm.c is using few macro which is defined in this header
> file in later patches. Then we can move all macro definition to
> i915_drv.h or else can put few macro in intel_csr.c and few in
> i915_drv.h. Give me your suggestion.

Yes, I missed assert_csr_loaded(). As this is really something internal
to the DMC firmware, I would just move assert_csr_loaded() to
intel_csr.c and export that from intel_drv.h.

--Imre