Re: [PATCH 3/3] drm/i915: Simplify watermark/init_clock_gating setup

[Daniel Vetter <daniel@ffwll.ch>]

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