| Message ID | 1376111536-12461-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 09, 2013 at 10:12:12PM -0700, Ben Widawsky wrote: > I just noticed in our code we don't really check the assertion, and > given some of the code I am changing in this area, I feel a WARN is very > nice to have. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> This is BUG() class. A WARN by itself here isn't going to prevent calamity, so cleanup and let userspace die gracefully. The alternative is random hw borkage (hopefully triggering an OOPS before it gets too far). -Chris
On Sat, Aug 10, 2013 at 09:43:48AM +0100, Chris Wilson wrote: > On Fri, Aug 09, 2013 at 10:12:12PM -0700, Ben Widawsky wrote: > > I just noticed in our code we don't really check the assertion, and > > given some of the code I am changing in this area, I feel a WARN is very > > nice to have. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > This is BUG() class. A WARN by itself here isn't going to prevent > calamity, so cleanup and let userspace die gracefully. The alternative > is random hw borkage (hopefully triggering an OOPS before it gets too > far). I think WARN is ok, there should be enough time for it to hit the logs before the evenutal machine death. Generally if a WARN requires us to add more cleanup code it's imo not worth it. -Daniel
On Fri, Aug 09, 2013 at 10:12:12PM -0700, Ben Widawsky wrote: > I just noticed in our code we don't really check the assertion, and > given some of the code I am changing in this area, I feel a WARN is very > nice to have. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 498ef8a..b91a7f0 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3186,6 +3186,8 @@ search_free: > if (i915_is_ggtt(vm)) > obj->map_and_fenceable = mappable && fenceable; > > + WARN_ON(map_and_fenceable & !obj->map_and_fenceable); s/&/&&/ I think, although since both are bools it doesn't matter correctness-wise. -Daniel > + > trace_i915_vma_bind(vma, map_and_fenceable); > i915_gem_verify_gtt(dev); > return 0; > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Aug 10, 2013 at 10:58:30AM +0200, Daniel Vetter wrote: > On Fri, Aug 09, 2013 at 10:12:12PM -0700, Ben Widawsky wrote: > > I just noticed in our code we don't really check the assertion, and > > given some of the code I am changing in this area, I feel a WARN is very > > nice to have. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 498ef8a..b91a7f0 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3186,6 +3186,8 @@ search_free: > > if (i915_is_ggtt(vm)) > > obj->map_and_fenceable = mappable && fenceable; > > > > + WARN_ON(map_and_fenceable & !obj->map_and_fenceable); > > s/&/&&/ I think, although since both are bools it doesn't matter > correctness-wise. > -Daniel > You're absolutely right. It was a typo. Do you want resubmit, or can you fix while apply?
On Sat, Aug 10, 2013 at 10:17:39AM -0700, Ben Widawsky wrote: > On Sat, Aug 10, 2013 at 10:58:30AM +0200, Daniel Vetter wrote: > > On Fri, Aug 09, 2013 at 10:12:12PM -0700, Ben Widawsky wrote: > > > I just noticed in our code we don't really check the assertion, and > > > given some of the code I am changing in this area, I feel a WARN is very > > > nice to have. > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/i915_gem.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > > index 498ef8a..b91a7f0 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem.c > > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > > @@ -3186,6 +3186,8 @@ search_free: > > > if (i915_is_ggtt(vm)) > > > obj->map_and_fenceable = mappable && fenceable; > > > > > > + WARN_ON(map_and_fenceable & !obj->map_and_fenceable); > > > > s/&/&&/ I think, although since both are bools it doesn't matter > > correctness-wise. > > -Daniel > > > > You're absolutely right. It was a typo. Do you want resubmit, or can you > fix while apply? Fixed and applied, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 498ef8a..b91a7f0 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3186,6 +3186,8 @@ search_free: if (i915_is_ggtt(vm)) obj->map_and_fenceable = mappable && fenceable; + WARN_ON(map_and_fenceable & !obj->map_and_fenceable); + trace_i915_vma_bind(vma, map_and_fenceable); i915_gem_verify_gtt(dev); return 0;
I just noticed in our code we don't really check the assertion, and given some of the code I am changing in this area, I feel a WARN is very nice to have. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem.c | 2 ++ 1 file changed, 2 insertions(+)