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

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

On Wed, 11 May 2022 04:37:34 -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
> skip. Let it trigger the rpm resume by taking user
> forcewake.
>
> v2:
> - removed has_runtime_pm cond from
>   enable_one_screen_or_forcewake_and_wait(). [Ashutosh]
> - removed if (ms_data.res) cond from basic_subtest(). [Ashutosh]
> - clear forcewake in both only for headless. [Ashutosh]
>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  tests/i915/i915_pm_rpm.c | 80 ++++++++++++++++++++++++++++++----------
>  1 file changed, 61 insertions(+), 19 deletions(-)
>
> diff --git a/tests/i915/i915_pm_rpm.c b/tests/i915/i915_pm_rpm.c
> index a6035e60b..de71dd41c 100644
> --- a/tests/i915/i915_pm_rpm.c
> +++ b/tests/i915/i915_pm_rpm.c
> @@ -99,6 +99,7 @@ struct mode_set_data {
>	igt_display_t display;
>
>	uint32_t devid;
> +	int fw_fd;
>  };
>
>  /* Stuff we query at different times so we can compare. */
> @@ -369,6 +370,44 @@ static void enable_one_screen(struct mode_set_data *data)
>	igt_assert(wait_for_active()); \
>  } while (0)
>
> +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) {
> +		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)
> +{
> +	if (!default_mode_params)

Why introduce this new unrelated check here? Now someone needs to check if
this check is valid for headless. If we are introducing this here can we
also introduce it in enable_one_screen_or_forcewake_and_wait()?

Can we not just do:

	if (data->fw_fd)
                clear_forcewake(data);

as I was suggesting, or even unconditional is ok since clear_forcewake()
already has the check. Let's just drop the if () here then as you
previously had?

Also I have another comment. The kernel increments/decrements a reference
count when "i915_forcewake_user" debugfs is open/closed. So closing the
file doesn't necessary clear forcewake if a previous fd is open. How about
changing function names with get/put to make this clear. E.g.

* enable_one_screen_or_forcewake_get_and_wait
* forcewake_put instead of clear_forcewake
* disable_all_screens_or_forcewake_put_and_wait

But not strictly needed, you decide, just a suggestion.

Rest everything looks good.

With the if () dropped this is:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>