Re: [PATCH i-g-t 2/3] lib/igt_psr: add support for PR selective update

["Joshi, Kunal1" <kunal1.joshi@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6816 bytes --]

Hello Jouni,

On 2/18/2024 2:47 PM, Kunal Joshi wrote:
> Extend the tests to cover panel replay selective fetch feature.
>
>  From kms_psr2_sf test point of view we have check_pr_psr2_sel_fetch_support
> function to check if PR/PSR2 selective fetch is supported for an output
> if output supports selective fetch then we check we enter DEEP_SLEEP mode
> in run function
>
> v2: fixed dynamic test name
> v3: use check_psr2_support (Jouni)
> v4: split patches (Jouni)
>
> Cc: Jouni Högander<jouni.hogander@intel.com>
> Cc: Animesh Manna<animesh.manna@intel.com>
> Cc: Arun R Murthy<arun.r.murthy@intel.com>
> Signed-off-by: Kunal Joshi<kunal1.joshi@intel.com>
> ---
>   lib/igt_psr.c             | 36 ++++++++++++++++++++++++++----------
>   lib/igt_psr.h             |  6 +++---
>   tests/kms_cursor_legacy.c |  4 ++--
>   3 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> index cad8cce05..9accd2047 100644
> --- a/lib/igt_psr.c
> +++ b/lib/igt_psr.c
> @@ -41,7 +41,7 @@ bool psr_disabled_check(int debugfs_fd)
>   	return strstr(buf, "PSR mode: disabled\n");
>   }
>   
> -bool psr2_selective_fetch_check(int debugfs_fd, igt_output_t *output)
> +bool selective_fetch_check(int debugfs_fd, igt_output_t *output)
>   {
>   	char buf[PSR_STATUS_MAX_LEN];
>   	char debugfs_file[128] = {0};
> @@ -248,7 +248,9 @@ bool psr_sink_support(int device, int debugfs_fd, enum psr_mode mode, igt_output
>   		       (strstr(line, "PSR = yes") &&
>   		       (strstr(line, "[0x03]") || strstr(line, "[0x04]")));
>   	case PR_MODE:
> -		return strstr(line, "Panel Replay = yes");
> +		return strstr(line, "Panel Replay = yes, Panel Replay Selective Update = no");
> +	case PR_MODE_SEL_FETCH:
> +		return strstr(line, "Panel Replay = yes, Panel Replay Selective Update = yes");
>   	default:
>   		igt_assert_f(false, "Invalid psr mode\n");
>   		return false;
> @@ -317,7 +319,7 @@ bool i915_psr2_selective_fetch_check(int drm_fd, igt_output_t *output)
>   		return false;
>   
>   	debugfs_fd = igt_debugfs_dir(drm_fd);
> -	ret = psr2_selective_fetch_check(debugfs_fd, output);
> +	ret = selective_fetch_check(debugfs_fd, output);
>   	close(debugfs_fd);
>   
>   	return ret;
> @@ -334,17 +336,24 @@ bool i915_psr2_selective_fetch_check(int drm_fd, igt_output_t *output)
>    * Returns:
>    * True if PSR mode changed to PSR1, false otherwise.
>    */
> -bool i915_psr2_sel_fetch_to_psr1(int drm_fd, igt_output_t *output)
> +bool i915_pr_psr2_sel_fetch_to_pr_psr1(int drm_fd, igt_output_t *output)
>   {
>   	int debugfs_fd;
>   	bool ret = false;
> +	enum psr_mode mode;
>   
>   	if (!is_intel_device(drm_fd))
>   		return ret;
>   
>   	debugfs_fd = igt_debugfs_dir(drm_fd);
> -	if (psr2_selective_fetch_check(debugfs_fd, output)) {
> -		psr_set(drm_fd, debugfs_fd, PSR_MODE_1, output);
> +	if (selective_fetch_check(debugfs_fd, output)) {
> +		mode = psr_get_mode(debugfs_fd, output);
> +		if (mode == PR_MODE_SEL_FETCH)
> +			psr_set(drm_fd, debugfs_fd, PR_MODE, output);
> +		else if (mode == PSR_MODE_2_SEL_FETCH)
> +			psr_set(drm_fd, debugfs_fd, PSR_MODE_1, output);
> +		else
> +			igt_assert("switch not possible from current psr mode\n");
>   		ret = true;
>   	}
>   
> @@ -358,12 +367,17 @@ bool i915_psr2_sel_fetch_to_psr1(int drm_fd, igt_output_t *output)
>    * Restore PSR2 selective fetch after tests were executed, this function should
>    * only be called if i915_psr2_sel_fetch_to_psr1() returned true.
>    */
> -void i915_psr2_sel_fetch_restore(int drm_fd, igt_output_t *output)
> +void i915_pr_psr2_sel_fetch_restore(int drm_fd, igt_output_t *output)
>   {
>   	int debugfs_fd;
> +	enum psr_mode mode;
>   
>   	debugfs_fd = igt_debugfs_dir(drm_fd);
> -	psr_set(drm_fd, debugfs_fd, PSR_MODE_2_SEL_FETCH, output);
> +	mode = psr_get_mode(debugfs_fd, output);
> +	if (mode == PR_MODE)
> +		psr_set(drm_fd, debugfs_fd, PR_MODE_SEL_FETCH, output);
> +	else
> +		psr_set(drm_fd, debugfs_fd, PSR_MODE_2_SEL_FETCH, output);
>   	close(debugfs_fd);
>   }

What do you think of this change, do we need to care about PR/PSR2 
Selective fetch or just
write PSR_MODE_2_SEL_FETCH? Thanks and Regards Kunal Joshi

>   
> @@ -389,11 +403,13 @@ enum psr_mode psr_get_mode(int debugfs_fd, igt_output_t *output)
>   
>   	if (strstr(buf, "Panel Replay Enabled"))
>   		return PR_MODE;
> +	else if (strstr(buf, "Panel Replay Selective Update Enabled"))
> +		return PR_MODE_SEL_FETCH;
>   	else if (strstr(buf, "PSR2 selective fetch: enabled"))
>   		return PSR_MODE_2_SEL_FETCH;
> -	else if (strstr(buf, "PSR2 enabled"))
> +	else if (strstr(buf, "PSR2"))
>   		return PSR_MODE_2;
> -	else if (strstr(buf, "PSR1 enabled"))
> +	else if (strstr(buf, "PSR1"))
>   		return PSR_MODE_1;
>   
>   	return PSR_DISABLED;
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> index 372bef2b2..36ba7f068 100644
> --- a/lib/igt_psr.h
> +++ b/lib/igt_psr.h
> @@ -46,7 +46,7 @@ enum fbc_mode {
>   };
>   
>   bool psr_disabled_check(int debugfs_fd);
> -bool psr2_selective_fetch_check(int debugfs_fd, igt_output_t *output);
> +bool selective_fetch_check(int debugfs_fd, igt_output_t *output);
>   bool psr_wait_entry(int debugfs_fd, enum psr_mode mode, igt_output_t *output);
>   bool psr_wait_update(int debugfs_fd, enum psr_mode mode, igt_output_t *output);
>   bool psr_long_wait_update(int debugfs_fd, enum psr_mode mode, igt_output_t *output);
> @@ -59,8 +59,8 @@ enum psr_mode psr_get_mode(int debugfs_fd, igt_output_t *output);
>   
>   bool i915_psr2_selective_fetch_check(int drm_fd, igt_output_t *output);
>   
> -bool i915_psr2_sel_fetch_to_psr1(int drm_fd, igt_output_t *output);
> -void i915_psr2_sel_fetch_restore(int drm_fd, igt_output_t *output);
> +bool i915_pr_psr2_sel_fetch_to_pr_psr1(int drm_fd, igt_output_t *output);
> +void i915_pr_psr2_sel_fetch_restore(int drm_fd, igt_output_t *output);
>   bool is_psr_enable_possible(int drm_fd, enum psr_mode mode);
>   
>   #endif
> diff --git a/tests/kms_cursor_legacy.c b/tests/kms_cursor_legacy.c
> index a430f735a..91e5e9b07 100644
> --- a/tests/kms_cursor_legacy.c
> +++ b/tests/kms_cursor_legacy.c
> @@ -1849,7 +1849,7 @@ igt_main
>   		 * page flip with cursor legacy APIS when Intel's PSR2 selective
>   		 * fetch is enabled, so switching PSR1 for this whole test.
>   		 */
> -		intel_psr2_restore = i915_psr2_sel_fetch_to_psr1(display.drm_fd, NULL);
> +		intel_psr2_restore = i915_pr_psr2_sel_fetch_to_pr_psr1(display.drm_fd, NULL);
>   	}
>   
>   	igt_describe("Test checks how many cursor updates we can fit between vblanks "
> @@ -2074,7 +2074,7 @@ igt_main
>   
>   	igt_fixture {
>   		if (intel_psr2_restore)
> -			i915_psr2_sel_fetch_restore(display.drm_fd, NULL);
> +			i915_pr_psr2_sel_fetch_restore(display.drm_fd, NULL);
>   		igt_display_fini(&display);
>   		drm_close_driver(display.drm_fd);
>   	}

[-- Attachment #2: Type: text/html, Size: 7506 bytes --]