On Tue, Jul 07, 2015 at 06:10:12PM +0530, Animesh Manna wrote:
> v1: Based on review comments from Daniel following changes are done.
> - More focus is given for better synchronization.
> - Replaced async firmware loading by using request_firmawre() call.
> - Prevented entering in dc5/dc6 while firmware loading in process.
> Now register programming for dc5/dc6 always will happen followed
> by firmware loading.
> - Removed the csr-lock and csr-state which was used before.
> - Added a async work which is responsible for both loading the
> firmware and register programming for dc5/dc6.
> - Added flush_work() to explicitly synchronize the async work
> during suspend and driver unloading.
This should all be separate patches. Please split them up.
Thanks, Daniel
>
> Signed-off-by: Animesh Manna <animesh.ma...@intel.com>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 1 -
> drivers/gpu/drm/i915/i915_drv.c | 10 +--
> drivers/gpu/drm/i915/i915_drv.h | 13 ++-
> drivers/gpu/drm/i915/intel_csr.c | 141
> +++++++++++---------------------
> drivers/gpu/drm/i915/intel_drv.h | 5 +-
> drivers/gpu/drm/i915/intel_runtime_pm.c | 63 +++++++-------
> 6 files changed, 92 insertions(+), 141 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..1ebf0e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -820,7 +820,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);
>
> intel_pm_setup(dev);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e44dc0d..6049ce3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1019,11 +1019,8 @@ static int skl_suspend_complete(struct
> drm_i915_private *dev_priv)
> {
> /* Enabling DC6 is not a hard requirement to enter runtime D3 */
>
> - /*
> - * 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);
> + dev_priv->csr.dmc_present = false;
> + flush_work(&dev_priv->csr.csr_work);
>
> skl_uninit_cdclk(dev_priv);
>
> @@ -1071,10 +1068,7 @@ static int bxt_resume_prepare(struct drm_i915_private
> *dev_priv)
>
> static int skl_resume_prepare(struct drm_i915_private *dev_priv)
> {
> - struct drm_device *dev = dev_priv->dev;
> -
> skl_init_cdclk(dev_priv);
> - intel_csr_load_program(dev);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1dbd957..73509f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -737,20 +737,16 @@ 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 {
> + bool dc_state_req;
> + struct work_struct csr_work;
> 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;
> + bool dmc_present;
> };
>
> #define DEV_INFO_FOR_EACH_FLAG(func, sep) \
> @@ -2631,6 +2627,9 @@ extern void i915_update_gfx_val(struct drm_i915_private
> *dev_priv);
> int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
> void i915_firmware_load_error_print(const char *fw_path, int err);
>
> +/* intel_csr.c */
> +void intel_csr_setdc_work_fn(struct work_struct *__work);
> +
> /* intel_hotplug.c */
> void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32
> long_mask);
> void intel_hpd_init(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_csr.c
> b/drivers/gpu/drm/i915/intel_csr.c
> index 6d8a7bf..44f05a8 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_ver1.bin"
> @@ -199,48 +192,6 @@ static char intel_get_substepping(struct drm_device *dev)
> return -ENODATA;
> }
>
> -/**
> - * 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.
> - *
> - * CSR firmware is read from a .bin file and kept in internal memory one
> time.
> - * Everytime display comes back from low power state this function is called
> to
> - * copy the firmware from internal memory to registers.
> - */
> void intel_csr_load_program(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -252,7 +203,6 @@ void intel_csr_load_program(struct drm_device *dev)
> 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_BASE + i * 4,
> @@ -262,14 +212,11 @@ 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)
> +static void finish_csr_load(const struct firmware *fw,
> + struct drm_i915_private *dev_priv)
> {
> - struct drm_i915_private *dev_priv = context;
> struct drm_device *dev = dev_priv->dev;
> struct intel_css_header *css_header;
> struct intel_package_header *package_header;
> @@ -280,16 +227,15 @@ 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);
> - goto out;
> + return;
> }
>
> if ((stepping == -ENODATA) || (substepping == -ENODATA)) {
> DRM_ERROR("Unknown stepping info, firmware loading failed\n");
> - goto out;
> + return;
> }
>
> /* Extract CSS Header information*/
> @@ -298,7 +244,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> (css_header->header_len * 4)) {
> DRM_ERROR("Firmware has wrong CSS header length %u bytes\n",
> (css_header->header_len * 4));
> - goto out;
> + return;
> }
> readcount += sizeof(struct intel_css_header);
>
> @@ -309,7 +255,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> (package_header->header_len * 4)) {
> DRM_ERROR("Firmware has wrong package header length %u bytes\n",
> (package_header->header_len * 4));
> - goto out;
> + return;
> }
> readcount += sizeof(struct intel_package_header);
>
> @@ -329,7 +275,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> }
> if (dmc_offset == CSR_DEFAULT_FW_OFFSET) {
> DRM_ERROR("Firmware not supported for %c stepping\n", stepping);
> - goto out;
> + return;
> }
> readcount += dmc_offset;
>
> @@ -338,7 +284,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> if (sizeof(struct intel_dmc_header) != (dmc_header->header_len)) {
> DRM_ERROR("Firmware has wrong dmc header length %u bytes\n",
> (dmc_header->header_len));
> - goto out;
> + return;
> }
> readcount += sizeof(struct intel_dmc_header);
>
> @@ -346,7 +292,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> if (dmc_header->mmio_count > ARRAY_SIZE(csr->mmioaddr)) {
> DRM_ERROR("Firmware has wrong mmio count %u\n",
> dmc_header->mmio_count);
> - goto out;
> + return;
> }
> csr->mmio_count = dmc_header->mmio_count;
> for (i = 0; i < dmc_header->mmio_count; i++) {
> @@ -354,7 +300,7 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> dmc_header->mmioaddr[i] > CSR_MMIO_END_RANGE) {
> DRM_ERROR(" Firmware has wrong mmio address 0x%x\n",
> dmc_header->mmioaddr[i]);
> - goto out;
> + return;
> }
> csr->mmioaddr[i] = dmc_header->mmioaddr[i];
> csr->mmiodata[i] = dmc_header->mmiodata[i];
> @@ -364,14 +310,14 @@ static void finish_csr_load(const struct firmware *fw,
> void *context)
> nbytes = dmc_header->fw_size * 4;
> if (nbytes > CSR_MAX_FW_SIZE) {
> DRM_ERROR("CSR firmware too big (%u) bytes\n", nbytes);
> - goto out;
> + return;
> }
> csr->dmc_fw_size = dmc_header->fw_size;
>
> csr->dmc_payload = kmalloc(nbytes, GFP_KERNEL);
> if (!csr->dmc_payload) {
> DRM_ERROR("Memory allocation failed for dmc payload\n");
> - goto out;
> + return;
> }
>
> dmc_payload = csr->dmc_payload;
> @@ -387,16 +333,42 @@ 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;
> + dev_priv->csr.dmc_present = true;
>
> DRM_DEBUG_KMS("Finished loading %s\n", dev_priv->csr.fw_path);
> -out:
> - if (fw_loaded)
> - intel_runtime_pm_put(dev_priv);
> - else
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> +}
>
> - release_firmware(fw);
> +/**
> + * intel_display_load_csr() - write the firmware from memory to register.
> + * @dev: drm device.
> + *
> + * CSR firmware is read from a .bin file and kept in internal memory one
> time.
> + * Everytime display comes back from low power state this function is called
> to
> + * copy the firmware to registers.
> + */
> +
> +void intel_display_load_csr(struct drm_i915_private *dev_priv)
> +{
> + struct intel_csr *csr = &dev_priv->csr;
> + const struct firmware *fw;
> + int ret;
> +
> + if (dev_priv->csr.dmc_present)
> + intel_csr_load_program(dev_priv->dev);
> + else {
> + /* CSR supported for platform, load firmware */
> + ret = request_firmware(&fw, csr->fw_path,
> + &dev_priv->dev->pdev->dev);
> +
> + DRM_DEBUG_KMS("Loading %d\n", ret);
> +
> + if (ret) {
> + i915_firmware_load_error_print(csr->fw_path, ret);
> + return;
> + }
> + finish_csr_load(fw, dev_priv);
> + release_firmware(fw);
> + }
> }
>
> /**
> @@ -410,7 +382,6 @@ void intel_csr_ucode_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_csr *csr = &dev_priv->csr;
> - int ret;
>
> if (!HAS_CSR(dev))
> return;
> @@ -419,27 +390,12 @@ 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;
> }
>
> DRM_DEBUG_KMS("Loading %s\n", csr->fw_path);
>
> - /*
> - * Obtain a runtime pm reference, until CSR is loaded,
> - * to avoid entering runtime-suspend.
> - */
> - intel_runtime_pm_get(dev_priv);
> -
> - /* 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) {
> - i915_firmware_load_error_print(csr->fw_path, ret);
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> - }
> + INIT_WORK(&csr->csr_work, intel_csr_setdc_work_fn);
> }
>
> /**
> @@ -456,14 +412,13 @@ void intel_csr_ucode_fini(struct drm_device *dev)
> if (!HAS_CSR(dev))
> return;
>
> - intel_csr_load_status_set(dev_priv, FW_FAILED);
> + flush_work(&dev_priv->csr.csr_work);
> +
> 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 3f0a890..b427407 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1163,10 +1163,7 @@ 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_display_load_csr(struct drm_i915_private *dev_priv);
> 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 1a45385..61c018d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -527,7 +527,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");
> }
> @@ -563,6 +562,37 @@ static void skl_disable_dc6(struct drm_i915_private
> *dev_priv)
> POSTING_READ(DC_STATE_EN);
> }
>
> +void intel_csr_setdc_work_fn(struct work_struct *__work)
> +{
> + struct drm_i915_private *dev_priv =
> + container_of(__work, struct drm_i915_private, csr.csr_work);
> + struct intel_csr *csr = &dev_priv->csr;
> +
> + if (csr->dc_state_req) {
> + intel_display_load_csr(dev_priv);
> +
> + if (IS_SKYLAKE(dev_priv->dev))
> + skl_enable_dc6(dev_priv);
> + else
> + gen9_enable_dc5(dev_priv);
> + } else {
> + if (IS_SKYLAKE(dev_priv->dev)) {
> + skl_disable_dc6(dev_priv);
> + /*
> + * DDI buffer programming unnecessary during
> + * driver-load/resume as it's already done during
> + * modeset initialization then. It's also invalid
> + * here as encoder list is still uninitialized.
> + */
> + if (!dev_priv->power_domains.initializing)
> + intel_prepare_ddi(dev_priv->dev);
> + } else {
> + gen9_disable_dc5(dev_priv);
> + }
> + }
> +}
> +
> +
> static void skl_set_power_well(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well, bool enable)
> {
> @@ -612,18 +642,8 @@ static void skl_set_power_well(struct drm_i915_private
> *dev_priv,
> when request is to disable!\n");
> if ((GEN9_ENABLE_DC5(dev) || SKL_ENABLE_DC6(dev)) &&
> power_well->data == SKL_DISP_PW_2) {
> - if (SKL_ENABLE_DC6(dev)) {
> - skl_disable_dc6(dev_priv);
> - /*
> - * DDI buffer programming unnecessary
> during driver-load/resume
> - * as it's already done during modeset
> initialization then.
> - * It's also invalid here as encoder
> list is still uninitialized.
> - */
> - if
> (!dev_priv->power_domains.initializing)
> - intel_prepare_ddi(dev);
> - } else {
> - gen9_disable_dc5(dev_priv);
> - }
> + dev_priv->csr.dc_state_req = false;
> + schedule_work(&dev_priv->csr.csr_work);
> }
> I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> }
> @@ -644,21 +664,8 @@ 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 (SKL_ENABLE_DC6(dev))
> - skl_enable_dc6(dev_priv);
> - else
> - gen9_enable_dc5(dev_priv);
> + dev_priv->csr.dc_state_req = true;
> + schedule_work(&dev_priv->csr.csr_work);
> }
> }
> }
> --
> 2.0.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
