Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
On Thu, Jun 16, 2011 at 04:47:53PM +0100, Chris Wilson wrote: On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes jbar...@virtuousgeek.org wrote: On Thu, 16 Jun 2011 15:28:46 +0200 Wolfram Sang w.s...@pengutronix.de wrote: How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or a warning in the else-block? It seems to happen to users... Yeah, a BUG_ON would be fine. if (WARN_ON(!ptr, no display vtable)) return -ENODEV; That would mean converting the involved void-functions to int to propagate the error (intel_init_clock_gating, intel_modeset_init). Not a big deal, but quite intrusive. Do you really mean that? -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
On Sun, 19 Jun 2011 21:22:11 +0200, Wolfram Sang w.s...@pengutronix.de wrote: On Thu, Jun 16, 2011 at 04:47:53PM +0100, Chris Wilson wrote: On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes jbar...@virtuousgeek.org wrote: On Thu, 16 Jun 2011 15:28:46 +0200 Wolfram Sang w.s...@pengutronix.de wrote: How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or a warning in the else-block? It seems to happen to users... Yeah, a BUG_ON would be fine. if (WARN_ON(!ptr, no display vtable)) return -ENODEV; That would mean converting the involved void-functions to int to propagate the error (intel_init_clock_gating, intel_modeset_init). Not a big deal, but quite intrusive. Do you really mean that? A choice between a BUG_ON and error propagation? Choose error propagation, one day it will be for real. ;-) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) unconditionally calls the newly created
init_clock_gating-pointer. There is one case, however, where it does
not get set:
if (HAS_PCH_SPLIT(dev)) {
...
} else
dev_priv-display.update_wm = NULL;
}
We'll only hit this path on non-existent hardware. Since a clock
gating routine is required I'd rather just see the panic and add a new
routine at that time (i.e. what we normally do during bringup).
How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
a warning in the else-block? It seems to happen to users...
--
Pengutronix e.K. | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
On Thu, 16 Jun 2011 15:28:46 +0200
Wolfram Sang w.s...@pengutronix.de wrote:
Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) unconditionally calls the newly created
init_clock_gating-pointer. There is one case, however, where it does
not get set:
if (HAS_PCH_SPLIT(dev)) {
...
} else
dev_priv-display.update_wm = NULL;
}
We'll only hit this path on non-existent hardware. Since a clock
gating routine is required I'd rather just see the panic and add a new
routine at that time (i.e. what we normally do during bringup).
How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or
a warning in the else-block? It seems to happen to users...
Yeah, a BUG_ON would be fine.
--
Jesse Barnes, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
On Thu, 16 Jun 2011 08:15:57 -0700, Jesse Barnes jbar...@virtuousgeek.org wrote: On Thu, 16 Jun 2011 15:28:46 +0200 Wolfram Sang w.s...@pengutronix.de wrote: How about BUG_ON(!ptr) in the init-routine for a bit more grace? And/or a warning in the else-block? It seems to happen to users... Yeah, a BUG_ON would be fine. if (WARN_ON(!ptr, no display vtable)) return -ENODEV; -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: add check for valid init_clock_gating-pointer
On Thu, 16 Jun 2011 00:24:39 +0200
Wolfram Sang w.s...@pengutronix.de wrote:
Commit 6067aa (drm/i915: split clock gating init into per-chipset
functions) unconditionally calls the newly created
init_clock_gating-pointer. There is one case, however, where it does
not get set:
if (HAS_PCH_SPLIT(dev)) {
...
} else
dev_priv-display.update_wm = NULL;
}
We'll only hit this path on non-existent hardware. Since a clock
gating routine is required I'd rather just see the panic and add a new
routine at that time (i.e. what we normally do during bringup).
--
Jesse Barnes, Intel Open Source Technology Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
