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

[Zbigniew Kempczyński]

On Fri, Sep 27, 2024 at 01:27:18PM +0200, Manszewski, Christoph wrote:
> Hi Zbigniew,
> 
> On 27.09.2024 07:18, Zbigniew Kempczyński wrote:
> > On Thu, Sep 26, 2024 at 04:54:53PM +0200, Grzegorzek, Dominik wrote:
> > > On Thu, 2024-09-26 at 14:18 +0200, Dominik Karol Piątkowski wrote:
> > > > This patch introduces 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.
> > > > 
> > > 
> > > I think this should really go to igt_sysfs.c. And in sligtly different form:
> > > How about sth like:
> > 
> > We're compiling lib/xe_eudebug.c conditionally so I wouldn't pollute igt_sysfs.c
> > with eudebug code (at least yet).
> 
> I am a little bit confused by this comment as this is not eudebug specific
> code. The eudebug series did introduce some code to common/core igt library
> parts (like gpgpu_shader or intel_bachbuffer) which seemed to be fine (and
> rightly so IMO). I am not sure why this case is any different.

>From my pov:

1. changes in gpgpu_shader are related to shaders generation, they may be
used or not, but they don't assume eudebug is turned on in the linux kernel.
2. intel_batchbuffer changes (set + lr mode) also don't assume eudebug is in use
3. change in igt_sysfs assumes kernel supports eudebug, but we're still
on the way to it, so this file at the moment is not correct place imo.

As eudebug feature is big thing I would keep it separated as much as
possible. Create xe_eudebug_util.[ch] or whatever but keep it separated
but this is mine opinion and if someone else will convince me that
I'm wrong I can accept your changes.

--
Zbigniew

> 
> The only reason I could think of was that igt_sysfs.c might be for generic
> and not i915/Xe specific functions but from what I see, there is plenty
> i915/Xe specific stuff in there.
> 
> Thanks,
> Christoph
> 
> > 
> > --
> > Zbigniew
> > 
> > > 
> > > igt_sysfs_engine_set_property(engines_fd, "preempt_timeout_us", value);
> > > igt_sysfs_engine_set_property_bound(engines_fd, "preempt_timeout_us", max);
> > > 
> > > ,where max is a bool variable which tells whether it should be min or max.
> > > 
> > > To find engines_fd xe_sysfs_engine_open could be used.
> > > 
> > > Regards,
> > > Dominik
> > > 
> > > > Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski 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;
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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;
> > > > +}
> > > > +
> > > >   /* 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,
> > >