[PATCH i-g-t v2] tests/intel/xe_exec_reset: Add mocs reset test

Matt Roper matthew.d.roper at intel.com
Thu Nov 7 00:38:58 UTC 2024


On Fri, Nov 01, 2024 at 09:14:32PM +0530, janga.rahul.kumar at intel.com wrote:
> From: Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> 
> Check mocs configuration over GT reset.
> 
> v2: Address review comments. (Matt Roper)

Not a blocker, but in the future, it's better to say what actually
changed in the new version of the patch.  Quite often no one (including
the original reviewer) remembers exactly what the previous review
comments were so this vague statement doesn't really tell us anything
useful.  Changelogs like this also leave the question of whether _all_
the review comments were addressed or whether some were omitted.

> 
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: Janga Rahul Kumar <janga.rahul.kumar at intel.com>
> ---
>  tests/intel/xe_exec_reset.c | 43 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/tests/intel/xe_exec_reset.c b/tests/intel/xe_exec_reset.c
> index 43ef1e334..953b5be28 100644
> --- a/tests/intel/xe_exec_reset.c
> +++ b/tests/intel/xe_exec_reset.c
> @@ -715,6 +715,45 @@ gt_reset(int fd, int n_threads, int n_sec)
>  	free(threads);
>  }
>  
> +/**
> + * SUBTEST: gt-mocs-reset
> + * Description: Validate mocs register contents over GT reset
> + * Test category: mocs
> + * Functionality: mocs

I don't know if there are firm rules on how this IGT documentation is
supposed to work, but in my mind, "Functionality: reset" seems like it
would be more appropriate than "Functionality: mocs."  We're not really
testing whether the driver's MOCS settings are correct, just that the
driver's reset flows successfully perform the same MOCS setup that
happened during driver init.  Do we need to even specify a
"Functionality:" for subtests when they're the same as the encompassing
test?

> + *
> + */
> +static void
> +gt_mocs_reset(int fd, int gt)
> +{
> +	char path[256];
> +
> +	/* Mocs debugfs contents before and after suspend-resume.

This isn't a suspend-resume, it's a GT reset.

Aside from the comment fix here, the test logic looks fine, so with that
corrected,

        Reviewed-by: Matt Roper <matthew.d.roper at intel.com>


Matt

> +	 * Allocate memory to store 10k characters sufficient enough
> +	 * to store global mocs and lncf mocs data.
> +	 */
> +	char *mocs_content_pre = (char *)malloc(10000 * sizeof(char));
> +	char *mocs_contents_post = (char *)malloc(10000 * sizeof(char));
> +
> +	igt_assert(mocs_content_pre);
> +	igt_assert(mocs_contents_post);
> +
> +	sprintf(path, "gt%d/mocs", gt);
> +	igt_assert(igt_debugfs_exists(fd, path, O_RDONLY));
> +	igt_debugfs_dump(fd, path);
> +	igt_debugfs_read(fd, path, mocs_content_pre);
> +
> +	xe_force_gt_reset_sync(fd, gt);
> +
> +	igt_assert(igt_debugfs_exists(fd, path, O_RDONLY));
> +	igt_debugfs_dump(fd, path);
> +	igt_debugfs_read(fd, path, mocs_contents_post);
> +
> +	igt_assert(strcmp(mocs_content_pre, mocs_contents_post) == 0);
> +
> +	free(mocs_content_pre);
> +	free(mocs_contents_post);
> +}
> +
>  igt_main
>  {
>  	struct drm_xe_engine_class_instance *hwe;
> @@ -820,6 +859,10 @@ igt_main
>  	igt_subtest("gt-reset-stress")
>  		gt_reset(fd, 4, 1);
>  
> +	igt_subtest("gt-mocs-reset")
> +		xe_for_each_gt(fd, gt)
> +			gt_mocs_reset(fd, gt);
> +
>  	igt_fixture
>  		drm_close_driver(fd);
>  }
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation


More information about the igt-dev mailing list