[PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Matthew Auld]

In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, this
will enable us to freely add warnings which our CI will hopefully catch
but without fear of impacting production machines.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 51ec793f2e20..04c777e6d0bf 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -27,8 +27,10 @@
 
 #ifdef CONFIG_DRM_I915_DEBUG_GEM
 #define GEM_BUG_ON(expr) BUG_ON(expr)
+#define GEM_WARN_ON(expr) WARN_ON(expr)
 #else
 #define GEM_BUG_ON(expr) do { } while (0)
+#define GEM_WARN_ON(expr) do { } while (0)
 #endif
 
 #define I915_NUM_ENGINES 5
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: move vma sanity checking into i915_vma_bind

[Matthew Auld]

If we move the sanity checking from gen8_alloc_va_range_3lvl and
gen6_alloc_va_range into i915_vma_bind, we will increase our coverage to
now both callbacks. We also convert each WARN_ON over to a GEM_WARN_ON.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ------------
 drivers/gpu/drm/i915/i915_vma.c     |  6 ++++++
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 02fb063302bf..e0746eb5dc54 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1304,15 +1304,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 	int ret;
 
-	/* Wrap is never okay since we can only represent 48b, and we don't
-	 * actually use the other side of the canonical address space.
-	 */
-	if (WARN_ON(start + length < start))
-		return -ENODEV;
-
-	if (WARN_ON(start + length > vm->total))
-		return -ENODEV;
-
 	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
 	if (ret)
 		return ret;
@@ -1930,9 +1921,6 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 	uint32_t pde;
 	int ret;
 
-	if (WARN_ON(start_in + length_in > ppgtt->base.total))
-		return -ENODEV;
-
 	start = start_save = start_in;
 	length = length_save = length_in;
 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 4c91a68ecb6d..2c494191d7b3 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -176,6 +176,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	if (bind_flags == 0)
 		return 0;
 
+	if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
+		return -ENODEV;
+
+	if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
+		return -ENODEV;
+
 	if (vma_flags == 0 && vma->vm->allocate_va_range) {
 		trace_i915_va_alloc(vma);
 		ret = vma->vm->allocate_va_range(vma->vm,
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: move vma sanity checking into i915_vma_bind

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 10800 bytes --]

Hi Matthew,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20161202]
[cannot apply to v4.9-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matthew-Auld/drm-i915-introduce-GEM_WARN_ON/20161203-231346
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s3-12040051 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/io-mapping.h:21,
                    from drivers/gpu/drm/i915/i915_vma.h:28,
                    from drivers/gpu/drm/i915/i915_vma.c:25:
   drivers/gpu/drm/i915/i915_vma.c: In function 'i915_vma_bind':
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/gpu/drm/i915/i915_vma.c:179:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:179:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
         ^~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:149:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
>> drivers/gpu/drm/i915/i915_vma.c:179:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:179:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
         ^~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
>> drivers/gpu/drm/i915/i915_vma.c:179:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:179:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
         ^~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:149:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
   drivers/gpu/drm/i915/i915_vma.c:182:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:182:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
         ^~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:149:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^~~~
   drivers/gpu/drm/i915/i915_vma.c:182:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:182:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
         ^~~~~~~~~~~
   drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   include/linux/compiler.h:160:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^~~~
   drivers/gpu/drm/i915/i915_vma.c:182:2: note: in expansion of macro 'if'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
     ^~
   drivers/gpu/drm/i915/i915_vma.c:182:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
         ^~~~~~~~~~~

vim +/if +179 drivers/gpu/drm/i915/i915_vma.c

    19	 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
    20	 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
    21	 * IN THE SOFTWARE.
    22	 *
    23	 */
    24	 
  > 25	#include "i915_vma.h"
    26	
    27	#include "i915_drv.h"
    28	#include "intel_ringbuffer.h"
    29	#include "intel_frontbuffer.h"
    30	
    31	#include <drm/drm_gem.h>
    32	
    33	static void
    34	i915_vma_retire(struct i915_gem_active *active,
    35			struct drm_i915_gem_request *rq)
    36	{
    37		const unsigned int idx = rq->engine->id;
    38		struct i915_vma *vma =
    39			container_of(active, struct i915_vma, last_read[idx]);
    40		struct drm_i915_gem_object *obj = vma->obj;
    41	
    42		GEM_BUG_ON(!i915_vma_has_active_engine(vma, idx));
    43	
    44		i915_vma_clear_active(vma, idx);
    45		if (i915_vma_is_active(vma))
    46			return;
    47	
    48		list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
    49		if (unlikely(i915_vma_is_closed(vma) && !i915_vma_is_pinned(vma)))
    50			WARN_ON(i915_vma_unbind(vma));
    51	
    52		GEM_BUG_ON(!i915_gem_object_is_active(obj));
    53		if (--obj->active_count)
    54			return;
    55	
    56		/* Bump our place on the bound list to keep it roughly in LRU order
    57		 * so that we don't steal from recently used but inactive objects
    58		 * (unless we are forced to ofc!)
    59		 */
    60		if (obj->bind_count)
    61			list_move_tail(&obj->global_link, &rq->i915->mm.bound_list);
    62	
    63		obj->mm.dirty = true; /* be paranoid  */
    64	
    65		if (i915_gem_object_has_active_reference(obj)) {
    66			i915_gem_object_clear_active_reference(obj);
    67			i915_gem_object_put(obj);
    68		}
    69	}
    70	
    71	static struct i915_vma *
    72	__i915_vma_create(struct drm_i915_gem_object *obj,
    73			  struct i915_address_space *vm,
    74			  const struct i915_ggtt_view *view)
    75	{
    76		struct i915_vma *vma;
    77		struct rb_node *rb, **p;
    78		int i;
    79	
    80		GEM_BUG_ON(vm->closed);
    81	
    82		vma = kmem_cache_zalloc(to_i915(obj->base.dev)->vmas, GFP_KERNEL);
    83		if (vma == NULL)
    84			return ERR_PTR(-ENOMEM);
    85	
    86		INIT_LIST_HEAD(&vma->exec_list);
    87		for (i = 0; i < ARRAY_SIZE(vma->last_read); i++)
    88			init_request_active(&vma->last_read[i], i915_vma_retire);
    89		init_request_active(&vma->last_fence, NULL);
    90		list_add(&vma->vm_link, &vm->unbound_list);
    91		vma->vm = vm;
    92		vma->obj = obj;
    93		vma->size = obj->base.size;
    94	
    95		if (view) {
    96			vma->ggtt_view = *view;
    97			if (view->type == I915_GGTT_VIEW_PARTIAL) {
    98				vma->size = view->params.partial.size;
    99				vma->size <<= PAGE_SHIFT;
   100			} else if (view->type == I915_GGTT_VIEW_ROTATED) {
   101				vma->size =
   102					intel_rotation_info_size(&view->params.rotated);
   103				vma->size <<= PAGE_SHIFT;
   104			}
   105		}
   106	
   107		if (i915_is_ggtt(vm)) {
   108			vma->flags |= I915_VMA_GGTT;
   109			list_add(&vma->obj_link, &obj->vma_list);
   110		} else {
   111			i915_ppgtt_get(i915_vm_to_ppgtt(vm));
   112			list_add_tail(&vma->obj_link, &obj->vma_list);
   113		}
   114	
   115		rb = NULL;
   116		p = &obj->vma_tree.rb_node;
   117		while (*p) {
   118			struct i915_vma *pos;
   119	
   120			rb = *p;
   121			pos = rb_entry(rb, struct i915_vma, obj_node);
   122			if (i915_vma_compare(pos, vm, view) < 0)
   123				p = &rb->rb_right;
   124			else
   125				p = &rb->rb_left;
   126		}
   127		rb_link_node(&vma->obj_node, rb, p);
   128		rb_insert_color(&vma->obj_node, &obj->vma_tree);
   129	
   130		return vma;
   131	}
   132	
   133	struct i915_vma *
   134	i915_vma_create(struct drm_i915_gem_object *obj,
   135			struct i915_address_space *vm,
   136			const struct i915_ggtt_view *view)
   137	{
   138		lockdep_assert_held(&obj->base.dev->struct_mutex);
   139		GEM_BUG_ON(view && !i915_is_ggtt(vm));
   140		GEM_BUG_ON(i915_gem_obj_to_vma(obj, vm, view));
   141	
   142		return __i915_vma_create(obj, vm, view);
   143	}
   144	
   145	/**
   146	 * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space.
   147	 * @vma: VMA to map
   148	 * @cache_level: mapping cache level
   149	 * @flags: flags like global or local mapping
   150	 *
   151	 * DMA addresses are taken from the scatter-gather table of this object (or of
   152	 * this VMA in case of non-default GGTT views) and PTE entries set up.
   153	 * Note that DMA addresses are also the only part of the SG table we care about.
   154	 */
   155	int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
   156			  u32 flags)
   157	{
   158		u32 bind_flags;
   159		u32 vma_flags;
   160		int ret;
   161	
   162		if (WARN_ON(flags == 0))
   163			return -EINVAL;
   164	
   165		bind_flags = 0;
   166		if (flags & PIN_GLOBAL)
   167			bind_flags |= I915_VMA_GLOBAL_BIND;
   168		if (flags & PIN_USER)
   169			bind_flags |= I915_VMA_LOCAL_BIND;
   170	
   171		vma_flags = vma->flags & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND);
   172		if (flags & PIN_UPDATE)
   173			bind_flags |= vma_flags;
   174		else
   175			bind_flags &= ~vma_flags;
   176		if (bind_flags == 0)
   177			return 0;
   178	
 > 179		if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
   180			return -ENODEV;
   181	
   182		if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26044 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: move vma sanity checking into i915_vma_bind

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7394 bytes --]

Hi Matthew,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20161202]
[cannot apply to v4.9-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matthew-Auld/drm-i915-introduce-GEM_WARN_ON/20161203-231346
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/i915/i915_gem_request.h:30:0,
                    from drivers/gpu/drm/i915/i915_gem_timeline.h:30,
                    from drivers/gpu/drm/i915/i915_gem_gtt.h:40,
                    from drivers/gpu/drm/i915/i915_vma.h:32,
                    from drivers/gpu/drm/i915/i915_vma.c:25:
   drivers/gpu/drm/i915/i915_vma.c: In function 'i915_vma_bind':
>> drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
>> drivers/gpu/drm/i915/i915_vma.c:179:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
         ^~~~~~~~~~~
>> drivers/gpu/drm/i915/i915_gem.h:33:27: error: expected expression before 'do'
    #define GEM_WARN_ON(expr) do { } while (0)
                              ^
   drivers/gpu/drm/i915/i915_vma.c:182:6: note: in expansion of macro 'GEM_WARN_ON'
     if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
         ^~~~~~~~~~~

vim +/GEM_WARN_ON +179 drivers/gpu/drm/i915/i915_vma.c

    19	 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
    20	 * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
    21	 * IN THE SOFTWARE.
    22	 *
    23	 */
    24	 
  > 25	#include "i915_vma.h"
    26	
    27	#include "i915_drv.h"
    28	#include "intel_ringbuffer.h"
    29	#include "intel_frontbuffer.h"
    30	
    31	#include <drm/drm_gem.h>
    32	
    33	static void
    34	i915_vma_retire(struct i915_gem_active *active,
    35			struct drm_i915_gem_request *rq)
    36	{
    37		const unsigned int idx = rq->engine->id;
    38		struct i915_vma *vma =
    39			container_of(active, struct i915_vma, last_read[idx]);
    40		struct drm_i915_gem_object *obj = vma->obj;
    41	
    42		GEM_BUG_ON(!i915_vma_has_active_engine(vma, idx));
    43	
    44		i915_vma_clear_active(vma, idx);
    45		if (i915_vma_is_active(vma))
    46			return;
    47	
    48		list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
    49		if (unlikely(i915_vma_is_closed(vma) && !i915_vma_is_pinned(vma)))
    50			WARN_ON(i915_vma_unbind(vma));
    51	
    52		GEM_BUG_ON(!i915_gem_object_is_active(obj));
    53		if (--obj->active_count)
    54			return;
    55	
    56		/* Bump our place on the bound list to keep it roughly in LRU order
    57		 * so that we don't steal from recently used but inactive objects
    58		 * (unless we are forced to ofc!)
    59		 */
    60		if (obj->bind_count)
    61			list_move_tail(&obj->global_link, &rq->i915->mm.bound_list);
    62	
    63		obj->mm.dirty = true; /* be paranoid  */
    64	
    65		if (i915_gem_object_has_active_reference(obj)) {
    66			i915_gem_object_clear_active_reference(obj);
    67			i915_gem_object_put(obj);
    68		}
    69	}
    70	
    71	static struct i915_vma *
    72	__i915_vma_create(struct drm_i915_gem_object *obj,
    73			  struct i915_address_space *vm,
    74			  const struct i915_ggtt_view *view)
    75	{
    76		struct i915_vma *vma;
    77		struct rb_node *rb, **p;
    78		int i;
    79	
    80		GEM_BUG_ON(vm->closed);
    81	
    82		vma = kmem_cache_zalloc(to_i915(obj->base.dev)->vmas, GFP_KERNEL);
    83		if (vma == NULL)
    84			return ERR_PTR(-ENOMEM);
    85	
    86		INIT_LIST_HEAD(&vma->exec_list);
    87		for (i = 0; i < ARRAY_SIZE(vma->last_read); i++)
    88			init_request_active(&vma->last_read[i], i915_vma_retire);
    89		init_request_active(&vma->last_fence, NULL);
    90		list_add(&vma->vm_link, &vm->unbound_list);
    91		vma->vm = vm;
    92		vma->obj = obj;
    93		vma->size = obj->base.size;
    94	
    95		if (view) {
    96			vma->ggtt_view = *view;
    97			if (view->type == I915_GGTT_VIEW_PARTIAL) {
    98				vma->size = view->params.partial.size;
    99				vma->size <<= PAGE_SHIFT;
   100			} else if (view->type == I915_GGTT_VIEW_ROTATED) {
   101				vma->size =
   102					intel_rotation_info_size(&view->params.rotated);
   103				vma->size <<= PAGE_SHIFT;
   104			}
   105		}
   106	
   107		if (i915_is_ggtt(vm)) {
   108			vma->flags |= I915_VMA_GGTT;
   109			list_add(&vma->obj_link, &obj->vma_list);
   110		} else {
   111			i915_ppgtt_get(i915_vm_to_ppgtt(vm));
   112			list_add_tail(&vma->obj_link, &obj->vma_list);
   113		}
   114	
   115		rb = NULL;
   116		p = &obj->vma_tree.rb_node;
   117		while (*p) {
   118			struct i915_vma *pos;
   119	
   120			rb = *p;
   121			pos = rb_entry(rb, struct i915_vma, obj_node);
   122			if (i915_vma_compare(pos, vm, view) < 0)
   123				p = &rb->rb_right;
   124			else
   125				p = &rb->rb_left;
   126		}
   127		rb_link_node(&vma->obj_node, rb, p);
   128		rb_insert_color(&vma->obj_node, &obj->vma_tree);
   129	
   130		return vma;
   131	}
   132	
   133	struct i915_vma *
   134	i915_vma_create(struct drm_i915_gem_object *obj,
   135			struct i915_address_space *vm,
   136			const struct i915_ggtt_view *view)
   137	{
   138		lockdep_assert_held(&obj->base.dev->struct_mutex);
   139		GEM_BUG_ON(view && !i915_is_ggtt(vm));
   140		GEM_BUG_ON(i915_gem_obj_to_vma(obj, vm, view));
   141	
   142		return __i915_vma_create(obj, vm, view);
   143	}
   144	
   145	/**
   146	 * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address space.
   147	 * @vma: VMA to map
   148	 * @cache_level: mapping cache level
   149	 * @flags: flags like global or local mapping
   150	 *
   151	 * DMA addresses are taken from the scatter-gather table of this object (or of
   152	 * this VMA in case of non-default GGTT views) and PTE entries set up.
   153	 * Note that DMA addresses are also the only part of the SG table we care about.
   154	 */
   155	int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
   156			  u32 flags)
   157	{
   158		u32 bind_flags;
   159		u32 vma_flags;
   160		int ret;
   161	
   162		if (WARN_ON(flags == 0))
   163			return -EINVAL;
   164	
   165		bind_flags = 0;
   166		if (flags & PIN_GLOBAL)
   167			bind_flags |= I915_VMA_GLOBAL_BIND;
   168		if (flags & PIN_USER)
   169			bind_flags |= I915_VMA_LOCAL_BIND;
   170	
   171		vma_flags = vma->flags & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND);
   172		if (flags & PIN_UPDATE)
   173			bind_flags |= vma_flags;
   174		else
   175			bind_flags &= ~vma_flags;
   176		if (bind_flags == 0)
   177			return 0;
   178	
 > 179		if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
   180			return -ENODEV;
   181	
   182		if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56838 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Ville Syrjälä]

On Fri, Dec 02, 2016 at 01:23:13PM +0000, Matthew Auld wrote:
> In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, this
> will enable us to freely add warnings which our CI will hopefully catch
> but without fear of impacting production machines.

Impacting performance?

> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 51ec793f2e20..04c777e6d0bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -27,8 +27,10 @@
>  
>  #ifdef CONFIG_DRM_I915_DEBUG_GEM
>  #define GEM_BUG_ON(expr) BUG_ON(expr)
> +#define GEM_WARN_ON(expr) WARN_ON(expr)
>  #else
>  #define GEM_BUG_ON(expr) do { } while (0)
> +#define GEM_WARN_ON(expr) do { } while (0)
>  #endif
>  
>  #define I915_NUM_ENGINES 5
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Chris Wilson]

On Fri, Dec 02, 2016 at 01:23:13PM +0000, Matthew Auld wrote:
> In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, this
> will enable us to freely add warnings which our CI will hopefully catch
> but without fear of impacting production machines.
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index 51ec793f2e20..04c777e6d0bf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -27,8 +27,10 @@
>  
>  #ifdef CONFIG_DRM_I915_DEBUG_GEM
>  #define GEM_BUG_ON(expr) BUG_ON(expr)
> +#define GEM_WARN_ON(expr) WARN_ON(expr)
>  #else
>  #define GEM_BUG_ON(expr) do { } while (0)
> +#define GEM_WARN_ON(expr) do { } while (0)

#define GEM_WARN_ON 0

i.e. so that if (GEM_WARN_ON(insane_condition()) compiles out correctly
without DEBUG_GEM enabled.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Matthew Auld]

On 2 December 2016 at 13:54, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Dec 02, 2016 at 01:23:13PM +0000, Matthew Auld wrote:
>> In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, this
>> will enable us to freely add warnings which our CI will hopefully catch
>> but without fear of impacting production machines.
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_gem.h | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
>> index 51ec793f2e20..04c777e6d0bf 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.h
>> +++ b/drivers/gpu/drm/i915/i915_gem.h
>> @@ -27,8 +27,10 @@
>>
>>  #ifdef CONFIG_DRM_I915_DEBUG_GEM
>>  #define GEM_BUG_ON(expr) BUG_ON(expr)
>> +#define GEM_WARN_ON(expr) WARN_ON(expr)
>>  #else
>>  #define GEM_BUG_ON(expr) do { } while (0)
>> +#define GEM_WARN_ON(expr) do { } while (0)
>
> #define GEM_WARN_ON 0
Oops. As an alternative, would you be offended with something like:

#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0)

Then the compiler will still check the validity of the expression, but
will still compile everything out, and then we don't accidentally
break the build when compiling without DEBUG_GEM ?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Chris Wilson]

On Fri, Dec 02, 2016 at 03:11:05PM +0000, Matthew Auld wrote:
> On 2 December 2016 at 13:54, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Fri, Dec 02, 2016 at 01:23:13PM +0000, Matthew Auld wrote:
> >> In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, this
> >> will enable us to freely add warnings which our CI will hopefully catch
> >> but without fear of impacting production machines.
> >>
> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> >> index 51ec793f2e20..04c777e6d0bf 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.h
> >> +++ b/drivers/gpu/drm/i915/i915_gem.h
> >> @@ -27,8 +27,10 @@
> >>
> >>  #ifdef CONFIG_DRM_I915_DEBUG_GEM
> >>  #define GEM_BUG_ON(expr) BUG_ON(expr)
> >> +#define GEM_WARN_ON(expr) WARN_ON(expr)
> >>  #else
> >>  #define GEM_BUG_ON(expr) do { } while (0)
> >> +#define GEM_WARN_ON(expr) do { } while (0)
> >
> > #define GEM_WARN_ON 0
> Oops. As an alternative, would you be offended with something like:
> 
> #define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0)
> 
> Then the compiler will still check the validity of the expression, but
> will still compile everything out, and then we don't accidentally
> break the build when compiling without DEBUG_GEM ?

Ooh, new shiny. Uses sizeof()! Can you prepare a patch to fixup
GEM_BUG_ON as well?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: introduce GEM_WARN_ON

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: introduce GEM_WARN_ON
URL   : https://patchwork.freedesktop.org/series/16278/
State : warning

== Summary ==

Series 16278v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/16278/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:230  dwarn:1   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

273202788eba907049316b3ccc2f4d1fd30aff1d drm-tip: 2016y-12m-02d-11h-27m-42s UTC integration manifest
05e8d2b drm/i915: move vma sanity checking into i915_vma_bind
91fae78 drm/i915: introduce GEM_WARN_ON

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3175/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: introduce GEM_WARN_ON

[Matthew Auld]

In a similar spirit to GEM_BUG_ON we now also have GEM_WARN_ON, with the
simple goal of expressing warnings which are truly insane, and so are
only really useful for CI where we have some abusive tests.

v2:
  - use BUILD_BUG_ON_INVALID for !DEBUG_GEM
  - clarify commit message

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 8801a14a78a2..a585d47c420a 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -27,8 +27,10 @@
 
 #ifdef CONFIG_DRM_I915_DEBUG_GEM
 #define GEM_BUG_ON(expr) BUG_ON(expr)
+#define GEM_WARN_ON(expr) WARN_ON(expr)
 #else
 #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
+#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0)
 #endif
 
 #define I915_NUM_ENGINES 5
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: move vma sanity checking into i915_vma_bind

[Matthew Auld]

If we move the sanity checking from gen8_alloc_va_range_3lvl and
gen6_alloc_va_range into i915_vma_bind, we will increase our coverage to
now both callbacks. We also convert each WARN_ON over to a GEM_WARN_ON.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ------------
 drivers/gpu/drm/i915/i915_vma.c     |  6 ++++++
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ef00d36680c9..4f405b445b6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1303,15 +1303,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 	int ret;
 
-	/* Wrap is never okay since we can only represent 48b, and we don't
-	 * actually use the other side of the canonical address space.
-	 */
-	if (WARN_ON(start + length < start))
-		return -ENODEV;
-
-	if (WARN_ON(start + length > vm->total))
-		return -ENODEV;
-
 	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
 	if (ret)
 		return ret;
@@ -1929,9 +1920,6 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 	uint32_t pde;
 	int ret;
 
-	if (WARN_ON(start_in + length_in > ppgtt->base.total))
-		return -ENODEV;
-
 	start = start_save = start_in;
 	length = length_save = length_in;
 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 37c3eebe8316..9e121222c5eb 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -176,6 +176,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	if (bind_flags == 0)
 		return 0;
 
+	if (GEM_WARN_ON(vma->node.start + vma->node.size < vma->node.start))
+		return -ENODEV;
+
+	if (GEM_WARN_ON(vma->node.start + vma->node.size > vma->vm->total))
+		return -ENODEV;
+
 	if (vma_flags == 0 && vma->vm->allocate_va_range) {
 		trace_i915_va_alloc(vma);
 		ret = vma->vm->allocate_va_range(vma->vm,
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: move vma sanity checking into i915_vma_bind

[Chris Wilson]

On Tue, Dec 13, 2016 at 04:00:59PM +0000, Matthew Auld wrote:
> If we move the sanity checking from gen8_alloc_va_range_3lvl and
> gen6_alloc_va_range into i915_vma_bind, we will increase our coverage to
> now both callbacks. We also convert each WARN_ON over to a GEM_WARN_ON.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ------------
>  drivers/gpu/drm/i915/i915_vma.c     |  6 ++++++
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ef00d36680c9..4f405b445b6c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1303,15 +1303,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
>  	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
>  	int ret;
>  
> -	/* Wrap is never okay since we can only represent 48b, and we don't
> -	 * actually use the other side of the canonical address space.
> -	 */
> -	if (WARN_ON(start + length < start))
> -		return -ENODEV;
> -
> -	if (WARN_ON(start + length > vm->total))
> -		return -ENODEV;
> -
>  	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
>  	if (ret)
>  		return ret;
> @@ -1929,9 +1920,6 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
>  	uint32_t pde;
>  	int ret;
>  
> -	if (WARN_ON(start_in + length_in > ppgtt->base.total))
> -		return -ENODEV;
> -
>  	start = start_save = start_in;
>  	length = length_save = length_in;
>  
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 37c3eebe8316..9e121222c5eb 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -176,6 +176,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  	if (bind_flags == 0)
>  		return 0;

#define range_overfows(start, size, max) ({
	typeof(start) start__ = (start);
	typeof(size) size__ = (size);
	typeof(max) max__ = (max);
	(void)(&start__ == &size__);
	(void)(&start__ == &max__);
	start > max || size > max - start;
})

if (GEM_WARN_ON(range_overflows(vma->node.start,
				vma->node.size,
				vma->vm->total))
    return -ENODEV;

Then look at pread/pwrite for where we can reuse range_overflows(), e.g.
if (range_overflows(args->offset, args->size, obj->base.size)) {

Might need a range_overflows_t();
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx