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@xxxxxxxxxx> > >>> Tested-by: tagorereddy <tagore.chandan@xxxxxxxxx> > >>> --- > >>> 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 > >> -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel