Re: [Intel-gfx] [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt
On Mon, Nov 25, 2013 at 09:08:50PM +0200, Ville Syrjälä wrote: On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote: On Mon, Nov 25, 2013 at 06:06:18PM +, Chris Wilson wrote: On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote: Since the beginning, the functions which try to properly reference the aliasing PPGTT have deferences a potentially null aliasing_ppgtt member. Since the accessors are meant to be global, this will not do. Introduced originally in: commit a70a3148b0c61cb7c588ea650db785b261b378a3 Author: Ben Widawsky b...@bwidawsk.net Date: Wed Jul 31 16:59:56 2013 -0700 drm/i915: Make proper functions for VMs Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..bc5c865 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) Where's the dereference? gcc is smarter than your average bear. -Chris I had assumed: dev_priv-mm.aliasing_ppgtt-base in cases when aliasing_ppgtt was NULL. Given that I never actually hit this, I agree GCC must be doing something. Sounds like another discussion on the implementation of offsetof(). Is such behavior documented somewhere? (forgive the lazy) IIRC the C standard does say that for *foo it's as if both and * weren't there (and IIRC the same for foo[x]), but doesn't really say that kind of thing for foo-bar. I guess it's a gray area, and just happens work that way on certain compilers. In this particular case I think there's one slight issue. If aliasing_pggtt == NULL, and someone passes in vm == NULL by accident, then it'll take the branch and use ggtt because aliasing_ppgtt-base will be NULL too (due to base being at offset 0 in the struct). Now, I don't know if a NULL vm could survive this far into the code, but it it can, then it might make debugging a bit more fun. If you pass in a NULL vm then you get to keep both pieces. And wrt portability atm you can pick any compiler for the linux kernel as long as it's gcc compatible. So imo nothing to really worry about. Aside: We really should start to ditch all these (vm, obj) pair lookups and move to using vmas everywhere ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt
Since the beginning, the functions which try to properly reference the aliasing PPGTT have deferences a potentially null aliasing_ppgtt member. Since the accessors are meant to be global, this will not do. Introduced originally in: commit a70a3148b0c61cb7c588ea650db785b261b378a3 Author: Ben Widawsky b...@bwidawsk.net Date: Wed Jul 31 16:59:56 2013 -0700 drm/i915: Make proper functions for VMs Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..bc5c865 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) vm = dev_priv-gtt.base; BUG_ON(list_empty(o-vma_list)); @@ -5012,7 +5013,8 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) vm = dev_priv-gtt.base; BUG_ON(list_empty(o-vma_list)); -- 1.8.4.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt
On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote: Since the beginning, the functions which try to properly reference the aliasing PPGTT have deferences a potentially null aliasing_ppgtt member. Since the accessors are meant to be global, this will not do. Introduced originally in: commit a70a3148b0c61cb7c588ea650db785b261b378a3 Author: Ben Widawsky b...@bwidawsk.net Date: Wed Jul 31 16:59:56 2013 -0700 drm/i915: Make proper functions for VMs Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..bc5c865 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) Where's the dereference? gcc is smarter than your average bear. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt
On Mon, Nov 25, 2013 at 06:06:18PM +, Chris Wilson wrote: On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote: Since the beginning, the functions which try to properly reference the aliasing PPGTT have deferences a potentially null aliasing_ppgtt member. Since the accessors are meant to be global, this will not do. Introduced originally in: commit a70a3148b0c61cb7c588ea650db785b261b378a3 Author: Ben Widawsky b...@bwidawsk.net Date: Wed Jul 31 16:59:56 2013 -0700 drm/i915: Make proper functions for VMs Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..bc5c865 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) Where's the dereference? gcc is smarter than your average bear. -Chris I had assumed: dev_priv-mm.aliasing_ppgtt-base in cases when aliasing_ppgtt was NULL. Given that I never actually hit this, I agree GCC must be doing something. Is such behavior documented somewhere? (forgive the lazy) -- Chris Wilson, Intel Open Source Technology Centre -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt
On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote: On Mon, Nov 25, 2013 at 06:06:18PM +, Chris Wilson wrote: On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote: Since the beginning, the functions which try to properly reference the aliasing PPGTT have deferences a potentially null aliasing_ppgtt member. Since the accessors are meant to be global, this will not do. Introduced originally in: commit a70a3148b0c61cb7c588ea650db785b261b378a3 Author: Ben Widawsky b...@bwidawsk.net Date: Wed Jul 31 16:59:56 2013 -0700 drm/i915: Make proper functions for VMs Signed-off-by: Ben Widawsky b...@bwidawsk.net --- drivers/gpu/drm/i915/i915_gem.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 40d9dcf..bc5c865 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4971,7 +4971,8 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o, struct drm_i915_private *dev_priv = o-base.dev-dev_private; struct i915_vma *vma; - if (vm == dev_priv-mm.aliasing_ppgtt-base) + if (!dev_priv-mm.aliasing_ppgtt || + vm == dev_priv-mm.aliasing_ppgtt-base) Where's the dereference? gcc is smarter than your average bear. -Chris I had assumed: dev_priv-mm.aliasing_ppgtt-base in cases when aliasing_ppgtt was NULL. Given that I never actually hit this, I agree GCC must be doing something. Sounds like another discussion on the implementation of offsetof(). Is such behavior documented somewhere? (forgive the lazy) IIRC the C standard does say that for *foo it's as if both and * weren't there (and IIRC the same for foo[x]), but doesn't really say that kind of thing for foo-bar. I guess it's a gray area, and just happens work that way on certain compilers. In this particular case I think there's one slight issue. If aliasing_pggtt == NULL, and someone passes in vm == NULL by accident, then it'll take the branch and use ggtt because aliasing_ppgtt-base will be NULL too (due to base being at offset 0 in the struct). Now, I don't know if a NULL vm could survive this far into the code, but it it can, then it might make debugging a bit more fun. As a side note, we actually depend on such things in the mode setting code. Ie. ((struct intel_crtc*)NULL)-base must be NULL, otherwise interesting bugs would happen. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
