[Intel-gfx] [PATCH] drm/i915/selftests: Check for incomplete LRI from the context image
Gwan-gyeong Mun
gwan-gyeong.mun at intel.com
Tue Sep 13 14:17:00 UTC 2022
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 at i915_selftest@live at 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 at intel.com>
On 9/13/22 1:03 PM, Karolina Drobnik wrote:
> From: Chris Wilson <chris at 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 at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Signed-off-by: Karolina Drobnik <karolina.drobnik at 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(>->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--) {
More information about the Intel-gfx
mailing list