RE: [PATCH] tests/intel/xe_configfs: Use available engine for ctx-restore subtests

Gote, Nitin R
2026-05-04T07:34:46Z

Hi Stuart,

> -----Original Message-----
> From: Summers, Stuart <stuart.summers@intel.com>
> Sent: Thursday, April 30, 2026 1:48 AM
> To: igt-dev@lists.freedesktop.org; Gote, Nitin R <nitin.r.gote@intel.com>;
> kamil.konieczny@linux.intel.com
> Cc: marcin.bernatowicz@linux.intel.com
> Subject: Re: [PATCH] tests/intel/xe_configfs: Use available engine for ctx-restore
> subtests
>
> On Wed, 2026-04-29 at 11:45 +0530, Nitin Gote wrote:
> > The ctx-restore-post-bb and ctx-restore-mid-bb subtests were hardcoded
> > to use the "rcs" engine class. Platforms without a render engine (e.g.
> > PVC, CRI) fail because the batch buffer is never injected into any
> > engine's LRC, causing register readback to fail.
> >
> > Fix by dynamically detecting an available engine in the fixture using
> > xe_for_each_engine(). Prefer render ("rcs") when available, otherwise
> > fall back to compute ("ccs"). Every xe platform has at least one of
> > these engine classes. Remove the hardcoded "rcs" from test data and
> > add the detected engine name at runtime.
> >
> > v2: Add igt_require_f(engine, "explanation\n") (Kamil)
> >
> > Fixes: b6abdc26e01b ("tests/intel/xe_configfs: Check
> > ctx_restore_post_bb")
> > Cc: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> > Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> > ---
> >  tests/intel/xe_configfs.c | 64 +++++++++++++++++++++++++++----------
> > --
> >  1 file changed, 44 insertions(+), 20 deletions(-)
> >
> > diff --git a/tests/intel/xe_configfs.c b/tests/intel/xe_configfs.c
> > index 755524e7c..f03cf820f 100644
> > --- a/tests/intel/xe_configfs.c
> > +++ b/tests/intel/xe_configfs.c
> > @@ -261,11 +261,12 @@ static void test_ctx_restore_invalid(int
> > configfs_device_fd, const char *type)
> >   * SUBTEST: ctx-restore-mid-bb
> >   * Description: Validate ctx_restore_mid_bb attribute
> >   */
> > -static void test_ctx_restore(int configfs_device_fd, const char
> > *type)
> > +static void test_ctx_restore(int configfs_device_fd, const char
> > *type,
> > +                            const char *engine)
> >  {
> >         static const struct value {
> >                 const char *test;
> > -               const char *in;
> > +               const char *in[3];
> >                 const char *out;
> >                 uint32_t reg[4];
> >                 uint32_t reg_val[4];
> > @@ -276,41 +277,46 @@ static void test_ctx_restore(int
> > configfs_device_fd, const char *type)
> >                  * previous execution set a specific value in the HW
> >                  */
> >                 { .test = "cmd-single",
> > -                 .in = "rcs cmd 11000001 4F100 DEA0BEE0",
>
> In the for loop that appends the array elements below for in[], I see you're always
> appending a \n after each element. But in the original code there is only a \n after
> the first - and in the case of a single line, there is no line break as you can see just
> above here.
>
> Ok.. looking in the driver code it seems like we skip over any
> spaces/tabs/newlines before parsing the instructions. So I agree probably better
> to be consistent and add the same format to both of these...
>

Yes, the kernel parser parse_wa_bb_lines() skips newlines/spaces/tabs between commands,
so the trailing \n is harmless. It keeps the loop simpler and matches the style other subtests already use.

> > -                 .out = "rcs: 11000001 0004f100 dea0bee0\n",
> > +                 .in = { "cmd 11000001 4F100 DEA0BEE0" },
> > +                 .out = "11000001 0004f100 dea0bee0",
> >                   .reg = { 0x4f100 },
> >                   .reg_val = { 0xdea0bee0 },
> >                 },
> >                 { .test = "cmd-single-multi-values",
> > -                 .in = "rcs cmd 11000003 4F100 DEA1BEE1 4F104
> > DEA2BEE2",
> > -                 .out = "rcs: 11000003 0004f100 dea1bee1 0004f104
> > dea2bee2\n",
> > +                 .in = { "cmd 11000003 4F100 DEA1BEE1 4F104
> > DEA2BEE2" },
> > +                 .out = "11000003 0004f100 dea1bee1 0004f104
> > dea2bee2",
> >                   .reg = { 0x4f100, 0x4f104 },
> >                   .reg_val = { 0xdea1bee1, 0xdea2bee2 },
> >                 },
> >                 { .test = "cmd-multi",
> > -                 .in = "rcs cmd 11000001 4F100 DEA3BEE3\n"
> > -                       "rcs cmd 11000001 4F104 DEA4BEE4",
> > -                 .out = "rcs: 11000001 0004f100 dea3bee3 11000001
> > 0004f104 dea4bee4\n",
> > +                 .in = { "cmd 11000001 4F100 DEA3BEE3",
> > +                         "cmd 11000001 4F104 DEA4BEE4" },
> > +                 .out = "11000001 0004f100 dea3bee3 11000001
> > 0004f104 dea4bee4",
> >                   .reg = { 0x4f100, 0x4f104 },
> >                   .reg_val = { 0xdea3bee3, 0xdea4bee4 },
> >                 },
> >                 { .test = "reg-single",
> > -                 .in = "rcs reg 4F100 DEA5BEE5",
> > -                 .out = "rcs: 11000001 0004f100 dea5bee5\n",
> > +                 .in = { "reg 4F100 DEA5BEE5" },
> > +                 .out = "11000001 0004f100 dea5bee5",
> >                   .reg = { 0x4f100 },
> >                   .reg_val = { 0xdea5bee5 },
> >                 },
> >                 { .test = "reg-multi",
> > -                 .in = "rcs reg 4F100 DEA6BEE6\n"
> > -                       "rcs reg 4F104 DEA7BEE7",
> > -                 .out = "rcs: 11000001 0004f100 dea6bee6 11000001
> > 0004f104 dea7bee7\n",
> > +                 .in = { "reg 4F100 DEA6BEE6",
> > +                         "reg 4F104 DEA7BEE7" },
> > +                 .out = "11000001 0004f100 dea6bee6 11000001
> > 0004f104 dea7bee7",
> >                   .reg = { 0x4f100, 0x4f104 },
> >                   .reg_val = { 0xdea6bee6, 0xdea7bee7 },
> >                 },
> >         };
> > +       char in[4096] = { };
> > +       char out[4096] = { };
> >         char buf[4096] = { };
> >         char file[64] = { };
> >
> > +       igt_require_f(engine,
> > +                     "No render or compute engine available for ctx-
> > restore test\n");
> > +
> >         snprintf(file, sizeof(file), "ctx_restore_%s_bb", type);
> >
> >         for (size_t i = 0; i < ARRAY_SIZE(values); i++) { @@ -319,14
> > +325,19 @@ static void test_ctx_restore(int configfs_device_fd, const
> > char *type)
> >                 igt_audio_driver_unload(NULL);
> >                 igt_kmod_unbind("xe", bus_addr);
> >
> > +               for (int j = 0, off = 0; v->in[j]; j++)
> > +                       off += snprintf(in + off, sizeof(in) - off,
> > "%s %s\n",
> > +                                       engine, v->in[j]);
> > +               snprintf(out, sizeof(out), "%s: %s\n", engine, v-
> > >out);
> > +
> >                 igt_info("Test %s\n", v->test);
> > -               igt_debug("bb '%s'\n", v->in);
> > -               igt_assert(igt_sysfs_set(configfs_device_fd, file, v-
> > >in));
> > +               igt_debug("bb '%s'\n", in);
> > +               igt_assert(igt_sysfs_set(configfs_device_fd, file,
> > in));
> >
> >                 igt_assert(igt_sysfs_read(configfs_device_fd, file,
> > buf,
> >                                           sizeof(buf) - 1));
> > -               if (strcmp(v->out, buf)) {
> > -                       igt_debug("Expecting '%s' but found '%s'\n",
> > v->out, buf);
> > +               if (strcmp(out, buf)) {
> > +                       igt_debug("Expecting '%s' but found '%s'\n",
> > out, buf);
> >                         igt_fail(IGT_EXIT_FAILURE);
> >                 }
> >
> > @@ -364,12 +375,25 @@ int igt_main()
> >         int fd, configfs_fd, configfs_device_fd;
> >         uint32_t devid;
> >         bool is_vf_device;
> > +       const char *engine = NULL;
> >
> >         igt_fixture() {
> > +               struct drm_xe_engine_class_instance *hwe;
> > +
> >                 fd = drm_open_driver(DRIVER_XE);
> >                 devid = intel_get_drm_devid(fd);
> >                 is_vf_device = intel_is_vf_device(fd);
> >                 set_bus_addr(fd);
> > +
> > +               xe_for_each_engine(fd, hwe) {
> > +                       if (hwe->engine_class ==
> > DRM_XE_ENGINE_CLASS_RENDER) {
> > +                               engine = "rcs";
> > +                               break;
>
> Is there a reason we aren't just calling this for all supported engines?
>

The intent of this patch was just to unblock platforms without RCS (PVC/CRI).
Every xe platform has at least one of RCS or CCS, so covering these two is sufficient to unblock platforms for now.
Iterating over all supported engine classes would be a useful coverage enhancement,
but seems like a separate scope. Happy to do it as a separate patch if you'd prefer.

> > +                       }
> > +                       if (!engine && hwe->engine_class ==
> > DRM_XE_ENGINE_CLASS_COMPUTE)
> > +                               engine = "ccs";
>
> Seems safer to have a break here too.. I realize we're stepping through the
> classes sequentially and rcs comes before ccs, but in the rare event that we
> decide to change that for some reason, having the break here would at least add
> consistency. Worst case of course we'd just move back to rcs for platforms that
> have rcs and ccs.. so not horrible in the current implementation, but I still suggest
> adding it.
>

Sure, I'll add the break after the CCS in v3.

Thank you for the review,
- Nitin

> Thanks,
> Stuart
>
> > +               }
> > +
> >                 drm_close_driver(fd);
> >
> >                 configfs_fd = igt_configfs_open("xe"); @@ -419,7
> > +443,7 @@ int igt_main()
> >         igt_subtest("ctx-restore-post-bb") {
> >                 igt_skip_on_f(is_vf_device, "MMIO register readback
> > not possible on VF\n");
> >                 configfs_device_fd =
> > create_device_configfs_group(configfs_fd);
> > -               test_ctx_restore(configfs_device_fd, "post");
> > +               test_ctx_restore(configfs_device_fd, "post", engine);
> >                 close_configfs_group(configfs_fd, configfs_device_fd);
> >         }
> >
> > @@ -435,7 +459,7 @@ int igt_main()
> >         igt_subtest("ctx-restore-mid-bb") {
> >                 igt_skip_on_f(is_vf_device, "MMIO register readback
> > not possible on VF\n");
> >                 configfs_device_fd =
> > create_device_configfs_group(configfs_fd);
> > -               test_ctx_restore(configfs_device_fd, "mid");
> > +               test_ctx_restore(configfs_device_fd, "mid", engine);
> >                 close_configfs_group(configfs_fd, configfs_device_fd);
> >         }
> >