[Intel-gfx] [PATCH 14/28] drm/i915: disable watermarks on VLV, pondicherry takes care of this

[Jesse Barnes]

On Wed, 6 Mar 2013 20:32:22 +0100
Daniel Vetter <daniel at ffwll.ch> wrote:

> On Wed, Mar 06, 2013 at 09:19:27PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 06, 2013 at 08:14:54PM +0100, Daniel Vetter wrote:
> > > On Wed, Mar 06, 2013 at 09:09:06PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Mar 06, 2013 at 07:56:33PM +0100, Daniel Vetter wrote:
> > > > > On Fri, Mar 01, 2013 at 01:14:17PM -0800, Jesse Barnes wrote:
> > > > > > For current usage, not needed.
> > > > > > 
> > > > > > Signed-off-by: Vijay Purushothaman <vijay.a.purushothaman at intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_pm.c |    4 +++-
> > > > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 7de8cec..b8f5a17 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -4211,7 +4211,9 @@ void intel_init_pm(struct drm_device *dev)
> > > > > >  		} else
> > > > > >  			dev_priv->display.update_wm = NULL;
> > > > > >  	} else if (IS_VALLEYVIEW(dev)) {
> > > > > > -		dev_priv->display.update_wm = valleyview_update_wm;
> > > > > > +//		dev_priv->display.update_wm = valleyview_update_wm;
> > > > > > +		dev_priv->display.update_wm = NULL;
> > > > > > +//		dev_priv->display.update_sprite_wm = valleyview_update_sprite_wm;
> > > > > 
> > > > > Either kill this all (it's kzalloced so no need for NULL assignments) or
> > > > > add a giant comment explaining what's going on. Adding commented-out code
> > > > > without comment is strange ...
> > > > 
> > > > The whole premise that pondicherry magically handles things is a bit
> > > > weird too. I suppose all it really means is that the default WMs were
> > > > high enough to avoid underruns for the guys who tested this.
> > > > 
> > > > Actually wasn't there a comment on an earlier version of this patch
> > > > that there was a div by zero or something, and that's the real reason
> > > > for this patch?
> > > 
> > > Fyi the patch which fixed div-by-zero on other platforms is:
> > > 
> > > commit 3490ea5de6ac4af309c3df8a26a5cca61306334c
> > > Author: Chris Wilson <chris at chris-wilson.co.uk>
> > > Date:   Mon Jan 7 10:11:40 2013 +0000
> > > 
> > >     drm/i915: Treat crtc->mode.clock == 0 as disabled
> > 
> > Now I actually remember that Jani did try reverting this patch in his
> > tree, and didn't get any div by zeros. So we were a bit confused about
> > the patch at the time. If he had that fix in his tree, then that would
> > explain it. Maybe this patch can be dropped then?
> 
> This is a fallout from the reworked modeset code. Since we now read out
> the bios hw state, if the new state leaves pipe B running, but enables
> pipe A (happens only when the bios enables pipe B) we try to recompute
> watermarks for pipe B, but that mode is 0. Hence boom.
> 
> This patch can be killed once the hw state readout is properly extended to
> cover the required information (i.e. we need to move all the wm code to
> only look at pipe_config and ensure that all the values in there are
> correct).
> 
> Once booted, you can't hit this any more.

I don't think this patch is necessary anymore... just tested and things
work fine w/o so I'll drop.

-- 
Jesse Barnes, Intel Open Source Technology Center