| Message ID | 1432203592-11503-1-git-send-email-animesh.manna@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna 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@intel.com> Sorry if this cross with my comments, but upon further digging into this code we don't even need a completion at all. As long as we prevent the power well from getting disabled by holding a reference for it before we launch the firmware loader it'll all be fine. And the async work can then just drop that reference when everything is set up. Yes this means runtime D3 requires the firmware to be loaded, but that's imo not a problem. Also doing it this way is more in-line with how we do all the other async setup. But there's another serious issue with this design here, see below. > --- > 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)) { The problem is with this design that this wait here isn't allowed to ever time out, since if it does the book-keeping gets out of sync. And for a bunch of reasons (delays, deadlocks) we can't have that wait here really at all, much less one that never times out. Chandra has already hit this on a system where the firmware just isn't available. Thanks, Daniel > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 5/21/2015 5:41 PM, Daniel Vetter wrote: > On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna 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@intel.com> > Sorry if this cross with my comments, but upon further digging into this > code we don't even need a completion at all. As long as we prevent the > power well from getting disabled by holding a reference for it before we > launch the firmware loader it'll all be fine. And the async work can then > just drop that reference when everything is set up. > > Yes this means runtime D3 requires the firmware to be loaded, but that's > imo not a problem. > > Also doing it this way is more in-line with how we do all the other async > setup. But there's another serious issue with this design here, see below. > Ok, previously I was thinking more on how to pass the firmware loading status before enabling low power states (dc5/dc6). Now while thinking about power management framework I have a fundamental doubt - if got request to disable a power-well and firmware loading failed for some reason, should we disable the power-well(option1) or keep it enable(option2). Both the cases will not trigger for dc5/dc6. I understood from your comment to follow option2, I will change the design accordingly as the current implementation based on option1. To implement option2, I can see there is one mutex present for a power domain, do not have mutex for each power-well, which need to be added and can be used as you mentioned above. If we want to follow option1, I can think a better way of managing firmware loading. As firmware is required when display engine goes to low power state, so we can only parse the packaged firmware during driver initialization. Firmware loading (writing to registers) can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6. Planning to implement option2 and will send for review, please correct me if I understood wrongly. >> --- >> 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)) { > The problem is with this design that this wait here isn't allowed to ever > time out, since if it does the book-keeping gets out of sync. And for a > bunch of reasons (delays, deadlocks) we can't have that wait here really > at all, much less one that never times out. > > Chandra has already hit this on a system where the firmware just isn't > available. > > Thanks, Daniel > >> 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@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote: > > > On 5/21/2015 5:41 PM, Daniel Vetter wrote: > >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna 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@intel.com> > >Sorry if this cross with my comments, but upon further digging into this > >code we don't even need a completion at all. As long as we prevent the > >power well from getting disabled by holding a reference for it before we > >launch the firmware loader it'll all be fine. And the async work can then > >just drop that reference when everything is set up. > > > >Yes this means runtime D3 requires the firmware to be loaded, but that's > >imo not a problem. > > > >Also doing it this way is more in-line with how we do all the other async > >setup. But there's another serious issue with this design here, see below. > > > Ok, previously I was thinking more on how to pass the firmware loading status > before enabling low power states (dc5/dc6). Now while thinking about power > management framework I have a fundamental doubt - if got request to disable a power-well > and firmware loading failed for some reason, should we disable the power-well(option1) or > keep it enable(option2). Both the cases will not trigger for dc5/dc6. > > I understood from your comment to follow option2, I will change the design accordingly > as the current implementation based on option1. > > To implement option2, I can see there is one mutex present for a power domain, do not have > mutex for each power-well, which need to be added and can be used as you mentioned above. > > If we want to follow option1, I can think a better way of managing firmware loading. > As firmware is required when display engine goes to low power state, so we can only parse > the packaged firmware during driver initialization. Firmware loading (writing to registers) > can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6. > > Planning to implement option2 and will send for review, please correct me if I understood wrongly. power wells are reference counted, which means the only thing you need to do is grab such a reference. Then the corresponding power well won't ever get disabled until the async firmware loader has done it's job and released the power well reference again. See e.g. how the async rps setup grabs a runtime_pm reference (works the same for power wells, they simply nest within the runtime pm stuff). So for option2 no fiddling with mutexes or power well internals needed at all (well the wait_for can be removed afterwards ofc). And we don't need any other mutex, the csr_mutex and crs.state can be removed afterwards too. -Daniel
On Thu, 2015-05-21 at 23:29 +0200, Daniel Vetter wrote: > On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote: > > > > > > On 5/21/2015 5:41 PM, Daniel Vetter wrote: > > >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna 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@intel.com> > > >Sorry if this cross with my comments, but upon further digging into this > > >code we don't even need a completion at all. As long as we prevent the > > >power well from getting disabled by holding a reference for it before we > > >launch the firmware loader it'll all be fine. And the async work can then > > >just drop that reference when everything is set up. > > > > > >Yes this means runtime D3 requires the firmware to be loaded, but that's > > >imo not a problem. > > > > > >Also doing it this way is more in-line with how we do all the other async > > >setup. But there's another serious issue with this design here, see below. > > > > > Ok, previously I was thinking more on how to pass the firmware loading status > > before enabling low power states (dc5/dc6). Now while thinking about power > > management framework I have a fundamental doubt - if got request to disable a power-well > > and firmware loading failed for some reason, should we disable the power-well(option1) or > > keep it enable(option2). Both the cases will not trigger for dc5/dc6. > > > > I understood from your comment to follow option2, I will change the design accordingly > > as the current implementation based on option1. > > > > To implement option2, I can see there is one mutex present for a power domain, do not have > > mutex for each power-well, which need to be added and can be used as you mentioned above. > > > > If we want to follow option1, I can think a better way of managing firmware loading. > > As firmware is required when display engine goes to low power state, so we can only parse > > the packaged firmware during driver initialization. Firmware loading (writing to registers) > > can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6. > > > > Planning to implement option2 and will send for review, please correct me if I understood wrongly. > > power wells are reference counted, which means the only thing you need to > do is grab such a reference. Then the corresponding power well won't ever > get disabled until the async firmware loader has done it's job and > released the power well reference again. > > See e.g. how the async rps setup grabs a runtime_pm reference (works the > same for power wells, they simply nest within the runtime pm stuff). So > for option2 no fiddling with mutexes or power well internals needed at all > (well the wait_for can be removed afterwards ofc). And we don't need any > other mutex, the csr_mutex and crs.state can be removed afterwards too. > -Daniel Hi Daniel, We already are grabbing RPM reference before start of DMC FW load and release post load completion. DC5/6 can happen without Runtime PM as well. So we need to wait for CSR FW load for some time once we disable PW2. Having completion instead of csr.lock+csr.state is correct thing to do. Thanks, Sagar
On 04/06/15 06:59, Sagar Arun Kamble wrote: > > Hi Daniel, > > We already are grabbing RPM reference before start of DMC FW load and > release post load completion. > > DC5/6 can happen without Runtime PM as well. So we need to wait for CSR > FW load for some time once we disable PW2. > > Having completion instead of csr.lock+csr.state is correct thing to do. > > Thanks, > Sagar > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx Hi guys, we have a new unified loader for GuC and HuC, which we intend should be usable for any other microcontrollers that might be added in future. So it would be good to consider using it for the DMC/CSR/etc. The most-recently posted version was Alex's on 2015-04-29, but we're just about to re-post an updated patchset for command submission via the GuC, which will include the both unified firmware loading module and the GuC-specific part of the loader. Please have a look at these patches and let me know whether there's anything that would help with using the unified code for your uC(s). http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html One thing that may not be obvious from the posted patches is that the uC-specific code can override the way the f/w image is saved e.g. if the loaded file image contains sections that are only needed once, they need not be saved; more generally, if the way that the data in the image file is organised doesn't match the way the data is used, the loader can shuffle the content around for the convenience of the final consumer. (We needed that at one stage, as the GuC firmware format didn't match the constraints of the GuC DMA engine; but the format has been fixed since, so the data-shuffler is not part of the public patch sequence). Thanks, Dave
On Thu, Jun 04, 2015 at 11:29:35AM +0530, Sagar Arun Kamble wrote: > On Thu, 2015-05-21 at 23:29 +0200, Daniel Vetter wrote: > > On Thu, May 21, 2015 at 10:35:07PM +0530, Animesh Manna wrote: > > > > > > > > > On 5/21/2015 5:41 PM, Daniel Vetter wrote: > > > >On Thu, May 21, 2015 at 03:49:52PM +0530, Animesh Manna 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@intel.com> > > > >Sorry if this cross with my comments, but upon further digging into this > > > >code we don't even need a completion at all. As long as we prevent the > > > >power well from getting disabled by holding a reference for it before we > > > >launch the firmware loader it'll all be fine. And the async work can then > > > >just drop that reference when everything is set up. > > > > > > > >Yes this means runtime D3 requires the firmware to be loaded, but that's > > > >imo not a problem. > > > > > > > >Also doing it this way is more in-line with how we do all the other async > > > >setup. But there's another serious issue with this design here, see below. > > > > > > > Ok, previously I was thinking more on how to pass the firmware loading status > > > before enabling low power states (dc5/dc6). Now while thinking about power > > > management framework I have a fundamental doubt - if got request to disable a power-well > > > and firmware loading failed for some reason, should we disable the power-well(option1) or > > > keep it enable(option2). Both the cases will not trigger for dc5/dc6. > > > > > > I understood from your comment to follow option2, I will change the design accordingly > > > as the current implementation based on option1. > > > > > > To implement option2, I can see there is one mutex present for a power domain, do not have > > > mutex for each power-well, which need to be added and can be used as you mentioned above. > > > > > > If we want to follow option1, I can think a better way of managing firmware loading. > > > As firmware is required when display engine goes to low power state, so we can only parse > > > the packaged firmware during driver initialization. Firmware loading (writing to registers) > > > can be done in skl_enable_dc6/gen9_enable_dc5 function itself and then trigger for dc5/dc6. > > > > > > Planning to implement option2 and will send for review, please correct me if I understood wrongly. > > > > power wells are reference counted, which means the only thing you need to > > do is grab such a reference. Then the corresponding power well won't ever > > get disabled until the async firmware loader has done it's job and > > released the power well reference again. > > > > See e.g. how the async rps setup grabs a runtime_pm reference (works the > > same for power wells, they simply nest within the runtime pm stuff). So > > for option2 no fiddling with mutexes or power well internals needed at all > > (well the wait_for can be removed afterwards ofc). And we don't need any > > other mutex, the csr_mutex and crs.state can be removed afterwards too. > > -Daniel > > Hi Daniel, > > We already are grabbing RPM reference before start of DMC FW load and > release post load completion. > > DC5/6 can happen without Runtime PM as well. So we need to wait for CSR > FW load for some time once we disable PW2. If dc5/6 can happen despite you holding a runtime pm reference, then you're holding the wrong runtime pm reference. They nest, hence just walk up the chain of power domains until you have one that does prevent dc5/6. I guess there's a confusion between runtime pm as the overall concept (which I'm talking about here, implemented in intel_rpm.c) and the linux runtime pm api which is only used for the very last level of runtime pm in i915 to control entry/exit to D3. On top of that we stack the power wells i915 specific runtime pm api and the corresponding busyness tracking on the render side. > Having completion instead of csr.lock+csr.state is correct thing to do. I hope the above explanation helps. I can follow up with a sketch patch too if you want. Cheers, Daniel
On Thu, Jun 04, 2015 at 03:36:32PM +0100, Dave Gordon wrote: > On 04/06/15 06:59, Sagar Arun Kamble wrote: > > > > Hi Daniel, > > > > We already are grabbing RPM reference before start of DMC FW load and > > release post load completion. > > > > DC5/6 can happen without Runtime PM as well. So we need to wait for CSR > > FW load for some time once we disable PW2. > > > > Having completion instead of csr.lock+csr.state is correct thing to do. > > > > Thanks, > > Sagar > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > Hi guys, > > we have a new unified loader for GuC and HuC, which we intend should be > usable for any other microcontrollers that might be added in future. So > it would be good to consider using it for the DMC/CSR/etc. > > The most-recently posted version was Alex's on 2015-04-29, but we're > just about to re-post an updated patchset for command submission via the > GuC, which will include the both unified firmware loading module and the > GuC-specific part of the loader. > > Please have a look at these patches and let me know whether there's > anything that would help with using the unified code for your uC(s). > > http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html > http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html > > One thing that may not be obvious from the posted patches is that the > uC-specific code can override the way the f/w image is saved e.g. if the > loaded file image contains sections that are only needed once, they need > not be saved; more generally, if the way that the data in the image file > is organised doesn't match the way the data is used, the loader can > shuffle the content around for the convenience of the final consumer. > (We needed that at one stage, as the GuC firmware format didn't match > the constraints of the GuC DMA engine; but the format has been fixed > since, so the data-shuffler is not part of the public patch sequence). Hm just looked at the generic firmware loader and not convinced this is a good idea at all. As this discussion here shows synchronization here is a tricky business. And in general driver load ordering is full of peril. Trying to force a common abstraction for something rather trivial (we have completions for the most basic case already) will only result in pains trying to work around the not-quite-fitting infrastructure. In my experience trying to extract common code at all costs is harmful way too often. -Daniel
On 15/06/15 11:07, Daniel Vetter wrote: > On Thu, Jun 04, 2015 at 03:36:32PM +0100, Dave Gordon wrote: >> On 04/06/15 06:59, Sagar Arun Kamble wrote: >>> >>> Hi Daniel, >>> >>> We already are grabbing RPM reference before start of DMC FW load and >>> release post load completion. >>> >>> DC5/6 can happen without Runtime PM as well. So we need to wait for CSR >>> FW load for some time once we disable PW2. >>> >>> Having completion instead of csr.lock+csr.state is correct thing to do. >>> >>> Thanks, >>> Sagar >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> Hi guys, >> >> we have a new unified loader for GuC and HuC, which we intend should be >> usable for any other microcontrollers that might be added in future. So >> it would be good to consider using it for the DMC/CSR/etc. >> >> The most-recently posted version was Alex's on 2015-04-29, but we're >> just about to re-post an updated patchset for command submission via the >> GuC, which will include the both unified firmware loading module and the >> GuC-specific part of the loader. >> >> Please have a look at these patches and let me know whether there's >> anything that would help with using the unified code for your uC(s). >> >> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065575.html >> http://lists.freedesktop.org/archives/intel-gfx/2015-April/065583.html >> >> One thing that may not be obvious from the posted patches is that the >> uC-specific code can override the way the f/w image is saved e.g. if the >> loaded file image contains sections that are only needed once, they need >> not be saved; more generally, if the way that the data in the image file >> is organised doesn't match the way the data is used, the loader can >> shuffle the content around for the convenience of the final consumer. >> (We needed that at one stage, as the GuC firmware format didn't match >> the constraints of the GuC DMA engine; but the format has been fixed >> since, so the data-shuffler is not part of the public patch sequence). > > Hm just looked at the generic firmware loader and not convinced this is a > good idea at all. As this discussion here shows synchronization here is a > tricky business. And in general driver load ordering is full of peril. > Trying to force a common abstraction for something rather trivial (we have > completions for the most basic case already) will only result in pains > trying to work around the not-quite-fitting infrastructure. > > In my experience trying to extract common code at all costs is harmful > way too often. > -Daniel That version is now obsolete; the new version of the generic loader code is in the GuC submission series that I just posted. Thanks, .Dave.
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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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"); } } }
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@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(-)