Re: [Intel-gfx] [PATCH] drm/i915/selftests: Fix selftest_mocs for DGFX

Daniele Ceraolo Spurio
2020-02-14T19:36:20Z

On 2/14/20 10:29 AM, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2020-02-14 17:56:58)
>>
>>
>> On 2/12/20 4:49 PM, Brian Welty wrote:
>>>
>>> On 2/12/2020 4:34 PM, Chris Wilson wrote:
>>>> Quoting Brian Welty (2020-02-13 00:14:18)
>>>>> For DGFX devices, the MOCS control value is not initialized or used.
>>>>
>>>> Then why is the table populated?
>>>> -Chris
>>>>
>>>
>>> The format has changed (been reduced?) for DGFX. drm_i915_mocs_entry.l3cc_value is what is still initialized/used.
>>> Probably first needed is the patch that defines the table entries for DGFX.
>>> Ugh, I didn't notice this wasn't applied yet. Let me ask about this.
>>>
>>
>> We do have:
>>
>> commit e6e2ac07118b15f25683fcbd59ea1be73ec9465d
>> Author: Lucas De Marchi <lucas.demarchi@intel.com>
>> Date: Thu Oct 24 12:51:21 2019 -0700
>>
>> drm/i915: do not set MOCS control values on dgfx
>>
>> So I see no reason not to add this change to the test side to match
>> that. Maybe we can add an additional check in the test to validate that
>> all the control_entries are set to 0 in the table on DGFX?
>
> My expectation was that as we were not setting mocs values, we would not
> have defined a table for it. However, the table is combined for mocs and
> l3cc. l3cc is still used, right?
>

yes, l3cc is still used. The diff below looks ok to me to keep the
table-driven approach.

Daniele

> My ideal would be that our tables did remain the truth value we could
> use directly -- that would require splitting the tables though.
>
> If we did something like
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_mocs.c b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> index de1f83100fb6..2c636257f12c 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> @@ -12,7 +12,8 @@
> #include "selftests/igt_spinner.h"
>
> struct live_mocs {
> - struct drm_i915_mocs_table table;
> + struct drm_i915_mocs_table mocs;
> + struct drm_i915_mocs_table l3cc;
> struct i915_vma *scratch;
> void *vaddr;
> };
> @@ -68,13 +69,32 @@ static struct i915_vma *create_scratch(struct intel_gt *gt)
> return vma;
> }
>
> +static bool has_l3cc(struct drm_i915_private *i915)
> +{
> + return true;
> +}
> +
> +static bool has_mocs(struct drm_i915_private *i915)
> +{
> + return !IS_DGFX(i915);
> +}
> +
> static int live_mocs_init(struct live_mocs *arg, struct intel_gt *gt)
> {
> + struct drm_i915_mocs_table table;
> int err;
>
> - if (!get_mocs_settings(gt->i915, &arg->table))
> + memset(arg, 0, sizeof(*arg));
> +
> + if (!get_mocs_settings(gt->i915, &table))
> return -EINVAL;
>
> + if (has_l3cc(gt->i915))
> + arg->l3cc = table;
> +
> + if (has_mocs(gt->i915))
> + arg->mocs = table;
> +
> arg->scratch = create_scratch(gt);
> if (IS_ERR(arg->scratch))
> return PTR_ERR(arg->scratch);
> @@ -223,9 +243,9 @@ static int check_mocs_engine(struct live_mocs *arg,
> /* Read the mocs tables back using SRM */
> offset = i915_ggtt_offset(vma);
> if (!err)
> - err = read_mocs_table(rq, &arg->table, &offset);
> + err = read_mocs_table(rq, &arg->mocs, &offset);
> if (!err && ce->engine->class == RENDER_CLASS)
> - err = read_l3cc_table(rq, &arg->table, &offset);
> + err = read_l3cc_table(rq, &arg->l3cc, &offset);
> offset -= i915_ggtt_offset(vma);
> GEM_BUG_ON(offset > PAGE_SIZE);
>
> @@ -236,9 +256,9 @@ static int check_mocs_engine(struct live_mocs *arg,
> /* Compare the results against the expected tables */
> vaddr = arg->vaddr;
> if (!err)
> - err = check_mocs_table(ce->engine, &arg->table, &vaddr);
> + err = check_mocs_table(ce->engine, &arg->mocs, &vaddr);
> if (!err && ce->engine->class == RENDER_CLASS)
> - err = check_l3cc_table(ce->engine, &arg->table, &vaddr);
> + err = check_l3cc_table(ce->engine, &arg->l3cc, &vaddr);
> if (err)
> return err;
>
>
> we could retain the table driven approach?
> -Chris
>