[PATCH] [i-g-t] tests/amdgpu/amd_ilr: Fix eDP PSR not be re-enabled after test
Wu, Hersen
hersenxs.wu at amd.com
Mon Apr 1 23:31:01 UTC 2024
[Public]
Hi Wayne,
Thank for your review. See my reply inline.
I am going to post new change for review.
Hesen
-----Original Message-----
From: Lin, Wayne <Wayne.Lin at amd.com>
Sent: Wednesday, March 27, 2024 11:59 PM
To: Wu, Hersen <hersenxs.wu at amd.com>; igt-dev at lists.freedesktop.org; Siqueira, Rodrigo <Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo <Aurabindo.Pillai at amd.com>; Hung, Alex <Alex.Hung at amd.com>; Mahfooz, Hamza <Hamza.Mahfooz at amd.com>; Li, Sun peng (Leo) <Sunpeng.Li at amd.com>
Cc: markyacoub at google.com; Wu, Hersen <hersenxs.wu at amd.com>
Subject: RE: [PATCH] [i-g-t] tests/amdgpu/amd_ilr: Fix eDP PSR not be re-enabled after test
[Public]
Hi Hersen,
Add few comments inline. On the other hand, would be good to add the version in the subject and also to record changes made by each version.
Thanks.
Regards,
Wayne Lin
> -----Original Message-----
> From: Hersen Wu <hersenxs.wu at amd.com>
> Sent: Monday, March 25, 2024 9:19 PM
> To: igt-dev at lists.freedesktop.org; Siqueira, Rodrigo
> <Rodrigo.Siqueira at amd.com>; Pillai, Aurabindo
> <Aurabindo.Pillai at amd.com>; Hung, Alex <Alex.Hung at amd.com>; Mahfooz,
> Hamza <Hamza.Mahfooz at amd.com>; Li, Sun peng (Leo)
> <Sunpeng.Li at amd.com>; Lin, Wayne <Wayne.Lin at amd.com>
> Cc: markyacoub at google.com; Wu, Hersen <hersenxs.wu at amd.com>
> Subject: [PATCH] [i-g-t] tests/amdgpu/amd_ilr: Fix eDP PSR not be
> re-enabled after test
>
> From: Hersen Wu <Hersenxs.Wu at amd.com>
>
> At end of test, with disallow_edp_enter_psr = 0, run DPMS off, on for
> eDP. This will trigger DRM kernel driver enable eDP PSR.
>
> Signed-off-by: Hersen Wu <Hersenxs.Wu at amd.com>
> ---
> lib/igt_amd.c | 5 ---
> tests/amdgpu/amd_ilr.c | 73 ++++++++++++++++++++++++++++++++++---
> -----
> 2 files changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/lib/igt_amd.c b/lib/igt_amd.c index 623883dbc..d10c3c1f2
> 100644
> --- a/lib/igt_amd.c
> +++ b/lib/igt_amd.c
> @@ -1183,11 +1183,6 @@ void igt_amd_disallow_edp_enter_psr(int
> drm_fd, char *connector_name, bool enabl
> const char *allow_edp_psr = "1";
> const char *dis_allow_edp_psr = "0";
>
> - /* if current psr is not enabled, skip this debugfs */
> - if (!igt_amd_psr_support_drv(drm_fd, connector_name,
> PSR_MODE_1) &&
> - !igt_amd_psr_support_drv(drm_fd, connector_name,
> PSR_MODE_2))
> - return;
> -
> fd = igt_debugfs_connector_dir(drm_fd, connector_name,
> O_RDONLY);
> igt_assert(fd >= 0);
> ret = openat(fd, DEBUGFS_DISALLOW_EDP_ENTER_PSR, O_WRONLY); diff
> --git a/tests/amdgpu/amd_ilr.c b/tests/amdgpu/amd_ilr.c index
> 46ad6f60a..f63a4f782 100644
> --- a/tests/amdgpu/amd_ilr.c
> +++ b/tests/amdgpu/amd_ilr.c
> @@ -199,28 +199,33 @@ static void test_flow(data_t *data, enum
> sub_test
> option)
> igt_enable_connectors(data->drm_fd);
>
> for_each_connected_output(&data->display, output) {
> - if (!igt_amd_output_has_ilr_setting(data->drm_fd, output-
> >name) ||
> + if ((output->config.connector->connector_type !=
> DRM_MODE_CONNECTOR_eDP) ||
I think adding this line should be redundant since ilr_setting debugfs entry is limited to be created for edp connector already in amdgpu_dm_debugfs.c
[Hersen] By latest IGT source, writeback connector is set with is_atomic = true before output is built. Within amd_ilr, for_each_connected_output will loop to writeback connector,
The following message will show in IGT log.
output Writeback-1: ilr_setting debugfs not supported
Skipping output: Writeback-1
Amd_ilr test is for eDP. It is better now showing writeback within IGT log.
> + !igt_amd_output_has_ilr_setting(data->drm_fd,
> output->name) ||
> !igt_amd_output_has_link_settings(data->drm_fd,
> output->name)) {
> igt_info("Skipping output: %s\n", output->name);
> continue;
> }
>
> - /* igt_amd_output_has_ilr_setting only checks if debugfs
> - * exist. ilr settings could be all 0s -- not supported.
> - * IGT needs to check if ilr settings values are supported.
> - */
> - igt_amd_read_ilr_setting(data->drm_fd, output->name, data-
> >supported_ilr);
> - if (data->supported_ilr[0] == 0)
> - continue;
> -
> igt_info("Testing on output: %s\n", output->name);
>
> + /* states under /sys/kernel/debug/dri/0/eDP-1:
> + * psr_capability.driver_support (drv_support_psr): yes
> + * ilr_setting (intermediate link rates capabilities,
> + * ilr_cap): yes/no
> + * kernel driver disallow_edp_enter_psr (dis_psr): no
> + */
> +
> /* Init only if display supports ilr link settings */
> test_init(data, output);
>
> + /* eDP enter power saving mode within test_init
> + * drv_support_psr: yes; ilr_cap: no; dis_psr: no
> + */
> +
> /* Disable eDP PSR to avoid timeout when reading CRC */
> igt_amd_disallow_edp_enter_psr(data->drm_fd, output-
> >name, true);
>
> + /* drv_support_psr: yes; ilr_cap: no; dis_psr: yes */
> mode = igt_output_get_mode(output);
> igt_assert(mode);
>
> @@ -229,8 +234,30 @@ static void test_flow(data_t *data, enum sub_test
> option)
> mode->vdisplay,
> DRM_FORMAT_XRGB8888,
> 0, &data->fb);
> igt_plane_set_fb(data->primary, &data->fb);
> +
> + /* drv_support_psr: yes; ilr_cap: no; dis_psr: yes
> + * commit stream. eDP exit power saving mode.
> + */
> igt_display_commit_atomic(&data->display,
> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>
> + /* drv_support_psr: no; ilr_cap: yes; dis_psr: yes.
> + * With dis_psr yes, drm kernel driver
> + * disable psr, psr_en is set to no.
> + */
> +
> + /* igt_amd_output_has_ilr_setting only checks if debugfs
> + * exist. ilr settings could be all 0s -- not supported.
> + * IGT needs to check if ilr settings values are supported.
> + * Supported_ilr is read from DPCD registers. Make sure
> + * eDP exiting power saving mode before reading
> supported_ilr.
> + */
> + igt_amd_read_ilr_setting(data->drm_fd, output->name,
> + data-
> >supported_ilr);
> + if (data->supported_ilr[0] == 0) {
> + /* Enable PSR after reading eDP Rx CRC */
> + igt_amd_disallow_edp_enter_psr(data->drm_fd,
> output->name, false);
Should we also switch dpms off->on to have drv_support_psr become 'yes'?
What about we move this checking just right after test_init() and force DPMS on Before reading ilr capabilities? Then we can return quickly and don't have to touch dis_psr.
[Hersen] Move if (data->supported_ilr[0] == 0) { continue before test_init.
> + continue;
> + }
> +
> /* Collect info of Reported Lane Count & ILR */
> igt_amd_read_link_settings(data->drm_fd, output->name,
> data->lane_count,
> data->link_rate, data-
> >link_spread_spectrum); @@ -247,18 +274,38 @@ static void
> test_flow(data_t *data, enum sub_test option)
> break;
> }
>
> + /* drv_support_psr: no; ilr_cap: yes; dis_psr: yes */
> + kmstest_set_connector_dpms(data->drm_fd,
> + output->config.connector, DRM_MODE_DPMS_OFF);
> +
> + /* eDP enter power saving mode.
> + * drv_support_psr: no; ilr_cap: no; dis_psr: yes.
> + */
> +
> + /* Enable PSR after reading eDP Rx CRC */
> + igt_amd_disallow_edp_enter_psr(data->drm_fd, output-
> >name, false);
> +
> + /* drv_support_psr: no; ilr_cap: yes: dis_psr: no */
> +
> + /* eDP exit power saving mode and setup psr */
> + kmstest_set_connector_dpms(data->drm_fd,
> + output->config.connector, DRM_MODE_DPMS_ON);
> +
> + /* drv_support_psr: yes; ilr_cap: yes: dis_psr: no */
> +
> /* Reset preferred link settings*/
> memset(data->supported_ilr, 0, sizeof(data->supported_ilr));
> igt_amd_write_ilr_setting(data->drm_fd, output->name, 0,
> 0);
>
> + /* drv_support_psr: yes; ilr_cap: yes; dis_psr: no */
> +
> + /* commit 0 stream. eDP enter power saving mode */
> igt_remove_fb(data->drm_fd, &data->fb);
>
> - test_fini(data);
> + /* drv_support_psr: yes; ilr_cap: no; dis_psr: no */
>
> - /* Enable eDP PSR */
> - igt_amd_disallow_edp_enter_psr(data->drm_fd, output-
> >name, false);
> + test_fini(data);
> }
> -
> }
>
> igt_main
> --
> 2.25.1
More information about the igt-dev
mailing list