[PATCH i-g-t 1/1] lib/xe/xe_query: Wait for xe_supports_faults

[Cavitt, Jonathan]

-----Original Message-----
From: Kamil Konieczny <kamil.konieczny at linux.intel.com> 
Sent: Wednesday, May 8, 2024 9:24 AM
To: igt-dev at lists.freedesktop.org
Cc: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>; Welty, Brian <brian.welty at intel.com>; Mistat, Tomasz <tomasz.mistat at intel.com>; Girotra, Himanshu <himanshu.girotra at intel.com>
Subject: Re: [PATCH i-g-t 1/1] lib/xe/xe_query: Wait for xe_supports_faults
> 
> Hi Jonathan,
> On 2024-05-03 at 12:37:14 -0700, Jonathan Cavitt wrote:
> > It's possible for xe_supports_faults to return false if the system is
> > busy with multiple running tests.  This is because the check looks for
> > all active VMs and searches for VMs that do not have faults enabled,
> > returning false if any exist.  Recently, this check has been changed to
> > return EBUSY when the check fails in this way, so wait for up to ten
> > seconds for all the active VMs to flush out before proceeding.
> > 
> > Suggested-by: Brian Welty <brian.welty at intel.com>
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > ---
> >  lib/xe/xe_query.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> > index 6df8f42649..5458c73417 100644
> > --- a/lib/xe/xe_query.c
> > +++ b/lib/xe/xe_query.c
> > @@ -314,8 +314,13 @@ bool xe_supports_faults(int fd)
> >  		.flags = DRM_XE_VM_CREATE_FLAG_LR_MODE |
> >  			 DRM_XE_VM_CREATE_FLAG_FAULT_MODE,
> >  	};
> > -
> > -	supports_faults = !igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > +	struct timespec tv = {};
> > +	int result, timeout;
> > +	do {
> > +		result = igt_ioctl(fd, DRM_IOCTL_XE_VM_CREATE, &create);
> > +		supports_faults = !result;
> > +		timeout = igt_seconds_elapsed(&tv);
> > +	} while (result == -EBUSY && timeout < 10);
> -------------------------------- ^^^^^^^^^^^^
> 
> Waiting any number of seconds in library function is way too much,
> imho this is ok in test itself, not on lib.

Is the suggestion here that we should perform the wait "on" xe_supports_faults,
rather than "in" xe_supports_faults?  I.E. that we do something like this instead
in xe_exec_fault_mode.c:

        igt_fixture {
                bool supports_faults;
                struct timespec tv = {};
                fd = drm_open_driver(DRIVER_XE);
                do {
                        supports_faults = xe_supports_faults(fd);
                } while (!supports_faults && igt_seconds_elapsed(&tv) < 10);
                igt_require(supports_faults);
        }

I can do this, though if xe_supports_faults returns false for any non-EBUSY related
reasons, we won't be able to detect it from here and we'll spend ten seconds waiting
for xe_supports_faults to return true when it strictly cannot.  Is this an acceptable
tradeoff?
-Jonathan Cavitt

> 
> Regards,
> Kamil
> 
> >  
> >  	if (supports_faults)
> >  		xe_vm_destroy(fd, create.vm_id);
> > -- 
> > 2.25.1
> > 
>