[Intel-gfx] [PATCH 03/37] drm/i915: Add some selftests for sg_table manipulation

[Chris Wilson]

On Thu, Jan 12, 2017 at 10:56:42AM +0000, Tvrtko Ursulin wrote:
> 
> On 11/01/2017 21:09, Chris Wilson wrote:
> >+static bool alloc_table(struct pfn_table *pt,
> >+			unsigned long count,
> >+			unsigned long max)
> >+{
> >+	struct scatterlist *sg;
> >+	unsigned long n, pfn;
> >+
> >+	if (sg_alloc_table(&pt->st, max,
> >+			   GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN))
> >+		return false;
> >+
> >+	pt->start = PFN_BIAS;
> >+	pfn = pt->start;
> >+	sg = pt->st.sgl;
> >+	for (n = 0; n < count; n++) {
> >+		if (n)
> >+			sg = sg_next(sg);
> >+		sg_set_page(sg, pfn_to_page(pfn), (n + 1)*PAGE_SIZE, 0);
> 
> For count = BIT(max_order) (== 1M) the last sg entry will have a
> size of 4GiB which would overflow sg->length, no? Especially with
> size + offset below. Unless for_each_prime_number stops below
> max_order. But where?

Stops at the last prime number less than 20, 19. A GEM_BUG_ON() would
clarify that.
> 
> >+		pfn += n + 1;
> 
> Please describe what kind of table and why you are building with a
> comment. If tests have no comments and do, on the first look, not
> fully obvious things, then it is a lot of effort in the future to
> work on fixes and/or maintain the code.

Creative block? If I start along those lines, I'll end up adding more
tests... Oh well.

> >+	}
> >+	sg_mark_end(sg);
> >+	pt->st.nents = n;
> >+	pt->end = pfn;
> >+
> >+	return true;
> >+}
> >+
> >+static int igt_sg_alloc(void *ignored)
> >+{
> >+	I915_SELFTEST_TIMEOUT(end_time);
> >+	const unsigned long max_order = 20; /* approximating a 4GiB object */
> >+	unsigned long prime;
> >+
> >+	for_each_prime_number(prime, max_order) {
> >+		unsigned long size = BIT(prime);
> >+		int offset;
> >+
> >+		for (offset = -1; offset <= 1; offset++) {
> >+			struct pfn_table pt;
> >+			int err;
> >+
> >+			if (!alloc_table(&pt, size + offset, size + offset))
> >+				return 0; /* out of memory, give up */
> >+
> >+			err = expect_pfn_sgtable(&pt, "sg_alloc_table");
> >+			sg_free_table(&pt.st);
> >+			if (err)
> >+				return err;
> >+
> >+			if (time_after(jiffies, end_time)) {
> >+				pr_warn("%s timed out: last table size %lu\n",
> >+					__func__, size + offset);
> >+				return 0;
> >+			}
> 
> Maybe a fmt+vaarg timeout helper since I expect there'll be a lot of
> this. Like:
> 
> if (i915_selftest_timeout(end_time, "fmt %s", arg))
> 	return 0;
> 
> It is slightly tidier.

Nice.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre