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

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

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@intel.com>
>>> Sent: Wednesday, January 17, 2024 8:52 PM
>>> To: Kamil Konieczny <kamil.konieczny@linux.intel.com>; igt-
>>> dev@lists.freedesktop.org; Gupta, Anshuman <anshuman.gupta@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@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 */
>              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

> 
> 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
>>>>>