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

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

On Tue, 03 May 2022 18:16:00 -0700, Dixit, Ashutosh wrote:
>
> 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?

Or we could do this:

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

Basically, I think either make both enable and disable functions
unconditional, or set/clear forcewake in both only for headless (and name
the functions appropriately).