From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id AA92A10F02C for ; Wed, 4 May 2022 02:51:25 +0000 (UTC) Date: Tue, 03 May 2022 19:51:24 -0700 Message-ID: <874k26qac3.wl-ashutosh.dixit@intel.com> From: "Dixit, Ashutosh" To: Anshuman Gupta In-Reply-To: <875ymmqer3.wl-ashutosh.dixit@intel.com> References: <20220421170245.11898-1-anshuman.gupta@intel.com> <20220421170245.11898-4-anshuman.gupta@intel.com> <875ymmqer3.wl-ashutosh.dixit@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 3/3] i915_pm_rpm: rpm resume by user forcewake List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org, Chris Wilson , badal.nilawar@intel.com Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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).