Re: [Intel-gfx] [PATCH 32/37] drm/i915: Exercise i915_vma_pin/i915_vma_insert
On Fri, Jan 13, 2017 at 02:49:49PM +0200, Joonas Lahtinen wrote: > On ke, 2017-01-11 at 21:09 +, 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 > > > > > +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 */ > > > > > + > > + [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. It was providing some information in the error report, and I was too lazy to include the appropriate string. Why do you never let me be lazy! -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: [Intel-gfx] [PATCH 32/37] drm/i915: Exercise i915_vma_pin/i915_vma_insert
On ke, 2017-01-11 at 21:09 +, 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 > +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 */ > + > + [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 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