From 414b7999b8bef3d79c563d8305b1df18da701634 Mon Sep 17 00:00:00 2001 From: Daniel Vetter Date: Thu, 12 Nov 2015 17:10:37 +0200 Subject: [PATCH] drm/i915/gen9: Remove csr.state, csr_lock and related code. This removes two anti-patterns: - Locking shouldn't be used to synchronize with async work (of any form, whether callbacks, workers or other threads). This is what the mutex_lock/unlock seems to have been for in intel_csr_load_program. Instead ordering should be ensured with the generic wait_for_completion()/complete(). Or more specific functions provided by the core kernel like e.g. flush_work()/cancel_work_sync() in the case of synchronizing with a work item. - Don't invent own completion like the following code did with the (already removed) wait_for(csr_load_status_get()) pattern - it's really hard to get these right when you want them to be _really_ correct (and be fast) in all cases. Furthermore it's easier to read code using the well-known primitives than new ones using non-standard names. Before enabling/disabling DC6 check if the firmware is loaded successfully. This is guaranteed during runtime s/r, since otherwise we don't enable RPM, but not during system s/r. Note that it's still unclear whether we need to enable/disable DC6 during system s/r, until that's clarified, keep the current behavior and enable/disable DC6. Also after this patch there is a race during system s/r where the firmware may not be loaded yet, that's addressed in an upcoming patch. v2-v3: - unchanged v4: - rebased on latest drm-intel-nightly Cc: Damien Lespiau Cc: Imre Deak Cc: Sunil Kamath Signed-off-by: Daniel Vetter Signed-off-by: Animesh Manna [imre: added code and note about checking if the firmware loaded ok, before enabling/disabling it] Reviewed-by: Animesh Manna Signed-off-by: Imre Deak Tested-by: Daniel Stone # SKL Signed-off-by: Jani Nikula Link: http://patchwork.freedesktop.org/patch/msgid/1447341037-2623-1-git-send-email-imre.deak@intel.com --- drivers/gpu/drm/i915/i915_dma.c | 1 - drivers/gpu/drm/i915/i915_drv.c | 11 ++---- drivers/gpu/drm/i915/i915_drv.h | 10 ------ drivers/gpu/drm/i915/intel_csr.c | 46 +------------------------ drivers/gpu/drm/i915/intel_drv.h | 3 -- drivers/gpu/drm/i915/intel_runtime_pm.c | 17 ++------- 6 files changed, 5 insertions(+), 83 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index f46d03484a92..76a53e87d337 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -926,7 +926,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) spin_lock_init(&dev_priv->mmio_flip_lock); mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); - mutex_init(&dev_priv->csr_lock); mutex_init(&dev_priv->av_mutex); intel_pm_setup(dev); diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9f552094b41b..aa34fcb0bdc2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1087,18 +1087,11 @@ static int i915_pm_resume(struct device *dev) static int skl_suspend_complete(struct drm_i915_private *dev_priv) { - enum csr_state state; /* Enabling DC6 is not a hard requirement to enter runtime D3 */ skl_uninit_cdclk(dev_priv); - /* 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) + if (dev_priv->csr.dmc_payload) skl_enable_dc6(dev_priv); return 0; @@ -1147,7 +1140,7 @@ static int skl_resume_prepare(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev; - if (intel_csr_load_status_get(dev_priv) == FW_LOADED) + if (dev_priv->csr.dmc_payload) skl_disable_dc6(dev_priv); skl_init_cdclk(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c5f36b4f4f16..5c732f87b9ce 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -738,12 +738,6 @@ struct intel_uncore { #define CSR_VERSION_MAJOR(version) ((version) >> 16) #define CSR_VERSION_MINOR(version) ((version) & 0xffff) -enum csr_state { - FW_UNINITIALIZED = 0, - FW_LOADED, - FW_FAILED -}; - struct intel_csr { const char *fw_path; uint32_t *dmc_payload; @@ -752,7 +746,6 @@ struct intel_csr { uint32_t mmio_count; uint32_t mmioaddr[8]; uint32_t mmiodata[8]; - enum csr_state state; }; #define DEV_INFO_FOR_EACH_FLAG(func, sep) \ @@ -1709,9 +1702,6 @@ struct drm_i915_private { 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 diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index b29dd23a8b9c..11efa131d346 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -198,40 +198,6 @@ static const struct stepping_info *intel_get_stepping_info(struct drm_device *de return NULL; } -/** - * 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_lock); - state = dev_priv->csr.state; - mutex_unlock(&dev_priv->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_lock); - dev_priv->csr.state = state; - mutex_unlock(&dev_priv->csr_lock); -} - /** * intel_csr_load_program() - write the firmware from memory to register. * @dev: drm device. @@ -260,7 +226,6 @@ void intel_csr_load_program(struct drm_device *dev) if (I915_READ(CSR_PROGRAM(0))) return; - mutex_lock(&dev_priv->csr_lock); fw_size = dev_priv->csr.dmc_fw_size; for (i = 0; i < fw_size; i++) I915_WRITE(CSR_PROGRAM(i), payload[i]); @@ -269,9 +234,6 @@ 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_lock); } static void finish_csr_load(const struct firmware *fw, void *context) @@ -412,8 +374,6 @@ out: CSR_VERSION_MAJOR(csr->version), CSR_VERSION_MINOR(csr->version)); } else { - intel_csr_load_status_set(dev_priv, FW_FAILED); - i915_firmware_load_error_print(csr->fw_path, 0); } @@ -442,7 +402,6 @@ void intel_csr_ucode_init(struct drm_device *dev) csr->fw_path = I915_CSR_BXT; else { DRM_ERROR("Unexpected: no known CSR firmware for platform\n"); - intel_csr_load_status_set(dev_priv, FW_FAILED); return; } @@ -459,10 +418,8 @@ void intel_csr_ucode_init(struct drm_device *dev) &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); - } } /** @@ -479,6 +436,5 @@ 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); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 57c419d3a593..5f42511599fe 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1221,9 +1221,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); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 81319fd1023c..35a344d799eb 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -663,8 +663,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, } else { if (enable_requested) { if (IS_SKYLAKE(dev) && - (power_well->data == SKL_DISP_PW_1) && - (intel_csr_load_status_get(dev_priv) == FW_LOADED)) + (power_well->data == SKL_DISP_PW_1)) DRM_DEBUG_KMS("Not Disabling PW1, dmc will handle\n"); else { I915_WRITE(HSW_PWR_WELL_DRIVER, tmp & ~req_mask); @@ -673,20 +672,8 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv, } if (GEN9_ENABLE_DC5(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_DEBUG("CSR firmware not ready (%d)\n", - state); - else + power_well->data == SKL_DISP_PW_2) gen9_enable_dc5(dev_priv); - } } } -- 2.20.1