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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 29, 2015 at 10:59:20PM +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.
> - Changed the firmware name as skl_dmc_ver1.bin, followed naming convention <platform>_dmc_<api-version>.bin (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 | 367 +++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   5 +
>  6 files changed, 420 insertions(+), 3 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 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;

Another small one: If we have a substruct we put the lock withing that
substruct, e.g. dev_priv->psr.lock. Can you pls do another patch on top to
move that?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx