[PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display

[Kamil Konieczny]

Hi Sundaresan,,
On 2024-08-09 at 14:34:23 +0530, Sundaresan, Sujaritha wrote:
> 
> On 8/6/2024 6:20 PM, Kamil Konieczny wrote:
> > Hi Sundaresan,,
> > On 2024-08-06 at 11:50:46 +0530, Sundaresan, Sujaritha wrote:
> > > On 8/1/2024 5:15 PM, Kamil Konieczny wrote:
> > > > Hi Sujaritha,
> > > > On 2024-07-30 at 17:05:08 +0530, Sujaritha Sundaresan wrote:
> > > > 
> > > > small nit about subject, you wrote:
> > > > 
> > > > [PATCH i-g-t, v3] tests/intel: Add tests to run suspend without display
> > > > 
> > > > imho this should be:
> > > > 
> > > > [PATCH i-g-t, v3] tests/intel/xe_pm: Add tests for suspend without display
> > > > 
> > > > More nits below.
> > > Hey Kamil,
> > > 
> > > Sure this change I can make.
> > > 
> > > > > Add tests to validate basic execution suspend/resume cycle
> > > > > without display module to rule out display related issues
> > > > > from the suspend/resume stack.
> > > > > 
> > > > > v2: Add normal reload cycle after running test (Anshuman)
> > > > > 
> > > > > v3: Rebase
> > > > > 
> > > > > Signed-off-by: Sujaritha Sundaresan<sujaritha.sundaresan at intel.com>
> > > > > Reviewed-by: Anshuman Gupta<anshuman.gupta at intel.com>
> > > > > ---
> > > > >    tests/intel/xe_pm.c | 34 ++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 34 insertions(+)
> > > > > 
> > > > > diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> > > > > index 8b115e2f6..03f742265 100644
> > > > > --- a/tests/intel/xe_pm.c
> > > > > +++ b/tests/intel/xe_pm.c
> > > > > @@ -17,6 +17,7 @@
> > > > >    #include "igt.h"
> > > > >    #include "lib/igt_device.h"
> > > > > +#include "lib/igt_kmod.h"
> > > > >    #include "lib/igt_pm.h"
> > > > >    #include "lib/igt_sysfs.h"
> > > > >    #include "lib/igt_syncobj.h"
> > > > > @@ -229,6 +230,10 @@ static void close_fw_handle(int sig)
> > > > >     * Description: suspend/autoresume on %arg[1] state and exec after RPM
> > > > >     * Functionality: pm - %arg[1]
> > > > >     *
> > > > > + * SUBTEST: %s-without-display
> > > > > + * Description: suspend/autoresume on %arg[1] state without display
> > > > > + * Functionality: pm - %arg[1]
> > > > I see you copy-pasted it but imho both Description and
> > > > Functionality documentation fields should be static, here and
> > > > in other places.
> > > > +cc Katarzyna Piecielska<katarzyna.piecielska at intel.com>
> 
> Hi Kamil,
> 
> Sorry I didn't get this change. This is inline with the rest of the file
> right ?

Yes but this looks strange, so I added Katarzyna to discussion.

> 
> > > > 
> > > > > + *
> > > > >     * arg[1]:
> > > > >     *
> > > > >     * @s2idle:	s2idle
> > > > > @@ -681,6 +686,7 @@ igt_main
> > > > >    	struct drm_xe_engine_class_instance *hwe;
> > > > >    	device_t device;
> > > > >    	uint32_t d3cold_allowed;
> > > > > +	const char *opts;
> > > > >    	int sysfs_fd;
> > > > >    	const struct s_state {
> > > > > @@ -757,6 +763,34 @@ igt_main
> > > > >    					  NO_RPM, 0);
> > > > >    		}
> > > > > +		igt_subtest_f("%s-without-display", s->name) {
> > > > > +
> > > > > +			if (!drmModeGetResources(device.fd_xe))
> > > > > +				return;
> > > > Why 'return' here?! Imho this should be checked in fixture
> > > > or be a skip. Or other way around - what about a headless board
> > > > or one without any connected display?
> > > > 
> > > > Regards,
> > > > Kamil
> > > I think this patch idea sort stemmed from the cases where we have a display
> > > connected and
> > > 
> > > want to make sure that the suspend/resume issues are not being caused by the
> > > display.
> > > 
> > > But would you suggest expanding the test to have the headless/no display
> > > situations? If so what changes are you suggesting for that ?
> > > 
> > > Thanks,
> > > 
> > > Suja
> > > 
> > I would suggest turn this into a igt_skip_on_f(), not a return.
> > 
> > Regards,
> > Kamil
> 
> Sure I will switch this to
> 
> igt_skip_on(!drmModeGetResources(device.fd_xe))
> 
> Thanks,
> 
> Suja
> 

On the second look - can we just ignore this and proceed with test?
It should do no harm when you have no display connected?

Regards,
Kamil

> > > > > +
> > > > > +			xe_for_each_engine(device.fd_xe, hwe) {
> > > > > +
> > > > > +				igt_debug("Reload w/o display\n");
> > > > > +
> > > > > +				igt_kmsg(KMSG_INFO "Unloading Xe\n");
> > > > > +				igt_assert_eq(igt_xe_driver_unload(), 0);
> > > > > +
> > > > > +				igt_kmsg(KMSG_INFO "Re-loading Xe without display\n");
> > > > > +				igt_assert_eq(igt_xe_driver_load("enable_display=0"), 0);
> > > > > +
> > > > > +				test_exec(device, hwe, 1, 2, s->state,
> > > > > +					  NO_RPM, 0);
> > > > > +
> > > > > +				igt_debug("Reload as normal\n");
> > > > > +
> > > > > +				igt_kmsg(KMSG_INFO "Unloading Xe\n");
> > > > > +				igt_assert_eq(igt_xe_driver_unload(), 0);
> > > > > +
> > > > > +				igt_kmsg(KMSG_INFO "Re-loading Xe\n");
> > > > > +				igt_assert_eq(igt_xe_driver_load(opts), 0);
> > > > > +			}
> > > > > +		}
> > > > > +
> > > > >    		for (const struct vm_op *op = vm_op; op->name; op++) {
> > > > >    			igt_subtest_f("%s-vm-bind-%s", s->name, op->name) {
> > > > >    				xe_for_each_engine(device.fd_xe, hwe)
> > > > > -- 
> > > > > 2.34.1
> > > > >