Re: [PATCH 32/37] drm/i915: Exercise i915_vma_pin/i915_vma_insert

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2017-01-11 at 21:09 +0000, Chris Wilson wrote:
> High-level testing of the struct drm_mm by verifying our handling of
> weird requests to i915_vma_pin.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +static int igt_vma_pin1(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	const struct pin_mode modes[] = {
> +		[0] = { 0, PIN_GLOBAL, assert_pin_valid },

Now that pin_mode is introduced far, use named initializers (especially
when the array starts with plenty of zeros in the first column). Or at
the least, make a comment /* size, flags, assert_func */

<SNIP>

> +
> +		[24] = { 8192, PIN_GLOBAL | PIN_OFFSET_BIAS | (i915->ggtt.mappable_end - 4096), assert_pin_valid },
> +
> +#if !IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)

Better drop the unnecessarily verbose [NN] =, if an another #if section
is added, it'll be bad. And no real benefit either, inserting a test in
the middle will be bad too, it's not like we never forget something
from first iteration.

> +		/* Misusing BIAS is a programming error (it is not controllable
> +		 * from userspace) so when debugging is enabled, it explodes.
> +		 * However, the tests are still quite interesting for checking
> +		 * variable start, end and size.
> +		 */

Are they ever run? I'd imagine not in the CI at least. Should not hurt
with explanation.

With the array sanitized;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx