[PATCH i-g-t 4/5] tests/intel/xe_pm: Only check the rpm resume after the first mmap operation

[Nilawar, Badal]

On 14-05-2024 20:49, Nilawar, Badal wrote:
> 
> 
> On 14-05-2024 00:25, Rodrigo Vivi wrote:
>> The very first memory operation on a mmaped region after runtime suspend,
>> the device will be resumed if the memory used is vram.
>> However, after this first operation with page fault, the memory
>> can be migrated to the system memory.
This probably due to xe_bo_evict_all() in D3 cold.
>>
>> During this migration, Xe kernel will get a notification at
>> xe_bo_move_notify, and the bo will be removed from the
>> vram_userfault list. Then, on the next suspend, we won't mark
>> bo for page fault and resume won't happen and the active count
>> won't increase. This is okay, since the bo is now in the system
>> memory we don't need to wake up the device. But the current
>> test is wrong, because it assumes that the bo keeps forever
>> in vram, what is the issue here.
Not completely wrong in D3 Hot.
>>
>> Only checking the resume after the first operation is the
>> enough. But also, ensure that both read and write operations
>> are actually validated.
Should we keep "first operation" check only for D3 cold and previous 
logic for D3 hot? May be combine patch 4 and 5 to handle this.

Regards,
Badal
>>
>> Cc: Badal Nilawar <badal.nilawar at intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta at intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> ---
>>   tests/intel/xe_pm.c | 36 +++++++++++++++++++++++++-----------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
>> index 72d484163..81226a910 100644
>> --- a/tests/intel/xe_pm.c
>> +++ b/tests/intel/xe_pm.c
>> @@ -37,6 +37,11 @@
>>   #define PREFETCH (0x1 << 1)
>>   #define UNBIND_ALL (0x1 << 2)
>> +enum mem_op {
>> +    READ,
>> +    WRITE,
>> +};
>> +
>>   typedef struct {
>>       int fd_xe;
>>       struct pci_device *pci_xe;
>> @@ -525,7 +530,8 @@ static void test_vram_d3cold_threshold(device_t 
>> device, int sysfs_fd)
>>    *
>>    * Functionality: pm-d3
>>    */
>> -static void test_mmap(device_t device, uint32_t placement, uint32_t 
>> flags)
>> +static void test_mmap(device_t device, uint32_t placement, uint32_t 
>> flags,
>> +              enum mem_op first_op)
>>   {
>>       size_t bo_size = 8192;
>>       uint32_t *map = NULL;
>> @@ -563,8 +569,12 @@ static void test_mmap(device_t device, uint32_t 
>> placement, uint32_t flags)
>>       
>> igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>>       active_time = igt_pm_get_runtime_active_time(device.pci_xe);
>> -    for (i = 0; i < bo_size / sizeof(*map); i++)
>> -        igt_assert(map[i] == MAGIC_1);
>> +    for (i = 0; i < bo_size / sizeof(*map); i++) {
>> +        if (first_op == READ)
>> +            igt_assert(map[i] == MAGIC_1);
>> +        else
>> +            map[i] = MAGIC_2;
>> +    }
>>       /* dgfx page-fault on mmaping should wake the gpu */
>>       if (xe_has_vram(device.fd_xe) && flags & 
>> DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
>> @@ -574,12 +584,12 @@ static void test_mmap(device_t device, uint32_t 
>> placement, uint32_t flags)
>>       
>> igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>>       active_time = igt_pm_get_runtime_active_time(device.pci_xe);
> Above line can be removed.
>> -    for (i = 0; i < bo_size / sizeof(*map); i++)
>> -        map[i] = MAGIC_2;
>> -
>> -    if (xe_has_vram(device.fd_xe) && flags & 
>> DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
>> -        igt_assert(igt_pm_get_runtime_active_time(device.pci_xe) >
>> -               active_time);
>> +    for (i = 0; i < bo_size / sizeof(*map); i++) {
>> +        if (first_op == READ)
>> +            map[i] = MAGIC_2;
>> +        else
>> +            igt_assert(map[i] == MAGIC_2);
>> +    }
>>       
>> igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
>> @@ -781,7 +791,8 @@ igt_main
>>                    "when device along with parent bridge in d3");
>>           igt_subtest("d3-mmap-system") {
>>               dpms_on_off(device, DRM_MODE_DPMS_OFF);
>> -            test_mmap(device, system_memory(device.fd_xe), 0);
>> +            test_mmap(device, system_memory(device.fd_xe), 0, READ);
>> +            test_mmap(device, system_memory(device.fd_xe), 0, WRITE);
>>               dpms_on_off(device, DRM_MODE_DPMS_ON);
>>           }
>> @@ -800,7 +811,10 @@ igt_main
>>               /* Give some auto suspend delay to validate rpm active 
>> during page fault */
>>               igt_pm_set_autosuspend_delay(device.pci_xe, 1000);
>>               dpms_on_off(device, DRM_MODE_DPMS_OFF);
>> -            test_mmap(device, vram_memory(device.fd_xe, 0), 
>> DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>> +            test_mmap(device, vram_memory(device.fd_xe, 0),
>> +                  DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM, READ);
>> +            test_mmap(device, vram_memory(device.fd_xe, 0),
>> +                  DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM, 
>> WRITE);With a comment this is
> Reviewed-by: Badal Nilawar <badal.nilawar at intel.com>
> 
> Regards,
> Badal
>>               dpms_on_off(device, DRM_MODE_DPMS_ON);
>>               igt_pm_set_autosuspend_delay(device.pci_xe, delay_ms);
>>           }