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

2017-01-13 Thread Chris Wilson
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

2017-01-13 Thread Joonas Lahtinen
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