Re: [igt-dev] [PATCH i-g-t 3/3] i915_pm_rpm: rpm resume by user forcewake

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 21 Apr 2022 10:02:45 -0700, Anshuman Gupta wrote:
>
> Few gem rpm tests relies on enabling kms crtc in order to
> trigger rpm resume but on headless platforms these tests
> skips.

skip

> Let it triggered the rpm resume by taking user forcewake.

Let it trigger rpm resume by taking user forcewake.

> +static void
> +enable_one_screen_or_forcewake_and_wait(struct mode_set_data *data)
> +{
> +	bool headless;
> +
> +	/* Try to resume by enabling any type of display */
> +	headless = !enable_one_screen_with_type(data, SCREEN_TYPE_ANY);
> +
> +	/*
> +	 * Get User Forcewake to trigger rpm resume in case of headless
> +	 * as well as no display being connected.
> +	 */
> +	if (headless && has_runtime_pm) {

I think we should remove 'has_runtime_pm' from here, it's
confusing. E.g. what should we do in the '(headless && !has_runtime_pm)'
case? There is already a 'igt_require(has_runtime_pm)' in
setup_environment() triggered from igt_fixture.

Different question: do we need to do this only in headless or can we do
this unconditionally (i.e. get forcewake both with and without display)? If
we can do this unconditionally the function becomes
enable_one_screen_and_forcewake_and_wait() (s/or/and/).

(Note to myself: with this change display tests will use
enable_one_screen_and_wait() and skip when no display and tests which can
run with or without display will use
enable_one_screen_or_forcewake_and_wait(). For display tests we do need to
call enable_one_screen_with_type() to initialize display).

> +		data->fw_fd = igt_open_forcewake_handle(drm_fd);
> +		igt_require(data->fw_fd > 0);
> +	}
> +	igt_assert(wait_for_active());
> +}
> +
> +static void clear_forcewake(struct mode_set_data *data)
> +{
> +	if (data->fw_fd <= 0)
> +		return;
> +
> +	data->fw_fd = close(data->fw_fd);
> +	igt_assert_eq(data->fw_fd, 0);
> +}
> +
> +static void
> +disable_all_screens_or_clr_forcewake_and_wait(struct mode_set_data *data)
> +{
> +	clear_forcewake(data);
> +	disable_all_screens(data);
> +	igt_assert(wait_for_suspended());
> +}
> +

Change function name to disable_all_screens_and_clr_forcewake_and_wait()
(s/or/and/) since we are doing both?

>  static drmModePropertyBlobPtr get_connector_edid(drmModeConnectorPtr connector,
>						 int index)
>  {
> @@ -842,8 +879,10 @@ static void basic_subtest(void)
>  {
>	disable_all_screens_and_wait(&ms_data);
>
> -	if (ms_data.res)
> -		enable_one_screen_and_wait(&ms_data);
> +	if (ms_data.res) {

We shouldn't have this check now since we are now supporting headless?

> +		enable_one_screen_or_forcewake_and_wait(&ms_data);
> +		clear_forcewake(&ms_data);
> +	}

/snip/

> @@ -2195,8 +2237,10 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>		pm_test_caching();
>	}
>
> -	igt_fixture
> +	igt_fixture {
>		teardown_environment(false);
> +		clear_forcewake(&ms_data);

Also looks like we do not need to install an exit_handler (which calls
clear_forcewake()) since just closing the fd disables forcewake (and fd
will be closed on process exit whether in error or not). So that part is ok.

Other tests seem display related but do we also need to change pm_test_tiling()?