Message ID | 1446069547-24760-5-git-send-email-imre.deak@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 10/29/2015 3:28 AM, Imre Deak wrote: > From: Daniel Vetter <daniel.vetter@intel.com> > > 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. Yes as DC5 and DC6 display low power state is coupled with dmc firmware so it is a safe to have a check before enable/disable dc5/dc6. > > 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. > > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Sunil Kamath <sunil.kamath@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Animesh Manna <animesh.manna@intel.com> > [imre: added code and note about checking if the firmware loaded ok, > before enabling/disabling it] Reviewed-by: Animesh Manna <animesh.manna@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/i915_gpu_error.c | 2 +- > 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 ++---------- > 7 files changed, 6 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 5d68942..621914c 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -897,7 +897,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 121c539..7efc798 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1050,18 +1050,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; > @@ -1110,7 +1103,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 0bee438..0aeac06 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) \ > @@ -1724,9 +1717,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/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c > index aaf4a1f..25dc4f1 100644 > --- a/drivers/gpu/drm/i915/i915_gpu_error.c > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c > @@ -370,7 +370,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, > if (HAS_CSR(dev)) { > struct intel_csr *csr = &dev_priv->csr; > > - err_printf(m, "DMC load state: %d\n", csr->state); > + err_printf(m, "DMC loaded: %s\n", yesno(csr->dmc_payload)); > err_printf(m, "DMC fw version: %d.%d\n", > CSR_VERSION_MAJOR(csr->version), > CSR_VERSION_MINOR(csr->version)); > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c > index cd6fb58d..eddad62 100644 > --- a/drivers/gpu/drm/i915/intel_csr.c > +++ b/drivers/gpu/drm/i915/intel_csr.c > @@ -203,40 +203,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_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. > * > @@ -264,7 +230,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]); > @@ -273,9 +238,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) > @@ -415,8 +377,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); > } > > @@ -445,7 +405,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; > } > > @@ -462,10 +421,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); > - } > } > > /** > @@ -482,6 +439,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 3d1c744..2e48b3f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1206,9 +1206,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 92746e1..30b5801 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -664,8 +664,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); > @@ -674,20 +673,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); > - } > } > } >
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 5d68942..621914c 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -897,7 +897,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 121c539..7efc798 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1050,18 +1050,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; @@ -1110,7 +1103,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 0bee438..0aeac06 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) \ @@ -1724,9 +1717,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/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c index aaf4a1f..25dc4f1 100644 --- a/drivers/gpu/drm/i915/i915_gpu_error.c +++ b/drivers/gpu/drm/i915/i915_gpu_error.c @@ -370,7 +370,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m, if (HAS_CSR(dev)) { struct intel_csr *csr = &dev_priv->csr; - err_printf(m, "DMC load state: %d\n", csr->state); + err_printf(m, "DMC loaded: %s\n", yesno(csr->dmc_payload)); err_printf(m, "DMC fw version: %d.%d\n", CSR_VERSION_MAJOR(csr->version), CSR_VERSION_MINOR(csr->version)); diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index cd6fb58d..eddad62 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -203,40 +203,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_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. * @@ -264,7 +230,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]); @@ -273,9 +238,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) @@ -415,8 +377,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); } @@ -445,7 +405,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; } @@ -462,10 +421,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); - } } /** @@ -482,6 +439,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 3d1c744..2e48b3f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1206,9 +1206,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 92746e1..30b5801 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -664,8 +664,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); @@ -674,20 +673,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); - } } }