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

[Lionel Landwerlin]

On 24/05/18 16:23, Chris Wilson wrote:
> 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);
>
> ?

It's not that simple :(

Here is the subslice mask of my BXT 2x6 : 0b110
So I think stuff can get fused off anywhere.

>
>> +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.

I'm just being lazy...
I thought doing stuff from scratch would be a fair amount of additional 
code.
Will update.

>
>> +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.

Oh! Nice, will use that.

>
>> +       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?

Thanks.

>
>> +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.

Thanks, will try to remember that.

> -Chris
>