[Intel-gfx] [PATCH] drm/i915/skl: replace csr_mutex by completion in csr firmware loading
Daniel Vetter
daniel at ffwll.ch
Mon Jun 29 01:39:26 PDT 2015
Quick summary of the tasks around dmc loader that we talked about in
our mtg just now.
- replace runtime_pm_get/put with display_power_get/put to make sure
we prevent entering dc5/6 code while the firmware loading is in
progress. Then remove the wait code in the skl power well code and
replace it with a simple status check (maybe add a new dmc_present
boolean) to decide which path to take. This way we only ever run that
code when firmware loading is guaranteed to have either completed or
failed, which means no bugs with tricky fallback/upgrade code at
runtime.
- replace request_firmware_nowait with the synchronous
request_firmware, but run from our own async work item. Use flush_work
instead of csr_load_status_set in suspend and driver unload code to
explicitly synchronize with that async work. Note that this replacment
also has the upside that request_firmware properly supports the
(optional, modern linux distros disable it) upstream userspace
firmware loader fallback.
The goal is to replace the hand-rolled synchronization with
established primitives (flush_work and our power well get/put code)
and completely remove the csr_load_status_set call and the csr_lock
mutex.
Thanks, Daniel
On Thu, May 21, 2015 at 12:19 PM, Animesh Manna <animesh.manna at intel.com> wrote:
> Before enabling dc5/dc6, used wait for completion instead of busy waiting.
>
> v1:
> - Based on review comment from Daniel replaced mutex and related
> implementation with completion. In current patch not used power
> domain lock, don't want to block runtime power management if dmc
> firmware failed to load. Will analyzing further and possibly send
> as a incremental patch.
> - Based on review comment from Damien, warning for firmware loading
> failure is removed.
>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 1 -
> drivers/gpu/drm/i915/i915_drv.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 12 ++-----
> drivers/gpu/drm/i915/intel_csr.c | 63 +++------------------------------
> drivers/gpu/drm/i915/intel_drv.h | 3 --
> drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++------
> 6 files changed, 12 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 78e6ae8..1f5c86c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -816,7 +816,6 @@ 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.csr_lock);
>
> intel_pm_setup(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6bb6c47..c102268 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1034,7 +1034,7 @@ static int skl_suspend_complete(struct drm_i915_private *dev_priv)
> * This is to ensure that CSR isn't identified as loaded before
> * CSR-loading program is called during runtime-resume.
> */
> - intel_csr_load_status_set(dev_priv, FW_UNINITIALIZED);
> + reinit_completion(&dev_priv->csr.is_loaded);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 43011d7..423afa9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -50,6 +50,7 @@
> #include <linux/intel-iommu.h>
> #include <linux/kref.h>
> #include <linux/pm_qos.h>
> +#include <linux/completion.h>
>
> /* General customization:
> */
> @@ -669,23 +670,14 @@ struct intel_uncore {
> #define for_each_fw_domain(domain__, dev_priv__, i__) \
> for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
>
> -enum csr_state {
> - FW_UNINITIALIZED = 0,
> - FW_LOADED,
> - FW_FAILED
> -};
> -
> struct intel_csr {
> - /* CSR protection, used to protect firmware loading status: csr_state */
> - struct mutex csr_lock;
> -
> + struct completion is_loaded;
> const char *fw_path;
> __be32 *dmc_payload;
> uint32_t dmc_fw_size;
> uint32_t mmio_count;
> uint32_t mmioaddr[8];
> uint32_t mmiodata[8];
> - enum csr_state state;
> };
>
> #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index d1da2db..fec2bc5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -32,13 +32,6 @@
> * onwards to drive newly added DMC (Display microcontroller) in display
> * engine to save and restore the state of display engine when it enter into
> * low-power state and comes back to normal.
> - *
> - * Firmware loading status will be one of the below states: FW_UNINITIALIZED,
> - * FW_LOADED, FW_FAILED.
> - *
> - * Once the firmware is written into the registers status will be moved from
> - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will
> - * be moved to FW_FAILED.
> */
>
> #define I915_CSR_SKL "i915/skl_dmc_ver4.bin"
> @@ -200,40 +193,6 @@ static char intel_get_substepping(struct drm_device *dev)
> }
>
> /**
> - * intel_csr_load_status_get() - to get firmware loading status.
> - * @dev_priv: i915 device.
> - *
> - * This function helps to get the firmware loading status.
> - *
> - * Return: Firmware loading status.
> - */
> -enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv)
> -{
> - enum csr_state state;
> -
> - mutex_lock(&dev_priv->csr.csr_lock);
> - state = dev_priv->csr.state;
> - mutex_unlock(&dev_priv->csr.csr_lock);
> -
> - return state;
> -}
> -
> -/**
> - * intel_csr_load_status_set() - help to set firmware loading status.
> - * @dev_priv: i915 device.
> - * @state: enumeration of firmware loading status.
> - *
> - * Set the firmware loading status.
> - */
> -void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
> - enum csr_state state)
> -{
> - mutex_lock(&dev_priv->csr.csr_lock);
> - dev_priv->csr.state = state;
> - mutex_unlock(&dev_priv->csr.csr_lock);
> -}
> -
> -/**
> * intel_csr_load_program() - write the firmware from memory to register.
> * @dev: drm device.
> *
> @@ -252,7 +211,6 @@ void intel_csr_load_program(struct drm_device *dev)
> return;
> }
>
> - mutex_lock(&dev_priv->csr.csr_lock);
> fw_size = dev_priv->csr.dmc_fw_size;
> for (i = 0; i < fw_size; i++)
> I915_WRITE(CSR_PROGRAM_BASE + i * 4,
> @@ -262,9 +220,7 @@ void intel_csr_load_program(struct drm_device *dev)
> I915_WRITE(dev_priv->csr.mmioaddr[i],
> dev_priv->csr.mmiodata[i]);
> }
> -
> - dev_priv->csr.state = FW_LOADED;
> - mutex_unlock(&dev_priv->csr.csr_lock);
> + complete(&dev_priv->csr.is_loaded);
> }
>
> static void finish_csr_load(const struct firmware *fw, void *context)
> @@ -280,7 +236,6 @@ static void finish_csr_load(const struct firmware *fw, void *context)
> uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
> uint32_t i;
> __be32 *dmc_payload;
> - bool fw_loaded = false;
>
> if (!fw) {
> i915_firmware_load_error_print(csr->fw_path, 0);
> @@ -387,14 +342,9 @@ static void finish_csr_load(const struct firmware *fw, void *context)
>
> /* load csr program during system boot, as needed for DC states */
> intel_csr_load_program(dev);
> - fw_loaded = true;
>
> out:
> - if (fw_loaded)
> - intel_runtime_pm_put(dev_priv);
> - else
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> -
> + intel_runtime_pm_put(dev_priv);
> release_firmware(fw);
> }
>
> @@ -418,7 +368,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
> csr->fw_path = I915_CSR_SKL;
> else {
> DRM_ERROR("Unexpected: no known CSR firmware for platform\n");
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> return;
> }
>
> @@ -428,15 +377,15 @@ void intel_csr_ucode_init(struct drm_device *dev)
> */
> intel_runtime_pm_get(dev_priv);
>
> + init_completion(&csr->is_loaded);
> +
> /* 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) {
> + if (ret)
> i915_firmware_load_error_print(csr->fw_path, ret);
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> - }
> }
>
> /**
> @@ -453,13 +402,11 @@ void intel_csr_ucode_fini(struct drm_device *dev)
> if (!HAS_CSR(dev))
> return;
>
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> kfree(dev_priv->csr.dmc_payload);
> }
>
> void assert_csr_loaded(struct drm_i915_private *dev_priv)
> {
> - WARN((intel_csr_load_status_get(dev_priv) != FW_LOADED), "CSR is not loaded.\n");
> WARN(!I915_READ(CSR_PROGRAM_BASE),
> "CSR program storage start is NULL\n");
> WARN(!I915_READ(CSR_SSP_BASE), "CSR SSP Base Not fine\n");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ea20edb..e022c98 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1155,9 +1155,6 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
>
> /* intel_csr.c */
> void intel_csr_ucode_init(struct drm_device *dev);
> -enum csr_state intel_csr_load_status_get(struct drm_i915_private *dev_priv);
> -void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
> - enum csr_state state);
> void intel_csr_load_program(struct drm_device *dev);
> void intel_csr_ucode_fini(struct drm_device *dev);
> void assert_csr_loaded(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b393db7..5a830d3 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -525,7 +525,6 @@ static void assert_can_disable_dc6(struct drm_i915_private *dev_priv)
> if (dev_priv->power_domains.initializing)
> return;
>
> - assert_csr_loaded(dev_priv);
> WARN(!(I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC6),
> "DC6 already programmed to be disabled.\n");
> }
> @@ -642,21 +641,14 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>
> if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> power_well->data == SKL_DISP_PW_2) {
> - enum csr_state state;
> - /* TODO: wait for a completion event or
> - * similar here instead of busy
> - * waiting using wait_for function.
> - */
> - wait_for((state = intel_csr_load_status_get(dev_priv)) !=
> - FW_UNINITIALIZED, 1000);
> - if (state != FW_LOADED)
> - DRM_ERROR("CSR firmware not ready (%d)\n",
> - state);
> - else
> + if (wait_for_completion_timeout(
> + &dev_priv->csr.is_loaded, 100)) {
> if (SKL_ENABLE_DC6(dev))
> skl_enable_dc6(dev_priv);
> else
> gen9_enable_dc5(dev_priv);
> + } else
> + DRM_ERROR("CSR firmware not ready\n");
> }
> }
> }
> --
> 2.0.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the Intel-gfx
mailing list