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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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