Re: [Intel-gfx] [PATCH 2/4] drm/i915/selftests: Check for incomplete LRI from the context image

["Thomas Hellström (Intel)" <thomas_os@shipmail.org>]

On 3/14/22 19:20, Ramalingam C wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> In order to keep the context image parser simple, we assume that all
> commands follow a similar format. A few, especially not MI commands on
> the render engines, have fixed lengths not encoded in a length field.
> This caused us to incorrectly skip over 3D state commands, and start
> interpretting context data as instructions. Eventually, as Daniele

interpreting


> discovered, this would lead us to find addition LRI as part of the data
> and mistakenly add invalid LRI commands to the context probes.
>
> Stop parsing after we see the first !MI command, as we know we will have
> seen all the context registers by that point. (Mostly true for all gen so far,
> though the render context does have LRI after the first page that we
> have been ignoring so far. It would be useful to extract those as well
> so that we have the full list of user accesisble registers.)
accessible
>
> Similarly, emit a warning if we do try to emit an invalid zero-length
> LRI.
>
> Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>

Acked-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>




> ---
>   drivers/gpu/drm/i915/gt/selftest_lrc.c | 61 +++++++++++++++++++++++---
>   1 file changed, 54 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 13f57c7c4224..0a8ed4246082 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -27,6 +27,9 @@
>   #define NUM_GPR 16
>   #define NUM_GPR_DW (NUM_GPR * 2) /* each GPR is 2 dwords */
>   
> +#define LRI_HEADER MI_INSTR(0x22, 0)
> +#define LRI_LENGTH_MASK GENMASK(7, 0)
> +
>   static struct i915_vma *create_scratch(struct intel_gt *gt)
>   {
>   	return __vm_create_scratch_for_read_pinned(&gt->ggtt->vm, PAGE_SIZE);
> @@ -180,7 +183,7 @@ static int live_lrc_layout(void *arg)
>   				continue;
>   			}
>   
> -			if ((lri & GENMASK(31, 23)) != MI_INSTR(0x22, 0)) {
> +			if ((lri & GENMASK(31, 23)) != LRI_HEADER) {
>   				pr_err("%s: Expected LRI command at dword %d, found %08x\n",
>   				       engine->name, dw, lri);
>   				err = -EINVAL;
> @@ -945,18 +948,40 @@ store_context(struct intel_context *ce, struct i915_vma *scratch)
>   	hw = defaults;
>   	hw += LRC_STATE_OFFSET / sizeof(*hw);
>   	do {
> -		u32 len = hw[dw] & 0x7f;
> +		u32 len = hw[dw] & LRI_LENGTH_MASK;
> +
> +		/*
> +		 * Keep it simple, skip parsing complex commands
> +		 *
> +		 * At present, there are no more MI_LOAD_REGISTER_IMM
> +		 * commands after the first 3D state command. Rather
> +		 * than include a table (see i915_cmd_parser.c) of all
> +		 * the possible commands and their instruction lengths
> +		 * (or mask for variable length instructions), assume
> +		 * we have gathered the complete list of registers and
> +		 * bail out.
> +		 */
> +		if ((hw[dw] >> INSTR_CLIENT_SHIFT) != INSTR_MI_CLIENT)
> +			break;
>   
>   		if (hw[dw] == 0) {
>   			dw++;
>   			continue;
>   		}
>   
> -		if ((hw[dw] & GENMASK(31, 23)) != MI_INSTR(0x22, 0)) {
> +		if ((hw[dw] & GENMASK(31, 23)) != LRI_HEADER) {
> +			/* Assume all other MI commands match LRI length mask */
>   			dw += len + 2;
>   			continue;
>   		}
>   
> +		if (!len) {
> +			pr_err("%s: invalid LRI found in context image\n",
> +			       ce->engine->name);
> +			igt_hexdump(defaults, PAGE_SIZE);
> +			break;
> +		}
> +
>   		dw++;
>   		len = (len + 1) / 2;
>   		while (len--) {
> @@ -1108,18 +1133,29 @@ static struct i915_vma *load_context(struct intel_context *ce, u32 poison)
>   	hw = defaults;
>   	hw += LRC_STATE_OFFSET / sizeof(*hw);
>   	do {
> -		u32 len = hw[dw] & 0x7f;
> +		u32 len = hw[dw] & LRI_LENGTH_MASK;
> +
> +		/* For simplicity, break parsing at the first complex command */
> +		if ((hw[dw] >> INSTR_CLIENT_SHIFT) != INSTR_MI_CLIENT)
> +			break;
>   
>   		if (hw[dw] == 0) {
>   			dw++;
>   			continue;
>   		}
>   
> -		if ((hw[dw] & GENMASK(31, 23)) != MI_INSTR(0x22, 0)) {
> +		if ((hw[dw] & GENMASK(31, 23)) != LRI_HEADER) {
>   			dw += len + 2;
>   			continue;
>   		}
>   
> +		if (!len) {
> +			pr_err("%s: invalid LRI found in context image\n",
> +			       ce->engine->name);
> +			igt_hexdump(defaults, PAGE_SIZE);
> +			break;
> +		}
> +
>   		dw++;
>   		len = (len + 1) / 2;
>   		*cs++ = MI_LOAD_REGISTER_IMM(len);
> @@ -1248,18 +1284,29 @@ static int compare_isolation(struct intel_engine_cs *engine,
>   	hw = defaults;
>   	hw += LRC_STATE_OFFSET / sizeof(*hw);
>   	do {
> -		u32 len = hw[dw] & 0x7f;
> +		u32 len = hw[dw] & LRI_LENGTH_MASK;
> +
> +		/* For simplicity, break parsing at the first complex command */
> +		if ((hw[dw] >> INSTR_CLIENT_SHIFT) != INSTR_MI_CLIENT)
> +			break;
>   
>   		if (hw[dw] == 0) {
>   			dw++;
>   			continue;
>   		}
>   
> -		if ((hw[dw] & GENMASK(31, 23)) != MI_INSTR(0x22, 0)) {
> +		if ((hw[dw] & GENMASK(31, 23)) != LRI_HEADER) {
>   			dw += len + 2;
>   			continue;
>   		}
>   
> +		if (!len) {
> +			pr_err("%s: invalid LRI found in context image\n",
> +			       engine->name);
> +			igt_hexdump(defaults, PAGE_SIZE);
> +			break;
> +		}
> +
>   		dw++;
>   		len = (len + 1) / 2;
>   		while (len--) {