[PATCH i-g-t 1/2] lib/xe_eudebug: Introduce preempt_timeout helpers

[Cavitt, Jonathan]

-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Dominik Karol Piatkowski
Sent: Thursday, September 26, 2024 5:19 AM
To: igt-dev at lists.freedesktop.org
Cc: Kempczynski, Zbigniew <zbigniew.kempczynski at intel.com>; Grzegorzek, Dominik <dominik.grzegorzek at intel.com>; Manszewski, Christoph <christoph.manszewski at intel.com>; Sikora, Pawel <pawel.sikora at intel.com>; Piatkowski, Dominik Karol <dominik.karol.piatkowski at intel.com>
Subject: [PATCH i-g-t 1/2] lib/xe_eudebug: Introduce preempt_timeout helpers
> 
> This patch introduces preempt_timeout helpers:

Nit:
IIRC we probably shouldn't refer to the patches/commits directly in the commit message.  Maybe something like this instead:

"""
Introduce the following preempt_timeout helpers:
"""

>  - xe_eudebug_set_preempt_timeout_to_max
>    Sets preempt_timeout_us for given engine_class to maximum.
>  - xe_eudebug_restore_preempt_timeout
>    Sets preempt_timeout_us for given engine_class to previous value.
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski at intel.com>

Some minor nits and non-blocking suggestions, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

> ---
>  lib/xe/xe_eudebug.c | 120 ++++++++++++++++++++++++++++++++++++++++++++
>  lib/xe/xe_eudebug.h |   3 ++
>  2 files changed, 123 insertions(+)
> 
> diff --git a/lib/xe/xe_eudebug.c b/lib/xe/xe_eudebug.c
> index 2c7cd2ae1..229f9e6a1 100644
> --- a/lib/xe/xe_eudebug.c
> +++ b/lib/xe/xe_eudebug.c
> @@ -16,6 +16,7 @@
>  #include "intel_pat.h"
>  #include "xe_eudebug.h"
>  #include "xe_ioctl.h"
> +#include "xe_drm.h"
>  
>  struct event_trigger {
>  	xe_eudebug_trigger_fn fn;
> @@ -1717,6 +1718,125 @@ bool xe_eudebug_enable(int fd, bool enable)
>  	return old;
>  }
>  
> +/**
> + * xe_eudebug_set_preempt_timeout_to_max
> + * @fd: xe client
> + * @engine_class: engine class to modify (drm_xe_engine_class_instance->engine_class)
> + * @value: pointer to variable to store previous preempt_timeout value in, needed for restore
> + *
> + * Sets preempt_timeout_us for given engine_class to maximum.
> + *
> + * Returns: true on success, false on failure.
> + */
> +bool xe_eudebug_set_preempt_timeout_to_max(int fd, uint16_t engine_class, uint32_t *value)
> +{
> +	int engines_fd;
> +	int gt_fd;
> +	uint32_t max_value;
> +
> +	igt_assert(engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
> +		   engine_class == DRM_XE_ENGINE_CLASS_COMPUTE);
> +
> +	gt_fd = xe_sysfs_gt_open(fd, 0);
> +	if (gt_fd == -1) {
> +		igt_debug("Failed to open gt.\n");
> +
> +		return false;
> +	}
> +
> +	if (engine_class == DRM_XE_ENGINE_CLASS_RENDER)
> +		engines_fd = openat(gt_fd, "engines/rcs", O_RDONLY);
> +	else
> +		engines_fd = openat(gt_fd, "engines/ccs", O_RDONLY);
> +
> +	if (engines_fd == -1) {
> +		igt_debug("Failed to open engine class.\n");
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_get_u32(engines_fd, "preempt_timeout_us", value)) {
> +		igt_debug("Failed to read preempt_timeout_us.\n");
> +		close(engines_fd);
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_get_u32(engines_fd, "preempt_timeout_max", &max_value)) {
> +		igt_debug("Failed to read preempt_timeout_max.\n");
> +		close(engines_fd);
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_set_u32(engines_fd, "preempt_timeout_us", max_value)) {
> +		igt_debug("Failed to write preempt_timeout_us.\n");
> +		close(engines_fd);
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	close(engines_fd);
> +	close(gt_fd);
> +
> +	return true;

Non-blocking:
We might be able to collapse some of these for branches using goto statements:

"""
	int engines_fd;
	int gt_fd;
	uint32_t max_value;
	bool ret = false;
...
	if (engines_fd == -1) {
		igt_debug("Failed to open engine class.\n");
		goto engine_err;
	}

	if (!__igt_sysfs_get_u32(engines_fd, "preempt_timeout_us", value)) {
		igt_debug("Failed to read preempt_timeout_us.\n");
		goto sysfs_err;
	}

	if (!__igt_sysfs_get_u32(engines_fd, "preempt_timeout_max", &max_value)) {
		igt_debug("Failed to read preempt_timeout_max.\n");
		goto sysfs_err;
	}

	if (!__igt_sysfs_set_u32(engines_fd, "preempt_timeout_us", max_value)) {
		igt_debug("Failed to write preempt_timeout_us.\n");
		goto sysfs_err;
	}

	ret = true;

sysfs_err:
	close(engines_fd);
engine_err:
	close(gt_fd);
	return ret;
"""

This isn't strictly necessary, just something to consider.

> +}
> +
> +/**
> + * xe_eudebug_restore_preempt_timeout
> + * @fd: xe client
> + * @engine_class: engine class to modify (drm_xe_engine_class_instance->engine_class)
> + * @value: previous preempt_timeout value
> + *
> + * Sets preempt_timeout_us for given engine_class to previous value.
> + *
> + * Returns: true on success, false on failure.
> + */
> +bool xe_eudebug_restore_preempt_timeout(int fd, uint16_t engine_class, uint32_t value)
> +{
> +	int engines_fd;
> +	int gt_fd;
> +
> +	igt_assert(engine_class == DRM_XE_ENGINE_CLASS_RENDER ||
> +		   engine_class == DRM_XE_ENGINE_CLASS_COMPUTE);
> +
> +	gt_fd = xe_sysfs_gt_open(fd, 0);
> +	if (gt_fd == -1) {
> +		igt_debug("Failed to open gt.\n");
> +
> +		return false;
> +	}
> +
> +	if (engine_class == DRM_XE_ENGINE_CLASS_RENDER)
> +		engines_fd = openat(gt_fd, "engines/rcs", O_RDONLY);
> +	else
> +		engines_fd = openat(gt_fd, "engines/ccs", O_RDONLY);
> +
> +	if (engines_fd == -1) {
> +		igt_debug("Failed to open engine class.\n");
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	if (!__igt_sysfs_set_u32(engines_fd, "preempt_timeout_us", value)) {
> +		igt_debug("Failed to write preempt_timeout_us.\n");
> +		close(engines_fd);
> +		close(gt_fd);
> +
> +		return false;
> +	}
> +
> +	close(engines_fd);
> +	close(gt_fd);
> +
> +	return true;

Non-blocking:
Similar to above, but there's less to collapse, so it's even less necessary.
-Jonathan Cavitt

> +}
> +
>  /* Eu debugger wrappers around resource creating xe ioctls. */
>  
>  /**
> diff --git a/lib/xe/xe_eudebug.h b/lib/xe/xe_eudebug.h
> index 29ab68fee..144035862 100644
> --- a/lib/xe/xe_eudebug.h
> +++ b/lib/xe/xe_eudebug.h
> @@ -167,6 +167,9 @@ void xe_eudebug_client_set_data(struct xe_eudebug_client *c, void *ptr);
>  
>  bool xe_eudebug_enable(int fd, bool enable);
>  
> +bool xe_eudebug_set_preempt_timeout_to_max(int fd, uint16_t engine_class, uint32_t *value);
> +bool xe_eudebug_restore_preempt_timeout(int fd, uint16_t engine_class, uint32_t value);
> +
>  int xe_eudebug_client_open_driver(struct xe_eudebug_client *c);
>  void xe_eudebug_client_close_driver(struct xe_eudebug_client *c, int fd);
>  uint32_t xe_eudebug_client_vm_create(struct xe_eudebug_client *c, int fd,
> -- 
> 2.34.1
> 
>