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

[Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>]

Hi Karolina,

This patch had previously been sent to the mailing list by Ram, then an 
update to the commit message and an ack from Thomas. therefore I think 
that should be included.
https://patchwork.freedesktop.org/series/101353/
https://patchwork.freedesktop.org/patch/484356/?series=101353&rev=6

And since this patch fixes the following issues, and there is a related 
test case, could you gently add these to the commit message as well?

Testcase: igt@i915_selftest@live@gt_lrc
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6580
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6670

others looks good to me.

Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>

On 9/13/22 1:03 PM, Karolina Drobnik 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
> interpreting context data as instructions. Eventually, as Daniele
> 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.)
> 
> 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: Karolina Drobnik <karolina.drobnik@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 1109088fe8f6..954a1c5c10ef 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);
> @@ -202,7 +205,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;
> @@ -987,18 +990,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--) {
> @@ -1150,18 +1175,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);
> @@ -1292,18 +1328,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--) {