From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTPS id C2FED1132D0 for ; Tue, 17 May 2022 17:05:08 +0000 (UTC) Date: Tue, 17 May 2022 10:05:07 -0700 Message-ID: <87o7zwf5to.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Anshuman Gupta In-Reply-To: <20220511113734.27776-5-anshuman.gupta@intel.com> References: <20220511113734.27776-1-anshuman.gupta@intel.com> <20220511113734.27776-5-anshuman.gupta@intel.com> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Subject: Re: [igt-dev] [PATCH i-g-t v2 4/4] i915_pm_rpm: rpm resume by user forcewake List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: petri.latvala@intel.com, Chris Wilson , igt-dev@lists.freedesktop.org, badal.nilawar@intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 > Signed-off-by: Anshuman Gupta > --- > 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