[i-g-t] tests/intel/i915_suspend: Fix WARNs of invalid fd

[Kamil Konieczny]

Hi Modem,,
On 2024-01-24 at 13:46:20 +0530, Modem, Bhanuprakash wrote:
> Hi Kamil/Anshuman,
> 
> On 19-01-2024 07:02 pm, Kamil Konieczny wrote:
> > Hi Gupta,,
> > On 2024-01-18 at 04:44:59 +0000, Gupta, Anshuman wrote:
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
> > > > Sent: Wednesday, January 17, 2024 8:52 PM
> > > > To: Kamil Konieczny <kamil.konieczny at linux.intel.com>; igt-
> > > > dev at lists.freedesktop.org; Gupta, Anshuman <anshuman.gupta at intel.com>
> > > > Subject: Re: [i-g-t] tests/intel/i915_suspend: Fix WARNs of invalid fd
> > > > 
> > > > Hi Kamil,
> > > > 
> > > > On 17-01-2024 04:42 pm, Kamil Konieczny wrote:
> > > > > Hi Bhanuprakash,
> > > > > On 2024-01-09 at 18:48:02 +0530, Bhanuprakash Modem wrote:
> > > > > > Call drm_close_driver() only if fd >=0, otherwise it'll will throw WARN:
> > > > > > 
> > > > > > (i915_suspend:7563) drmtest-WARNING: Don't attempt to close
> > > > > > standard/invalid file descriptor: -1
> > > > > > 
> > > > > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > > > > > ---
> > > > > >    tests/intel/i915_suspend.c | 4 +++-
> > > > > >    1 file changed, 3 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/tests/intel/i915_suspend.c b/tests/intel/i915_suspend.c
> > > > > > index c25805584..82cabfa41 100644
> > > > > > --- a/tests/intel/i915_suspend.c
> > > > > > +++ b/tests/intel/i915_suspend.c
> > > > > > @@ -300,7 +300,9 @@ test_suspend_without_i915(int state)
> > > > > >    		igt_pm_get_d3cold_allowed(card.pci_slot_name,
> > > > &d3cold_allowed);
> > > > > >    		igt_pm_set_d3cold_allowed(card.pci_slot_name, 0);
> > > > > >    	}
> > > > > > -	drm_close_driver(fd);
> > > > > 
> > > > > imho proper fix would be remove this and
> > > > > fd=__drm_open_driver(DRIVER_INTEL)
> > > > > from this function, as function is named:
> > > > > test_suspend_without_i915
> > > > 
> > > > I don't have much expertise in D states. After reading the comments in the
> > > > subtest "When module is unloaded and s2idle is triggered, PCI core leaves the
> > > > endpoint in D0 and the bridge in D3 state causing PCIE spec violation and
> > > > config space is read as 0xFF. Keep the bridge in D0 before module unload to
> > > > prevent this issue"
> > > This is not a real time use case, either a system boots with i915 driver or not.
> > > Removing i915 module and trigger suspend is not a valid use case but we want to test that
> > > suspend coverage without i915. So we need to disallow d3cold(which will bring the pcie bridge to D0).
> > > igt_pm_set_d3cold_allowed(card.pci_slot_name, 0);
> > > ***above is applicable tp  discrete card.
> > 
> > So this code should look like:
> > 
> > if (state == SUSPEND_STATE_FREEZE) {
> >      igt_devices_scan(false);
> >      if (igt_device_find_first_i915_discrete_card(&card)) {
> >              fd = __drm_open_driver(DRIVER_INTEL); /* write comment why we need this */

Or something like: if(!module_loaded(DRIVER_INTEL))
                    module_load();

> >              igt_pm_get_d3cold_allowed(card.pci_slot_name, &d3cold_allowed);
> >              igt_pm_set_d3cold_allowed(card.pci_slot_name, 0);
> >              drm_close_driver(fd);
> >      }
> > }
> > 
> > But this will fix problem on iGPU but still doesn't answer why
> > __drm_open_driver fails... as in dmesg from fails I see i915
> > was loaded/unloaded successfully.
> 
> I think, we can fix this as a separate activity. Meanwhile, shall we merge
> this patch?
> 
> - Bhanu
> 

Yes, we can discuss that later, you can add my r-b to your
original solution,

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

> > 
> > Regards,
> > Kamil
> > 
> > > Thanks,
> > > Anshuman Gupta
> > > > 
> > > > IMHO, still this patch is a valid fix. Perhaps, we can drop
> > > > drm_close_driver() and use close().
> > > > 
> > > > + Anshuman to comment
> > > > 
> > > > - Bhanu
> > > > 
> > > > > 
> > > > > we are expecting i915 driver to be _unloaded_.
> > > > > 
> > > > > Regards,
> > > > > Kamil
> > > > > 
> > > > > > +
> > > > > > +	if (fd >= 0)
> > > > > > +		drm_close_driver(fd);
> > > > > > 
> > > > > >    	igt_kmsg(KMSG_INFO "Unloading i915\n");
> > > > > >    	igt_assert_eq(igt_i915_driver_unload(),0);
> > > > > > --
> > > > > > 2.40.0
> > > > > >