[PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

[Joshi, Kunal1]

Hello Bhanu,

-----Original Message-----
From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com> 
Sent: Wednesday, January 24, 2024 8:13 PM
To: Joshi, Kunal1 <kunal1.joshi at intel.com>; igt-dev at lists.freedesktop.org; B, Jeevan <jeevan.b at intel.com>; Srinivas, Vidya <vidya.srinivas at intel.com>
Subject: Re: [PATCH i-g-t 2/2] tests/intel/kms_psr: enable given psr mode only after checking enable_psr modparam

Hi Kunal,

On 24-01-2024 04:04 pm, Kunal Joshi wrote:
> don't enable a psr mode with psr_debug debugfs, instead check what's 
> the modparam value for i915_enable_psr
> 
> Signed-off-by: Kunal Joshi <kunal1.joshi at intel.com>
> Reviewed-by: Jeevan B <jeevan.b at intel.com>
> Tested-by: Vidya Srinivas <vidya.srinivas at intel.com>
> ---
>   tests/intel/kms_psr.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/tests/intel/kms_psr.c b/tests/intel/kms_psr.c index 
> 349671b00..521d4c708 100644
> --- a/tests/intel/kms_psr.c
> +++ b/tests/intel/kms_psr.c
> @@ -497,16 +497,28 @@ static void fill_render(data_t *data, const 
> struct igt_fb *fb,
>   
>   static bool psr_wait_entry_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);

This change will cause the skips in dynamic subtests. Skipping the dynamic subtest is not recommended, instead don't run the test.

So the proper place for this check would be before calling the igt_dynamic().

- Bhanu

I would advise we change this approach and skip dynamic test
instead of using continue and potentially hiding an issue.

> +
>   	return psr_wait_entry(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_wait_update_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_wait_update(data->debugfs_fd, data->op_psr_mode, data->output);
>   }
>   
>   static bool psr_enable_if_enabled(data_t *data)
>   {
> +	if (!is_psr_enable_possible(data->drm_fd, data->op_psr_mode))
> +		igt_skip("enable_psr modparam doesn't allow psr mode %d\n",
> +			 data->op_psr_mode);
> +
>   	return psr_enable(data->drm_fd, data->debugfs_fd, data->op_psr_mode);
>   }
>
Thanks and Regards
Kunal Joshi