[v2,13/13] drm/i915: Acquire P-Unit access when modifying P-Unit settings
diff mbox

Message ID 20170123210958.18410-14-hdegoede@redhat.com
State Awaiting Upstream
Headers show

Commit Message

Hans de Goede Jan. 23, 2017, 9:09 p.m. UTC
Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
around P-Unit write accesses.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
Changes in v2:
-Spelling: P-Unit, PMIC
-Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
---
 drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
 drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
 3 files changed, 24 insertions(+)

Comments

Ville Syrjälä Jan. 27, 2017, 1:51 p.m. UTC | #1
On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> around P-Unit write accesses.

Can't we just stuff the calls into the actual punit write function
rather than sprinkling them all over the place?

+ a comment would be nice why it's there.

Do we need a kconfig select/depends on the iosf_mbi thing? Or some
ifdefs?

> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
> Changes in v2:
> -Spelling: P-Unit, PMIC
> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> ---
>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5604701..13e5152 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -47,6 +47,7 @@
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  #include <linux/reservation.h>
> +#include <asm/iosf_mbi.h>
>  
>  static bool is_mmio_work(struct intel_flip_work *work)
>  {
> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  		cmd = 0;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
> +
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK;
>  	val |= (cmd << DSPFREQGUAR_SHIFT);
> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	mutex_lock(&dev_priv->sb_lock);
> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK_CHV;
>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	intel_update_cdclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 249623d..adff84a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -32,6 +32,7 @@
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <asm/iosf_mbi.h>
>  
>  /**
>   * DOC: RC6
> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>  	if (enable)
> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>  		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>  
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_acquire();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	if (enable)
> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>  
> +	iosf_mbi_punit_release();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		mutex_lock(&dev_priv->rps.hw_lock);
> +		iosf_mbi_punit_acquire();
>  
>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>  		}
>  
> +		iosf_mbi_punit_release();
>  		mutex_unlock(&dev_priv->rps.hw_lock);
>  	}
>  
> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>  
>  	if (val != dev_priv->rps.cur_freq) {
> +		iosf_mbi_punit_acquire();
>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> +		iosf_mbi_punit_release();
>  		if (!IS_CHERRYVIEW(dev_priv))
>  			gen6_set_rps_thresholds(dev_priv, val);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c0b7e95..e66bcc8 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -28,6 +28,7 @@
>  
>  #include <linux/pm_runtime.h>
>  #include <linux/vgaarb.h>
> +#include <asm/iosf_mbi.h>
>  
>  #include "i915_drv.h"
>  #include "intel_drv.h"
> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_acquire();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>  	ctrl &= ~mask;
>  	ctrl |= state;
> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>  
> +	iosf_mbi_punit_release();
> +
>  #undef COND
>  
>  out:
> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_acquire();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	ctrl &= ~DP_SSC_MASK(pipe);
>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>  
> +	iosf_mbi_punit_release();
> +
>  #undef COND
>  
>  out:
> -- 
> 2.9.3
Hans de Goede Jan. 28, 2017, 4:25 p.m. UTC | #2
Hi,

On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>> around P-Unit write accesses.
>
> Can't we just stuff the calls into the actual punit write function
> rather than sprinkling them all over the place?

punit access is acquired across sections like this:

         iosf_mbi_punit_acquire();

         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
         val &= ~DSPFREQGUAR_MASK;
         val |= (cmd << DSPFREQGUAR_SHIFT);
         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
                      50)) {
                 DRM_ERROR("timed out waiting for CDclk change\n");
         }
         iosf_mbi_punit_release();

Where we want to wait for the requested change to have taken
effect before releasing the punit.

>
> + a comment would be nice why it's there.

I will add comments to the acquire calls.

> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> ifdefs?

No, the iosf_mbi header defines empty inline versions of
iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
this does mean that iosf_mbi must be builtin if the i915
driver is. I'll add:

	depends on DRM_I915=IOSF_MBI || IOSF_MBI=y

To the i915 Kconfig to enforce this.

Regards,

Hans


>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>> Changes in v2:
>> -Spelling: P-Unit, PMIC
>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>> ---
>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5604701..13e5152 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -47,6 +47,7 @@
>>  #include <drm/drm_rect.h>
>>  #include <linux/dma_remapping.h>
>>  #include <linux/reservation.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  static bool is_mmio_work(struct intel_flip_work *work)
>>  {
>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		cmd = 0;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>> +
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT);
>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	mutex_lock(&dev_priv->sb_lock);
>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK_CHV;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	intel_update_cdclk(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 249623d..adff84a 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -32,6 +32,7 @@
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include <linux/module.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  /**
>>   * DOC: RC6
>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>  	if (enable)
>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>  		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_acquire();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	if (enable)
>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>
>> +	iosf_mbi_punit_release();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>
>>  	if (IS_CHERRYVIEW(dev_priv)) {
>>  		mutex_lock(&dev_priv->rps.hw_lock);
>> +		iosf_mbi_punit_acquire();
>>
>>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>  		}
>>
>> +		iosf_mbi_punit_release();
>>  		mutex_unlock(&dev_priv->rps.hw_lock);
>>  	}
>>
>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>
>>  	if (val != dev_priv->rps.cur_freq) {
>> +		iosf_mbi_punit_acquire();
>>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>> +		iosf_mbi_punit_release();
>>  		if (!IS_CHERRYVIEW(dev_priv))
>>  			gen6_set_rps_thresholds(dev_priv, val);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index c0b7e95..e66bcc8 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -28,6 +28,7 @@
>>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/vgaarb.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_acquire();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>  	ctrl &= ~mask;
>>  	ctrl |= state;
>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>
>> +	iosf_mbi_punit_release();
>> +
>>  #undef COND
>>
>>  out:
>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_acquire();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	ctrl &= ~DP_SSC_MASK(pipe);
>>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>
>> +	iosf_mbi_punit_release();
>> +
>>  #undef COND
>>
>>  out:
>> --
>> 2.9.3
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Jan. 28, 2017, 5:18 p.m. UTC | #3
Hi,

On 01/28/2017 05:25 PM, Hans de Goede wrote:
> Hi,
>
> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>> around P-Unit write accesses.
>>
>> Can't we just stuff the calls into the actual punit write function
>> rather than sprinkling them all over the place?
>
> punit access is acquired across sections like this:
>
>         iosf_mbi_punit_acquire();
>
>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>         val &= ~DSPFREQGUAR_MASK;
>         val |= (cmd << DSPFREQGUAR_SHIFT);
>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>                      50)) {
>                 DRM_ERROR("timed out waiting for CDclk change\n");
>         }
>         iosf_mbi_punit_release();
>
> Where we want to wait for the requested change to have taken
> effect before releasing the punit.
>
>>
>> + a comment would be nice why it's there.
>
> I will add comments to the acquire calls.
>
>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>> ifdefs?
>
> No, the iosf_mbi header defines empty inline versions of
> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> this does mean that iosf_mbi must be builtin if the i915
> driver is. I'll add:
>
>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>
> To the i915 Kconfig to enforce this.

Hmm, ok so that does not work (long cyclic dependency through the
selection of ACPI_VIDEO).

So I've now added this instead:

	# iosf_mbi needs to be builtin if we are builtin
	select IOSF_MBI if DRM_I915=y

Regards,

Hans


>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>> ---
>>> Changes in v2:
>>> -Spelling: P-Unit, PMIC
>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>  3 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 5604701..13e5152 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -47,6 +47,7 @@
>>>  #include <drm/drm_rect.h>
>>>  #include <linux/dma_remapping.h>
>>>  #include <linux/reservation.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>  {
>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>          cmd = 0;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      val &= ~DSPFREQGUAR_MASK;
>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>               50)) {
>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>      }
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>
>>>      mutex_lock(&dev_priv->sb_lock);
>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>               50)) {
>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>      }
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>
>>>      intel_update_cdclk(dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 249623d..adff84a 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -32,6 +32,7 @@
>>>  #include "../../../platform/x86/intel_ips.h"
>>>  #include <linux/module.h>
>>>  #include <drm/drm_atomic_helper.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  /**
>>>   * DOC: RC6
>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>      u32 val;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>      if (enable)
>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>  }
>>>
>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>      u32 val;
>>>
>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>> +    iosf_mbi_punit_acquire();
>>>
>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      if (enable)
>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>
>>> +    iosf_mbi_punit_release();
>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>  }
>>>
>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>
>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>> +        iosf_mbi_punit_acquire();
>>>
>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>          }
>>>
>>> +        iosf_mbi_punit_release();
>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>      }
>>>
>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>
>>>      if (val != dev_priv->rps.cur_freq) {
>>> +        iosf_mbi_punit_acquire();
>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>> +        iosf_mbi_punit_release();
>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>      }
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index c0b7e95..e66bcc8 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -28,6 +28,7 @@
>>>
>>>  #include <linux/pm_runtime.h>
>>>  #include <linux/vgaarb.h>
>>> +#include <asm/iosf_mbi.h>
>>>
>>>  #include "i915_drv.h"
>>>  #include "intel_drv.h"
>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>      if (COND)
>>>          goto out;
>>>
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>      ctrl &= ~mask;
>>>      ctrl |= state;
>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>                state,
>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>
>>> +    iosf_mbi_punit_release();
>>> +
>>>  #undef COND
>>>
>>>  out:
>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>      if (COND)
>>>          goto out;
>>>
>>> +    iosf_mbi_punit_acquire();
>>> +
>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>                state,
>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>
>>> +    iosf_mbi_punit_release();
>>> +
>>>  #undef COND
>>>
>>>  out:
>>> --
>>> 2.9.3
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjälä Jan. 30, 2017, 1:10 p.m. UTC | #4
On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> > Hi,
> >
> > On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>> around P-Unit write accesses.
> >>
> >> Can't we just stuff the calls into the actual punit write function
> >> rather than sprinkling them all over the place?
> >
> > punit access is acquired across sections like this:
> >
> >         iosf_mbi_punit_acquire();
> >
> >         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >         val &= ~DSPFREQGUAR_MASK;
> >         val |= (cmd << DSPFREQGUAR_SHIFT);
> >         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >                      50)) {
> >                 DRM_ERROR("timed out waiting for CDclk change\n");
> >         }
> >         iosf_mbi_punit_release();
> >
> > Where we want to wait for the requested change to have taken
> > effect before releasing the punit.

Hmm. That's somewhat unfortunate. It also highlights a problem with the
patch wrt. RPS. We don't wait for the GPU to actually change frequencies
in set_rps() because that would slow things down too much. So I have to
wonder how much luck is needed to make this workaround really effective.

> >
> >>
> >> + a comment would be nice why it's there.
> >
> > I will add comments to the acquire calls.
> >
> >> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >> ifdefs?
> >
> > No, the iosf_mbi header defines empty inline versions of
> > iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> > this does mean that iosf_mbi must be builtin if the i915
> > driver is. I'll add:
> >
> >     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >
> > To the i915 Kconfig to enforce this.
> 
> Hmm, ok so that does not work (long cyclic dependency through the
> selection of ACPI_VIDEO).
> 
> So I've now added this instead:
> 
> 	# iosf_mbi needs to be builtin if we are builtin
> 	select IOSF_MBI if DRM_I915=y

That's probably not going to help anyone since i915 is usually a module.

> 
> Regards,
> 
> Hans
> 
> 
> >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> >>> ---
> >>> Changes in v2:
> >>> -Spelling: P-Unit, PMIC
> >>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
> >>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
> >>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
> >>>  3 files changed, 24 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 5604701..13e5152 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -47,6 +47,7 @@
> >>>  #include <drm/drm_rect.h>
> >>>  #include <linux/dma_remapping.h>
> >>>  #include <linux/reservation.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  static bool is_mmio_work(struct intel_flip_work *work)
> >>>  {
> >>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>          cmd = 0;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      val &= ~DSPFREQGUAR_MASK;
> >>>      val |= (cmd << DSPFREQGUAR_SHIFT);
> >>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>               50)) {
> >>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>      }
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>
> >>>      mutex_lock(&dev_priv->sb_lock);
> >>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      val &= ~DSPFREQGUAR_MASK_CHV;
> >>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>               50)) {
> >>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>      }
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>
> >>>      intel_update_cdclk(dev_priv);
> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>> index 249623d..adff84a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>> @@ -32,6 +32,7 @@
> >>>  #include "../../../platform/x86/intel_ips.h"
> >>>  #include <linux/module.h>
> >>>  #include <drm/drm_atomic_helper.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  /**
> >>>   * DOC: RC6
> >>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>      u32 val;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> >>>      if (enable)
> >>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> >>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> >>>
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>  }
> >>>
> >>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>      u32 val;
> >>>
> >>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>> +    iosf_mbi_punit_acquire();
> >>>
> >>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      if (enable)
> >>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
> >>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>
> >>> +    iosf_mbi_punit_release();
> >>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>  }
> >>>
> >>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>
> >>>      if (IS_CHERRYVIEW(dev_priv)) {
> >>>          mutex_lock(&dev_priv->rps.hw_lock);
> >>> +        iosf_mbi_punit_acquire();
> >>>
> >>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>>          }
> >>>
> >>> +        iosf_mbi_punit_release();
> >>>          mutex_unlock(&dev_priv->rps.hw_lock);
> >>>      }
> >>>
> >>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
> >>>
> >>>      if (val != dev_priv->rps.cur_freq) {
> >>> +        iosf_mbi_punit_acquire();
> >>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>> +        iosf_mbi_punit_release();
> >>>          if (!IS_CHERRYVIEW(dev_priv))
> >>>              gen6_set_rps_thresholds(dev_priv, val);
> >>>      }
> >>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> index c0b7e95..e66bcc8 100644
> >>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>> @@ -28,6 +28,7 @@
> >>>
> >>>  #include <linux/pm_runtime.h>
> >>>  #include <linux/vgaarb.h>
> >>> +#include <asm/iosf_mbi.h>
> >>>
> >>>  #include "i915_drv.h"
> >>>  #include "intel_drv.h"
> >>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>      if (COND)
> >>>          goto out;
> >>>
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
> >>>      ctrl &= ~mask;
> >>>      ctrl |= state;
> >>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>                state,
> >>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
> >>>
> >>> +    iosf_mbi_punit_release();
> >>> +
> >>>  #undef COND
> >>>
> >>>  out:
> >>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>      if (COND)
> >>>          goto out;
> >>>
> >>> +    iosf_mbi_punit_acquire();
> >>> +
> >>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>      ctrl &= ~DP_SSC_MASK(pipe);
> >>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> >>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>                state,
> >>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
> >>>
> >>> +    iosf_mbi_punit_release();
> >>> +
> >>>  #undef COND
> >>>
> >>>  out:
> >>> --
> >>> 2.9.3
> >>
Hans de Goede Jan. 30, 2017, 3:02 p.m. UTC | #5
Hi,

On 30-01-17 14:10, Ville Syrjälä wrote:
> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>> around P-Unit write accesses.
>>>>
>>>> Can't we just stuff the calls into the actual punit write function
>>>> rather than sprinkling them all over the place?
>>>
>>> punit access is acquired across sections like this:
>>>
>>>         iosf_mbi_punit_acquire();
>>>
>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>         val &= ~DSPFREQGUAR_MASK;
>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>                      50)) {
>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>         }
>>>         iosf_mbi_punit_release();
>>>
>>> Where we want to wait for the requested change to have taken
>>> effect before releasing the punit.
>
> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> in set_rps() because that would slow things down too much. So I have to
> wonder how much luck is needed to make this workaround really effective.

So the history of this patch-set is that I wrote this patch before
writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
active (patch 12/13). Since a lot of testing was done with this
patch included in the patch-set and since it seemed a good idea
regardless (given my experience with accessing the punit vs
pmic bus accesses) I decided to leave it in.

Possibly just the patch to get FORCEWAKE_ALL is enough, that one
actually fixed things for me. That is also why I made this the
last patch in the set. I asked tagorereddy to test his system
without this patch, but he did not get around to that.

After all we do tell the punit to not touch the bus by acquiring
the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
for RPS freq changes it honors that and properly waits. Maybe it
honors that for all punit requests i915 does and the only real
problem is the forcewake stuff ?

I can try to drop this patch from my queue and run without it
for a while and see if things don't regress. And also ask
tagorereddy again to test his system that way.

Does that (dropping this patch for now) sound like a good idea?

>>>> + a comment would be nice why it's there.
>>>
>>> I will add comments to the acquire calls.
>>>
>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>> ifdefs?
>>>
>>> No, the iosf_mbi header defines empty inline versions of
>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>> this does mean that iosf_mbi must be builtin if the i915
>>> driver is. I'll add:
>>>
>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>
>>> To the i915 Kconfig to enforce this.
>>
>> Hmm, ok so that does not work (long cyclic dependency through the
>> selection of ACPI_VIDEO).
>>
>> So I've now added this instead:
>>
>> 	# iosf_mbi needs to be builtin if we are builtin
>> 	select IOSF_MBI if DRM_I915=y
>
> That's probably not going to help anyone since i915 is usually a module.

Right, that is fine, then either the IOSF_MBI symbols are available,
or IOSF_MBI is disabled and we get the inline nops from the header.

The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
realistic IMHO, but will get triggered by the random-config testing
several contributors do and lead to an unresolved symbol error there.

Hmm, thinking about this, this hunk actually belongs in 12/13 as that
is the first patch to use iosf_mbi functions.

Regards,

Hans



>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>>>> ---
>>>>> Changes in v2:
>>>>> -Spelling: P-Unit, PMIC
>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>> ---
>>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>>>  3 files changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 5604701..13e5152 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -47,6 +47,7 @@
>>>>>  #include <drm/drm_rect.h>
>>>>>  #include <linux/dma_remapping.h>
>>>>>  #include <linux/reservation.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>>>  {
>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>          cmd = 0;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      val &= ~DSPFREQGUAR_MASK;
>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>               50)) {
>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>      }
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>
>>>>>      mutex_lock(&dev_priv->sb_lock);
>>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>               50)) {
>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>      }
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>
>>>>>      intel_update_cdclk(dev_priv);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>> index 249623d..adff84a 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>> @@ -32,6 +32,7 @@
>>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>>  #include <linux/module.h>
>>>>>  #include <drm/drm_atomic_helper.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  /**
>>>>>   * DOC: RC6
>>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>      u32 val;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>>>      if (enable)
>>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>  }
>>>>>
>>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>      u32 val;
>>>>>
>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +    iosf_mbi_punit_acquire();
>>>>>
>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      if (enable)
>>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>  }
>>>>>
>>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>
>>>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>>>> +        iosf_mbi_punit_acquire();
>>>>>
>>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>>          }
>>>>>
>>>>> +        iosf_mbi_punit_release();
>>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>      }
>>>>>
>>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>>>
>>>>>      if (val != dev_priv->rps.cur_freq) {
>>>>> +        iosf_mbi_punit_acquire();
>>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>> +        iosf_mbi_punit_release();
>>>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>>>      }
>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> index c0b7e95..e66bcc8 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>> @@ -28,6 +28,7 @@
>>>>>
>>>>>  #include <linux/pm_runtime.h>
>>>>>  #include <linux/vgaarb.h>
>>>>> +#include <asm/iosf_mbi.h>
>>>>>
>>>>>  #include "i915_drv.h"
>>>>>  #include "intel_drv.h"
>>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>      if (COND)
>>>>>          goto out;
>>>>>
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>>>      ctrl &= ~mask;
>>>>>      ctrl |= state;
>>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>                state,
>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>> +
>>>>>  #undef COND
>>>>>
>>>>>  out:
>>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>      if (COND)
>>>>>          goto out;
>>>>>
>>>>> +    iosf_mbi_punit_acquire();
>>>>> +
>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>                state,
>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>>>
>>>>> +    iosf_mbi_punit_release();
>>>>> +
>>>>>  #undef COND
>>>>>
>>>>>  out:
>>>>> --
>>>>> 2.9.3
>>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjälä Jan. 30, 2017, 3:11 p.m. UTC | #6
On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-01-17 14:10, Ville Syrjälä wrote:
> > On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>> around P-Unit write accesses.
> >>>>
> >>>> Can't we just stuff the calls into the actual punit write function
> >>>> rather than sprinkling them all over the place?
> >>>
> >>> punit access is acquired across sections like this:
> >>>
> >>>         iosf_mbi_punit_acquire();
> >>>
> >>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>         val &= ~DSPFREQGUAR_MASK;
> >>>         val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>>                      50)) {
> >>>                 DRM_ERROR("timed out waiting for CDclk change\n");
> >>>         }
> >>>         iosf_mbi_punit_release();
> >>>
> >>> Where we want to wait for the requested change to have taken
> >>> effect before releasing the punit.
> >
> > Hmm. That's somewhat unfortunate. It also highlights a problem with the
> > patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> > in set_rps() because that would slow things down too much. So I have to
> > wonder how much luck is needed to make this workaround really effective.
> 
> So the history of this patch-set is that I wrote this patch before
> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> active (patch 12/13). Since a lot of testing was done with this
> patch included in the patch-set and since it seemed a good idea
> regardless (given my experience with accessing the punit vs
> pmic bus accesses) I decided to leave it in.
> 
> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> actually fixed things for me. That is also why I made this the
> last patch in the set. I asked tagorereddy to test his system
> without this patch, but he did not get around to that.
> 
> After all we do tell the punit to not touch the bus by acquiring
> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> for RPS freq changes it honors that and properly waits. Maybe it
> honors that for all punit requests i915 does and the only real
> problem is the forcewake stuff ?
> 
> I can try to drop this patch from my queue and run without it
> for a while and see if things don't regress. And also ask
> tagorereddy again to test his system that way.
> 
> Does that (dropping this patch for now) sound like a good idea?

More test results couldn't hurt at least. It also makes me wonder if
just bumping the timeouts to some ridiculously high value would fix
the problem as well.

> 
> >>>> + a comment would be nice why it's there.
> >>>
> >>> I will add comments to the acquire calls.
> >>>
> >>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>> ifdefs?
> >>>
> >>> No, the iosf_mbi header defines empty inline versions of
> >>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>> this does mean that iosf_mbi must be builtin if the i915
> >>> driver is. I'll add:
> >>>
> >>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>
> >>> To the i915 Kconfig to enforce this.
> >>
> >> Hmm, ok so that does not work (long cyclic dependency through the
> >> selection of ACPI_VIDEO).
> >>
> >> So I've now added this instead:
> >>
> >> 	# iosf_mbi needs to be builtin if we are builtin
> >> 	select IOSF_MBI if DRM_I915=y
> >
> > That's probably not going to help anyone since i915 is usually a module.
> 
> Right, that is fine, then either the IOSF_MBI symbols are available,
> or IOSF_MBI is disabled and we get the inline nops from the header.
> 
> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> realistic IMHO, but will get triggered by the random-config testing
> several contributors do and lead to an unresolved symbol error there.

Well, from the user POV anything with IOSF_MBI==n can be a problem.
So I'm not sure if we should allow that.

> 
> Hmm, thinking about this, this hunk actually belongs in 12/13 as that
> is the first patch to use iosf_mbi functions.
> 
> Regards,
> 
> Hans
> 
> 
> 
> >>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> -Spelling: P-Unit, PMIC
> >>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
> >>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
> >>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
> >>>>>  3 files changed, 24 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 5604701..13e5152 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -47,6 +47,7 @@
> >>>>>  #include <drm/drm_rect.h>
> >>>>>  #include <linux/dma_remapping.h>
> >>>>>  #include <linux/reservation.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  static bool is_mmio_work(struct intel_flip_work *work)
> >>>>>  {
> >>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>          cmd = 0;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      val &= ~DSPFREQGUAR_MASK;
> >>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>               50)) {
> >>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>      }
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>
> >>>>>      mutex_lock(&dev_priv->sb_lock);
> >>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
> >>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> >>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
> >>>>>               50)) {
> >>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>      }
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>
> >>>>>      intel_update_cdclk(dev_priv);
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> index 249623d..adff84a 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>> @@ -32,6 +32,7 @@
> >>>>>  #include "../../../platform/x86/intel_ips.h"
> >>>>>  #include <linux/module.h>
> >>>>>  #include <drm/drm_atomic_helper.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  /**
> >>>>>   * DOC: RC6
> >>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>>>      u32 val;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
> >>>>>      if (enable)
> >>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
> >>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
> >>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>  }
> >>>>>
> >>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>>>      u32 val;
> >>>>>
> >>>>>      mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>>
> >>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      if (enable)
> >>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
> >>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
> >>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>  }
> >>>>>
> >>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>>>
> >>>>>      if (IS_CHERRYVIEW(dev_priv)) {
> >>>>>          mutex_lock(&dev_priv->rps.hw_lock);
> >>>>> +        iosf_mbi_punit_acquire();
> >>>>>
> >>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
> >>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> >>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
> >>>>>          }
> >>>>>
> >>>>> +        iosf_mbi_punit_release();
> >>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
> >>>>>      }
> >>>>>
> >>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
> >>>>>
> >>>>>      if (val != dev_priv->rps.cur_freq) {
> >>>>> +        iosf_mbi_punit_acquire();
> >>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> >>>>> +        iosf_mbi_punit_release();
> >>>>>          if (!IS_CHERRYVIEW(dev_priv))
> >>>>>              gen6_set_rps_thresholds(dev_priv, val);
> >>>>>      }
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> index c0b7e95..e66bcc8 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >>>>> @@ -28,6 +28,7 @@
> >>>>>
> >>>>>  #include <linux/pm_runtime.h>
> >>>>>  #include <linux/vgaarb.h>
> >>>>> +#include <asm/iosf_mbi.h>
> >>>>>
> >>>>>  #include "i915_drv.h"
> >>>>>  #include "intel_drv.h"
> >>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>>>      if (COND)
> >>>>>          goto out;
> >>>>>
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
> >>>>>      ctrl &= ~mask;
> >>>>>      ctrl |= state;
> >>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
> >>>>>                state,
> >>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>> +
> >>>>>  #undef COND
> >>>>>
> >>>>>  out:
> >>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>>>      if (COND)
> >>>>>          goto out;
> >>>>>
> >>>>> +    iosf_mbi_punit_acquire();
> >>>>> +
> >>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>      ctrl &= ~DP_SSC_MASK(pipe);
> >>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> >>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
> >>>>>                state,
> >>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
> >>>>>
> >>>>> +    iosf_mbi_punit_release();
> >>>>> +
> >>>>>  #undef COND
> >>>>>
> >>>>>  out:
> >>>>> --
> >>>>> 2.9.3
> >>>>
> >
Hans de Goede Jan. 30, 2017, 3:27 p.m. UTC | #7
Hi,

On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>>         iosf_mbi_punit_acquire();
>>>>>
>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>                      50)) {
>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>         }
>>>>>         iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least. It also makes me wonder if
> just bumping the timeouts to some ridiculously high value would fix
> the problem as well.

I've already tried bumping the forcewake timeout from 50 to 250ms,
before writing the patch to just get forcewake_all before the pmic
bus access begins, that does not fix things, and since we busy wait
for this timeout from non-sleeping context 250ms already is way too
high.

>>>>>> + a comment would be nice why it's there.
>>>>>
>>>>> I will add comments to the acquire calls.
>>>>>
>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>> ifdefs?
>>>>>
>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>> driver is. I'll add:
>>>>>
>>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>
>>>>> To the i915 Kconfig to enforce this.
>>>>
>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>> selection of ACPI_VIDEO).
>>>>
>>>> So I've now added this instead:
>>>>
>>>> 	# iosf_mbi needs to be builtin if we are builtin
>>>> 	select IOSF_MBI if DRM_I915=y
>>>
>>> That's probably not going to help anyone since i915 is usually a module.
>>
>> Right, that is fine, then either the IOSF_MBI symbols are available,
>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>
>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>> realistic IMHO, but will get triggered by the random-config testing
>> several contributors do and lead to an unresolved symbol error there.
>
> Well, from the user POV anything with IOSF_MBI==n can be a problem.
> So I'm not sure if we should allow that.

So you're suggesting we just add an unconditional "select IOSF_MBI"
to the i915 Kconfig entry?

Regards,

Hans



>>>>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>>>>>>> ---
>>>>>>> Changes in v2:
>>>>>>> -Spelling: P-Unit, PMIC
>>>>>>> -Adjust for iosf_mbi_punit_lock/_unlock to _acquire/_release rename
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>>>>>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>>>>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>>>>>>  3 files changed, 24 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> index 5604701..13e5152 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>>> @@ -47,6 +47,7 @@
>>>>>>>  #include <drm/drm_rect.h>
>>>>>>>  #include <linux/dma_remapping.h>
>>>>>>>  #include <linux/reservation.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  static bool is_mmio_work(struct intel_flip_work *work)
>>>>>>>  {
>>>>>>> @@ -6421,6 +6422,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>          cmd = 0;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      val &= ~DSPFREQGUAR_MASK;
>>>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>> @@ -6430,6 +6433,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>               50)) {
>>>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>      }
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->sb_lock);
>>>>>>> @@ -6497,6 +6501,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>      cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      val &= ~DSPFREQGUAR_MASK_CHV;
>>>>>>>      val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>>>>>>> @@ -6506,6 +6511,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>>>>>>>               50)) {
>>>>>>>          DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>      }
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>
>>>>>>>      intel_update_cdclk(dev_priv);
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> index 249623d..adff84a 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>>> @@ -32,6 +32,7 @@
>>>>>>>  #include "../../../platform/x86/intel_ips.h"
>>>>>>>  #include <linux/module.h>
>>>>>>>  #include <drm/drm_atomic_helper.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  /**
>>>>>>>   * DOC: RC6
>>>>>>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>      u32 val;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>>>>>>      if (enable)
>>>>>>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>                FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>>>>>>          DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>      u32 val;
>>>>>>>
>>>>>>>      mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>      val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      if (enable)
>>>>>>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
>>>>>>>          val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>>>>>>      vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>>      mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>  }
>>>>>>>
>>>>>>> @@ -4553,6 +4558,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>>>
>>>>>>>      if (IS_CHERRYVIEW(dev_priv)) {
>>>>>>>          mutex_lock(&dev_priv->rps.hw_lock);
>>>>>>> +        iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>          val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>          if (val & DSP_MAXFIFO_PM5_ENABLE)
>>>>>>> @@ -4582,6 +4588,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>>>>>>                  wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>>>>>>          }
>>>>>>>
>>>>>>> +        iosf_mbi_punit_release();
>>>>>>>          mutex_unlock(&dev_priv->rps.hw_lock);
>>>>>>>      }
>>>>>>>
>>>>>>> @@ -4988,7 +4995,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>>>>>      I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>>>>>>
>>>>>>>      if (val != dev_priv->rps.cur_freq) {
>>>>>>> +        iosf_mbi_punit_acquire();
>>>>>>>          vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
>>>>>>> +        iosf_mbi_punit_release();
>>>>>>>          if (!IS_CHERRYVIEW(dev_priv))
>>>>>>>              gen6_set_rps_thresholds(dev_priv, val);
>>>>>>>      }
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> index c0b7e95..e66bcc8 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>>>>>> @@ -28,6 +28,7 @@
>>>>>>>
>>>>>>>  #include <linux/pm_runtime.h>
>>>>>>>  #include <linux/vgaarb.h>
>>>>>>> +#include <asm/iosf_mbi.h>
>>>>>>>
>>>>>>>  #include "i915_drv.h"
>>>>>>>  #include "intel_drv.h"
>>>>>>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>>>      if (COND)
>>>>>>>          goto out;
>>>>>>>
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>>>>>>      ctrl &= ~mask;
>>>>>>>      ctrl |= state;
>>>>>>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>>>>>>>                state,
>>>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>> +
>>>>>>>  #undef COND
>>>>>>>
>>>>>>>  out:
>>>>>>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>>>      if (COND)
>>>>>>>          goto out;
>>>>>>>
>>>>>>> +    iosf_mbi_punit_acquire();
>>>>>>> +
>>>>>>>      ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>      ctrl &= ~DP_SSC_MASK(pipe);
>>>>>>>      ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>>>>>>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>>>>>>>                state,
>>>>>>>                vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
>>>>>>>
>>>>>>> +    iosf_mbi_punit_release();
>>>>>>> +
>>>>>>>  #undef COND
>>>>>>>
>>>>>>>  out:
>>>>>>> --
>>>>>>> 2.9.3
>>>>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjälä Jan. 30, 2017, 3:38 p.m. UTC | #8
On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 30-01-17 16:11, Ville Syrjälä wrote:
> > On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 30-01-17 14:10, Ville Syrjälä wrote:
> >>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
> >>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
> >>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
> >>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
> >>>>>>> around P-Unit write accesses.
> >>>>>>
> >>>>>> Can't we just stuff the calls into the actual punit write function
> >>>>>> rather than sprinkling them all over the place?
> >>>>>
> >>>>> punit access is acquired across sections like this:
> >>>>>
> >>>>>         iosf_mbi_punit_acquire();
> >>>>>
> >>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >>>>>         val &= ~DSPFREQGUAR_MASK;
> >>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
> >>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
> >>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
> >>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
> >>>>>                      50)) {
> >>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
> >>>>>         }
> >>>>>         iosf_mbi_punit_release();
> >>>>>
> >>>>> Where we want to wait for the requested change to have taken
> >>>>> effect before releasing the punit.
> >>>
> >>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
> >>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
> >>> in set_rps() because that would slow things down too much. So I have to
> >>> wonder how much luck is needed to make this workaround really effective.
> >>
> >> So the history of this patch-set is that I wrote this patch before
> >> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
> >> active (patch 12/13). Since a lot of testing was done with this
> >> patch included in the patch-set and since it seemed a good idea
> >> regardless (given my experience with accessing the punit vs
> >> pmic bus accesses) I decided to leave it in.
> >>
> >> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
> >> actually fixed things for me. That is also why I made this the
> >> last patch in the set. I asked tagorereddy to test his system
> >> without this patch, but he did not get around to that.
> >>
> >> After all we do tell the punit to not touch the bus by acquiring
> >> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
> >> for RPS freq changes it honors that and properly waits. Maybe it
> >> honors that for all punit requests i915 does and the only real
> >> problem is the forcewake stuff ?
> >>
> >> I can try to drop this patch from my queue and run without it
> >> for a while and see if things don't regress. And also ask
> >> tagorereddy again to test his system that way.
> >>
> >> Does that (dropping this patch for now) sound like a good idea?
> >
> > More test results couldn't hurt at least. It also makes me wonder if
> > just bumping the timeouts to some ridiculously high value would fix
> > the problem as well.
> 
> I've already tried bumping the forcewake timeout from 50 to 250ms,
> before writing the patch to just get forcewake_all before the pmic
> bus access begins, that does not fix things,

And you bumped the i2c mutex timeout as well? Or does that fail somehow
gracefully if it can't get the mutex?

> and since we busy wait
> for this timeout from non-sleeping context 250ms already is way too
> high.

Sure, but I'm just trying to understand if the problem is simply caused
by proceeding with some hardware access without getting the i2c mutex.

> 
> >>>>>> + a comment would be nice why it's there.
> >>>>>
> >>>>> I will add comments to the acquire calls.
> >>>>>
> >>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
> >>>>>> ifdefs?
> >>>>>
> >>>>> No, the iosf_mbi header defines empty inline versions of
> >>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
> >>>>> this does mean that iosf_mbi must be builtin if the i915
> >>>>> driver is. I'll add:
> >>>>>
> >>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
> >>>>>
> >>>>> To the i915 Kconfig to enforce this.
> >>>>
> >>>> Hmm, ok so that does not work (long cyclic dependency through the
> >>>> selection of ACPI_VIDEO).
> >>>>
> >>>> So I've now added this instead:
> >>>>
> >>>> 	# iosf_mbi needs to be builtin if we are builtin
> >>>> 	select IOSF_MBI if DRM_I915=y
> >>>
> >>> That's probably not going to help anyone since i915 is usually a module.
> >>
> >> Right, that is fine, then either the IOSF_MBI symbols are available,
> >> or IOSF_MBI is disabled and we get the inline nops from the header.
> >>
> >> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
> >> realistic IMHO, but will get triggered by the random-config testing
> >> several contributors do and lead to an unresolved symbol error there.
> >
> > Well, from the user POV anything with IOSF_MBI==n can be a problem.
> > So I'm not sure if we should allow that.
> 
> So you're suggesting we just add an unconditional "select IOSF_MBI"
> to the i915 Kconfig entry?

Yeah, that should at least cut down the number of people accidentally
misconfiguring their kernels and hitting this problem in the future.
Hans de Goede Jan. 30, 2017, 4:33 p.m. UTC | #9
Hi,

On 30-01-17 16:38, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:27:58PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 16:11, Ville Syrjälä wrote:
>>> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>>>> around P-Unit write accesses.
>>>>>>>>
>>>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>>>> rather than sprinkling them all over the place?
>>>>>>>
>>>>>>> punit access is acquired across sections like this:
>>>>>>>
>>>>>>>         iosf_mbi_punit_acquire();
>>>>>>>
>>>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>>>                      50)) {
>>>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>>>         }
>>>>>>>         iosf_mbi_punit_release();
>>>>>>>
>>>>>>> Where we want to wait for the requested change to have taken
>>>>>>> effect before releasing the punit.
>>>>>
>>>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>>>> in set_rps() because that would slow things down too much. So I have to
>>>>> wonder how much luck is needed to make this workaround really effective.
>>>>
>>>> So the history of this patch-set is that I wrote this patch before
>>>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>>>> active (patch 12/13). Since a lot of testing was done with this
>>>> patch included in the patch-set and since it seemed a good idea
>>>> regardless (given my experience with accessing the punit vs
>>>> pmic bus accesses) I decided to leave it in.
>>>>
>>>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>>>> actually fixed things for me. That is also why I made this the
>>>> last patch in the set. I asked tagorereddy to test his system
>>>> without this patch, but he did not get around to that.
>>>>
>>>> After all we do tell the punit to not touch the bus by acquiring
>>>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>>>> for RPS freq changes it honors that and properly waits. Maybe it
>>>> honors that for all punit requests i915 does and the only real
>>>> problem is the forcewake stuff ?
>>>>
>>>> I can try to drop this patch from my queue and run without it
>>>> for a while and see if things don't regress. And also ask
>>>> tagorereddy again to test his system that way.
>>>>
>>>> Does that (dropping this patch for now) sound like a good idea?
>>>
>>> More test results couldn't hurt at least. It also makes me wonder if
>>> just bumping the timeouts to some ridiculously high value would fix
>>> the problem as well.
>>
>> I've already tried bumping the forcewake timeout from 50 to 250ms,
>> before writing the patch to just get forcewake_all before the pmic
>> bus access begins, that does not fix things,
>
> And you bumped the i2c mutex timeout as well? Or does that fail somehow
> gracefully if it can't get the mutex?

It will fail the i2c transfer with -ETIMEOUT, which will make the driver
report an error instead of e.g. the battery level, but it should not
affect the forcewake calls and those still failed with the large
timeout. So yes basically the i2c mutex fails gracefully.

>
>> and since we busy wait
>> for this timeout from non-sleeping context 250ms already is way too
>> high.
>
> Sure, but I'm just trying to understand if the problem is simply caused
> by proceeding with some hardware access without getting the i2c mutex.

Understood.

>>>>>>>> + a comment would be nice why it's there.
>>>>>>>
>>>>>>> I will add comments to the acquire calls.
>>>>>>>
>>>>>>>> Do we need a kconfig select/depends on the iosf_mbi thing? Or some
>>>>>>>> ifdefs?
>>>>>>>
>>>>>>> No, the iosf_mbi header defines empty inline versions of
>>>>>>> iosf_mbi_punit_acquire / _release if IOSF_MBI is disabled,
>>>>>>> this does mean that iosf_mbi must be builtin if the i915
>>>>>>> driver is. I'll add:
>>>>>>>
>>>>>>>     depends on DRM_I915=IOSF_MBI || IOSF_MBI=y
>>>>>>>
>>>>>>> To the i915 Kconfig to enforce this.
>>>>>>
>>>>>> Hmm, ok so that does not work (long cyclic dependency through the
>>>>>> selection of ACPI_VIDEO).
>>>>>>
>>>>>> So I've now added this instead:
>>>>>>
>>>>>> 	# iosf_mbi needs to be builtin if we are builtin
>>>>>> 	select IOSF_MBI if DRM_I915=y
>>>>>
>>>>> That's probably not going to help anyone since i915 is usually a module.
>>>>
>>>> Right, that is fine, then either the IOSF_MBI symbols are available,
>>>> or IOSF_MBI is disabled and we get the inline nops from the header.
>>>>
>>>> The problem scenario is DRM_I915=y and IOSF_MBI=m, which is not very
>>>> realistic IMHO, but will get triggered by the random-config testing
>>>> several contributors do and lead to an unresolved symbol error there.
>>>
>>> Well, from the user POV anything with IOSF_MBI==n can be a problem.
>>> So I'm not sure if we should allow that.
>>
>> So you're suggesting we just add an unconditional "select IOSF_MBI"
>> to the i915 Kconfig entry?
>
> Yeah, that should at least cut down the number of people accidentally
> misconfiguring their kernels and hitting this problem in the future.

Ok.

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede Feb. 10, 2017, 10:19 a.m. UTC | #10
Hi,

On 30-01-17 16:11, Ville Syrjälä wrote:
> On Mon, Jan 30, 2017 at 04:02:19PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 30-01-17 14:10, Ville Syrjälä wrote:
>>> On Sat, Jan 28, 2017 at 06:18:45PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 01/28/2017 05:25 PM, Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 01/27/2017 02:51 PM, Ville Syrjälä wrote:
>>>>>> On Mon, Jan 23, 2017 at 10:09:58PM +0100, Hans de Goede wrote:
>>>>>>> Make sure the P-Unit or the PMIC i2c bus is not in use when we send a
>>>>>>> request to the P-Unit by calling iosf_mbi_punit_acquire() / _release()
>>>>>>> around P-Unit write accesses.
>>>>>>
>>>>>> Can't we just stuff the calls into the actual punit write function
>>>>>> rather than sprinkling them all over the place?
>>>>>
>>>>> punit access is acquired across sections like this:
>>>>>
>>>>>         iosf_mbi_punit_acquire();
>>>>>
>>>>>         val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>>>>         val &= ~DSPFREQGUAR_MASK;
>>>>>         val |= (cmd << DSPFREQGUAR_SHIFT);
>>>>>         vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>>>>         if (wait_for((vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) &
>>>>>                       DSPFREQSTAT_MASK) == (cmd << DSPFREQSTAT_SHIFT),
>>>>>                      50)) {
>>>>>                 DRM_ERROR("timed out waiting for CDclk change\n");
>>>>>         }
>>>>>         iosf_mbi_punit_release();
>>>>>
>>>>> Where we want to wait for the requested change to have taken
>>>>> effect before releasing the punit.
>>>
>>> Hmm. That's somewhat unfortunate. It also highlights a problem with the
>>> patch wrt. RPS. We don't wait for the GPU to actually change frequencies
>>> in set_rps() because that would slow things down too much. So I have to
>>> wonder how much luck is needed to make this workaround really effective.
>>
>> So the history of this patch-set is that I wrote this patch before
>> writing the patch to get FORCEWAKE_ALL before the pmic bus becomes
>> active (patch 12/13). Since a lot of testing was done with this
>> patch included in the patch-set and since it seemed a good idea
>> regardless (given my experience with accessing the punit vs
>> pmic bus accesses) I decided to leave it in.
>>
>> Possibly just the patch to get FORCEWAKE_ALL is enough, that one
>> actually fixed things for me. That is also why I made this the
>> last patch in the set. I asked tagorereddy to test his system
>> without this patch, but he did not get around to that.
>>
>> After all we do tell the punit to not touch the bus by acquiring
>> the pmic bus semaphore from i2c-desigware-baytrail.c, so maybe
>> for RPS freq changes it honors that and properly waits. Maybe it
>> honors that for all punit requests i915 does and the only real
>> problem is the forcewake stuff ?
>>
>> I can try to drop this patch from my queue and run without it
>> for a while and see if things don't regress. And also ask
>> tagorereddy again to test his system that way.
>>
>> Does that (dropping this patch for now) sound like a good idea?
>
> More test results couldn't hurt at least.

Ok, I've done a whole bunch of suspend + resume cycles on my cht
tablet with this patch dropped and things still work fine
(where as without the first 12 patches of this patch-set that
  was a guarenteed way to get a forcewake timeout followed by
  a lockup).

So it indeed seems this test is not necessary. I'll send a v3
with that patch dropped, as well as your comments for
patches 11 and 12 being addressed.

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5604701..13e5152 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -47,6 +47,7 @@ 
 #include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 #include <linux/reservation.h>
+#include <asm/iosf_mbi.h>
 
 static bool is_mmio_work(struct intel_flip_work *work)
 {
@@ -6421,6 +6422,8 @@  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		cmd = 0;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK;
 	val |= (cmd << DSPFREQGUAR_SHIFT);
@@ -6430,6 +6433,7 @@  static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	mutex_lock(&dev_priv->sb_lock);
@@ -6497,6 +6501,7 @@  static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK_CHV;
 	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
@@ -6506,6 +6511,7 @@  static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 249623d..adff84a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -32,6 +32,7 @@ 
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
 #include <drm/drm_atomic_helper.h>
+#include <asm/iosf_mbi.h>
 
 /**
  * DOC: RC6
@@ -276,6 +277,7 @@  static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
 	if (enable)
@@ -290,6 +292,7 @@  static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
 		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
 
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -298,6 +301,7 @@  static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_acquire();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	if (enable)
@@ -306,6 +310,7 @@  static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 		val &= ~DSP_MAXFIFO_PM5_ENABLE;
 	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
 
+	iosf_mbi_punit_release();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -4553,6 +4558,7 @@  void vlv_wm_get_hw_state(struct drm_device *dev)
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
+		iosf_mbi_punit_acquire();
 
 		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 		if (val & DSP_MAXFIFO_PM5_ENABLE)
@@ -4582,6 +4588,7 @@  void vlv_wm_get_hw_state(struct drm_device *dev)
 				wm->level = VLV_WM_LEVEL_DDR_DVFS;
 		}
 
+		iosf_mbi_punit_release();
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	}
 
@@ -4988,7 +4995,9 @@  static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
 	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
 
 	if (val != dev_priv->rps.cur_freq) {
+		iosf_mbi_punit_acquire();
 		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+		iosf_mbi_punit_release();
 		if (!IS_CHERRYVIEW(dev_priv))
 			gen6_set_rps_thresholds(dev_priv, val);
 	}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c0b7e95..e66bcc8 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -28,6 +28,7 @@ 
 
 #include <linux/pm_runtime.h>
 #include <linux/vgaarb.h>
+#include <asm/iosf_mbi.h>
 
 #include "i915_drv.h"
 #include "intel_drv.h"
@@ -1027,6 +1028,8 @@  static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_acquire();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
 	ctrl &= ~mask;
 	ctrl |= state;
@@ -1037,6 +1040,8 @@  static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
 
+	iosf_mbi_punit_release();
+
 #undef COND
 
 out:
@@ -1643,6 +1648,8 @@  static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_acquire();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	ctrl &= ~DP_SSC_MASK(pipe);
 	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
@@ -1653,6 +1660,8 @@  static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
 
+	iosf_mbi_punit_release();
+
 #undef COND
 
 out: