[igt-dev] [PATCH i-g-t v5 2/2] tests: add slice power programming test

[Chris Wilson]

Quoting Lionel Landwerlin (2018-05-24 15:58:57)
> +static int drm_fd;
> +static int devid;
> +static uint64_t device_slice_mask = 0;
> +static uint64_t device_subslice_mask = 0;
> +static uint32_t device_slice_count = 0;
> +static uint32_t device_subslice_count = 0;

Any reason for using globals?

> +static uint64_t mask_minus_one(uint64_t mask)

minus/plus was making me think you were trying to do the equivalent of
mask += 1 etc.

I think mask_clear_first() and mask_set_next() would be clearer, and
I would just lose the _one variants and just pass N in everywhere.

> +{
> +       int i;
> +

mask_clear_first:
	bit = ffs(mask);
	igt_assert(bit);
	return mask & ~BIT(bit-1);

mask_set_next:
	bit = ffs(~mask);
	igt_assert(bit);
	return mask | BIT(bit-1);

?

> +static uint32_t
> +read_rpcs_reg(drm_intel_bufmgr *bufmgr,
> +             drm_intel_context *context,
> +             uint32_t expected_slices)
> +{
> +       struct intel_batchbuffer *batch;
> +       drm_intel_bo *dst_bo;

Bleurgh. Why the dependency on libdrm_intel?

Do you have to keep using libdrm_intel? For testing the kernel
interfaces, the obfuscation is not helpful.

> +static bool
> +platform_has_per_context_sseu_support(void)
> +{
> +       drm_intel_bufmgr *bufmgr;
> +       drm_intel_context *context;
> +       struct drm_i915_gem_context_param arg;
> +       struct drm_i915_gem_context_param_sseu sseu;
> +       uint32_t context_id;
> +       int ret;
> +
> +       bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> +       igt_assert(bufmgr);
> +
> +       context = drm_intel_gem_context_create(bufmgr);
> +       igt_assert(context);
> +
> +       ret = drm_intel_gem_context_get_id(context, &context_id);
> +       igt_assert_eq(ret, 0);
> +
> +       memset(&sseu, 0, sizeof(sseu));
> +       sseu.class = 0; /* rcs */
> +       sseu.instance = 0;
> +
> +       memset(&arg, 0, sizeof(arg));
> +       arg.ctx_id = context_id;

0 is a perfectly valid context, owned by the fd.

> +       arg.param = I915_CONTEXT_PARAM_SSEU;
> +       arg.value = (uintptr_t) &sseu;
> +
> +       if (igt_ioctl(drm_fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &arg))
> +               return false;
> +
> +       if (igt_ioctl(drm_fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &arg))
> +               return false;

gem_context_get_param/set_param?

> +static void
> +test_sseu_invalid_engine(void)
> +{
> +       drm_intel_bufmgr *bufmgr;
> +       drm_intel_context *context;
> +       struct drm_i915_gem_context_param arg;
> +       struct drm_i915_gem_context_param_sseu sseu;
> +       uint32_t context_id;
> +       int ret;
> +
> +       bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> +       igt_assert(bufmgr);
> +
> +       context = drm_intel_gem_context_create(bufmgr);
> +       igt_assert(context);
> +
> +       memset(&sseu, 0, sizeof(sseu));
> +
> +       ret = drm_intel_gem_context_get_id(context, &context_id);
> +       igt_assert_eq(ret, 0);
> +
> +       memset(&arg, 0, sizeof(arg));
> +       arg.ctx_id = context_id;
> +       arg.param = I915_CONTEXT_PARAM_SSEU;
> +       arg.value = (uintptr_t) &sseu;
> +
> +       sseu.class = 0xffff; /* invalid */
> +       sseu.instance = 0;
> +       do_ioctl_err(drm_fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &arg, EINVAL);

__gem_context_get_param

Never use do_ioctl or do_ioctl_err in new code. The resultant error
messages are a punishment upon the unfortunate reader.
-Chris