From: "Nilawar, Badal" <badal.nilawar@intel.com>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>, <igt-dev@lists.freedesktop.org>
Cc: <intel-xe@lists.freedesktop.org>,
Anshuman Gupta <anshuman.gupta@intel.com>
Subject: Re: [PATCH i-g-t 4/5] tests/intel/xe_pm: Only check the rpm resume after the first mmap operation
Date: Tue, 14 May 2024 22:09:00 +0530 [thread overview]
Message-ID: <ef48d5cb-7ddd-4c88-b0ef-d1def71aafd0@intel.com> (raw)
In-Reply-To: <c33614a5-2e89-427f-8e8e-6126ef728901@intel.com>
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@intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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@intel.com>
>
> Regards,
> Badal
>> dpms_on_off(device, DRM_MODE_DPMS_ON);
>> igt_pm_set_autosuspend_delay(device.pci_xe, delay_ms);
>> }
next prev parent reply other threads:[~2024-05-14 16:39 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-13 18:55 [PATCH i-g-t 1/5] tests/intel/xe_pm: Update runtime pm conditions Rodrigo Vivi
2024-05-13 18:55 ` [PATCH i-g-t 2/5] lib/igt_pm: Fix and standardize IGT PM library documentation Rodrigo Vivi
2024-05-14 6:49 ` Riana Tauro
2024-05-13 18:55 ` [PATCH i-g-t 3/5] tests/intel/xe_pm: Also disable display for mmap_system test Rodrigo Vivi
2024-05-14 6:12 ` Gupta, Anshuman
2024-05-14 6:46 ` Nilawar, Badal
2024-05-13 18:55 ` [PATCH i-g-t 4/5] tests/intel/xe_pm: Only check the rpm resume after the first mmap operation Rodrigo Vivi
2024-05-14 15:19 ` Nilawar, Badal
2024-05-14 16:39 ` Nilawar, Badal [this message]
2024-05-13 18:55 ` [PATCH i-g-t 5/5] tests/intel/xe_pm: Convert mmap tests to use existing d3 helpers Rodrigo Vivi
2024-05-13 19:01 ` ✗ CI.Patch_applied: failure for series starting with [i-g-t,1/5] tests/intel/xe_pm: Update runtime pm conditions Patchwork
2024-05-13 20:03 ` Rodrigo Vivi
2024-05-16 16:49 ` [PATCH i-g-t 1/5] " Francois Dugast
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ef48d5cb-7ddd-4c88-b0ef-d1def71aafd0@intel.com \
--to=badal.nilawar@intel.com \
--cc=anshuman.gupta@intel.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=rodrigo.vivi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).