* [PATCH 0/3] drm/i915: BDW watermark fixes
@ 2014-01-07 14:14 ville.syrjala
2014-01-07 14:14 ` [PATCH 1/3] drm/i915: Fix watermark code for BDW ville.syrjala
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: ville.syrjala @ 2014-01-07 14:14 UTC (permalink / raw)
To: intel-gfx
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Seems I was a bit sloppy when I sent out the last set of ILK/SNB/IVB watermark
patches and I forgot to update them for BDW. And I was even sloppier before
that since I forgot to actually hook up the watermark vfuncs on BDW. This
series should fix it all up.
I'm not sure what the 3.13 status is at the moment, but if there's still time
to get something in, then a slightly modified version of patch 2
(just undo the haswell->ilk rename) should be what we want there. If there's
no chance to get it into 3.13 the I guess we should just slap a cc stable on
it.
Ville Syrjälä (3):
drm/i915: Fix watermark code for BDW
drm/i915: Enable watermarks for BDW
drm/i915: Simplify watermark/init_clock_gating setup
drivers/gpu/drm/i915/intel_pm.c | 81 ++++++++++++-----------------------------
1 file changed, 23 insertions(+), 58 deletions(-)
--
1.8.3.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] drm/i915: Fix watermark code for BDW 2014-01-07 14:14 [PATCH 0/3] drm/i915: BDW watermark fixes ville.syrjala @ 2014-01-07 14:14 ` ville.syrjala 2014-01-07 14:14 ` [PATCH 2/3] drm/i915: Enable watermarks " ville.syrjala 2014-01-07 14:14 ` [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup ville.syrjala 2 siblings, 0 replies; 8+ messages in thread From: ville.syrjala @ 2014-01-07 14:14 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Looks like I forgot to update the ILK/SNB/IVB watermark patches to deal with BDW. Add the relevant BDW checks to make sure we take the HSW codepaths on BDW as well. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 469170c..5637183 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2013,7 +2013,7 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[5]) { struct drm_i915_private *dev_priv = dev->dev_private; - if (IS_HASWELL(dev)) { + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { uint64_t sskpd = I915_READ64(MCH_SSKPD); wm[0] = (sskpd >> 56) & 0xFF; @@ -2061,7 +2061,7 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5]) static int ilk_wm_max_level(const struct drm_device *dev) { /* how many WM levels are we expecting */ - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) return 4; else if (INTEL_INFO(dev)->gen >= 6) return 3; @@ -2180,7 +2180,7 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc, ilk_compute_wm_level(dev_priv, level, params, &pipe_wm->wm[level]); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc); /* At least LP0 must be valid */ @@ -2275,7 +2275,7 @@ static unsigned int ilk_wm_lp_latency(struct drm_device *dev, int level) { struct drm_i915_private *dev_priv = dev->dev_private; - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) return 2 * level; else return dev_priv->wm.pri_latency[level]; @@ -2490,7 +2490,7 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv, I915_WRITE(PIPE_WM_LINETIME(PIPE_C), results->wm_linetime[2]); if (dirty & WM_DIRTY_DDB) { - if (IS_HASWELL(dev)) { + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) { val = I915_READ(WM_MISC); if (results->partitioning == INTEL_DDB_PART_1_2) val &= ~WM_MISC_DATA_PARTITION_5_6; @@ -2629,7 +2629,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc) }; hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe)); if (intel_crtc_active(crtc)) { @@ -2676,7 +2676,7 @@ void ilk_wm_get_hw_state(struct drm_device *dev) hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB); hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) hw->partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ? INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2; else if (IS_IVYBRIDGE(dev)) -- 1.8.3.2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] drm/i915: Enable watermarks for BDW 2014-01-07 14:14 [PATCH 0/3] drm/i915: BDW watermark fixes ville.syrjala 2014-01-07 14:14 ` [PATCH 1/3] drm/i915: Fix watermark code for BDW ville.syrjala @ 2014-01-07 14:14 ` ville.syrjala 2014-01-07 14:14 ` [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup ville.syrjala 2 siblings, 0 replies; 8+ messages in thread From: ville.syrjala @ 2014-01-07 14:14 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> We forgot to intialize the watermark vfuncs for BDW, and hence the watermarks were never updated. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 5637183..04e1e29 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5628,6 +5628,17 @@ void intel_init_pm(struct drm_device *dev) } dev_priv->display.init_clock_gating = haswell_init_clock_gating; } else if (INTEL_INFO(dev)->gen == 8) { + if (dev_priv->wm.pri_latency[0] && + dev_priv->wm.spr_latency[0] && + dev_priv->wm.cur_latency[0]) { + dev_priv->display.update_wm = ilk_update_wm; + dev_priv->display.update_sprite_wm = + ilk_update_sprite_wm; + } else { + DRM_DEBUG_KMS("Failed to read display plane latency. " + "Disable CxSR\n"); + dev_priv->display.update_wm = NULL; + } dev_priv->display.init_clock_gating = gen8_init_clock_gating; } else dev_priv->display.update_wm = NULL; -- 1.8.3.2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup 2014-01-07 14:14 [PATCH 0/3] drm/i915: BDW watermark fixes ville.syrjala 2014-01-07 14:14 ` [PATCH 1/3] drm/i915: Fix watermark code for BDW ville.syrjala 2014-01-07 14:14 ` [PATCH 2/3] drm/i915: Enable watermarks " ville.syrjala @ 2014-01-07 14:14 ` ville.syrjala 2014-01-07 19:34 ` Paulo Zanoni 2 siblings, 1 reply; 8+ messages in thread From: ville.syrjala @ 2014-01-07 14:14 UTC (permalink / raw) To: intel-gfx From: Ville Syrjälä <ville.syrjala@linux.intel.com> Avoid duplicating the same piece of code several times by separating the watemark vfunc setup from the init_clock_gating vfunc setup on PCH platforms. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++-------------------------------- 1 file changed, 16 insertions(+), 62 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 04e1e29..a177a93 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) if (HAS_PCH_SPLIT(dev)) { intel_setup_wm_latency(dev); - if (IS_GEN5(dev)) { - if (dev_priv->wm.pri_latency[1] && - dev_priv->wm.spr_latency[1] && - dev_priv->wm.cur_latency[1]) { - dev_priv->display.update_wm = ilk_update_wm; - dev_priv->display.update_sprite_wm = - ilk_update_sprite_wm; - } else { - DRM_DEBUG_KMS("Failed to get proper latency. " - "Disable CxSR\n"); - dev_priv->display.update_wm = NULL; - } + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) || + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) { + dev_priv->display.update_wm = ilk_update_wm; + dev_priv->display.update_sprite_wm = ilk_update_sprite_wm; + } else { + DRM_DEBUG_KMS("Failed to read display plane latency. " + "Disable CxSR\n"); + } + + if (IS_GEN5(dev)) dev_priv->display.init_clock_gating = ironlake_init_clock_gating; - } else if (IS_GEN6(dev)) { - if (dev_priv->wm.pri_latency[0] && - dev_priv->wm.spr_latency[0] && - dev_priv->wm.cur_latency[0]) { - dev_priv->display.update_wm = ilk_update_wm; - dev_priv->display.update_sprite_wm = - ilk_update_sprite_wm; - } else { - DRM_DEBUG_KMS("Failed to read display plane latency. " - "Disable CxSR\n"); - dev_priv->display.update_wm = NULL; - } + else if (IS_GEN6(dev)) dev_priv->display.init_clock_gating = gen6_init_clock_gating; - } else if (IS_IVYBRIDGE(dev)) { - if (dev_priv->wm.pri_latency[0] && - dev_priv->wm.spr_latency[0] && - dev_priv->wm.cur_latency[0]) { - dev_priv->display.update_wm = ilk_update_wm; - dev_priv->display.update_sprite_wm = - ilk_update_sprite_wm; - } else { - DRM_DEBUG_KMS("Failed to read display plane latency. " - "Disable CxSR\n"); - dev_priv->display.update_wm = NULL; - } + else if (IS_IVYBRIDGE(dev)) dev_priv->display.init_clock_gating = ivybridge_init_clock_gating; - } else if (IS_HASWELL(dev)) { - if (dev_priv->wm.pri_latency[0] && - dev_priv->wm.spr_latency[0] && - dev_priv->wm.cur_latency[0]) { - dev_priv->display.update_wm = ilk_update_wm; - dev_priv->display.update_sprite_wm = - ilk_update_sprite_wm; - } else { - DRM_DEBUG_KMS("Failed to read display plane latency. " - "Disable CxSR\n"); - dev_priv->display.update_wm = NULL; - } + else if (IS_HASWELL(dev)) dev_priv->display.init_clock_gating = haswell_init_clock_gating; - } else if (INTEL_INFO(dev)->gen == 8) { - if (dev_priv->wm.pri_latency[0] && - dev_priv->wm.spr_latency[0] && - dev_priv->wm.cur_latency[0]) { - dev_priv->display.update_wm = ilk_update_wm; - dev_priv->display.update_sprite_wm = - ilk_update_sprite_wm; - } else { - DRM_DEBUG_KMS("Failed to read display plane latency. " - "Disable CxSR\n"); - dev_priv->display.update_wm = NULL; - } + else if (INTEL_INFO(dev)->gen == 8) dev_priv->display.init_clock_gating = gen8_init_clock_gating; - } else - dev_priv->display.update_wm = NULL; } else if (IS_VALLEYVIEW(dev)) { dev_priv->display.update_wm = valleyview_update_wm; dev_priv->display.init_clock_gating = -- 1.8.3.2 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup 2014-01-07 14:14 ` [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup ville.syrjala @ 2014-01-07 19:34 ` Paulo Zanoni 2014-01-07 19:53 ` Ville Syrjälä 2014-01-07 21:47 ` Daniel Vetter 0 siblings, 2 replies; 8+ messages in thread From: Paulo Zanoni @ 2014-01-07 19:34 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Intel Graphics Development 2014/1/7 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Avoid duplicating the same piece of code several times by separating > the watemark vfunc setup from the init_clock_gating vfunc setup on PCH > platforms. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++-------------------------------- > 1 file changed, 16 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 04e1e29..a177a93 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) > if (HAS_PCH_SPLIT(dev)) { > intel_setup_wm_latency(dev); > > - if (IS_GEN5(dev)) { > - if (dev_priv->wm.pri_latency[1] && > - dev_priv->wm.spr_latency[1] && > - dev_priv->wm.cur_latency[1]) { > - dev_priv->display.update_wm = ilk_update_wm; > - dev_priv->display.update_sprite_wm = > - ilk_update_sprite_wm; > - } else { > - DRM_DEBUG_KMS("Failed to get proper latency. " > - "Disable CxSR\n"); > - dev_priv->display.update_wm = NULL; > - } > + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && > + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) || > + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && > + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) { > + dev_priv->display.update_wm = ilk_update_wm; > + dev_priv->display.update_sprite_wm = ilk_update_sprite_wm; My tiny little brain doesn't remember why on gen5 we check for latency[1] instead of latency[0]. I know this is not the goal if your patch, but maybe a follow-up patch adding a little comment would be nice :) For the 3 patches on the series: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + } else { > + DRM_DEBUG_KMS("Failed to read display plane latency. " > + "Disable CxSR\n"); > + } > + > + if (IS_GEN5(dev)) > dev_priv->display.init_clock_gating = ironlake_init_clock_gating; > - } else if (IS_GEN6(dev)) { > - if (dev_priv->wm.pri_latency[0] && > - dev_priv->wm.spr_latency[0] && > - dev_priv->wm.cur_latency[0]) { > - dev_priv->display.update_wm = ilk_update_wm; > - dev_priv->display.update_sprite_wm = > - ilk_update_sprite_wm; > - } else { > - DRM_DEBUG_KMS("Failed to read display plane latency. " > - "Disable CxSR\n"); > - dev_priv->display.update_wm = NULL; > - } > + else if (IS_GEN6(dev)) > dev_priv->display.init_clock_gating = gen6_init_clock_gating; > - } else if (IS_IVYBRIDGE(dev)) { > - if (dev_priv->wm.pri_latency[0] && > - dev_priv->wm.spr_latency[0] && > - dev_priv->wm.cur_latency[0]) { > - dev_priv->display.update_wm = ilk_update_wm; > - dev_priv->display.update_sprite_wm = > - ilk_update_sprite_wm; > - } else { > - DRM_DEBUG_KMS("Failed to read display plane latency. " > - "Disable CxSR\n"); > - dev_priv->display.update_wm = NULL; > - } > + else if (IS_IVYBRIDGE(dev)) > dev_priv->display.init_clock_gating = ivybridge_init_clock_gating; > - } else if (IS_HASWELL(dev)) { > - if (dev_priv->wm.pri_latency[0] && > - dev_priv->wm.spr_latency[0] && > - dev_priv->wm.cur_latency[0]) { > - dev_priv->display.update_wm = ilk_update_wm; > - dev_priv->display.update_sprite_wm = > - ilk_update_sprite_wm; > - } else { > - DRM_DEBUG_KMS("Failed to read display plane latency. " > - "Disable CxSR\n"); > - dev_priv->display.update_wm = NULL; > - } > + else if (IS_HASWELL(dev)) > dev_priv->display.init_clock_gating = haswell_init_clock_gating; > - } else if (INTEL_INFO(dev)->gen == 8) { > - if (dev_priv->wm.pri_latency[0] && > - dev_priv->wm.spr_latency[0] && > - dev_priv->wm.cur_latency[0]) { > - dev_priv->display.update_wm = ilk_update_wm; > - dev_priv->display.update_sprite_wm = > - ilk_update_sprite_wm; > - } else { > - DRM_DEBUG_KMS("Failed to read display plane latency. " > - "Disable CxSR\n"); > - dev_priv->display.update_wm = NULL; > - } > + else if (INTEL_INFO(dev)->gen == 8) > dev_priv->display.init_clock_gating = gen8_init_clock_gating; > - } else > - dev_priv->display.update_wm = NULL; > } else if (IS_VALLEYVIEW(dev)) { > dev_priv->display.update_wm = valleyview_update_wm; > dev_priv->display.init_clock_gating = > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup 2014-01-07 19:34 ` Paulo Zanoni @ 2014-01-07 19:53 ` Ville Syrjälä 2014-01-07 21:47 ` Daniel Vetter 1 sibling, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2014-01-07 19:53 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development On Tue, Jan 07, 2014 at 05:34:40PM -0200, Paulo Zanoni wrote: > 2014/1/7 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Avoid duplicating the same piece of code several times by separating > > the watemark vfunc setup from the init_clock_gating vfunc setup on PCH > > platforms. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++-------------------------------- > > 1 file changed, 16 insertions(+), 62 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 04e1e29..a177a93 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) > > if (HAS_PCH_SPLIT(dev)) { > > intel_setup_wm_latency(dev); > > > > - if (IS_GEN5(dev)) { > > - if (dev_priv->wm.pri_latency[1] && > > - dev_priv->wm.spr_latency[1] && > > - dev_priv->wm.cur_latency[1]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to get proper latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && > > + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) || > > + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && > > + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) { > > + dev_priv->display.update_wm = ilk_update_wm; > > + dev_priv->display.update_sprite_wm = ilk_update_sprite_wm; > > My tiny little brain doesn't remember why on gen5 we check for > latency[1] instead of latency[0]. I know this is not the goal if your > patch, but maybe a follow-up patch adding a little comment would be > nice :) I did it that way since the latency[0] values are hardcoded on ILK. So we can't tell from latency[0] values whether the BIOS programmed the latency values or not. I had an idea to kill these checks, and possibly just dynamically calculate the max level for which we have latency data. But that's for another patch. > > For the 3 patches on the series: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > + } else { > > + DRM_DEBUG_KMS("Failed to read display plane latency. " > > + "Disable CxSR\n"); > > + } > > + > > + if (IS_GEN5(dev)) > > dev_priv->display.init_clock_gating = ironlake_init_clock_gating; > > - } else if (IS_GEN6(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display plane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + else if (IS_GEN6(dev)) > > dev_priv->display.init_clock_gating = gen6_init_clock_gating; > > - } else if (IS_IVYBRIDGE(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display plane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + else if (IS_IVYBRIDGE(dev)) > > dev_priv->display.init_clock_gating = ivybridge_init_clock_gating; > > - } else if (IS_HASWELL(dev)) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display plane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + else if (IS_HASWELL(dev)) > > dev_priv->display.init_clock_gating = haswell_init_clock_gating; > > - } else if (INTEL_INFO(dev)->gen == 8) { > > - if (dev_priv->wm.pri_latency[0] && > > - dev_priv->wm.spr_latency[0] && > > - dev_priv->wm.cur_latency[0]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to read display plane latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + else if (INTEL_INFO(dev)->gen == 8) > > dev_priv->display.init_clock_gating = gen8_init_clock_gating; > > - } else > > - dev_priv->display.update_wm = NULL; > > } else if (IS_VALLEYVIEW(dev)) { > > dev_priv->display.update_wm = valleyview_update_wm; > > dev_priv->display.init_clock_gating = > > -- > > 1.8.3.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup 2014-01-07 19:34 ` Paulo Zanoni 2014-01-07 19:53 ` Ville Syrjälä @ 2014-01-07 21:47 ` Daniel Vetter 2014-01-08 9:17 ` Ville Syrjälä 1 sibling, 1 reply; 8+ messages in thread From: Daniel Vetter @ 2014-01-07 21:47 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development On Tue, Jan 07, 2014 at 05:34:40PM -0200, Paulo Zanoni wrote: > 2014/1/7 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Avoid duplicating the same piece of code several times by separating > > the watemark vfunc setup from the init_clock_gating vfunc setup on PCH > > platforms. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++-------------------------------- > > 1 file changed, 16 insertions(+), 62 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 04e1e29..a177a93 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) > > if (HAS_PCH_SPLIT(dev)) { > > intel_setup_wm_latency(dev); > > > > - if (IS_GEN5(dev)) { > > - if (dev_priv->wm.pri_latency[1] && > > - dev_priv->wm.spr_latency[1] && > > - dev_priv->wm.cur_latency[1]) { > > - dev_priv->display.update_wm = ilk_update_wm; > > - dev_priv->display.update_sprite_wm = > > - ilk_update_sprite_wm; > > - } else { > > - DRM_DEBUG_KMS("Failed to get proper latency. " > > - "Disable CxSR\n"); > > - dev_priv->display.update_wm = NULL; > > - } > > + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && > > + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) || > > + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && > > + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) { > > + dev_priv->display.update_wm = ilk_update_wm; > > + dev_priv->display.update_sprite_wm = ilk_update_sprite_wm; > > My tiny little brain doesn't remember why on gen5 we check for > latency[1] instead of latency[0]. I know this is not the goal if your > patch, but maybe a follow-up patch adding a little comment would be > nice :) > > For the 3 patches on the series: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Merged to dinq, thanks for patches and review. And I'll use this occasion to repeat my plea to make our underrun reporting much louder - I fear that we'll silently break the watermark code soonish again if we don't ramp up automated testing. And I'd hate to see all the hard effort from you two wasted like that. btw for the funny wm.*_latency checking logic Paulo was confused about: Would it make sense for intel_setup_wm_latency to return a bool indicating whether doing the wm latency setup has succeeded or not? Also I wonder w a bit whether this is a real worl failure mode or whether we should just make the debug message a bit louder ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup 2014-01-07 21:47 ` Daniel Vetter @ 2014-01-08 9:17 ` Ville Syrjälä 0 siblings, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2014-01-08 9:17 UTC (permalink / raw) To: Daniel Vetter; +Cc: Intel Graphics Development On Tue, Jan 07, 2014 at 10:47:27PM +0100, Daniel Vetter wrote: > On Tue, Jan 07, 2014 at 05:34:40PM -0200, Paulo Zanoni wrote: > > 2014/1/7 <ville.syrjala@linux.intel.com>: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Avoid duplicating the same piece of code several times by separating > > > the watemark vfunc setup from the init_clock_gating vfunc setup on PCH > > > platforms. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 78 +++++++++-------------------------------- > > > 1 file changed, 16 insertions(+), 62 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 04e1e29..a177a93 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -5575,73 +5575,27 @@ void intel_init_pm(struct drm_device *dev) > > > if (HAS_PCH_SPLIT(dev)) { > > > intel_setup_wm_latency(dev); > > > > > > - if (IS_GEN5(dev)) { > > > - if (dev_priv->wm.pri_latency[1] && > > > - dev_priv->wm.spr_latency[1] && > > > - dev_priv->wm.cur_latency[1]) { > > > - dev_priv->display.update_wm = ilk_update_wm; > > > - dev_priv->display.update_sprite_wm = > > > - ilk_update_sprite_wm; > > > - } else { > > > - DRM_DEBUG_KMS("Failed to get proper latency. " > > > - "Disable CxSR\n"); > > > - dev_priv->display.update_wm = NULL; > > > - } > > > + if ((IS_GEN5(dev) && dev_priv->wm.pri_latency[1] && > > > + dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) || > > > + (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] && > > > + dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) { > > > + dev_priv->display.update_wm = ilk_update_wm; > > > + dev_priv->display.update_sprite_wm = ilk_update_sprite_wm; > > > > My tiny little brain doesn't remember why on gen5 we check for > > latency[1] instead of latency[0]. I know this is not the goal if your > > patch, but maybe a follow-up patch adding a little comment would be > > nice :) > > > > For the 3 patches on the series: > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Merged to dinq, thanks for patches and review. And I'll use this occasion > to repeat my plea to make our underrun reporting much louder - I fear that > we'll silently break the watermark code soonish again if we don't ramp up > automated testing. And I'd hate to see all the hard effort from you two > wasted like that. > > btw for the funny wm.*_latency checking logic Paulo was confused about: > Would it make sense for intel_setup_wm_latency to return a bool indicating > whether doing the wm latency setup has succeeded or not? Yeah, that could also work. > Also I wonder w > a bit whether this is a real worl failure mode or whether we should just > make the debug message a bit louder ... I don't think it should happen unless the BIOS is broken. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-01-08 9:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-07 14:14 [PATCH 0/3] drm/i915: BDW watermark fixes ville.syrjala 2014-01-07 14:14 ` [PATCH 1/3] drm/i915: Fix watermark code for BDW ville.syrjala 2014-01-07 14:14 ` [PATCH 2/3] drm/i915: Enable watermarks " ville.syrjala 2014-01-07 14:14 ` [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup ville.syrjala 2014-01-07 19:34 ` Paulo Zanoni 2014-01-07 19:53 ` Ville Syrjälä 2014-01-07 21:47 ` Daniel Vetter 2014-01-08 9:17 ` Ville Syrjälä
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox