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

[Kamil Konieczny]

Hi,
On 2024-05-08 at 16:43:04 +0000, Cavitt, Jonathan wrote:
> -----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
> 

If you are doing this just after igt_main you do not expect
it will return EBUSY. Other way would be to return errno from
lib function and let subtest decide if it want to wait.
There you could also account for simulation, where it can
take longer. Btw is there any sysfs param for it?

Regards,
Kamil

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