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

[Daniel Vetter]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch