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

["Summers, Stuart" <stuart.summers@intel.com>]

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

> -                 .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?

> +                       }
> +                       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.

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);
>         }
>