| Message ID | 1380311846-1581-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; -Chris
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? -Daniel
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.
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)) { + WARN(1, "bad divider values on pipe %d\n", crtc->pipe); + return; + } + clock.vco = refclk * clock.m1 * clock.m2 / clock.n; clock.dot = 2 * clock.vco / (clock.p1 * clock.p2);
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(+)