[PATCH] [i-g-t] tests/amdgpu/amd_ilr: Fix eDP PSR not be re-enabled after test
Lin, Wayne
Wayne.Lin at amd.com
Thu Mar 28 03:58:59 UTC 2024
[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
> + !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.
> + 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