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

[Daniele Ceraolo Spurio]

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 at 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
>