Re: [PATCH 3/5] drm/i915/vlv: warn on bad VLV PLL divider values

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 27 Sep 2013 22:11:33 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Sep 27, 2013 at 09:05:24PM +0100, Chris Wilson wrote:
> > On Fri, Sep 27, 2013 at 12:57:24PM -0700, Jesse Barnes wrote:
> > > This avoids a divide by zero and warns appropriately on this serious bug.
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 8da1c96..9a83236 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5109,6 +5109,11 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
> > >  	clock.p1 = (mdiv >> DPIO_P1_SHIFT) & 7;
> > >  	clock.p2 = (mdiv >> DPIO_P2_SHIFT) & 0x1f;
> > >  
> > > +	if (!clock.n || !(clock.p1 * clock.p2)) {
> > 
> > if ((clock.n & clock.p1 & clock.p2) == 0) {
> > 
> > or just
> > 
> > if (!clock.n || !clock.p1 || !clock.p2)
> > 
> > > +		WARN(1, "bad divider values on pipe %d\n", crtc->pipe);
> > > +		return;
> > > +	}
> > > +
> > 
> > So I think you want
> > 
> > if (WARN_ONCE(clock.n == 0 || clock.p1 == 0 || clock.p2 == 0,
> >               "bad divider values on pipe %d (m1=%d, m2=%d, p1=%d, p2=%d, n=%d)\n",
> > 	      crtc->pipe, clock.m1, clock.m2, clock.p1, clock.p2, clock.n))
> >    return;
> 
> So the bios leaves a broken setup behind or is our hw state readout not
> quite careful enough?

No this just happens when we're really broken.  E.g. we don't set the
integrated clock bit and then try to enable a PLL and fail all over.

Shouldn't happen in practice.

-- 
Jesse Barnes, Intel Open Source Technology Center